| Message ID | 20250204093756.253642-2-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] net: fec: Refactor MAC reset to function | expand |
On Tue, 4 Feb 2025 10:37:54 +0100 Csókás, Bence wrote: > 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`. 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.
Hi, On 2025. 02. 04. 16:45, Jakub Kicinski wrote: > Please don't post new versions in-reply-to, and add lore links to > the previous version in the changelog. Will do. Is it okay to only include the last version, or should I collect them going back to v1? > On Tue, 4 Feb 2025 10:37:54 +0100 Csókás, Bence wrote: >> 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`. > > 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. True, but he also said: On 2025. 01. 21. 17:09, Badel, Laurent wrote: > 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. I understand he is cautious, but I'd argue that the fact that two people already posted Reviewed-by: (not counting Simon, who since withdrew it), means that others also agree that we should err on the OTHER side of caution, and do the check in both cases. He also mentions that the reason he didn't do the check in `fec_stop()` was that he believed that the only time that gets called is on driver/interface remove, but that is not the case, as I outlined in the message already. Bence
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);