[PATCH] regulator: fixed: Default enable high on DT regulators

[PATCH] regulator: fixed: Default enable high on DT regulators

[Linus Walleij]

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(-)

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";
-- 
2.17.1

Re: [PATCH] regulator: fixed: Default enable high on DT regulators

[John Stultz]

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>

Re: [PATCH] regulator: fixed: Default enable high on DT regulators

[Leonard Crestez]

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

Re: [PATCH] regulator: fixed: Default enable high on DT regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: fixed: Default enable high on DT regulators

[Leonard Crestez]

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/

Re: [PATCH] regulator: fixed: Default enable high on DT regulators

[Linus Walleij]

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

Re: [PATCH] regulator: fixed: Default enable high on DT regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]