[PATCH V2] net: stmmac: dwmac-tegra: Fix link bring-up sequence

[PATCH V2] net: stmmac: dwmac-tegra: Fix link bring-up sequence

[Paritosh Dixit]

The Tegra MGBE driver sometimes fails to initialize, reporting the
following error, and as a result, it is unable to acquire an IP
address with DHCP:

 tegra-mgbe 6800000.ethernet: timeout waiting for link to become ready

As per the recommendation from the Tegra hardware design team, fix this
issue by:
- clearing the PHY_RDY bit before setting the CDR_RESET bit and then
setting PHY_RDY bit before clearing CDR_RESET bit. This ensures valid
data is present at UPHY RX inputs before starting the CDR lock.
- adding the required delays when bringing up the UPHY lane. Note we
need to use delays here because there is no alternative, such as
polling, for these cases. Using the usleep_range() instead of ndelay()
as sleeping is preferred over busy wait loop.

Without this change we would see link failures on boot sometimes as
often as 1 in 5 boots. With this fix we have not observed any failures
in over 1000 boots.

Fixes: d8ca113724e7 ("net: stmmac: tegra: Add MGBE support")
Signed-off-by: Paritosh Dixit <paritoshd@nvidia.com>
---
Changes since V1:
- Replaced ndelay() with usleep_range() as sleeping is preferred over
busy wait loop.
- Replaced c99 comments '// ...' with ansi comments '/* ... */'.

 drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
