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

[PATCH net-next v2 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

Since v1:
  - The RGMII skew delays are now set based on the PHY interface mode
    being used (RGMII vs ID vs RXID vs TXID).
  - When RGMII is used, the skew delays are set to their default value,
    meaning we do not rely anymore on the bootloader's configuration.
  - Improved the commit messages.
  - s/phy_interface_mode_is_rgmii/phy_interface_is_rgmii/


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

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

-- 
2.24.1

[PATCH net-next v2 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..c389d7e59f91 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_is_rgmii(phydev)) {
+		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_is_rgmii(phydev)) {
+		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

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

[Andrew Lunn]

On Fri, Feb 28, 2020 at 04:57:00PM +0100, 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next v2 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 v2 3/3] net: phy: mscc: RGMII skew delay configuration

[Antoine Tenart]

This patch adds support for configuring the RGMII skew delays in Rx and
Tx. The delay value is retrieved from the device tree, and set based on
the PHY interface mode (rgmii, rgmii-id, rgmii-rx, rgmii-tx). If no
configuration is provided in the device tree, or of a delay isn't used,
its value will be set to the default one at probe time: this driver do
not rely anymore on the bootloader configuration for RGMII skews.

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

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index c389d7e59f91..4e9d788d95b9 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
@@ -2680,6 +2684,49 @@ static bool vsc8584_is_pkg_init(struct phy_device *phydev, bool reversed)
 	return false;
 }
 
+static void vsc8584_rgmii_set_skews(struct phy_device *phydev)
+{
+	u32 skew_rx, skew_tx;
+	struct device *dev = &phydev->mdio.dev;
+
+	/* We first set the Rx and Tx skews to their default value in h/w
+	 * (0.2 ns).
+	 */
+	skew_rx = VSC8584_RGMII_SKEW_0_2;
+	skew_tx = VSC8584_RGMII_SKEW_0_2;
+
+	/* Based on the interface mode, we then retrieve (if available) Rx
+	 * and/or Tx skews from the device tree. We do not fail if the
+	 * properties do not exist, the default skew configuration is a valid
+	 * one.
+	 */
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-rx",
+				     &skew_rx);
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-tx",
+				     &skew_tx);
+
+	/* Make sure we did not retrieve unsupported values. */
+	if (skew_rx > VSC8584_RGMII_SKEW_3_4) {
+		phydev_err(phydev, "Invalid Rx skew, fix the device tree.");
+		skew_rx = VSC8584_RGMII_SKEW_0_2;
+	}
+	if (skew_tx > VSC8584_RGMII_SKEW_3_4) {
+		phydev_err(phydev, "Invalid Tx skew, fix the device tree.");
+		skew_tx = VSC8584_RGMII_SKEW_0_2;
+	}
+
+	/* Finally we do apply the skew configuration. */
+	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));
+}
+
 static int vsc8584_config_init(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531 = phydev->priv;
@@ -2826,6 +2873,9 @@ static int vsc8584_config_init(struct phy_device *phydev)
 	       (VSC8584_MAC_IF_SELECTION_SGMII << VSC8584_MAC_IF_SELECTION_POS);
 	ret = phy_write(phydev, MSCC_PHY_EXT_PHY_CNTL_1, val);
 
+	if (phy_interface_is_rgmii(phydev))
+		vsc8584_rgmii_set_skews(phydev);
+
 	ret = genphy_soft_reset(phydev);
 	if (ret)
 		return ret;
-- 
2.24.1

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

[Andrew Lunn]

> +static void vsc8584_rgmii_set_skews(struct phy_device *phydev)
> +{
> +	u32 skew_rx, skew_tx;
> +	struct device *dev = &phydev->mdio.dev;
> +
> +	/* We first set the Rx and Tx skews to their default value in h/w
> +	 * (0.2 ns).
> +	 */
> +	skew_rx = VSC8584_RGMII_SKEW_0_2;
> +	skew_tx = VSC8584_RGMII_SKEW_0_2;
> +
> +	/* Based on the interface mode, we then retrieve (if available) Rx
> +	 * and/or Tx skews from the device tree. We do not fail if the
> +	 * properties do not exist, the default skew configuration is a valid
> +	 * one.
> +	 */
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
> +		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-rx",
> +				     &skew_rx);
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> +		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-tx",
> +				     &skew_tx);

