net: stmmac: Wait a bit for the reset to take effect

[PATCH] net: stmmac: Wait a bit for the reset to take effect

Posted by Bernd Edlinger 2 years, 2 months ago

otherwise the synopsys_id value may be read out wrong,
because the GMAC_VERSION register might still be in reset
state, for at least 1 us after the reset is de-asserted.

Add a wait for 10 us before continuing to be on the safe side.

Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 5801f4d50f95..e485f4db3605 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7398,6 +7398,9 @@ int stmmac_dvr_probe(struct device *device,
 		dev_err(priv->device, "unable to bring out of ahb reset: %pe\n",
 			ERR_PTR(ret));
 
+	/* Wait a bit for the reset to take effect */
+	udelay(10);
+
 	/* Init MAC and get the capabilities */
 	ret = stmmac_hw_init(priv);
 	if (ret)
-- 
2.39.2

Re: [PATCH] net: stmmac: Wait a bit for the reset to take effect

Posted by Paolo Abeni 2 years, 1 month ago

On Mon, 2023-10-30 at 07:01 +0100, Bernd Edlinger wrote:
> otherwise the synopsys_id value may be read out wrong,
> because the GMAC_VERSION register might still be in reset
> state, for at least 1 us after the reset is de-asserted.
> 
> Add a wait for 10 us before continuing to be on the safe side.

This looks like a bugfix: you should target explicitly the 'net' tree,
adding such tag into the subj. More importantly you should include a
suitable 'Fixes' tag.

Please send a new revision with the above changes. You can retain the
already collected reviewed tags.

Thanks,

Paolo

Re: [PATCH] net: stmmac: Wait a bit for the reset to take effect

Posted by Bernd Edlinger 2 years, 1 month ago

On 11/2/23 12:25, Paolo Abeni wrote:
> On Mon, 2023-10-30 at 07:01 +0100, Bernd Edlinger wrote:
>> otherwise the synopsys_id value may be read out wrong,
>> because the GMAC_VERSION register might still be in reset
>> state, for at least 1 us after the reset is de-asserted.
>>
>> Add a wait for 10 us before continuing to be on the safe side.
> 
> This looks like a bugfix: you should target explicitly the 'net' tree,
I dont understand, did you mean to add "net:" to the subject?
It is already "net: stmmac: ..." or did you mean to send this message to
another mailing list?

> adding such tag into the subj. More importantly you should include a
> suitable 'Fixes' tag.
> 

You mean an informal description of what this fixes?
like
Fixes: Randomly occurring "Version ID not available" messages.

Or do mean to pick a commit where the error was introduced?  I doubt
I am able to do the necessary bisection steps, as this seems like an
issue that was introduced by the initial design.  And I also doubt that
it can only affect the GMAC_VERSION register.
To make that clear, the issue is totally harmless, maybe some performance
degradation but I became aware of it only because I was trying to solve
a totally different issue, and therefore I have looked very closely at the
printk messages, so I spotted the anomaly to tracked it down the reason for
this flaw.

Thanks
Bernd.

> Please send a new revision with the above changes. You can retain the
> already collected reviewed tags.
> 
> Thanks,
> 
> Paolo
>

Re: [PATCH] net: stmmac: Wait a bit for the reset to take effect

Posted by Serge Semin 2 years, 2 months ago

On Mon, Oct 30, 2023 at 07:01:11AM +0100, Bernd Edlinger wrote:
> otherwise the synopsys_id value may be read out wrong,
> because the GMAC_VERSION register might still be in reset
> state, for at least 1 us after the reset is de-asserted.

From what have you got that delay value?

-Serge(y)

> 
> Add a wait for 10 us before continuing to be on the safe side.
> 
> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 5801f4d50f95..e485f4db3605 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7398,6 +7398,9 @@ int stmmac_dvr_probe(struct device *device,
>  		dev_err(priv->device, "unable to bring out of ahb reset: %pe\n",
>  			ERR_PTR(ret));
>  
> +	/* Wait a bit for the reset to take effect */
> +	udelay(10);
> +
>  	/* Init MAC and get the capabilities */
>  	ret = stmmac_hw_init(priv);
>  	if (ret)
> -- 
> 2.39.2
> 
>

Re: [PATCH] net: stmmac: Wait a bit for the reset to take effect

Posted by Bernd Edlinger 2 years, 2 months ago

On 10/31/23 11:32, Serge Semin wrote:
> On Mon, Oct 30, 2023 at 07:01:11AM +0100, Bernd Edlinger wrote:
>> otherwise the synopsys_id value may be read out wrong,
>> because the GMAC_VERSION register might still be in reset
>> state, for at least 1 us after the reset is de-asserted.
> 
> From what have you got that delay value?
> 

Just try and error, with very old linux versions and old gcc versions
the synopsys_id was read out correctly most of the time (but not always),
with recent linux versions and recnet gcc versions it was read out
wrongly most of the time, but again not always.
I don't have access to the VHDL code in question, so I cannot
tell why it takes so long to get the correct values, I also do not
have more than a few hardware samples, so I cannot tell how long
this timeout must be in worst case.
Experimentally I can tell that the register is read several times
as zero immediately after the reset is de-asserted, also adding several
no-ops is not enough, adding a printk is enough, also udelay(1) seems to
be enough but I tried that not very often, and I have not access to many
hardware samples to be 100% sure about the necessary delay.
And since the udelay here is only executed once per device instance,
it seems acceptable to delay the boot for 10 us.

BTW: my hardware's synopsys id is 0x37.

Bernd.

> -Serge(y)
> 
>>
>> Add a wait for 10 us before continuing to be on the safe side.
>>
>> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 5801f4d50f95..e485f4db3605 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -7398,6 +7398,9 @@ int stmmac_dvr_probe(struct device *device,
>>  		dev_err(priv->device, "unable to bring out of ahb reset: %pe\n",
>>  			ERR_PTR(ret));
>>  
>> +	/* Wait a bit for the reset to take effect */
>> +	udelay(10);
>> +
>>  	/* Init MAC and get the capabilities */
>>  	ret = stmmac_hw_init(priv);
>>  	if (ret)
>> -- 
>> 2.39.2
>>
>>

Re: [PATCH] net: stmmac: Wait a bit for the reset to take effect

Posted by Serge Semin 2 years, 1 month ago

On Tue, Oct 31, 2023 at 05:10:24PM +0100, Bernd Edlinger wrote:
> 
> 
> On 10/31/23 11:32, Serge Semin wrote:
> > On Mon, Oct 30, 2023 at 07:01:11AM +0100, Bernd Edlinger wrote:
> >> otherwise the synopsys_id value may be read out wrong,
> >> because the GMAC_VERSION register might still be in reset
> >> state, for at least 1 us after the reset is de-asserted.
> > 
> > From what have you got that delay value?
> > 
> 
> Just try and error, with very old linux versions and old gcc versions
> the synopsys_id was read out correctly most of the time (but not always),
> with recent linux versions and recnet gcc versions it was read out
> wrongly most of the time, but again not always.
> I don't have access to the VHDL code in question, so I cannot
> tell why it takes so long to get the correct values, I also do not
> have more than a few hardware samples, so I cannot tell how long
> this timeout must be in worst case.
> Experimentally I can tell that the register is read several times
> as zero immediately after the reset is de-asserted, also adding several
> no-ops is not enough, adding a printk is enough, also udelay(1) seems to
> be enough but I tried that not very often, and I have not access to many
> hardware samples to be 100% sure about the necessary delay.
> And since the udelay here is only executed once per device instance,
> it seems acceptable to delay the boot for 10 us.
> 
> BTW: my hardware's synopsys id is 0x37.

Well, the delay value is highly hardware-dependent depending on the
IP-core version, generation (MAC1000, GMAC, QoS Eth, XGMAC, XLGMAC,
etc), IP-core synthesize parameters and finally the platform-specific
ref clocks implementation and their rates. So no matter how many you
try to figure out a safest value I guess you won't be able to find out
the common value for all the devices. Though seeing nobody has
reported so far any problem with that then it seems rare among the DW
*MAC* devices. So since you get to a add a very small delay in just a
perf non-critical path it won't hurt for the rest of the platforms.
But please very thoroughly define the problem in the commit message:
what hardware you have (SoC, platform, etc), in what conditions you
see the problem (what you already described in your reply to me), how
you've got to the 10us value, etc. It will be useful in case if
somebody would want for instance make the delay platform-dependent or
whatever.

-Serge(y)

> 
> 
> Bernd.
> 
> > -Serge(y)
> > 
> >>
> >> Add a wait for 10 us before continuing to be on the safe side.
> >>
> >> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
> >> ---
> >>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> index 5801f4d50f95..e485f4db3605 100644
> >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> @@ -7398,6 +7398,9 @@ int stmmac_dvr_probe(struct device *device,
> >>  		dev_err(priv->device, "unable to bring out of ahb reset: %pe\n",
> >>  			ERR_PTR(ret));
> >>  
> >> +	/* Wait a bit for the reset to take effect */
> >> +	udelay(10);
> >> +
> >>  	/* Init MAC and get the capabilities */
> >>  	ret = stmmac_hw_init(priv);
> >>  	if (ret)
> >> -- 
> >> 2.39.2
> >>
> >>

Re: [PATCH] net: stmmac: Wait a bit for the reset to take effect

Posted by Jiri Pirko 2 years, 2 months ago

Mon, Oct 30, 2023 at 07:01:11AM CET, bernd.edlinger@hotmail.de wrote:
>otherwise the synopsys_id value may be read out wrong,
>because the GMAC_VERSION register might still be in reset
>state, for at least 1 us after the reset is de-asserted.
>
>Add a wait for 10 us before continuing to be on the safe side.
>
>Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>