Message-ID: <c83f0193-ce24-4a3e-87d1-f52587e13ca4@lunn.ch>
Date: Wed, 22 Jan 2025 14:39:30 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: Ninad Palsule <ninad@...ux.ibm.com>,
	Jacky Chou <jacky_chou@...eedtech.com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
	"andrew@...econstruct.com.au" <andrew@...econstruct.com.au>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"eajames@...ux.ibm.com" <eajames@...ux.ibm.com>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"joel@....id.au" <joel@....id.au>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	"linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"minyard@....org" <minyard@....org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"openipmi-developer@...ts.sourceforge.net" <openipmi-developer@...ts.sourceforge.net>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"ratbert@...aday-tech.com" <ratbert@...aday-tech.com>,
	"robh@...nel.org" <robh@...nel.org>
Subject: Re: 回覆: 回覆: [PATCH v2 05/10]
 ARM: dts: aspeed: system1: Add RGMII support

> I myself got it wrong, as the kernel doc explicitely says that the "rgmii"
> phy-mode is the one to use to get MAC-side delay insertion, whereas the way I
> understand it, mac-side delay insertion doesn't really depend on the phy-mode
> passed from DT. Ideally we would even consider that these mac-side delay
> insertion would have to be ignored in basic 'RGMII' mode, but I think that would
> break quite some existing setups ?
> 
> Can we consider an update in the kernel doc along these lines :
> 
> ---
>  Documentation/networking/phy.rst | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/networking/phy.rst b/Documentation/networking/phy.rst
> index f64641417c54..7ab77f9867a0 100644
> --- a/Documentation/networking/phy.rst
> +++ b/Documentation/networking/phy.rst
> @@ -106,14 +106,17 @@ Whenever possible, use the PHY side RGMII delay for these reasons:
>    configure correctly a specified delay enables more designs with similar delay
>    requirements to be operated correctly
>  
> -For cases where the PHY is not capable of providing this delay, but the
> -Ethernet MAC driver is capable of doing so, the correct phy_interface_t value
> -should be PHY_INTERFACE_MODE_RGMII, and the Ethernet MAC driver should be
> -configured correctly in order to provide the required transmit and/or receive
> -side delay from the perspective of the PHY device. Conversely, if the Ethernet
> -MAC driver looks at the phy_interface_t value, for any other mode but
> -PHY_INTERFACE_MODE_RGMII, it should make sure that the MAC-level delays are
> -disabled.
> +The MAC driver may add delays if the PCB doesn't include any. This can be
> +detected based on firmware "rx-internal-delay-ps" and "tx-internal-delay-ps"
> +properties.
> +
> +When the MAC driver can insert the delays, it should always do so when these
> +properties are present and non-zero, regardless of the RGMII mode specified.
> +
> +However, the MAC driver must adjust the PHY_INTERFACE_MODE_RGMII_* mode it passes
> +to the connected PHY device (through phy_attach or phylink_create() for example)
> +to account for MAC-side delay insertion, so that the the PHY device knows
> +if any delays still needs insertion on either TX or RX paths.

You dropped:

   For cases where the PHY is not capable of providing this delay...

This is something i would like to keep, to strengthen that we really
do want the PHY to add the delays. Many MACs are capable of adding
delays, but we don't want them to, the PHY should do it, so we have
consistency.

The language i've tried to use is that "rx-internal-delay-ps" and
"tx-internal-delay-ps" can be used to fine tune the delays, so i'm
expecting their values to be small, because the PHY is adding the 2ns,
and the MAC is just adding/removing 0-200ps etc. I've also used the
same terminology for PHY drivers, the PHY DT properties for delays are
used for fine tuning, but the basic 2ns on/off comes from the phy-mode
passed to phylib.

If it is just fine tuning, and not adding the full 2ns, it should just
pass phy-mode straight through.

So your text becomes something like:

  The MAC driver may fine tune the delays. This can be configured
  based on firmware "rx-internal-delay-ps" and "tx-internal-delay-ps"
  properties. These values are expected to be small, not the full 2ns
  delay.

  A MAC driver inserting these fine tuning delays should always do so
  when these properties are present and non-zero, regardless of the
  RGMII mode specified.

Then we can address when the MAC adds the full 2ns.

  For cases where the PHY is not capable of providing the 2ns delay,
  the MAC must provide it, if the phy-mode indicates the PCB is not
  providing the delays. The MAC driver must adjust the
  PHY_INTERFACE_MODE_RGMII_* mode it passes to the connected PHY
  device (through phy_attach or phylink_create() for example) to
  account for MAC-side delay insertion, so that the the PHY device
  does not add additional delays.

I also think we need something near the beginning like:

  The device tree property phy-mode describes the hardware. When used
  with RGMII, its value indicates if the hardware, i.e. the PCB,
  provides the 2ns delay required for RGMII. A phy-mode of 'rgmii'
  indicates the PCB is adding the 2ns delay. For other values, the
  MAC/PHY pair must insert the needed 2ns delay, with the strong
  preference the PHY adds the delay.

	Andrew

Powered by blists - more mailing lists