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

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

[Bernd Edlinger]

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

[Jiri Pirko]

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>

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

[Serge Semin]

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

[Bernd Edlinger]

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

[Serge Semin]

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

[Paolo Abeni]

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

[Bernd Edlinger]

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
> 

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

[Bernd Edlinger]

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.

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

Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
 1 file changed, 3 insertions(+)

v2: rebased to v6.7, retested and updated the commit message
as suggested Serge Semins review comment:
https://lore.kernel.org/lkml/b4mpa62b2juln47374x6xxnbozb7fcfgztrc5ounk4tvscs3wg@mixnvsoqno7j/
and retained Jiri Pirkos Reviwed-by from:
https://lore.kernel.org/lkml/ZT+Zq4j9iQj1+Xai@nanopsycho/


Thanks
Bernd.

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 37e64283f910..b8e8f6e963f2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7440,6 +7440,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 v2] net: stmmac: Wait a bit for the reset to take effect

[Paolo Abeni]

On Mon, 2024-01-15 at 20:21 +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.
> 
> > 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.
> 
> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
> 
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Please have a better look at the process documentation.

No empty lines are allowed in the tag area.

A fixes tag is requires, something alike:

Fixes: <blamed commit hash> ("<blamed commit title>")

A bisection is not strictly required, you just need to be reasonably
confident about the the culprit.

You need to include the relevant target tree into the subj prefix (in
this case 'net').

Please include in the recipients list the persons that provided
feedback on previous release (Serge is missing).

I'm unsure why/how Andrew landed in the recipients list!?!

Cheers,

Paolo

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

[Bernd Edlinger]

On 1/16/24 13:22, Paolo Abeni wrote:
>>
>> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
>>
>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> 
> Please have a better look at the process documentation.
> 

Yeah, I'm still new here.  Thanks for your patience...

> No empty lines are allowed in the tag area.
> 

Ah, okay, understood.

> A fixes tag is requires, something alike:
> 
> Fixes: <blamed commit hash> ("<blamed commit title>")
> 
> A bisection is not strictly required, you just need to be reasonably
> confident about the the culprit.
> 
> You need to include the relevant target tree into the subj prefix (in
> this case 'net').
> 

The subject line is now:
"net: stmmac: Wait a bit for the reset to take effect"

So what exactly should I use here, next time?

> Please include in the recipients list the persons that provided
> feedback on previous release (Serge is missing).
> 
> I'm unsure why/how Andrew landed in the recipients list!?!
> 

My mistake, sorry for the spam, Andrew.


Bernd.

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

[Bernd Edlinger]

On 1/16/24 13:22, Paolo Abeni wrote:
> 
> A fixes tag is requires, something alike:
> 
> Fixes: <blamed commit hash> ("<blamed commit title>")
> 
> A bisection is not strictly required, you just need to be reasonably
> confident about the the culprit.
> 

Okay, I think finally I found the commit that introduced
the broken reset logic:

commit c5e4ddbdfa1134a36589c1466ed4abb85fe6f976
Author: Chen-Yu Tsai <wens@csie.org>
Date:   Fri Jan 17 21:24:41 2014 +0800

    net: stmmac: Add support for optional reset control
    
    The DWMAC has a reset assert line, which is used on some SoCs. Add an
    optional reset control to stmmac driver core.
    
    To support reset control deferred probing, this patch changes the driver
    probe function to return the actual error, instead of just -EINVAL.
    
    Signed-off-by: Chen-Yu Tsai <wens@csie.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

that commit moved the reset de-assert next to the stmmac_hw_init function,
without any delay in between.

So I think I can now add
Fixes c5e4ddbdfa11 ("net: stmmac: Add support for optional reset control")

> You need to include the relevant target tree into the subj prefix (in
> this case 'net').

Will do, but please clarify how exactly I need to change the subject line.


Thanks
Bernd.

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

[Paolo Abeni]

On Fri, 2024-01-19 at 11:27 +0100, Bernd Edlinger wrote:
> On 1/16/24 13:22, Paolo Abeni wrote:
> 
> > You need to include the relevant target tree into the subj prefix (in
> > this case 'net').
> 
> Will do, but please clarify how exactly I need to change the subject line.

The prefix part should be alike: "[PATCH net v3]"

