[PATCH resubmit] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down

[PATCH resubmit] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down

[Csókás Bence]

FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which
makes all 1588 functionality shut down on link-down. However, some
functionality needs to be retained (e.g. PPS) even without link.

Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>
---
 drivers/net/ethernet/freescale/fec_main.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 63707e065141..652251e48ad4 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -273,8 +273,11 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #define FEC_MMFR_TA		(2 << 16)
 #define FEC_MMFR_DATA(v)	(v & 0xffff)
 /* FEC ECR bits definition */
-#define FEC_ECR_MAGICEN		(1 << 2)
-#define FEC_ECR_SLEEP		(1 << 3)
+#define FEC_ECR_RESET   BIT(0)
+#define FEC_ECR_ETHEREN BIT(1)
+#define FEC_ECR_MAGICEN BIT(2)
+#define FEC_ECR_SLEEP   BIT(3)
+#define FEC_ECR_EN1588  BIT(4)
 
 #define FEC_MII_TIMEOUT		30000 /* us */
 
@@ -1213,7 +1216,7 @@ fec_restart(struct net_device *ndev)
 	}
 
 	if (fep->bufdesc_ex)
-		ecntl |= (1 << 4);
+		ecntl |= FEC_ECR_EN1588;
 
 	if (fep->quirks & FEC_QUIRK_DELAYED_CLKS_SUPPORT &&
 	    fep->rgmii_txc_dly)
@@ -1314,6 +1317,7 @@ fec_stop(struct net_device *ndev)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
 	u32 val;
+	u32 ecntl = 0;
 
 	/* We cannot expect a graceful transmit stop without link !!! */
 	if (fep->link) {
@@ -1342,12 +1346,17 @@ fec_stop(struct net_device *ndev)
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
 
+	if (fep->bufdesc_ex)
+		ecntl |= FEC_ECR_EN1588;
+
 	/* We have to keep ENET enabled to have MII interrupt stay working */
 	if (fep->quirks & FEC_QUIRK_ENET_MAC &&
 		!(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) {
-		writel(2, fep->hwp + FEC_ECNTRL);
+		ecntl |= FEC_ECR_ETHEREN;
 		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
 	}
+
+	writel(ecntl, fep->hwp + FEC_ECNTRL);
 }
 
 
-- 
2.25.1

Re: [PATCH resubmit] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down

[Andrew Lunn]

On Wed, Feb 07, 2024 at 01:36:11PM +0100, Csókás Bence wrote:
> FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which
> makes all 1588 functionality shut down on link-down. However, some
> functionality needs to be retained (e.g. PPS) even without link.

This is the second version of the patch, so the subject should say v2
within the [PATCH ].

Is this fixing a regression, or did it never work correctly?

> Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 63707e065141..652251e48ad4 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -273,8 +273,11 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
>  #define FEC_MMFR_TA		(2 << 16)
>  #define FEC_MMFR_DATA(v)	(v & 0xffff)
>  /* FEC ECR bits definition */
> -#define FEC_ECR_MAGICEN		(1 << 2)
> -#define FEC_ECR_SLEEP		(1 << 3)
> +#define FEC_ECR_RESET   BIT(0)
> +#define FEC_ECR_ETHEREN BIT(1)
> +#define FEC_ECR_MAGICEN BIT(2)
> +#define FEC_ECR_SLEEP   BIT(3)
> +#define FEC_ECR_EN1588  BIT(4)

There was a request to keep the indentation the same. So the BIT()
need moving right.

>  
>  #define FEC_MII_TIMEOUT		30000 /* us */
>  
> @@ -1213,7 +1216,7 @@ fec_restart(struct net_device *ndev)
>  	}
>  
>  	if (fep->bufdesc_ex)
> -		ecntl |= (1 << 4);
> +		ecntl |= FEC_ECR_EN1588;

Please could you split this into two patches. The first patch
introduced the new #defines, and uses them in the existing code. That
should be obviously correct. And a second patch adding the new
functionality, with a good commit message explaining the change,
particularly the Why?

>  
>  	if (fep->quirks & FEC_QUIRK_DELAYED_CLKS_SUPPORT &&
>  	    fep->rgmii_txc_dly)
> @@ -1314,6 +1317,7 @@ fec_stop(struct net_device *ndev)
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>  	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
>  	u32 val;
> +	u32 ecntl = 0;

Reverse Christmas tree, as pointed out in your first version of the
patch.

Thanks
    Andrew

---
pw-bot: cr

Re: [PATCH resubmit] net: fec: Add ECR bit macros, fix FEC_ECR_EN1588 being cleared on link-down

[Csókás Bence]

2024. 02. 07. 15:44 keltezéssel, Andrew Lunn írta:
> On Wed, Feb 07, 2024 at 01:36:11PM +0100, Csókás Bence wrote:
>> FEC_ECR_EN1588 bit gets cleared after MAC reset in `fec_stop()`, which
>> makes all 1588 functionality shut down on link-down. However, some
>> functionality needs to be retained (e.g. PPS) even without link.
> 
> This is the second version of the patch, so the subject should say v2
> within the [PATCH ].

This is the same as the other, just added the rationale text to the 
message body.

> Is this fixing a regression, or did it never work correctly?

PTP on this family of Ethernet controllers is utterly broken. See commit 
f79959220fa5fbda939592bf91c7a9ea90419040 (reverted). I plan on 
re-submitting that entire patch, if i can figure out the locking 
problems that have arisen.

> Please could you split this into two patches. The first patch
> introduced the new #defines, and uses them in the existing code. That
> should be obviously correct. And a second patch adding the new
> functionality, with a good commit message explaining the change,
> particularly the Why?

Sure.

> There was a request to keep the indentation the same. So the BIT()
> need moving right.

> Reverse Christmas tree, as pointed out in your first version of the
> patch.

Ah, sorry, I missed those. Also can do.

> Thanks
>      Andrew
> 
> ---
> pw-bot: cr
> 

Bence