| Message ID | 20240611080405.673431-1-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [resubmit,2] net: fec: Fix FEC_ECR_EN1588 being cleared on link-down | expand |
On Tue, Jun 11, 2024 at 10:04:05AM +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, and all the extended registers > disappear, on link-down, making the adapter fall back to compatibility > "dumb mode". However, some functionality needs to be retained (e.g. PPS) > even without link. > > Fixes: 6605b730c061 ("FEC: Add time stamping code and a PTP hardware clock") > Cc: Richard Cochran <richardcochran@gmail.com> > > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Tue, 11 Jun 2024 10:04:05 +0200 Csókás, Bence wrote: > + if (fep->bufdesc_ex) { > + val = readl(fep->hwp + FEC_ECNTRL); > + val |= FEC_ECR_EN1588; > + writel(val, fep->hwp + FEC_ECNTRL); FEC_ECNTRL gets written multiple times in this function, including with 0, and then you RMW it to add this flag. Is this intentional? It really seems like you should be adding this flag more consistently or making sure its not cleared, rather than appending "add it back" at the end of the function...
On 6/13/24 17:12, Jakub Kicinski wrote: > On Tue, 11 Jun 2024 10:04:05 +0200 Csókás, Bence wrote: >> + if (fep->bufdesc_ex) { >> + val = readl(fep->hwp + FEC_ECNTRL); >> + val |= FEC_ECR_EN1588; >> + writel(val, fep->hwp + FEC_ECNTRL); > > FEC_ECNTRL gets written multiple times in this function, > including with 0, and then you RMW it to add this flag. > > Is this intentional? It really seems like you should be > adding this flag more consistently or making sure its > not cleared, rather than appending "add it back" at the > end of the function... It only writes 0 if WOL is disabled AND the device has the MULTI_QUEUES quirk. Otherwise, we either write FEC_ECR_RESET, which resets the device (and the HW changes ECNTRL to its reset value), OR we RMW set the WOL sleep bits. And then, if some more quirks are set, we set ETHEREN. So I think RMW is the safest route here, instead of trying to keep track of all these different branches, re-read ECNTRL after reset etc. Bence
On Fri, 14 Jun 2024 09:59:16 +0200 Csókás Bence wrote: > It only writes 0 if WOL is disabled AND the device has the MULTI_QUEUES > quirk. Otherwise, we either write FEC_ECR_RESET, which resets the device > (and the HW changes ECNTRL to its reset value), OR we RMW set the WOL > sleep bits. And then, if some more quirks are set, we set ETHEREN. > > So I think RMW is the safest route here, instead of trying to keep track > of all these different branches, re-read ECNTRL after reset etc. Okay, just resend without the empty line between tags then.
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
FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which makes all 1588 functionality shut down, and all the extended registers disappear, on link-down, making the adapter fall back to compatibility "dumb mode". However, some functionality needs to be retained (e.g. PPS) even without link. Fixes: 6605b730c061 ("FEC: Add time stamping code and a PTP hardware clock") Cc: Richard Cochran <richardcochran@gmail.com> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> --- drivers/net/ethernet/freescale/fec_main.c | 6 ++++++ 1 file changed, 6 insertions(+)