| Message ID | 20250121103857.12007-3-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: fec: Refactor MAC reset to function | expand |
On Tue, Jan 21, 2025 at 11:38:58AM +0100, Csókás, Bence wrote: > The core is reset both in `fec_restart()` > (called on link-up) and `fec_stop()` > (going to sleep, driver remove etc.). > These two functions had their separate > implementations, which was at first only > a register write and a `udelay()` (and > the accompanying block comment). > However, since then we got soft-reset > (MAC disable) and Wake-on-LAN support, > which meant that these implementations > diverged, often causing bugs. For instance, > as of now, `fec_stop()` does not check for > `FEC_QUIRK_NO_HARD_RESET`. To eliminate > this bug-source, refactor implementation > to a common function. > > Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up") > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> > --- > > Notes: > Recommended options for this patch: > `--color-moved --color-moved-ws=allow-indentation-change` > > drivers/net/ethernet/freescale/fec_main.c | 50 +++++++++++------------ > 1 file changed, 23 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 68725506a095..850ef3de74ec 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1064,6 +1064,27 @@ static void fec_enet_enable_ring(struct net_device *ndev) > } > } > > +/* 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. > + */ > +static void fec_ctrl_reset(struct fec_enet_private *fep, bool wol) > +{ > + if (!wol || !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { > + if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES || > + ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) { > + writel(0, fep->hwp + FEC_ECNTRL); > + } else { > + writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL); > + udelay(10); > + } > + } else { > + val = readl(fep->hwp + FEC_ECNTRL); > + val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); > + writel(val, fep->hwp + FEC_ECNTRL); > + } > +} > + > /* > * This function is called to start or restart the FEC during a link > * change, transmit timeout, or to reconfigure the FEC. The network > @@ -1080,17 +1101,7 @@ fec_restart(struct net_device *ndev) > if (fep->bufdesc_ex) > 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. > - */ > - if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES || > - ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) { > - writel(0, fep->hwp + FEC_ECNTRL); > - } else { > - writel(1, fep->hwp + FEC_ECNTRL); Nit, in case of a need for v2 you can mark in commit message that FEC_ECR_RESET == 1, so define can be use instead of 1. > - udelay(10); > - } > + fec_ctrl_reset(fep, false); > > /* > * enet-mac reset will reset mac address registers too, > @@ -1344,22 +1355,7 @@ fec_stop(struct net_device *ndev) > if (fep->bufdesc_ex) > 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. > - */ > - if (!(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { > - if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES) { > - writel(0, fep->hwp + FEC_ECNTRL); > - } else { > - writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL); > - udelay(10); > - } > - } else { > - val = readl(fep->hwp + FEC_ECNTRL); > - val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); > - writel(val, fep->hwp + FEC_ECNTRL); > - } > + fec_ctrl_reset(fep, true); > writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); > writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK); > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > -- > 2.48.1
Hi, On 21.01.25 11:38, Csókás, Bence wrote: > The core is reset both in `fec_restart()` > (called on link-up) and `fec_stop()` > (going to sleep, driver remove etc.). > These two functions had their separate > implementations, which was at first only > a register write and a `udelay()` (and > the accompanying block comment). > However, since then we got soft-reset > (MAC disable) and Wake-on-LAN support, > which meant that these implementations > diverged, often causing bugs. For instance, > as of now, `fec_stop()` does not check for > `FEC_QUIRK_NO_HARD_RESET`. To eliminate > this bug-source, refactor implementation > to a common function. please make the lines a bit longer for v2. 43 characters is much too limited. Thanks, Ahmad > > Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up") > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> > --- > > Notes: > Recommended options for this patch: > `--color-moved --color-moved-ws=allow-indentation-change` > > drivers/net/ethernet/freescale/fec_main.c | 50 +++++++++++------------ > 1 file changed, 23 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 68725506a095..850ef3de74ec 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1064,6 +1064,27 @@ static void fec_enet_enable_ring(struct net_device *ndev) > } > } > > +/* 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. > + */ > +static void fec_ctrl_reset(struct fec_enet_private *fep, bool wol) > +{ > + if (!wol || !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { > + if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES || > + ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) { > + writel(0, fep->hwp + FEC_ECNTRL); > + } else { > + writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL); > + udelay(10); > + } > + } else { > + val = readl(fep->hwp + FEC_ECNTRL); > + val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); > + writel(val, fep->hwp + FEC_ECNTRL); > + } > +} > + > /* > * This function is called to start or restart the FEC during a link > * change, transmit timeout, or to reconfigure the FEC. The network > @@ -1080,17 +1101,7 @@ fec_restart(struct net_device *ndev) > if (fep->bufdesc_ex) > 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. > - */ > - if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES || > - ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) { > - writel(0, fep->hwp + FEC_ECNTRL); > - } else { > - writel(1, fep->hwp + FEC_ECNTRL); > - udelay(10); > - } > + fec_ctrl_reset(fep, false); > > /* > * enet-mac reset will reset mac address registers too, > @@ -1344,22 +1355,7 @@ fec_stop(struct net_device *ndev) > if (fep->bufdesc_ex) > 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. > - */ > - if (!(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { > - if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES) { > - writel(0, fep->hwp + FEC_ECNTRL); > - } else { > - writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL); > - udelay(10); > - } > - } else { > - val = readl(fep->hwp + FEC_ECNTRL); > - val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); > - writel(val, fep->hwp + FEC_ECNTRL); > - } > + fec_ctrl_reset(fep, true); > writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); > writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK); >
On Tue, Jan 21, 2025 at 11:38:58AM +0100, Csókás, Bence wrote: > The core is reset both in `fec_restart()` > (called on link-up) and `fec_stop()` > (going to sleep, driver remove etc.). > These two functions had their separate > implementations, which was at first only > a register write and a `udelay()` (and > the accompanying block comment). > However, since then we got soft-reset > (MAC disable) and Wake-on-LAN support, > which meant that these implementations > diverged, often causing bugs. For instance, > as of now, `fec_stop()` does not check for > `FEC_QUIRK_NO_HARD_RESET`. To eliminate > this bug-source, refactor implementation > to a common function. > > Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up") > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> > --- > > Notes: > Recommended options for this patch: > `--color-moved --color-moved-ws=allow-indentation-change` > > drivers/net/ethernet/freescale/fec_main.c | 50 +++++++++++------------ > 1 file changed, 23 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 68725506a095..850ef3de74ec 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1064,6 +1064,27 @@ static void fec_enet_enable_ring(struct net_device *ndev) > } > } > > +/* 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. > + */ > +static void fec_ctrl_reset(struct fec_enet_private *fep, bool wol) > +{ > + if (!wol || !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { > + if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES || > + ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) { > + writel(0, fep->hwp + FEC_ECNTRL); > + } else { > + writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL); > + udelay(10); > + } > + } else { > + val = readl(fep->hwp + FEC_ECNTRL); > + val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); > + writel(val, fep->hwp + FEC_ECNTRL); Hi Bence, It seems that this does not compile because val is not declared in this scope. > + } > +} > + > /* > * This function is called to start or restart the FEC during a link > * change, transmit timeout, or to reconfigure the FEC. The network Please observe the rule regarding not posting updated patches within 24h when preparing v2.
Hi all, On 2025. 01. 21. 16:19, Simon Horman wrote: > It seems that this does not compile because val is not declared in this scope. Ohh, shoot, I forgot a `-a` in `git commit --amend`... Rookie mistake; will be fixed. > Please observe the rule regarding not posting updated patches within 24h > when preparing v2. Of course. On 2025. 01. 21. 15:36, Ahmad Fatoum wrote: > please make the lines a bit longer for v2. 43 characters is much too limited. Reformatted to 80 cols. On 2025. 01. 21. 15:10, Michal Swiatkowski wrote: >> - } else { >> - writel(1, fep->hwp + FEC_ECNTRL); > Nit, in case of a need for v2 you can mark in commit message that > FEC_ECR_RESET == 1, so define can be use instead of 1. This was somehow missed from my earlier refactor in commit ff049886671c ("net: fec: Refactor: #define magic constants"). Adding this to the msg. Bence
Hi Bence and thanks for the patch. Leaving out the check for FEC_QUIRK_NO_HARD_RESET in fec_stop() was, in fact, not unintentional. Although a hard reset in fec_restart() caused link issues with the iMX28, I had no particular reason to believe that it would also cause issues in fec_stop(), since at this point you're turning off the interface, and I did not observe any particular problems either, so I did not think the same modification was warranted there. If you have reason to believe that this is a bug, then it should be fixed, but currently I don't see why this is the case here. I think a refactoring duplicated code is a good idea, but since it also includes a modification of the behavior (specifically, there is a possible path where FEC_QUIRK_NO_HARD_RESET is set and the link is up, where fec_stop() will issue a soft reset instead of a hard reset), I would prefer to know that this change is indeed necessary. If others disagree and there's a consensus that this change is ok, I'm happy for the patch to get through, but I tend to err on the side of caution in such cases. An additional comment - this is just my personal opinion - but in fec_ctrl_reset(), it seems to me that the function of the wol argument really is to distinguish if we're using the fec_restart() or the fec_stop() implementation, so I think the naming may be a bit misleading in this case. Best regards, Laurent On Tue, Jan 21, 2025 at 11:38:58AM +0100, Csókás, Bence wrote: > The core is reset both in `fec_restart()` (called on link-up) and > `fec_stop()` (going to sleep, driver remove etc.). > These two functions had their separate implementations, which was at > first only a register write and a `udelay()` (and the accompanying > block comment). > However, since then we got soft-reset > (MAC disable) and Wake-on-LAN support, which meant that these > implementations diverged, often causing bugs. For instance, as of now, > `fec_stop()` does not check for `FEC_QUIRK_NO_HARD_RESET`. To > eliminate this bug-source, refactor implementation to a common > function. > > Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link > up") > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> > --- > > Notes: > Recommended options for this patch: > `--color-moved --color-moved-ws=allow-indentation-change` > > drivers/net/ethernet/freescale/fec_main.c | 50 > +++++++++++------------ > 1 file changed, 23 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > b/drivers/net/ethernet/freescale/fec_main.c > index 68725506a095..850ef3de74ec 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1064,6 +1064,27 @@ static void fec_enet_enable_ring(struct net_device *ndev) > } > } > > +/* 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. > + */ > +static void fec_ctrl_reset(struct fec_enet_private *fep, bool wol) { > + if (!wol || !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { > + if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES || > + ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) { > + writel(0, fep->hwp + FEC_ECNTRL); > + } else { > + writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL); > + udelay(10); > + } > + } else { > + val = readl(fep->hwp + FEC_ECNTRL); > + val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); > + writel(val, fep->hwp + FEC_ECNTRL); > + } > +} > + > /* > * This function is called to start or restart the FEC during a link > * change, transmit timeout, or to reconfigure the FEC. The network > @@ -1080,17 +1101,7 @@ fec_restart(struct net_device *ndev) > if (fep->bufdesc_ex) > 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. > - */ > - if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES || > - ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) { > - writel(0, fep->hwp + FEC_ECNTRL); > - } else { > - writel(1, fep->hwp + FEC_ECNTRL); Nit, in case of a need for v2 you can mark in commit message that FEC_ECR_RESET == 1, so define can be use instead of 1. > - udelay(10); > - } > + fec_ctrl_reset(fep, false); > > /* > * enet-mac reset will reset mac address registers too, @@ > -1344,22 +1355,7 @@ fec_stop(struct net_device *ndev) > if (fep->bufdesc_ex) > 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. > - */ > - if (!(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { > - if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES) { > - writel(0, fep->hwp + FEC_ECNTRL); > - } else { > - writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL); > - udelay(10); > - } > - } else { > - val = readl(fep->hwp + FEC_ECNTRL); > - val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); > - writel(val, fep->hwp + FEC_ECNTRL); > - } > + fec_ctrl_reset(fep, true); > writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); > writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK); > > -- > 2.48.1
On Tue, Jan 21, 2025 at 11:38:58AM +0100, Csókás, Bence wrote: > The core is reset both in `fec_restart()` > (called on link-up) and `fec_stop()` > (going to sleep, driver remove etc.). > These two functions had their separate > implementations, which was at first only > a register write and a `udelay()` (and > the accompanying block comment). > However, since then we got soft-reset > (MAC disable) and Wake-on-LAN support, > which meant that these implementations > diverged, often causing bugs. For instance, > as of now, `fec_stop()` does not check for > `FEC_QUIRK_NO_HARD_RESET`. To eliminate > this bug-source, refactor implementation > to a common function. > > Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up") > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> The subject say "Refactor...." A refactor generally does not need a Fixes: tag. Maybe make the subject reflect the bug you are fixing, and reword the commit message to focus on the bug being fixed, not the refactor. Andrew
On Tue, 21 Jan 2025 16:51:51 +0100 Csókás Bence wrote: > On 2025. 01. 21. 15:36, Ahmad Fatoum wrote: > > please make the lines a bit longer for v2. 43 characters is much too > limited. > > Reformatted to 80 cols. Quoting "Submitting patches": The body of the explanation, line wrapped at 75 columns, which will be copied to the permanent changelog to describe this patch. https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format
Hi Laurent, On 2025. 01. 21. 17:09, Badel, Laurent wrote: > Hi Bence and thanks for the patch. thanks for your input. > Leaving out the check for FEC_QUIRK_NO_HARD_RESET in fec_stop() was, in fact, > not unintentional. Although a hard reset in fec_restart() caused link issues > with the iMX28, I had no particular reason to believe that it would also cause > issues in fec_stop(), since at this point you're turning off the interface, and > I did not observe any particular problems either, so I did not think the same > modification was warranted there. I had a feeling it was intentional, however, `fec_stop()` is called all over the place - not just when removing the interface (e.g. unloading the driver), but also by the PM subsystem for entering suspend, restarting auto-negotiation, for handling Pause frames and changing HW-accelerated RX checksum checking... > If you have reason to believe that this is a bug, then it should be fixed, but > currently I don't see why this is the case here. I think a refactoring > duplicated code is a good idea, but since it also includes a modification of > the behavior (specifically, there is a possible path where > FEC_QUIRK_NO_HARD_RESET is set and the link is up, where fec_stop() will issue > a soft reset instead of a hard reset), I would prefer to know that this change > is indeed necessary. > > If others disagree and there's a consensus that this change is ok, I'm happy > for the patch to get through, but I tend to err on the side of caution in such > cases. To me, the name `FEC_QUIRK_NO_HARD_RESET`, and its doc-comment seems to suggest that we do *not* want to hard-reset this MAC *ever*; not in the codepath of `fec_restart()` and not in `fec_stop()`. Did you observe problems on i.MX28 if you soft-reset it in stop()? I _might_ be able to get my hands on an i.MX287 and test, but I have no idea if it is working; I took it out from the junk bin. Right now, we're chasing a different bug on the i.MX6, and this was just meant to reduce the amount of clutter we have to cut through. > An additional comment - this is just my personal opinion - but in > fec_ctrl_reset(), it seems to me that the function of the wol argument really > is to distinguish if we're using the fec_restart() or the fec_stop() > implementation, so I think the naming may be a bit misleading in this case. True, but I would prefer to keep it separate, i.e. the `wol` parameter should really only control whether we want to enable WoL. If we decide to keep the old behavior of not honoring FEC_QUIRK_NO_HARD_RESET in stop(), I'd rather add a new parameter `allow_soft_reset`. That way, if ever someone needs to call `fec_ctrl_reset()`, they will be able to give it values they make sense at that point-of-call, instead of having to "fake" either restart() or stop(). Bence
> The core is reset both in `fec_restart()` > (called on link-up) and `fec_stop()` > (going to sleep, driver remove etc.). > These two functions had their separate > implementations, which was at first only > a register write and a `udelay()` (and > the accompanying block comment). > However, since then we got soft-reset > (MAC disable) and Wake-on-LAN support, > which meant that these implementations > diverged, often causing bugs. For instance, > as of now, `fec_stop()` does not check for > `FEC_QUIRK_NO_HARD_RESET`. To eliminate > this bug-source, refactor implementation > to a common function. > > Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up") > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> > --- > > Notes: > Recommended options for this patch: > `--color-moved --color-moved-ws=allow-indentation-change` > > drivers/net/ethernet/freescale/fec_main.c | 50 +++++++++++------------ > 1 file changed, 23 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > b/drivers/net/ethernet/freescale/fec_main.c > index 68725506a095..850ef3de74ec 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1064,6 +1064,27 @@ static void fec_enet_enable_ring(struct net_device > *ndev) > } > } > > +/* 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. > + */ > +static void fec_ctrl_reset(struct fec_enet_private *fep, bool wol) 'wol' gives me the feeling that it indicates whether the WOL function is enabled. And the else branch will be taken if 'wol' is true. But in fact, even if 'wol' is true, it may go to the reset branch. This is a bit confusing. How about the following changes? static void fec_ctrl_reset(struct fec_enet_private *fep, bool wol) { if (wol) { u32 val; val = readl(fep->hwp + FEC_ECNTRL); val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); writel(val, fep->hwp + FEC_ECNTRL); } else if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES || ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) { writel(0, fep->hwp + FEC_ECNTRL); } else { writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL); udelay(10); } } > +{ > + if (!wol || !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { > + if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES || > + ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) { > + writel(0, fep->hwp + FEC_ECNTRL); > + } else { > + writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL); > + udelay(10); > + } > + } else { > + val = readl(fep->hwp + FEC_ECNTRL); > + val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); > + writel(val, fep->hwp + FEC_ECNTRL); > + } > +} > + > /* > * This function is called to start or restart the FEC during a link > * change, transmit timeout, or to reconfigure the FEC. The network > @@ -1080,17 +1101,7 @@ fec_restart(struct net_device *ndev) > if (fep->bufdesc_ex) > 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. > - */ > - if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES || > - ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) { > - writel(0, fep->hwp + FEC_ECNTRL); > - } else { > - writel(1, fep->hwp + FEC_ECNTRL); > - udelay(10); > - } > + fec_ctrl_reset(fep, false); > > /* > * enet-mac reset will reset mac address registers too, > @@ -1344,22 +1355,7 @@ fec_stop(struct net_device *ndev) > if (fep->bufdesc_ex) > 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. > - */ > - if (!(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { > - if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES) { > - writel(0, fep->hwp + FEC_ECNTRL); > - } else { > - writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL); > - udelay(10); > - } > - } else { > - val = readl(fep->hwp + FEC_ECNTRL); > - val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); > - writel(val, fep->hwp + FEC_ECNTRL); > - } > + fec_ctrl_reset(fep, true); > writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); > writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK); > > -- > 2.48.1 >
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 68725506a095..850ef3de74ec 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1064,6 +1064,27 @@ static void fec_enet_enable_ring(struct net_device *ndev) } } +/* 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. + */ +static void fec_ctrl_reset(struct fec_enet_private *fep, bool wol) +{ + if (!wol || !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { + if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES || + ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) { + writel(0, fep->hwp + FEC_ECNTRL); + } else { + writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL); + udelay(10); + } + } else { + val = readl(fep->hwp + FEC_ECNTRL); + val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); + writel(val, fep->hwp + FEC_ECNTRL); + } +} + /* * This function is called to start or restart the FEC during a link * change, transmit timeout, or to reconfigure the FEC. The network @@ -1080,17 +1101,7 @@ fec_restart(struct net_device *ndev) if (fep->bufdesc_ex) 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. - */ - if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES || - ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) { - writel(0, fep->hwp + FEC_ECNTRL); - } else { - writel(1, fep->hwp + FEC_ECNTRL); - udelay(10); - } + fec_ctrl_reset(fep, false); /* * enet-mac reset will reset mac address registers too, @@ -1344,22 +1355,7 @@ fec_stop(struct net_device *ndev) if (fep->bufdesc_ex) 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. - */ - if (!(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { - if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES) { - writel(0, fep->hwp + FEC_ECNTRL); - } else { - writel(FEC_ECR_RESET, fep->hwp + FEC_ECNTRL); - udelay(10); - } - } else { - val = readl(fep->hwp + FEC_ECNTRL); - val |= (FEC_ECR_MAGICEN | FEC_ECR_SLEEP); - writel(val, fep->hwp + FEC_ECNTRL); - } + fec_ctrl_reset(fep, true); writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
The core is reset both in `fec_restart()` (called on link-up) and `fec_stop()` (going to sleep, driver remove etc.). These two functions had their separate implementations, which was at first only a register write and a `udelay()` (and the accompanying block comment). However, since then we got soft-reset (MAC disable) and Wake-on-LAN support, which meant that these implementations diverged, often causing bugs. For instance, as of now, `fec_stop()` does not check for `FEC_QUIRK_NO_HARD_RESET`. To eliminate this bug-source, refactor implementation to a common function. Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up") Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> --- Notes: Recommended options for this patch: `--color-moved --color-moved-ws=allow-indentation-change` drivers/net/ethernet/freescale/fec_main.c | 50 +++++++++++------------ 1 file changed, 23 insertions(+), 27 deletions(-)