Hi Antoine

That is not the correct meaning of PHY_INTERFACE_MODE_RGMII_ID etc.
PHY_INTERFACE_MODE_RGMII_ID means add a 2ns delay on both RX and TX.
PHY_INTERFACE_MODE_RGMII_RXID means add a 2ns delay on the RX.
PHY_INTERFACE_MODE_RGMII means 0 delay, with the assumption that
either the MAC is adding the delay, or the PCB is designed with extra
copper tracks to add the delay.

You only need "vsc8584,rgmii-skew-rx" when 2ns is not correct for you
board, and you need more fine grain control.

What is not clearly defined, is how you combine
PHY_INTERFACE_MODE_RGMII* with DT properties. I guess i would enforce
that phydev->interface is PHY_INTERFACE_MODE_RGMII and then the delays
in DT are absolute values.

There is also some discussion about what should go in DT. #defines
like you have, or time in pS, and the driver converts to register
values. I generally push towards pS.

	 Andrew

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

[Antoine Tenart]

Hi Andrew,

On Fri, Feb 28, 2020 at 05:29:42PM +0100, Andrew Lunn wrote:
> > +static void vsc8584_rgmii_set_skews(struct phy_device *phydev)
> > +{
> > +	u32 skew_rx, skew_tx;
> > +	struct device *dev = &phydev->mdio.dev;
> > +
> > +	/* We first set the Rx and Tx skews to their default value in h/w
> > +	 * (0.2 ns).
> > +	 */
> > +	skew_rx = VSC8584_RGMII_SKEW_0_2;
> > +	skew_tx = VSC8584_RGMII_SKEW_0_2;
> > +
> > +	/* Based on the interface mode, we then retrieve (if available) Rx
> > +	 * and/or Tx skews from the device tree. We do not fail if the
> > +	 * properties do not exist, the default skew configuration is a valid
> > +	 * one.
> > +	 */
> > +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
> > +		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-rx",
> > +				     &skew_rx);
> > +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> > +		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-tx",
> > +				     &skew_tx);
> 
> That is not the correct meaning of PHY_INTERFACE_MODE_RGMII_ID etc.
> PHY_INTERFACE_MODE_RGMII_ID means add a 2ns delay on both RX and TX.
> PHY_INTERFACE_MODE_RGMII_RXID means add a 2ns delay on the RX.
> PHY_INTERFACE_MODE_RGMII means 0 delay, with the assumption that
> either the MAC is adding the delay, or the PCB is designed with extra
> copper tracks to add the delay.
> 
> You only need "vsc8584,rgmii-skew-rx" when 2ns is not correct for you
> board, and you need more fine grain control.

I did not know that, thanks for the explanation! So if ID/TXID/RXID is
used, I should configure the Rx and/or Tx skew with
VSC8584_RGMII_SKEW_2_0, except if the dt says otherwise.

> What is not clearly defined, is how you combine
> PHY_INTERFACE_MODE_RGMII* with DT properties. I guess i would enforce
> that phydev->interface is PHY_INTERFACE_MODE_RGMII and then the delays
> in DT are absolute values.

So, if there's a value in the device tree, and the mode corresponds
(RXID for Rx skew), we do use the dt value. That should look like what's
in the patch (except for the default value used when no configuration is
provided in the dt).

> There is also some discussion about what should go in DT. #defines
> like you have, or time in pS, and the driver converts to register
> values. I generally push towards pS.

That would allow a more generic dt binding, and could be used by other
PHY drivers. The difficulty would be to map the pS to allowed values in
the h/w. Should we round them to the upper or lower bound?

I also saw the micrel-ksz90x1 dt documentation describes many skews, not
only one for Rx and one for Tx. How would the generic dt binding would
look like then?

Thanks!
Antoine

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

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

[Andrew Lunn]

> I did not know that, thanks for the explanation! So if ID/TXID/RXID is
> used, I should configure the Rx and/or Tx skew with
> VSC8584_RGMII_SKEW_2_0, except if the dt says otherwise.

Yes.

> > What is not clearly defined, is how you combine
> > PHY_INTERFACE_MODE_RGMII* with DT properties. I guess i would enforce
> > that phydev->interface is PHY_INTERFACE_MODE_RGMII and then the delays
> > in DT are absolute values.
> 
> So, if there's a value in the device tree, and the mode corresponds
> (RXID for Rx skew), we do use the dt value. That should look like what's
> in the patch (except for the default value used when no configuration is
> provided in the dt).

