[PATCH v2] net: fec: Refactor: #define magic constants

[PATCH v2] net: fec: Refactor: #define magic constants

[Csókás Bence]

Add defines for bits of ECR, RCR control registers, TX watermark etc.

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

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 63707e065141..a16220eff9b3 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -85,8 +85,6 @@ static int fec_enet_xdp_tx_xmit(struct fec_enet_private *fep,
 
 static const u16 fec_enet_vlan_pri_to_queue[8] = {0, 0, 1, 1, 1, 2, 2, 2};
 
-/* Pause frame feild and FIFO threshold */
-#define FEC_ENET_FCE	(1 << 5)
 #define FEC_ENET_RSEM_V	0x84
 #define FEC_ENET_RSFL_V	16
 #define FEC_ENET_RAEM_V	0x8
@@ -240,8 +238,8 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #define PKT_MINBUF_SIZE		64
 
 /* FEC receive acceleration */
-#define FEC_RACC_IPDIS		(1 << 1)
-#define FEC_RACC_PRODIS		(1 << 2)
+#define FEC_RACC_IPDIS		BIT(1)
+#define FEC_RACC_PRODIS		BIT(2)
 #define FEC_RACC_SHIFT16	BIT(7)
 #define FEC_RACC_OPTIONS	(FEC_RACC_IPDIS | FEC_RACC_PRODIS)
 
@@ -273,8 +271,23 @@ 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_ECR_BYTESWP         BIT(8)
+/* FEC RCR bits definition */
+#define FEC_RCR_LOOP            BIT(0)
+#define FEC_RCR_HALFDPX         BIT(1)
+#define FEC_RCR_MII             BIT(2)
+#define FEC_RCR_PROMISC         BIT(3)
+#define FEC_RCR_BC_REJ          BIT(4)
+#define FEC_RCR_FLOWCTL         BIT(5)
+#define FEC_RCR_RMII            BIT(8)
+#define FEC_RCR_10BASET         BIT(9)
+/* TX WMARK bits */
+#define FEC_TXWMRK_STRFWD       BIT(8)
 
 #define FEC_MII_TIMEOUT		30000 /* us */
 
@@ -1137,18 +1150,18 @@ fec_restart(struct net_device *ndev)
 		    fep->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
 			rcntl |= (1 << 6);
 		else if (fep->phy_interface == PHY_INTERFACE_MODE_RMII)
-			rcntl |= (1 << 8);
+			rcntl |= FEC_RCR_RMII;
 		else
-			rcntl &= ~(1 << 8);
+			rcntl &= ~FEC_RCR_RMII;
 
 		/* 1G, 100M or 10M */
 		if (ndev->phydev) {
 			if (ndev->phydev->speed == SPEED_1000)
 				ecntl |= (1 << 5);
 			else if (ndev->phydev->speed == SPEED_100)
-				rcntl &= ~(1 << 9);
+				rcntl &= ~FEC_RCR_10BASET;
 			else
-				rcntl |= (1 << 9);
+				rcntl |= FEC_RCR_10BASET;
 		}
 	} else {
 #ifdef FEC_MIIGSK_ENR
@@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev)
 	if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) ||
 	    ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) &&
 	     ndev->phydev && ndev->phydev->pause)) {
-		rcntl |= FEC_ENET_FCE;
+		rcntl |= FEC_RCR_FLOWCTL;
 
 		/* set FIFO threshold parameter to reduce overrun */
 		writel(FEC_ENET_RSEM_V, fep->hwp + FEC_R_FIFO_RSEM);
@@ -1192,7 +1205,7 @@ fec_restart(struct net_device *ndev)
 		/* OPD */
 		writel(FEC_ENET_OPD_V, fep->hwp + FEC_OPD);
 	} else {
-		rcntl &= ~FEC_ENET_FCE;
+		rcntl &= ~FEC_RCR_FLOWCTL;
 	}
 #endif /* !defined(CONFIG_M5272) */
 
