[PATCH 1/2] ag71xx: Add support for RMII, RGMII and SGMII

[PATCH 1/2] ag71xx: Add support for RMII, RGMII and SGMII

[Hauke Mehrtens]

The GMAC0 on the AR9344 also supports RMII and RGMII. This is an
external interface which gets connected to an external PHY or an
external switch. Without this patch the driver does not load on PHYs
configured to RMII or RGMII.

The QCA9563 often uses SGMII to connect to external switches.

This still misses the external interface configuration, but that was
also not done before the switch to phylink.

Fixes: 892e09153fa3 ("net: ag71xx: port to phylink")
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/net/ethernet/atheros/ag71xx.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
index 02b7705393ca..69125f870363 100644
--- a/drivers/net/ethernet/atheros/ag71xx.c
+++ b/drivers/net/ethernet/atheros/ag71xx.c
@@ -874,8 +874,11 @@ static void ag71xx_mac_validate(struct phylink_config *config,
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 
 	if (state->interface != PHY_INTERFACE_MODE_NA &&
+	    state->interface != PHY_INTERFACE_MODE_MII &&
+	    state->interface != PHY_INTERFACE_MODE_RMII &&
 	    state->interface != PHY_INTERFACE_MODE_GMII &&
-	    state->interface != PHY_INTERFACE_MODE_MII) {
+	    state->interface != PHY_INTERFACE_MODE_SGMII &&
+	    phy_interface_mode_is_rgmii(state->interface)) {
 		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
 		return;
 	}
@@ -889,7 +892,9 @@ static void ag71xx_mac_validate(struct phylink_config *config,
 	phylink_set(mask, 100baseT_Full);
 
 	if (state->interface == PHY_INTERFACE_MODE_NA ||
-	    state->interface == PHY_INTERFACE_MODE_GMII) {
+	    state->interface == PHY_INTERFACE_MODE_GMII ||
+	    state->interface == PHY_INTERFACE_MODE_SGMII ||
+	    phy_interface_mode_is_rgmii(state->interface)) {
 		phylink_set(mask, 1000baseT_Full);
 		phylink_set(mask, 1000baseX_Full);
 	}
-- 
2.20.1

[PATCH 2/2] ag71xx: Configure Ethernet interface

[Hauke Mehrtens]

Configure the Ethernet interface when the register range is provided.

The GMAC0 of the AR9330 and AR9340 can be operated in different modes,
like MII, RMII, GMII and RGMII. This allows to configure this mode in
the interface register block.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/net/ethernet/atheros/ag71xx.c | 76 +++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
index 69125f870363..7ef16fdd6617 100644
--- a/drivers/net/ethernet/atheros/ag71xx.c
+++ b/drivers/net/ethernet/atheros/ag71xx.c
@@ -64,6 +64,14 @@
 #define AG71XX_MDIO_DELAY	5
 #define AG71XX_MDIO_MAX_CLK	5000000
 
+/* GMAC Interface Registers */
+#define AG71XX_GMAC_REG_ETH_CFG	0x00
+#define ETH_CFG_RGMII_GE0	BIT(0)
+#define ETH_CFG_MII_GE0		BIT(1)
+#define ETH_CFG_GMII_GE0	BIT(2)
+#define ETH_CFG_RMII_GE0_AR933X	BIT(9)
+#define ETH_CFG_RMII_GE0_AR934X	BIT(10)
+
 /* Register offsets */
 #define AG71XX_REG_MAC_CFG1	0x0000
 #define MAC_CFG1_TXE		BIT(0)	/* Tx Enable */
@@ -311,6 +319,8 @@ struct ag71xx {
 	/* From this point onwards we're not looking at per-packet fields. */
 	void __iomem *mac_base;
 
+	void __iomem *inf_base;
+
 	struct ag71xx_desc *stop_desc;
 	dma_addr_t stop_desc_dma;
 
@@ -364,6 +374,18 @@ static u32 ag71xx_rr(struct ag71xx *ag, unsigned int reg)
 	return ioread32(ag->mac_base + reg);
 }
 
+static void ag71xx_inf_wr(struct ag71xx *ag, unsigned int reg, u32 value)
+{
+	iowrite32(value, ag->inf_base + reg);
+	/* flush write */
+	(void)ioread32(ag->inf_base + reg);
+}
+
+static u32 ag71xx_inf_rr(struct ag71xx *ag, unsigned int reg)
+{
+	return ioread32(ag->inf_base + reg);
+}
+
 static void ag71xx_sb(struct ag71xx *ag, unsigned int reg, u32 mask)
 {
 	void __iomem *r;
@@ -848,6 +870,52 @@ static void ag71xx_hw_start(struct ag71xx *ag)
 	netif_wake_queue(ag->ndev);
 }
 
+static void ag71xx_mac_config_inf(struct ag71xx *ag,
+				  const struct phylink_link_state *state)
+{
+	u32 val;
+
+	if (!ag71xx_is(ag, AR9330) && !ag71xx_is(ag, AR9340))
+		return;
+
+	val = ag71xx_inf_rr(ag, AG71XX_GMAC_REG_ETH_CFG);
+
+	val &= ~ETH_CFG_MII_GE0;
+	val &= ~ETH_CFG_GMII_GE0;
+	val &= ~ETH_CFG_RGMII_GE0;
+	if (ag71xx_is(ag, AR9330))
+		val &= ~ETH_CFG_RMII_GE0_AR933X;
+	else
+		val &= ~ETH_CFG_RMII_GE0_AR934X;
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_MII:
+		val |= ETH_CFG_MII_GE0;
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		if (ag71xx_is(ag, AR9330))
+			val |= ETH_CFG_RMII_GE0_AR933X;
+		else
+			val |= ETH_CFG_RMII_GE0_AR934X;
+		break;
+	case PHY_INTERFACE_MODE_GMII:
+		val |= ETH_CFG_GMII_GE0;
+		break;
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		val |= ETH_CFG_RGMII_GE0;
+		break;
+	default:
+		netif_err(ag, link, ag->ndev,
+			  "Unsupported interface: %d\n",
+			  state->interface);
+		return;
+	}
+	ag71xx_inf_wr(ag, AG71XX_GMAC_REG_ETH_CFG, val);
+}
+
 static void ag71xx_mac_config(struct phylink_config *config, unsigned int mode,
 			      const struct phylink_link_state *state)
 {
@@ -859,6 +927,9 @@ static void ag71xx_mac_config(struct phylink_config *config, unsigned int mode,
 	if (!ag71xx_is(ag, AR7100) && !ag71xx_is(ag, AR9130))
 		ag71xx_fast_reset(ag);
 
+	if (ag->inf_base)
+		ag71xx_mac_config_inf(ag, state);
+
 	if (ag->tx_ring.desc_split) {
 		ag->fifodata[2] &= 0xffff;
 		ag->fifodata[2] |= ((2048 - ag->tx_ring.desc_split) / 4) << 16;
@@ -1703,6 +1774,11 @@ static int ag71xx_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	/* The interface resource is optional */
+	ag->inf_base = devm_platform_ioremap_resource_byname(pdev, "interface");
+	if (IS_ERR(ag->inf_base) && PTR_ERR(ag->inf_base) != -ENOENT)
+		return PTR_ERR(ag->inf_base);
+
 	ag->clk_eth = devm_clk_get(&pdev->dev, "eth");
 	if (IS_ERR(ag->clk_eth)) {
 		netif_err(ag, probe, ndev, "Failed to get eth clk.\n");
-- 
2.20.1

Re: [PATCH 1/2] ag71xx: Add support for RMII, RGMII and SGMII

[Oleksij Rempel]

Am 02.03.20 um 01:18 schrieb Hauke Mehrtens:
> The GMAC0 on the AR9344 also supports RMII and RGMII. This is an
> external interface which gets connected to an external PHY or an
> external switch. Without this patch the driver does not load on PHYs
> configured to RMII or RGMII.
>
> The QCA9563 often uses SGMII to connect to external switches.
>
> This still misses the external interface configuration, but that was
> also not done before the switch to phylink.
>
> Fixes: 892e09153fa3 ("net: ag71xx: port to phylink")
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/net/ethernet/atheros/ag71xx.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
> index 02b7705393ca..69125f870363 100644
> --- a/drivers/net/ethernet/atheros/ag71xx.c
> +++ b/drivers/net/ethernet/atheros/ag71xx.c
> @@ -874,8 +874,11 @@ static void ag71xx_mac_validate(struct phylink_config *config,
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>
>  	if (state->interface != PHY_INTERFACE_MODE_NA &&
> +	    state->interface != PHY_INTERFACE_MODE_MII &&
> +	    state->interface != PHY_INTERFACE_MODE_RMII &&
>  	    state->interface != PHY_INTERFACE_MODE_GMII &&
> -	    state->interface != PHY_INTERFACE_MODE_MII) {
> +	    state->interface != PHY_INTERFACE_MODE_SGMII &&
> +	    phy_interface_mode_is_rgmii(state->interface)) {
>  		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
>  		return;
>  	}

Starting from now we need to do validation based on port and SoC. Not all mode are valid for all SoC
and ports. See ar9331_sw_phylink_validate as example.

> @@ -889,7 +892,9 @@ static void ag71xx_mac_validate(struct phylink_config *config,
>  	phylink_set(mask, 100baseT_Full);
>
>  	if (state->interface == PHY_INTERFACE_MODE_NA ||
> -	    state->interface == PHY_INTERFACE_MODE_GMII) {
> +	    state->interface == PHY_INTERFACE_MODE_GMII ||
> +	    state->interface == PHY_INTERFACE_MODE_SGMII ||
> +	    phy_interface_mode_is_rgmii(state->interface)) {
>  		phylink_set(mask, 1000baseT_Full);
>  		phylink_set(mask, 1000baseX_Full);
>  	}
>


--
Regards,
Oleksij

Re: [PATCH 2/2] ag71xx: Configure Ethernet interface

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 5287 bytes --]

Am 02.03.20 um 01:18 schrieb Hauke Mehrtens:
> Configure the Ethernet interface when the register range is provided.
> 
> The GMAC0 of the AR9330 

I was not able to find any ar9330 documentation or chip. Are there any the wild? The AR9331 do not
have support for external PHYs. RGMII on GMAC0 seems to be not supported or not working. At least,
currently I was not able to get it in working state.

> and AR9340 can be operated in different modes,
> like MII, RMII, GMII and RGMII. This allows to configure this mode in
> the interface register block.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/net/ethernet/atheros/ag71xx.c | 76 +++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
> index 69125f870363..7ef16fdd6617 100644
> --- a/drivers/net/ethernet/atheros/ag71xx.c
> +++ b/drivers/net/ethernet/atheros/ag71xx.c
> @@ -64,6 +64,14 @@
>  #define AG71XX_MDIO_DELAY	5
>  #define AG71XX_MDIO_MAX_CLK	5000000
>  
> +/* GMAC Interface Registers */
> +#define AG71XX_GMAC_REG_ETH_CFG	0x00
> +#define ETH_CFG_RGMII_GE0	BIT(0)
> +#define ETH_CFG_MII_GE0		BIT(1)
> +#define ETH_CFG_GMII_GE0	BIT(2)
> +#define ETH_CFG_RMII_GE0_AR933X	BIT(9)
> +#define ETH_CFG_RMII_GE0_AR934X	BIT(10)

According to the ar9344 doc this bit is MII_CNTL_SPEED, not RMII.

> +
>  /* Register offsets */
>  #define AG71XX_REG_MAC_CFG1	0x0000
>  #define MAC_CFG1_TXE		BIT(0)	/* Tx Enable */
> @@ -311,6 +319,8 @@ struct ag71xx {
>  	/* From this point onwards we're not looking at per-packet fields. */
>  	void __iomem *mac_base;
>  
> +	void __iomem *inf_base;

please use regmap instead.

> +
>  	struct ag71xx_desc *stop_desc;
>  	dma_addr_t stop_desc_dma;
>  
> @@ -364,6 +374,18 @@ static u32 ag71xx_rr(struct ag71xx *ag, unsigned int reg)
>  	return ioread32(ag->mac_base + reg);
>  }
>  
> +static void ag71xx_inf_wr(struct ag71xx *ag, unsigned int reg, u32 value)
> +{
> +	iowrite32(value, ag->inf_base + reg);
> +	/* flush write */
> +	(void)ioread32(ag->inf_base + reg);
> +}
> +
> +static u32 ag71xx_inf_rr(struct ag71xx *ag, unsigned int reg)
> +{
> +	return ioread32(ag->inf_base + reg);
> +}
> +
>  static void ag71xx_sb(struct ag71xx *ag, unsigned int reg, u32 mask)
>  {
>  	void __iomem *r;
> @@ -848,6 +870,52 @@ static void ag71xx_hw_start(struct ag71xx *ag)
>  	netif_wake_queue(ag->ndev);
>  }
>  
> +static void ag71xx_mac_config_inf(struct ag71xx *ag,
> +				  const struct phylink_link_state *state)
> +{
> +	u32 val;
> +
> +	if (!ag71xx_is(ag, AR9330) && !ag71xx_is(ag, AR9340))
> +		return;

We should do complete validate (port, chip and so on) here again, or do proper validation in
ag71xx_mac_validate() and trust provided results here.

I would prefer to do proper validation only one time.

> +
> +	val = ag71xx_inf_rr(ag, AG71XX_GMAC_REG_ETH_CFG);
> +
> +	val &= ~ETH_CFG_MII_GE0;
> +	val &= ~ETH_CFG_GMII_GE0;
> +	val &= ~ETH_CFG_RGMII_GE0;

Better val &= ~(ETH_CFG_MII_GE0 | ETH_CFG_GMII_GE0 ...)

> +	if (ag71xx_is(ag, AR9330))
> +		val &= ~ETH_CFG_RMII_GE0_AR933X;
> +	else
> +		val &= ~ETH_CFG_RMII_GE0_AR934X;
> +
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_MII:
> +		val |= ETH_CFG_MII_GE0;
> +		break;
> +	case PHY_INTERFACE_MODE_RMII:
> +		if (ag71xx_is(ag, AR9330))
> +			val |= ETH_CFG_RMII_GE0_AR933X;
> +		else
> +			val |= ETH_CFG_RMII_GE0_AR934X;
> +		break;
> +	case PHY_INTERFACE_MODE_GMII:
> +		val |= ETH_CFG_GMII_GE0;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		val |= ETH_CFG_RGMII_GE0;
> +		break;
> +	default:
> +		netif_err(ag, link, ag->ndev,
> +			  "Unsupported interface: %d\n",
> +			  state->interface);
> +		return;
> +	}
> +	ag71xx_inf_wr(ag, AG71XX_GMAC_REG_ETH_CFG, val);
> +}
> +
>  static void ag71xx_mac_config(struct phylink_config *config, unsigned int mode,
>  			      const struct phylink_link_state *state)
>  {
> @@ -859,6 +927,9 @@ static void ag71xx_mac_config(struct phylink_config *config, unsigned int mode,
>  	if (!ag71xx_is(ag, AR7100) && !ag71xx_is(ag, AR9130))
>  		ag71xx_fast_reset(ag);
>  
> +	if (ag->inf_base)
> +		ag71xx_mac_config_inf(ag, state);
> +
>  	if (ag->tx_ring.desc_split) {
>  		ag->fifodata[2] &= 0xffff;
>  		ag->fifodata[2] |= ((2048 - ag->tx_ring.desc_split) / 4) << 16;
> @@ -1703,6 +1774,11 @@ static int ag71xx_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	/* The interface resource is optional */
> +	ag->inf_base = devm_platform_ioremap_resource_byname(pdev, "interface");
> +	if (IS_ERR(ag->inf_base) && PTR_ERR(ag->inf_base) != -ENOENT)
> +		return PTR_ERR(ag->inf_base);

Please use regmap, this register should be shared with switch driver at least on ar9331. You will
need to define separate devicetree node for ETH_CFG register and link it to the eth0 node.

>  	ag->clk_eth = devm_clk_get(&pdev->dev, "eth");
>  	if (IS_ERR(ag->clk_eth)) {
>  		netif_err(ag, probe, ndev, "Failed to get eth clk.\n");
> 


-- 
Regards,
Oleksij


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