[PATCH net-next 0/3] net: phy: mscc: add support for RGMII MAC mode

[PATCH net-next 0/3] net: phy: mscc: add support for RGMII MAC mode

[Antoine Tenart]

Hello,

This series adds support for the RGMII MAC mode for the VSC8584 PHY
family.

The first patch adds support for configuring the PHY MAC mode based on
phydev->interface.

The second and third patches add new dt bindings for the MSCC driver, to
configure the RGMII Tx and Rx skews from the device tree.

Thanks!
Antoine


Antoine Tenart (3):
  net: phy: mscc: add support for RGMII MAC mode
  dt-bindings: net: phy: mscc: document rgmii skew properties
  net: phy: mscc: implement RGMII skew delay configuration

 .../bindings/net/mscc-phy-vsc8531.txt         |  8 +++
 drivers/net/phy/mscc.c                        | 51 ++++++++++++++-----
 include/dt-bindings/net/mscc-phy-vsc8531.h    | 10 ++++
 3 files changed, 57 insertions(+), 12 deletions(-)

-- 
2.24.1

[PATCH net-next 1/3] net: phy: mscc: add support for RGMII MAC mode

[Antoine Tenart]

This patch adds support for connecting VSC8584 PHYs to the MAC using
RGMII.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/mscc.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index d24577de0775..ecb45c43e5ed 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -272,6 +272,7 @@ enum macsec_bank {
 #define MAC_CFG_MASK			  0xc000
 #define MAC_CFG_SGMII			  0x0000
 #define MAC_CFG_QSGMII			  0x4000
+#define MAC_CFG_RGMII			  0x8000
 
 /* Test page Registers */
 #define MSCC_PHY_TEST_PAGE_5		  5
@@ -2751,27 +2752,35 @@ static int vsc8584_config_init(struct phy_device *phydev)
 
 	val = phy_base_read(phydev, MSCC_PHY_MAC_CFG_FASTLINK);
 	val &= ~MAC_CFG_MASK;
-	if (phydev->interface == PHY_INTERFACE_MODE_QSGMII)
+	if (phydev->interface == PHY_INTERFACE_MODE_QSGMII) {
 		val |= MAC_CFG_QSGMII;
-	else
+	} else if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
 		val |= MAC_CFG_SGMII;
+	} else if (phy_interface_mode_is_rgmii(phydev->interface)) {
+		val |= MAC_CFG_RGMII;
+	} else {
+		ret = -EINVAL;
+		goto err;
+	}
 
 	ret = phy_base_write(phydev, MSCC_PHY_MAC_CFG_FASTLINK, val);
 	if (ret)
 		goto err;
 
