| Message ID | 20210817033848.1396749-4-jay.xu@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | regulator pre-enable | expand |
On Tue, Aug 17, 2021 at 11:38:48AM +0800, Jianqun Xu wrote: > + } else if (event & REGULATOR_EVENT_ENABLE) { > + uV = regulator_get_voltage(supply->reg); > } else { I am very surprised this doesn't cause locking issues given that we might call notifiers with the regulator API's locks held. Have you tested this with lockdep on?
Hi Mark -------------- jay.xu@rock-chips.com >On Tue, Aug 17, 2021 at 11:38:48AM +0800, Jianqun Xu wrote: > >> + } else if (event & REGULATOR_EVENT_ENABLE) { >> + uV = regulator_get_voltage(supply->reg); >> } else { > >I am very surprised this doesn't cause locking issues given that we >might call notifiers with the regulator API's locks held. Have you >tested this with lockdep on? Thanks for your reply, there really has locking issue, our test team fail to find out it but get a pass result. So, if the voltage cannot get here by driver, can the notify pass it, like pre-version patch?
Hi Mark -------------- jay.xu@rock-chips.com >Hi Mark >-------------- >jay.xu@rock-chips.com >>On Tue, Aug 17, 2021 at 11:38:48AM +0800, Jianqun Xu wrote: >> >>> + } else if (event & REGULATOR_EVENT_ENABLE) { >>> + uV = regulator_get_voltage(supply->reg); >>> } else { >> >>I am very surprised this doesn't cause locking issues given that we >>might call notifiers with the regulator API's locks held. Have you >>tested this with lockdep on? >Thanks for your reply, there really has locking issue, our test team fail to find out it >but get a pass result. > >So, if the voltage cannot get here by driver, can the notify pass it, like pre-version patch? I think it's possilbe to store the voltage in driver side, and do the io-domain config after EVENT_ENABLE notify witout voltage, I will push it to io-domain maintainer to review, can I add yours' review in that patch ?
diff --git a/drivers/soc/rockchip/io-domain.c b/drivers/soc/rockchip/io-domain.c index cf8182fc3642..3c59077fafb1 100644 --- a/drivers/soc/rockchip/io-domain.c +++ b/drivers/soc/rockchip/io-domain.c @@ -123,6 +123,12 @@ static int rockchip_iodomain_notify(struct notifier_block *nb, } else if (event & (REGULATOR_EVENT_VOLTAGE_CHANGE | REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE)) { uV = (unsigned long)data; + } else if (event & REGULATOR_EVENT_PRE_ENABLE) { + uV = MAX_VOLTAGE_3_3; + } else if (event & REGULATOR_EVENT_PRE_DISABLE) { + uV = MAX_VOLTAGE_3_3; + } else if (event & REGULATOR_EVENT_ENABLE) { + uV = regulator_get_voltage(supply->reg); } else { return NOTIFY_OK; }
Do a fix to rockchip io-domain, follow this orders: * system running state -> io-domain vsel to 3.3V -> regulator_enable -> vsel change according to regulator voltage * system running state -> io-domain vsel to 3.3V -> regulator_disable Found on some Rockchip SoCs, the regulator enable or disable without care about the io-domain maybe caused soc damaged. Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com> --- v2: - use "uV = regulator_get_voltage(supply->reg);" but from notify data v1: - first version drivers/soc/rockchip/io-domain.c | 6 ++++++ 1 file changed, 6 insertions(+)