* Re: [PATCH v2] regulator: core: Resolve supply name earlier to prevent double-init @ 2022-08-03 14:33 Vincent Legoll 2022-08-04 10:44 ` [PATCH v3] " Christian Kohlschütter 0 siblings, 1 reply; 18+ messages in thread From: Vincent Legoll @ 2022-08-03 14:33 UTC (permalink / raw) To: christian Cc: broonie, heiko, lgirdwood, linux-arm-kernel, linux-kernel, linux-mmc, linux-rockchip, m.reichl, robin.murphy, wens Hello, it looks like you left a test for '&& rdev->constraints' in the if after you moved the 'if (!rdev->constraints) {' above it. -- Vincent Legoll ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3] regulator: core: Resolve supply name earlier to prevent double-init 2022-08-03 14:33 [PATCH v2] regulator: core: Resolve supply name earlier to prevent double-init Vincent Legoll @ 2022-08-04 10:44 ` Christian Kohlschütter 2022-08-15 11:17 ` Mark Brown 0 siblings, 1 reply; 18+ messages in thread From: Christian Kohlschütter @ 2022-08-04 10:44 UTC (permalink / raw) To: Vincent Legoll, Mark Brown Cc: Heiko Stübner, Liam Girdwood, linux-arm-kernel, linux-kernel, Linux MMC List, open list:ARM/Rockchip SoC..., Markus Reichl, Robin Murphy, wens 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=C3=BCtter <christian@kohlschutter.com> --- drivers/regulator/core.c | 51 +++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 398c8d6afd4..5c2b97ea633 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -5492,7 +5492,38 @@ regulator_register(const struct regulator_desc = *regulator_desc, BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier); INIT_DELAYED_WORK(&rdev->disable_work, regulator_disable_work); =20 - /* preform any regulator specific init */ + /* set regulator constraints */ + if (init_data) + rdev->constraints =3D kmemdup(&init_data->constraints, + sizeof(*rdev->constraints), + GFP_KERNEL); + else + rdev->constraints =3D = kzalloc(sizeof(*rdev->constraints), + GFP_KERNEL); + if (!rdev->constraints) { + ret =3D -ENOMEM; + goto clean; + } + + if (init_data && init_data->supply_regulator) + rdev->supply_name =3D init_data->supply_regulator; + else if (regulator_desc->supply_name) + rdev->supply_name =3D regulator_desc->supply_name; + + if ((rdev->supply_name && !rdev->supply) && rdev->constraints + && (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 =3D 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 =3D init_data->regulator_init(rdev->reg_data); if (ret < 0) @@ -5518,24 +5549,6 @@ regulator_register(const struct regulator_desc = *regulator_desc, (unsigned long) atomic_inc_return(®ulator_no)); dev_set_drvdata(&rdev->dev, rdev); =20 - /* set regulator constraints */ - if (init_data) - rdev->constraints =3D kmemdup(&init_data->constraints, - sizeof(*rdev->constraints), - GFP_KERNEL); - else - rdev->constraints =3D = kzalloc(sizeof(*rdev->constraints), - GFP_KERNEL); - if (!rdev->constraints) { - ret =3D -ENOMEM; - goto wash; - } - - if (init_data && init_data->supply_regulator) - rdev->supply_name =3D init_data->supply_regulator; - else if (regulator_desc->supply_name) - rdev->supply_name =3D regulator_desc->supply_name; - ret =3D set_machine_constraints(rdev); if (ret =3D=3D -EPROBE_DEFER) { /* Regulator might be in bypass mode and so needs its = supply --=20 2.36.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3] regulator: core: Resolve supply name earlier to prevent double-init 2022-08-04 10:44 ` [PATCH v3] " Christian Kohlschütter @ 2022-08-15 11:17 ` Mark Brown 2022-08-18 12:46 ` [PATCH v4] " Christian Kohlschütter 0 siblings, 1 reply; 18+ messages in thread From: Mark Brown @ 2022-08-15 11:17 UTC (permalink / raw) To: Christian Kohlschütter Cc: Vincent Legoll, Heiko Stübner, Liam Girdwood, linux-arm-kernel, linux-kernel, Linux MMC List, open list:ARM/Rockchip SoC..., Markus Reichl, Robin Murphy, wens [-- Attachment #1: Type: text/plain, Size: 324 bytes --] On Thu, Aug 04, 2022 at 12:44:29PM +0200, Christian Kohlschütter wrote: > 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 doesn't apply against current code, please check and resend. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4] regulator: core: Resolve supply name earlier to prevent double-init 2022-08-15 11:17 ` Mark Brown @ 2022-08-18 12:46 ` Christian Kohlschütter [not found] ` <CGME20220825113251eucas1p247c3d57de823da148ca4790975a4aba8@eucas1p2.samsung.com> 0 siblings, 1 reply; 18+ messages in thread From: Christian Kohlschütter @ 2022-08-18 12:46 UTC (permalink / raw) To: broonie Cc: christian, heiko, lgirdwood, linux-arm-kernel, linux-kernel, linux-mmc, linux-rockchip, m.reichl, robin.murphy, vincent.legoll, wens 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> --- 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 -- 2.36.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
[parent not found: <CGME20220825113251eucas1p247c3d57de823da148ca4790975a4aba8@eucas1p2.samsung.com>]
* Re: [PATCH v4] regulator: core: Resolve supply name earlier to prevent double-init [not found] ` <CGME20220825113251eucas1p247c3d57de823da148ca4790975a4aba8@eucas1p2.samsung.com> @ 2022-08-25 11:32 ` Marek Szyprowski 2022-08-25 12:21 ` Mark Brown 0 siblings, 1 reply; 18+ messages in thread From: Marek Szyprowski @ 2022-08-25 11:32 UTC (permalink / raw) To: Christian Kohlschütter, broonie Cc: heiko, lgirdwood, linux-arm-kernel, linux-kernel, linux-mmc, linux-rockchip, m.reichl, robin.murphy, vincent.legoll, wens 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 -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4] regulator: core: Resolve supply name earlier to prevent double-init 2022-08-25 11:32 ` Marek Szyprowski @ 2022-08-25 12:21 ` Mark Brown 2022-08-25 14:23 ` Marek Szyprowski 0 siblings, 1 reply; 18+ messages in thread From: Mark Brown @ 2022-08-25 12:21 UTC (permalink / raw) To: Marek Szyprowski Cc: Christian Kohlschütter, heiko, lgirdwood, linux-arm-kernel, linux-kernel, linux-mmc, linux-rockchip, m.reichl, robin.murphy, vincent.legoll, wens [-- Attachment #1: Type: text/plain, Size: 1062 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4] regulator: core: Resolve supply name earlier to prevent double-init 2022-08-25 12:21 ` Mark Brown @ 2022-08-25 14:23 ` Marek Szyprowski 2022-08-25 15:18 ` Christian Kohlschütter 0 siblings, 1 reply; 18+ messages in thread From: Marek Szyprowski @ 2022-08-25 14:23 UTC (permalink / raw) To: Mark Brown Cc: Christian Kohlschütter, heiko, lgirdwood, linux-arm-kernel, linux-kernel, linux-mmc, linux-rockchip, m.reichl, robin.murphy, vincent.legoll, wens 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 -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4] regulator: core: Resolve supply name earlier to prevent double-init 2022-08-25 14:23 ` Marek Szyprowski @ 2022-08-25 15:18 ` Christian Kohlschütter 2022-08-25 21:28 ` [PATCH v5] " Christian Kohlschütter 0 siblings, 1 reply; 18+ messages in thread From: Christian Kohlschütter @ 2022-08-25 15:18 UTC (permalink / raw) To: Marek Szyprowski, Mark Brown Cc: Heiko Stübner, Liam Girdwood, linux-arm-kernel, linux-kernel, Linux MMC List, open list:ARM/Rockchip SoC..., Markus Reichl, Robin Murphy, Vincent Legoll, wens > 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init 2022-08-25 15:18 ` Christian Kohlschütter @ 2022-08-25 21:28 ` Christian Kohlschütter 2022-08-25 21:35 ` Christian Kohlschütter ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Christian Kohlschütter @ 2022-08-25 21:28 UTC (permalink / raw) To: broonie, m.szyprowski Cc: christian, heiko, lgirdwood, linux-arm-kernel, linux-kernel, linux-mmc, linux-rockchip, m.reichl, robin.murphy, vincent.legoll, wens 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> --- drivers/regulator/core.c | 58 ++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 77f60eef960..2ff0ab2730f 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -5391,6 +5391,7 @@ regulator_register(const struct regulator_desc *regulator_desc, bool dangling_of_gpiod = false; struct device *dev; int ret, i; + bool resolved_early = false; if (cfg == NULL) return ERR_PTR(-EINVAL); @@ -5494,24 +5495,10 @@ 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 */ - if (init_data && init_data->regulator_init) { - ret = init_data->regulator_init(rdev->reg_data); - if (ret < 0) - goto clean; - } - - if (config->ena_gpiod) { - ret = regulator_ena_gpio_request(rdev, config); - if (ret != 0) { - rdev_err(rdev, "Failed to request enable GPIO: %pe\n", - ERR_PTR(ret)); - goto clean; - } - /* The regulator core took over the GPIO descriptor */ - dangling_cfg_gpiod = false; - dangling_of_gpiod = false; - } + 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; /* register with sysfs */ rdev->dev.class = ®ulator_class; @@ -5533,13 +5520,38 @@ regulator_register(const struct regulator_desc *regulator_desc, 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; + if ((rdev->supply_name && !rdev->supply) && + (rdev->constraints->always_on || + rdev->constraints->boot_on)) { + ret = regulator_resolve_supply(rdev); + if (ret != 0) + rdev_dbg(rdev, "Unable to resolve supply early: %pe\n", + ERR_PTR(ret)); + + resolved_early = true; + } + + /* perform any regulator specific init */ + if (init_data && init_data->regulator_init) { + ret = init_data->regulator_init(rdev->reg_data); + if (ret < 0) + goto wash; + } + + if (config->ena_gpiod) { + ret = regulator_ena_gpio_request(rdev, config); + if (ret != 0) { + rdev_err(rdev, "Failed to request enable GPIO: %pe\n", + ERR_PTR(ret)); + goto wash; + } + /* The regulator core took over the GPIO descriptor */ + dangling_cfg_gpiod = false; + dangling_of_gpiod = false; + } ret = set_machine_constraints(rdev); - if (ret == -EPROBE_DEFER) { + if (ret == -EPROBE_DEFER && !resolved_early) { /* Regulator might be in bypass mode and so needs its supply * to set the constraints */ -- 2.36.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init 2022-08-25 21:28 ` [PATCH v5] " Christian Kohlschütter @ 2022-08-25 21:35 ` Christian Kohlschütter 2022-08-26 5:55 ` Marek Szyprowski ` (2 subsequent siblings) 3 siblings, 0 replies; 18+ messages in thread From: Christian Kohlschütter @ 2022-08-25 21:35 UTC (permalink / raw) To: Mark Brown, Marek Szyprowski Cc: Heiko Stübner, Liam Girdwood, linux-arm-kernel, linux-kernel, Linux MMC List, open list:ARM/Rockchip SoC..., Markus Reichl, Robin Murphy, Vincent Legoll, wens This needed some further rearrangement. Please validate v5 of the patch. Any branch that has the v4 patch should revert that change and freshly re-apply the v5 patch. I hope this simplifies merging into mainline. Verified by 1. rebooting numerous times (no mmc errors, partitions on mmc SD card are properly detected, no hangs upon reboot, i.e., the change appears to work) 2. "cat /sys/kernel/debug/regulator/regulator_summary" now correctly shows regulators, regulator-dummy use count is 1 3. ensuring that no entries for "(null)-SUPPLY' were found in dmesg or /sys Thanks, Christian ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init 2022-08-25 21:28 ` [PATCH v5] " Christian Kohlschütter 2022-08-25 21:35 ` Christian Kohlschütter @ 2022-08-26 5:55 ` Marek Szyprowski 2022-08-29 15:43 ` Mark Brown 2023-02-17 23:22 ` Saravana Kannan 3 siblings, 0 replies; 18+ messages in thread From: Marek Szyprowski @ 2022-08-26 5:55 UTC (permalink / raw) To: Christian Kohlschütter, broonie Cc: heiko, lgirdwood, linux-arm-kernel, linux-kernel, linux-mmc, linux-rockchip, m.reichl, robin.murphy, vincent.legoll, wens On 25.08.2022 23:28, Christian Kohlschütter wrote: > 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> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/regulator/core.c | 58 ++++++++++++++++++++++++---------------- > 1 file changed, 35 insertions(+), 23 deletions(-) > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > index 77f60eef960..2ff0ab2730f 100644 > --- a/drivers/regulator/core.c > +++ b/drivers/regulator/core.c > @@ -5391,6 +5391,7 @@ regulator_register(const struct regulator_desc *regulator_desc, > bool dangling_of_gpiod = false; > struct device *dev; > int ret, i; > + bool resolved_early = false; > > if (cfg == NULL) > return ERR_PTR(-EINVAL); > @@ -5494,24 +5495,10 @@ 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 */ > - if (init_data && init_data->regulator_init) { > - ret = init_data->regulator_init(rdev->reg_data); > - if (ret < 0) > - goto clean; > - } > - > - if (config->ena_gpiod) { > - ret = regulator_ena_gpio_request(rdev, config); > - if (ret != 0) { > - rdev_err(rdev, "Failed to request enable GPIO: %pe\n", > - ERR_PTR(ret)); > - goto clean; > - } > - /* The regulator core took over the GPIO descriptor */ > - dangling_cfg_gpiod = false; > - dangling_of_gpiod = false; > - } > + 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; > > /* register with sysfs */ > rdev->dev.class = ®ulator_class; > @@ -5533,13 +5520,38 @@ regulator_register(const struct regulator_desc *regulator_desc, > 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; > + if ((rdev->supply_name && !rdev->supply) && > + (rdev->constraints->always_on || > + rdev->constraints->boot_on)) { > + ret = regulator_resolve_supply(rdev); > + if (ret != 0) > + rdev_dbg(rdev, "Unable to resolve supply early: %pe\n", > + ERR_PTR(ret)); > + > + resolved_early = true; > + } > + > + /* perform any regulator specific init */ > + if (init_data && init_data->regulator_init) { > + ret = init_data->regulator_init(rdev->reg_data); > + if (ret < 0) > + goto wash; > + } > + > + if (config->ena_gpiod) { > + ret = regulator_ena_gpio_request(rdev, config); > + if (ret != 0) { > + rdev_err(rdev, "Failed to request enable GPIO: %pe\n", > + ERR_PTR(ret)); > + goto wash; > + } > + /* The regulator core took over the GPIO descriptor */ > + dangling_cfg_gpiod = false; > + dangling_of_gpiod = false; > + } > > ret = set_machine_constraints(rdev); > - if (ret == -EPROBE_DEFER) { > + if (ret == -EPROBE_DEFER && !resolved_early) { > /* Regulator might be in bypass mode and so needs its supply > * to set the constraints > */ Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init 2022-08-25 21:28 ` [PATCH v5] " Christian Kohlschütter 2022-08-25 21:35 ` Christian Kohlschütter 2022-08-26 5:55 ` Marek Szyprowski @ 2022-08-29 15:43 ` Mark Brown 2022-08-29 17:01 ` Christian Kohlschütter 2023-02-17 23:22 ` Saravana Kannan 3 siblings, 1 reply; 18+ messages in thread From: Mark Brown @ 2022-08-29 15:43 UTC (permalink / raw) To: Christian Kohlschütter Cc: m.szyprowski, heiko, lgirdwood, linux-arm-kernel, linux-kernel, linux-mmc, linux-rockchip, m.reichl, robin.murphy, vincent.legoll, wens [-- Attachment #1: Type: text/plain, Size: 850 bytes --] On Thu, Aug 25, 2022 at 09:28:42PM +0000, Christian Kohlschütter wrote: > 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. Please do not submit new versions of already applied patches, please submit incremental updates to the existing code. Modifying existing commits creates problems for other users building on top of those commits so it's best practice to only change pubished git commits if absolutely essential. Please don't send new patches in reply to old patches or serieses, this makes it harder for both people and tools to understand what is going on - it can bury things in mailboxes and make it difficult to keep track of what current patches are, both for the new patches and the old ones. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init 2022-08-29 15:43 ` Mark Brown @ 2022-08-29 17:01 ` Christian Kohlschütter 0 siblings, 0 replies; 18+ messages in thread From: Christian Kohlschütter @ 2022-08-29 17:01 UTC (permalink / raw) To: Mark Brown, Marek Szyprowski Cc: Heiko Stübner, Liam Girdwood, linux-arm-kernel, linux-kernel, Linux MMC List, open list:ARM/Rockchip SoC..., Markus Reichl, Robin Murphy, Vincent Legoll, wens > On 29. Aug 2022, at 17:43, Mark Brown <broonie@kernel.org> wrote: > > On Thu, Aug 25, 2022 at 09:28:42PM +0000, Christian Kohlschütter wrote: >> 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. > > Please do not submit new versions of already applied patches, please > submit incremental updates to the existing code. Modifying existing > commits creates problems for other users building on top of those > commits so it's best practice to only change pubished git commits if > absolutely essential. > > Please don't send new patches in reply to old patches or serieses, this > makes it harder for both people and tools to understand what is going > on - it can bury things in mailboxes and make it difficult to keep track > of what current patches are, both for the new patches and the old ones. My apologies, I wasn't aware that's not the preferred way forward. Following up with "regulator: core: Fix regulator supply registration with sysfs", see https://lore.kernel.org/all/20220829165543.24856-1-christian@kohlschutter.com/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init 2022-08-25 21:28 ` [PATCH v5] " Christian Kohlschütter ` (2 preceding siblings ...) 2022-08-29 15:43 ` Mark Brown @ 2023-02-17 23:22 ` Saravana Kannan 2023-02-17 23:33 ` Christian Kohlschütter 3 siblings, 1 reply; 18+ messages in thread From: Saravana Kannan @ 2023-02-17 23:22 UTC (permalink / raw) To: Christian Kohlschütter Cc: broonie, m.szyprowski, heiko, lgirdwood, linux-arm-kernel, linux-kernel, linux-mmc, linux-rockchip, m.reichl, robin.murphy, vincent.legoll, wens On Thu, Aug 25, 2022 at 2:28 PM Christian Kohlschütter <christian@kohlschutter.com> wrote: > > 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. In your case, can you elaborate which part of the constraints/init twice caused the issue? I'm trying to simplify some of the supply resolving code and I'm trying to not break your use case. -Saravana > > 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> > --- > drivers/regulator/core.c | 58 ++++++++++++++++++++++++---------------- > 1 file changed, 35 insertions(+), 23 deletions(-) > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > index 77f60eef960..2ff0ab2730f 100644 > --- a/drivers/regulator/core.c > +++ b/drivers/regulator/core.c > @@ -5391,6 +5391,7 @@ regulator_register(const struct regulator_desc *regulator_desc, > bool dangling_of_gpiod = false; > struct device *dev; > int ret, i; > + bool resolved_early = false; > > if (cfg == NULL) > return ERR_PTR(-EINVAL); > @@ -5494,24 +5495,10 @@ 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 */ > - if (init_data && init_data->regulator_init) { > - ret = init_data->regulator_init(rdev->reg_data); > - if (ret < 0) > - goto clean; > - } > - > - if (config->ena_gpiod) { > - ret = regulator_ena_gpio_request(rdev, config); > - if (ret != 0) { > - rdev_err(rdev, "Failed to request enable GPIO: %pe\n", > - ERR_PTR(ret)); > - goto clean; > - } > - /* The regulator core took over the GPIO descriptor */ > - dangling_cfg_gpiod = false; > - dangling_of_gpiod = false; > - } > + 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; > > /* register with sysfs */ > rdev->dev.class = ®ulator_class; > @@ -5533,13 +5520,38 @@ regulator_register(const struct regulator_desc *regulator_desc, > 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; > + if ((rdev->supply_name && !rdev->supply) && > + (rdev->constraints->always_on || > + rdev->constraints->boot_on)) { > + ret = regulator_resolve_supply(rdev); > + if (ret != 0) > + rdev_dbg(rdev, "Unable to resolve supply early: %pe\n", > + ERR_PTR(ret)); > + > + resolved_early = true; > + } > + > + /* perform any regulator specific init */ > + if (init_data && init_data->regulator_init) { > + ret = init_data->regulator_init(rdev->reg_data); > + if (ret < 0) > + goto wash; > + } > + > + if (config->ena_gpiod) { > + ret = regulator_ena_gpio_request(rdev, config); > + if (ret != 0) { > + rdev_err(rdev, "Failed to request enable GPIO: %pe\n", > + ERR_PTR(ret)); > + goto wash; > + } > + /* The regulator core took over the GPIO descriptor */ > + dangling_cfg_gpiod = false; > + dangling_of_gpiod = false; > + } > > ret = set_machine_constraints(rdev); > - if (ret == -EPROBE_DEFER) { > + if (ret == -EPROBE_DEFER && !resolved_early) { > /* Regulator might be in bypass mode and so needs its supply > * to set the constraints > */ > -- > 2.36.2 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init 2023-02-17 23:22 ` Saravana Kannan @ 2023-02-17 23:33 ` Christian Kohlschütter 2023-02-17 23:46 ` Saravana Kannan 0 siblings, 1 reply; 18+ messages in thread From: Christian Kohlschütter @ 2023-02-17 23:33 UTC (permalink / raw) To: Saravana Kannan Cc: Mark Brown, Marek Szyprowski, Heiko Stübner, lgirdwood, linux-arm-kernel, linux-kernel, linux-mmc, linux-rockchip, m.reichl, robin.murphy, vincent.legoll, wens On 18. Feb 2023, at 00:22, Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Aug 25, 2022 at 2:28 PM Christian Kohlschütter > <christian@kohlschutter.com> wrote: >> >> 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. > > In your case, can you elaborate which part of the constraints/init > twice caused the issue? > > I'm trying to simplify some of the supply resolving code and I'm > trying to not break your use case. > > -Saravana Here's a write-up of my use case, and how we got to the solution: https://kohlschuetter.github.io/blog/posts/2022/10/28/linux-nanopi-r4s/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init 2023-02-17 23:33 ` Christian Kohlschütter @ 2023-02-17 23:46 ` Saravana Kannan 2023-02-18 0:01 ` Christian Kohlschütter 0 siblings, 1 reply; 18+ messages in thread From: Saravana Kannan @ 2023-02-17 23:46 UTC (permalink / raw) To: Christian Kohlschütter Cc: Mark Brown, Marek Szyprowski, Heiko Stübner, lgirdwood, linux-arm-kernel, linux-kernel, linux-mmc, linux-rockchip, m.reichl, robin.murphy, vincent.legoll, wens On Fri, Feb 17, 2023 at 3:33 PM Christian Kohlschütter <christian@kohlschutter.com> wrote: > > On 18. Feb 2023, at 00:22, Saravana Kannan <saravanak@google.com> wrote: > > > > On Thu, Aug 25, 2022 at 2:28 PM Christian Kohlschütter > > <christian@kohlschutter.com> wrote: > >> > >> 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. > > > > In your case, can you elaborate which part of the constraints/init > > twice caused the issue? > > > > I'm trying to simplify some of the supply resolving code and I'm > > trying to not break your use case. > > > > -Saravana > > Here's a write-up of my use case, and how we got to the solution: > https://kohlschuetter.github.io/blog/posts/2022/10/28/linux-nanopi-r4s/ I did read the write up before I sent my request. I'm asking for specifics on which functions in the set_machine_constraints() was causing the issue. And it's also a bit unclear to me if the issue was with having stuff called twice on the alway-on regulator or the supply. -Saravana ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init 2023-02-17 23:46 ` Saravana Kannan @ 2023-02-18 0:01 ` Christian Kohlschütter 2023-02-18 0:05 ` Saravana Kannan 0 siblings, 1 reply; 18+ messages in thread From: Christian Kohlschütter @ 2023-02-18 0:01 UTC (permalink / raw) To: Saravana Kannan Cc: Mark Brown, Marek Szyprowski, Heiko Stübner, lgirdwood, linux-arm-kernel, linux-kernel, linux-mmc, linux-rockchip, m.reichl, robin.murphy, vincent.legoll, wens On 18. Feb 2023, at 00:46, Saravana Kannan <saravanak@google.com> wrote: > > On Fri, Feb 17, 2023 at 3:33 PM Christian Kohlschütter > <christian@kohlschutter.com> wrote: >> >> On 18. Feb 2023, at 00:22, Saravana Kannan <saravanak@google.com> wrote: >>> >>> On Thu, Aug 25, 2022 at 2:28 PM Christian Kohlschütter >>> <christian@kohlschutter.com> wrote: >>>> >>>> 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. >>> >>> In your case, can you elaborate which part of the constraints/init >>> twice caused the issue? >>> >>> I'm trying to simplify some of the supply resolving code and I'm >>> trying to not break your use case. >>> >>> -Saravana >> >> Here's a write-up of my use case, and how we got to the solution: >> https://kohlschuetter.github.io/blog/posts/2022/10/28/linux-nanopi-r4s/ > > I did read the write up before I sent my request. I'm asking for > specifics on which functions in the set_machine_constraints() was > causing the issue. And it's also a bit unclear to me if the issue was > with having stuff called twice on the alway-on regulator or the > supply. > > -Saravana I'm afraid I cannot give a more detailed answer than what's in the write up and the previous discussion on this mailing list; I thought it's pretty detailed already. However, it should be relatively straightforward to reproduce the issue. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init 2023-02-18 0:01 ` Christian Kohlschütter @ 2023-02-18 0:05 ` Saravana Kannan 0 siblings, 0 replies; 18+ messages in thread From: Saravana Kannan @ 2023-02-18 0:05 UTC (permalink / raw) To: Christian Kohlschütter Cc: Mark Brown, Marek Szyprowski, Heiko Stübner, lgirdwood, linux-arm-kernel, linux-kernel, linux-mmc, linux-rockchip, m.reichl, robin.murphy, vincent.legoll, wens On Fri, Feb 17, 2023 at 4:01 PM Christian Kohlschütter <christian@kohlschutter.com> wrote: > > On 18. Feb 2023, at 00:46, Saravana Kannan <saravanak@google.com> wrote: > > > > On Fri, Feb 17, 2023 at 3:33 PM Christian Kohlschütter > > <christian@kohlschutter.com> wrote: > >> > >> On 18. Feb 2023, at 00:22, Saravana Kannan <saravanak@google.com> wrote: > >>> > >>> On Thu, Aug 25, 2022 at 2:28 PM Christian Kohlschütter > >>> <christian@kohlschutter.com> wrote: > >>>> > >>>> 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. > >>> > >>> In your case, can you elaborate which part of the constraints/init > >>> twice caused the issue? > >>> > >>> I'm trying to simplify some of the supply resolving code and I'm > >>> trying to not break your use case. > >>> > >>> -Saravana > >> > >> Here's a write-up of my use case, and how we got to the solution: > >> https://kohlschuetter.github.io/blog/posts/2022/10/28/linux-nanopi-r4s/ > > > > I did read the write up before I sent my request. I'm asking for > > specifics on which functions in the set_machine_constraints() was > > causing the issue. And it's also a bit unclear to me if the issue was > > with having stuff called twice on the alway-on regulator or the > > supply. > > > > -Saravana > > I'm afraid I cannot give a more detailed answer than what's in the write up and the previous discussion on this mailing list; I thought it's pretty detailed already. Well, I do my best not to break your use case with whatever info you are willing to provide. We'll figure it out one way or another I suppose. > However, it should be relatively straightforward to reproduce the issue. If one has the hardware. Which I don't. -Saravana ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-02-18 0:06 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-03 14:33 [PATCH v2] regulator: core: Resolve supply name earlier to prevent double-init Vincent Legoll 2022-08-04 10:44 ` [PATCH v3] " Christian Kohlschütter 2022-08-15 11:17 ` Mark Brown 2022-08-18 12:46 ` [PATCH v4] " Christian Kohlschütter [not found] ` <CGME20220825113251eucas1p247c3d57de823da148ca4790975a4aba8@eucas1p2.samsung.com> 2022-08-25 11:32 ` Marek Szyprowski 2022-08-25 12:21 ` Mark Brown 2022-08-25 14:23 ` Marek Szyprowski 2022-08-25 15:18 ` Christian Kohlschütter 2022-08-25 21:28 ` [PATCH v5] " Christian Kohlschütter 2022-08-25 21:35 ` Christian Kohlschütter 2022-08-26 5:55 ` Marek Szyprowski 2022-08-29 15:43 ` Mark Brown 2022-08-29 17:01 ` Christian Kohlschütter 2023-02-17 23:22 ` Saravana Kannan 2023-02-17 23:33 ` Christian Kohlschütter 2023-02-17 23:46 ` Saravana Kannan 2023-02-18 0:01 ` Christian Kohlschütter 2023-02-18 0:05 ` Saravana Kannan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).