-	val = PROC_CMD_MCB_ACCESS_MAC_CONF | PROC_CMD_RST_CONF_PORT |
-		PROC_CMD_READ_MOD_WRITE_PORT;
-	if (phydev->interface == PHY_INTERFACE_MODE_QSGMII)
-		val |= PROC_CMD_QSGMII_MAC;
-	else
-		val |= PROC_CMD_SGMII_MAC;
+	if (!phy_interface_mode_is_rgmii(phydev->interface)) {
+		val = PROC_CMD_MCB_ACCESS_MAC_CONF | PROC_CMD_RST_CONF_PORT |
+		      PROC_CMD_READ_MOD_WRITE_PORT;
+		if (phydev->interface == PHY_INTERFACE_MODE_QSGMII)
+			val |= PROC_CMD_QSGMII_MAC;
+		else
+			val |= PROC_CMD_SGMII_MAC;
 
-	ret = vsc8584_cmd(phydev, val);
-	if (ret)
-		goto err;
+		ret = vsc8584_cmd(phydev, val);
+		if (ret)
+			goto err;
 
-	usleep_range(10000, 20000);
+		usleep_range(10000, 20000);
+	}
 
 	/* Disable SerDes for 100Base-FX */
 	ret = vsc8584_cmd(phydev, PROC_CMD_FIBER_MEDIA_CONF |
-- 
2.24.1

[PATCH net-next 2/3] dt-bindings: net: phy: mscc: document rgmii skew properties

[Antoine Tenart]

This patch documents two new properties to describe RGMII skew delays in
both Rx and Tx: vsc8584,rgmii-skew-rx and vsc8584,rgmii-skew-tx.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 .../devicetree/bindings/net/mscc-phy-vsc8531.txt       |  8 ++++++++
 include/dt-bindings/net/mscc-phy-vsc8531.h             | 10 ++++++++++
 2 files changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 5ff37c68c941..c682b6e74b14 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
@@ -31,6 +31,14 @@ Optional properties:
 			  VSC8531_LINK_100_ACTIVITY (2),
 			  VSC8531_LINK_ACTIVITY (0) and
 			  VSC8531_DUPLEX_COLLISION (8).
+- vsc8584,rgmii-skew-rx	: RGMII skew delay in Rx.
+			  Allowed values are defined in
+			  "include/dt-bindings/net/mscc-phy-vsc8531.h".
+			  Default value is VSC8584_RGMII_SKEW_0_2.
+- vsc8584,rgmii-skew-tx	: RGMII skew delay in Tx.
+			  Allowed values are defined in
+			  "include/dt-bindings/net/mscc-phy-vsc8531.h".
+			  Default value is VSC8584_RGMII_SKEW_0_2.
 
 
 Table: 1 - Edge rate change
diff --git a/include/dt-bindings/net/mscc-phy-vsc8531.h b/include/dt-bindings/net/mscc-phy-vsc8531.h
index 9eb2ec2b2ea9..02313cb3fc35 100644
--- a/include/dt-bindings/net/mscc-phy-vsc8531.h
+++ b/include/dt-bindings/net/mscc-phy-vsc8531.h
@@ -28,4 +28,14 @@
 #define VSC8531_FORCE_LED_OFF           14
 #define VSC8531_FORCE_LED_ON            15
 
+/* RGMII skew values, in ns */
+#define VSC8584_RGMII_SKEW_0_2		0
+#define VSC8584_RGMII_SKEW_0_8		1
+#define VSC8584_RGMII_SKEW_1_1		2
+#define VSC8584_RGMII_SKEW_1_7		3
+#define VSC8584_RGMII_SKEW_2_0		4
+#define VSC8584_RGMII_SKEW_2_3		5
+#define VSC8584_RGMII_SKEW_2_6		6
+#define VSC8584_RGMII_SKEW_3_4		7
+
 #endif
-- 
2.24.1

[PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration

[Antoine Tenart]

This patch adds support for configuring the RGMII skews in Rx and Tx
thanks to properties defined in the device tree.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/mscc.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index ecb45c43e5ed..56d6a45a90c2 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -192,6 +192,10 @@ enum macsec_bank {
 /* Extended Page 2 Registers */
 #define MSCC_PHY_CU_PMD_TX_CNTL		  16
 
+#define MSCC_PHY_RGMII_SETTINGS		  18
+#define RGMII_SKEW_RX_POS		  1
+#define RGMII_SKEW_TX_POS		  4
+
 #define MSCC_PHY_RGMII_CNTL		  20
 #define RGMII_RX_CLK_DELAY_MASK		  0x0070
 #define RGMII_RX_CLK_DELAY_POS		  4
@@ -2682,6 +2686,7 @@ static bool vsc8584_is_pkg_init(struct phy_device *phydev, bool reversed)
 
 static int vsc8584_config_init(struct phy_device *phydev)
 {
+	u32 skew_rx = VSC8584_RGMII_SKEW_0_2, skew_tx = VSC8584_RGMII_SKEW_0_2;
 	struct vsc8531_private *vsc8531 = phydev->priv;
 	u16 addr, val;
 	int ret, i;
@@ -2830,6 +2835,19 @@ static int vsc8584_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	if (of_find_property(dev->of_node, "vsc8584,rgmii-skew-rx", NULL) ||
+	    of_find_property(dev->of_node, "vsc8584,rgmii-skew-tx", NULL)) {
+		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-rx", &skew_rx);
+		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-tx", &skew_tx);
+
+		phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
+				 MSCC_PHY_RGMII_SETTINGS,
+				 (0x7 << RGMII_SKEW_RX_POS) |
+				 (0x7 << RGMII_SKEW_TX_POS),
+				 (skew_rx << RGMII_SKEW_RX_POS) |
+				 (skew_tx << RGMII_SKEW_TX_POS));
+	}
+
 	for (i = 0; i < vsc8531->nleds; i++) {
 		ret = vsc85xx_led_cntl_set(phydev, i, vsc8531->leds_mode[i]);
 		if (ret)
-- 
2.24.1

Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration

[Andrew Lunn]

On Thu, Feb 27, 2020 at 04:28:59PM +0100, Antoine Tenart wrote:
> This patch adds support for configuring the RGMII skews in Rx and Tx
> thanks to properties defined in the device tree.

Hi Antoine

What you are not handling here in this patchset is the four RGMII
modes, and what they mean in terms of delay:

        PHY_INTERFACE_MODE_RGMII,
        PHY_INTERFACE_MODE_RGMII_ID,
        PHY_INTERFACE_MODE_RGMII_RXID,
        PHY_INTERFACE_MODE_RGMII_TXID,

The PHY driver should be adding delays based on these
values. Generally, that is enough to make the link work. You only need
additional skew in DT when you need finer grain control than what
these provide.

      Andrew

Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration

[Antoine Tenart]

Hello Andrew,

On Thu, Feb 27, 2020 at 04:51:28PM +0100, Andrew Lunn wrote:
> On Thu, Feb 27, 2020 at 04:28:59PM +0100, Antoine Tenart wrote:
> > This patch adds support for configuring the RGMII skews in Rx and Tx
> > thanks to properties defined in the device tree.
> 
> Hi Antoine
> 
> What you are not handling here in this patchset is the four RGMII
> modes, and what they mean in terms of delay:
> 
>         PHY_INTERFACE_MODE_RGMII,
>         PHY_INTERFACE_MODE_RGMII_ID,
>         PHY_INTERFACE_MODE_RGMII_RXID,
>         PHY_INTERFACE_MODE_RGMII_TXID,
> 
> The PHY driver should be adding delays based on these
> values. Generally, that is enough to make the link work. You only need
> additional skew in DT when you need finer grain control than what
> these provide.

Oh, that's right. I'll fix the series and resubmit.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 1/3] net: phy: mscc: add support for RGMII MAC mode

[Quentin Schulz]

Hi Antoine,

I guess I slipped through in your Cc list but now that it's too late to 
undo it, I'll give my 2¢ :)

On 2020-02-27 16:28, Antoine Tenart wrote:
> This patch adds support for connecting VSC8584 PHYs to the MAC using
> RGMII.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> ---
>  drivers/net/phy/mscc.c | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> index d24577de0775..ecb45c43e5ed 100644
> --- a/drivers/net/phy/mscc.c
> +++ b/drivers/net/phy/mscc.c
> @@ -272,6 +272,7 @@ enum macsec_bank {
>  #define MAC_CFG_MASK			  0xc000
>  #define MAC_CFG_SGMII			  0x0000
>  #define MAC_CFG_QSGMII			  0x4000
> +#define MAC_CFG_RGMII			  0x8000
> 
>  /* Test page Registers */
>  #define MSCC_PHY_TEST_PAGE_5		  5
> @@ -2751,27 +2752,35 @@ static int vsc8584_config_init(struct
> phy_device *phydev)
> 
>  	val = phy_base_read(phydev, MSCC_PHY_MAC_CFG_FASTLINK);
>  	val &= ~MAC_CFG_MASK;
> -	if (phydev->interface == PHY_INTERFACE_MODE_QSGMII)
> +	if (phydev->interface == PHY_INTERFACE_MODE_QSGMII) {
>  		val |= MAC_CFG_QSGMII;
> -	else
> +	} else if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
>  		val |= MAC_CFG_SGMII;
> +	} else if (phy_interface_mode_is_rgmii(phydev->interface)) {

Nitpick:
I don't know much the difference between that one and 
phy_interface_is_rgmii wrt when one should be used and not the other, 
but seeing the implementation 
(https://elixir.bootlin.com/linux/latest/source/include/linux/phy.h#L999)... 
we should be safe :) Since you already have a phydev in hands at that 
time, maybe using phy_interface_is_rgmii would be cleaner? (shorter).

> +		val |= MAC_CFG_RGMII;
> +	} else {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> 
>  	ret = phy_base_write(phydev, MSCC_PHY_MAC_CFG_FASTLINK, val);
>  	if (ret)
>  		goto err;
> 
> -	val = PROC_CMD_MCB_ACCESS_MAC_CONF | PROC_CMD_RST_CONF_PORT |
> -		PROC_CMD_READ_MOD_WRITE_PORT;
> -	if (phydev->interface == PHY_INTERFACE_MODE_QSGMII)
> -		val |= PROC_CMD_QSGMII_MAC;
> -	else
> -		val |= PROC_CMD_SGMII_MAC;
> +	if (!phy_interface_mode_is_rgmii(phydev->interface)) {

Ditto.

Thanks,
Quentin

Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration

[Quentin Schulz]

Hi Antoine,

It's still me, nitpicker.

On 2020-02-27 16:28, Antoine Tenart wrote:
> This patch adds support for configuring the RGMII skews in Rx and Tx
> thanks to properties defined in the device tree.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> ---
>  drivers/net/phy/mscc.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> index ecb45c43e5ed..56d6a45a90c2 100644
> --- a/drivers/net/phy/mscc.c
> +++ b/drivers/net/phy/mscc.c
> @@ -192,6 +192,10 @@ enum macsec_bank {
>  /* Extended Page 2 Registers */
>  #define MSCC_PHY_CU_PMD_TX_CNTL		  16
> 
> +#define MSCC_PHY_RGMII_SETTINGS		  18
> +#define RGMII_SKEW_RX_POS		  1
> +#define RGMII_SKEW_TX_POS		  4
> +
>  #define MSCC_PHY_RGMII_CNTL		  20
>  #define RGMII_RX_CLK_DELAY_MASK		  0x0070
>  #define RGMII_RX_CLK_DELAY_POS		  4
> @@ -2682,6 +2686,7 @@ static bool vsc8584_is_pkg_init(struct
> phy_device *phydev, bool reversed)
> 
>  static int vsc8584_config_init(struct phy_device *phydev)
>  {
> +	u32 skew_rx = VSC8584_RGMII_SKEW_0_2, skew_tx = 
> VSC8584_RGMII_SKEW_0_2;
>  	struct vsc8531_private *vsc8531 = phydev->priv;
>  	u16 addr, val;
>  	int ret, i;
> @@ -2830,6 +2835,19 @@ static int vsc8584_config_init(struct phy_device 
> *phydev)
>  	if (ret)
>  		return ret;
> 
> +	if (of_find_property(dev->of_node, "vsc8584,rgmii-skew-rx", NULL) ||
> +	    of_find_property(dev->of_node, "vsc8584,rgmii-skew-tx", NULL)) {
> +		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-rx", 
> &skew_rx);
> +		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-tx", 
> &skew_tx);
> +

Reading the code, I think **!**of_property_read_u32 could directly 
replace of_find_property in your condition and spare you two calls to 
that function.

Also, do we actually need to write that register only when skews are 
defined in the DT? Can't we just write to it anyway (I guess the fact 
that 0_2 skew is actually 0 in value should put me on the right path but 
I prefer to ask).

Final nitpick: I would see a check of the skew_rx/tx from DT before you 
put them in the following line, they could be drastically different from 
0-8 value set that you expect considering you're reading a u32 (pass 
them through a GENMASK at least?)

> +		phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
> +				 MSCC_PHY_RGMII_SETTINGS,
> +				 (0x7 << RGMII_SKEW_RX_POS) |
> +				 (0x7 << RGMII_SKEW_TX_POS),
> +				 (skew_rx << RGMII_SKEW_RX_POS) |
> +				 (skew_tx << RGMII_SKEW_TX_POS));
> +	}
> +

Thanks,
Quentin

Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration

[Andrew Lunn]

> Also, do we actually need to write that register only when skews are defined
> in the DT? Can't we just write to it anyway (I guess the fact that 0_2 skew
> is actually 0 in value should put me on the right path but I prefer to ask).

Hi Quentin

Ideally, you don't want to rely on the boot loader doing some
magic. So i would prefer the skew is set to 0 if the properties are
not present.

    Andrew

Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration

[Antoine Tenart]

On Thu, Feb 27, 2020 at 05:25:06PM +0100, Andrew Lunn wrote:
> > Also, do we actually need to write that register only when skews are defined
> > in the DT? Can't we just write to it anyway (I guess the fact that 0_2 skew
> > is actually 0 in value should put me on the right path but I prefer to ask).
> 
> Ideally, you don't want to rely on the boot loader doing some
> magic. So i would prefer the skew is set to 0 if the properties are
> not present.

Will do.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration

[Antoine Tenart]

Hi Quentin,

On Thu, Feb 27, 2020 at 05:21:58PM +0100, Quentin Schulz wrote:
> On 2020-02-27 16:28, Antoine Tenart wrote:
> > 
> > +	if (of_find_property(dev->of_node, "vsc8584,rgmii-skew-rx", NULL) ||
> > +	    of_find_property(dev->of_node, "vsc8584,rgmii-skew-tx", NULL)) {
> > +		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-rx",
> > &skew_rx);
> > +		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-tx",
> > &skew_tx);
> > +
> 
> Reading the code, I think **!**of_property_read_u32 could directly replace
> of_find_property in your condition and spare you two calls to that function.

Sure.

> Final nitpick: I would see a check of the skew_rx/tx from DT before you put
> them in the following line, they could be drastically different from 0-8
> value set that you expect considering you're reading a u32 (pass them
> through a GENMASK at least?)

That makes sense, I can add a check.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com