Skip to contentSkip to conversation

net: stmmac: qcon-ethqos: "rgmii" accessor cleanups

PATCH Superseded Russell King (Oracle) 3 participants 9 messages Nov 19, 2025 – Nov 20, 2025

Subsystems (6)
Networking Network drivers Arm Qualcomm mailing list Arm STM32 +2 more subsystems

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Russell King (Oracle)

author
#0 lore raw

cover net: stmmac: qcon-ethqos: "rgmii" accessor cleanups

This series cleans up the "rgmii" accessors in qcom-ethqos.

readl() and writel() return and take a u32 for the value. Rather than implicitly casting this to an int, keep it as a u32.

Add set/clear functions to reduce the code and make it easier to read.

Finally, convert the open-coded poll loops to use the iopoll helpers.

 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 245 +++++++++------------
 1 file changed, 110 insertions(+), 135 deletions(-)
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
#1 lore raw

PATCH 1/3 net: stmmac: qcom-ethqos: use u32 for rgmii read/write/update

readl() returns a u32, and writel() takes a "u32" for the value. These are used in rgmii_readl()() and rgmii_writel(), but the value and return are "int". As these are 32-bit register values which are not signed, use "u32".

These changes do not cause generated code changes.

Update rgmii_updatel() to use u32 for mask and val. Changing "mask" to "u32" also does not cause generated code changes. However, changing "val" causes the generated assembly to be re-ordered for aarch64.

Update the temporary variables used with the rgmii functions to use u32.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c  | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +8 -8
index 1a616a71c36a..ae3cf163005b 100644
@@ -116,21 +116,21 @@ struct qcom_ethqos {
 	bool needs_sgmii_loopback;
 };
 
-static int rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
+static u32 rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
 {
 	return readl(ethqos->rgmii_base + offset);
 }
 
-static void rgmii_writel(struct qcom_ethqos *ethqos,
-			 int value, unsigned int offset)
+static void rgmii_writel(struct qcom_ethqos *ethqos, u32 value,
+			 unsigned int offset)
 {
 	writel(value, ethqos->rgmii_base + offset);
 }
 
