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

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH net-next 0/3] net: phy: mscc: add support for RGMII MAC mode
@ 2020-02-27 15:28 Antoine Tenart
  2020-02-27 15:28 ` [PATCH net-next 1/3] " Antoine Tenart
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Antoine Tenart @ 2020-02-27 15:28 UTC (permalink / raw)
  To: davem, andrew, f.fainelli, hkallweit1
  Cc: Antoine Tenart, netdev, linux-kernel, foss

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH net-next 1/3] net: phy: mscc: add support for RGMII MAC mode
  2020-02-27 15:28 [PATCH net-next 0/3] net: phy: mscc: add support for RGMII MAC mode Antoine Tenart
@ 2020-02-27 15:28 ` Antoine Tenart
  2020-02-27 16:09   ` Quentin Schulz
  2020-02-27 15:28 ` [PATCH net-next 2/3] dt-bindings: net: phy: mscc: document rgmii skew properties Antoine Tenart
  2020-02-27 15:28 ` [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration Antoine Tenart
  2 siblings, 1 reply; 13+ messages in thread
From: Antoine Tenart @ 2020-02-27 15:28 UTC (permalink / raw)
  To: davem, andrew, f.fainelli, hkallweit1
  Cc: Antoine Tenart, netdev, linux-kernel, foss

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 1/3] net: phy: mscc: add support for RGMII MAC mode
  2020-02-27 15:28 ` [PATCH net-next 1/3] " Antoine Tenart
@ 2020-02-27 16:09   ` Quentin Schulz
  0 siblings, 0 replies; 13+ messages in thread
From: Quentin Schulz @ 2020-02-27 16:09 UTC (permalink / raw)
  To: Antoine Tenart
  Cc: davem, andrew, f.fainelli, hkallweit1, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH net-next 2/3] dt-bindings: net: phy: mscc: document rgmii skew properties
  2020-02-27 15:28 [PATCH net-next 0/3] net: phy: mscc: add support for RGMII MAC mode Antoine Tenart
  2020-02-27 15:28 ` [PATCH net-next 1/3] " Antoine Tenart
