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

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

[Csókás, 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)
+{
+	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

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

[Michal Swiatkowski]

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

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

[Ahmad Fatoum]

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);
>  


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Simon Horman]

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.

-- 
pw-bot: changes-requested

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

[Csókás Bence]

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

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

[Andrew Lunn]

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

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

[Jakub Kicinski]

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

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

[Wei Fang]

> 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
> 

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

[Csókás, 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`, 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(-)

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

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

[Jacob Keller]

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>

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

[Wei Fang]

> 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
> 

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

[Csókás, 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`, 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>
---

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`
    Changes in v3:
    * rename parameter to `allow_wol`
    Changes in v3:
    * clarify message
    * collect Jacob's tag
    * rebased onto c2933b2befe2

 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 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);
 
-- 
2.48.1

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

[Simon Horman]

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>

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

[Simon Horman]

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/

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

[Csókás Bence]

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

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

[Simon Horman]

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.