Message-ID: <bad83fec-afca-4c41-bee4-e4e4f9ced57a@lunn.ch>
Date: Thu, 31 Jul 2025 15:31:52 +0200
From: Andrew Lunn <andrew@...n.ch>
To: 李志 <lizhi2@...incomputing.com>
Cc: weishangjuan@...incomputing.com, andrew+netdev@...n.ch,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, mcoquelin.stm32@...il.com,
	alexandre.torgue@...s.st.com, rmk+kernel@...linux.org.uk,
	yong.liang.choong@...ux.intel.com, vladimir.oltean@....com,
	jszhang@...nel.org, jan.petrous@....nxp.com,
	prabhakar.mahadev-lad.rj@...renesas.com, inochiama@...il.com,
	boon.khai.ng@...era.com, dfustini@...storrent.com, 0x1207@...il.com,
	linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, ningyu@...incomputing.com,
	linmin@...incomputing.com, pinkesh.vaghela@...fochips.com
Subject: Re: Re: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700
 ethernet driver

> > You hardware has a lot of flexibility, but none of if should actually
> > be needed, if you follow the standard.
> > 
> > So phy-mode = "rgmii-id"; should be all you need for most boards.
> > Everything else should be optional, with sensible defaults.
> > 
> 
> On our platform, the vendor-specific attributes eswin,dly-param-* were
> initially introduced to compensate for board-specific variations in RGMII
> signal timing, primarily due to differences in PCB trace lengths.

So it seems like, because you have the flexibility in the hardware,
you designed your PCB poorly, breaking the standard, so now must have
these properties.  It would of been much better if you had stuck to
the standard...

Please ensure your default values, when nothing is specified in DT,
correspond to a board which actually fulfils the standard. The next
board which is made using this device can then avoid having anything
special in there DT blob.

> These attributes allow fine-grained, per-signal delay control for RXD, TXD,
> TXEN, RXDV, RXCLK, and TXCLK, based on empirically derived optimal phase
> settings.
> In our experience, setting phy-mode = "rgmii-id" alone, along with only
> the standard properties rx-internal-delay-ps and tx-internal-delay-ps,
> has proven insufficient to meet our hardware's timing requirements.

You don't need vendor properties for RXCLK and TXCLK, that is what
tx-internal-delay-ps and rx-internal-delay-ps do. They change the
clock signal relative to TX and RX data. So you only need properties
for TXEN and RXDV. You should probably call these
eswin,txen-internal-delay-ps and eswin,rxdv-internal-delay-ps.  In the
binding you need to clearly define what these mean, for your hardware,
i.e.  what is the delay relative to?

> 1. Setting all delay parameters (RXD, TXD, TXEN, RXDV, RXCLK, and TXCLK)
>    using vendor-specific attributes eswin,dly-param-*.
>    e.g.
>    eswin,dly-param-1000m = <0x20202020 0x96205A20 0x20202020>;
> 2. Setting delay parameters (RXD, TXD, TXEN, RXDV) using vendor-specific
>    attributes eswin,dly-param-* , RXCLK using rx-internal-delay-ps and
>    TXCLK using tx-internal-delay-ps.
>    e.g
>    eswin,dly-param-1000m = <0x20202020 0x80200020 0x20202020>;
>    rx-internal-delay-ps = <9000>;
>    tx-internal-delay-ps = <2200>;

Neither. DT should not contain HW values you poke into registers. They
should be SI using, in this case, pico seconds. From these delays in
picoseconds, have the driver calculate what values should be written
into the registers.

And these delay values are unlikely to be correct. You are using
rgmii-id, so the PHY is adding 2ns. You want the MAC to make small
tuning adjustments, so 200 could be reasonable, but 9000ps is way too
big.

	Andrew

Powered by blists - more mailing lists