| Message ID | 20240916141931.742734-2-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] net: fec: Restart PPS after link state change | expand |
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 > >
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
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 >
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 */
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(+)