@ 2020-02-27 15:28 ` Antoine Tenart
  2020-02-27 15:28 ` [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration Antoine Tenart
  2 siblings, 0 replies; 13+ messages in thread
From: Antoine Tenart @ 2020-02-27 15:28 UTC (permalink / raw)
  To: davem, andrew, f.fainelli, hkallweit1
  Cc: Antoine Tenart, netdev, linux-kernel, foss

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration
  2020-02-27 15:28 [PATCH net-next 0/3] net: phy: mscc: add support for RGMII MAC mode Antoine Tenart
  2020-02-27 15:28 ` [PATCH net-next 1/3] " Antoine Tenart
  2020-02-27 15:28 ` [PATCH net-next 2/3] dt-bindings: net: phy: mscc: document rgmii skew properties Antoine Tenart
@ 2020-02-27 15:28 ` Antoine Tenart
  2020-02-27 15:51   ` Andrew Lunn
                     ` (3 more replies)
  2 siblings, 4 replies; 13+ messages in thread
From: Antoine Tenart @ 2020-02-27 15:28 UTC (permalink / raw)
  To: davem, andrew, f.fainelli, hkallweit1
  Cc: Antoine Tenart, netdev, linux-kernel, foss

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration
  2020-02-27 15:28 ` [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration Antoine Tenart
@ 2020-02-27 15:51   ` Andrew Lunn
  2020-02-27 16:01     ` Antoine Tenart
  2020-02-27 16:21   ` Quentin Schulz
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Andrew Lunn @ 2020-02-27 15:51 UTC (permalink / raw)
  To: Antoine Tenart; +Cc: davem, f.fainelli, hkallweit1, netdev, linux-kernel, foss

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration
  2020-02-27 15:51   ` Andrew Lunn
@ 2020-02-27 16:01     ` Antoine Tenart
  0 siblings, 0 replies; 13+ messages in thread
From: Antoine Tenart @ 2020-02-27 16:01 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Antoine Tenart, davem, f.fainelli, hkallweit1, netdev,
	linux-kernel, foss

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration
  2020-02-27 15:28 ` [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration Antoine Tenart
  2020-02-27 15:51   ` Andrew Lunn
@ 2020-02-27 16:21   ` Quentin Schulz
  2020-02-27 16:25     ` Andrew Lunn
  2020-02-27 18:55     ` Antoine Tenart
  2020-02-28 15:07   ` kbuild test robot
  2020-02-28 16:24   ` kbuild test robot
  3 siblings, 2 replies; 13+ messages in thread
From: Quentin Schulz @ 2020-02-27 16:21 UTC (permalink / raw)
  To: Antoine Tenart
  Cc: davem, andrew, f.fainelli, hkallweit1, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration
  2020-02-27 16:21   ` Quentin Schulz
@ 2020-02-27 16:25     ` Andrew Lunn
  2020-02-27 18:53       ` Antoine Tenart
  2020-02-27 18:55     ` Antoine Tenart
  1 sibling, 1 reply; 13+ messages in thread
From: Andrew Lunn @ 2020-02-27 16:25 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Antoine Tenart, davem, f.fainelli, hkallweit1, netdev, linux-kernel

> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration
  2020-02-27 16:25     ` Andrew Lunn
@ 2020-02-27 18:53       ` Antoine Tenart
  0 siblings, 0 replies; 13+ messages in thread
From: Antoine Tenart @ 2020-02-27 18:53 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Quentin Schulz, Antoine Tenart, davem, f.fainelli, hkallweit1,
	netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration
  2020-02-27 16:21   ` Quentin Schulz
  2020-02-27 16:25     ` Andrew Lunn
@ 2020-02-27 18:55     ` Antoine Tenart
  1 sibling, 0 replies; 13+ messages in thread
From: Antoine Tenart @ 2020-02-27 18:55 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Antoine Tenart, davem, andrew, f.fainelli, hkallweit1, netdev,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration
  2020-02-27 15:28 ` [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration Antoine Tenart
  2020-02-27 15:51   ` Andrew Lunn
  2020-02-27 16:21   ` Quentin Schulz
@ 2020-02-28 15:07   ` kbuild test robot
  2020-02-28 16:24   ` kbuild test robot
  3 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2020-02-28 15:07 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3223 bytes --]

Hi Antoine,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on robh/for-next net/master linus/master v5.6-rc3 next-20200227]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Antoine-Tenart/net-phy-mscc-add-support-for-RGMII-MAC-mode/20200228-185429
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 2b99e54b30ed56201dedd91e6049ed83aa9d2302
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/phy/mscc.c: In function 'vsc8584_config_init':
>> drivers/net/phy/mscc.c:2838:23: error: 'dev' undeclared (first use in this function); did you mean 'cdev'?
     if (of_find_property(dev->of_node, "vsc8584,rgmii-skew-rx", NULL) ||
                          ^~~
                          cdev
   drivers/net/phy/mscc.c:2838:23: note: each undeclared identifier is reported only once for each function it appears in

vim +2838 drivers/net/phy/mscc.c

  2825	
  2826		phy_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_STANDARD);
  2827	
  2828		val = phy_read(phydev, MSCC_PHY_EXT_PHY_CNTL_1);
  2829		val &= ~(MEDIA_OP_MODE_MASK | VSC8584_MAC_IF_SELECTION_MASK);
  2830		val |= (MEDIA_OP_MODE_COPPER << MEDIA_OP_MODE_POS) |
  2831		       (VSC8584_MAC_IF_SELECTION_SGMII << VSC8584_MAC_IF_SELECTION_POS);
  2832		ret = phy_write(phydev, MSCC_PHY_EXT_PHY_CNTL_1, val);
  2833	
  2834		ret = genphy_soft_reset(phydev);
  2835		if (ret)
  2836			return ret;
  2837	
> 2838		if (of_find_property(dev->of_node, "vsc8584,rgmii-skew-rx", NULL) ||
  2839		    of_find_property(dev->of_node, "vsc8584,rgmii-skew-tx", NULL)) {
  2840			of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-rx", &skew_rx);
  2841			of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-tx", &skew_tx);
  2842	
  2843			phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
  2844					 MSCC_PHY_RGMII_SETTINGS,
  2845					 (0x7 << RGMII_SKEW_RX_POS) |
  2846					 (0x7 << RGMII_SKEW_TX_POS),
  2847					 (skew_rx << RGMII_SKEW_RX_POS) |
  2848					 (skew_tx << RGMII_SKEW_TX_POS));
  2849		}
  2850	
  2851		for (i = 0; i < vsc8531->nleds; i++) {
  2852			ret = vsc85xx_led_cntl_set(phydev, i, vsc8531->leds_mode[i]);
  2853			if (ret)
  2854				return ret;
  2855		}
  2856	
  2857		return 0;
  2858	
  2859	err:
  2860		mutex_unlock(&phydev->mdio.bus->mdio_lock);
  2861		return ret;
  2862	}
  2863	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 53776 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration
  2020-02-27 15:28 ` [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration Antoine Tenart
                     ` (2 preceding siblings ...)
  2020-02-28 15:07   ` kbuild test robot
@ 2020-02-28 16:24   ` kbuild test robot
  3 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2020-02-28 16:24 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 4299 bytes --]

