| Message ID | 20181001204346.4655-1-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | 28be5f15df2ee6882b0a122693159c96a28203c7 |
| Headers | show |
| Series | regulator: fixed: Default enable high on DT regulators | expand |
On Mon, Oct 1, 2018 at 1:43 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > commit efdfeb079cc3 > ("regulator: fixed: Convert to use GPIO descriptor only") > switched to use gpiod_get() to look up the regulator from the > gpiolib core whether that is device tree or boardfile. > > This meant that we activate the code in > a603a2b8d86e ("gpio: of: Add special quirk to parse regulator flags") > which means the descriptors coming from the device tree already > have the right inversion and open drain semantics set up from > the gpiolib core. > > As the fixed regulator was inspected again we got the > inverted inversion and things broke. > > Fix it by ignoring the config in the device tree for now: the > later patches in the series will push all inversion handling > over to the gpiolib core and set it up properly in the > boardfiles for legacy devices, but I did not finish that > for this kernel cycle. > > Fixes: commit efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only") > Reported-by: Leonard Crestez <leonard.crestez@nxp.com> > Reported-by: Fabio Estevam <festevam@gmail.com> > Reported-by: John Stultz <john.stultz@linaro.org> > Reported-by: Anders Roxell <anders.roxell@linaro.org> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> This seems to solve it! Tested-by: John Stultz <john.stultz@linaro.org>
On Mon, 2018-10-01 at 22:43 +0200, Linus Walleij wrote: > commit efdfeb079cc3 > ("regulator: fixed: Convert to use GPIO descriptor only") > switched to use gpiod_get() to look up the regulator from the > gpiolib core whether that is device tree or boardfile. > > This meant that we activate the code in > a603a2b8d86e ("gpio: of: Add special quirk to parse regulator flags") > which means the descriptors coming from the device tree already > have the right inversion and open drain semantics set up from > the gpiolib core. > > As the fixed regulator was inspected again we got the > inverted inversion and things broke. > > Fix it by ignoring the config in the device tree for now: the > later patches in the series will push all inversion handling > over to the gpiolib core and set it up properly in the > boardfiles for legacy devices, but I did not finish that > for this kernel cycle. > > Fixes: commit efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only") > Reported-by: Leonard Crestez <leonard.crestez@nxp.com> > Reported-by: Fabio Estevam <festevam@gmail.com> > Reported-by: John Stultz <john.stultz@linaro.org> > Reported-by: Anders Roxell <anders.roxell@linaro.org> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> This doesn't work for imx6sx-sdb but I suspect an imx issue. Log: [ 0.150198] regulator-enet-3v3 GPIO handle specifies active low - ignored [ 0.150258] gpio_value: 38 set 1 [ 0.150283] gpio_direction: 38 out (0) ... [ 1.962493] regulator_enable: name=enet_3v3 [ 1.966709] gpio_value: 38 set 0 [ 1.970005] regulator_enable_delay: name=enet_3v3 [ 1.974730] regulator_enable_complete: name=enet_3v3 ... [ 4.097077] fec 2188000.ethernet eth0: Unable to connect to phy [ 4.109219] IP-Config: Failed to open eth0 [ 4.115557] fec 21b4000.ethernet eth1: Unable to connect to phy [ 4.123690] IP-Config: Failed to open eth1 This turns the phy off and on again instead of leaving it up from uboot and it doesn't work for some reason. However looking at reg_fixed_voltage_probe introducing an edge seems to be intentional for regulators which are not marked with "enabled-at-boot". Right? It's possible that you exposed an imx board-specific bug: maybe power cycling the phy after uboot needs some missing fixup? Apparently if I revert your patch the old behavior is to never touch this GPIO. I spent a while debugging this and the cause seems to be that this regulator has the "gpios" property instead of "gpio". The "gpios" property is not actually handled by old regulator-fixed of_get_named_gpio(np, "gpio", 0) call but only by the new path going through of_find_gpio. I can also break boot by fixing the gpios property on stable 4.18: reg_enet_3v3: regulator-enet-3v3 { compatible = "regulator-fixed"; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_enet_3v3>; regulator-name = "enet_3v3"; regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; - gpios = <&gpio2 6 GPIO_ACTIVE_LOW>; + gpio = <&gpio2 6 GPIO_ACTIVE_LOW>; }; Any suggestions? This turned out to be quite messy. -- Regards, Leonard
On Tue, Oct 02, 2018 at 01:42:38PM +0000, Leonard Crestez wrote: > This turns the phy off and on again instead of leaving it up from uboot > and it doesn't work for some reason. However looking at > reg_fixed_voltage_probe introducing an edge seems to be intentional for > regulators which are not marked with "enabled-at-boot". Right? No, that's definitely not desired. We don't want to change the state of the regulator at all if we can avoid it unless the user explicitly asked for it. > It's possible that you exposed an imx board-specific bug: maybe power > cycling the phy after uboot needs some missing fixup? It'd probably also be good to sort this out though.
On Wed, 2018-10-03 at 13:10 +0100, Mark Brown wrote: > On Tue, Oct 02, 2018 at 01:42:38PM +0000, Leonard Crestez wrote: > > > This turns the phy off and on again instead of leaving it up from uboot > > and it doesn't work for some reason. However looking at > > reg_fixed_voltage_probe introducing an edge seems to be intentional for > > regulators which are not marked with "enabled-at-boot". Right? > > No, that's definitely not desired. We don't want to change the state of > the regulator at all if we can avoid it unless the user explicitly asked > for it. That also makes sense, for a top level perspective. But reg_fixed_voltage_probe contains the following snippet: if (config->enabled_at_boot) { if (config->enable_high) cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH; else cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW; } else { if (config->enable_high) cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW; else cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH; } Unless config->enabled_at_boot the GPIO is initialized with the opposite polarity of enabled. This means that fixed regulators which are not marked with "enabled-at-boot" but happen to be "on" anyway are always power cycled. This is from before recent changes by Linus, code dates from 2012 commit 25a53dfbfbfd ("regulator: fixed: Use core GPIO enable support"). Even before that the logic was similar: drvdata->is_enabled = config->enabled_at_boot; ret = drvdata->is_enabled ? config->enable_high : !config->enable_high; gpio_flag = ret ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW; Looking further back it seems that this behavior has always been present in fixed-regulator code. In theory it might be possible to request the GPIO while asking to keep the value from the bootloader? Maybe I'm confused but I don't see an easy way to do this through the GPIO api; functions for requesting in output mode all seem to also ask for the initial value. GPIOD_ASIS looks close but it doesn't even adjust the direction. > > It's possible that you exposed an imx board-specific bug: maybe power > > cycling the phy after uboot needs some missing fixup? > > It'd probably also be good to sort this out though. Yes, handled separately: https://lore.kernel.org/patchwork/patch/994871/
On Wed, Oct 3, 2018 at 8:08 PM Leonard Crestez <leonard.crestez@nxp.com> wrote: > On Wed, 2018-10-03 at 13:10 +0100, Mark Brown wrote: > > On Tue, Oct 02, 2018 at 01:42:38PM +0000, Leonard Crestez wrote: > > > > > This turns the phy off and on again instead of leaving it up from uboot > > > and it doesn't work for some reason. However looking at > > > reg_fixed_voltage_probe introducing an edge seems to be intentional for > > > regulators which are not marked with "enabled-at-boot". Right? > > > > No, that's definitely not desired. We don't want to change the state of > > the regulator at all if we can avoid it unless the user explicitly asked > > for it. > > That also makes sense, for a top level perspective. But > reg_fixed_voltage_probe contains the following snippet: > > if (config->enabled_at_boot) { > if (config->enable_high) > cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH; > else > cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW; > } else { > if (config->enable_high) > cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW; > else > cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH; > } GPIOD_* flags nowadays but yes. > Unless config->enabled_at_boot the GPIO is initialized with the > opposite polarity of enabled. This means that fixed regulators which > are not marked with "enabled-at-boot" but happen to be "on" anyway are > always power cycled. > > This is from before recent changes by Linus, code dates from 2012 > commit 25a53dfbfbfd ("regulator: fixed: Use core GPIO enable support"). > Even before that the logic was similar: > > drvdata->is_enabled = config->enabled_at_boot; > ret = drvdata->is_enabled ? > config->enable_high : !config->enable_high; > gpio_flag = ret ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW; > > Looking further back it seems that this behavior has always been > present in fixed-regulator code. Yep, I can't take the credit for that, I just tried to preserve the logic that was already there. > In theory it might be possible to request the GPIO while asking to keep > the value from the bootloader? > > Maybe I'm confused but I don't see an > easy way to do this through the GPIO api; functions for requesting in > output mode all seem to also ask for the initial value. > > GPIOD_ASIS looks close but it doesn't even adjust the direction. If the bootloader set up the line value, isn't it reasonable to assume it also set up the direction (to output)? How else would it even work? That said, you could just call gpiod_direction_output() after getting the handle if you want to make sure it is set as output. We are floating patches that will re-enable the GPIO code to call .get_direction() on all descriptors at boot. After that you can use gpiod_get_direction() to determine if it needs to be switched, but that would be a bit overengineered IMO. I guess you can make a patch using GPIOD_ASIS, but I am worried that several systems will depend on the active driving of this since as you describe, it has been like this for ages. Yet again I'm not the conservative kind, as you notice, so by all means try it! :) > > > It's possible that you exposed an imx board-specific bug: maybe power > > > cycling the phy after uboot needs some missing fixup? > > > > It'd probably also be good to sort this out though. > > Yes, handled separately: https://lore.kernel.org/patchwork/patch/994871/ That one was especially interesting! Yours, Linus Walleij
On Wed, Oct 03, 2018 at 06:08:09PM +0000, Leonard Crestez wrote: > On Wed, 2018-10-03 at 13:10 +0100, Mark Brown wrote: > > No, that's definitely not desired. We don't want to change the state of > > the regulator at all if we can avoid it unless the user explicitly asked > > for it. > That also makes sense, for a top level perspective. But > reg_fixed_voltage_probe contains the following snippet: ... > In theory it might be possible to request the GPIO while asking to keep > the value from the bootloader? Maybe I'm confused but I don't see an > easy way to do this through the GPIO api; functions for requesting in > output mode all seem to also ask for the initial value. There isn't one. You're not supposed to read back an output GPIO at all unfortunately, it makes this sort of handover stuff a problem. > GPIOD_ASIS looks close but it doesn't even adjust the direction. That's new - it's probably closest to what we want but we need to be able to read back the state as well and will want to make it output if it isn't already.
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c index 1142f195529b..7d639ad953b6 100644 --- a/drivers/regulator/fixed.c +++ b/drivers/regulator/fixed.c @@ -79,9 +79,14 @@ of_get_fixed_voltage_config(struct device *dev, of_property_read_u32(np, "startup-delay-us", &config->startup_delay); - config->enable_high = of_property_read_bool(np, "enable-active-high"); - config->gpio_is_open_drain = of_property_read_bool(np, - "gpio-open-drain"); + /* + * FIXME: we pulled active low/high and open drain handling into + * gpiolib so it will be handled there. Delete this in the second + * step when we also remove the custom inversion handling for all + * legacy boardfiles. + */ + config->enable_high = 1; + config->gpio_is_open_drain = 0; if (of_find_property(np, "vin-supply", NULL)) config->input_supply = "vin";
commit efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only") switched to use gpiod_get() to look up the regulator from the gpiolib core whether that is device tree or boardfile. This meant that we activate the code in a603a2b8d86e ("gpio: of: Add special quirk to parse regulator flags") which means the descriptors coming from the device tree already have the right inversion and open drain semantics set up from the gpiolib core. As the fixed regulator was inspected again we got the inverted inversion and things broke. Fix it by ignoring the config in the device tree for now: the later patches in the series will push all inversion handling over to the gpiolib core and set it up properly in the boardfiles for legacy devices, but I did not finish that for this kernel cycle. Fixes: commit efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only") Reported-by: Leonard Crestez <leonard.crestez@nxp.com> Reported-by: Fabio Estevam <festevam@gmail.com> Reported-by: John Stultz <john.stultz@linaro.org> Reported-by: Anders Roxell <anders.roxell@linaro.org> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/regulator/fixed.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) -- 2.17.1