Message-ID: <b0be71ba-e2e0-4a6d-94ba-72d54959c929@cherry.de>
Date: Mon, 16 Jun 2025 11:14:01 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Andrew Lunn <andrew@...n.ch>
Cc: Jakob Unterwurzacher <jakobunt@...il.com>, foss+kernel@...il.net,
 conor+dt@...nel.org, devicetree@...r.kernel.org, heiko@...ech.de,
 jakob.unterwurzacher@...rry.de, krzk+dt@...nel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-rockchip@...ts.infradead.org, robh@...nel.org,
 Kever Yang <kever.yang@...k-chips.com>
Subject: Re: [PATCH v2] arm64: dts: rockchip: support Ethernet Switch adapter
 for RK3588 Jaguar

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

Powered by blists - more mailing lists