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 <[email protected]>
---
drivers/regulator/core.c | 42 ++++++++++++++++++++++++++--------------
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c4d844ffad7a..728840827e9c 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5433,7 +5433,34 @@ 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 (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
+ && (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)
@@ -5459,24 +5486,11 @@ 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.1
Hi all,
This is a follow-up patch from my patch on mmc side:
[PATCH v4] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4
https://patchwork.kernel.org/project/linux-rockchip/patch/[email protected]/
Thanks to Robin Murphy's help, we were able to figure out that my NanoPI R4S's SD-Card voltage regulator was initialized twice and that a voltage drop was the reason for the initialization failure.
Adding an mdelay to the init code, or — surprisingly — adding a "regulator-uv-protection-microvolt" declaration had "fixed" the issue in 99/100 tries.
Best,
Christian
from arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi:
vcc3v0_sd: vcc3v0-sd {
compatible = "regulator-fixed";
enable-active-high;
gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
pinctrl-names = "default";
pinctrl-0 = <&sdmmc0_pwr_h>;
regulator-always-on;
regulator-min-microvolt = <3000000>;
regulator-max-microvolt = <3000000>;
regulator-name = "vcc3v0_sd";
vin-supply = <&vcc3v3_sys>;
};
> Am 15.07.2022 um 22:32 schrieb Christian Kohlschütter <[email protected]>:
>
> 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 <[email protected]>
> ---
> drivers/regulator/core.c | 42 ++++++++++++++++++++++++++--------------
> 1 file changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index c4d844ffad7a..728840827e9c 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -5433,7 +5433,34 @@ 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 (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
> + && (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)
> @@ -5459,24 +5486,11 @@ 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.1
>
On Fri, Jul 15, 2022 at 10:32:16PM +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.
One small thing below but otherwise I think this should be fine, however
since we're very near the merge window I'd rather hold off any apply at
-rc1, just to give more time for things to get tested.
> - /* 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;
> }
The check for allocation failure should get pulled earlier in the
function along with the allocation, no sense in doing any of the other
work if we're going to fail.
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 <[email protected]>
---
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);
- /* 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
+ && (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)
@@ -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);
- /* 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
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
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 <[email protected]>
---
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
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.
From: Christian Kohlschütter <[email protected]>
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 <[email protected]>
---
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
Hi Christian,
On 18.08.2022 14:46, Christian Kohlschütter wrote:
> From: Christian Kohlschütter <[email protected]>
>
> 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 <[email protected]>
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
On Thu, Aug 25, 2022 at 01:32:50PM +0200, Marek Szyprowski wrote:
> This patch landed recently in linux next as commit 8a866d527ac0
> ("regulator: core: Resolve supply name earlier to prevent double-init").
> Unfortunately it breaks booting of Samsung Exynos 5800 based Peach-Pi
> (arch/arm/boot/dts/exynos5800-peach-pi.dts) and Peach-Pit
> (arch/arm/boot/dts/exynos5420-peach-pit.dts) Chromebooks. The last
> message in the kernel log is a message about disabling 'vdd_1v2'
> regulator. This regulator is not used directly, however it is a supply
> for other critical regulators.
This suggests that supplies are ending up not getting bound. Could you
perhaps add logging to check that we're attempting to resolve the supply
(in the
+ if ((rdev->supply_name && !rdev->supply) &&
+ (rdev->constraints->always_on ||
+ rdev->constraints->boot_on)) {
block)? I'd also note that it's useful to paste the actual error
messages you're seeing rather than just a description of them.
Hi Mark,
On 25.08.2022 14:21, Mark Brown wrote:
> On Thu, Aug 25, 2022 at 01:32:50PM +0200, Marek Szyprowski wrote:
>
>> This patch landed recently in linux next as commit 8a866d527ac0
>> ("regulator: core: Resolve supply name earlier to prevent double-init").
>> Unfortunately it breaks booting of Samsung Exynos 5800 based Peach-Pi
>> (arch/arm/boot/dts/exynos5800-peach-pi.dts) and Peach-Pit
>> (arch/arm/boot/dts/exynos5420-peach-pit.dts) Chromebooks. The last
>> message in the kernel log is a message about disabling 'vdd_1v2'
>> regulator. This regulator is not used directly, however it is a supply
>> for other critical regulators.
> This suggests that supplies are ending up not getting bound. Could you
> perhaps add logging to check that we're attempting to resolve the supply
> (in the
>
>
> + if ((rdev->supply_name && !rdev->supply) &&
> + (rdev->constraints->always_on ||
> + rdev->constraints->boot_on)) {
>
> block)?
I've spent a little time debugging this issue and here are my findings.
The problem is during the 'vdd_mif' regulator registration. It has one
supply called 'inb1' and provided by 'vdd_1v2' regulator. Both 'vdd_mif'
and 'vdd_1v2' regulators are provided by the same PMIC.
The problem is that 'inb1' supply is being routed to dummy regulator
after this change. The regulator_resolve_supply(), which is just after
the above mentioned check, returns 0 and bounds 'vdd_mif' supply to
dummy-regulator. This happens because regulator_dev_lookup() called in
regulator_resolve_supply() returns -19, what in turn lets the code to
use dummy-regulator. I didn't check why it doesn't return -EPROBE_DEFER
in that case yet.
> I'd also note that it's useful to paste the actual error
> messages you're seeing rather than just a description of them.
There is really nothing more that I can paste here:
[ 32.306264] systemd-logind[1375]: New seat seat0.
[ 32.331790] systemd-logind[1375]: Watching system buttons on
/dev/input/event1 (gpio-keys)
[ 32.550686] systemd-logind[1375]: Watching system buttons on
/dev/input/event0 (cros_ec)
[ 32.570493] systemd-logind[1375]: Failed to start user service,
ignoring: Unknown unit: [email protected]
[ 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
> On 25. Aug 2022, at 16:23, Marek Szyprowski <[email protected]> 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: [email protected]
> [ 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
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 <[email protected]>
---
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
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
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 <[email protected]>
Tested-by: Marek Szyprowski <[email protected]>
> ---
> 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
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.
> On 29. Aug 2022, at 17:43, Mark Brown <[email protected]> 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/[email protected]/
> On 18. Aug 2022, at 17:22, Mark Brown <[email protected]> wrote:
>
> On Fri, 22 Jul 2022 19:42:27 +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 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.
>>
>> [...]
>
> Applied to
>
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
>
> Thanks!
>
> [1/1] regulator: core: Resolve supply name earlier to prevent double-init
> commit: 8a866d527ac0441c0eb14a991fa11358b476b11d
>
> All being well this means that it will be integrated into the linux-next
> tree (usually sometime in the next 24 hours) and sent to Linus during
> the next merge window (or sooner if it is a bug fix), however if
> problems are discovered then the patch may be dropped or reverted.
>
> You may get further e-mails resulting from automated or manual testing
> and review of the tree, please engage with people reporting problems and
> send followup patches addressing any issues that are reported if needed.
>
> If any updates are required or you are submitting further changes they
> should be sent as incremental updates against current git, existing
> patches will not be replaced.
>
> Please add any relevant lists and maintainers to the CCs when replying
> to this mail.
>
> Thanks,
> Mark
I've finally managed to publish a blog post about this journey into regulator land; I hope you find it worthwhile.
https://kohlschuetter.github.io/blog/posts/2022/10/28/linux-nanopi-r4s/
Thanks to everybody involved for getting this far!
Special thanks go to Robin Murphy for pulling out the oscillator, and Mark Brown for helping that these changes get into 6.1.
Best,
Christian
On Thu, Aug 25, 2022 at 2:28 PM Christian Kohlschütter
<[email protected]> 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 <[email protected]>
> ---
> 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
>
>
On 18. Feb 2023, at 00:22, Saravana Kannan <[email protected]> wrote:
>
> On Thu, Aug 25, 2022 at 2:28 PM Christian Kohlschütter
> <[email protected]> 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/
On Fri, Feb 17, 2023 at 3:33 PM Christian Kohlschütter
<[email protected]> wrote:
>
> On 18. Feb 2023, at 00:22, Saravana Kannan <[email protected]> wrote:
> >
> > On Thu, Aug 25, 2022 at 2:28 PM Christian Kohlschütter
> > <[email protected]> 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
On 18. Feb 2023, at 00:46, Saravana Kannan <[email protected]> wrote:
>
> On Fri, Feb 17, 2023 at 3:33 PM Christian Kohlschütter
> <[email protected]> wrote:
>>
>> On 18. Feb 2023, at 00:22, Saravana Kannan <[email protected]> wrote:
>>>
>>> On Thu, Aug 25, 2022 at 2:28 PM Christian Kohlschütter
>>> <[email protected]> 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.
On Fri, Feb 17, 2023 at 4:01 PM Christian Kohlschütter
<[email protected]> wrote:
>
> On 18. Feb 2023, at 00:46, Saravana Kannan <[email protected]> wrote:
> >
> > On Fri, Feb 17, 2023 at 3:33 PM Christian Kohlschütter
> > <[email protected]> wrote:
> >>
> >> On 18. Feb 2023, at 00:22, Saravana Kannan <[email protected]> wrote:
> >>>
> >>> On Thu, Aug 25, 2022 at 2:28 PM Christian Kohlschütter
> >>> <[email protected]> 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