Cheers,

Paolo

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

[Andrew Lunn]

On Mon, Jan 15, 2024 at 08:21:42PM +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.
> 
> > 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

Is zero a valid synopsys_id? If its not, maybe do the wait conditional
on the first read returning 0?

   Andrew

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

[Bernd Edlinger]

On 1/16/24 23:35, Andrew Lunn wrote:
> On Mon, Jan 15, 2024 at 08:21:42PM +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.
>>
>>> 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
> 
> Is zero a valid synopsys_id? If its not, maybe do the wait conditional
> on the first read returning 0?
> 

I don't know at all.  And actually, I am more concerned that other registers
might be unreliable within the first microsecond after reset is de-asserted.

As I mentioned earlier the VHDL source code is obfuscated and I cannot
tell anything about it, maybe people from synopsys can shed some light
on the issue.


Thanks
Bernd.

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

[Jose Abreu]

From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Wed, Jan 17, 2024 at 16:48:22

> I don't know at all.  And actually, I am more concerned that other registers
> might be unreliable within the first microsecond after reset is de-asserted.

Are you guaranteeing that the documented PoR time is achieved before reading registers?

> As I mentioned earlier the VHDL source code is obfuscated and I cannot
> tell anything about it, maybe people from synopsys can shed some light
> on the issue.

This ID must always be present; it should be different than zero.

Thanks,
Jose

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

[Bernd Edlinger]

On 1/17/24 17:55, Jose Abreu wrote:
> From: Bernd Edlinger <bernd.edlinger@hotmail.de>
> Date: Wed, Jan 17, 2024 at 16:48:22
> 
>> I don't know at all.  And actually, I am more concerned that other registers
>> might be unreliable within the first microsecond after reset is de-asserted.
> 
> Are you guaranteeing that the documented PoR time is achieved before reading registers?
> 

Yes, that is the idea, why I added the udelay directly after releasing the reset,
thus simply delaying the execution of the stmmac_hw_init function, and not directly
where the synopsys_id register is accessed.


Thanks
Bernd.

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

[Jose Abreu]

From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Fri, Jan 19, 2024 at 07:15:31

> On 1/17/24 17:55, Jose Abreu wrote:
> > From: Bernd Edlinger <bernd.edlinger@hotmail.de>
> > Date: Wed, Jan 17, 2024 at 16:48:22
> > 
> >> I don't know at all.  And actually, I am more concerned that other registers
> >> might be unreliable within the first microsecond after reset is de-asserted.
> > 
> > Are you guaranteeing that the documented PoR time is achieved before reading registers?
> > 
> 
> Yes, that is the idea, why I added the udelay directly after releasing the reset,
> thus simply delaying the execution of the stmmac_hw_init function, and not directly
> where the synopsys_id register is accessed.

I understand your point, but the delay should be on reset function itself, since it depends
on the SoC that stmmac is integrated.

Please refer to reset_simple_reset(), where usleep_range() is used.

Thanks,
Jose

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

[Bernd Edlinger]

On 1/19/24 11:38, Jose Abreu wrote:
> I understand your point, but the delay should be on reset function itself, since it depends
> on the SoC that stmmac is integrated.
> 
> Please refer to reset_simple_reset(), where usleep_range() is used.
> 

Okay, in my case the SOC is an Altera CycloneV and reset control seems to be an altr,rst-mgr
which is indeed based on this reset_simple_reset.

So it implements reset_control_assert, reset_control_deassert, and reset_control_reset.
But the above mentioned delay affects only the width of the reset pulse that is generated
by the reset_control_reset method.

However if you look at the code in stmmac_dvr_proble where the reset pulse is generated,
you will see that the reset pulse is only generated with reset_control_assert/deassert:

        if (priv->plat->stmmac_rst) {
                ret = reset_control_assert(priv->plat->stmmac_rst);
                reset_control_deassert(priv->plat->stmmac_rst);
                /* Some reset controllers have only reset callback instead of
                 * assert + deassert callbacks pair.
                 */
                if (ret == -ENOTSUPP)
                        reset_control_reset(priv->plat->stmmac_rst);
        }

