Message-Id: <20251031022700.492740-1-cong.yi@linux.dev>
Date: Fri, 31 Oct 2025 10:27:00 +0800
From: Yi Cong <cong.yi@...ux.dev>
To: andrew@...n.ch
Cc: Frank.Sae@...or-comm.com,
	cong.yi@...ux.dev,
	davem@...emloft.net,
	hkallweit1@...il.com,
	kuba@...nel.org,
	linux@...linux.org.uk,
	netdev@...r.kernel.org,
	yicong@...inos.cn
Subject: Re: [PATCH net-next 1/2] net: phy: motorcomm: correct the default rx delay config for the rgmii

On Thu, 30 Oct 2025 13:57:58 +0100, Andrew Lunn <andrew@...n.ch> wrote:
>
> On Thu, Oct 30, 2025 at 10:25:09AM +0800, Yi Cong wrote:
> > On Wed, 29 Oct 2025 13:07:35 +0100, Andrew Lunn <andrew@...n.ch> wrote:
> > >
> > > On Wed, Oct 29, 2025 at 11:00:42AM +0800, Yi Cong wrote:
> > > > From: Yi Cong <yicong@...inos.cn>
> > > >
> > > > According to the dataSheet, rx delay default value is set to 0.
> > >
> > > You need to be careful here, or you will break working boards. Please
> > > add to the commit message why this is safe.
> > >
> > > Also, motorcomm,yt8xxx.yaml says:
> > >
> > >   rx-internal-delay-ps:
> > >     description: |
> > >       RGMII RX Clock Delay used only when PHY operates in RGMII mode with
> > >       internal delay (phy-mode is 'rgmii-id' or 'rgmii-rxid') in pico-seconds.
> > >     enum: [ 0, 150, 300, 450, 600, 750, 900, 1050, 1200, 1350, 1500, 1650,
> > >             1800, 1900, 1950, 2050, 2100, 2200, 2250, 2350, 2500, 2650, 2800,
> > >             2950, 3100, 3250, 3400, 3550, 3700, 3850, 4000, 4150 ]
> > >     default: 1950
> >
> > Hi, Andrew, thanks for your reply!
> >
> > Let me add the following information:
> >
> > The chip documentation I have for the YT8521 and YT8531S:
> > "YT8521SH/YT8521SC Application Note, Version v1.7, Release Date: January 3, 2024"
> > "YT8531SH/YT8531SC Application Note, Version v1.2, Release Date: November 21, 2023"
> >
> > Both documents specify the RGMII delay configuration as follows:
> > The RX delay value can be set via Ext Reg 0xA003[13:10], where each
> > increment of 1 adds 150ps. After power-on, the default value of
> > bits [13:10] is 0.
> > The TX delay value can be set via Ext Reg 0xA003[3:0], with the
> > default value of bits [3:0] being 1 after power-on.
> >
> > I reviewed the commit history of this driver code. When YT8521 support
> > was initially added, the code configuration matched the chip manual:
> > 70479a40954c ("net: phy: Add driver for Motorcomm yt8521 gigabit ethernet phy")
> >
> > However, later when DTS support was added:
> > a6e68f0f8769 ("net: phy: Add dts support for Motorcomm yt8521 gigabit ethernet phy")
> > the default values were changed to 1.950ns.
>
> Read carefully...
>
> > >       RGMII RX Clock Delay used only when PHY operates in RGMII mode with
> > >       internal delay (phy-mode is 'rgmii-id' or 'rgmii-rxid') in pico-seconds.
>
> What do rgmii-id and rgmii-rxid mean?
>
> > Indeed, the RGMII standard specifies that the clock signal should be
> > delayed by 1-2ns relative to the data signals to ensure proper setup/hold
> > timing, which is likely the origin of the 1.950ns value in the YAML and
> > current code.
>
> https://elixir.bootlin.com/linux/v6.16.5/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L264
>
> > More importantly, the current Motorcomm driver's delay configuration logic
> > is incomplete. In the projects I've worked on, some configurations are
> > obtained from DTS, some from ACPI, but many manufacturers prefer to directly
> > set the delay values in BIOS based on their hardware design.
>
> Which is somewhat fine. If you want to the driver to not touch the PHY
> configuration you use PHY_INTERFACE_MODE_NA
>
> https://elixir.bootlin.com/linux/v6.16.5/source/include/linux/phy.h#L72
>
> However, in general, Linux should not rely on the BIOS, Linux should
> correctly configure the hardware, and in this case, it is trivial to
> do, since it is already all implemented.