No. I would not do that. PHY_INTERFACE_MODE_RGMII_RXID means 2ns delay
for RX. So how do you then interpret the DT property. Is it 2ns + the
DT delay? That would then mean you need negative values in DT if you
want short delays than 2ns.

Which is why i suggest PHY_INTERFACE_MODE_RGMII. It is then clear you
have a base delay of 0ns, and the DT property then has the absolute
value of the delay.

> > There is also some discussion about what should go in DT. #defines
> > like you have, or time in pS, and the driver converts to register
> > values. I generally push towards pS.
> 
> That would allow a more generic dt binding, and could be used by other
> PHY drivers. The difficulty would be to map the pS to allowed values in
> the h/w. Should we round them to the upper or lower bound?

I would document the accepted values in DT, and return -EINVAL if DT
does not exactly match one of the listed values. Plus a phydev_err()
message to help debug.

> I also saw the micrel-ksz90x1 dt documentation describes many skews, not
> only one for Rx and one for Tx. How would the generic dt binding would
> look like then?

It is a balancing act. Do you actually need dt properties for your
hardware? Are the standard 2ns delays sufficient. For many designs it
is. Just because the hardware supports all sorts of configurations,
are they actually needed? It seems like adding delays are needed for a
few boards. But do all the properties exposed for the Micrel PHY every
get used, or is it a case of, the hardware has it, lets make it
available, even if nobody ever uses it?

So i think a standardized DT for delays is good, but i would not go
any further.

    Andrew

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

[Antoine Tenart]

On Fri, Feb 28, 2020 at 06:26:16PM +0100, Andrew Lunn wrote:
> > > What is not clearly defined, is how you combine
> > > PHY_INTERFACE_MODE_RGMII* with DT properties. I guess i would enforce
> > > that phydev->interface is PHY_INTERFACE_MODE_RGMII and then the delays
> > > in DT are absolute values.
> > 
> > So, if there's a value in the device tree, and the mode corresponds
> > (RXID for Rx skew), we do use the dt value. That should look like what's
> > in the patch (except for the default value used when no configuration is
> > provided in the dt).
> 
> No. I would not do that. PHY_INTERFACE_MODE_RGMII_RXID means 2ns delay
> for RX. So how do you then interpret the DT property. Is it 2ns + the
> DT delay? That would then mean you need negative values in DT if you
> want short delays than 2ns.
> 
> Which is why i suggest PHY_INTERFACE_MODE_RGMII. It is then clear you
> have a base delay of 0ns, and the DT property then has the absolute
> value of the delay.

OK I see, to avoid confusion we could either define RGMII_ID or RGMII
and fixed delays in the dt if the 2ns one isn't what we need.

We may need an RGMII dedicated documentation then, to avoid future
confusion :)

> > > There is also some discussion about what should go in DT. #defines
> > > like you have, or time in pS, and the driver converts to register
> > > values. I generally push towards pS.
> > 
> > That would allow a more generic dt binding, and could be used by other
> > PHY drivers. The difficulty would be to map the pS to allowed values in
> > the h/w. Should we round them to the upper or lower bound?
> 
> I would document the accepted values in DT, and return -EINVAL if DT
> does not exactly match one of the listed values. Plus a phydev_err()
> message to help debug.

OK.

> > I also saw the micrel-ksz90x1 dt documentation describes many skews, not
> > only one for Rx and one for Tx. How would the generic dt binding would
> > look like then?
> 
> It is a balancing act. Do you actually need dt properties for your
> hardware? Are the standard 2ns delays sufficient. For many designs it
> is. Just because the hardware supports all sorts of configurations,
> are they actually needed? It seems like adding delays are needed for a
> few boards. But do all the properties exposed for the Micrel PHY every
> get used, or is it a case of, the hardware has it, lets make it
> available, even if nobody ever uses it?

Right, this kind of settings shouldn't be needed for lots of boards, so
we can add a per-PHY binding, only when it's needed. The board I'm
currently working on do use smaller delays than 2ns and I was told to
use even lower ones if the connexion wasn't stable. But then do we
really need such a configuration upstream (apart from supporting the 2ns
delays)? That's a good question :)

Thanks!
Antoine

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