[PATCH v2] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down

[PATCH v2] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down

[Csókás, Bence]

FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which
makes all 1588 functionality shut down on link-down. However, some
functionality needs to be retained (e.g. PPS) even without link.

Signed-off-by: "Csókás, Bence" <csokas.bence@prolan.hu>
---
 drivers/net/ethernet/freescale/fec_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 881ece735dcf..fb19295529a2 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1361,6 +1361,12 @@ fec_stop(struct net_device *ndev)
 		writel(FEC_ECR_ETHEREN, fep->hwp + FEC_ECNTRL);
 		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
 	}
+
+	if (fep->bufdesc_ex) {
+		val = readl(fep->hwp + FEC_ECNTRL);
+		val |= FEC_ECR_EN1588;
+		writel(val, fep->hwp + FEC_ECNTRL);
+	}
 }
 
 static void
-- 
2.34.1

[PATCH v2] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down

[Hariprasad Kelam]

> FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which makes
> all 1588 functionality shut down on link-down. However, some functionality
> needs to be retained (e.g. PPS) even without link.
> 


    Since this patch is targeted for net, please add fixes tag.


Thanks,
Hariprasad k
> Signed-off-by: "Csókás, Bence" <csokas.bence@prolan.hu>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 881ece735dcf..fb19295529a2 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1361,6 +1361,12 @@ fec_stop(struct net_device *ndev)
>  		writel(FEC_ECR_ETHEREN, fep->hwp + FEC_ECNTRL);
>  		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
>  	}
> +
> +	if (fep->bufdesc_ex) {
> +		val = readl(fep->hwp + FEC_ECNTRL);
> +		val |= FEC_ECR_EN1588;
> +		writel(val, fep->hwp + FEC_ECNTRL);
> +	}
>  }
> 
>  static void
> --
> 2.34.1
> 
> 

Re: [PATCH v2] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down

[Csókás Bence]

Hi!

On 6/7/24 10:32, Hariprasad Kelam wrote:
>> FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which makes
>> all 1588 functionality shut down on link-down. However, some functionality
>> needs to be retained (e.g. PPS) even without link.
>>
> 
> 
>      Since this patch is targeted for net, please add fixes tag.

This issue has existed for "practically all time". I guess if I had to 
pick one commit, it would be:

Fixes: 6605b730c061 FEC: Add time stamping code and a PTP hardware clock

I don't know if it makes sense to add this ancient commit from 22 years 
ago, but if so, then so be it.

Bence

Re: [PATCH v2] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down

[Simon Horman]

On Fri, Jun 07, 2024 at 11:12:56AM +0200, Csókás Bence wrote:
> Hi!
> 
> On 6/7/24 10:32, Hariprasad Kelam wrote:
> > > FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which makes
> > > all 1588 functionality shut down on link-down. However, some functionality
> > > needs to be retained (e.g. PPS) even without link.
> > > 
> > 
> > 
> >      Since this patch is targeted for net, please add fixes tag.
> 
> This issue has existed for "practically all time". I guess if I had to pick
> one commit, it would be:
> 
> Fixes: 6605b730c061 FEC: Add time stamping code and a PTP hardware clock
> 
> I don't know if it makes sense to add this ancient commit from 22 years ago,
> but if so, then so be it.

Thanks Csókás,

The practice is to use use even ancient commits in Fixes tags,
as it indicates how far back backports should go in theory,
even if backports don't go that far back in practice.

Re: [PATCH v2] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down

[Andrew Lunn]

On Fri, Jun 07, 2024 at 10:18:55AM +0200, Csókás, Bence wrote:
> FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which
> makes all 1588 functionality shut down on link-down. However, some
> functionality needs to be retained (e.g. PPS) even without link.

I don't know much about PPS. Could you point to some documentation,
list email etc, which indicated PPS without link is expected to work.

Please also Cc: Richard Cochran for changes like this.

Thanks
	Andrew

Re: [PATCH v2] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down

[Csókás Bence]

Hi!

