| Message ID | 20250122163935.213313-2-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: fec: Refactor MAC reset to function | expand |
On 1/22/2025 8:39 AM, 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`, and `fec_restart()` missed the refactor in > commit ff049886671c ("net: fec: Refactor: #define magic constants") > renaming the "magic" constant `1` to `FEC_ECR_RESET`. > > To eliminate this bug-source, refactor implementation to a common function. > > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up") > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> > --- > You didn't mention the tree this targets. This is a bug fix, so this should be targeted to net. > Notes: > Recommended options for this patch: > `--color-moved --color-moved-ws=allow-indentation-change` > Changes in v2: > * collect Michal's tag > * reformat message to 75 cols > * fix missing `u32 val` > > drivers/net/ethernet/freescale/fec_main.c | 52 +++++++++++------------ > 1 file changed, 25 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 68725506a095..520fe638ea00 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1064,6 +1064,29 @@ 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) > +{ > + u32 val; > + > + 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); > + } > +} > + Typically this kind of refactor to functions is kept separate from bug fixes. However, I think in this case, the diff size is small enough it is easy to review, so I think its worth taking as-is to net. Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> 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`, and `fec_restart()` missed the refactor in > commit ff049886671c ("net: fec: Refactor: #define magic constants") > renaming the "magic" constant `1` to `FEC_ECR_RESET`. > > To eliminate this bug-source, refactor implementation to a common function. > > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up") > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> > --- You seem to have missed Andrew's comment in the v1. If this patch is just a refactor, then the Fixes tag should be removed, and the target tree should be net-next. If not, please describe the bug in more detail in the commit message. > > Notes: > Recommended options for this patch: > `--color-moved --color-moved-ws=allow-indentation-change` > Changes in v2: > * collect Michal's tag > * reformat message to 75 cols > * fix missing `u32 val` > > drivers/net/ethernet/freescale/fec_main.c | 52 +++++++++++------------ > 1 file changed, 25 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > b/drivers/net/ethernet/freescale/fec_main.c > index 68725506a095..520fe638ea00 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1064,6 +1064,29 @@ 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) > +{ > + u32 val; > + > + 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 +1103,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 +1357,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..520fe638ea00 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1064,6 +1064,29 @@ 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) +{ + u32 val; + + 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 +1103,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 +1357,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`, and `fec_restart()` missed the refactor in commit ff049886671c ("net: fec: Refactor: #define magic constants") renaming the "magic" constant `1` to `FEC_ECR_RESET`. To eliminate this bug-source, refactor implementation to a common function. Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> 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` Changes in v2: * collect Michal's tag * reformat message to 75 cols * fix missing `u32 val` drivers/net/ethernet/freescale/fec_main.c | 52 +++++++++++------------ 1 file changed, 25 insertions(+), 27 deletions(-)