Re: [PATCH v2] arm64: dts: rockchip: support Ethernet Switch adapter for RK3588 Jaguar

[Quentin Schulz]

Hi Andrew,

On 6/15/25 4:53 PM, Andrew Lunn wrote:
> On Fri, Jun 13, 2025 at 04:27:54PM +0200, Quentin Schulz wrote:
> > Hi Andrew,
> >
> > On 5/28/25 3:09 PM, Andrew Lunn wrote:
> > > On Wed, May 28, 2025 at 09:56:51AM +0200, Quentin Schulz wrote:
> > > > Hi Andrew,
> > > >
> > > > On 5/27/25 6:18 PM, Andrew Lunn wrote:
> > > > > On Tue, May 27, 2025 at 03:11:42PM +0200, Jakob Unterwurzacher wrote:
[...]
> > > > I'll need to implement reading the delay from the stmmac driver to use this
> > > > property, do I need to restrict reading this property to the SoC we tested
> > > > (RK3588)?
> > >
> > > Yes, please only allow it to be used on RK3588, and any other SoC you
> > > can test and verify its behaviour.
> >
> > Coming back to this topic, I'm unfortunately the bearer of some bad news.
> >
> > I implemented the suggested logic (see at the end of this mail) and then
> > went to validate it with Jakob's help. Unfortunately, it seems that the
> > delay value really isn't stable or reliable.
> >
> > We tested the same adapter with two different main boards (the same product,
> > just two different units). With a value of 0x40 for tx_delay (which should
> > be ~2000ps if we have a 31ps per tx_delay unit as empirically decided), we
> > have one board with 1778ps and one with 1391ps. Following a hunch, we
> > started to stress (or cool) the device (with stress-ng/a fan) and it did
> > slightly change the result too. Changing the CPU operating points (and by
> > extension at least CPU clocks) didn't impact the result though.
>
> Thanks for taking such a scientific approach to this. Most developers
> try values until it works, and call it done. It is nice to see
> somebody doing some real study.
>
> Russell quoted the standard, which says the delay needs to be between
> 1ns and 2.6ns, which is quite a wide range. So for a tx_delay value of
> 0x40, nominally 2000ps, your two values are within that range, and so
> conform to the standard.

If there's a source about the 2.6ns being the upper limit, I would 
appreciate a link to it (or the maths leading to this claim) :) My 
understanding is: at least 1.2ns.

Considering that for 125MHz TXC (for 1GbE), the clock period is 8ns and 
that two

> > While this could be observed with tx_delay property too, this property
> > doesn't claim to provide a value in picoseconds that tx-internal-delay-ps
> > would (but at the same time this didn't stop it to be implemented for the
> > DSA switch we have which claims "more than 1.5ns" and nothing more, so maybe
> > that would be acceptable?).
> >
> > I feel uncomfortable contributing this considering the wildly different
> > results across our very small test sample pool of two units and slightly
> > different operating temperature.
>
> I can understand that. But there is another way to look at this. I am
> making a big jump from just two boards, but it seems to me, tx_delay
> and rx_delay are pointless, if they produce such a wide range of
> values from what should be identical boards. They cannot be used for
> fine tuning because the same value has a 387ps difference, which is
> huge compared to the 31ps step.

I'm making the same conclusion.

I'm not sure though it's a good idea to force the user to implement the 
delay in the PHY only. I can tell you that we have two variants of 
RK3399 Puma, RK3588 Jaguar and RK3588 Tiger with either a KSZ9031 or a 
KSZ9131 PHY. As far as I understood from reading the driver, the delay 
is implemented differently for both PHYs. KSZ9131 adds a DLL-based clock 
skew (4.9.3.1 in datasheet[1]) in the appropriate direction whenever 
phy-mode requires a delay be added by MAC/PHY. KSZ9031 doesn't. Both 
KSZ9031 and KSZ9131 have per-lane skews. If I wanted the delay to be 
added by the PHY in the DT, I basically wouldn't be able to share the DT 
between both variants of our boards as the KSZ9131 would add a 2ns 
delay[2] based on phy-mode but not KSZ9031 where I assume I would need 
to use pad skews instead to add up to 1.9ns (TXC; 1.4ns for RXC) but 
then we would have ~4ns for KSZ9131 which is likely out of spec :/ Today 
it works by "chance" because our phy-mode is rgmii but the delay is 
added by the MAC (due to the long-standing mistake in the Rockchip GMAC 
driver). I have not tested any of the above claims.

[1] 
https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/00002841D.pdf
[2] Link [1] at 4.9.3 RGMII TIMING

> It seems to me, rx_delay and tx_delay should be deprecated, set to 0,
> and let the PHY do they delays. If i remember correctly, that is what
> you ended up with for you board?

This is what we ended up doing for our adapter board indeed.

I'm trying to figure out if there's a way to not break stuff by forcing 
the properties to 0 and deprecating the values.

tx_delay is used (by the driver) whenever the delay is added by the PCB 
on TXC (based on phy-mode, which is incorrect). Same for rx_delay but 
for RXC. So if we set phy-mode correctly (as it's being pushed lately 
for Rockchip boards), tx_delay/rx_delay will need to be set to 0 
explicitly whenever the delay is expected to be NOT implemented by the 
PCB (so in addition to the phy-mode saying "delay not added by the PCB").

If the properties are missing in the DT, they are assigned a default 
value. So we cannot simply remove them or expect them gone (we also want 
DT backward compatibility so cannot change the driver behavior :/).

The only way forward that I see is forcing the properties to be 0 in 
device tree and warn if they are non-zero or something. Will trigger a 
bunch of warning/errors on existing Device Trees though. And this will 
not allow us to use two slightly different PHYs for our products without 
having two separate Device Trees.

Cheers,
Quentin