mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Sascha Hauer <s.hauer@pengutronix.de>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc: Barebox List <barebox@lists.infradead.org>
Subject: Re: [PATCH 1/2] net: designware: eqos: reset phy
Date: Tue, 8 Jun 2021 10:58:17 +0200	[thread overview]
Message-ID: <20210608085817.GD5267@pengutronix.de> (raw)
In-Reply-To: <2166dc52-64da-14bb-987c-7000c31c6882@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


  reply	other threads:[~2021-06-08  9:00 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-07 14:10 Sascha Hauer
2021-06-07 14:10 ` [PATCH 2/2] net: eqos: Rockchip support Sascha Hauer
2021-06-07 16:05   ` Ahmad Fatoum
2021-06-07 15:59 ` [PATCH 1/2] net: designware: eqos: reset phy Ahmad Fatoum
2021-06-07 22:22   ` Sascha Hauer
2021-06-08  7:31     ` Ahmad Fatoum
2021-06-08  8:58       ` Sascha Hauer [this message]
2021-06-09  8:04         ` Ahmad Fatoum

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210608085817.GD5267@pengutronix.de \
    --to=s.hauer@pengutronix.de \
    --cc=a.fatoum@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox