On Fri, Feb 28, 2020 at 06:26:16PM +0100, Andrew Lunn wrote:

quoted

quoted

What is not clearly defined, is how you combine
PHY_INTERFACE_MODE_RGMII* with DT properties. I guess i would enforce
that phydev->interface is PHY_INTERFACE_MODE_RGMII and then the delays
in DT are absolute values.

So, if there's a value in the device tree, and the mode corresponds
(RXID for Rx skew), we do use the dt value. That should look like what's
in the patch (except for the default value used when no configuration is
provided in the dt).

No. I would not do that. PHY_INTERFACE_MODE_RGMII_RXID means 2ns delay
for RX. So how do you then interpret the DT property. Is it 2ns + the
DT delay? That would then mean you need negative values in DT if you
want short delays than 2ns.

Which is why i suggest PHY_INTERFACE_MODE_RGMII. It is then clear you
have a base delay of 0ns, and the DT property then has the absolute
value of the delay.

OK I see, to avoid confusion we could either define RGMII_ID or RGMII
and fixed delays in the dt if the 2ns one isn't what we need.

We may need an RGMII dedicated documentation then, to avoid future
confusion :)

quoted

quoted

There is also some discussion about what should go in DT. #defines
like you have, or time in pS, and the driver converts to register
values. I generally push towards pS.

That would allow a more generic dt binding, and could be used by other
PHY drivers. The difficulty would be to map the pS to allowed values in
the h/w. Should we round them to the upper or lower bound?

I would document the accepted values in DT, and return -EINVAL if DT
does not exactly match one of the listed values. Plus a phydev_err()
message to help debug.

OK.

quoted

I also saw the micrel-ksz90x1 dt documentation describes many skews, not
only one for Rx and one for Tx. How would the generic dt binding would
look like then?

It is a balancing act. Do you actually need dt properties for your
hardware? Are the standard 2ns delays sufficient. For many designs it
is. Just because the hardware supports all sorts of configurations,
are they actually needed? It seems like adding delays are needed for a
few boards. But do all the properties exposed for the Micrel PHY every
get used, or is it a case of, the hardware has it, lets make it
available, even if nobody ever uses it?

Right, this kind of settings shouldn't be needed for lots of boards, so
we can add a per-PHY binding, only when it's needed. The board I'm
currently working on do use smaller delays than 2ns and I was told to
use even lower ones if the connexion wasn't stable. But then do we
really need such a configuration upstream (apart from supporting the 2ns
delays)? That's a good question :)

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com