| Message ID | 20230322064550.2378-1-hau@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] r8169: fix rtl8168h rx crc error | expand |
The 03/22/2023 14:45, ChunHao Lin wrote: > > When link speed is 10 Mbps and temperature is under -20°C, rtl8168h may > have rx crc error. Disable phy 10 Mbps pll off to fix this issue. Don't forget to add the fixes tag. Another comment that I usually get is to replace hardcoded values with defines, but on the other side I can see that this file already has plently of hardcoded values. Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com> > > Signed-off-by: ChunHao Lin <hau@realtek.com> > --- > drivers/net/ethernet/realtek/r8169_phy_config.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/ethernet/realtek/r8169_phy_config.c b/drivers/net/ethernet/realtek/r8169_phy_config.c > index 930496cd34ed..b50f16786c24 100644 > --- a/drivers/net/ethernet/realtek/r8169_phy_config.c > +++ b/drivers/net/ethernet/realtek/r8169_phy_config.c > @@ -826,6 +826,9 @@ static void rtl8168h_2_hw_phy_config(struct rtl8169_private *tp, > /* disable phy pfm mode */ > phy_modify_paged(phydev, 0x0a44, 0x11, BIT(7), 0); > > + /* disable 10m pll off */ > + phy_modify_paged(phydev, 0x0a43, 0x10, BIT(0), 0); > + > rtl8168g_disable_aldps(phydev); > rtl8168g_config_eee_phy(phydev); > } > -- > 2.37.2 >
> The 03/22/2023 14:45, ChunHao Lin wrote: > > > > When link speed is 10 Mbps and temperature is under -20°C, rtl8168h > > may have rx crc error. Disable phy 10 Mbps pll off to fix this issue. > > Don't forget to add the fixes tag. > Another comment that I usually get is to replace hardcoded values with > defines, but on the other side I can see that this file already has plently of > hardcoded values. > It is not a fix for a specific commit. PHY 10m pll off is an power saving feature which is enabled by H/W default. This issue can be fixed by disable PHY 10m pll off. > Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com> > > > > > Signed-off-by: ChunHao Lin <hau@realtek.com> > > --- > > drivers/net/ethernet/realtek/r8169_phy_config.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/net/ethernet/realtek/r8169_phy_config.c > > b/drivers/net/ethernet/realtek/r8169_phy_config.c > > index 930496cd34ed..b50f16786c24 100644 > > --- a/drivers/net/ethernet/realtek/r8169_phy_config.c > > +++ b/drivers/net/ethernet/realtek/r8169_phy_config.c > > @@ -826,6 +826,9 @@ static void rtl8168h_2_hw_phy_config(struct > rtl8169_private *tp, > > /* disable phy pfm mode */ > > phy_modify_paged(phydev, 0x0a44, 0x11, BIT(7), 0); > > > > + /* disable 10m pll off */ > > + phy_modify_paged(phydev, 0x0a43, 0x10, BIT(0), 0); > > + > > rtl8168g_disable_aldps(phydev); > > rtl8168g_config_eee_phy(phydev); } > > -- > > 2.37.2 > > > > -- > /Horatiu > > ------Please consider the environment before printing this e-mail.
On Wed, 22 Mar 2023 12:13:12 +0000 Hau wrote: > > Don't forget to add the fixes tag. > > Another comment that I usually get is to replace hardcoded values with > > defines, but on the other side I can see that this file already has plently of > > hardcoded values. > > It is not a fix for a specific commit. PHY 10m pll off is an power > saving feature which is enabled by H/W default. This issue can be > fixed by disable PHY 10m pll off. How far back can the issue be reproduced? Is it only possible with certain device types? Then the Fixes tag should point at the commit which added support for the devices. Was it always present since 2.6 kernels? Put the first commit in the git history as Fixes.
> On Wed, 22 Mar 2023 12:13:12 +0000 Hau wrote: > > > Don't forget to add the fixes tag. > > > Another comment that I usually get is to replace hardcoded values > > > with defines, but on the other side I can see that this file already > > > has plently of hardcoded values. > > > > It is not a fix for a specific commit. PHY 10m pll off is an power > > saving feature which is enabled by H/W default. This issue can be > > fixed by disable PHY 10m pll off. > > How far back can the issue be reproduced? Is it only possible with certain > device types? Then the Fixes tag should point at the commit which added > support for the devices. Was it always present since 2.6 kernels? Put the first > commit in the git history as Fixes. > RTL8168H is the only chip we know that has this issue. This issue is related to the H/W default setting. So I will add a Fixes tag to the commit which added support for this chip and submit the patch again. ------Please consider the environment before printing this e-mail.
diff --git a/drivers/net/ethernet/realtek/r8169_phy_config.c b/drivers/net/ethernet/realtek/r8169_phy_config.c index 930496cd34ed..b50f16786c24 100644 --- a/drivers/net/ethernet/realtek/r8169_phy_config.c +++ b/drivers/net/ethernet/realtek/r8169_phy_config.c @@ -826,6 +826,9 @@ static void rtl8168h_2_hw_phy_config(struct rtl8169_private *tp, /* disable phy pfm mode */ phy_modify_paged(phydev, 0x0a44, 0x11, BIT(7), 0); + /* disable 10m pll off */ + phy_modify_paged(phydev, 0x0a43, 0x10, BIT(0), 0); + rtl8168g_disable_aldps(phydev); rtl8168g_config_eee_phy(phydev); }
When link speed is 10 Mbps and temperature is under -20°C, rtl8168h may have rx crc error. Disable phy 10 Mbps pll off to fix this issue. Signed-off-by: ChunHao Lin <hau@realtek.com> --- drivers/net/ethernet/realtek/r8169_phy_config.c | 3 +++ 1 file changed, 3 insertions(+)