The phy-mode is read by the MAC from ACPI. The same MAC paired with
different PHYs, or different vendors using the same MAC+PHY combination
but with varying motherboard designs, may result in differences here
(I've encountered both rgmii-id and rgmii-rxid).

For new products, I might recommend using PHY_INTERFACE_MODE_NA, but
for existing customers who are already using other configurations,
I no longer have control.

> > In fact, Motorcomm's Linux 5.4 driver versions guided PC motherboard
> > manufacturers to configure the delay values through BIOS, and the driver
> > code did not touch the delay registers (Ext Reg 0xA003). This means that
> > upgrading to a newer kernel version, where the driver writes 1.950ns
> > by default, could cause communication failures.
>
> Only if the MAC driver requests the PHY to adds delays.
>
> > To summarize, the current issues with the Motorcomm driver are:
> > 1. It only supports configuration via DTS, not via ACPI
> >     —— I may implement this myself or coordinate with Motorcomm
> >        in the future.
>
> FYI:
>
> https://elixir.bootlin.com/linux/v6.16.5/source/Documentation/firmware-guide/acpi/dsd/phy.rst
>
> You need to work within this framework.

Thank you. I will refer to these documents in my development work.



Our company is an operating system vendor, and our customers' hardware
varies greatly. Therefore, we must strive for maximum compatibility
—this includes accommodating diverse hardware designs from different
customers, as well as supporting upgrade requirements for computers
already sold to end users. I'm not sure if this aligns with the Linux
kernel community's principles.

Given these considerations, the two patches I submitted aim to align
the register's default values with those specified in the chip datasheet.
This ensures, at a minimum, that most customers' products—designed according
to the PHY manufacturer's specifications—will function correctly.

For the minority with special requirements—such as choosing rgmii-id or
rgmii-rxid, using different delay values—we allow configuration via DTS/ACPI,
support BIOS settings, and even ensure functionality when no explicit
configuration is applied, relying on the chip's default settings.

Returning to the current patch: changing the default value to match the
datasheet will not affect any phy-mode directly. This is because the delay value
is first obtained, and then its application is determined based on the phy-mode:
```
static int ytphy_rgmii_clk_delay_config(struct phy_device *phydev)
{
	...
	/* get rx/tx delay value */
	rx_reg = ytphy_get_delay_reg_value(phydev, "rx-internal-delay-ps",
					   ytphy_rgmii_delays, tb_size,
					   &rxc_dly_en,
					   YT8521_RC1R_RGMII_0_000_NS);
	tx_reg = ytphy_get_delay_reg_value(phydev, "tx-internal-delay-ps",
					   ytphy_rgmii_delays, tb_size, NULL,
					   YT8521_RC1R_RGMII_NONLINEAR_0_750_NS);

	/* Set it based on different phy-mode */
	switch (phydev->interface) {
	case PHY_INTERFACE_MODE_RGMII:
		rxc_dly_en = 0;
		break;
	case PHY_INTERFACE_MODE_RGMII_RXID:
		val |= FIELD_PREP(YT8521_RC1R_RX_DELAY_MASK, rx_reg);
		break;
	case PHY_INTERFACE_MODE_RGMII_TXID:
		rxc_dly_en = 0;
		val |= FIELD_PREP(YT8521_RC1R_GE_TX_DELAY_MASK, tx_reg);
		break;
	case PHY_INTERFACE_MODE_RGMII_ID:
		val |= FIELD_PREP(YT8521_RC1R_RX_DELAY_MASK, rx_reg) |
		       FIELD_PREP(YT8521_RC1R_GE_TX_DELAY_MASK, tx_reg);
		break;
	default: /* do not support other modes */
		return -EOPNOTSUPP;
	}
    ...
}
```

Regards,
    Yi Cong

Powered by blists - more mailing lists