I don't know which reset controller that would be, where only a reset_control_reset is
available, but in my case ret == 0, and even if I could get the reset_control_reset
to be used, the issue is not the duration how long the reset line is in active state,
but the duration that is needed for the device to recover from the reset.


Thanks
Bernd.

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

[Jose Abreu]

From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Mon, Jan 22, 2024 at 06:41:46

> On 1/19/24 11:38, Jose Abreu wrote:
> > I understand your point, but the delay should be on reset function itself, since it depends
> > on the SoC that stmmac is integrated.
> > 
> > Please refer to reset_simple_reset(), where usleep_range() is used.
> > 
> 
> Okay, in my case the SOC is an Altera CycloneV and reset control seems to be an altr,rst-mgr
> which is indeed based on this reset_simple_reset.
> 
> So it implements reset_control_assert, reset_control_deassert, and reset_control_reset.
> But the above mentioned delay affects only the width of the reset pulse that is generated
> by the reset_control_reset method.
> 
> However if you look at the code in stmmac_dvr_proble where the reset pulse is generated,
> you will see that the reset pulse is only generated with reset_control_assert/deassert:
> 
>         if (priv->plat->stmmac_rst) {
>                 ret = reset_control_assert(priv->plat->stmmac_rst);
>                 reset_control_deassert(priv->plat->stmmac_rst);
>                 /* Some reset controllers have only reset callback instead of
>                  * assert + deassert callbacks pair.
>                  */
>                 if (ret == -ENOTSUPP)
>                         reset_control_reset(priv->plat->stmmac_rst);
>         }
> 
> I don't know which reset controller that would be, where only a reset_control_reset is
> available, but in my case ret == 0, and even if I could get the reset_control_reset
> to be used, the issue is not the duration how long the reset line is in active state,
> but the duration that is needed for the device to recover from the reset.

Sorry, I indeed missed the fact that on simple_reset the deassert is done
after the delay. But my point was that what you are facing is expected since
most of SoC chips need some time to recover from reset, and this time is
greatly dependent on integration' reference clock value (lower reference
values cause "recover" to take longer).

I have no objection to your patch since it's indeed a very small value, but
I believe reset framework and/or Altera' reset manager should take these delays
into account on deassert, since they are expected to happen.

Thanks,
Jose

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

[Bernd Edlinger]

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.

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

Fixes: c5e4ddbdfa11 ("net: stmmac: Add support for optional reset control")
Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
 1 file changed, 3 insertions(+)

v2: rebased to v6.7, retested and updated the commit message
as suggested Serge Semins review comment:
https://lore.kernel.org/lkml/b4mpa62b2juln47374x6xxnbozb7fcfgztrc5ounk4tvscs3wg@mixnvsoqno7j/
and retained Jiri Pirkos Reviwed-by from:
https://lore.kernel.org/lkml/ZT+Zq4j9iQj1+Xai@nanopsycho/

v3: addressed review comments.


Thanks
Bernd.

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a0e46369ae15..b334eb16da23 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7542,6 +7542,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 v3] net: stmmac: Wait a bit for the reset to take effect

[Serge Semin]

On Mon, Jan 22, 2024 at 07:19:09PM +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.
> 
> > 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.
> 
> Fixes: c5e4ddbdfa11 ("net: stmmac: Add support for optional reset control")
> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Thanks for taking all the notes into account. No objections from my
side:

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> v2: rebased to v6.7, retested and updated the commit message
> as suggested Serge Semins review comment:
> https://lore.kernel.org/lkml/b4mpa62b2juln47374x6xxnbozb7fcfgztrc5ounk4tvscs3wg@mixnvsoqno7j/
> and retained Jiri Pirkos Reviwed-by from:
> https://lore.kernel.org/lkml/ZT+Zq4j9iQj1+Xai@nanopsycho/
> 
> v3: addressed review comments.
> 
> 
> Thanks
> Bernd.
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index a0e46369ae15..b334eb16da23 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7542,6 +7542,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 v3] net: stmmac: Wait a bit for the reset to take effect

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 22 Jan 2024 19:19:09 +0100 you 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.
> 
> > From what have you got that delay value?
> 
> [...]

Here is the summary with links:
  - [net,v3] net: stmmac: Wait a bit for the reset to take effect
    https://git.kernel.org/netdev/net/c/a5f5eee282a0

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