[PATCH net] r8169: fix rtl8168h rx crc error

[PATCH net] r8169: fix rtl8168h rx crc error

[ChunHao Lin]

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(+)

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

Re: [PATCH net] r8169: fix rtl8168h rx crc error

[Horatiu Vultur]

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
> 

-- 
/Horatiu

RE: [PATCH net] r8169: fix rtl8168h rx crc error

[Hau]

> 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.

Re: [PATCH net] r8169: fix rtl8168h rx crc error

[Jakub Kicinski]

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.

RE: [PATCH net] r8169: fix rtl8168h rx crc error

[Hau]

> 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.