| Message ID | 20220825212842.7176-1-christian@kohlschutter.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] regulator: core: Resolve supply name earlier to prevent double-init | expand |
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 <christian@kohlschutter.com> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/regulator/core.c | 58 ++++++++++++++++++++++++---------------- > 1 file changed, 35 insertions(+), 23 deletions(-) > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > index 77f60eef960..2ff0ab2730f 100644 > --- a/drivers/regulator/core.c > +++ b/drivers/regulator/core.c > @@ -5391,6 +5391,7 @@ regulator_register(const struct regulator_desc *regulator_desc, > bool dangling_of_gpiod = false; > struct device *dev; > int ret, i; > + bool resolved_early = false; > > if (cfg == NULL) > return ERR_PTR(-EINVAL); > @@ -5494,24 +5495,10 @@ regulator_register(const struct regulator_desc *regulator_desc, > BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier); > INIT_DELAYED_WORK(&rdev->disable_work, regulator_disable_work); > > - /* preform any regulator specific init */ > - if (init_data && init_data->regulator_init) { > - ret = init_data->regulator_init(rdev->reg_data); > - if (ret < 0) > - goto clean; > - } > - > - if (config->ena_gpiod) { > - ret = regulator_ena_gpio_request(rdev, config); > - if (ret != 0) { > - rdev_err(rdev, "Failed to request enable GPIO: %pe\n", > - ERR_PTR(ret)); > - goto clean; > - } > - /* The regulator core took over the GPIO descriptor */ > - dangling_cfg_gpiod = false; > - dangling_of_gpiod = false; > - } > + if (init_data && init_data->supply_regulator) > + rdev->supply_name = init_data->supply_regulator; > + else if (regulator_desc->supply_name) > + rdev->supply_name = regulator_desc->supply_name; > > /* register with sysfs */ > rdev->dev.class = ®ulator_class; > @@ -5533,13 +5520,38 @@ regulator_register(const struct regulator_desc *regulator_desc, > goto wash; > } > > - if (init_data && init_data->supply_regulator) > - rdev->supply_name = init_data->supply_regulator; > - else if (regulator_desc->supply_name) > - rdev->supply_name = regulator_desc->supply_name; > + if ((rdev->supply_name && !rdev->supply) && > + (rdev->constraints->always_on || > + rdev->constraints->boot_on)) { > + ret = regulator_resolve_supply(rdev); > + if (ret != 0) > + rdev_dbg(rdev, "Unable to resolve supply early: %pe\n", > + ERR_PTR(ret)); > + > + resolved_early = true; > + } > + > + /* perform any regulator specific init */ > + if (init_data && init_data->regulator_init) { > + ret = init_data->regulator_init(rdev->reg_data); > + if (ret < 0) > + goto wash; > + } > + > + if (config->ena_gpiod) { > + ret = regulator_ena_gpio_request(rdev, config); > + if (ret != 0) { > + rdev_err(rdev, "Failed to request enable GPIO: %pe\n", > + ERR_PTR(ret)); > + goto wash; > + } > + /* The regulator core took over the GPIO descriptor */ > + dangling_cfg_gpiod = false; > + dangling_of_gpiod = false; > + } > > ret = set_machine_constraints(rdev); > - if (ret == -EPROBE_DEFER) { > + if (ret == -EPROBE_DEFER && !resolved_early) { > /* Regulator might be in bypass mode and so needs its supply > * to set the constraints > */ Best regards
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 <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/
On Thu, Aug 25, 2022 at 2:28 PM Christian Kohlschütter <christian@kohlschutter.com> wrote: > > Previously, an unresolved regulator supply reference upon calling > regulator_register on an always-on or boot-on regulator caused > set_machine_constraints to be called twice. > > This in turn may initialize the regulator twice, leading to voltage > glitches that are timing-dependent. A simple, unrelated configuration > change may be enough to hide this problem, only to be surfaced by > chance. In your case, can you elaborate which part of the constraints/init twice caused the issue? I'm trying to simplify some of the supply resolving code and I'm trying to not break your use case. -Saravana > > One such example is the SD-Card voltage regulator in a NanoPI R4S that > would not initialize reliably unless the registration flow was just > complex enough to allow the regulator to properly reset between calls. > > Fix this by re-arranging regulator_register, trying resolve the > regulator's supply early enough that set_machine_constraints does not > need to be called twice. > > Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com> > --- > drivers/regulator/core.c | 58 ++++++++++++++++++++++++---------------- > 1 file changed, 35 insertions(+), 23 deletions(-) > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > index 77f60eef960..2ff0ab2730f 100644 > --- a/drivers/regulator/core.c > +++ b/drivers/regulator/core.c > @@ -5391,6 +5391,7 @@ regulator_register(const struct regulator_desc *regulator_desc, > bool dangling_of_gpiod = false; > struct device *dev; > int ret, i; > + bool resolved_early = false; > > if (cfg == NULL) > return ERR_PTR(-EINVAL); > @@ -5494,24 +5495,10 @@ regulator_register(const struct regulator_desc *regulator_desc, > BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier); > INIT_DELAYED_WORK(&rdev->disable_work, regulator_disable_work); > > - /* preform any regulator specific init */ > - if (init_data && init_data->regulator_init) { > - ret = init_data->regulator_init(rdev->reg_data); > - if (ret < 0) > - goto clean; > - } > - > - if (config->ena_gpiod) { > - ret = regulator_ena_gpio_request(rdev, config); > - if (ret != 0) { > - rdev_err(rdev, "Failed to request enable GPIO: %pe\n", > - ERR_PTR(ret)); > - goto clean; > - } > - /* The regulator core took over the GPIO descriptor */ > - dangling_cfg_gpiod = false; > - dangling_of_gpiod = false; > - } > + if (init_data && init_data->supply_regulator) > + rdev->supply_name = init_data->supply_regulator; > + else if (regulator_desc->supply_name) > + rdev->supply_name = regulator_desc->supply_name; > > /* register with sysfs */ > rdev->dev.class = ®ulator_class; > @@ -5533,13 +5520,38 @@ regulator_register(const struct regulator_desc *regulator_desc, > goto wash; > } > > - if (init_data && init_data->supply_regulator) > - rdev->supply_name = init_data->supply_regulator; > - else if (regulator_desc->supply_name) > - rdev->supply_name = regulator_desc->supply_name; > + if ((rdev->supply_name && !rdev->supply) && > + (rdev->constraints->always_on || > + rdev->constraints->boot_on)) { > + ret = regulator_resolve_supply(rdev); > + if (ret != 0) > + rdev_dbg(rdev, "Unable to resolve supply early: %pe\n", > + ERR_PTR(ret)); > + > + resolved_early = true; > + } > + > + /* perform any regulator specific init */ > + if (init_data && init_data->regulator_init) { > + ret = init_data->regulator_init(rdev->reg_data); > + if (ret < 0) > + goto wash; > + } > + > + if (config->ena_gpiod) { > + ret = regulator_ena_gpio_request(rdev, config); > + if (ret != 0) { > + rdev_err(rdev, "Failed to request enable GPIO: %pe\n", > + ERR_PTR(ret)); > + goto wash; > + } > + /* The regulator core took over the GPIO descriptor */ > + dangling_cfg_gpiod = false; > + dangling_of_gpiod = false; > + } > > ret = set_machine_constraints(rdev); > - if (ret == -EPROBE_DEFER) { > + if (ret == -EPROBE_DEFER && !resolved_early) { > /* Regulator might be in bypass mode and so needs its supply > * to set the constraints > */ > -- > 2.36.2 > >
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/
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
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.
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
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 */
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(-)