Re: [PATCH] net: fec: Refactor MAC reset to function

[Jacob Keller <jacob.e.keller@intel.com>]

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>