Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Leonard Crestez]

Hello,

This patch seems to break network booting on imx6sx-sdb in linux-next
because the enet phy regulator is not on. Reverting the patch fixes
boot.

Here is the regulator definition:

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>;
};

Here are some prints with the patch enabled:

[    0.153150] reg-fixed-voltage regulator-enet-3v3: OUT_HIGH gflags 0x7
[    0.153171] reg_fixed_voltage_probe(179): regulator-enet-3v3 call gpiod_get_optional gflags=0x7 ena_gpio_invert
[    0.153218] regulator-enet-3v3 GPIO handle specifies active low - ignored
[    0.153233] of_gpio_flags_quirks(83): regulator-enet-3v3 set active low because !enable-active-high
[    0.153258] of_get_named_gpiod_flags: parsed 'gpios' property of node '/regulator-enet-3v3[0]' - status (0)
[    0.153310] gpio_value: 38 set 0
[    0.153332] gpio_direction: 38 out (0)
[    0.153364] enet_3v3: ena_gpiod=(ptrval) ena_gpio=0 init=0 valid=1
[    0.153377] enet_3v3: request GPIO
[    0.153393] enet_3v3: already have gpiod
[    0.153423] enet_3v3: 3300 mV 
...
[    3.867827] enet_3v3: GPIO enable count=0 inv=1
[    3.872432] enet_3v3: set value 0
[    3.875779] gpio_value: 38 set 1

That "gpio_value: 30 set 1" tracepoint is wrong, the line is set high.

It seems that gpiod_set_value will check FLAG_ACTIVE_LOW and
automatically invert so maybe ena_gpio_invert should not be used if a
full gpiod is passed to regulator?

--- drivers/regulator/fixed.c
+++ drivers/regulator/fixed.c
@@ -146,7 +146,6 @@ static int reg_fixed_voltage_probe(struct
platform_device *pdev)
 
        drvdata->desc.fixed_uV = config->microvolts;
 
-       cfg.ena_gpio_invert = !config->enable_high;
        if (config->enabled_at_boot) {
                if (config->enable_high)
                        gflags = GPIOD_OUT_HIGH;

All these high/low inversions and flags are extremely confusing to me.

Link to original thread: https://lkml.org/lkml/2018/9/6/485

--
Regards,
Leonard

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Linus Walleij]

On Mon, Oct 1, 2018 at 8:53 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:

> This patch seems to break network booting on imx6sx-sdb in linux-next
> because the enet phy regulator is not on. Reverting the patch fixes
> boot.

Thanks for reporting.

John Stultz reported the same problem I'm trying to debug it.

> Here is the regulator definition:
>
> 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>;
> };

This is a bit odd actually, the GPIO_ACTIVE_LOW flag will
be ignored as you see:

> [    0.153171] reg_fixed_voltage_probe(179): regulator-enet-3v3 call gpiod_get_optional gflags=0x7 ena_gpio_invert
> [    0.153218] regulator-enet-3v3 GPIO handle specifies active low - ignored
> [    0.153233] of_gpio_flags_quirks(83): regulator-enet-3v3 set active low because !enable-active-high

Because regulators don't specify active high/low in the second
cell because of legacy bindings.

So this should not be in the device tree anyway, it should be
GPIO_ACTIVE_HIGH or just 0.

> That "gpio_value: 30 set 1" tracepoint is wrong, the line is set high.
>
> It seems that gpiod_set_value will check FLAG_ACTIVE_LOW and
> automatically invert
(...)
>so maybe ena_gpio_invert should not be used if a
> full gpiod is passed to regulator?
(...)
> -       cfg.ena_gpio_invert = !config->enable_high;

Indeed. I will look closer so it's the right fix and provide a patch.

> All these high/low inversions and flags are extremely confusing to me.

Yeah it's what I'm trying to get rid of with these patches,
this is just the first patch in a series that move inversion over
to the GPIO library.

Yours,
Linus Walleij

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Fabio Estevam]

Hi Linus,

On Mon, Oct 1, 2018 at 5:17 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> > 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>;
> > };
>
> This is a bit odd actually, the GPIO_ACTIVE_LOW flag will
> be ignored as you see:

Yes, the flag will be ignored by the regulator driver, but the dts
description is correct: it is an active low GPIO that turns on the
reg_enet_3v3 regulator.

The 'enable-active-high' flag needs to be passed to indicate an active
high polarity.

> > [    0.153171] reg_fixed_voltage_probe(179): regulator-enet-3v3 call gpiod_get_optional gflags=0x7 ena_gpio_invert
> > [    0.153218] regulator-enet-3v3 GPIO handle specifies active low - ignored
> > [    0.153233] of_gpio_flags_quirks(83): regulator-enet-3v3 set active low because !enable-active-high
>
> Because regulators don't specify active high/low in the second
> cell because of legacy bindings.
>
> So this should not be in the device tree anyway, it should be
> GPIO_ACTIVE_HIGH or just 0.

Then it would provide a wrong description that does not describe the reality.

Re: [PATCH v7] regulator: fixed: Convert to use GPIO descriptor only

[Linus Walleij]

On Mon, Oct 1, 2018 at 10:36 PM Fabio Estevam <festevam@gmail.com> wrote:
> On Mon, Oct 1, 2018 at 5:17 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> > > 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>;
> > > };
> >
> > This is a bit odd actually, the GPIO_ACTIVE_LOW flag will
> > be ignored as you see:
>
> Yes, the flag will be ignored by the regulator driver, but the dts
> description is correct: it is an active low GPIO that turns on the
> reg_enet_3v3 regulator.
>
> The 'enable-active-high' flag needs to be passed to indicate an active
> high polarity.

Yes.

> > > [    0.153171] reg_fixed_voltage_probe(179): regulator-enet-3v3 call gpiod_get_optional gflags=0x7 ena_gpio_invert
> > > [    0.153218] regulator-enet-3v3 GPIO handle specifies active low - ignored
> > > [    0.153233] of_gpio_flags_quirks(83): regulator-enet-3v3 set active low because !enable-active-high
> >
> > Because regulators don't specify active high/low in the second
> > cell because of legacy bindings.
> >
> > So this should not be in the device tree anyway, it should be
> > GPIO_ACTIVE_HIGH or just 0.
>
> Then it would provide a wrong description that does not describe the reality.

OK my bad, by all means keep it. :)

The warning message does not say the description is wrong, just that it will
be ignored, and it is there for the users to be aware of that setting
GPIO_ACTIVE_LOW will have no semantic effect.

We introduced it because we were worried that if we don't print that,
users will tend to think that their GPIO_ACTIVE_LOW flags are
respected, so it was the best we could think of.

The real problem, of course, is that the bindings are ambiguous with
the elder binding taking precedence. Sorry about that, we were young
and didn't know how to do it the right way. Lesson learnt.

Yours,
Linus Walleij