Re: [PATCH 1/2] net: designware: eqos: reset phy

[Sascha Hauer <s.hauer@pengutronix.de>]

On Tue, Jun 08, 2021 at 09:31:03AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 08.06.21 00:22, Sascha Hauer wrote:
> > On Mon, Jun 07, 2021 at 05:59:02PM +0200, Ahmad Fatoum wrote:
> >> Hello Sascha,
> >>
> >> On 07.06.21 16:10, Sascha Hauer wrote:
> >>> The designware eqos DT binding has support for specifying reset GPIOs.
> >>> Add support for them.
> >>>
> >>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> >>> ---
> >>>  drivers/net/designware_eqos.c | 33 +++++++++++++++++++++++++++++++++
> >>>  drivers/of/of_gpio.c          |  7 +++++++
> >>>  2 files changed, 40 insertions(+)
> >>>
> >>> diff --git a/drivers/net/designware_eqos.c b/drivers/net/designware_eqos.c
> >>> index d2baaeaf63..0321024169 100644
> >>> --- a/drivers/net/designware_eqos.c
> >>> +++ b/drivers/net/designware_eqos.c
> >>> @@ -8,9 +8,11 @@
> >>>  
> >>>  #include <common.h>
> >>>  #include <init.h>
> >>> +#include <gpio.h>
> >>>  #include <dma.h>
> >>>  #include <net.h>
> >>>  #include <of_net.h>
> >>> +#include <of_gpio.h>
> >>>  #include <linux/iopoll.h>
> >>>  #include <linux/time.h>
> >>>  #include <linux/sizes.h>
> >>> @@ -189,6 +191,33 @@ struct eqos_desc {
> >>>  
> >>>  #define MII_BUSY		(1 << 0)
> >>>  
> >>> +static int eqos_phy_reset(struct device_d *dev, struct eqos *eqos)
> >>> +{
> >>> +	int phy_reset;
> >>> +	int ret;
> >>> +	u32 delays[3] = { 0, 0, 0 };
> >>> +
> >>> +	phy_reset = of_get_named_gpio(dev->device_node, "snps,reset-gpio", 0);
> >>> +
> >>> +        if (!gpio_is_valid(phy_reset))
> >>> +		return 0;
> >>
> >> Whitespace is wrong.
> >>
> >>> +
> >>> +	ret = gpio_request(phy_reset, "phy-reset");
> >>> +	if (ret)
> >>> +		return ret;
> >>
> >> Can you use gpiod_get instead? This would reduce the boilerplate here.
> > 
> > Sure. I didn't realize I don't honour the active high/low flags the way
> > I did it.
> > 
> >>
> >>> +
> >>> +	of_property_read_u32_array(dev->device_node,
> >>> +				   "snps,reset-delays-us",
> >>> +				   delays, ARRAY_SIZE(delays));
> >>> +
> >>
> >> Looks strange to read out a delay and not act on it. I'd prefer
> >> waiting delays[0] here.
> > 
> > Yes, it looks strange, but that's because the binding doesn't make much
> > sense. Why should I insert a delay before doing anything?
> 
>                    .--------.
> 
>      POR --------->|R  flip |---- Regulator ----> PHY VDD
> 
>                 .->|S  flop |
> 
>                 |  `--------'
> 
>                 |   
> 
>                 |   
> 
>                 |
> 
> RESET GPIO -----'`-------------------------------> PHY Reset
> 
> (active low)
> 
> It's stupid, but it works with Linux and wouldn't with barebox
> (if PHY VDD takes too long to stabilize)... ^^'

What we are doing is:

1) RESET_GPIO has unknown state
2) gpio_request()
3) RESET_GPIO has the same unknown state
4) udelay(delays[0])
5) RESET_GPIO still has the same unknown state
6) gpio_set_active()
7) RESET_GPIO puts phy into reset

Your FLIPFLOP will also only set its output at step 7), so your
regulator stabilization time begins at 7), you would still have
to make delays[1] long enough for the regulator to stabilize and
the phy come up.
What matters here is the point where we put RESET_GPIO into a known
state. Whether you do step 4) or leave it out makes no difference.

> 
> > I can a delay here, it wouldn't make much difference as all users except
> > one specify zero delay here anyway. For the one exception I would bet
> > someone has inserted the first delay without a good reason, they are all
> > 10000.
> 
> That's probably true. I still think mimicking Linux' interpretation
> of bindings is a good general rule to follow.

As long as they make sense, yes.

> 
> >>
> >>> +	gpio_direction_active(phy_reset, 0);
> >>
> >> This always sets logical zero, because gpio_request above doesn't
> >> accept a flag telling it otherwise. You'll need of_get_named_gpio_flags
> >> here, at which point, you'll have basically open-coded gpiod_get(), so
> >> you could use that.
> > 
> > Right.
> > 
> >>
> >>> +	udelay(delays[1]);
> >>
> >> Linux rounds up to 1 msec granularity here. We should do likewise.
> > 
> > I don't see a good reason for that. The Linux driver used udelay()
> > initially, that was changed to msleep as the times were too long for
> > busy waiting. I have no clue why the author didn't use usleep_range
> > instead.
> 
> Same reason: Device trees are tested with Linux. They've a better chance
> of just working when we round up wait times the same way.

I am generally with you, but in this case the binding is very clear and
if you find a bug in the dts with this case, then be happy and fix it.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox