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

[PATCH] 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.

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>
---
 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..c81ae5f8fef4 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);
 
+	ndelay(50);  // 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);
 
+	ndelay(500);  // 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;
 	}
 
+	ndelay(50);  // 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);
 
+	ndelay(50);  // 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);
 
+	ndelay(50);  // 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] net: stmmac: dwmac-tegra: Fix link bring-up sequence

[Jon Hunter]

On 23/09/2024 14:44, 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.
> 
> 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>
> ---
>   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..c81ae5f8fef4 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);
>   
> +	ndelay(50);  // 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);
>   
> +	ndelay(500);  // 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;
>   	}
>   
> +	ndelay(50);  // 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);
>   
> +	ndelay(50);  // 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);
>   
> +	ndelay(50);  // 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);


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

Thanks!
Jon

-- 
nvpublic

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]

On Mon, Sep 23, 2024 at 09:44:10AM GMT, 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.

Did you do any testing with only these changes and without the delays?
Sounds to me like the sequence was blatantly wrong before, so maybe
fixing that already fixes the issue?

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

One reason why I'm hoping that's enough is because ndelay() isn't great.
For one it can return early, and it's also usually not very precise. If
I look at the boot log on a Tegra234 device, the architecture timer (off
of which the ndelay() implementation on arm64 runs) runs at 31.25 MHz so
that gives us around 32 ns of precision.

On the other hand, some of these delays are fairly long for busy loops.
I'm not too worried about those 50ns ones, but the 500ns is quite a long
time (from the point of view of a CPU).

All in all, I wonder if we wouldn't be better off increasing these
delays to the point where we can use usleep_range(). That will make
the overall lane bringup slightly longer (though it should still be well
below 1ms, so hardly noticeable from a user's perspective) but has the
benefit of not blocking the CPU during this time.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jon Hunter]

Hi Thierry,

On 25/09/2024 14:40, Thierry Reding wrote:
> On Mon, Sep 23, 2024 at 09:44:10AM GMT, 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.
> 
> Did you do any testing with only these changes and without the delays?
> Sounds to me like the sequence was blatantly wrong before, so maybe
> fixing that already fixes the issue?


Paritosh was able to confirm that the 30ms delay was the key one to 
fixing this specific issue. However, when we reviewed this with the 
design team the other delays and updates to the sequence were 
recommended. This has been implemented internally in the relevant 
drivers and so we wanted to align the upstream driver with this too. So 
we are trying to keep the sequence aligned.

>> - 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.
> 
> One reason why I'm hoping that's enough is because ndelay() isn't great.
> For one it can return early, and it's also usually not very precise. If
> I look at the boot log on a Tegra234 device, the architecture timer (off
> of which the ndelay() implementation on arm64 runs) runs at 31.25 MHz so
> that gives us around 32 ns of precision.
> 
> On the other hand, some of these delays are fairly long for busy loops.
> I'm not too worried about those 50ns ones, but the 500ns is quite a long
> time (from the point of view of a CPU).
> 
> All in all, I wonder if we wouldn't be better off increasing these
> delays to the point where we can use usleep_range(). That will make
> the overall lane bringup slightly longer (though it should still be well
> below 1ms, so hardly noticeable from a user's perspective) but has the
> benefit of not blocking the CPU during this time.


Yes we can certainly increase the delay and use usleep_range() as you 
prefer. Let us know what you would recommend here.

Cheers,
Jon

-- 
nvpublic

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

[Paolo Abeni]

On 9/27/24 17:28, Jon Hunter wrote:
> On 25/09/2024 14:40, Thierry Reding wrote:
>> All in all, I wonder if we wouldn't be better off increasing these
>> delays to the point where we can use usleep_range(). That will make
>> the overall lane bringup slightly longer (though it should still be well
>> below 1ms, so hardly noticeable from a user's perspective) but has the
>> benefit of not blocking the CPU during this time.
> 
> Yes we can certainly increase the delay and use usleep_range() as you
> prefer. Let us know what you would recommend here.

Use of usleep_range() would be definitely preferrable.

Additionally, please replace c99 comments '// ...' with ansi ones:
'/* ... */'

Thanks,

Paolo

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

[Jon Hunter]

On 01/10/2024 09:43, Paolo Abeni wrote:
> On 9/27/24 17:28, Jon Hunter wrote:
>> On 25/09/2024 14:40, Thierry Reding wrote:
>>> All in all, I wonder if we wouldn't be better off increasing these
>>> delays to the point where we can use usleep_range(). That will make
>>> the overall lane bringup slightly longer (though it should still be well
>>> below 1ms, so hardly noticeable from a user's perspective) but has the
>>> benefit of not blocking the CPU during this time.
>>
>> Yes we can certainly increase the delay and use usleep_range() as you
>> prefer. Let us know what you would recommend here.
> 
> Use of usleep_range() would be definitely preferrable.

Thanks for the feedback.

Thierry, let us know whether we should keep the 50/500ns ndelay() or 
switch to 10-20us usleep_range() as per the kernel documentation for 
less than 10us it says the typical guidance is to use udelay.

> Additionally, please replace c99 comments '// ...' with ansi ones:
> '/* ... */'

Paritosh, please make the above change for the comment style.

Thanks
Jon

-- 
nvpublic

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

On Mon, Oct 07, 2024 at 02:50:56PM GMT, Jon Hunter wrote:
> 
> On 01/10/2024 09:43, Paolo Abeni wrote:
> > On 9/27/24 17:28, Jon Hunter wrote:
> > > On 25/09/2024 14:40, Thierry Reding wrote:
> > > > All in all, I wonder if we wouldn't be better off increasing these
> > > > delays to the point where we can use usleep_range(). That will make
> > > > the overall lane bringup slightly longer (though it should still be well
> > > > below 1ms, so hardly noticeable from a user's perspective) but has the
> > > > benefit of not blocking the CPU during this time.
> > > 
> > > Yes we can certainly increase the delay and use usleep_range() as you
> > > prefer. Let us know what you would recommend here.
> > 
> > Use of usleep_range() would be definitely preferrable.
> 
> Thanks for the feedback.
> 
> Thierry, let us know whether we should keep the 50/500ns ndelay() or switch
> to 10-20us usleep_range() as per the kernel documentation for less than 10us
> it says the typical guidance is to use udelay.

Let's go with the usleep_range() if it works.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]