-static void rgmii_updatel(struct qcom_ethqos *ethqos,
-			  int mask, int val, unsigned int offset)
+static void rgmii_updatel(struct qcom_ethqos *ethqos, u32 mask, u32 val,
+			  unsigned int offset)
 {
-	unsigned int temp;
+	u32 temp;
 
 	temp = rgmii_readl(ethqos, offset);
 	temp = (temp & ~(mask)) | val;

@@ -300,8 +300,8 @@ static const struct ethqos_emac_driver_data emac_v4_0_0_data = {
 static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
 {
 	struct device *dev = &ethqos->pdev->dev;
-	unsigned int val;
 	int retry = 1000;
+	u32 val;
 
 	/* Set CDR_EN */
 	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_CDR_EN,

@@ -535,7 +535,7 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
 static int ethqos_configure_rgmii(struct qcom_ethqos *ethqos, int speed)
 {
 	struct device *dev = &ethqos->pdev->dev;
-	volatile unsigned int dll_lock;
+	volatile u32 dll_lock;
 	unsigned int i, retry = 1000;
 
 	/* Reset to POR values and enable clk */
Signature · 1 line
2.47.3
Reviewed-by on v1 1/3
#2 lore raw

Re: [PATCH net-next 1/3] net: stmmac: qcom-ethqos: use u32 for rgmii read/write/update

Quoted 10 lines · Russell King (Oracle)

readl() returns a u32, and writel() takes a "u32" for the value. These are used in rgmii_readl()() and rgmii_writel(), but the value and return are "int". As these are 32-bit register values which are not signed, use "u32".

These changes do not cause generated code changes.

Update rgmii_updatel() to use u32 for mask and val. Changing "mask" to "u32" also does not cause generated code changes. However, changing "val" causes the generated assembly to be re-ordered for aarch64.

No changes, on clang 21.1.5 at least

Quoted 6 lines

Update the temporary variables used with the rgmii functions to use u32.

Signed-off-by Russell King (Oracle) <[email protected]>

---

Reviewed-by Konrad Dybcio <[email protected]>

Konrad

#3 lore raw

PATCH 2/3 net: stmmac: qcom-ethqos: add rgmii set/clear functions

In reply to Russell King (Oracle)

The driver has a lot of bit manipulation of the RGMII registers. Add a pair of helpers to set bits and clear bits, converting the various calls to rgmii_updatel() as appropriate.

Most of the change was done via this sed script:

/rgmii_updatel/ {
	N
	/,$/N
	/mask, / ! {
		s|rgmii_updatel\(([^,]*,\s+([^,]*),\s+)\2,\s+|rgmii_setmask(\1|
		s|rgmii_updatel\(([^,]*,\s+([^,]*),\s+)0,\s+|rgmii_clrmask(\1|
		s|^\s+$||
	}
}

and then formatting tweaked where necessary.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 .../stmicro/stmmac/dwmac-qcom-ethqos.c        | 187 +++++++++---------
 1 file changed, 89 insertions(+), 98 deletions(-)
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +89 -98
index ae3cf163005b..cdaf02471d3a 100644
@@ -137,6 +137,18 @@ static void rgmii_updatel(struct qcom_ethqos *ethqos, u32 mask, u32 val,
 	rgmii_writel(ethqos, temp, offset);
 }
 
+static void rgmii_setmask(struct qcom_ethqos *ethqos, u32 mask,
+			  unsigned int offset)
+{
+	rgmii_updatel(ethqos, mask, mask, offset);
+}
+
+static void rgmii_clrmask(struct qcom_ethqos *ethqos, u32 mask,
+			  unsigned int offset)
+{
+	rgmii_updatel(ethqos, mask, 0, offset);
+}
+
 static void rgmii_dump(void *priv)
 {
 	struct qcom_ethqos *ethqos = priv;

@@ -194,8 +206,7 @@ qcom_ethqos_set_sgmii_loopback(struct qcom_ethqos *ethqos, bool enable)
 static void ethqos_set_func_clk_en(struct qcom_ethqos *ethqos)
 {
 	qcom_ethqos_set_sgmii_loopback(ethqos, true);
-	rgmii_updatel(ethqos, RGMII_CONFIG_FUNC_CLK_EN,
-		      RGMII_CONFIG_FUNC_CLK_EN, RGMII_IO_MACRO_CONFIG);
+	rgmii_setmask(ethqos, RGMII_CONFIG_FUNC_CLK_EN, RGMII_IO_MACRO_CONFIG);
 }
 
 static const struct ethqos_emac_por emac_v2_3_0_por[] = {

@@ -304,27 +315,25 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
 	u32 val;
 
 	/* Set CDR_EN */
-	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_CDR_EN,
-		      SDCC_DLL_CONFIG_CDR_EN, SDCC_HC_REG_DLL_CONFIG);
+	rgmii_setmask(ethqos, SDCC_DLL_CONFIG_CDR_EN, SDCC_HC_REG_DLL_CONFIG);
 
 	/* Set CDR_EXT_EN */
-	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_CDR_EXT_EN,
-		      SDCC_DLL_CONFIG_CDR_EXT_EN, SDCC_HC_REG_DLL_CONFIG);
+	rgmii_setmask(ethqos, SDCC_DLL_CONFIG_CDR_EXT_EN,
+		      SDCC_HC_REG_DLL_CONFIG);
 
 	/* Clear CK_OUT_EN */
-	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_CK_OUT_EN,
-		      0, SDCC_HC_REG_DLL_CONFIG);
+	rgmii_clrmask(ethqos, SDCC_DLL_CONFIG_CK_OUT_EN,
+		      SDCC_HC_REG_DLL_CONFIG);
 
 	/* Set DLL_EN */
-	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_DLL_EN,
-		      SDCC_DLL_CONFIG_DLL_EN, SDCC_HC_REG_DLL_CONFIG);
+	rgmii_setmask(ethqos, SDCC_DLL_CONFIG_DLL_EN,  SDCC_HC_REG_DLL_CONFIG);
 
 	if (!ethqos->has_emac_ge_3) {
-		rgmii_updatel(ethqos, SDCC_DLL_MCLK_GATING_EN,
-			      0, SDCC_HC_REG_DLL_CONFIG);
+		rgmii_clrmask(ethqos, SDCC_DLL_MCLK_GATING_EN,
+			      SDCC_HC_REG_DLL_CONFIG);
 
-		rgmii_updatel(ethqos, SDCC_DLL_CDR_FINE_PHASE,
-			      0, SDCC_HC_REG_DLL_CONFIG);
+		rgmii_clrmask(ethqos, SDCC_DLL_CDR_FINE_PHASE,
+			      SDCC_HC_REG_DLL_CONFIG);
 	}
 
 	/* Wait for CK_OUT_EN clear */

@@ -340,8 +349,8 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
 		dev_err(dev, "Clear CK_OUT_EN timedout\n");
 
 	/* Set CK_OUT_EN */
-	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_CK_OUT_EN,
-		      SDCC_DLL_CONFIG_CK_OUT_EN, SDCC_HC_REG_DLL_CONFIG);
+	rgmii_setmask(ethqos, SDCC_DLL_CONFIG_CK_OUT_EN,
+		      SDCC_HC_REG_DLL_CONFIG);
 
 	/* Wait for CK_OUT_EN set */
 	retry = 1000;

@@ -357,12 +366,12 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
 		dev_err(dev, "Set CK_OUT_EN timedout\n");
 
 	/* Set DDR_CAL_EN */
-	rgmii_updatel(ethqos, SDCC_DLL_CONFIG2_DDR_CAL_EN,
-		      SDCC_DLL_CONFIG2_DDR_CAL_EN, SDCC_HC_REG_DLL_CONFIG2);
+	rgmii_setmask(ethqos, SDCC_DLL_CONFIG2_DDR_CAL_EN,
+		      SDCC_HC_REG_DLL_CONFIG2);
 
 	if (!ethqos->has_emac_ge_3) {
-		rgmii_updatel(ethqos, SDCC_DLL_CONFIG2_DLL_CLOCK_DIS,
-			      0, SDCC_HC_REG_DLL_CONFIG2);
+		rgmii_clrmask(ethqos, SDCC_DLL_CONFIG2_DLL_CLOCK_DIS,
+			      SDCC_HC_REG_DLL_CONFIG2);
 
 		rgmii_updatel(ethqos, SDCC_DLL_CONFIG2_MCLK_FREQ_CALC,
 			      0x1A << 10, SDCC_HC_REG_DLL_CONFIG2);

@@ -370,8 +379,7 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
 		rgmii_updatel(ethqos, SDCC_DLL_CONFIG2_DDR_TRAFFIC_INIT_SEL,
 			      BIT(2), SDCC_HC_REG_DLL_CONFIG2);
 
-		rgmii_updatel(ethqos, SDCC_DLL_CONFIG2_DDR_TRAFFIC_INIT_SW,
-			      SDCC_DLL_CONFIG2_DDR_TRAFFIC_INIT_SW,
+		rgmii_setmask(ethqos, SDCC_DLL_CONFIG2_DDR_TRAFFIC_INIT_SW,
 			      SDCC_HC_REG_DLL_CONFIG2);
 	}
 

@@ -392,8 +400,8 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
 		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;
 
 	/* Disable loopback mode */
-	rgmii_updatel(ethqos, RGMII_CONFIG2_TX_TO_RX_LOOPBACK_EN,
-		      0, RGMII_IO_MACRO_CONFIG2);
+	rgmii_clrmask(ethqos, RGMII_CONFIG2_TX_TO_RX_LOOPBACK_EN,
+		      RGMII_IO_MACRO_CONFIG2);
 
 	/* Determine if this platform wants loopback enabled after programming */
 	if (ethqos->rgmii_config_loopback_en)

@@ -402,29 +410,26 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
 		loopback = 0;
 
 	/* Select RGMII, write 0 to interface select */
-	rgmii_updatel(ethqos, RGMII_CONFIG_INTF_SEL,
-		      0, RGMII_IO_MACRO_CONFIG);
+	rgmii_clrmask(ethqos, RGMII_CONFIG_INTF_SEL,  RGMII_IO_MACRO_CONFIG);
 
 	switch (speed) {
 	case SPEED_1000:
-		rgmii_updatel(ethqos, RGMII_CONFIG_DDR_MODE,
-			      RGMII_CONFIG_DDR_MODE, RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG_BYPASS_TX_ID_EN,
-			      0, RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG_POS_NEG_DATA_SEL,
-			      RGMII_CONFIG_POS_NEG_DATA_SEL,
+		rgmii_setmask(ethqos, RGMII_CONFIG_DDR_MODE,
 			      RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG_PROG_SWAP,
-			      RGMII_CONFIG_PROG_SWAP, RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG2_DATA_DIVIDE_CLK_SEL,
-			      0, RGMII_IO_MACRO_CONFIG2);
+		rgmii_clrmask(ethqos, RGMII_CONFIG_BYPASS_TX_ID_EN,
+			      RGMII_IO_MACRO_CONFIG);
+		rgmii_setmask(ethqos, RGMII_CONFIG_POS_NEG_DATA_SEL,
+			      RGMII_IO_MACRO_CONFIG);
+		rgmii_setmask(ethqos, RGMII_CONFIG_PROG_SWAP,
+			      RGMII_IO_MACRO_CONFIG);
+		rgmii_clrmask(ethqos, RGMII_CONFIG2_DATA_DIVIDE_CLK_SEL,
+			      RGMII_IO_MACRO_CONFIG2);
 
 		rgmii_updatel(ethqos, RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN,
 			      phase_shift, RGMII_IO_MACRO_CONFIG2);
-		rgmii_updatel(ethqos, RGMII_CONFIG2_RSVD_CONFIG15,
-			      0, RGMII_IO_MACRO_CONFIG2);
-		rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
-			      RGMII_CONFIG2_RX_PROG_SWAP,
+		rgmii_clrmask(ethqos, RGMII_CONFIG2_RSVD_CONFIG15,
+			      RGMII_IO_MACRO_CONFIG2);
+		rgmii_setmask(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
 			      RGMII_IO_MACRO_CONFIG2);
 
 		/* PRG_RCLK_DLY = TCXO period * TCXO_CYCLES_CNT / 2 * RX delay ns,

@@ -439,87 +444,78 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
 			rgmii_updatel(ethqos, SDCC_DDR_CONFIG_PRG_RCLK_DLY,
 				      57, SDCC_HC_REG_DDR_CONFIG);
 		}
-		rgmii_updatel(ethqos, SDCC_DDR_CONFIG_PRG_DLY_EN,
-			      SDCC_DDR_CONFIG_PRG_DLY_EN,
+		rgmii_setmask(ethqos, SDCC_DDR_CONFIG_PRG_DLY_EN,
 			      SDCC_HC_REG_DDR_CONFIG);
 		rgmii_updatel(ethqos, RGMII_CONFIG_LOOPBACK_EN,
 			      loopback, RGMII_IO_MACRO_CONFIG);
 		break;
 
 	case SPEED_100:
-		rgmii_updatel(ethqos, RGMII_CONFIG_DDR_MODE,
-			      RGMII_CONFIG_DDR_MODE, RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG_BYPASS_TX_ID_EN,
-			      RGMII_CONFIG_BYPASS_TX_ID_EN,
+		rgmii_setmask(ethqos, RGMII_CONFIG_DDR_MODE,
+			      RGMII_IO_MACRO_CONFIG);
+		rgmii_setmask(ethqos, RGMII_CONFIG_BYPASS_TX_ID_EN,
+			      RGMII_IO_MACRO_CONFIG);
+		rgmii_clrmask(ethqos, RGMII_CONFIG_POS_NEG_DATA_SEL,
 			      RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG_POS_NEG_DATA_SEL,
-			      0, RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG_PROG_SWAP,
-			      0, RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG2_DATA_DIVIDE_CLK_SEL,
-			      0, RGMII_IO_MACRO_CONFIG2);
+		rgmii_clrmask(ethqos, RGMII_CONFIG_PROG_SWAP,
+			      RGMII_IO_MACRO_CONFIG);
+		rgmii_clrmask(ethqos, RGMII_CONFIG2_DATA_DIVIDE_CLK_SEL,
+			      RGMII_IO_MACRO_CONFIG2);
 		rgmii_updatel(ethqos, RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN,
 			      phase_shift, RGMII_IO_MACRO_CONFIG2);
 		rgmii_updatel(ethqos, RGMII_CONFIG_MAX_SPD_PRG_2,
 			      BIT(6), RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG2_RSVD_CONFIG15,
-			      0, RGMII_IO_MACRO_CONFIG2);
+		rgmii_clrmask(ethqos, RGMII_CONFIG2_RSVD_CONFIG15,
+			      RGMII_IO_MACRO_CONFIG2);
 
 		if (ethqos->has_emac_ge_3)
-			rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
-				      RGMII_CONFIG2_RX_PROG_SWAP,
+			rgmii_setmask(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
 				      RGMII_IO_MACRO_CONFIG2);
 		else
-			rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
-				      0, RGMII_IO_MACRO_CONFIG2);
+			rgmii_clrmask(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
+				      RGMII_IO_MACRO_CONFIG2);
 
 		/* Write 0x5 to PRG_RCLK_DLY_CODE */
 		rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE,
 			      (BIT(29) | BIT(27)), SDCC_HC_REG_DDR_CONFIG);
-		rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY,
-			      SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY,
+		rgmii_setmask(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY,
 			      SDCC_HC_REG_DDR_CONFIG);
-		rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN,
-			      SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN,
+		rgmii_setmask(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN,
 			      SDCC_HC_REG_DDR_CONFIG);
 		rgmii_updatel(ethqos, RGMII_CONFIG_LOOPBACK_EN,
 			      loopback, RGMII_IO_MACRO_CONFIG);
 		break;
 
 	case SPEED_10:
-		rgmii_updatel(ethqos, RGMII_CONFIG_DDR_MODE,
-			      RGMII_CONFIG_DDR_MODE, RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG_BYPASS_TX_ID_EN,
-			      RGMII_CONFIG_BYPASS_TX_ID_EN,
+		rgmii_setmask(ethqos, RGMII_CONFIG_DDR_MODE,
+			      RGMII_IO_MACRO_CONFIG);
+		rgmii_setmask(ethqos, RGMII_CONFIG_BYPASS_TX_ID_EN,
+			      RGMII_IO_MACRO_CONFIG);
+		rgmii_clrmask(ethqos, RGMII_CONFIG_POS_NEG_DATA_SEL,
 			      RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG_POS_NEG_DATA_SEL,
-			      0, RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG_PROG_SWAP,
-			      0, RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG2_DATA_DIVIDE_CLK_SEL,
-			      0, RGMII_IO_MACRO_CONFIG2);
+		rgmii_clrmask(ethqos, RGMII_CONFIG_PROG_SWAP,
+			      RGMII_IO_MACRO_CONFIG);
+		rgmii_clrmask(ethqos, RGMII_CONFIG2_DATA_DIVIDE_CLK_SEL,
+			      RGMII_IO_MACRO_CONFIG2);
 		rgmii_updatel(ethqos, RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN,
 			      phase_shift, RGMII_IO_MACRO_CONFIG2);
 		rgmii_updatel(ethqos, RGMII_CONFIG_MAX_SPD_PRG_9,
 			      BIT(12) | GENMASK(9, 8),
 			      RGMII_IO_MACRO_CONFIG);
-		rgmii_updatel(ethqos, RGMII_CONFIG2_RSVD_CONFIG15,
-			      0, RGMII_IO_MACRO_CONFIG2);
+		rgmii_clrmask(ethqos, RGMII_CONFIG2_RSVD_CONFIG15,
+			      RGMII_IO_MACRO_CONFIG2);
 		if (ethqos->has_emac_ge_3)
-			rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
-				      RGMII_CONFIG2_RX_PROG_SWAP,
+			rgmii_setmask(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
 				      RGMII_IO_MACRO_CONFIG2);
 		else
-			rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
-				      0, RGMII_IO_MACRO_CONFIG2);
+			rgmii_clrmask(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
+				      RGMII_IO_MACRO_CONFIG2);
 		/* Write 0x5 to PRG_RCLK_DLY_CODE */
 		rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE,
 			      (BIT(29) | BIT(27)), SDCC_HC_REG_DDR_CONFIG);
-		rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY,
-			      SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY,
+		rgmii_setmask(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY,
 			      SDCC_HC_REG_DDR_CONFIG);
-		rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN,
-			      SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN,
+		rgmii_setmask(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN,
 			      SDCC_HC_REG_DDR_CONFIG);
 		rgmii_updatel(ethqos, RGMII_CONFIG_LOOPBACK_EN,
 			      loopback, RGMII_IO_MACRO_CONFIG);

@@ -547,12 +543,12 @@ static int ethqos_configure_rgmii(struct qcom_ethqos *ethqos, int speed)
 	/* Initialize the DLL first */
 
 	/* Set DLL_RST */
-	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_DLL_RST,
-		      SDCC_DLL_CONFIG_DLL_RST, SDCC_HC_REG_DLL_CONFIG);
+	rgmii_setmask(ethqos, SDCC_DLL_CONFIG_DLL_RST,
+		      SDCC_HC_REG_DLL_CONFIG);
 
 	/* Set PDN */
-	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_PDN,
-		      SDCC_DLL_CONFIG_PDN, SDCC_HC_REG_DLL_CONFIG);
+	rgmii_setmask(ethqos, SDCC_DLL_CONFIG_PDN,
+		      SDCC_HC_REG_DLL_CONFIG);
 
 	if (ethqos->has_emac_ge_3) {
 		if (speed == SPEED_1000) {

@@ -566,21 +562,18 @@ static int ethqos_configure_rgmii(struct qcom_ethqos *ethqos, int speed)
 	}
 
 	/* Clear DLL_RST */
-	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_DLL_RST, 0,
-		      SDCC_HC_REG_DLL_CONFIG);
+	rgmii_clrmask(ethqos, SDCC_DLL_CONFIG_DLL_RST, SDCC_HC_REG_DLL_CONFIG);
 
 	/* Clear PDN */
-	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_PDN, 0,
-		      SDCC_HC_REG_DLL_CONFIG);
+	rgmii_clrmask(ethqos, SDCC_DLL_CONFIG_PDN, SDCC_HC_REG_DLL_CONFIG);
 
 	if (speed != SPEED_100 && speed != SPEED_10) {
 		/* Set DLL_EN */
-		rgmii_updatel(ethqos, SDCC_DLL_CONFIG_DLL_EN,
-			      SDCC_DLL_CONFIG_DLL_EN, SDCC_HC_REG_DLL_CONFIG);
+		rgmii_setmask(ethqos, SDCC_DLL_CONFIG_DLL_EN,
+			      SDCC_HC_REG_DLL_CONFIG);
 
 		/* Set CK_OUT_EN */
-		rgmii_updatel(ethqos, SDCC_DLL_CONFIG_CK_OUT_EN,
-			      SDCC_DLL_CONFIG_CK_OUT_EN,
+		rgmii_setmask(ethqos, SDCC_DLL_CONFIG_CK_OUT_EN,
 			      SDCC_HC_REG_DLL_CONFIG);
 
 		/* Set USR_CTL bit 26 with mask of 3 bits */

@@ -631,15 +624,13 @@ static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos, int speed)
 
 	switch (speed) {
 	case SPEED_2500:
-		rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
-			      RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
+		rgmii_setmask(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
 			      RGMII_IO_MACRO_CONFIG2);
 		ethqos_set_serdes_speed(ethqos, SPEED_2500);
 		ethqos_pcs_set_inband(priv, false);
 		break;
 	case SPEED_1000:
-		rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
-			      RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
+		rgmii_setmask(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
 			      RGMII_IO_MACRO_CONFIG2);
 		ethqos_set_serdes_speed(ethqos, SPEED_1000);
 		ethqos_pcs_set_inband(priv, true);
Signature · 1 line
2.47.3
#4 lore raw

Re: [PATCH net-next 2/3] net: stmmac: qcom-ethqos: add rgmii set/clear functions

Quoted 28 lines · Russell King (Oracle)

The driver has a lot of bit manipulation of the RGMII registers. Add a pair of helpers to set bits and clear bits, converting the various calls to rgmii_updatel() as appropriate.

Most of the change was done via this sed script:

/rgmii_updatel/ {
	N
	/,$/N
	/mask, / ! {
		s|rgmii_updatel\(([^,]*,\s+([^,]*),\s+)\2,\s+|rgmii_setmask(\1|
		s|rgmii_updatel\(([^,]*,\s+([^,]*),\s+)0,\s+|rgmii_clrmask(\1|
		s|^\s+$||
	}
}

and then formatting tweaked where necessary.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 .../stmicro/stmmac/dwmac-qcom-ethqos.c        | 187 +++++++++---------
 1 file changed, 89 insertions(+), 98 deletions(-)
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +0 -0
index ae3cf163005b..cdaf02471d3a 100644

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c static void rgmii_updatel(struct qcom_ethqos *ethqos, u32 mask, u32 val, · line 144

Quoted 8 lines
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +5 -0
@@ -137,6 +137,18 @@ static void rgmii_updatel(struct qcom_ethqos *ethqos, u32 mask, u32 val,
 	rgmii_writel(ethqos, temp, offset);
 }
 
+static void rgmii_setmask(struct qcom_ethqos *ethqos, u32 mask,
+			  unsigned int offset)
+{
+	rgmii_updatel(ethqos, mask, mask, offset);
+}

It's almost unbelieveable there's no set/clr/rmw generics for readl and friends

[...]

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c static int ethqos_dll_configure(struct qcom_ethqos *ethqos) · line 329

Quoted 4 lines
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +1 -2
 	/* Set DLL_EN */
-	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_DLL_EN,
-		      SDCC_DLL_CONFIG_DLL_EN, SDCC_HC_REG_DLL_CONFIG);
+	rgmii_setmask(ethqos, SDCC_DLL_CONFIG_DLL_EN,  SDCC_HC_REG_DLL_CONFIG);

double space

[...]

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed) · line 413

Quoted 4 lines
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +1 -2
 	/* Select RGMII, write 0 to interface select */
-	rgmii_updatel(ethqos, RGMII_CONFIG_INTF_SEL,
-		      0, RGMII_IO_MACRO_CONFIG);
+	rgmii_clrmask(ethqos, RGMII_CONFIG_INTF_SEL,  RGMII_IO_MACRO_CONFIG);

and here

Everything else looks in tact

Konrad

author
#5 lore raw

Re: [PATCH net-next 2/3] net: stmmac: qcom-ethqos: add rgmii set/clear functions

Quoted 32 lines · Konrad Dybcio
Quoted 31 lines · Russell King (Oracle)

The driver has a lot of bit manipulation of the RGMII registers. Add a pair of helpers to set bits and clear bits, converting the various calls to rgmii_updatel() as appropriate.

Most of the change was done via this sed script:

/rgmii_updatel/ {
	N
	/,$/N
	/mask, / ! {
		s|rgmii_updatel\(([^,]*,\s+([^,]*),\s+)\2,\s+|rgmii_setmask(\1|
		s|rgmii_updatel\(([^,]*,\s+([^,]*),\s+)0,\s+|rgmii_clrmask(\1|
		s|^\s+$||
	}
}

and then formatting tweaked where necessary.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 .../stmicro/stmmac/dwmac-qcom-ethqos.c        | 187 +++++++++---------
 1 file changed, 89 insertions(+), 98 deletions(-)
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +0 -0
index ae3cf163005b..cdaf02471d3a 100644
@@ -137,6 +137,18 @@ static void rgmii_updatel(struct qcom_ethqos *ethqos, u32 mask, u32 val,
 	rgmii_writel(ethqos, temp, offset);
 }
Quoted 8 lines

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c static void rgmii_updatel(struct qcom_ethqos *ethqos, u32 mask, u32 val, · line 144

Quoted 5 lines
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +5 -0
+static void rgmii_setmask(struct qcom_ethqos *ethqos, u32 mask,
+			  unsigned int offset)
+{
+	rgmii_updatel(ethqos, mask, mask, offset);
+}

It's almost unbelieveable there's no set/clr/rmw generics for readl and friends

Consider what that would mean - such operations can not be atomic, but users would likely not realise, which means we get a load of new potential bugs. Not having these means that driver authors get to code this up, and because they realise they have to do separate read and write operations, it's more obvious that there may be races.

The phy_* accessors are different - these take the bus lock while they operate, and thus are atomic.

Quoted 11 lines

[...]

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c static int ethqos_dll_configure(struct qcom_ethqos *ethqos) · line 329

Quoted 4 lines
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +1 -2
 	/* Set DLL_EN */
-	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_DLL_EN,
-		      SDCC_DLL_CONFIG_DLL_EN, SDCC_HC_REG_DLL_CONFIG);
+	rgmii_setmask(ethqos, SDCC_DLL_CONFIG_DLL_EN,  SDCC_HC_REG_DLL_CONFIG);

double space

[...]

Quoted 8 lines

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed) · line 413

Quoted 4 lines
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +1 -2
 	/* Select RGMII, write 0 to interface select */
-	rgmii_updatel(ethqos, RGMII_CONFIG_INTF_SEL,
-		      0, RGMII_IO_MACRO_CONFIG);
+	rgmii_clrmask(ethqos, RGMII_CONFIG_INTF_SEL,  RGMII_IO_MACRO_CONFIG);

and here

Everything else looks in tact

Thanks.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
#6 lore raw

Re: [PATCH net-next 2/3] net: stmmac: qcom-ethqos: add rgmii set/clear functions

Quoted 42 lines · Russell King (Oracle)
Quoted 32 lines · Konrad Dybcio
Quoted 31 lines · Russell King (Oracle)

The driver has a lot of bit manipulation of the RGMII registers. Add a pair of helpers to set bits and clear bits, converting the various calls to rgmii_updatel() as appropriate.

Most of the change was done via this sed script:

/rgmii_updatel/ {
	N
	/,$/N
	/mask, / ! {
		s|rgmii_updatel\(([^,]*,\s+([^,]*),\s+)\2,\s+|rgmii_setmask(\1|
		s|rgmii_updatel\(([^,]*,\s+([^,]*),\s+)0,\s+|rgmii_clrmask(\1|
		s|^\s+$||
	}
}

and then formatting tweaked where necessary.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 .../stmicro/stmmac/dwmac-qcom-ethqos.c        | 187 +++++++++---------
 1 file changed, 89 insertions(+), 98 deletions(-)
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +0 -0
index ae3cf163005b..cdaf02471d3a 100644
@@ -137,6 +137,18 @@ static void rgmii_updatel(struct qcom_ethqos *ethqos, u32 mask, u32 val,
 	rgmii_writel(ethqos, temp, offset);
 }
Quoted 8 lines

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c static void rgmii_updatel(struct qcom_ethqos *ethqos, u32 mask, u32 val, · line 144

Quoted 5 lines
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +5 -0
+static void rgmii_setmask(struct qcom_ethqos *ethqos, u32 mask,
+			  unsigned int offset)
+{
+	rgmii_updatel(ethqos, mask, mask, offset);
+}

It's almost unbelieveable there's no set/clr/rmw generics for readl and friends

Quoted 5 lines

Consider what that would mean - such operations can not be atomic, but users would likely not realise, which means we get a load of new potential bugs. Not having these means that driver authors get to code this up, and because they realise they have to do separate read and write operations, it's more obvious that there may be races.

Right, I don't think that would show up a lot in practice, but the 1 case it did would be exhaustively painful to debug

Konrad

#7 lore raw

PATCH 3/3 net: stmmac: qcom-ethqos: use read_poll_timeout_atomic()

In reply to Russell King (Oracle)

Use read_poll_timeout_atomic() to poll the rgmii registers rather than open-coding the polling.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 .../stmicro/stmmac/dwmac-qcom-ethqos.c        | 44 ++++++-------------
 1 file changed, 14 insertions(+), 30 deletions(-)
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +14 -30
index cdaf02471d3a..13cddd56d71f 100644
@@ -311,7 +311,6 @@ static const struct ethqos_emac_driver_data emac_v4_0_0_data = {
 static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
 {
 	struct device *dev = &ethqos->pdev->dev;
-	int retry = 1000;
 	u32 val;
 
 	/* Set CDR_EN */

@@ -337,15 +336,10 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
 	}
 
 	/* Wait for CK_OUT_EN clear */
-	do {
-		val = rgmii_readl(ethqos, SDCC_HC_REG_DLL_CONFIG);
-		val &= SDCC_DLL_CONFIG_CK_OUT_EN;
-		if (!val)
-			break;
-		mdelay(1);
-		retry--;
-	} while (retry > 0);
-	if (!retry)
+	if (read_poll_timeout_atomic(rgmii_readl, val,
+				     !(val & SDCC_DLL_CONFIG_CK_OUT_EN),
+				     1000, 1000000, false,
+				     ethqos, SDCC_HC_REG_DLL_CONFIG))
 		dev_err(dev, "Clear CK_OUT_EN timedout\n");
 
 	/* Set CK_OUT_EN */

@@ -353,16 +347,10 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
 		      SDCC_HC_REG_DLL_CONFIG);
 
 	/* Wait for CK_OUT_EN set */
-	retry = 1000;
-	do {
-		val = rgmii_readl(ethqos, SDCC_HC_REG_DLL_CONFIG);
-		val &= SDCC_DLL_CONFIG_CK_OUT_EN;
-		if (val)
-			break;
-		mdelay(1);
-		retry--;
-	} while (retry > 0);
-	if (!retry)
+	if (read_poll_timeout_atomic(rgmii_readl, val,
+				     val & SDCC_DLL_CONFIG_CK_OUT_EN,
+				     1000, 1000000, false,
+				     ethqos, SDCC_HC_REG_DLL_CONFIG))
 		dev_err(dev, "Set CK_OUT_EN timedout\n");
 
 	/* Set DDR_CAL_EN */

@@ -531,8 +519,8 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
 static int ethqos_configure_rgmii(struct qcom_ethqos *ethqos, int speed)
 {
 	struct device *dev = &ethqos->pdev->dev;
-	volatile u32 dll_lock;
-	unsigned int i, retry = 1000;
+	unsigned int i;
+	u32 val;
 
 	/* Reset to POR values and enable clk */
 	for (i = 0; i < ethqos->num_por; i++)

@@ -582,14 +570,10 @@ static int ethqos_configure_rgmii(struct qcom_ethqos *ethqos, int speed)
 				      SDCC_USR_CTL);
 
 		/* wait for DLL LOCK */
-		do {
-			mdelay(1);
-			dll_lock = rgmii_readl(ethqos, SDC4_STATUS);
-			if (dll_lock & SDC4_STATUS_DLL_LOCK)
-				break;
-			retry--;
-		} while (retry > 0);
-		if (!retry)
+		if (read_poll_timeout_atomic(rgmii_readl, val,
+					     val & SDC4_STATUS_DLL_LOCK,
+					     1000, 1000000, true,
+					     ethqos, SDC4_STATUS))
 			dev_err(dev, "Timeout while waiting for DLL lock\n");
 	}
Signature · 1 line
2.47.3