[PATCH] net: phy: realtek: omit setting PHY-side delay when "rgmii" specified

[Icenowy Zheng]

Currently there are many boards that just set "rgmii" as phy-mode in the
device tree, and leave the hardware [TR]XDLY pins to set PHY delay mode.

In order to keep old device tree working, omit setting delay for just
"RGMII" without any internal delay suffix, otherwise many devices are
broken.

The definition of "rgmii" in the DT binding document is "RX and TX
delays are added by MAC when required", which at least literally do not
forbid the PHY to add delays.

Fixes: bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config")
Signed-off-by: Icenowy Zheng <ice...@aosc.io>
---
drivers/net/phy/realtek.c | 8 --------
1 file changed, 8 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index fb1db713b7fb..7d32db1c789f 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -189,11 +189,6 @@ static int rtl8211f_config_init(struct phy_device *phydev)
phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val);

switch (phydev->interface) {
- case PHY_INTERFACE_MODE_RGMII:
- val_txdly = 0;
- val_rxdly = 0;
- break;
-
case PHY_INTERFACE_MODE_RGMII_RXID:
val_txdly = 0;
val_rxdly = RTL8211F_RX_DELAY;
@@ -253,9 +248,6 @@ static int rtl8211e_config_init(struct phy_device *phydev)

/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
switch (phydev->interface) {
- case PHY_INTERFACE_MODE_RGMII:
- val = RTL8211E_CTRL_DELAY | 0;
- break;
case PHY_INTERFACE_MODE_RGMII_ID:
val = RTL8211E_CTRL_DELAY | RTL8211E_TX_DELAY | RTL8211E_RX_DELAY;
break;
--
2.28.0

[Andrew Lunn]

On Sun, Oct 25, 2020 at 04:55:56PM +0800, Icenowy Zheng wrote:
> Currently there are many boards that just set "rgmii" as phy-mode in the
> device tree, and leave the hardware [TR]XDLY pins to set PHY delay mode.
>
> In order to keep old device tree working, omit setting delay for just
> "RGMII" without any internal delay suffix, otherwise many devices are
> broken.

Hi Icenowy

We have been here before with the Atheros PHY. It did not correctly
implement one of the delay modes, until somebody really did need that
mode. So the driver was fixed. And we then found a number of device
trees were also buggy. It was painful for a while, but all the device
trees got fixed.

We should do the same here. Please submit patches for the device tree
files.

Andrew

[Icenowy Zheng]

于 2020年10月25日 GMT+08:00 下午10:18:25, Andrew Lunn <and...@lunn.ch> 写到:

>On Sun, Oct 25, 2020 at 04:55:56PM +0800, Icenowy Zheng wrote:
>> Currently there are many boards that just set "rgmii" as phy-mode in
>the
>> device tree, and leave the hardware [TR]XDLY pins to set PHY delay
>mode.
>>
>> In order to keep old device tree working, omit setting delay for just
>> "RGMII" without any internal delay suffix, otherwise many devices are
>> broken.
>
>Hi Icenowy
>
>We have been here before with the Atheros PHY. It did not correctly
>implement one of the delay modes, until somebody really did need that
>mode. So the driver was fixed. And we then found a number of device
>trees were also buggy. It was painful for a while, but all the device
>trees got fixed.

1. As the PHY chip has hardware configuration for configuring delays,
we should at least have a mode that respects what's set on the hardware.
2. As I know, at least Fedora ships a device tree with their bootloader, and
the DT will not be updated with kernel. This enforces compatibility
with old DTs even if they're broken.

Personally I think if someone wants a mode that explicitly disable delays
in the PHY, a new mode may be created now, maybe called "rgmii-noid".

[Andrew Lunn]

On Sun, Oct 25, 2020 at 10:27:05PM +0800, Icenowy Zheng wrote:
>
>
> 于 2020年10月25日 GMT+08:00 下午10:18:25, Andrew Lunn <and...@lunn.ch> 写到:
> >On Sun, Oct 25, 2020 at 04:55:56PM +0800, Icenowy Zheng wrote:
> >> Currently there are many boards that just set "rgmii" as phy-mode in
> >the
> >> device tree, and leave the hardware [TR]XDLY pins to set PHY delay
> >mode.
> >>
> >> In order to keep old device tree working, omit setting delay for just
> >> "RGMII" without any internal delay suffix, otherwise many devices are
> >> broken.
> >
> >Hi Icenowy
> >
> >We have been here before with the Atheros PHY. It did not correctly
> >implement one of the delay modes, until somebody really did need that
> >mode. So the driver was fixed. And we then found a number of device
> >trees were also buggy. It was painful for a while, but all the device
> >trees got fixed.
>
> 1. As the PHY chip has hardware configuration for configuring delays,
> we should at least have a mode that respects what's set on the hardware.

Yes, that is PHY_INTERFACE_MODE_NA. In DT, set the phy-mode to "". Or
for most MAC drivers, don't list a phy-mode at all.

> 2. As I know, at least Fedora ships a device tree with their bootloader, and
> the DT will not be updated with kernel.

I would check that. Debian does the exact opposite, the last time i
looked. It always uses the DT that come with the kernel because it
understands DT can have bugs, like all software.

Andrew

[Icenowy Zheng]

于 2020年10月25日 GMT+08:00 下午10:36:08, Andrew Lunn <and...@lunn.ch> 写到:

However, we still need to tell the MAC it's RGMII mode that is in use, not
MII/RMII/*MII. So the phy-mode still needs to be something that
contains rgmii.

[Andrew Lunn]

> >> 1. As the PHY chip has hardware configuration for configuring delays,
> >> we should at least have a mode that respects what's set on the
> >hardware.
> >
> >Yes, that is PHY_INTERFACE_MODE_NA. In DT, set the phy-mode to "". Or
> >for most MAC drivers, don't list a phy-mode at all.
>
> However, we still need to tell the MAC it's RGMII mode that is in use, not
> MII/RMII/*MII. So the phy-mode still needs to be something that
> contains rgmii.

So for this MAC driver, you are going to have to fix the device tree.
And for the short turn, maybe implement the workaround discussed in
the other thread.

Andrew

[Icenowy Zheng]

于 2020年10月26日 GMT+08:00 上午1:28:48, Andrew Lunn <and...@lunn.ch> 写到:

>> >> 1. As the PHY chip has hardware configuration for configuring
>delays,
>> >> we should at least have a mode that respects what's set on the
>> >hardware.
>> >
>> >Yes, that is PHY_INTERFACE_MODE_NA. In DT, set the phy-mode to "".
>Or
>> >for most MAC drivers, don't list a phy-mode at all.

By referring to linux/phy.h, NA means not applicable. This surely
do not apply when RGMII is really in use.

>>
>> However, we still need to tell the MAC it's RGMII mode that is in
>use, not
>> MII/RMII/*MII. So the phy-mode still needs to be something that
>> contains rgmii.
>
>So for this MAC driver, you are going to have to fix the device tree.
>And for the short turn, maybe implement the workaround discussed in
>the other thread.

I think no document declares RGMII must have all internal delays
of the PHY explicitly disabled. It just says RGMII.

I think the situation that RGMII is in use and PHY has the right to
decide whether to add delay or not surely matches "rgmii", and
to explicitly disable internal delays we need some other thing
like "rgmii-noid".

>
> Andrew

[Andrew Lunn]

> By referring to linux/phy.h, NA means not applicable. This surely
> do not apply when RGMII is really in use.

It means the PHY driver should not touch the mode, something else has
set it up. That could be strapping, the bootloader, ACPI firmware,
whatever.

> I think no document declares RGMII must have all internal delays
> of the PHY explicitly disabled. It just says RGMII.

Please take a look at all the other PHY drivers. They should all
disable delays when passed PHY_INTERFACE_MODE_RGMII.

Andrew

[Samuel Holland]

Icenowy,

Documentation/networking/phy.rst also makes this clear:

PHY_INTERFACE_MODE_RGMII: the PHY is not responsible for inserting any internal
delay by itself, it assumes that either the Ethernet MAC (if capable or the PCB
traces) insert the correct 1.5-2ns delay

Regards,
Samuel