| Message ID | 20201025085556.2861021-1-icenowy@aosc.io |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: realtek: omit setting PHY-side delay when "rgmii" specified | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Guessed tree name to be net-next |
| jkicinski/subject_prefix | warning | Target tree name not specified in the subject |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | fail | Errors and warnings before: 4 this patch: 4 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | fail | Link |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
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
于 2020年10月25日 GMT+08:00 下午10:18:25, Andrew Lunn <andrew@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". > >We should do the same here. Please submit patches for the device tree >files. > > Andrew
On Sun, Oct 25, 2020 at 10:27:05PM +0800, Icenowy Zheng wrote: > > > 于 2020年10月25日 GMT+08:00 下午10:18:25, Andrew Lunn <andrew@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
于 2020年10月25日 GMT+08:00 下午10:36:08, Andrew Lunn <andrew@lunn.ch> 写到: >On Sun, Oct 25, 2020 at 10:27:05PM +0800, Icenowy Zheng wrote: >> >> >> 于 2020年10月25日 GMT+08:00 下午10:18:25, Andrew Lunn <andrew@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. 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. > >> 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
> >> 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
于 2020年10月26日 GMT+08:00 上午1:28:48, Andrew Lunn <andrew@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
> 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
Icenowy, On 10/26/20 7:12 AM, Andrew Lunn wrote: >> 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. 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
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;
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 <icenowy@aosc.io> --- drivers/net/phy/realtek.c | 8 -------- 1 file changed, 8 deletions(-)