| Message ID | 20250204093604.253436-2-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: fec: Refactor MAC reset to function | expand |
On Tue, Feb 04, 2025 at 10:36:03AM +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`, meaning the MII/RMII mode is cleared on eg. > a PM power-down event; and `fec_restart()` missed the refactor renaming > the "magic" constant `1` to `FEC_ECR_RESET`. > > To harmonize current implementations, and eliminate this source of > potential future bugs, refactor implementation to a common function. > > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up") > Fixes: ff049886671c ("net: fec: Refactor: #define magic constants") > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> Reviewed-by: Simon Horman <horms@kernel.org>
On Wed, Feb 05, 2025 at 01:28:32PM +0000, Simon Horman wrote: > On Tue, Feb 04, 2025 at 10:36:03AM +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`, meaning the MII/RMII mode is cleared on eg. > > a PM power-down event; and `fec_restart()` missed the refactor renaming > > the "magic" constant `1` to `FEC_ECR_RESET`. > > > > To harmonize current implementations, and eliminate this source of > > potential future bugs, refactor implementation to a common function. > > > > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > > Fixes: c730ab423bfa ("net: fec: Fix temporary RMII clock reset on link up") > > Fixes: ff049886671c ("net: fec: Refactor: #define magic constants") > > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> > > Reviewed-by: Simon Horman <horms@kernel.org> Sorry, I think I have to take that back for now. Unfortunately when I was looking over this I failed to notice: that there is a newer version of this patch (v3); the following response from Jakub to v3; and, the response from Laurent that he is referring to. "Laurent responded to v1 saying this was intentional. Please give more details on how problem you're seeing and on what platforms. Otherwise this is not a fix but refactoring. "Please don't post new versions in-reply-to, and add lore links to the previous version in the changelog. Link: https://lore.kernel.org/netdev/20250204074504.523c794c@kernel.org/
Hi, sorry for the confusion. I accidentally sent that one while editing the headers. On 2025. 02. 05. 14:48, Simon Horman wrote: >> Reviewed-by: Simon Horman <horms@kernel.org> > > Sorry, I think I have to take that back for now. Would you keep it if I retargeted to net-next without the Fixes: tags? Bence
On Thu, Feb 06, 2025 at 02:02:50PM +0100, Csókás Bence wrote: > Hi, > sorry for the confusion. I accidentally sent that one while editing the > headers. > > On 2025. 02. 05. 14:48, Simon Horman wrote: > > > Reviewed-by: Simon Horman <horms@kernel.org> > > > > Sorry, I think I have to take that back for now. > > Would you keep it if I retargeted to net-next without the Fixes: tags? Yes, I think that would address my concerns.
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index f7c4ce8e9a26..a86cfebedaa8 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1093,6 +1093,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 allow_wol) +{ + u32 val; + + if (!allow_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 @@ -1109,17 +1132,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, @@ -1373,22 +1386,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);