gpio_request_by_name for GPIOD_IS_OUT drives GPIO before specifically instructed to

[Tim Harvey]

On Tue, Mar 1, 2022 at 10:59 AM Sean Anderson <sean.anderson at seco.com> wrote:
>
> Hi Tim,
>
> On 3/1/22 1:45 PM, Tim Harvey wrote:
> > Greetings,
> >
> > I'm seeing an issue in U-Boot caused by gpio_request_by_name driving a
> > GPIO output before it has been given an output level with
> > {dm_}gpio_set_value.
> >
> > In my particular instance I have a network PHY that can encounter
> > errata if it gets reset more than once (fun time with this one!)
> > declared liks this:
> >
> > &ethphy0 {
> >        reset-gpios = <&gpio3 0 GPIO_ACTIVE_LOW>;
> >        reset-assert-us = <1000>;
> >        reset-deassert-us = <300000>;
> > };
> >
> > Through testing it has been found that this PHY must be held in reset
> > on power-up and released from reset only once. This is probably the
> > third time I've run into such an issue in the 20 years I've worked
> > with hardware - it's rare and certainly a result of poor reset
> > handling in IC's but not a unique situation (especially at a time
> > where we can't be too choosy about what parts go on boards).
> >
> > When eth-phy-uclass.c calls gpio_request_by_name(dev, "reset-gpios",
> > 0, &uc_priv->reset_gpio, GPIOD_IS_OUT) direction_output is called
> > which sets the GPIO to a high effectively taking the PHY out of reset
> > before eth_phy_reset is called to do so. Setting the gpio as
> > GPIO_ACTIVE_LOW will drive it low keeping it in reset until ready to
> > de-assert it but then the polarity is inverted which results in the
> > PHY being left in reset permanently.
>
> Can you try
>
> gpio_request_by_name(dev, "reset-gpios", 0, &uc_priv->reset_gpio, \
>         GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE)
>

Sean,

Thanks - this does work! When looking into this flag originally I
misunderstood what that flag does. It causes the GPIO to be
initialized in the 'active' (asserted) state vs the 'inactive'
(de-asserted) state.

I would certainly think that reset gpios should be initialized in the
de-asserted case but I wonder if I go and change phy-reset-gpio in
drivers/net/fec_mxc.c and phy-reset in dirvers/net/eth-phy-uclass.c if
this would cause issues for other boards?

diff --git a/drivers/net/eth-phy-uclass.c b/drivers/net/eth-phy-uclass.c
index 1f285f7afd20..27b77444a0c5 100644
--- a/drivers/net/eth-phy-uclass.c
+++ b/drivers/net/eth-phy-uclass.c
@@ -137,7 +137,7 @@ static int eth_phy_of_to_plat(struct udevice *dev)
        /* search "reset-gpios" in phy node */
        ret = gpio_request_by_name(dev, "reset-gpios", 0,
                                   &uc_priv->reset_gpio,
-                                  GPIOD_IS_OUT);
+                                  GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
        if (ret && ret != -ENOENT)
                return ret;

diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
index 985b03844739..c1f4930172cf 100644
--- a/drivers/net/fec_mxc.c
+++ b/drivers/net/fec_mxc.c
@@ -1579,7 +1579,7 @@ static int fecmxc_of_to_plat(struct udevice *dev)

 #if CONFIG_IS_ENABLED(DM_GPIO)
        ret = gpio_request_by_name(dev, "phy-reset-gpios", 0,
-                                  &priv->phy_reset_gpio, GPIOD_IS_OUT);
+                                  &priv->phy_reset_gpio, GPIOD_IS_OUT
| GPIOD_IS_OUT_ACTIVE);
        if (ret < 0)
                return 0; /* property is optional, don't return error! */

Tim

> ?
>
> --Sean
>
> > This affects a wide variety of gpio based things including gpio-hogs.
> >
> > I'm looking for some input on how to best address this issue. I
> > haven't finished walking through all the kernel code but I believe
> > when a gpio is requested it does so without configuring that gpio. Any
> > ideas?
> >
> > Best regards,
> >
> > Tim
> >