[PATCH v2 0/5] net: sun8i-emac: Allwinner D1 Support

[PATCH v2 0/5] net: sun8i-emac: Allwinner D1 Support

[Samuel Holland]

D1 is a RISC-V SoC containing an EMAC compatible with the A64 EMAC. In a
very roundabout way, this series finishes adding support for the D1 EMAC:
patch 4 resolves a compiler warning when building the driver for RISC-V.
The rest of the series is just cleanup requested by Jagan.

Changes in v2:
 - Add a structure for driver data, and put the syscon offset there

Samuel Holland (5):
  net: sun8i-emac: Add a structure for variant data
  net: sun8i-emac: Add a flag for RMII support
  net: sun8i-emac: Add a flag for the internal PHY switch
  net: sun8i-emac: Use common syscon setup for R40
  net: sun8i-emac: Remove the SoC variant ID

 drivers/net/sun8i_emac.c | 96 ++++++++++++++++++++++------------------
 1 file changed, 53 insertions(+), 43 deletions(-)

-- 
2.37.4

[PATCH v2 1/5] net: sun8i-emac: Add a structure for variant data

[Samuel Holland]

Currently, EMAC variants are distinguished by their identity, but this
gets unwieldy as more overlapping variants are added. Add a structure so
we can describe the individual feature differences between the variants.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v2:
 - New patch for v2

 drivers/net/sun8i_emac.c | 65 +++++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 20 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index e800a326b8..986e565cd8 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -127,7 +127,7 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-enum emac_variant {
+enum emac_variant_id {
 	A83T_EMAC = 1,
 	H3_EMAC,
 	A64_EMAC,
@@ -135,6 +135,10 @@ enum emac_variant {
 	H6_EMAC,
 };
 
+struct emac_variant {
+	enum emac_variant_id	variant;
+};
+
 struct emac_dma_desc {
 	u32 status;
 	u32 ctl_size;
@@ -160,7 +164,7 @@ struct emac_eth_dev {
 	u32 tx_slot;
 	bool use_internal_phy;
 
-	enum emac_variant variant;
+	const struct emac_variant *variant;
 	void *mac_reg;
 	phys_addr_t sysctl_reg;
 	struct phy_device *phydev;
@@ -317,7 +321,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
 {
 	u32 reg;
 
-	if (priv->variant == R40_GMAC) {
+	if (priv->variant->variant == R40_GMAC) {
 		/* Select RGMII for R40 */
 		reg = readl(priv->sysctl_reg + 0x164);
 		reg |= SC_ETCS_INT_GMII |
@@ -333,9 +337,9 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
 	reg = sun8i_emac_set_syscon_ephy(priv, reg);
 
 	reg &= ~(SC_ETCS_MASK | SC_EPIT);
-	if (priv->variant == H3_EMAC ||
-	    priv->variant == A64_EMAC ||
-	    priv->variant == H6_EMAC)
+	if (priv->variant->variant == H3_EMAC ||
+	    priv->variant->variant == A64_EMAC ||
+	    priv->variant->variant == H6_EMAC)
 		reg &= ~SC_RMII_EN;
 
 	switch (priv->interface) {
@@ -349,9 +353,9 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
 		reg |= SC_EPIT | SC_ETCS_INT_GMII;
 		break;
 	case PHY_INTERFACE_MODE_RMII:
-		if (priv->variant == H3_EMAC ||
-		    priv->variant == A64_EMAC ||
-		    priv->variant == H6_EMAC) {
+		if (priv->variant->variant == H3_EMAC ||
+		    priv->variant->variant == A64_EMAC ||
+		    priv->variant->variant == H6_EMAC) {
 			reg |= SC_RMII_EN | SC_ETCS_EXT_GMII;
 		break;
 		}
@@ -806,7 +810,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
 		return -EINVAL;
 	}
 
-	priv->variant = dev_get_driver_data(dev);
+	priv->variant = (const void *)dev_get_driver_data(dev);
 
 	if (!priv->variant) {
 		printf("%s: Missing variant\n", __func__);
@@ -860,7 +864,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
 	if (pdata->phy_interface == PHY_INTERFACE_MODE_NA)
 		return -EINVAL;
 
-	if (priv->variant == H3_EMAC) {
+	if (priv->variant->variant == H3_EMAC) {
 		ret = sun8i_handle_internal_phy(dev, priv);
 		if (ret)
 			return ret;
@@ -900,16 +904,37 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
 	return 0;
 }
 
+static const struct emac_variant emac_variant_a83t = {
+	.variant		= A83T_EMAC,
+};
+
+static const struct emac_variant emac_variant_h3 = {
+	.variant		= H3_EMAC,
+};
+
+static const struct emac_variant emac_variant_r40 = {
+	.variant		= R40_GMAC,
+};
+
+static const struct emac_variant emac_variant_a64 = {
+	.variant		= A64_EMAC,
+};
+
+static const struct emac_variant emac_variant_h6 = {
+	.variant		= H6_EMAC,
+};
+
 static const struct udevice_id sun8i_emac_eth_ids[] = {
-	{.compatible = "allwinner,sun8i-h3-emac", .data = (uintptr_t)H3_EMAC },
-	{.compatible = "allwinner,sun50i-a64-emac",
-		.data = (uintptr_t)A64_EMAC },
-	{.compatible = "allwinner,sun8i-a83t-emac",
-		.data = (uintptr_t)A83T_EMAC },
-	{.compatible = "allwinner,sun8i-r40-gmac",
-		.data = (uintptr_t)R40_GMAC },
-	{.compatible = "allwinner,sun50i-h6-emac",
-		.data = (uintptr_t)H6_EMAC },
+	{ .compatible = "allwinner,sun8i-a83t-emac",
+	  .data = (ulong)&emac_variant_a83t },
+	{ .compatible = "allwinner,sun8i-h3-emac",
+	  .data = (ulong)&emac_variant_h3 },
+	{ .compatible = "allwinner,sun8i-r40-gmac",
+	  .data = (ulong)&emac_variant_r40 },
+	{ .compatible = "allwinner,sun50i-a64-emac",
+	  .data = (ulong)&emac_variant_a64 },
+	{ .compatible = "allwinner,sun50i-h6-emac",
+	  .data = (ulong)&emac_variant_h6 },
 	{ }
 };
 
-- 
2.37.4

[PATCH v2 2/5] net: sun8i-emac: Add a flag for RMII support

[Samuel Holland]

Describe this feature instead of using the SoC ID.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v2:
 - New patch for v2

 drivers/net/sun8i_emac.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 986e565cd8..f232b8f087 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -137,6 +137,7 @@ enum emac_variant_id {
 
 struct emac_variant {
 	enum emac_variant_id	variant;
+	bool			support_rmii;
 };
 
 struct emac_dma_desc {
@@ -337,9 +338,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
 	reg = sun8i_emac_set_syscon_ephy(priv, reg);
 
 	reg &= ~(SC_ETCS_MASK | SC_EPIT);
-	if (priv->variant->variant == H3_EMAC ||
-	    priv->variant->variant == A64_EMAC ||
-	    priv->variant->variant == H6_EMAC)
+	if (priv->variant->support_rmii)
 		reg &= ~SC_RMII_EN;
 
 	switch (priv->interface) {
@@ -353,13 +352,10 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
 		reg |= SC_EPIT | SC_ETCS_INT_GMII;
 		break;
 	case PHY_INTERFACE_MODE_RMII:
-		if (priv->variant->variant == H3_EMAC ||
-		    priv->variant->variant == A64_EMAC ||
-		    priv->variant->variant == H6_EMAC) {
+		if (priv->variant->support_rmii) {
 			reg |= SC_RMII_EN | SC_ETCS_EXT_GMII;
-		break;
+			break;
 		}
-		/* RMII not supported on A83T */
 	default:
 		debug("%s: Invalid PHY interface\n", __func__);
 		return -EINVAL;
@@ -910,6 +906,7 @@ static const struct emac_variant emac_variant_a83t = {
 
 static const struct emac_variant emac_variant_h3 = {
 	.variant		= H3_EMAC,
+	.support_rmii		= true,
 };
 
 static const struct emac_variant emac_variant_r40 = {
@@ -918,10 +915,12 @@ static const struct emac_variant emac_variant_r40 = {
 
 static const struct emac_variant emac_variant_a64 = {
 	.variant		= A64_EMAC,
+	.support_rmii		= true,
 };
 
 static const struct emac_variant emac_variant_h6 = {
 	.variant		= H6_EMAC,
+	.support_rmii		= true,
 };
 
 static const struct udevice_id sun8i_emac_eth_ids[] = {
-- 
2.37.4

[PATCH v2 3/5] net: sun8i-emac: Add a flag for the internal PHY switch

[Samuel Holland]

Describe this feature instead of using the SoC ID.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v2:
 - New patch for v2

 drivers/net/sun8i_emac.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index f232b8f087..36cc2498b5 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -137,6 +137,7 @@ enum emac_variant_id {
 
 struct emac_variant {
 	enum emac_variant_id	variant;
+	bool			soc_has_internal_phy;
 	bool			support_rmii;
 };
 
@@ -860,7 +861,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
 	if (pdata->phy_interface == PHY_INTERFACE_MODE_NA)
 		return -EINVAL;
 
-	if (priv->variant->variant == H3_EMAC) {
+	if (priv->variant->soc_has_internal_phy) {
 		ret = sun8i_handle_internal_phy(dev, priv);
 		if (ret)
 			return ret;
@@ -906,6 +907,7 @@ static const struct emac_variant emac_variant_a83t = {
 
 static const struct emac_variant emac_variant_h3 = {
 	.variant		= H3_EMAC,
+	.soc_has_internal_phy	= true,
 	.support_rmii		= true,
 };
 
-- 
2.37.4

[PATCH v2 4/5] net: sun8i-emac: Use common syscon setup for R40

[Samuel Holland]

While R40 puts the EMAC syscon register at a different address from
other variants, the relevant portion of the register's layout is the
same. Factor out the register offset so the same code can be shared
by all variants. This matches what the Linux driver does.

This change provides two benefits beyond the simplification:
 - R40 boards now respect the RX delays from the devicetree
 - This resolves a warning on architectures where readl/writel
   expect the address to have a pointer type, not phys_addr_t.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v2:
 - Add a structure for driver data, and put the syscon offset there

 drivers/net/sun8i_emac.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 36cc2498b5..231aac19e3 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -137,6 +137,7 @@ enum emac_variant_id {
 
 struct emac_variant {
 	enum emac_variant_id	variant;
+	uint			syscon_offset;
 	bool			soc_has_internal_phy;
 	bool			support_rmii;
 };
@@ -168,7 +169,7 @@ struct emac_eth_dev {
 
 	const struct emac_variant *variant;
 	void *mac_reg;
-	phys_addr_t sysctl_reg;
+	void *sysctl_reg;
 	struct phy_device *phydev;
 	struct mii_dev *bus;
 	struct clk tx_clk;
@@ -323,18 +324,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
 {
 	u32 reg;
 
-	if (priv->variant->variant == R40_GMAC) {
-		/* Select RGMII for R40 */
-		reg = readl(priv->sysctl_reg + 0x164);
-		reg |= SC_ETCS_INT_GMII |
-		       SC_EPIT |
-		       (CONFIG_GMAC_TX_DELAY << SC_ETXDC_OFFSET);
-
-		writel(reg, priv->sysctl_reg + 0x164);
-		return 0;
-	}
-
-	reg = readl(priv->sysctl_reg + 0x30);
+	reg = readl(priv->sysctl_reg);
 
 	reg = sun8i_emac_set_syscon_ephy(priv, reg);
 
@@ -370,7 +360,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
 		reg |= ((pdata->rx_delay_ps / 100) << SC_ERXDC_OFFSET)
 			 & SC_ERXDC_MASK;
 
-	writel(reg, priv->sysctl_reg + 0x30);
+	writel(reg, priv->sysctl_reg);
 
 	return 0;
 }
@@ -793,6 +783,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
 	struct sun8i_eth_pdata *sun8i_pdata = dev_get_plat(dev);
 	struct eth_pdata *pdata = &sun8i_pdata->eth_pdata;
 	struct emac_eth_dev *priv = dev_get_priv(dev);
+	phys_addr_t syscon_base;
 	const fdt32_t *reg;
 	int node = dev_of_offset(dev);
 	int offset = 0;
@@ -838,13 +829,15 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
 		      __func__);
 		return -EINVAL;
 	}
-	priv->sysctl_reg = fdt_translate_address((void *)gd->fdt_blob,
-						 offset, reg);
-	if (priv->sysctl_reg == FDT_ADDR_T_NONE) {
+
+	syscon_base = fdt_translate_address((void *)gd->fdt_blob, offset, reg);
+	if (syscon_base == FDT_ADDR_T_NONE) {
 		debug("%s: Cannot find syscon base address\n", __func__);
 		return -EINVAL;
 	}
 
+	priv->sysctl_reg = (void *)syscon_base + priv->variant->syscon_offset;
+
 	pdata->phy_interface = -1;
 	priv->phyaddr = -1;
 	priv->use_internal_phy = false;
@@ -903,25 +896,30 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
 
 static const struct emac_variant emac_variant_a83t = {
 	.variant		= A83T_EMAC,
+	.syscon_offset		= 0x30,
 };
 
 static const struct emac_variant emac_variant_h3 = {
 	.variant		= H3_EMAC,
+	.syscon_offset		= 0x30,
 	.soc_has_internal_phy	= true,
 	.support_rmii		= true,
 };
 
 static const struct emac_variant emac_variant_r40 = {
 	.variant		= R40_GMAC,
+	.syscon_offset		= 0x164,
 };
 
 static const struct emac_variant emac_variant_a64 = {
 	.variant		= A64_EMAC,
+	.syscon_offset		= 0x30,
 	.support_rmii		= true,
 };
 
 static const struct emac_variant emac_variant_h6 = {
 	.variant		= H6_EMAC,
+	.syscon_offset		= 0x30,
 	.support_rmii		= true,
 };
 
-- 
2.37.4

[PATCH v2 5/5] net: sun8i-emac: Remove the SoC variant ID

[Samuel Holland]

Now that all differences in functionality are covered by individual
flags, remove the enumeration of SoC variants.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v2:
 - New patch for v2

 drivers/net/sun8i_emac.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
index 231aac19e3..04c3274fbe 100644
--- a/drivers/net/sun8i_emac.c
+++ b/drivers/net/sun8i_emac.c
@@ -127,16 +127,7 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-enum emac_variant_id {
-	A83T_EMAC = 1,
-	H3_EMAC,
-	A64_EMAC,
-	R40_GMAC,
-	H6_EMAC,
-};
-
 struct emac_variant {
-	enum emac_variant_id	variant;
 	uint			syscon_offset;
 	bool			soc_has_internal_phy;
 	bool			support_rmii;
@@ -895,30 +886,25 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
 }
 
 static const struct emac_variant emac_variant_a83t = {
-	.variant		= A83T_EMAC,
 	.syscon_offset		= 0x30,
 };
 
 static const struct emac_variant emac_variant_h3 = {
-	.variant		= H3_EMAC,
 	.syscon_offset		= 0x30,
 	.soc_has_internal_phy	= true,
 	.support_rmii		= true,
 };
 
 static const struct emac_variant emac_variant_r40 = {
-	.variant		= R40_GMAC,
 	.syscon_offset		= 0x164,
 };
 
 static const struct emac_variant emac_variant_a64 = {
-	.variant		= A64_EMAC,
 	.syscon_offset		= 0x30,
 	.support_rmii		= true,
 };
 
 static const struct emac_variant emac_variant_h6 = {
-	.variant		= H6_EMAC,
 	.syscon_offset		= 0x30,
 	.support_rmii		= true,
 };
-- 
2.37.4

Re: [PATCH v2 0/5] net: sun8i-emac: Allwinner D1 Support

[Andre Przywara]

On Sun, 22 Jan 2023 16:51:01 -0600
Samuel Holland <samuel@sholland.org> wrote:

> D1 is a RISC-V SoC containing an EMAC compatible with the A64 EMAC. In a
> very roundabout way, this series finishes adding support for the D1 EMAC:
> patch 4 resolves a compiler warning when building the driver for RISC-V.
> The rest of the series is just cleanup requested by Jagan.

I like this one, nice cleanup, and nice patch structuring.
I will do a more thorough review later (ping me shall I forget about
this), but I aim to push this rather sooner than later.

Cheers,
Andre

> Changes in v2:
>  - Add a structure for driver data, and put the syscon offset there
> 
> Samuel Holland (5):
>   net: sun8i-emac: Add a structure for variant data
>   net: sun8i-emac: Add a flag for RMII support
>   net: sun8i-emac: Add a flag for the internal PHY switch
>   net: sun8i-emac: Use common syscon setup for R40
>   net: sun8i-emac: Remove the SoC variant ID
> 
>  drivers/net/sun8i_emac.c | 96 ++++++++++++++++++++++------------------
>  1 file changed, 53 insertions(+), 43 deletions(-)
> 

Re: [PATCH v2 1/5] net: sun8i-emac: Add a structure for variant data

[Andre Przywara]

On 22/01/2023 22:51, Samuel Holland wrote:
> Currently, EMAC variants are distinguished by their identity, but this
> gets unwieldy as more overlapping variants are added. Add a structure so
> we can describe the individual feature differences between the variants.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Looks alright, given that this is temporary/preparatory.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
> 
> Changes in v2:
>   - New patch for v2
> 
>   drivers/net/sun8i_emac.c | 65 +++++++++++++++++++++++++++-------------
>   1 file changed, 45 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> index e800a326b8..986e565cd8 100644
> --- a/drivers/net/sun8i_emac.c
> +++ b/drivers/net/sun8i_emac.c
> @@ -127,7 +127,7 @@
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> -enum emac_variant {
> +enum emac_variant_id {
>   	A83T_EMAC = 1,
>   	H3_EMAC,
>   	A64_EMAC,
> @@ -135,6 +135,10 @@ enum emac_variant {
>   	H6_EMAC,
>   };
>   
> +struct emac_variant {
> +	enum emac_variant_id	variant;
> +};
> +
>   struct emac_dma_desc {
>   	u32 status;
>   	u32 ctl_size;
> @@ -160,7 +164,7 @@ struct emac_eth_dev {
>   	u32 tx_slot;
>   	bool use_internal_phy;
>   
> -	enum emac_variant variant;
> +	const struct emac_variant *variant;
>   	void *mac_reg;
>   	phys_addr_t sysctl_reg;
>   	struct phy_device *phydev;
> @@ -317,7 +321,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
>   {
>   	u32 reg;
>   
> -	if (priv->variant == R40_GMAC) {
> +	if (priv->variant->variant == R40_GMAC) {
>   		/* Select RGMII for R40 */
>   		reg = readl(priv->sysctl_reg + 0x164);
>   		reg |= SC_ETCS_INT_GMII |
> @@ -333,9 +337,9 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
>   	reg = sun8i_emac_set_syscon_ephy(priv, reg);
>   
>   	reg &= ~(SC_ETCS_MASK | SC_EPIT);
> -	if (priv->variant == H3_EMAC ||
> -	    priv->variant == A64_EMAC ||
> -	    priv->variant == H6_EMAC)
> +	if (priv->variant->variant == H3_EMAC ||
> +	    priv->variant->variant == A64_EMAC ||
> +	    priv->variant->variant == H6_EMAC)
>   		reg &= ~SC_RMII_EN;
>   
>   	switch (priv->interface) {
> @@ -349,9 +353,9 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
>   		reg |= SC_EPIT | SC_ETCS_INT_GMII;
>   		break;
>   	case PHY_INTERFACE_MODE_RMII:
> -		if (priv->variant == H3_EMAC ||
> -		    priv->variant == A64_EMAC ||
> -		    priv->variant == H6_EMAC) {
> +		if (priv->variant->variant == H3_EMAC ||
> +		    priv->variant->variant == A64_EMAC ||
> +		    priv->variant->variant == H6_EMAC) {
>   			reg |= SC_RMII_EN | SC_ETCS_EXT_GMII;
>   		break;
>   		}
> @@ -806,7 +810,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
>   		return -EINVAL;
>   	}
>   
> -	priv->variant = dev_get_driver_data(dev);
> +	priv->variant = (const void *)dev_get_driver_data(dev);
>   
>   	if (!priv->variant) {
>   		printf("%s: Missing variant\n", __func__);
> @@ -860,7 +864,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
>   	if (pdata->phy_interface == PHY_INTERFACE_MODE_NA)
>   		return -EINVAL;
>   
> -	if (priv->variant == H3_EMAC) {
> +	if (priv->variant->variant == H3_EMAC) {
>   		ret = sun8i_handle_internal_phy(dev, priv);
>   		if (ret)
>   			return ret;
> @@ -900,16 +904,37 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
>   	return 0;
>   }
>   
> +static const struct emac_variant emac_variant_a83t = {
> +	.variant		= A83T_EMAC,
> +};
> +
> +static const struct emac_variant emac_variant_h3 = {
> +	.variant		= H3_EMAC,
> +};
> +
> +static const struct emac_variant emac_variant_r40 = {
> +	.variant		= R40_GMAC,
> +};
> +
> +static const struct emac_variant emac_variant_a64 = {
> +	.variant		= A64_EMAC,
> +};
> +
> +static const struct emac_variant emac_variant_h6 = {
> +	.variant		= H6_EMAC,
> +};
> +
>   static const struct udevice_id sun8i_emac_eth_ids[] = {
> -	{.compatible = "allwinner,sun8i-h3-emac", .data = (uintptr_t)H3_EMAC },
> -	{.compatible = "allwinner,sun50i-a64-emac",
> -		.data = (uintptr_t)A64_EMAC },
> -	{.compatible = "allwinner,sun8i-a83t-emac",
> -		.data = (uintptr_t)A83T_EMAC },
> -	{.compatible = "allwinner,sun8i-r40-gmac",
> -		.data = (uintptr_t)R40_GMAC },
> -	{.compatible = "allwinner,sun50i-h6-emac",
> -		.data = (uintptr_t)H6_EMAC },
> +	{ .compatible = "allwinner,sun8i-a83t-emac",
> +	  .data = (ulong)&emac_variant_a83t },
> +	{ .compatible = "allwinner,sun8i-h3-emac",
> +	  .data = (ulong)&emac_variant_h3 },
> +	{ .compatible = "allwinner,sun8i-r40-gmac",
> +	  .data = (ulong)&emac_variant_r40 },
> +	{ .compatible = "allwinner,sun50i-a64-emac",
> +	  .data = (ulong)&emac_variant_a64 },
> +	{ .compatible = "allwinner,sun50i-h6-emac",
> +	  .data = (ulong)&emac_variant_h6 },
>   	{ }
>   };
>   

Re: [PATCH v2 2/5] net: sun8i-emac: Add a flag for RMII support

[Andre Przywara]

On 22/01/2023 22:51, Samuel Holland wrote:
> Describe this feature instead of using the SoC ID.

Looks alright, also compared against the kernel driver.

> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
> 
> Changes in v2:
>   - New patch for v2
> 
>   drivers/net/sun8i_emac.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> index 986e565cd8..f232b8f087 100644
> --- a/drivers/net/sun8i_emac.c
> +++ b/drivers/net/sun8i_emac.c
> @@ -137,6 +137,7 @@ enum emac_variant_id {
>   
>   struct emac_variant {
>   	enum emac_variant_id	variant;
> +	bool			support_rmii;
>   };
>   
>   struct emac_dma_desc {
> @@ -337,9 +338,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
>   	reg = sun8i_emac_set_syscon_ephy(priv, reg);
>   
>   	reg &= ~(SC_ETCS_MASK | SC_EPIT);
> -	if (priv->variant->variant == H3_EMAC ||
> -	    priv->variant->variant == A64_EMAC ||
> -	    priv->variant->variant == H6_EMAC)
> +	if (priv->variant->support_rmii)
>   		reg &= ~SC_RMII_EN;
>   
>   	switch (priv->interface) {
> @@ -353,13 +352,10 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
>   		reg |= SC_EPIT | SC_ETCS_INT_GMII;
>   		break;
>   	case PHY_INTERFACE_MODE_RMII:
> -		if (priv->variant->variant == H3_EMAC ||
> -		    priv->variant->variant == A64_EMAC ||
> -		    priv->variant->variant == H6_EMAC) {
> +		if (priv->variant->support_rmii) {
>   			reg |= SC_RMII_EN | SC_ETCS_EXT_GMII;
> -		break;
> +			break;
>   		}
> -		/* RMII not supported on A83T */
>   	default:
>   		debug("%s: Invalid PHY interface\n", __func__);
>   		return -EINVAL;
> @@ -910,6 +906,7 @@ static const struct emac_variant emac_variant_a83t = {
>   
>   static const struct emac_variant emac_variant_h3 = {
>   	.variant		= H3_EMAC,
> +	.support_rmii		= true,
>   };
>   
>   static const struct emac_variant emac_variant_r40 = {
> @@ -918,10 +915,12 @@ static const struct emac_variant emac_variant_r40 = {
>   
>   static const struct emac_variant emac_variant_a64 = {
>   	.variant		= A64_EMAC,
> +	.support_rmii		= true,
>   };
>   
>   static const struct emac_variant emac_variant_h6 = {
>   	.variant		= H6_EMAC,
> +	.support_rmii		= true,
>   };
>   
>   static const struct udevice_id sun8i_emac_eth_ids[] = {

Re: [PATCH v2 3/5] net: sun8i-emac: Add a flag for the internal PHY switch

[Andre Przywara]

On 22/01/2023 22:51, Samuel Holland wrote:
> Describe this feature instead of using the SoC ID.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>
> Changes in v2:
>   - New patch for v2
>
>   drivers/net/sun8i_emac.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> index f232b8f087..36cc2498b5 100644
> --- a/drivers/net/sun8i_emac.c
> +++ b/drivers/net/sun8i_emac.c
> @@ -137,6 +137,7 @@ enum emac_variant_id {
>
>   struct emac_variant {
>       enum emac_variant_id    variant;
> +     bool                    soc_has_internal_phy;
>       bool                    support_rmii;
>   };
>
> @@ -860,7 +861,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
>       if (pdata->phy_interface == PHY_INTERFACE_MODE_NA)
>               return -EINVAL;
>
> -     if (priv->variant->variant == H3_EMAC) {
> +     if (priv->variant->soc_has_internal_phy) {
>               ret = sun8i_handle_internal_phy(dev, priv);
>               if (ret)
>                       return ret;
> @@ -906,6 +907,7 @@ static const struct emac_variant emac_variant_a83t = {
>
>   static const struct emac_variant emac_variant_h3 = {
>       .variant                = H3_EMAC,
> +     .soc_has_internal_phy   = true,
>       .support_rmii           = true,
>   };
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v2 4/5] net: sun8i-emac: Use common syscon setup for R40

[Andre Przywara]

On 22/01/2023 22:51, Samuel Holland wrote:
> While R40 puts the EMAC syscon register at a different address from
> other variants, the relevant portion of the register's layout is the
> same. Factor out the register offset so the same code can be shared
> by all variants. This matches what the Linux driver does.
> 
> This change provides two benefits beyond the simplification:
>   - R40 boards now respect the RX delays from the devicetree
>   - This resolves a warning on architectures where readl/writel
>     expect the address to have a pointer type, not phys_addr_t.
> 

Nice cleanup, and makes it obvious that the R40 isn't such a special 
snowflake after all.

> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre


> ---
> 
> Changes in v2:
>   - Add a structure for driver data, and put the syscon offset there
> 
>   drivers/net/sun8i_emac.c | 32 +++++++++++++++-----------------
>   1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> index 36cc2498b5..231aac19e3 100644
> --- a/drivers/net/sun8i_emac.c
> +++ b/drivers/net/sun8i_emac.c
> @@ -137,6 +137,7 @@ enum emac_variant_id {
>   
>   struct emac_variant {
>   	enum emac_variant_id	variant;
> +	uint			syscon_offset;
>   	bool			soc_has_internal_phy;
>   	bool			support_rmii;
>   };
> @@ -168,7 +169,7 @@ struct emac_eth_dev {
>   
>   	const struct emac_variant *variant;
>   	void *mac_reg;
> -	phys_addr_t sysctl_reg;
> +	void *sysctl_reg;
>   	struct phy_device *phydev;
>   	struct mii_dev *bus;
>   	struct clk tx_clk;
> @@ -323,18 +324,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
>   {
>   	u32 reg;
>   
> -	if (priv->variant->variant == R40_GMAC) {
> -		/* Select RGMII for R40 */
> -		reg = readl(priv->sysctl_reg + 0x164);
> -		reg |= SC_ETCS_INT_GMII |
> -		       SC_EPIT |
> -		       (CONFIG_GMAC_TX_DELAY << SC_ETXDC_OFFSET);
> -
> -		writel(reg, priv->sysctl_reg + 0x164);
> -		return 0;
> -	}
> -
> -	reg = readl(priv->sysctl_reg + 0x30);
> +	reg = readl(priv->sysctl_reg);
>   
>   	reg = sun8i_emac_set_syscon_ephy(priv, reg);
>   
> @@ -370,7 +360,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
>   		reg |= ((pdata->rx_delay_ps / 100) << SC_ERXDC_OFFSET)
>   			 & SC_ERXDC_MASK;
>   
> -	writel(reg, priv->sysctl_reg + 0x30);
> +	writel(reg, priv->sysctl_reg);
>   
>   	return 0;
>   }
> @@ -793,6 +783,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
>   	struct sun8i_eth_pdata *sun8i_pdata = dev_get_plat(dev);
>   	struct eth_pdata *pdata = &sun8i_pdata->eth_pdata;
>   	struct emac_eth_dev *priv = dev_get_priv(dev);
> +	phys_addr_t syscon_base;
>   	const fdt32_t *reg;
>   	int node = dev_of_offset(dev);
>   	int offset = 0;
> @@ -838,13 +829,15 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
>   		      __func__);
>   		return -EINVAL;
>   	}
> -	priv->sysctl_reg = fdt_translate_address((void *)gd->fdt_blob,
> -						 offset, reg);
> -	if (priv->sysctl_reg == FDT_ADDR_T_NONE) {
> +
> +	syscon_base = fdt_translate_address((void *)gd->fdt_blob, offset, reg);
> +	if (syscon_base == FDT_ADDR_T_NONE) {
>   		debug("%s: Cannot find syscon base address\n", __func__);
>   		return -EINVAL;
>   	}
>   
> +	priv->sysctl_reg = (void *)syscon_base + priv->variant->syscon_offset;
> +
>   	pdata->phy_interface = -1;
>   	priv->phyaddr = -1;
>   	priv->use_internal_phy = false;
> @@ -903,25 +896,30 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
>   
>   static const struct emac_variant emac_variant_a83t = {
>   	.variant		= A83T_EMAC,
> +	.syscon_offset		= 0x30,
>   };
>   
>   static const struct emac_variant emac_variant_h3 = {
>   	.variant		= H3_EMAC,
> +	.syscon_offset		= 0x30,
>   	.soc_has_internal_phy	= true,
>   	.support_rmii		= true,
>   };
>   
>   static const struct emac_variant emac_variant_r40 = {
>   	.variant		= R40_GMAC,
> +	.syscon_offset		= 0x164,
>   };
>   
>   static const struct emac_variant emac_variant_a64 = {
>   	.variant		= A64_EMAC,
> +	.syscon_offset		= 0x30,
>   	.support_rmii		= true,
>   };
>   
>   static const struct emac_variant emac_variant_h6 = {
>   	.variant		= H6_EMAC,
> +	.syscon_offset		= 0x30,
>   	.support_rmii		= true,
>   };
>   

Re: [PATCH v2 5/5] net: sun8i-emac: Remove the SoC variant ID

[Andre Przywara]

On 22/01/2023 22:51, Samuel Holland wrote:

Hi,

> Now that all differences in functionality are covered by individual
> flags, remove the enumeration of SoC variants.

My favourite patch of this series ;-)

> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre


> ---
> 
> Changes in v2:
>   - New patch for v2
> 
>   drivers/net/sun8i_emac.c | 14 --------------
>   1 file changed, 14 deletions(-)
> 
> diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> index 231aac19e3..04c3274fbe 100644
> --- a/drivers/net/sun8i_emac.c
> +++ b/drivers/net/sun8i_emac.c
> @@ -127,16 +127,7 @@
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> -enum emac_variant_id {
> -	A83T_EMAC = 1,
> -	H3_EMAC,
> -	A64_EMAC,
> -	R40_GMAC,
> -	H6_EMAC,
> -};
> -
>   struct emac_variant {
> -	enum emac_variant_id	variant;
>   	uint			syscon_offset;
>   	bool			soc_has_internal_phy;
>   	bool			support_rmii;
> @@ -895,30 +886,25 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
>   }
>   
>   static const struct emac_variant emac_variant_a83t = {
> -	.variant		= A83T_EMAC,
>   	.syscon_offset		= 0x30,
>   };
>   
>   static const struct emac_variant emac_variant_h3 = {
> -	.variant		= H3_EMAC,
>   	.syscon_offset		= 0x30,
>   	.soc_has_internal_phy	= true,
>   	.support_rmii		= true,
>   };
>   
>   static const struct emac_variant emac_variant_r40 = {
> -	.variant		= R40_GMAC,
>   	.syscon_offset		= 0x164,
>   };
>   
>   static const struct emac_variant emac_variant_a64 = {
> -	.variant		= A64_EMAC,
>   	.syscon_offset		= 0x30,
>   	.support_rmii		= true,
>   };
>   
>   static const struct emac_variant emac_variant_h6 = {
> -	.variant		= H6_EMAC,
>   	.syscon_offset		= 0x30,
>   	.support_rmii		= true,
>   };

Re: [PATCH v2 0/5] net: sun8i-emac: Allwinner D1 Support

[Andre Przywara]

On Sun, 22 Jan 2023 16:51:01 -0600
Samuel Holland <samuel@sholland.org> wrote:

Hi Joe, Ramon,

> D1 is a RISC-V SoC containing an EMAC compatible with the A64 EMAC. In a
> very roundabout way, this series finishes adding support for the D1 EMAC:
> patch 4 resolves a compiler warning when building the driver for RISC-V.
> The rest of the series is just cleanup requested by Jagan.

So I reviewed, but also tested this series on an H3, A64, H6 and R40
board, and Ethernet still worked on all of them.
How do we handle the merge? Do you want to ACK the patches, and I
take them through the sunxi tree? Or do you want to take the series?
Patchwork put this on my plate, but get_maintainer.pl listed you two as
the maintainers.

Cheers,
Andre


> Changes in v2:
>  - Add a structure for driver data, and put the syscon offset there
> 
> Samuel Holland (5):
>   net: sun8i-emac: Add a structure for variant data
>   net: sun8i-emac: Add a flag for RMII support
>   net: sun8i-emac: Add a flag for the internal PHY switch
>   net: sun8i-emac: Use common syscon setup for R40
>   net: sun8i-emac: Remove the SoC variant ID
> 
>  drivers/net/sun8i_emac.c | 96 ++++++++++++++++++++++------------------
>  1 file changed, 53 insertions(+), 43 deletions(-)
> 

Re: [PATCH v2 1/5] net: sun8i-emac: Add a structure for variant data

[Ramon Fried]

On Mon, Jan 23, 2023 at 7:21 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On 22/01/2023 22:51, Samuel Holland wrote:
> > Currently, EMAC variants are distinguished by their identity, but this
> > gets unwieldy as more overlapping variants are added. Add a structure so
> > we can describe the individual feature differences between the variants.
> >
> > Signed-off-by: Samuel Holland <samuel@sholland.org>
>
> Looks alright, given that this is temporary/preparatory.
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>
> Cheers,
> Andre
>
> > ---
> >
> > Changes in v2:
> >   - New patch for v2
> >
> >   drivers/net/sun8i_emac.c | 65 +++++++++++++++++++++++++++-------------
> >   1 file changed, 45 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> > index e800a326b8..986e565cd8 100644
> > --- a/drivers/net/sun8i_emac.c
> > +++ b/drivers/net/sun8i_emac.c
> > @@ -127,7 +127,7 @@
> >
> >   DECLARE_GLOBAL_DATA_PTR;
> >
> > -enum emac_variant {
> > +enum emac_variant_id {
> >       A83T_EMAC = 1,
> >       H3_EMAC,
> >       A64_EMAC,
> > @@ -135,6 +135,10 @@ enum emac_variant {
> >       H6_EMAC,
> >   };
> >
> > +struct emac_variant {
> > +     enum emac_variant_id    variant;
> > +};
> > +
> >   struct emac_dma_desc {
> >       u32 status;
> >       u32 ctl_size;
> > @@ -160,7 +164,7 @@ struct emac_eth_dev {
> >       u32 tx_slot;
> >       bool use_internal_phy;
> >
> > -     enum emac_variant variant;
> > +     const struct emac_variant *variant;
> >       void *mac_reg;
> >       phys_addr_t sysctl_reg;
> >       struct phy_device *phydev;
> > @@ -317,7 +321,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
> >   {
> >       u32 reg;
> >
> > -     if (priv->variant == R40_GMAC) {
> > +     if (priv->variant->variant == R40_GMAC) {
> >               /* Select RGMII for R40 */
> >               reg = readl(priv->sysctl_reg + 0x164);
> >               reg |= SC_ETCS_INT_GMII |
> > @@ -333,9 +337,9 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
> >       reg = sun8i_emac_set_syscon_ephy(priv, reg);
> >
> >       reg &= ~(SC_ETCS_MASK | SC_EPIT);
> > -     if (priv->variant == H3_EMAC ||
> > -         priv->variant == A64_EMAC ||
> > -         priv->variant == H6_EMAC)
> > +     if (priv->variant->variant == H3_EMAC ||
> > +         priv->variant->variant == A64_EMAC ||
> > +         priv->variant->variant == H6_EMAC)
> >               reg &= ~SC_RMII_EN;
> >
> >       switch (priv->interface) {
> > @@ -349,9 +353,9 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
> >               reg |= SC_EPIT | SC_ETCS_INT_GMII;
> >               break;
> >       case PHY_INTERFACE_MODE_RMII:
> > -             if (priv->variant == H3_EMAC ||
> > -                 priv->variant == A64_EMAC ||
> > -                 priv->variant == H6_EMAC) {
> > +             if (priv->variant->variant == H3_EMAC ||
> > +                 priv->variant->variant == A64_EMAC ||
> > +                 priv->variant->variant == H6_EMAC) {
> >                       reg |= SC_RMII_EN | SC_ETCS_EXT_GMII;
> >               break;
> >               }
> > @@ -806,7 +810,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
> >               return -EINVAL;
> >       }
> >
> > -     priv->variant = dev_get_driver_data(dev);
> > +     priv->variant = (const void *)dev_get_driver_data(dev);
> >
> >       if (!priv->variant) {
> >               printf("%s: Missing variant\n", __func__);
> > @@ -860,7 +864,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
> >       if (pdata->phy_interface == PHY_INTERFACE_MODE_NA)
> >               return -EINVAL;
> >
> > -     if (priv->variant == H3_EMAC) {
> > +     if (priv->variant->variant == H3_EMAC) {
> >               ret = sun8i_handle_internal_phy(dev, priv);
> >               if (ret)
> >                       return ret;
> > @@ -900,16 +904,37 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
> >       return 0;
> >   }
> >
> > +static const struct emac_variant emac_variant_a83t = {
> > +     .variant                = A83T_EMAC,
> > +};
> > +
> > +static const struct emac_variant emac_variant_h3 = {
> > +     .variant                = H3_EMAC,
> > +};
> > +
> > +static const struct emac_variant emac_variant_r40 = {
> > +     .variant                = R40_GMAC,
> > +};
> > +
> > +static const struct emac_variant emac_variant_a64 = {
> > +     .variant                = A64_EMAC,
> > +};
> > +
> > +static const struct emac_variant emac_variant_h6 = {
> > +     .variant                = H6_EMAC,
> > +};
> > +
> >   static const struct udevice_id sun8i_emac_eth_ids[] = {
> > -     {.compatible = "allwinner,sun8i-h3-emac", .data = (uintptr_t)H3_EMAC },
> > -     {.compatible = "allwinner,sun50i-a64-emac",
> > -             .data = (uintptr_t)A64_EMAC },
> > -     {.compatible = "allwinner,sun8i-a83t-emac",
> > -             .data = (uintptr_t)A83T_EMAC },
> > -     {.compatible = "allwinner,sun8i-r40-gmac",
> > -             .data = (uintptr_t)R40_GMAC },
> > -     {.compatible = "allwinner,sun50i-h6-emac",
> > -             .data = (uintptr_t)H6_EMAC },
> > +     { .compatible = "allwinner,sun8i-a83t-emac",
> > +       .data = (ulong)&emac_variant_a83t },
> > +     { .compatible = "allwinner,sun8i-h3-emac",
> > +       .data = (ulong)&emac_variant_h3 },
> > +     { .compatible = "allwinner,sun8i-r40-gmac",
> > +       .data = (ulong)&emac_variant_r40 },
> > +     { .compatible = "allwinner,sun50i-a64-emac",
> > +       .data = (ulong)&emac_variant_a64 },
> > +     { .compatible = "allwinner,sun50i-h6-emac",
> > +       .data = (ulong)&emac_variant_h6 },
> >       { }
> >   };
> >
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH v2 2/5] net: sun8i-emac: Add a flag for RMII support

[Ramon Fried]

On Mon, Jan 23, 2023 at 7:23 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On 22/01/2023 22:51, Samuel Holland wrote:
> > Describe this feature instead of using the SoC ID.
>
> Looks alright, also compared against the kernel driver.
>
> > Signed-off-by: Samuel Holland <samuel@sholland.org>
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>
> Cheers,
> Andre
>
> > ---
> >
> > Changes in v2:
> >   - New patch for v2
> >
> >   drivers/net/sun8i_emac.c | 15 +++++++--------
> >   1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> > index 986e565cd8..f232b8f087 100644
> > --- a/drivers/net/sun8i_emac.c
> > +++ b/drivers/net/sun8i_emac.c
> > @@ -137,6 +137,7 @@ enum emac_variant_id {
> >
> >   struct emac_variant {
> >       enum emac_variant_id    variant;
> > +     bool                    support_rmii;
> >   };
> >
> >   struct emac_dma_desc {
> > @@ -337,9 +338,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
> >       reg = sun8i_emac_set_syscon_ephy(priv, reg);
> >
> >       reg &= ~(SC_ETCS_MASK | SC_EPIT);
> > -     if (priv->variant->variant == H3_EMAC ||
> > -         priv->variant->variant == A64_EMAC ||
> > -         priv->variant->variant == H6_EMAC)
> > +     if (priv->variant->support_rmii)
> >               reg &= ~SC_RMII_EN;
> >
> >       switch (priv->interface) {
> > @@ -353,13 +352,10 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
> >               reg |= SC_EPIT | SC_ETCS_INT_GMII;
> >               break;
> >       case PHY_INTERFACE_MODE_RMII:
> > -             if (priv->variant->variant == H3_EMAC ||
> > -                 priv->variant->variant == A64_EMAC ||
> > -                 priv->variant->variant == H6_EMAC) {
> > +             if (priv->variant->support_rmii) {
> >                       reg |= SC_RMII_EN | SC_ETCS_EXT_GMII;
> > -             break;
> > +                     break;
> >               }
> > -             /* RMII not supported on A83T */
> >       default:
> >               debug("%s: Invalid PHY interface\n", __func__);
> >               return -EINVAL;
> > @@ -910,6 +906,7 @@ static const struct emac_variant emac_variant_a83t = {
> >
> >   static const struct emac_variant emac_variant_h3 = {
> >       .variant                = H3_EMAC,
> > +     .support_rmii           = true,
> >   };
> >
> >   static const struct emac_variant emac_variant_r40 = {
> > @@ -918,10 +915,12 @@ static const struct emac_variant emac_variant_r40 = {
> >
> >   static const struct emac_variant emac_variant_a64 = {
> >       .variant                = A64_EMAC,
> > +     .support_rmii           = true,
> >   };
> >
> >   static const struct emac_variant emac_variant_h6 = {
> >       .variant                = H6_EMAC,
> > +     .support_rmii           = true,
> >   };
> >
> >   static const struct udevice_id sun8i_emac_eth_ids[] = {
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH v2 3/5] net: sun8i-emac: Add a flag for the internal PHY switch

[Ramon Fried]

On Mon, Jan 23, 2023 at 7:24 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On 22/01/2023 22:51, Samuel Holland wrote:
> > Describe this feature instead of using the SoC ID.
> >
> > Signed-off-by: Samuel Holland <samuel@sholland.org>
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>
> Cheers,
> Andre
>
> > ---
> >
> > Changes in v2:
> >   - New patch for v2
> >
> >   drivers/net/sun8i_emac.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> > index f232b8f087..36cc2498b5 100644
> > --- a/drivers/net/sun8i_emac.c
> > +++ b/drivers/net/sun8i_emac.c
> > @@ -137,6 +137,7 @@ enum emac_variant_id {
> >
> >   struct emac_variant {
> >       enum emac_variant_id    variant;
> > +     bool                    soc_has_internal_phy;
> >       bool                    support_rmii;
> >   };
> >
> > @@ -860,7 +861,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
> >       if (pdata->phy_interface == PHY_INTERFACE_MODE_NA)
> >               return -EINVAL;
> >
> > -     if (priv->variant->variant == H3_EMAC) {
> > +     if (priv->variant->soc_has_internal_phy) {
> >               ret = sun8i_handle_internal_phy(dev, priv);
> >               if (ret)
> >                       return ret;
> > @@ -906,6 +907,7 @@ static const struct emac_variant emac_variant_a83t = {
> >
> >   static const struct emac_variant emac_variant_h3 = {
> >       .variant                = H3_EMAC,
> > +     .soc_has_internal_phy   = true,
> >       .support_rmii           = true,
> >   };
> >
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH v2 4/5] net: sun8i-emac: Use common syscon setup for R40

[Ramon Fried]

On Mon, Jan 23, 2023 at 7:24 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On 22/01/2023 22:51, Samuel Holland wrote:
> > While R40 puts the EMAC syscon register at a different address from
> > other variants, the relevant portion of the register's layout is the
> > same. Factor out the register offset so the same code can be shared
> > by all variants. This matches what the Linux driver does.
> >
> > This change provides two benefits beyond the simplification:
> >   - R40 boards now respect the RX delays from the devicetree
> >   - This resolves a warning on architectures where readl/writel
> >     expect the address to have a pointer type, not phys_addr_t.
> >
>
> Nice cleanup, and makes it obvious that the R40 isn't such a special
> snowflake after all.
>
> > Signed-off-by: Samuel Holland <samuel@sholland.org>
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>
> Cheers,
> Andre
>
>
> > ---
> >
> > Changes in v2:
> >   - Add a structure for driver data, and put the syscon offset there
> >
> >   drivers/net/sun8i_emac.c | 32 +++++++++++++++-----------------
> >   1 file changed, 15 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> > index 36cc2498b5..231aac19e3 100644
> > --- a/drivers/net/sun8i_emac.c
> > +++ b/drivers/net/sun8i_emac.c
> > @@ -137,6 +137,7 @@ enum emac_variant_id {
> >
> >   struct emac_variant {
> >       enum emac_variant_id    variant;
> > +     uint                    syscon_offset;
> >       bool                    soc_has_internal_phy;
> >       bool                    support_rmii;
> >   };
> > @@ -168,7 +169,7 @@ struct emac_eth_dev {
> >
> >       const struct emac_variant *variant;
> >       void *mac_reg;
> > -     phys_addr_t sysctl_reg;
> > +     void *sysctl_reg;
> >       struct phy_device *phydev;
> >       struct mii_dev *bus;
> >       struct clk tx_clk;
> > @@ -323,18 +324,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
> >   {
> >       u32 reg;
> >
> > -     if (priv->variant->variant == R40_GMAC) {
> > -             /* Select RGMII for R40 */
> > -             reg = readl(priv->sysctl_reg + 0x164);
> > -             reg |= SC_ETCS_INT_GMII |
> > -                    SC_EPIT |
> > -                    (CONFIG_GMAC_TX_DELAY << SC_ETXDC_OFFSET);
> > -
> > -             writel(reg, priv->sysctl_reg + 0x164);
> > -             return 0;
> > -     }
> > -
> > -     reg = readl(priv->sysctl_reg + 0x30);
> > +     reg = readl(priv->sysctl_reg);
> >
> >       reg = sun8i_emac_set_syscon_ephy(priv, reg);
> >
> > @@ -370,7 +360,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
> >               reg |= ((pdata->rx_delay_ps / 100) << SC_ERXDC_OFFSET)
> >                        & SC_ERXDC_MASK;
> >
> > -     writel(reg, priv->sysctl_reg + 0x30);
> > +     writel(reg, priv->sysctl_reg);
> >
> >       return 0;
> >   }
> > @@ -793,6 +783,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
> >       struct sun8i_eth_pdata *sun8i_pdata = dev_get_plat(dev);
> >       struct eth_pdata *pdata = &sun8i_pdata->eth_pdata;
> >       struct emac_eth_dev *priv = dev_get_priv(dev);
> > +     phys_addr_t syscon_base;
> >       const fdt32_t *reg;
> >       int node = dev_of_offset(dev);
> >       int offset = 0;
> > @@ -838,13 +829,15 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
> >                     __func__);
> >               return -EINVAL;
> >       }
> > -     priv->sysctl_reg = fdt_translate_address((void *)gd->fdt_blob,
> > -                                              offset, reg);
> > -     if (priv->sysctl_reg == FDT_ADDR_T_NONE) {
> > +
> > +     syscon_base = fdt_translate_address((void *)gd->fdt_blob, offset, reg);
> > +     if (syscon_base == FDT_ADDR_T_NONE) {
> >               debug("%s: Cannot find syscon base address\n", __func__);
> >               return -EINVAL;
> >       }
> >
> > +     priv->sysctl_reg = (void *)syscon_base + priv->variant->syscon_offset;
> > +
> >       pdata->phy_interface = -1;
> >       priv->phyaddr = -1;
> >       priv->use_internal_phy = false;
> > @@ -903,25 +896,30 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
> >
> >   static const struct emac_variant emac_variant_a83t = {
> >       .variant                = A83T_EMAC,
> > +     .syscon_offset          = 0x30,
> >   };
> >
> >   static const struct emac_variant emac_variant_h3 = {
> >       .variant                = H3_EMAC,
> > +     .syscon_offset          = 0x30,
> >       .soc_has_internal_phy   = true,
> >       .support_rmii           = true,
> >   };
> >
> >   static const struct emac_variant emac_variant_r40 = {
> >       .variant                = R40_GMAC,
> > +     .syscon_offset          = 0x164,
> >   };
> >
> >   static const struct emac_variant emac_variant_a64 = {
> >       .variant                = A64_EMAC,
> > +     .syscon_offset          = 0x30,
> >       .support_rmii           = true,
> >   };
> >
> >   static const struct emac_variant emac_variant_h6 = {
> >       .variant                = H6_EMAC,
> > +     .syscon_offset          = 0x30,
> >       .support_rmii           = true,
> >   };
> >
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH v2 5/5] net: sun8i-emac: Remove the SoC variant ID

[Ramon Fried]

On Mon, Jan 23, 2023 at 7:24 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On 22/01/2023 22:51, Samuel Holland wrote:
>
> Hi,
>
> > Now that all differences in functionality are covered by individual
> > flags, remove the enumeration of SoC variants.
>
> My favourite patch of this series ;-)
>
> > Signed-off-by: Samuel Holland <samuel@sholland.org>
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>
> Cheers,
> Andre
>
>
> > ---
> >
> > Changes in v2:
> >   - New patch for v2
> >
> >   drivers/net/sun8i_emac.c | 14 --------------
> >   1 file changed, 14 deletions(-)
> >
> > diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> > index 231aac19e3..04c3274fbe 100644
> > --- a/drivers/net/sun8i_emac.c
> > +++ b/drivers/net/sun8i_emac.c
> > @@ -127,16 +127,7 @@
> >
> >   DECLARE_GLOBAL_DATA_PTR;
> >
> > -enum emac_variant_id {
> > -     A83T_EMAC = 1,
> > -     H3_EMAC,
> > -     A64_EMAC,
> > -     R40_GMAC,
> > -     H6_EMAC,
> > -};
> > -
> >   struct emac_variant {
> > -     enum emac_variant_id    variant;
> >       uint                    syscon_offset;
> >       bool                    soc_has_internal_phy;
> >       bool                    support_rmii;
> > @@ -895,30 +886,25 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
> >   }
> >
> >   static const struct emac_variant emac_variant_a83t = {
> > -     .variant                = A83T_EMAC,
> >       .syscon_offset          = 0x30,
> >   };
> >
> >   static const struct emac_variant emac_variant_h3 = {
> > -     .variant                = H3_EMAC,
> >       .syscon_offset          = 0x30,
> >       .soc_has_internal_phy   = true,
> >       .support_rmii           = true,
> >   };
> >
> >   static const struct emac_variant emac_variant_r40 = {
> > -     .variant                = R40_GMAC,
> >       .syscon_offset          = 0x164,
> >   };
> >
> >   static const struct emac_variant emac_variant_a64 = {
> > -     .variant                = A64_EMAC,
> >       .syscon_offset          = 0x30,
> >       .support_rmii           = true,
> >   };
> >
> >   static const struct emac_variant emac_variant_h6 = {
> > -     .variant                = H6_EMAC,
> >       .syscon_offset          = 0x30,
> >       .support_rmii           = true,
> >   };
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH v2 0/5] net: sun8i-emac: Allwinner D1 Support

[Ramon Fried]

On Wed, Feb 1, 2023 at 2:28 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Sun, 22 Jan 2023 16:51:01 -0600
> Samuel Holland <samuel@sholland.org> wrote:
>
> Hi Joe, Ramon,
>
> > D1 is a RISC-V SoC containing an EMAC compatible with the A64 EMAC. In a
> > very roundabout way, this series finishes adding support for the D1 EMAC:
> > patch 4 resolves a compiler warning when building the driver for RISC-V.
> > The rest of the series is just cleanup requested by Jagan.
>
> So I reviewed, but also tested this series on an H3, A64, H6 and R40
> board, and Ethernet still worked on all of them.
> How do we handle the merge? Do you want to ACK the patches, and I
> take them through the sunxi tree? Or do you want to take the series?
> Patchwork put this on my plate, but get_maintainer.pl listed you two as
> the maintainers.
Hi.
I no longer maintain a tree. so please go ahead and apply those.
They look good.
>
> Cheers,
> Andre
>
>
> > Changes in v2:
> >  - Add a structure for driver data, and put the syscon offset there
> >
> > Samuel Holland (5):
> >   net: sun8i-emac: Add a structure for variant data
> >   net: sun8i-emac: Add a flag for RMII support
> >   net: sun8i-emac: Add a flag for the internal PHY switch
> >   net: sun8i-emac: Use common syscon setup for R40
> >   net: sun8i-emac: Remove the SoC variant ID
> >
> >  drivers/net/sun8i_emac.c | 96 ++++++++++++++++++++++------------------
> >  1 file changed, 53 insertions(+), 43 deletions(-)
> >
>