* [PATCH 1/2] net: fec: Restart PPS after link state change
@ 2024-09-16 14:19 Csókás, Bence
2024-09-16 14:19 ` [PATCH 2/2] net: fec: Reload PTP registers after link-state change Csókás, Bence
2024-09-19 8:18 ` [PATCH 1/2] net: fec: Restart PPS after link state change Paolo Abeni
0 siblings, 2 replies; 6+ messages in thread
From: Csókás, Bence @ 2024-09-16 14:19 UTC (permalink / raw)
To: imx, netdev, linux-kernel
Cc: Csókás, Bence, Wei Fang, Shenwei Wang, Clark Wang,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Richard Cochran
On link state change, the controller gets reset,
causing PPS to drop out. Re-enable PPS if it was
enabled before the controller reset.
Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
---
drivers/net/ethernet/freescale/fec.h | 1 +
drivers/net/ethernet/freescale/fec_main.c | 7 ++++++-
drivers/net/ethernet/freescale/fec_ptp.c | 12 ++++++++++++
3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index a19cb2a786fd..afa0bfb974e6 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -695,6 +695,7 @@ struct fec_enet_private {
};
void fec_ptp_init(struct platform_device *pdev, int irq_idx);
+void fec_ptp_restore_state(struct fec_enet_private *fep);
void fec_ptp_stop(struct platform_device *pdev);
void fec_ptp_start_cyclecounter(struct net_device *ndev);
int fec_ptp_set(struct net_device *ndev, struct kernel_hwtstamp_config *config,
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index a923cb95cdc6..531b51091e7d 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1244,8 +1244,10 @@ fec_restart(struct net_device *ndev)
writel(ecntl, fep->hwp + FEC_ECNTRL);
fec_enet_active_rxring(ndev);
- if (fep->bufdesc_ex)
+ if (fep->bufdesc_ex) {
fec_ptp_start_cyclecounter(ndev);
+ fec_ptp_restore_state(fep);
+ }
/* Enable interrupts we wish to service */
if (fep->link)
@@ -1366,6 +1368,9 @@ fec_stop(struct net_device *ndev)
val = readl(fep->hwp + FEC_ECNTRL);
val |= FEC_ECR_EN1588;
writel(val, fep->hwp + FEC_ECNTRL);
+
+ fec_ptp_start_cyclecounter(ndev);
+ fec_ptp_restore_state(fep);
}
}
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index e32f6724f568..c5b89352373a 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -770,6 +770,18 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
schedule_delayed_work(&fep->time_keep, HZ);
}
+/* Restore PTP functionality after a reset */
+void fec_ptp_restore_state(struct fec_enet_private *fep)
+{
+ /* Restart PPS if needed */
+ if (fep->pps_enable) {
+ /* Reset turned it off, so adjust our status flag */
+ fep->pps_enable = 0;
+ /* Re-enable PPS */
+ fec_ptp_enable_pps(fep, 1);
+ }
+}
+
void fec_ptp_stop(struct platform_device *pdev)
{
struct net_device *ndev = platform_get_drvdata(pdev);
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] net: fec: Reload PTP registers after link-state change
2024-09-16 14:19 [PATCH 1/2] net: fec: Restart PPS after link state change Csókás, Bence
@ 2024-09-16 14:19 ` Csókás, Bence
2024-09-16 15:05 ` Frank Li
2024-09-19 8:18 ` [PATCH 1/2] net: fec: Restart PPS after link state change Paolo Abeni
1 sibling, 1 reply; 6+ messages in thread
From: Csókás, Bence @ 2024-09-16 14:19 UTC (permalink / raw)
To: imx, netdev, linux-kernel
Cc: Csókás, Bence, Wei Fang, Shenwei Wang, Clark Wang,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Richard Cochran
On link-state change, the controller gets reset,
which clears all PTP registers, including PHC time,
calibrated clock correction values etc. For correct
IEEE 1588 operation we need to restore these after
the reset.
Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
---
drivers/net/ethernet/freescale/fec.h | 7 +++++
drivers/net/ethernet/freescale/fec_main.c | 4 +++
drivers/net/ethernet/freescale/fec_ptp.c | 35 +++++++++++++++++++++++
3 files changed, 46 insertions(+)
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index afa0bfb974e6..efe770fe337d 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -691,11 +691,18 @@ struct fec_enet_private {
/* XDP BPF Program */
struct bpf_prog *xdp_prog;
+ struct {
+ u64 ns_sys, ns_phc;
+ u32 at_corr;
+ u8 at_inc_corr;
+ } ptp_saved_state;
+
u64 ethtool_stats[];
};
void fec_ptp_init(struct platform_device *pdev, int irq_idx);
void fec_ptp_restore_state(struct fec_enet_private *fep);
+void fec_ptp_save_state(struct fec_enet_private *fep);
void fec_ptp_stop(struct platform_device *pdev);
void fec_ptp_start_cyclecounter(struct net_device *ndev);
int fec_ptp_set(struct net_device *ndev, struct kernel_hwtstamp_config *config,
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 531b51091e7d..570f8a14d975 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1077,6 +1077,8 @@ fec_restart(struct net_device *ndev)
u32 rcntl = OPT_FRAME_SIZE | 0x04;
u32 ecntl = FEC_ECR_ETHEREN;
+ fec_ptp_save_state(fep);
+
/* Whack a reset. We should wait for this.
* For i.MX6SX SOC, enet use AXI bus, we use disable MAC
* instead of reset MAC itself.
@@ -1338,6 +1340,8 @@ fec_stop(struct net_device *ndev)
netdev_err(ndev, "Graceful transmit stop did not complete!\n");
}
+ fec_ptp_save_state(fep);
+
/* Whack a reset. We should wait for this.
* For i.MX6SX SOC, enet use AXI bus, we use disable MAC
* instead of reset MAC itself.
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index c5b89352373a..8011a6f3c4be 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -770,9 +770,44 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
schedule_delayed_work(&fep->time_keep, HZ);
}
+void fec_ptp_save_state(struct fec_enet_private *fep)
+{
+ unsigned long flags;
+ u32 atime_inc_corr;
+
+ spin_lock_irqsave(&fep->tmreg_lock, flags);
+
+ fep->ptp_saved_state.ns_phc = timecounter_read(&fep->tc);
+ fep->ptp_saved_state.ns_sys = ktime_get_ns();
+
+ fep->ptp_saved_state.at_corr = readl(fep->hwp + FEC_ATIME_CORR);
+ atime_inc_corr = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_CORR_MASK;
+ fep->ptp_saved_state.at_inc_corr = (u8)(atime_inc_corr >> FEC_T_INC_CORR_OFFSET);
+
+ spin_unlock_irqrestore(&fep->tmreg_lock, flags);
+}
+
/* Restore PTP functionality after a reset */
void fec_ptp_restore_state(struct fec_enet_private *fep)
{
+ u32 atime_inc = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK;
+ unsigned long flags;
+ u32 counter;
+ u64 ns;
+
+ spin_lock_irqsave(&fep->tmreg_lock, flags);
+
+ writel(fep->ptp_saved_state.at_corr, fep->hwp + FEC_ATIME_CORR);
+ atime_inc |= ((u32)fep->ptp_saved_state.at_inc_corr) << FEC_T_INC_CORR_OFFSET;
+ writel(atime_inc, fep->hwp + FEC_ATIME_INC);
+
+ ns = ktime_get_ns() - fep->ptp_saved_state.ns_sys + fep->ptp_saved_state.ns_phc;
+ counter = ns & fep->cc.mask;
+ writel(counter, fep->hwp + FEC_ATIME);
+ timecounter_init(&fep->tc, &fep->cc, ns);
+
+ spin_unlock_irqrestore(&fep->tmreg_lock, flags);
+
/* Restart PPS if needed */
if (fep->pps_enable) {
/* Reset turned it off, so adjust our status flag */
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] net: fec: Reload PTP registers after link-state change
2024-09-16 14:19 ` [PATCH 2/2] net: fec: Reload PTP registers after link-state change Csókás, Bence
@ 2024-09-16 15:05 ` Frank Li
2024-09-17 7:53 ` Csókás Bence
0 siblings, 1 reply; 6+ messages in thread
From: Frank Li @ 2024-09-16 15:05 UTC (permalink / raw)
To: Csókás, Bence
Cc: imx, netdev, linux-kernel, Wei Fang, Shenwei Wang, Clark Wang,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Richard Cochran
On Mon, Sep 16, 2024 at 04:19:31PM +0200, Csókás, Bence wrote:
> On link-state change, the controller gets reset,
> which clears all PTP registers, including PHC time,
> calibrated clock correction values etc. For correct
> IEEE 1588 operation we need to restore these after
> the reset.
I am not sure if it necessary. timer will be big offset after reset. ptpd
should set_time then do clock frequency adjust, supposed just few ms, ptp
time will get resync.
of course, restore these value may reduce the resync time.
Frank
>
> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
> ---
> drivers/net/ethernet/freescale/fec.h | 7 +++++
> drivers/net/ethernet/freescale/fec_main.c | 4 +++
> drivers/net/ethernet/freescale/fec_ptp.c | 35 +++++++++++++++++++++++
> 3 files changed, 46 insertions(+)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index afa0bfb974e6..efe770fe337d 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -691,11 +691,18 @@ struct fec_enet_private {
> /* XDP BPF Program */
> struct bpf_prog *xdp_prog;
>
> + struct {
> + u64 ns_sys, ns_phc;
> + u32 at_corr;
> + u8 at_inc_corr;
> + } ptp_saved_state;
> +
> u64 ethtool_stats[];
> };
>
> void fec_ptp_init(struct platform_device *pdev, int irq_idx);
> void fec_ptp_restore_state(struct fec_enet_private *fep);
> +void fec_ptp_save_state(struct fec_enet_private *fep);
> void fec_ptp_stop(struct platform_device *pdev);
> void fec_ptp_start_cyclecounter(struct net_device *ndev);
> int fec_ptp_set(struct net_device *ndev, struct kernel_hwtstamp_config *config,
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 531b51091e7d..570f8a14d975 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1077,6 +1077,8 @@ fec_restart(struct net_device *ndev)
> u32 rcntl = OPT_FRAME_SIZE | 0x04;
> u32 ecntl = FEC_ECR_ETHEREN;
>
> + fec_ptp_save_state(fep);
> +
> /* Whack a reset. We should wait for this.
> * For i.MX6SX SOC, enet use AXI bus, we use disable MAC
> * instead of reset MAC itself.
> @@ -1338,6 +1340,8 @@ fec_stop(struct net_device *ndev)
> netdev_err(ndev, "Graceful transmit stop did not complete!\n");
> }
>
> + fec_ptp_save_state(fep);
> +
> /* Whack a reset. We should wait for this.
> * For i.MX6SX SOC, enet use AXI bus, we use disable MAC
> * instead of reset MAC itself.
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index c5b89352373a..8011a6f3c4be 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -770,9 +770,44 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
> schedule_delayed_work(&fep->time_keep, HZ);
> }
>
> +void fec_ptp_save_state(struct fec_enet_private *fep)
> +{
> + unsigned long flags;
> + u32 atime_inc_corr;
> +
> + spin_lock_irqsave(&fep->tmreg_lock, flags);
> +
> + fep->ptp_saved_state.ns_phc = timecounter_read(&fep->tc);
> + fep->ptp_saved_state.ns_sys = ktime_get_ns();
> +
> + fep->ptp_saved_state.at_corr = readl(fep->hwp + FEC_ATIME_CORR);
> + atime_inc_corr = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_CORR_MASK;
> + fep->ptp_saved_state.at_inc_corr = (u8)(atime_inc_corr >> FEC_T_INC_CORR_OFFSET);
> +
> + spin_unlock_irqrestore(&fep->tmreg_lock, flags);
> +}
> +
> /* Restore PTP functionality after a reset */
> void fec_ptp_restore_state(struct fec_enet_private *fep)
> {
> + u32 atime_inc = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK;
> + unsigned long flags;
> + u32 counter;
> + u64 ns;
> +
> + spin_lock_irqsave(&fep->tmreg_lock, flags);
> +
> + writel(fep->ptp_saved_state.at_corr, fep->hwp + FEC_ATIME_CORR);
> + atime_inc |= ((u32)fep->ptp_saved_state.at_inc_corr) << FEC_T_INC_CORR_OFFSET;
> + writel(atime_inc, fep->hwp + FEC_ATIME_INC);
> +
> + ns = ktime_get_ns() - fep->ptp_saved_state.ns_sys + fep->ptp_saved_state.ns_phc;
> + counter = ns & fep->cc.mask;
> + writel(counter, fep->hwp + FEC_ATIME);
> + timecounter_init(&fep->tc, &fep->cc, ns);
> +
> + spin_unlock_irqrestore(&fep->tmreg_lock, flags);
> +
> /* Restart PPS if needed */
> if (fep->pps_enable) {
> /* Reset turned it off, so adjust our status flag */
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] net: fec: Reload PTP registers after link-state change
2024-09-16 15:05 ` Frank Li
@ 2024-09-17 7:53 ` Csókás Bence
2024-09-17 15:23 ` Frank Li
0 siblings, 1 reply; 6+ messages in thread
From: Csókás Bence @ 2024-09-17 7:53 UTC (permalink / raw)
To: Frank Li
Cc: imx, netdev, linux-kernel, Wei Fang, Shenwei Wang, Clark Wang,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Richard Cochran
Hi!
On 9/16/24 17:05, Frank Li wrote:
> On Mon, Sep 16, 2024 at 04:19:31PM +0200, Csókás, Bence wrote:
>> On link-state change, the controller gets reset,
>> which clears all PTP registers, including PHC time,
>> calibrated clock correction values etc. For correct
>> IEEE 1588 operation we need to restore these after
>> the reset.
>
> I am not sure if it necessary. timer will be big offset after reset. ptpd
> should set_time then do clock frequency adjust, supposed just few ms, ptp
> time will get resync.
>
> of course, restore these value may reduce the resync time.
>
> Frank
There's 3 problems with that:
1. ATCORR, ATINC and ATPER will not be restored, therefore precision
will be immediately lost.
2. ptpd does NOT set the time, only once, on startup. Currently, on
link-down, ptpd tries to correct for the missing 54 years by making the
PHC tick 3% faster (therefore the PPS signal will have a frequency error
as well), which will never get it there. One work-around is to
periodically re-start ptpd, but this is obviously sub-optimal.
3. If the PTP server goes away, there's no way to restore the time.
Whereas if you save and reload it, you can continue, although with
degraded precision.
Bence
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] net: fec: Reload PTP registers after link-state change
2024-09-17 7:53 ` Csókás Bence
@ 2024-09-17 15:23 ` Frank Li
0 siblings, 0 replies; 6+ messages in thread
From: Frank Li @ 2024-09-17 15:23 UTC (permalink / raw)
To: Csókás Bence
Cc: imx, netdev, linux-kernel, Wei Fang, Shenwei Wang, Clark Wang,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Richard Cochran
On Tue, Sep 17, 2024 at 09:53:07AM +0200, Csókás Bence wrote:
> Hi!
>
> On 9/16/24 17:05, Frank Li wrote:
> > On Mon, Sep 16, 2024 at 04:19:31PM +0200, Csókás, Bence wrote:
> > > On link-state change, the controller gets reset,
> > > which clears all PTP registers, including PHC time,
> > > calibrated clock correction values etc. For correct
> > > IEEE 1588 operation we need to restore these after
> > > the reset.
> >
> > I am not sure if it necessary. timer will be big offset after reset. ptpd
> > should set_time then do clock frequency adjust, supposed just few ms, ptp
> > time will get resync.
> >
> > of course, restore these value may reduce the resync time.
> >
> > Frank
>
> There's 3 problems with that:
> 1. ATCORR, ATINC and ATPER will not be restored, therefore precision will be
> immediately lost.
Yes, It will be good if you have compare time back to sync with/without
save and restore.
I think ptpd always to make it sync again, just take little bit longer.
> 2. ptpd does NOT set the time, only once, on startup. Currently, on
> link-down, ptpd tries to correct for the missing 54 years by making the PHC
> tick 3% faster (therefore the PPS signal will have a frequency error as
> well), which will never get it there. One work-around is to periodically
> re-start ptpd, but this is obviously sub-optimal.
Supposed it should be ptpd bug. I remember time draft is bigger enough, it
should be set the time again. Maybe ptp change the policy.
For example, if system suspend 1min and resume back, absolute offset will
be 1min offset. ptpd should set the time to catch up offset firstly, then
resync frequency.
> 3. If the PTP server goes away, there's no way to restore the time. Whereas
> if you save and reload it, you can continue, although with degraded
> precision.
This one make sense, you can add to commit message.
>
> Bence
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] net: fec: Restart PPS after link state change
2024-09-16 14:19 [PATCH 1/2] net: fec: Restart PPS after link state change Csókás, Bence
2024-09-16 14:19 ` [PATCH 2/2] net: fec: Reload PTP registers after link-state change Csókás, Bence
@ 2024-09-19 8:18 ` Paolo Abeni
1 sibling, 0 replies; 6+ messages in thread
From: Paolo Abeni @ 2024-09-19 8:18 UTC (permalink / raw)
To: Csókás, Bence, imx, netdev, linux-kernel
Cc: Wei Fang, Shenwei Wang, Clark Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Richard Cochran
On 9/16/24 16:19, Csókás, Bence wrote:
> On link state change, the controller gets reset,
> causing PPS to drop out. Re-enable PPS if it was
> enabled before the controller reset.
>
> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
This and the next patch look like a bugfix, as such they should target
the 'net' tree:
https://elixir.bootlin.com/linux/v6.11/source/Documentation/process/maintainer-netdev.rst#L64
and you should include suitable fixes tag in each of them.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-09-19 8:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-16 14:19 [PATCH 1/2] net: fec: Restart PPS after link state change Csókás, Bence
2024-09-16 14:19 ` [PATCH 2/2] net: fec: Reload PTP registers after link-state change Csókás, Bence
2024-09-16 15:05 ` Frank Li
2024-09-17 7:53 ` Csókás Bence
2024-09-17 15:23 ` Frank Li
2024-09-19 8:18 ` [PATCH 1/2] net: fec: Restart PPS after link state change Paolo Abeni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox