| Message ID | 20220809124119.29922-1-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | fec: Restart PPS after link state change | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Tue, Aug 09, 2022 at 02:41:19PM +0200, Csókás Bence wrote: > On link state change, the controller gets reset, > causing PPS to drop out. So we restart it if needed. > > Signed-off-by: Csókás Bence <csokas.bence@prolan.hu> > --- > drivers/net/ethernet/freescale/fec_main.c | 27 ++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index ca5d49361fdf..c264b1dd5286 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -954,6 +954,7 @@ fec_restart(struct net_device *ndev) > u32 temp_mac[2]; > u32 rcntl = OPT_FRAME_SIZE | 0x04; > u32 ecntl = 0x2; /* ETHEREN */ > + struct ptp_clock_request ptp_rq = { .type = PTP_CLK_REQ_PPS }; Is it safe to hard code this? What if the user configured PTP_CLK_REQ_EXTTS or PTP_CLK_REQ_PEROUT? > /* Whack a reset. We should wait for this. > * For i.MX6SX SOC, enet use AXI bus, we use disable MAC > @@ -1119,6 +1120,13 @@ fec_restart(struct net_device *ndev) > if (fep->bufdesc_ex) > fec_ptp_start_cyclecounter(ndev); > > + /* Restart PPS if needed */ > + if (fep->pps_enable) { > + /* Clear flag so fec_ptp_enable_pps() doesn't return immediately */ > + fep->pps_enable = 0; If reset causes PPS to stop, maybe it would be better to do this unconditionally? fep->pps_enable = 0; fep->ptp_caps.enable(&fep->ptp_caps, &ptp_rq, 1); > + if (fep->bufdesc_ex) > + ecntl |= (1 << 4); Please replace (1 << 4) with a #define to make it clear what this is doing. > + > /* We have to keep ENET enabled to have MII interrupt stay working */ > if (fep->quirks & FEC_QUIRK_ENET_MAC && > !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { > - writel(2, fep->hwp + FEC_ECNTRL); > + ecntl |= 0x2; > writel(rmii_mode, fep->hwp + FEC_R_CNTRL); > } > + > + writel(ecntl, fep->hwp + FEC_ECNTRL); > + > + if (fep->bufdesc_ex) > + fec_ptp_start_cyclecounter(ndev); > + > + /* Restart PPS if needed */ > + if (fep->pps_enable) { > + /* Clear flag so fec_ptp_enable_pps() doesn't return immediately */ > + fep->pps_enable = 0; > + fep->ptp_caps.enable(&fep->ptp_caps, &ptp_rq, 1); > + } So you re-start PPS in stop()? Should it keep outputting when the interface is down? Also, if it is always outputting, don't you need to stop it in fec_drv_remove(). You probably don't want to still going after the driver is unloaded. Andrew
On 2022. 08. 09. 19:28, Andrew Lunn wrote: > On Tue, Aug 09, 2022 at 02:41:19PM +0200, Csókás Bence wrote: >> On link state change, the controller gets reset, >> causing PPS to drop out. So we restart it if needed. >> >> Signed-off-by: Csókás Bence <csokas.bence@prolan.hu> >> --- >> drivers/net/ethernet/freescale/fec_main.c | 27 ++++++++++++++++++++++- >> 1 file changed, 26 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c >> index ca5d49361fdf..c264b1dd5286 100644 >> --- a/drivers/net/ethernet/freescale/fec_main.c >> +++ b/drivers/net/ethernet/freescale/fec_main.c >> @@ -954,6 +954,7 @@ fec_restart(struct net_device *ndev) >> u32 temp_mac[2]; >> u32 rcntl = OPT_FRAME_SIZE | 0x04; >> u32 ecntl = 0x2; /* ETHEREN */ >> + struct ptp_clock_request ptp_rq = { .type = PTP_CLK_REQ_PPS }; > > Is it safe to hard code this? What if the user configured > PTP_CLK_REQ_EXTTS or PTP_CLK_REQ_PEROUT? The fec driver doesn't support anything other than PTP_CLK_REQ_PPS. And if it will at some point, this will need to be amended anyways. > >> /* Whack a reset. We should wait for this. >> * For i.MX6SX SOC, enet use AXI bus, we use disable MAC >> @@ -1119,6 +1120,13 @@ fec_restart(struct net_device *ndev) >> if (fep->bufdesc_ex) >> fec_ptp_start_cyclecounter(ndev); >> >> + /* Restart PPS if needed */ >> + if (fep->pps_enable) { >> + /* Clear flag so fec_ptp_enable_pps() doesn't return immediately */ >> + fep->pps_enable = 0; > > If reset causes PPS to stop, maybe it would be better to do this > unconditionally? But if it wasn't enabled before the reset in the first place, we wouldn't want to unexpectedly start it. > > fep->pps_enable = 0; > fep->ptp_caps.enable(&fep->ptp_caps, &ptp_rq, 1); > >> + if (fep->bufdesc_ex) >> + ecntl |= (1 << 4); > > Please replace (1 << 4) with a #define to make it clear what this is doing. I took it from the original source, line 1138 as of commit #504148f. It is the EN1588 bit by the way. I shall replace it with a #define in both places then. Though the code is riddled with other magic numbers without explanation, and I probably won't be bothered to fix them all. > >> + >> /* We have to keep ENET enabled to have MII interrupt stay working */ >> if (fep->quirks & FEC_QUIRK_ENET_MAC && >> !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { >> - writel(2, fep->hwp + FEC_ECNTRL); >> + ecntl |= 0x2; >> writel(rmii_mode, fep->hwp + FEC_R_CNTRL); >> } >> + >> + writel(ecntl, fep->hwp + FEC_ECNTRL); >> + >> + if (fep->bufdesc_ex) >> + fec_ptp_start_cyclecounter(ndev); >> + >> + /* Restart PPS if needed */ >> + if (fep->pps_enable) { >> + /* Clear flag so fec_ptp_enable_pps() doesn't return immediately */ >> + fep->pps_enable = 0; >> + fep->ptp_caps.enable(&fep->ptp_caps, &ptp_rq, 1); >> + } > > So you re-start PPS in stop()? Should it keep outputting when the > interface is down? Yes. We use PPS to synchronize devices on a common backplane. We use PTP to sync this PPS to a master clock. But if PTP sync drops out, we wouldn't want the backplane-level synchronization to fail. The PPS needs to stay on as long as userspace *explicitly* disables it, regardless of what happens to the link. > > Also, if it is always outputting, don't you need to stop it in > fec_drv_remove(). You probably don't want to still going after the > driver is unloaded. Good point, that is one exception we could make to the above statement (though even in this case, userspace *really* should disable PPS before unloading the module). > > Andrew Thanks for the insights, Bence
> The fec driver doesn't support anything other than PTP_CLK_REQ_PPS. And if > it will at some point, this will need to be amended anyways. O.K. > > > > > /* Whack a reset. We should wait for this. > > > * For i.MX6SX SOC, enet use AXI bus, we use disable MAC > > > @@ -1119,6 +1120,13 @@ fec_restart(struct net_device *ndev) > > > if (fep->bufdesc_ex) > > > fec_ptp_start_cyclecounter(ndev); > > > + /* Restart PPS if needed */ > > > + if (fep->pps_enable) { > > > + /* Clear flag so fec_ptp_enable_pps() doesn't return immediately */ > > > + fep->pps_enable = 0; > > > > If reset causes PPS to stop, maybe it would be better to do this > > unconditionally? > > But if it wasn't enabled before the reset in the first place, we wouldn't > want to unexpectedly start it. We should decide what fep->pps_enable actually means. It should be enabled, or it is actually enabled? Then it becomes clear if the reset function should clear it to reflect the hardware, or if the fec_ptp_enable_pps() should not be looking at it, and needs to read the status from the hardware. > > fep->pps_enable = 0; > > fep->ptp_caps.enable(&fep->ptp_caps, &ptp_rq, 1); > > > > > + if (fep->bufdesc_ex) > > > + ecntl |= (1 << 4); > > > > Please replace (1 << 4) with a #define to make it clear what this is doing. > > I took it from the original source, line 1138 as of commit #504148f. It is > the EN1588 bit by the way. I shall replace it with a #define in both places > then. Though the code is riddled with other magic numbers without > explanation, and I probably won't be bothered to fix them all. Yes, i understand. It just makes it easier to review if you fixup parts of the code you are changing. > > So you re-start PPS in stop()? Should it keep outputting when the > > interface is down? > > Yes. We use PPS to synchronize devices on a common backplane. We use PTP to > sync this PPS to a master clock. But if PTP sync drops out, we wouldn't want > the backplane-level synchronization to fail. The PPS needs to stay on as > long as userspace *explicitly* disables it, regardless of what happens to > the link. We need the PTP Maintainers view on that. I don't know if that is normal or not. > > Also, if it is always outputting, don't you need to stop it in > > fec_drv_remove(). You probably don't want to still going after the > > driver is unloaded. > > Good point, that is one exception we could make to the above statement > (though even in this case, userspace *really* should disable PPS before > unloading the module). Never trust userspace. Ever. Andrew
On Thu, Aug 11, 2022 at 02:05:39AM +0200, Andrew Lunn wrote: > > Yes. We use PPS to synchronize devices on a common backplane. We use PTP to > > sync this PPS to a master clock. But if PTP sync drops out, we wouldn't want > > the backplane-level synchronization to fail. The PPS needs to stay on as > > long as userspace *explicitly* disables it, regardless of what happens to > > the link. > > We need the PTP Maintainers view on that. I don't know if that is > normal or not. IMO the least surprising behavior is that once enabled, a feature stays on until explicitly disabled. Thanks, Richard
On Wed, Aug 10, 2022 at 06:37:19PM -0700, Richard Cochran wrote: > On Thu, Aug 11, 2022 at 02:05:39AM +0200, Andrew Lunn wrote: > > > Yes. We use PPS to synchronize devices on a common backplane. We use PTP to > > > sync this PPS to a master clock. But if PTP sync drops out, we wouldn't want > > > the backplane-level synchronization to fail. The PPS needs to stay on as > > > long as userspace *explicitly* disables it, regardless of what happens to > > > the link. > > > > We need the PTP Maintainers view on that. I don't know if that is > > normal or not. > > IMO the least surprising behavior is that once enabled, a feature > stays on until explicitly disabled. O.K, thanks for the response. Your answer is a bit surprising to me. To me, an interface which is administratively down is completely inactive. The action to down it should disable everything. So your answer also implies PPS can be used before the interface is set administratively up? Andrew
On 2022. 08. 11. 4:05, Andrew Lunn wrote: > On Wed, Aug 10, 2022 at 06:37:19PM -0700, Richard Cochran wrote: >> On Thu, Aug 11, 2022 at 02:05:39AM +0200, Andrew Lunn wrote: >>>> Yes. We use PPS to synchronize devices on a common backplane. We use PTP to >>>> sync this PPS to a master clock. But if PTP sync drops out, we wouldn't want >>>> the backplane-level synchronization to fail. The PPS needs to stay on as >>>> long as userspace *explicitly* disables it, regardless of what happens to >>>> the link. >>> >>> We need the PTP Maintainers view on that. I don't know if that is >>> normal or not. >> >> IMO the least surprising behavior is that once enabled, a feature >> stays on until explicitly disabled. > > O.K, thanks for the response. > > Your answer is a bit surprising to me. To me, an interface which is > administratively down is completely inactive. The action to down it > should disable everything. I think you are confusing two states here. This patch addresses the bug thatcauses the PPS to drop when the Ethernet link goes away. The interface remainsUP the whole time. > > So your answer also implies PPS can be used before the interface is > set administratively up? The PPS can already be used before the first link-up, but once it has acquired a link once, PPS no longer works without a link. > > Andrew
On 2022. 08. 11. 2:05, Andrew Lunn wrote: >>> >>>> /* Whack a reset. We should wait for this. >>>> * For i.MX6SX SOC, enet use AXI bus, we use disable MAC >>>> @@ -1119,6 +1120,13 @@ fec_restart(struct net_device *ndev) >>>> if (fep->bufdesc_ex) >>>> fec_ptp_start_cyclecounter(ndev); >>>> + /* Restart PPS if needed */ >>>> + if (fep->pps_enable) { >>>> + /* Clear flag so fec_ptp_enable_pps() doesn't return immediately */ >>>> + fep->pps_enable = 0; >>> >>> If reset causes PPS to stop, maybe it would be better to do this >>> unconditionally? >> >> But if it wasn't enabled before the reset in the first place, we wouldn't >> want to unexpectedly start it. > > We should decide what fep->pps_enable actually means. It should be > enabled, or it is actually enabled? Then it becomes clear if the reset > function should clear it to reflect the hardware, or if the > fec_ptp_enable_pps() should not be looking at it, and needs to read > the status from the hardware. > `fep->pps_enable` is the state of the PPS the driver *believes* to be the case. After a reset, this belief may or may not be true anymore: if the driver believed formerly that the PPS is down, then after a reset, its belief will still be correct, thus nothing needs to be done about the situation. If, however, the driver thought that PPS was up, after controller reset, it no longer holds, so we need to update our world-view (`fep->pps_enable = 0;`), and then correct for the fact that PPS just unexpectedly stopped. >>> Also, if it is always outputting, don't you need to stop it in >>> fec_drv_remove(). You probably don't want to still going after the >>> driver is unloaded. >> >> Good point, that is one exception we could make to the above statement >> (though even in this case, userspace *really* should disable PPS before >> unloading the module). > > Never trust userspace. Ever. Amen. Said issue is already fixed in the next version of the patch. > > Andrew Bence
> `fep->pps_enable` is the state of the PPS the driver *believes* to be the > case. After a reset, this belief may or may not be true anymore: if the > driver believed formerly that the PPS is down, then after a reset, its > belief will still be correct, thus nothing needs to be done about the > situation. If, however, the driver thought that PPS was up, after controller > reset, it no longer holds, so we need to update our world-view > (`fep->pps_enable = 0;`), and then correct for the fact that PPS just > unexpectedly stopped. Your way of doing it just seems very unclean. I would make fec_ptp_enable_pps() read the actual status from the hardware. fep->pps_enable then has the clear meaning of user space requested it should be enabled. Andrew
On 2022. 08. 11. 15:30, Andrew Lunn wrote: >> `fep->pps_enable` is the state of the PPS the driver *believes* to be the >> case. After a reset, this belief may or may not be true anymore: if the >> driver believed formerly that the PPS is down, then after a reset, its >> belief will still be correct, thus nothing needs to be done about the >> situation. If, however, the driver thought that PPS was up, after controller >> reset, it no longer holds, so we need to update our world-view >> (`fep->pps_enable = 0;`), and then correct for the fact that PPS just >> unexpectedly stopped. > > Your way of doing it just seems very unclean. I would make > fec_ptp_enable_pps() read the actual status from the > hardware. fep->pps_enable then has the clear meaning of user space > requested it should be enabled. 1. It is not "my way", it is how it was in the original code. I am merely following those who came before me. 2. There is already a variable which holds userspace's wish: parameter `uint enable` in `fec_ptp_enable_pps()`. `fep->pps_enable` is whether the driver already fulfilled this wish. > > Andrew Honestly, I would rather see the entire `fec` driver re-written from scratch, it is really bad code and full of bugs. Plus, Fugang Duan's mail server keeps bouncing back all my emails (I can only hope he sees these mails through the mailing list). However, that exceeds my capabilities unfortunately (I know not nearly enough of the various fec-based controllers and their internals, I only have the i.MX6 TX6UL to test). So the best I can do is provide fixes to the bugs we experienced, while changing as little of the original driver's code as possible, so as to (hopefully) not introduce even more bugs. Bence
On Thu, Aug 11, 2022 at 04:05:03AM +0200, Andrew Lunn wrote: > So your answer also implies PPS can be used before the interface is > set administratively up? Why not? After all, the clock is (or should be) independent from the MAC. It works all by itself. Thanks, Richard
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index ca5d49361fdf..c264b1dd5286 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -954,6 +954,7 @@ fec_restart(struct net_device *ndev) u32 temp_mac[2]; u32 rcntl = OPT_FRAME_SIZE | 0x04; u32 ecntl = 0x2; /* ETHEREN */ + struct ptp_clock_request ptp_rq = { .type = PTP_CLK_REQ_PPS }; /* Whack a reset. We should wait for this. * For i.MX6SX SOC, enet use AXI bus, we use disable MAC @@ -1119,6 +1120,13 @@ fec_restart(struct net_device *ndev) if (fep->bufdesc_ex) fec_ptp_start_cyclecounter(ndev); + /* Restart PPS if needed */ + if (fep->pps_enable) { + /* Clear flag so fec_ptp_enable_pps() doesn't return immediately */ + fep->pps_enable = 0; + fep->ptp_caps.enable(&fep->ptp_caps, &ptp_rq, 1); + } + /* Enable interrupts we wish to service */ if (fep->link) writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK); @@ -1154,6 +1162,8 @@ fec_stop(struct net_device *ndev) struct fec_enet_private *fep = netdev_priv(ndev); u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8); u32 val; + struct ptp_clock_request ptp_rq = { .type = PTP_CLK_REQ_PPS }; + u32 ecntl = 0; /* We cannot expect a graceful transmit stop without link !!! */ if (fep->link) { @@ -1184,12 +1194,27 @@ fec_stop(struct net_device *ndev) } writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); + if (fep->bufdesc_ex) + ecntl |= (1 << 4); + /* We have to keep ENET enabled to have MII interrupt stay working */ if (fep->quirks & FEC_QUIRK_ENET_MAC && !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { - writel(2, fep->hwp + FEC_ECNTRL); + ecntl |= 0x2; writel(rmii_mode, fep->hwp + FEC_R_CNTRL); } + + writel(ecntl, fep->hwp + FEC_ECNTRL); + + if (fep->bufdesc_ex) + fec_ptp_start_cyclecounter(ndev); + + /* Restart PPS if needed */ + if (fep->pps_enable) { + /* Clear flag so fec_ptp_enable_pps() doesn't return immediately */ + fep->pps_enable = 0; + fep->ptp_caps.enable(&fep->ptp_caps, &ptp_rq, 1); + } }
On link state change, the controller gets reset, causing PPS to drop out. So we restart it if needed. Signed-off-by: Csókás Bence <csokas.bence@prolan.hu> --- drivers/net/ethernet/freescale/fec_main.c | 27 ++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)