@@ -1207,13 +1220,13 @@ fec_restart(struct net_device *ndev)
 
 	if (fep->quirks & FEC_QUIRK_ENET_MAC) {
 		/* enable ENET endian swap */
-		ecntl |= (1 << 8);
+		ecntl |= FEC_ECR_BYTESWP;
 		/* enable ENET store and forward mode */
-		writel(1 << 8, fep->hwp + FEC_X_WMRK);
+		writel(FEC_TXWMRK_STRFWD, fep->hwp + FEC_X_WMRK);
 	}
 
 	if (fep->bufdesc_ex)
-		ecntl |= (1 << 4);
+		ecntl |= FEC_ECR_EN1588;
 
 	if (fep->quirks & FEC_QUIRK_DELAYED_CLKS_SUPPORT &&
 	    fep->rgmii_txc_dly)
@@ -1312,7 +1325,8 @@ static void
 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 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & FEC_RCR_RMII;
+	u32 ecntl = 0;
 	u32 val;
 
 	/* We cannot expect a graceful transmit stop without link !!! */
@@ -1345,9 +1359,11 @@ fec_stop(struct net_device *ndev)
 	/* 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 v2] net: fec: Refactor: #define magic constants

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 5406 bytes --]

On 09.02.2024 10:11:01, Csókás Bence wrote:
> Add defines for bits of ECR, RCR control registers, TX watermark etc.
> 
> Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 50 +++++++++++++++--------
>  1 file changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 63707e065141..a16220eff9b3 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -85,8 +85,6 @@ static int fec_enet_xdp_tx_xmit(struct fec_enet_private *fep,
>  
>  static const u16 fec_enet_vlan_pri_to_queue[8] = {0, 0, 1, 1, 1, 2, 2, 2};
>  
> -/* Pause frame feild and FIFO threshold */
> -#define FEC_ENET_FCE	(1 << 5)
>  #define FEC_ENET_RSEM_V	0x84
>  #define FEC_ENET_RSFL_V	16
>  #define FEC_ENET_RAEM_V	0x8
> @@ -240,8 +238,8 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
>  #define PKT_MINBUF_SIZE		64
>  
>  /* FEC receive acceleration */
> -#define FEC_RACC_IPDIS		(1 << 1)
> -#define FEC_RACC_PRODIS		(1 << 2)
> +#define FEC_RACC_IPDIS		BIT(1)
> +#define FEC_RACC_PRODIS		BIT(2)
>  #define FEC_RACC_SHIFT16	BIT(7)
>  #define FEC_RACC_OPTIONS	(FEC_RACC_IPDIS | FEC_RACC_PRODIS)
>  
> @@ -273,8 +271,23 @@ 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_ECR_BYTESWP         BIT(8)
> +/* FEC RCR bits definition */
> +#define FEC_RCR_LOOP            BIT(0)
> +#define FEC_RCR_HALFDPX         BIT(1)
> +#define FEC_RCR_MII             BIT(2)
> +#define FEC_RCR_PROMISC         BIT(3)
> +#define FEC_RCR_BC_REJ          BIT(4)
> +#define FEC_RCR_FLOWCTL         BIT(5)
> +#define FEC_RCR_RMII            BIT(8)
> +#define FEC_RCR_10BASET         BIT(9)
> +/* TX WMARK bits */
> +#define FEC_TXWMRK_STRFWD       BIT(8)
>  
>  #define FEC_MII_TIMEOUT		30000 /* us */
>  
> @@ -1137,18 +1150,18 @@ fec_restart(struct net_device *ndev)
>  		    fep->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
>  			rcntl |= (1 << 6);
>  		else if (fep->phy_interface == PHY_INTERFACE_MODE_RMII)
> -			rcntl |= (1 << 8);
> +			rcntl |= FEC_RCR_RMII;
>  		else
> -			rcntl &= ~(1 << 8);
> +			rcntl &= ~FEC_RCR_RMII;
>  
>  		/* 1G, 100M or 10M */
>  		if (ndev->phydev) {
>  			if (ndev->phydev->speed == SPEED_1000)
>  				ecntl |= (1 << 5);
>  			else if (ndev->phydev->speed == SPEED_100)
> -				rcntl &= ~(1 << 9);
> +				rcntl &= ~FEC_RCR_10BASET;
>  			else
> -				rcntl |= (1 << 9);
> +				rcntl |= FEC_RCR_10BASET;
>  		}
>  	} else {
>  #ifdef FEC_MIIGSK_ENR
> @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev)
>  	if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) ||
>  	    ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) &&
>  	     ndev->phydev && ndev->phydev->pause)) {
> -		rcntl |= FEC_ENET_FCE;
> +		rcntl |= FEC_RCR_FLOWCTL;
>  
>  		/* set FIFO threshold parameter to reduce overrun */
>  		writel(FEC_ENET_RSEM_V, fep->hwp + FEC_R_FIFO_RSEM);
> @@ -1192,7 +1205,7 @@ fec_restart(struct net_device *ndev)
>  		/* OPD */
>  		writel(FEC_ENET_OPD_V, fep->hwp + FEC_OPD);
>  	} else {
> -		rcntl &= ~FEC_ENET_FCE;
> +		rcntl &= ~FEC_RCR_FLOWCTL;
>  	}
>  #endif /* !defined(CONFIG_M5272) */
>  
> @@ -1207,13 +1220,13 @@ fec_restart(struct net_device *ndev)
>  
>  	if (fep->quirks & FEC_QUIRK_ENET_MAC) {
>  		/* enable ENET endian swap */
> -		ecntl |= (1 << 8);
> +		ecntl |= FEC_ECR_BYTESWP;
>  		/* enable ENET store and forward mode */
> -		writel(1 << 8, fep->hwp + FEC_X_WMRK);
> +		writel(FEC_TXWMRK_STRFWD, fep->hwp + FEC_X_WMRK);
>  	}
>  
>  	if (fep->bufdesc_ex)
> -		ecntl |= (1 << 4);
> +		ecntl |= FEC_ECR_EN1588;
>  
>  	if (fep->quirks & FEC_QUIRK_DELAYED_CLKS_SUPPORT &&
>  	    fep->rgmii_txc_dly)
> @@ -1312,7 +1325,8 @@ static void
>  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 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & FEC_RCR_RMII;
> +	u32 ecntl = 0;

