Message-ID: <YfxtglvVDx2JJM9w@lunn.ch>
Date:   Fri, 4 Feb 2022 01:04:18 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Tim Harvey <tharvey@...eworks.com>
Cc:     Martin Schiller <ms@....tdt.de>, Hauke Mehrtens <hauke@...ke-m.de>,
        martin.blumenstingl@...glemail.com,
        Florian Fainelli <f.fainelli@...il.com>, hkallweit1@...il.com,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        David Miller <davem@...emloft.net>, kuba@...nel.org,
        netdev <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net v3] net: phy: intel-xway: enable integrated led
 functions

> Andrew,
> 
> The issue is that in an ideal world where all parts adhere to the
> JEDEC standards 2ns would be correct but that is not reality. In my
> experience with the small embedded boards I help design and support
> not about trace lengths as it would take inches to skew 0.5ns but
> instead differences in setup/hold values for various MAC/PHY parts
> that are likely not adhering the standards.
> 
> Some examples from current boards I support:
> - CN8030 MAC rgmii-rxid with intel-xway GPY111 PHY: we need to
> configure this for rx_delay=1.5ns and tx_delay=0.5ns

So i assume this is what broke for you. Your bootloader sets these
delays, phy-type is rgmii-id, and since you don't have the properties
for what exact delays you want it default to what 802.3 specifies,
2ns.

I actually think the current behaviour of the driver is correct.  By
saying phy-type = rgmii-id you are telling the PHY driver to insert
the delays and that is what it is doing.

In retrospect, i would say you had two choices to cleanly solve this.

1) Do exactly what the patch does, add properties to define what
actual delay you want, when you don't want 2ns.

2) Have the bootloader set the delay, but also tell Linux you have set
the phy mode including the delays, and it should not touch them. There
is a well defined way to do this:

PHY_INTERFACE_MODE_NA

It has two main use cases. The first is that the MAC and PHY are
integrated, there is no MII between them, phy-mode is completely
unnecessary. This is the primary meaning.

The second meaning is, something else has setup the phy mode, e.g. the
bootloader. Don't touch it.

This second meaning does not always work, since the driver might reset
the PHY, and depending on the PHY, that might clear whatever the
bootloader set. So it is not reliable.

> - CN8030 MAC rgmii-rxid with dp83867 GPY111 PHY: we need to configure
> this for rx_delay=2.0ns and tx_delay=2.0ns (as would be expected)
> - IMX8MM FEC MAC rgmii-id with intel-xway PHY: we need to configure
> this for rx_delay=2ns and tx_delay=2.5ns
> - IMX8MM FEC MAC rgmii-id with dp83867 PHY: we need to configure this
> for rx_delay=2.0ns and tx_delay=2.0ns (as would be expected)
> 
> The two boards above have identical well matched trace-lengths between
> the two PHY variant designs so you can see here that the intel-xway
> GPY111 PHY needs an extra 0.5ps delay to avoid RX errors on the
> far-off board.

So a couple of ideas.

Since GPY111 and dp83867 use different properties for the delays, just
put both in DT. The PHY driver will look for the property it
understands and ignore the other. So you can have different delays for
different PHYs.

We have started to standardize on the delay property. And new driver
should be using rx-internal-delay-ps and tx-internal-delay-ps. If you
have two drivers using the same property name, what i just suggested
will not work. Can you control the address the PHY uses? Put the
GPY111 on a different address to the dp83867. List them both in DT. If
you don't have a phy-handle in DT, the FEC will go hunting on its MDIO
bus and use the first PHY it finds. You can put the needed delay
properties in DT for the specific PHY.

	Andrew

Powered by blists - more mailing lists