| Message ID | 20240924093705.2897329-1-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net,1/2] net: fec: Restart PPS after link state change | expand |
> -----Original Message----- > From: Csókás, Bence <csokas.bence@prolan.hu> > Sent: 2024年9月24日 17:37 > To: Frank Li <Frank.Li@freescale.com>; David S. Miller > <davem@davemloft.net>; imx@lists.linux.dev; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org > Cc: Csókás, Bence <csokas.bence@prolan.hu>; Wei Fang <wei.fang@nxp.com>; > Shenwei Wang <shenwei.wang@nxp.com>; Clark Wang > <xiaoning.wang@nxp.com>; Eric Dumazet <edumazet@google.com>; Jakub > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard > Cochran <richardcochran@gmail.com> > Subject: [PATCH net 1/2] net: fec: Restart PPS after link state change > The "v2" descriptor is missing in the subject, and correct the mail address of Frank. > On link state change, the controller gets reset, causing PPS to drop out. > Re-enable PPS if it was enabled before the controller reset. > > Fixes: 6605b730c061 ("FEC: Add time stamping code and a PTP hardware > clock") > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> > --- > > Notes: > Changes in v2: > - store pps_enable's pre-reset value and clear it on restore > - acquire tmreg_lock when reading/writing fep->pps_enable > > drivers/net/ethernet/freescale/fec.h | 6 +++++ > drivers/net/ethernet/freescale/fec_main.c | 11 ++++++++- > drivers/net/ethernet/freescale/fec_ptp.c | 30 +++++++++++++++++++++++ > 3 files changed, 46 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/freescale/fec.h > b/drivers/net/ethernet/freescale/fec.h > index a19cb2a786fd..0552317a2554 100644 > --- a/drivers/net/ethernet/freescale/fec.h > +++ b/drivers/net/ethernet/freescale/fec.h > @@ -691,10 +691,16 @@ struct fec_enet_private { > /* XDP BPF Program */ > struct bpf_prog *xdp_prog; > > + struct { > + int pps_enable; > + } 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 acbb627d51bf..31ebf6a4f973 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. > @@ -1244,8 +1246,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) > @@ -1336,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. > @@ -1366,6 +1372,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 4cffda363a14..df1ef023493b 100644 > --- a/drivers/net/ethernet/freescale/fec_ptp.c > +++ b/drivers/net/ethernet/freescale/fec_ptp.c > @@ -764,6 +764,36 @@ 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; > + > + spin_lock_irqsave(&fep->tmreg_lock, flags); > + > + fep->ptp_saved_state.pps_enable = fep->pps_enable; > + > + spin_unlock_irqrestore(&fep->tmreg_lock, flags); } > + > +/* Restore PTP functionality after a reset */ void > +fec_ptp_restore_state(struct fec_enet_private *fep) { > + unsigned long flags; > + > + spin_lock_irqsave(&fep->tmreg_lock, flags); > + > + /* Reset turned it off, so adjust our status flag */ > + fep->pps_enable = 0; > + > + spin_unlock_irqrestore(&fep->tmreg_lock, flags); > + > + /* Restart PPS if needed */ > + if (fep->ptp_saved_state.pps_enable) { It's better to put " fep->pps_enable = 0" here so that it does not need to be set when PPS is disabled. > + /* 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 >
On 2024. 09. 25. 6:37, Wei Fang wrote: >> -----Original Message----- >> From: Csókás, Bence <csokas.bence@prolan.hu> >> Sent: 2024年9月24日 17:37 >> To: Frank Li <Frank.Li@freescale.com>; David S. Miller >> <davem@davemloft.net>; imx@lists.linux.dev; netdev@vger.kernel.org; >> linux-kernel@vger.kernel.org >> Cc: Csókás, Bence <csokas.bence@prolan.hu>; Wei Fang <wei.fang@nxp.com>; >> Shenwei Wang <shenwei.wang@nxp.com>; Clark Wang >> <xiaoning.wang@nxp.com>; Eric Dumazet <edumazet@google.com>; Jakub >> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Richard >> Cochran <richardcochran@gmail.com> >> Subject: [PATCH net 1/2] net: fec: Restart PPS after link state change >> > The "v2" descriptor is missing in the subject, and correct the mail address > of Frank. My bad, sorry. get_maintainer.pl spat out the old email address, as that was their former committer address, and I wasn't paying attention. >> +/* Restore PTP functionality after a reset */ void >> +fec_ptp_restore_state(struct fec_enet_private *fep) { >> + unsigned long flags; >> + >> + spin_lock_irqsave(&fep->tmreg_lock, flags); >> + >> + /* Reset turned it off, so adjust our status flag */ >> + fep->pps_enable = 0; >> + >> + spin_unlock_irqrestore(&fep->tmreg_lock, flags); >> + >> + /* Restart PPS if needed */ >> + if (fep->ptp_saved_state.pps_enable) { > > It's better to put " fep->pps_enable = 0" here so that it does > not need to be set when PPS is disabled. It doesn't hurt to set it to 0 when it's already 0, and it saves us having to unlock separately in the if {} and else blocks. Plus, after reset, PPS will be turned off unconditionally, since the actual HW gets reset. Bence
On 9/30/24 10:20, Csókás Bence wrote: > On 2024. 09. 25. 6:37, Wei Fang wrote: >>> +/* Restore PTP functionality after a reset */ void >>> +fec_ptp_restore_state(struct fec_enet_private *fep) { >>> + unsigned long flags; >>> + >>> + spin_lock_irqsave(&fep->tmreg_lock, flags); >>> + >>> + /* Reset turned it off, so adjust our status flag */ >>> + fep->pps_enable = 0; >>> + >>> + spin_unlock_irqrestore(&fep->tmreg_lock, flags); >>> + >>> + /* Restart PPS if needed */ >>> + if (fep->ptp_saved_state.pps_enable) { >> >> It's better to put " fep->pps_enable = 0" here so that it does >> not need to be set when PPS is disabled. > > It doesn't hurt to set it to 0 when it's already 0, and it saves us > having to unlock separately in the if {} and else blocks. Plus, after > reset, PPS will be turned off unconditionally, since the actual HW gets > reset. I agree with Csókás, the proposed code looks simpler and more readable. I'm applying this. Thanks! Paolo
Hello: This series was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Tue, 24 Sep 2024 11:37:04 +0200 you 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. > > Fixes: 6605b730c061 ("FEC: Add time stamping code and a PTP hardware clock") > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> > > [...] Here is the summary with links: - [net,1/2] net: fec: Restart PPS after link state change https://git.kernel.org/netdev/net/c/a1477dc87dc4 - [net,2/2] net: fec: Reload PTP registers after link-state change https://git.kernel.org/netdev/net/c/d9335d0232d2 You are awesome, thank you!
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h index a19cb2a786fd..0552317a2554 100644 --- a/drivers/net/ethernet/freescale/fec.h +++ b/drivers/net/ethernet/freescale/fec.h @@ -691,10 +691,16 @@ struct fec_enet_private { /* XDP BPF Program */ struct bpf_prog *xdp_prog; + struct { + int pps_enable; + } 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 acbb627d51bf..31ebf6a4f973 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. @@ -1244,8 +1246,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) @@ -1336,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. @@ -1366,6 +1372,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 4cffda363a14..df1ef023493b 100644 --- a/drivers/net/ethernet/freescale/fec_ptp.c +++ b/drivers/net/ethernet/freescale/fec_ptp.c @@ -764,6 +764,36 @@ 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; + + spin_lock_irqsave(&fep->tmreg_lock, flags); + + fep->ptp_saved_state.pps_enable = fep->pps_enable; + + spin_unlock_irqrestore(&fep->tmreg_lock, flags); +} + +/* Restore PTP functionality after a reset */ +void fec_ptp_restore_state(struct fec_enet_private *fep) +{ + unsigned long flags; + + spin_lock_irqsave(&fep->tmreg_lock, flags); + + /* Reset turned it off, so adjust our status flag */ + fep->pps_enable = 0; + + spin_unlock_irqrestore(&fep->tmreg_lock, flags); + + /* Restart PPS if needed */ + if (fep->ptp_saved_state.pps_enable) { + /* 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);
On link state change, the controller gets reset, causing PPS to drop out. Re-enable PPS if it was enabled before the controller reset. Fixes: 6605b730c061 ("FEC: Add time stamping code and a PTP hardware clock") Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> --- Notes: Changes in v2: - store pps_enable's pre-reset value and clear it on restore - acquire tmreg_lock when reading/writing fep->pps_enable drivers/net/ethernet/freescale/fec.h | 6 +++++ drivers/net/ethernet/freescale/fec_main.c | 11 ++++++++- drivers/net/ethernet/freescale/fec_ptp.c | 30 +++++++++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-)