| Message ID | 20220818124646.6005-1-christian@kohlschutter.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] regulator: core: Resolve supply name earlier to prevent double-init | expand |
Hi Christian, On 18.08.2022 14:46, Christian Kohlschütter wrote: > From: Christian Kohlschütter <christian@kohlschutter.com> > > Previously, an unresolved regulator supply reference upon calling > regulator_register on an always-on or boot-on regulator caused > set_machine_constraints to be called twice. > > This in turn may initialize the regulator twice, leading to voltage > glitches that are timing-dependent. A simple, unrelated configuration > change may be enough to hide this problem, only to be surfaced by > chance. > > One such example is the SD-Card voltage regulator in a NanoPI R4S that > would not initialize reliably unless the registration flow was just > complex enough to allow the regulator to properly reset between calls. > > Fix this by re-arranging regulator_register, trying resolve the > regulator's supply early enough that set_machine_constraints does not > need to be called twice. > > Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com> This patch landed recently in linux next as commit 8a866d527ac0 ("regulator: core: Resolve supply name earlier to prevent double-init"). Unfortunately it breaks booting of Samsung Exynos 5800 based Peach-Pi (arch/arm/boot/dts/exynos5800-peach-pi.dts) and Peach-Pit (arch/arm/boot/dts/exynos5420-peach-pit.dts) Chromebooks. The last message in the kernel log is a message about disabling 'vdd_1v2' regulator. This regulator is not used directly, however it is a supply for other critical regulators. In the kernel log I also noticed lots of the following warnings, which were not present before this patch: [ 2.977695] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 2.987715] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 2.997524] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.007560] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.017240] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.026765] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.036501] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.045809] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.055497] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.064765] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.074549] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.085336] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.094815] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.104282] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.113755] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.124446] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.135298] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.144788] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.154562] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! [ 3.164340] debugfs: Directory '(null)-SUPPLY' with parent 'reg-dummy-regulator-dummy' already present! They are printed when the main PMIC is being probed and registered. This shows that the supplies for some regulators are not properly found, probably due to the circular dependencies there ('vdd_1v2' is provided by the same PMIC, which use it as a supply for other regulators). Let me know if I can help debugging this issue further, but for now I would suggest reverting this change. > --- > drivers/regulator/core.c | 52 +++++++++++++++++++++++++--------------- > 1 file changed, 33 insertions(+), 19 deletions(-) > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > index d8373cb04f9..a5033c6ba01 100644 > --- a/drivers/regulator/core.c > +++ b/drivers/regulator/core.c > @@ -5496,7 +5496,39 @@ regulator_register(const struct regulator_desc *regulator_desc, > BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier); > INIT_DELAYED_WORK(&rdev->disable_work, regulator_disable_work); > > - /* preform any regulator specific init */ > + /* set regulator constraints */ > + if (init_data) > + rdev->constraints = kmemdup(&init_data->constraints, > + sizeof(*rdev->constraints), > + GFP_KERNEL); > + else > + rdev->constraints = kzalloc(sizeof(*rdev->constraints), > + GFP_KERNEL); > + if (!rdev->constraints) { > + ret = -ENOMEM; > + goto clean; > + } > + > + if (init_data && init_data->supply_regulator) > + rdev->supply_name = init_data->supply_regulator; > + else if (regulator_desc->supply_name) > + rdev->supply_name = regulator_desc->supply_name; > + > + if ((rdev->supply_name && !rdev->supply) && > + (rdev->constraints->always_on || > + rdev->constraints->boot_on)) { > + /* Try to resolve the name of the supplying regulator here first > + * so we prevent double-initializing the regulator, which may > + * cause timing-specific voltage brownouts/glitches that are > + * hard to debug. > + */ > + ret = regulator_resolve_supply(rdev); > + if (ret) > + rdev_dbg(rdev, "unable to resolve supply early: %pe\n", > + ERR_PTR(ret)); > + } > + > + /* perform any regulator specific init */ > if (init_data && init_data->regulator_init) { > ret = init_data->regulator_init(rdev->reg_data); > if (ret < 0) > @@ -5522,24 +5554,6 @@ regulator_register(const struct regulator_desc *regulator_desc, > (unsigned long) atomic_inc_return(®ulator_no)); > dev_set_drvdata(&rdev->dev, rdev); > > - /* set regulator constraints */ > - if (init_data) > - rdev->constraints = kmemdup(&init_data->constraints, > - sizeof(*rdev->constraints), > - GFP_KERNEL); > - else > - rdev->constraints = kzalloc(sizeof(*rdev->constraints), > - GFP_KERNEL); > - if (!rdev->constraints) { > - ret = -ENOMEM; > - goto wash; > - } > - > - if (init_data && init_data->supply_regulator) > - rdev->supply_name = init_data->supply_regulator; > - else if (regulator_desc->supply_name) > - rdev->supply_name = regulator_desc->supply_name; > - > ret = set_machine_constraints(rdev); > if (ret == -EPROBE_DEFER) { > /* Regulator might be in bypass mode and so needs its supply Best regards
On Thu, Aug 25, 2022 at 01:32:50PM +0200, Marek Szyprowski wrote: > This patch landed recently in linux next as commit 8a866d527ac0 > ("regulator: core: Resolve supply name earlier to prevent double-init"). > Unfortunately it breaks booting of Samsung Exynos 5800 based Peach-Pi > (arch/arm/boot/dts/exynos5800-peach-pi.dts) and Peach-Pit > (arch/arm/boot/dts/exynos5420-peach-pit.dts) Chromebooks. The last > message in the kernel log is a message about disabling 'vdd_1v2' > regulator. This regulator is not used directly, however it is a supply > for other critical regulators. This suggests that supplies are ending up not getting bound. Could you perhaps add logging to check that we're attempting to resolve the supply (in the + if ((rdev->supply_name && !rdev->supply) && + (rdev->constraints->always_on || + rdev->constraints->boot_on)) { block)? I'd also note that it's useful to paste the actual error messages you're seeing rather than just a description of them.
Hi Mark, On 25.08.2022 14:21, Mark Brown wrote: > On Thu, Aug 25, 2022 at 01:32:50PM +0200, Marek Szyprowski wrote: > >> This patch landed recently in linux next as commit 8a866d527ac0 >> ("regulator: core: Resolve supply name earlier to prevent double-init"). >> Unfortunately it breaks booting of Samsung Exynos 5800 based Peach-Pi >> (arch/arm/boot/dts/exynos5800-peach-pi.dts) and Peach-Pit >> (arch/arm/boot/dts/exynos5420-peach-pit.dts) Chromebooks. The last >> message in the kernel log is a message about disabling 'vdd_1v2' >> regulator. This regulator is not used directly, however it is a supply >> for other critical regulators. > This suggests that supplies are ending up not getting bound. Could you > perhaps add logging to check that we're attempting to resolve the supply > (in the > > > + if ((rdev->supply_name && !rdev->supply) && > + (rdev->constraints->always_on || > + rdev->constraints->boot_on)) { > > block)? I've spent a little time debugging this issue and here are my findings. The problem is during the 'vdd_mif' regulator registration. It has one supply called 'inb1' and provided by 'vdd_1v2' regulator. Both 'vdd_mif' and 'vdd_1v2' regulators are provided by the same PMIC. The problem is that 'inb1' supply is being routed to dummy regulator after this change. The regulator_resolve_supply(), which is just after the above mentioned check, returns 0 and bounds 'vdd_mif' supply to dummy-regulator. This happens because regulator_dev_lookup() called in regulator_resolve_supply() returns -19, what in turn lets the code to use dummy-regulator. I didn't check why it doesn't return -EPROBE_DEFER in that case yet. > I'd also note that it's useful to paste the actual error > messages you're seeing rather than just a description of them. There is really nothing more that I can paste here: [ 32.306264] systemd-logind[1375]: New seat seat0. [ 32.331790] systemd-logind[1375]: Watching system buttons on /dev/input/event1 (gpio-keys) [ 32.550686] systemd-logind[1375]: Watching system buttons on /dev/input/event0 (cros_ec) [ 32.570493] systemd-logind[1375]: Failed to start user service, ignoring: Unknown unit: user@0.service [ 32.750913] systemd-logind[1375]: New session c1 of user root. [ 35.070357] vdd_1v2: --- EOF --- Best regards
> On 25. Aug 2022, at 16:23, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Hi Mark, > > On 25.08.2022 14:21, Mark Brown wrote: >> On Thu, Aug 25, 2022 at 01:32:50PM +0200, Marek Szyprowski wrote: >> >>> This patch landed recently in linux next as commit 8a866d527ac0 >>> ("regulator: core: Resolve supply name earlier to prevent double-init"). >>> Unfortunately it breaks booting of Samsung Exynos 5800 based Peach-Pi >>> (arch/arm/boot/dts/exynos5800-peach-pi.dts) and Peach-Pit >>> (arch/arm/boot/dts/exynos5420-peach-pit.dts) Chromebooks. The last >>> message in the kernel log is a message about disabling 'vdd_1v2' >>> regulator. This regulator is not used directly, however it is a supply >>> for other critical regulators. >> This suggests that supplies are ending up not getting bound. Could you >> perhaps add logging to check that we're attempting to resolve the supply >> (in the >> >> >> + if ((rdev->supply_name && !rdev->supply) && >> + (rdev->constraints->always_on || >> + rdev->constraints->boot_on)) { >> >> block)? > > > I've spent a little time debugging this issue and here are my findings. > The problem is during the 'vdd_mif' regulator registration. It has one > supply called 'inb1' and provided by 'vdd_1v2' regulator. Both 'vdd_mif' > and 'vdd_1v2' regulators are provided by the same PMIC. > > The problem is that 'inb1' supply is being routed to dummy regulator > after this change. The regulator_resolve_supply(), which is just after > the above mentioned check, returns 0 and bounds 'vdd_mif' supply to > dummy-regulator. This happens because regulator_dev_lookup() called in > regulator_resolve_supply() returns -19, what in turn lets the code to > use dummy-regulator. I didn't check why it doesn't return -EPROBE_DEFER > in that case yet. > >> I'd also note that it's useful to paste the actual error >> messages you're seeing rather than just a description of them. > > There is really nothing more that I can paste here: > > [ 32.306264] systemd-logind[1375]: New seat seat0. > [ 32.331790] systemd-logind[1375]: Watching system buttons on > /dev/input/event1 (gpio-keys) > [ 32.550686] systemd-logind[1375]: Watching system buttons on > /dev/input/event0 (cros_ec) > [ 32.570493] systemd-logind[1375]: Failed to start user service, > ignoring: Unknown unit: user@0.service > [ 32.750913] systemd-logind[1375]: New session c1 of user root. > [ 35.070357] vdd_1v2: > > --- EOF --- > I can reproduce these findings (also see the difference in "cat /sys/kernel/debug/regulator/regulator_summary") The check "if (have_full_constraints())" in "regulator_resolve_supply" is called even though regulator_dev_lookup returned -ENODEV (-19). Since my patch now calls "regulator_resolve_supply" twice, the first round should really treat ENODEV as "defer". I propose adding a boolean defer argument to regulator_resolve_supply (with defer=true in the first, opportunistic run, and false in any other situation). I'll have a patch ready later tonight. Thanks! Christian
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index d8373cb04f9..a5033c6ba01 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -5496,7 +5496,39 @@ regulator_register(const struct regulator_desc *regulator_desc, BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier); INIT_DELAYED_WORK(&rdev->disable_work, regulator_disable_work); - /* preform any regulator specific init */ + /* set regulator constraints */ + if (init_data) + rdev->constraints = kmemdup(&init_data->constraints, + sizeof(*rdev->constraints), + GFP_KERNEL); + else + rdev->constraints = kzalloc(sizeof(*rdev->constraints), + GFP_KERNEL); + if (!rdev->constraints) { + ret = -ENOMEM; + goto clean; + } + + if (init_data && init_data->supply_regulator) + rdev->supply_name = init_data->supply_regulator; + else if (regulator_desc->supply_name) + rdev->supply_name = regulator_desc->supply_name; + + if ((rdev->supply_name && !rdev->supply) && + (rdev->constraints->always_on || + rdev->constraints->boot_on)) { + /* Try to resolve the name of the supplying regulator here first + * so we prevent double-initializing the regulator, which may + * cause timing-specific voltage brownouts/glitches that are + * hard to debug. + */ + ret = regulator_resolve_supply(rdev); + if (ret) + rdev_dbg(rdev, "unable to resolve supply early: %pe\n", + ERR_PTR(ret)); + } + + /* perform any regulator specific init */ if (init_data && init_data->regulator_init) { ret = init_data->regulator_init(rdev->reg_data); if (ret < 0) @@ -5522,24 +5554,6 @@ regulator_register(const struct regulator_desc *regulator_desc, (unsigned long) atomic_inc_return(®ulator_no)); dev_set_drvdata(&rdev->dev, rdev); - /* set regulator constraints */ - if (init_data) - rdev->constraints = kmemdup(&init_data->constraints, - sizeof(*rdev->constraints), - GFP_KERNEL); - else - rdev->constraints = kzalloc(sizeof(*rdev->constraints), - GFP_KERNEL); - if (!rdev->constraints) { - ret = -ENOMEM; - goto wash; - } - - if (init_data && init_data->supply_regulator) - rdev->supply_name = init_data->supply_regulator; - else if (regulator_desc->supply_name) - rdev->supply_name = regulator_desc->supply_name; - ret = set_machine_constraints(rdev); if (ret == -EPROBE_DEFER) { /* Regulator might be in bypass mode and so needs its supply