Hi Antoine,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on robh/for-next net/master linus/master v5.6-rc3 next-20200227]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Antoine-Tenart/net-phy-mscc-add-support-for-RGMII-MAC-mode/20200228-185429
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 2b99e54b30ed56201dedd91e6049ed83aa9d2302
config: arc-randconfig-a001-20200228 (attached as .config)
compiler: arc-elf-gcc (GCC) 9.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=9.2.0 make.cross ARCH=arc 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/firmware.h:6,
                    from drivers/net/phy/mscc.c:10:
   drivers/net/phy/mscc.c: In function 'vsc8584_config_init':
   drivers/net/phy/mscc.c:2838:23: error: 'dev' undeclared (first use in this function); did you mean 'cdev'?
    2838 |  if (of_find_property(dev->of_node, "vsc8584,rgmii-skew-rx", NULL) ||
         |                       ^~~
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
      58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
>> drivers/net/phy/mscc.c:2838:2: note: in expansion of macro 'if'
    2838 |  if (of_find_property(dev->of_node, "vsc8584,rgmii-skew-rx", NULL) ||
         |  ^~
   drivers/net/phy/mscc.c:2838:23: note: each undeclared identifier is reported only once for each function it appears in
    2838 |  if (of_find_property(dev->of_node, "vsc8584,rgmii-skew-rx", NULL) ||
         |                       ^~~
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
      58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
>> drivers/net/phy/mscc.c:2838:2: note: in expansion of macro 'if'
    2838 |  if (of_find_property(dev->of_node, "vsc8584,rgmii-skew-rx", NULL) ||
         |  ^~

vim +/if +2838 drivers/net/phy/mscc.c

  2825	
  2826		phy_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_STANDARD);
  2827	
  2828		val = phy_read(phydev, MSCC_PHY_EXT_PHY_CNTL_1);
  2829		val &= ~(MEDIA_OP_MODE_MASK | VSC8584_MAC_IF_SELECTION_MASK);
  2830		val |= (MEDIA_OP_MODE_COPPER << MEDIA_OP_MODE_POS) |
  2831		       (VSC8584_MAC_IF_SELECTION_SGMII << VSC8584_MAC_IF_SELECTION_POS);
  2832		ret = phy_write(phydev, MSCC_PHY_EXT_PHY_CNTL_1, val);
  2833	
  2834		ret = genphy_soft_reset(phydev);
  2835		if (ret)
  2836			return ret;
  2837	
> 2838		if (of_find_property(dev->of_node, "vsc8584,rgmii-skew-rx", NULL) ||
  2839		    of_find_property(dev->of_node, "vsc8584,rgmii-skew-tx", NULL)) {
  2840			of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-rx", &skew_rx);
  2841			of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-tx", &skew_tx);
  2842	
  2843			phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
  2844					 MSCC_PHY_RGMII_SETTINGS,
  2845					 (0x7 << RGMII_SKEW_RX_POS) |
  2846					 (0x7 << RGMII_SKEW_TX_POS),
  2847					 (skew_rx << RGMII_SKEW_RX_POS) |
  2848					 (skew_tx << RGMII_SKEW_TX_POS));
  2849		}
  2850	
  2851		for (i = 0; i < vsc8531->nleds; i++) {
  2852			ret = vsc85xx_led_cntl_set(phydev, i, vsc8531->leds_mode[i]);
  2853			if (ret)
  2854				return ret;
  2855		}
  2856	
  2857		return 0;
  2858	
  2859	err:
  2860		mutex_unlock(&phydev->mdio.bus->mdio_lock);
  2861		return ret;
  2862	}
  2863	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 29815 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-02-28 16:24 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-27 15:28 [PATCH net-next 0/3] net: phy: mscc: add support for RGMII MAC mode Antoine Tenart
2020-02-27 15:28 ` [PATCH net-next 1/3] " Antoine Tenart
2020-02-27 16:09   ` Quentin Schulz
2020-02-27 15:28 ` [PATCH net-next 2/3] dt-bindings: net: phy: mscc: document rgmii skew properties Antoine Tenart
2020-02-27 15:28 ` [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration Antoine Tenart
2020-02-27 15:51   ` Andrew Lunn
2020-02-27 16:01     ` Antoine Tenart
2020-02-27 16:21   ` Quentin Schulz
2020-02-27 16:25     ` Andrew Lunn
2020-02-27 18:53       ` Antoine Tenart
2020-02-27 18:55     ` Antoine Tenart
2020-02-28 15:07   ` kbuild test robot
2020-02-28 16:24   ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.