This is an unrelated change.

>  	u32 val;
>  
>  	/* We cannot expect a graceful transmit stop without link !!! */
> @@ -1345,9 +1359,11 @@ fec_stop(struct net_device *ndev)
>  	/* 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;

This is an unrelated change.

>  		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
>  	}
> +
> +	writel(ecntl, fep->hwp + FEC_ECNTRL);

This is an unrelated change.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] net: fec: Refactor: #define magic constants

[Denis Kirjanov]

On 2/9/24 12:11, Csókás Bence wrote:
> Add defines for bits of ECR, RCR control registers, TX watermark etc.
> 
> Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>

Please add net-next prefix

> ---
>  drivers/net/ethernet/freescale/fec_main.c | 50 +++++++++++++++--------
>  1 file changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 63707e065141..a16220eff9b3 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -85,8 +85,6 @@ static int fec_enet_xdp_tx_xmit(struct fec_enet_private *fep,
>  
>  static const u16 fec_enet_vlan_pri_to_queue[8] = {0, 0, 1, 1, 1, 2, 2, 2};
>  
> -/* Pause frame feild and FIFO threshold */
> -#define FEC_ENET_FCE	(1 << 5)
>  #define FEC_ENET_RSEM_V	0x84
>  #define FEC_ENET_RSFL_V	16
>  #define FEC_ENET_RAEM_V	0x8
> @@ -240,8 +238,8 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
>  #define PKT_MINBUF_SIZE		64
>  
>  /* FEC receive acceleration */
> -#define FEC_RACC_IPDIS		(1 << 1)
> -#define FEC_RACC_PRODIS		(1 << 2)
> +#define FEC_RACC_IPDIS		BIT(1)
> +#define FEC_RACC_PRODIS		BIT(2)
>  #define FEC_RACC_SHIFT16	BIT(7)
>  #define FEC_RACC_OPTIONS	(FEC_RACC_IPDIS | FEC_RACC_PRODIS)
>  
> @@ -273,8 +271,23 @@ 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_ECR_BYTESWP         BIT(8)
> +/* FEC RCR bits definition */
> +#define FEC_RCR_LOOP            BIT(0)
> +#define FEC_RCR_HALFDPX         BIT(1)
> +#define FEC_RCR_MII             BIT(2)
> +#define FEC_RCR_PROMISC         BIT(3)
> +#define FEC_RCR_BC_REJ          BIT(4)
> +#define FEC_RCR_FLOWCTL         BIT(5)
> +#define FEC_RCR_RMII            BIT(8)
> +#define FEC_RCR_10BASET         BIT(9)
> +/* TX WMARK bits */
> +#define FEC_TXWMRK_STRFWD       BIT(8)
>  
>  #define FEC_MII_TIMEOUT		30000 /* us */
>  
> @@ -1137,18 +1150,18 @@ fec_restart(struct net_device *ndev)
>  		    fep->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
>  			rcntl |= (1 << 6);
>  		else if (fep->phy_interface == PHY_INTERFACE_MODE_RMII)
> -			rcntl |= (1 << 8);
> +			rcntl |= FEC_RCR_RMII;
>  		else
> -			rcntl &= ~(1 << 8);
> +			rcntl &= ~FEC_RCR_RMII;
>  
>  		/* 1G, 100M or 10M */
>  		if (ndev->phydev) {
>  			if (ndev->phydev->speed == SPEED_1000)
>  				ecntl |= (1 << 5);
>  			else if (ndev->phydev->speed == SPEED_100)
> -				rcntl &= ~(1 << 9);
> +				rcntl &= ~FEC_RCR_10BASET;
>  			else
> -				rcntl |= (1 << 9);
> +				rcntl |= FEC_RCR_10BASET;
>  		}
>  	} else {
>  #ifdef FEC_MIIGSK_ENR
> @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev)
>  	if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) ||
>  	    ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) &&
>  	     ndev->phydev && ndev->phydev->pause)) {
> -		rcntl |= FEC_ENET_FCE;
> +		rcntl |= FEC_RCR_FLOWCTL;
>  
>  		/* set FIFO threshold parameter to reduce overrun */
>  		writel(FEC_ENET_RSEM_V, fep->hwp + FEC_R_FIFO_RSEM);
> @@ -1192,7 +1205,7 @@ fec_restart(struct net_device *ndev)
>  		/* OPD */
>  		writel(FEC_ENET_OPD_V, fep->hwp + FEC_OPD);
>  	} else {
> -		rcntl &= ~FEC_ENET_FCE;
> +		rcntl &= ~FEC_RCR_FLOWCTL;
>  	}
>  #endif /* !defined(CONFIG_M5272) */
>  
> @@ -1207,13 +1220,13 @@ fec_restart(struct net_device *ndev)
>  
>  	if (fep->quirks & FEC_QUIRK_ENET_MAC) {
>  		/* enable ENET endian swap */
> -		ecntl |= (1 << 8);
> +		ecntl |= FEC_ECR_BYTESWP;
>  		/* enable ENET store and forward mode */
> -		writel(1 << 8, fep->hwp + FEC_X_WMRK);
> +		writel(FEC_TXWMRK_STRFWD, fep->hwp + FEC_X_WMRK);
>  	}
>  
>  	if (fep->bufdesc_ex)
> -		ecntl |= (1 << 4);
> +		ecntl |= FEC_ECR_EN1588;
>  
>  	if (fep->quirks & FEC_QUIRK_DELAYED_CLKS_SUPPORT &&
>  	    fep->rgmii_txc_dly)
> @@ -1312,7 +1325,8 @@ static void
>  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 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & FEC_RCR_RMII;
> +	u32 ecntl = 0;
>  	u32 val;
>  
>  	/* We cannot expect a graceful transmit stop without link !!! */
> @@ -1345,9 +1359,11 @@ fec_stop(struct net_device *ndev)
>  	/* 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);
>  }
>  
>  

Re: [PATCH v2] net: fec: Refactor: #define magic constants

[Andrew Lunn]

> @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev)
>  	if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) ||
>  	    ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) &&
>  	     ndev->phydev && ndev->phydev->pause)) {
> -		rcntl |= FEC_ENET_FCE;
> +		rcntl |= FEC_RCR_FLOWCTL;

This immediately stood out to me while looking at the diff. Its not
obvious why this is correct. Looking back, i see you removed
FEC_ENET_FCE, not renamed it.

Ideally, you want lots of small patches which are obviously correct.
This change is not obvious, there is no explanation in the commit
message etc.

Please keep this patch about straight, obvious, replacement of bit
shifts with macros.

Do all other changes in additional patches. It is much easier to
review then, both by you before you post, and us when it hits the
list.

       Andrew

Re: [PATCH v2] net: fec: Refactor: #define magic constants

[Csókás Bence]

Hi!

2024. 02. 09. 14:53 keltezéssel, Andrew Lunn írta:
>> @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev)
>>   	if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) ||
>>   	    ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) &&
>>   	     ndev->phydev && ndev->phydev->pause)) {
>> -		rcntl |= FEC_ENET_FCE;
>> +		rcntl |= FEC_RCR_FLOWCTL;
> 
> This immediately stood out to me while looking at the diff. Its not
> obvious why this is correct. Looking back, i see you removed
> FEC_ENET_FCE, not renamed it.

What do you mean? I replaced FEC_ENET_FCE with FEC_RCR_FLOWCTL, to make 
it obvious that it represents a bit in RCR (or `rcntl` as it is called 
on this line). How is that not "renaming" it?

> Ideally, you want lots of small patches which are obviously correct.
> This change is not obvious, there is no explanation in the commit
> message etc.
> 
> Please keep this patch about straight, obvious, replacement of bit
> shifts with macros.

So, how should I break it up then? One patch for the ECR bits, one for 
RCR, one for TX watermark register, one for RACC? Or one commit 
introducing the constants, and another replacing usages with these?

> Do all other changes in additional patches. It is much easier to
> review then, both by you before you post, and us when it hits the
> list.
> 
>         Andrew
> 

Thanks,
Bence

Re: [PATCH v2] net: fec: Refactor: #define magic constants

[Andrew Lunn]

On Mon, Feb 12, 2024 at 03:49:42PM +0100, Csókás Bence wrote:
> Hi!
> 
> 2024. 02. 09. 14:53 keltezéssel, Andrew Lunn írta:
> > > @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev)
> > >   	if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) ||
> > >   	    ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) &&
> > >   	     ndev->phydev && ndev->phydev->pause)) {
> > > -		rcntl |= FEC_ENET_FCE;
> > > +		rcntl |= FEC_RCR_FLOWCTL;
> > 
> > This immediately stood out to me while looking at the diff. Its not
> > obvious why this is correct. Looking back, i see you removed
> > FEC_ENET_FCE, not renamed it.
> 
> What do you mean? I replaced FEC_ENET_FCE with FEC_RCR_FLOWCTL, to make it
> obvious that it represents a bit in RCR (or `rcntl` as it is called on this
> line). How is that not "renaming" it?

Going from FEC_NET_ to FEC_RCR_ in itself makes me ask questions. Was
it wrong before? Is this actually a fix? Is it correct now, or is this
a cut/paste typo? Looking at the rest of the patch there is no obvious
answer. As i said, you deleted FEC_ENET_FCE, but there is no
explanation why.

So what i'm asking for is obviously correct patches. You can add the
#defines, and replace (1 << X) with one of the new macros, and it
should be obvious.

However, the change above is not obviously correct, so some
explanation is required. And it is easier to do that in a patch
dedicated to this change, with a good explanation.

	Andrew

Re: [PATCH v2] net: fec: Refactor: #define magic constants

[Csókás Bence]

2024. 02. 12. 16:04 keltezéssel, Andrew Lunn írta:
> On Mon, Feb 12, 2024 at 03:49:42PM +0100, Csókás Bence wrote:
>> Hi!
>>
>> 2024. 02. 09. 14:53 keltezéssel, Andrew Lunn írta:
>>>> @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev)
>>>>    	if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) ||
>>>>    	    ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) &&
>>>>    	     ndev->phydev && ndev->phydev->pause)) {
>>>> -		rcntl |= FEC_ENET_FCE;
>>>> +		rcntl |= FEC_RCR_FLOWCTL;
>>>
>>> This immediately stood out to me while looking at the diff. Its not
>>> obvious why this is correct. Looking back, i see you removed
>>> FEC_ENET_FCE, not renamed it.
>>
>> What do you mean? I replaced FEC_ENET_FCE with FEC_RCR_FLOWCTL, to make it
>> obvious that it represents a bit in RCR (or `rcntl` as it is called on this
>> line). How is that not "renaming" it?
> 
> Going from FEC_NET_ to FEC_RCR_ in itself makes me ask questions. Was
> it wrong before? Is this actually a fix? Is it correct now, or is this
> a cut/paste typo? Looking at the rest of the patch there is no obvious
> answer. As i said, you deleted FEC_ENET_FCE, but there is no
> explanation why.

The name `FEC_ENET_FCE` does not tell us that this is the FCE (Flow 
Control Enable) bit (1 << 5) of the RCR (Receive Control Register). I 
added FEC_RCR_* macros for all RCR bits, and I named BIT(5) 
FEC_RCR_FLOWCTL, a much more descriptive name (in my opinion, at least).

> So what i'm asking for is obviously correct patches. You can add the
> #defines, and replace (1 << X) with one of the new macros, and it
> should be obvious.

I replaced `#define FEC_ENET_FCE (1 << 5)` with `#define FEC_RCR_FLOWCTL 
         BIT(5)`. I thought that was "obviously correct", but I can 
break the patch up more, if it helps readability.

> However, the change above is not obviously correct, so some
> explanation is required. And it is easier to do that in a patch
> dedicated to this change, with a good explanation.

So, a separate patch just for removing FEC_ENET_FCE and replacing all 
usages with FEC_RCR_FLOWCTL? And the rest can stay as-is?

> 
> 	Andrew
> 

Bence

Re: [PATCH v2] net: fec: Refactor: #define magic constants

[Andrew Lunn]

On Mon, Feb 12, 2024 at 04:11:10PM +0100, Csókás Bence wrote:
> 
> 
> 2024. 02. 12. 16:04 keltezéssel, Andrew Lunn írta:
> > On Mon, Feb 12, 2024 at 03:49:42PM +0100, Csókás Bence wrote:
> > > Hi!
> > > 
> > > 2024. 02. 09. 14:53 keltezéssel, Andrew Lunn írta:
> > > > > @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev)
> > > > >    	if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) ||
> > > > >    	    ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) &&
> > > > >    	     ndev->phydev && ndev->phydev->pause)) {
> > > > > -		rcntl |= FEC_ENET_FCE;
> > > > > +		rcntl |= FEC_RCR_FLOWCTL;
> > > > 
> > > > This immediately stood out to me while looking at the diff. Its not
> > > > obvious why this is correct. Looking back, i see you removed
> > > > FEC_ENET_FCE, not renamed it.
> > > 
> > > What do you mean? I replaced FEC_ENET_FCE with FEC_RCR_FLOWCTL, to make it
> > > obvious that it represents a bit in RCR (or `rcntl` as it is called on this
> > > line). How is that not "renaming" it?
> > 
> > Going from FEC_NET_ to FEC_RCR_ in itself makes me ask questions. Was
> > it wrong before? Is this actually a fix? Is it correct now, or is this
> > a cut/paste typo? Looking at the rest of the patch there is no obvious
> > answer. As i said, you deleted FEC_ENET_FCE, but there is no
> > explanation why.
> 
> The name `FEC_ENET_FCE` does not tell us that this is the FCE (Flow Control
> Enable) bit (1 << 5) of the RCR (Receive Control Register). I added
> FEC_RCR_* macros for all RCR bits, and I named BIT(5) FEC_RCR_FLOWCTL, a
> much more descriptive name (in my opinion, at least).

Some form of that would be good in the commit message. It explains the
'Why?' of the change.

> So, a separate patch just for removing FEC_ENET_FCE and replacing all usages
> with FEC_RCR_FLOWCTL? And the rest can stay as-is?

A few others made review comments as well. It could be addressing
those comments also requires more small patches. Sometimes you can
avoid review comments by thinking, what are reviewers going to ask,
and putting the answer to those questions in the commit message. This
might all seam like a lot of hassle now, but it will help getting your
future patchsets merged if you follow this advice.

      Andrew

Re: [PATCH v2] net: fec: Refactor: #define magic constants

[Csókás Bence]

2024. 02. 12. 17:03 keltezéssel, Andrew Lunn írta:
> On Mon, Feb 12, 2024 at 04:11:10PM +0100, Csókás Bence wrote:
>>
>>
>> 2024. 02. 12. 16:04 keltezéssel, Andrew Lunn írta:
>>> On Mon, Feb 12, 2024 at 03:49:42PM +0100, Csókás Bence wrote:
>>>> Hi!
>>>>
>>>> 2024. 02. 09. 14:53 keltezéssel, Andrew Lunn írta:
>>>>>> @@ -1181,7 +1194,7 @@ fec_restart(struct net_device *ndev)
>>>>>>     	if ((fep->pause_flag & FEC_PAUSE_FLAG_ENABLE) ||
>>>>>>     	    ((fep->pause_flag & FEC_PAUSE_FLAG_AUTONEG) &&
>>>>>>     	     ndev->phydev && ndev->phydev->pause)) {
>>>>>> -		rcntl |= FEC_ENET_FCE;
>>>>>> +		rcntl |= FEC_RCR_FLOWCTL;
>>>>>
>>>>> This immediately stood out to me while looking at the diff. Its not
>>>>> obvious why this is correct. Looking back, i see you removed
>>>>> FEC_ENET_FCE, not renamed it.
>>>>
>>>> What do you mean? I replaced FEC_ENET_FCE with FEC_RCR_FLOWCTL, to make it
>>>> obvious that it represents a bit in RCR (or `rcntl` as it is called on this
>>>> line). How is that not "renaming" it?
>>>
>>> Going from FEC_NET_ to FEC_RCR_ in itself makes me ask questions. Was
>>> it wrong before? Is this actually a fix? Is it correct now, or is this
>>> a cut/paste typo? Looking at the rest of the patch there is no obvious
>>> answer. As i said, you deleted FEC_ENET_FCE, but there is no
>>> explanation why.
>>
>> The name `FEC_ENET_FCE` does not tell us that this is the FCE (Flow Control
>> Enable) bit (1 << 5) of the RCR (Receive Control Register). I added
>> FEC_RCR_* macros for all RCR bits, and I named BIT(5) FEC_RCR_FLOWCTL, a
>> much more descriptive name (in my opinion, at least).
> 
> Some form of that would be good in the commit message. It explains the
> 'Why?' of the change.

Ok. I split that change into a separate patch with a quick summary of 
this, in v4 of this patch. Hopefully it is more clear now.

>> So, a separate patch just for removing FEC_ENET_FCE and replacing all usages
>> with FEC_RCR_FLOWCTL? And the rest can stay as-is?
> 
> A few others made review comments as well. It could be addressing
> those comments also requires more small patches. Sometimes you can
> avoid review comments by thinking, what are reviewers going to ask,
> and putting the answer to those questions in the commit message. This
> might all seam like a lot of hassle now, but it will help getting your
> future patchsets merged if you follow this advice.
> 
>        Andrew

I believe I addressed all other comments already, but do enlighten me if 
that is not the case:
* removed the "fix FEC_ECR_EN1588 being cleared on link-down" half of 
the patch (v2) - your feedback
* removed `u32 ecntl` from `fec_stop()` (v3) - requested by Marc
* added net-next subject-prefix (v3) - suggested by Denis
* factored out the removal of FEC_ENET_FCE (v4) - your feedback

Did I miss something?

Bence