index 362f85136c3e..6fdd94c8919e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
@@ -127,10 +127,12 @@ static int mgbe_uphy_lane_bringup_serdes_up(struct net_device *ndev, void *mgbe_
 	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_AUX_RX_IDDQ;
 	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
 
+	usleep_range(10, 20);  /* 50ns min delay needed as per HW design */
 	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
 	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_RX_SLEEP;
 	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
 
+	usleep_range(10, 20);  /* 500ns min delay needed as per HW design */
 	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
 	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_CAL_EN;
 	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
@@ -143,22 +145,30 @@ static int mgbe_uphy_lane_bringup_serdes_up(struct net_device *ndev, void *mgbe_
 		return err;
 	}
 
+	usleep_range(10, 20);  /* 50ns min delay needed as per HW design */
 	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
 	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_DATA_EN;
 	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
 
 	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
-	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_CDR_RESET;
+	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_RX_PCS_PHY_RDY;
 	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
 
+	usleep_range(10, 20);  /* 50ns min delay needed as per HW design */
 	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
-	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_RX_CDR_RESET;
+	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_CDR_RESET;
 	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
 
+	usleep_range(10, 20);  /* 50ns min delay needed as per HW design */
 	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
 	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_PCS_PHY_RDY;
 	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
 
+	msleep(30);  /* 30ms delay needed as per HW design */
+	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
+	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_RX_CDR_RESET;
+	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
+
 	err = readl_poll_timeout(mgbe->xpcs + XPCS_WRAP_IRQ_STATUS, value,
 				 value & XPCS_WRAP_IRQ_STATUS_PCS_LINK_STS,
 				 500, 500 * 2000);
-- 
2.25.1

Re: [PATCH V2] net: stmmac: dwmac-tegra: Fix link bring-up sequence

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Thu, 10 Oct 2024 10:29:08 -0400 you wrote:
> The Tegra MGBE driver sometimes fails to initialize, reporting the
> following error, and as a result, it is unable to acquire an IP
> address with DHCP:
> 
>  tegra-mgbe 6800000.ethernet: timeout waiting for link to become ready
> 
> As per the recommendation from the Tegra hardware design team, fix this
> issue by:
> - clearing the PHY_RDY bit before setting the CDR_RESET bit and then
> setting PHY_RDY bit before clearing CDR_RESET bit. This ensures valid
> data is present at UPHY RX inputs before starting the CDR lock.
> - adding the required delays when bringing up the UPHY lane. Note we
> need to use delays here because there is no alternative, such as
> polling, for these cases. Using the usleep_range() instead of ndelay()
> as sleeping is preferred over busy wait loop.
> 
> [...]

Here is the summary with links:
  - [V2] net: stmmac: dwmac-tegra: Fix link bring-up sequence
    https://git.kernel.org/netdev/net/c/1cff6ff302f5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH V2] net: stmmac: dwmac-tegra: Fix link bring-up sequence

[Jon Hunter]

On 10/10/2024 15:29, Paritosh Dixit wrote:
> The Tegra MGBE driver sometimes fails to initialize, reporting the
> following error, and as a result, it is unable to acquire an IP
> address with DHCP:
> 
>   tegra-mgbe 6800000.ethernet: timeout waiting for link to become ready
> 
> As per the recommendation from the Tegra hardware design team, fix this
> issue by:
> - clearing the PHY_RDY bit before setting the CDR_RESET bit and then
> setting PHY_RDY bit before clearing CDR_RESET bit. This ensures valid
> data is present at UPHY RX inputs before starting the CDR lock.
> - adding the required delays when bringing up the UPHY lane. Note we
> need to use delays here because there is no alternative, such as
> polling, for these cases. Using the usleep_range() instead of ndelay()
> as sleeping is preferred over busy wait loop.
> 
> Without this change we would see link failures on boot sometimes as
> often as 1 in 5 boots. With this fix we have not observed any failures
> in over 1000 boots.
> 
> Fixes: d8ca113724e7 ("net: stmmac: tegra: Add MGBE support")
> Signed-off-by: Paritosh Dixit <paritoshd@nvidia.com>
> ---
> Changes since V1:
> - Replaced ndelay() with usleep_range() as sleeping is preferred over
> busy wait loop.
> - Replaced c99 comments '// ...' with ansi comments '/* ... */'.
> 
>   drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> index 362f85136c3e..6fdd94c8919e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> @@ -127,10 +127,12 @@ static int mgbe_uphy_lane_bringup_serdes_up(struct net_device *ndev, void *mgbe_
>   	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_AUX_RX_IDDQ;
>   	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
>   
> +	usleep_range(10, 20);  /* 50ns min delay needed as per HW design */
>   	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
>   	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_RX_SLEEP;
>   	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
>   
> +	usleep_range(10, 20);  /* 500ns min delay needed as per HW design */
>   	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
>   	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_CAL_EN;
>   	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> @@ -143,22 +145,30 @@ static int mgbe_uphy_lane_bringup_serdes_up(struct net_device *ndev, void *mgbe_
>   		return err;
>   	}
>   
> +	usleep_range(10, 20);  /* 50ns min delay needed as per HW design */
>   	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
>   	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_DATA_EN;
>   	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
>   
>   	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> -	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_CDR_RESET;
> +	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_RX_PCS_PHY_RDY;
>   	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
>   
> +	usleep_range(10, 20);  /* 50ns min delay needed as per HW design */
>   	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> -	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_RX_CDR_RESET;
> +	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_CDR_RESET;
>   	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
>   
> +	usleep_range(10, 20);  /* 50ns min delay needed as per HW design */
>   	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
>   	value |= XPCS_WRAP_UPHY_RX_CONTROL_RX_PCS_PHY_RDY;
>   	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
>   
> +	msleep(30);  /* 30ms delay needed as per HW design */
> +	value = readl(mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> +	value &= ~XPCS_WRAP_UPHY_RX_CONTROL_RX_CDR_RESET;
> +	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> +
>   	err = readl_poll_timeout(mgbe->xpcs + XPCS_WRAP_IRQ_STATUS, value,
>   				 value & XPCS_WRAP_IRQ_STATUS_PCS_LINK_STS,
>   				 500, 500 * 2000);


This might be a bit late now seeing as it is applied but ...

Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>

Thanks!
Jon

-- 
nvpublic