On 6/10/24 21:13, Andrew Lunn wrote:
> On Fri, Jun 07, 2024 at 10:18:55AM +0200, Csókás, Bence wrote:
>> FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which
>> makes all 1588 functionality shut down on link-down. However, some
>> functionality needs to be retained (e.g. PPS) even without link.
> 
> I don't know much about PPS. Could you point to some documentation,
> list email etc, which indicated PPS without link is expected to work.
> 
> Please also Cc: Richard Cochran for changes like this.
> 
> Thanks
> 	Andrew

This is what Richard said two years ago on the now-reverted patch:

Link: https://lore.kernel.org/netdev/YvRdTwRM4JBc5RuV@hoboy.vegasvil.org

 > IMO the least surprising behavior is that once enabled, a feature
 > stays on until explicitly disabled.

The point of PPS is, it is a 1 Hz clock reference. We don't want to take 
away a clock signal because of an unexpected link fault, for instance.

Plus, this patch doesn't even re-enable PPS or any other 1588 functions, 
it just prevents the adapter from forgetting it is even 1588-capable. 
I'll resubmit with more clear wording and appropriate "Fixes:" and "Cc:" 
tags.

Bence

Re: [PATCH v2] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down

[Andrew Lunn]

On Tue, Jun 11, 2024 at 10:04:39AM +0200, Csókás Bence wrote:
> Hi!
> 
> On 6/10/24 21:13, Andrew Lunn wrote:
> > On Fri, Jun 07, 2024 at 10:18:55AM +0200, Csókás, Bence wrote:
> > > FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which
> > > makes all 1588 functionality shut down on link-down. However, some
> > > functionality needs to be retained (e.g. PPS) even without link.
> > 
> > I don't know much about PPS. Could you point to some documentation,
> > list email etc, which indicated PPS without link is expected to work.
> > 
> > Please also Cc: Richard Cochran for changes like this.
> > 
> > Thanks
> > 	Andrew
> 
> This is what Richard said two years ago on the now-reverted patch:
> 
> Link: https://lore.kernel.org/netdev/YvRdTwRM4JBc5RuV@hoboy.vegasvil.org

Thanks.

So when you have sync, you have a 1Hz clock, synchronised to the grand
master. When the link is down, or communication with the grand master
is lost, you get a free running clock of around 1Hz. I presume that if
the link does up again and communication to the grand master is
restored, there is a phase shift in the 1Hz clock, and a frequency
correction? The hardware has to cope with this.

> Plus, this patch doesn't even re-enable PPS or any other 1588 functions, it
> just prevents the adapter from forgetting it is even 1588-capable. I'll
> resubmit with more clear wording and appropriate "Fixes:" and "Cc:" tags.

Thanks

	Andrew

Re: [PATCH v2] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down

[Csókás Bence]

Hi!

On 6/11/24 15:06, Andrew Lunn wrote:
> On Tue, Jun 11, 2024 at 10:04:39AM +0200, Csókás Bence wrote:
>> Hi!
>>
>> On 6/10/24 21:13, Andrew Lunn wrote:
>>> On Fri, Jun 07, 2024 at 10:18:55AM +0200, Csókás, Bence wrote:
>>>> FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which
>>>> makes all 1588 functionality shut down on link-down. However, some
>>>> functionality needs to be retained (e.g. PPS) even without link.
>>>
>>> I don't know much about PPS. Could you point to some documentation,
>>> list email etc, which indicated PPS without link is expected to work.
>>>
>>> Please also Cc: Richard Cochran for changes like this.
>>>
>>> Thanks
>>> 	Andrew
>>
>> This is what Richard said two years ago on the now-reverted patch:
>>
>> Link: https://lore.kernel.org/netdev/YvRdTwRM4JBc5RuV@hoboy.vegasvil.org
> 
> Thanks.
> 
> So when you have sync, you have a 1Hz clock, synchronised to the grand
> master. When the link is down, or communication with the grand master
> is lost, you get a free running clock of around 1Hz. I presume that if
> the link does up again and communication to the grand master is
> restored, there is a phase shift in the 1Hz clock, and a frequency
> correction? The hardware has to cope with this.

Correct. And PTP4Linux is already capable of re-syncing (or at least it 
would be, if it weren't for the ENET controller reset. We still need to 
restore all the values of ATIME, ATCORR etc. registers, but that'll come 
in a future patch, when I figure out how to get out of the locking hell).

> Thanks
> 
> 	Andrew
> 

Bence