Re: [PATCH v2] regulator: core: Resolve supply name earlier to prevent double-init

Re: [PATCH v2] regulator: core: Resolve supply name earlier to prevent double-init

[Vincent Legoll]

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

[PATCH v3] regulator: core: Resolve supply name earlier to prevent double-init

[Christian Kohlschütter]

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(&regulator_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

Re: [PATCH v3] regulator: core: Resolve supply name earlier to prevent double-init

[Mark Brown]

[-- 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 --]

[PATCH v4] regulator: core: Resolve supply name earlier to prevent double-init

[Christian Kohlschütter]

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(&regulator_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

Re: [PATCH v4] regulator: core: Resolve supply name earlier to prevent double-init

[Marek Szyprowski]

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(&regulator_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

Re: [PATCH v4] regulator: core: Resolve supply name earlier to prevent double-init

[Mark Brown]

[-- 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 --]

Re: [PATCH v4] regulator: core: Resolve supply name earlier to prevent double-init

[Marek Szyprowski]

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

Re: [PATCH v4] regulator: core: Resolve supply name earlier to prevent double-init

[Christian Kohlschütter]

> 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

[PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init

[Christian Kohlschütter]

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 = &regulator_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

Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init

[Christian Kohlschütter]

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

Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init

[Marek Szyprowski]

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 = &regulator_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

Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init

[Mark Brown]

[-- 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 --]

Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init

[Christian Kohlschütter]

> 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/

Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init

[Saravana Kannan]

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 = &regulator_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
>
>

Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init

[Christian Kohlschütter]

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/

Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init

[Saravana Kannan]

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

Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init

[Christian Kohlschütter]

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.

Re: [PATCH v5] regulator: core: Resolve supply name earlier to prevent double-init

[Saravana Kannan]

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