All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
@ 2020-04-22  7:21 Oleksij Rempel
  2020-04-22  8:48 ` Philippe Schenker
                   ` (5 more replies)
  0 siblings, 6 replies; 30+ messages in thread
From: Oleksij Rempel @ 2020-04-22  7:21 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Heiner Kallweit
  Cc: Oleksij Rempel, David Jander, David S. Miller, kernel,
	linux-kernel, netdev, Philippe Schenker, Russell King

Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid, rgmii-rxid.

This PHY has an internal RX delay of 1.2ns and no delay for TX.

The pad skew registers allow to set the total TX delay to max 1.38ns and
the total RX delay to max of 2.58ns (configurable 1.38ns + build in
1.2ns) and a minimal delay of 0ns.

According to the RGMII v1.3 specification the delay provided by PCB traces
should be between 1.5ns and 2.0ns. The RGMII v2.0 allows to provide this
delay by MAC or PHY. So, we configure this PHY to the best values we can
get by this HW: TX delay to 1.38ns (max supported value) and RX delay to
1.80ns (best calculated delay)

The phy-modes can still be fine tuned/overwritten by *-skew-ps
device tree properties described in:
Documentation/devicetree/bindings/net/micrel-ksz90x1.txt

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
changes v3:
- change delay on RX line to 1.80ns
- add warning if *-skew-ps properties are used together with not rgmii
  mode. 

changes v2:
- change RX_ID value from 0x1a to 0xa. The overflow bit was detected by
  FIELD_PREP() build check.
  Reported-by: kbuild test robot <lkp@intel.com>

 drivers/net/phy/micrel.c | 128 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 123 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 05d20343b8161..045783eb4bc70 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -19,6 +19,7 @@
  *			 ksz9477
  */
 
+#include <linux/bitfield.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/phy.h>
@@ -490,9 +491,50 @@ static int ksz9021_config_init(struct phy_device *phydev)
 
 /* MMD Address 0x2 */
 #define MII_KSZ9031RN_CONTROL_PAD_SKEW	4
+#define MII_KSZ9031RN_RX_CTL_M		GENMASK(7, 4)
+#define MII_KSZ9031RN_TX_CTL_M		GENMASK(3, 0)
+
 #define MII_KSZ9031RN_RX_DATA_PAD_SKEW	5
+#define MII_KSZ9031RN_RXD3		GENMASK(15, 12)
+#define MII_KSZ9031RN_RXD2		GENMASK(11, 8)
+#define MII_KSZ9031RN_RXD1		GENMASK(7, 4)
+#define MII_KSZ9031RN_RXD0		GENMASK(3, 0)
+
 #define MII_KSZ9031RN_TX_DATA_PAD_SKEW	6
+#define MII_KSZ9031RN_TXD3		GENMASK(15, 12)
+#define MII_KSZ9031RN_TXD2		GENMASK(11, 8)
+#define MII_KSZ9031RN_TXD1		GENMASK(7, 4)
+#define MII_KSZ9031RN_TXD0		GENMASK(3, 0)
+
 #define MII_KSZ9031RN_CLK_PAD_SKEW	8
+#define MII_KSZ9031RN_GTX_CLK		GENMASK(9, 5)
+#define MII_KSZ9031RN_RX_CLK		GENMASK(4, 0)
+
+/* KSZ9031 has internal RGMII_IDRX = 1.2ns and RGMII_IDTX = 0ns. To
+ * provide different RGMII options we need to configure delay offset
+ * for each pad relative to build in delay.
+ */
+/* keep rx as "No delay adjustment" and set rx_clk to +0.60ns to get delays of
+ * 1.80ns
+ */
+#define RX_ID				0x7
+#define RX_CLK_ID			0x19
+
+/* set rx to +0.30ns and rx_clk to -0.90ns to compensate the
+ * internal 1.2ns delay.
+ */
+#define RX_ND				0xc
+#define RX_CLK_ND			0x0
+
+/* set tx to -0.42ns and tx_clk to +0.96ns to get 1.38ns delay */
+#define TX_ID				0x0
+#define TX_CLK_ID			0x1f
+
+/* set tx and tx_clk to "No delay adjustment" to keep 0ns
+ * dealy
+ */
+#define TX_ND				0x7
+#define TX_CLK_ND			0xf
 
 /* MMD Address 0x1C */
 #define MII_KSZ9031RN_EDPD		0x23
@@ -501,7 +543,8 @@ static int ksz9021_config_init(struct phy_device *phydev)
 static int ksz9031_of_load_skew_values(struct phy_device *phydev,
 				       const struct device_node *of_node,
 				       u16 reg, size_t field_sz,
-				       const char *field[], u8 numfields)
+				       const char *field[], u8 numfields,
+				       bool *update)
 {
 	int val[4] = {-1, -2, -3, -4};
 	int matches = 0;
@@ -517,6 +560,8 @@ static int ksz9031_of_load_skew_values(struct phy_device *phydev,
 	if (!matches)
 		return 0;
 
+	*update |= true;
+
 	if (matches < numfields)
 		newval = phy_read_mmd(phydev, 2, reg);
 	else
@@ -565,6 +610,67 @@ static int ksz9031_enable_edpd(struct phy_device *phydev)
 			     reg | MII_KSZ9031RN_EDPD_ENABLE);
 }
 
+static int ksz9031_config_rgmii_delay(struct phy_device *phydev)
+{
+	u16 rx, tx, rx_clk, tx_clk;
+	int ret;
+
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		tx = TX_ND;
+		tx_clk = TX_CLK_ND;
+		rx = RX_ND;
+		rx_clk = RX_CLK_ND;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		tx = TX_ID;
+		tx_clk = TX_CLK_ID;
+		rx = RX_ID;
+		rx_clk = RX_CLK_ID;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		tx = TX_ND;
+		tx_clk = TX_CLK_ND;
+		rx = RX_ID;
+		rx_clk = RX_CLK_ID;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		tx = TX_ID;
+		tx_clk = TX_CLK_ID;
+		rx = RX_ND;
+		rx_clk = RX_CLK_ND;
+		break;
+	default:
+		return 0;
+	}
+
+	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_CONTROL_PAD_SKEW,
+			    FIELD_PREP(MII_KSZ9031RN_RX_CTL_M, rx) |
+			    FIELD_PREP(MII_KSZ9031RN_TX_CTL_M, tx));
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_RX_DATA_PAD_SKEW,
+			    FIELD_PREP(MII_KSZ9031RN_RXD3, rx) |
+			    FIELD_PREP(MII_KSZ9031RN_RXD2, rx) |
+			    FIELD_PREP(MII_KSZ9031RN_RXD1, rx) |
+			    FIELD_PREP(MII_KSZ9031RN_RXD0, rx));
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_TX_DATA_PAD_SKEW,
+			    FIELD_PREP(MII_KSZ9031RN_TXD3, tx) |
+			    FIELD_PREP(MII_KSZ9031RN_TXD2, tx) |
+			    FIELD_PREP(MII_KSZ9031RN_TXD1, tx) |
+			    FIELD_PREP(MII_KSZ9031RN_TXD0, tx));
+	if (ret < 0)
+		return ret;
+
+	return phy_write_mmd(phydev, 2, MII_KSZ9031RN_CLK_PAD_SKEW,
+			     FIELD_PREP(MII_KSZ9031RN_GTX_CLK, tx_clk) |
+			     FIELD_PREP(MII_KSZ9031RN_RX_CLK, rx_clk));
+}
+
 static int ksz9031_config_init(struct phy_device *phydev)
 {
 	const struct device *dev = &phydev->mdio.dev;
@@ -597,21 +703,33 @@ static int ksz9031_config_init(struct phy_device *phydev)
 	} while (!of_node && dev_walker);
 
 	if (of_node) {
+		bool update = false;
+
+		if (phy_interface_is_rgmii(phydev)) {
+			result = ksz9031_config_rgmii_delay(phydev);
+			if (result < 0)
+				return result;
+		}
+
 		ksz9031_of_load_skew_values(phydev, of_node,
 				MII_KSZ9031RN_CLK_PAD_SKEW, 5,
-				clk_skews, 2);
+				clk_skews, 2, &update);
 
 		ksz9031_of_load_skew_values(phydev, of_node,
 				MII_KSZ9031RN_CONTROL_PAD_SKEW, 4,
-				control_skews, 2);
+				control_skews, 2, &update);
 
 		ksz9031_of_load_skew_values(phydev, of_node,
 				MII_KSZ9031RN_RX_DATA_PAD_SKEW, 4,
-				rx_data_skews, 4);
+				rx_data_skews, 4, &update);
 
 		ksz9031_of_load_skew_values(phydev, of_node,
 				MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4,
-				tx_data_skews, 4);
+				tx_data_skews, 4, &update);
+
+		if (update && phydev->interface != PHY_INTERFACE_MODE_RGMII)
+			phydev_warn(phydev,
+				    "*-skew-ps values should be used only with phy-mode = \"rgmii\"\n");
 
 		/* Silicon Errata Sheet (DS80000691D or DS80000692D):
 		 * When the device links in the 1000BASE-T slave mode only,
-- 
2.26.0.rc2


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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-22  7:21 [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY Oleksij Rempel
@ 2020-04-22  8:48 ` Philippe Schenker
  2020-04-22 13:39 ` Andrew Lunn
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: Philippe Schenker @ 2020-04-22  8:48 UTC (permalink / raw)
  To: o.rempel, hkallweit1, f.fainelli, andrew
  Cc: linux, kernel, davem, david, netdev, linux-kernel

On Wed, 2020-04-22 at 09:21 +0200, Oleksij Rempel wrote:
> Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid,
> rgmii-rxid.
> 
> This PHY has an internal RX delay of 1.2ns and no delay for TX.
> 
> The pad skew registers allow to set the total TX delay to max 1.38ns
> and
> the total RX delay to max of 2.58ns (configurable 1.38ns + build in
> 1.2ns) and a minimal delay of 0ns.
> 
> According to the RGMII v1.3 specification the delay provided by PCB
> traces
> should be between 1.5ns and 2.0ns. The RGMII v2.0 allows to provide
> this
> delay by MAC or PHY. So, we configure this PHY to the best values we
> can
> get by this HW: TX delay to 1.38ns (max supported value) and RX delay
> to
> 1.80ns (best calculated delay)
> 
> The phy-modes can still be fine tuned/overwritten by *-skew-ps
> device tree properties described in:
> Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Philippe Schenker <philippe.schenker@toradex.com>

> ---
> changes v3:
> - change delay on RX line to 1.80ns
> - add warning if *-skew-ps properties are used together with not rgmii
>   mode. 
> 
> changes v2:
> - change RX_ID value from 0x1a to 0xa. The overflow bit was detected
> by
>   FIELD_PREP() build check.
>   Reported-by: kbuild test robot <lkp@intel.com>
> 
>  drivers/net/phy/micrel.c | 128 +++++++++++++++++++++++++++++++++++++-
> -
>  1 file changed, 123 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 05d20343b8161..045783eb4bc70 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -19,6 +19,7 @@
>   *			 ksz9477
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/phy.h>
> @@ -490,9 +491,50 @@ static int ksz9021_config_init(struct phy_device
> *phydev)
>  
>  /* MMD Address 0x2 */
>  #define MII_KSZ9031RN_CONTROL_PAD_SKEW	4
> +#define MII_KSZ9031RN_RX_CTL_M		GENMASK(7, 4)
> +#define MII_KSZ9031RN_TX_CTL_M		GENMASK(3, 0)
> +
>  #define MII_KSZ9031RN_RX_DATA_PAD_SKEW	5
> +#define MII_KSZ9031RN_RXD3		GENMASK(15, 12)
> +#define MII_KSZ9031RN_RXD2		GENMASK(11, 8)
> +#define MII_KSZ9031RN_RXD1		GENMASK(7, 4)
> +#define MII_KSZ9031RN_RXD0		GENMASK(3, 0)
> +
>  #define MII_KSZ9031RN_TX_DATA_PAD_SKEW	6
> +#define MII_KSZ9031RN_TXD3		GENMASK(15, 12)
> +#define MII_KSZ9031RN_TXD2		GENMASK(11, 8)
> +#define MII_KSZ9031RN_TXD1		GENMASK(7, 4)
> +#define MII_KSZ9031RN_TXD0		GENMASK(3, 0)
> +
>  #define MII_KSZ9031RN_CLK_PAD_SKEW	8
> +#define MII_KSZ9031RN_GTX_CLK		GENMASK(9, 5)
> +#define MII_KSZ9031RN_RX_CLK		GENMASK(4, 0)
> +
> +/* KSZ9031 has internal RGMII_IDRX = 1.2ns and RGMII_IDTX = 0ns. To
> + * provide different RGMII options we need to configure delay offset
> + * for each pad relative to build in delay.
> + */
> +/* keep rx as "No delay adjustment" and set rx_clk to +0.60ns to get
> delays of
> + * 1.80ns
> + */
> +#define RX_ID				0x7
> +#define RX_CLK_ID			0x19
> +
> +/* set rx to +0.30ns and rx_clk to -0.90ns to compensate the
> + * internal 1.2ns delay.
> + */
> +#define RX_ND				0xc
> +#define RX_CLK_ND			0x0
> +
> +/* set tx to -0.42ns and tx_clk to +0.96ns to get 1.38ns delay */
> +#define TX_ID				0x0
> +#define TX_CLK_ID			0x1f
> +
> +/* set tx and tx_clk to "No delay adjustment" to keep 0ns
> + * dealy
> + */
> +#define TX_ND				0x7
> +#define TX_CLK_ND			0xf
>  
>  /* MMD Address 0x1C */
>  #define MII_KSZ9031RN_EDPD		0x23
> @@ -501,7 +543,8 @@ static int ksz9021_config_init(struct phy_device
> *phydev)
>  static int ksz9031_of_load_skew_values(struct phy_device *phydev,
>  				       const struct device_node
> *of_node,
>  				       u16 reg, size_t field_sz,
> -				       const char *field[], u8
> numfields)
> +				       const char *field[], u8
> numfields,
> +				       bool *update)
>  {
>  	int val[4] = {-1, -2, -3, -4};
>  	int matches = 0;
> @@ -517,6 +560,8 @@ static int ksz9031_of_load_skew_values(struct
> phy_device *phydev,
>  	if (!matches)
>  		return 0;
>  
> +	*update |= true;
> +
>  	if (matches < numfields)
>  		newval = phy_read_mmd(phydev, 2, reg);
>  	else
> @@ -565,6 +610,67 @@ static int ksz9031_enable_edpd(struct phy_device
> *phydev)
>  			     reg | MII_KSZ9031RN_EDPD_ENABLE);
>  }
>  
> +static int ksz9031_config_rgmii_delay(struct phy_device *phydev)
> +{
> +	u16 rx, tx, rx_clk, tx_clk;
> +	int ret;
> +
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		tx = TX_ND;
> +		tx_clk = TX_CLK_ND;
> +		rx = RX_ND;
> +		rx_clk = RX_CLK_ND;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		tx = TX_ID;
> +		tx_clk = TX_CLK_ID;
> +		rx = RX_ID;
> +		rx_clk = RX_CLK_ID;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		tx = TX_ND;
> +		tx_clk = TX_CLK_ND;
> +		rx = RX_ID;
> +		rx_clk = RX_CLK_ID;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		tx = TX_ID;
> +		tx_clk = TX_CLK_ID;
> +		rx = RX_ND;
> +		rx_clk = RX_CLK_ND;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_CONTROL_PAD_SKEW,
> +			    FIELD_PREP(MII_KSZ9031RN_RX_CTL_M, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TX_CTL_M, tx));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_RX_DATA_PAD_SKEW,
> +			    FIELD_PREP(MII_KSZ9031RN_RXD3, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_RXD2, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_RXD1, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_RXD0, rx));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_TX_DATA_PAD_SKEW,
> +			    FIELD_PREP(MII_KSZ9031RN_TXD3, tx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TXD2, tx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TXD1, tx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TXD0, tx));
> +	if (ret < 0)
> +		return ret;
> +
> +	return phy_write_mmd(phydev, 2, MII_KSZ9031RN_CLK_PAD_SKEW,
> +			     FIELD_PREP(MII_KSZ9031RN_GTX_CLK, tx_clk) |
> +			     FIELD_PREP(MII_KSZ9031RN_RX_CLK, rx_clk));
> +}
> +
>  static int ksz9031_config_init(struct phy_device *phydev)
>  {
>  	const struct device *dev = &phydev->mdio.dev;
> @@ -597,21 +703,33 @@ static int ksz9031_config_init(struct phy_device
> *phydev)
>  	} while (!of_node && dev_walker);
>  
>  	if (of_node) {
> +		bool update = false;
> +
> +		if (phy_interface_is_rgmii(phydev)) {
> +			result = ksz9031_config_rgmii_delay(phydev);
> +			if (result < 0)
> +				return result;
> +		}
> +
>  		ksz9031_of_load_skew_values(phydev, of_node,
>  				MII_KSZ9031RN_CLK_PAD_SKEW, 5,
> -				clk_skews, 2);
> +				clk_skews, 2, &update);
>  
>  		ksz9031_of_load_skew_values(phydev, of_node,
>  				MII_KSZ9031RN_CONTROL_PAD_SKEW, 4,
> -				control_skews, 2);
> +				control_skews, 2, &update);
>  
>  		ksz9031_of_load_skew_values(phydev, of_node,
>  				MII_KSZ9031RN_RX_DATA_PAD_SKEW, 4,
> -				rx_data_skews, 4);
> +				rx_data_skews, 4, &update);
>  
>  		ksz9031_of_load_skew_values(phydev, of_node,
>  				MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4,
> -				tx_data_skews, 4);
> +				tx_data_skews, 4, &update);
> +
> +		if (update && phydev->interface !=
> PHY_INTERFACE_MODE_RGMII)
> +			phydev_warn(phydev,
> +				    "*-skew-ps values should be used
> only with phy-mode = \"rgmii\"\n");
>  
>  		/* Silicon Errata Sheet (DS80000691D or DS80000692D):
>  		 * When the device links in the 1000BASE-T slave mode
> only,

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-22  7:21 [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY Oleksij Rempel
  2020-04-22  8:48 ` Philippe Schenker
@ 2020-04-22 13:39 ` Andrew Lunn
  2020-04-23  2:39 ` David Miller
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: Andrew Lunn @ 2020-04-22 13:39 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Florian Fainelli, Heiner Kallweit, David Jander, David S. Miller,
	kernel, linux-kernel, netdev, Philippe Schenker, Russell King

On Wed, Apr 22, 2020 at 09:21:37AM +0200, Oleksij Rempel wrote:
> Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid, rgmii-rxid.
> 
> This PHY has an internal RX delay of 1.2ns and no delay for TX.
> 
> The pad skew registers allow to set the total TX delay to max 1.38ns and
> the total RX delay to max of 2.58ns (configurable 1.38ns + build in
> 1.2ns) and a minimal delay of 0ns.
> 
> According to the RGMII v1.3 specification the delay provided by PCB traces
> should be between 1.5ns and 2.0ns. The RGMII v2.0 allows to provide this
> delay by MAC or PHY. So, we configure this PHY to the best values we can
> get by this HW: TX delay to 1.38ns (max supported value) and RX delay to
> 1.80ns (best calculated delay)
> 
> The phy-modes can still be fine tuned/overwritten by *-skew-ps
> device tree properties described in:
> Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

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

    Andrew

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-22  7:21 [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY Oleksij Rempel
  2020-04-22  8:48 ` Philippe Schenker
  2020-04-22 13:39 ` Andrew Lunn
@ 2020-04-23  2:39 ` David Miller
  2020-04-28 15:28 ` Geert Uytterhoeven
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: David Miller @ 2020-04-23  2:39 UTC (permalink / raw)
  To: o.rempel
  Cc: andrew, f.fainelli, hkallweit1, david, kernel, linux-kernel,
	netdev, philippe.schenker, linux

From: Oleksij Rempel <o.rempel@pengutronix.de>
Date: Wed, 22 Apr 2020 09:21:37 +0200

> Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid, rgmii-rxid.
> 
> This PHY has an internal RX delay of 1.2ns and no delay for TX.
> 
> The pad skew registers allow to set the total TX delay to max 1.38ns and
> the total RX delay to max of 2.58ns (configurable 1.38ns + build in
> 1.2ns) and a minimal delay of 0ns.
> 
> According to the RGMII v1.3 specification the delay provided by PCB traces
> should be between 1.5ns and 2.0ns. The RGMII v2.0 allows to provide this
> delay by MAC or PHY. So, we configure this PHY to the best values we can
> get by this HW: TX delay to 1.38ns (max supported value) and RX delay to
> 1.80ns (best calculated delay)
> 
> The phy-modes can still be fine tuned/overwritten by *-skew-ps
> device tree properties described in:
> Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Applied, thank you.

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-22  7:21 [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY Oleksij Rempel
                   ` (2 preceding siblings ...)
  2020-04-23  2:39 ` David Miller
@ 2020-04-28 15:28 ` Geert Uytterhoeven
  2020-04-28 15:47   ` Andrew Lunn
  2020-05-05 18:26 ` Grygorii Strashko
  2020-07-10 22:36 ` Alexandre Belloni
  5 siblings, 1 reply; 30+ messages in thread
From: Geert Uytterhoeven @ 2020-04-28 15:28 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David Jander,
	David S. Miller, Sascha Hauer, Linux Kernel Mailing List, netdev,
	Philippe Schenker, Russell King, Sergei Shtylyov, Linux-Renesas

Hi Oleksij,

On Wed, Apr 22, 2020 at 9:24 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid, rgmii-rxid.
>
> This PHY has an internal RX delay of 1.2ns and no delay for TX.
>
> The pad skew registers allow to set the total TX delay to max 1.38ns and
> the total RX delay to max of 2.58ns (configurable 1.38ns + build in
> 1.2ns) and a minimal delay of 0ns.
>
> According to the RGMII v1.3 specification the delay provided by PCB traces
> should be between 1.5ns and 2.0ns. The RGMII v2.0 allows to provide this
> delay by MAC or PHY. So, we configure this PHY to the best values we can
> get by this HW: TX delay to 1.38ns (max supported value) and RX delay to
> 1.80ns (best calculated delay)
>
> The phy-modes can still be fine tuned/overwritten by *-skew-ps
> device tree properties described in:
> Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

This is now commit bcf3440c6dd78bfe ("net: phy: micrel: add phy-mode
support for the KSZ9031 PHY") in net-next/master.

> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c

> @@ -597,21 +703,33 @@ static int ksz9031_config_init(struct phy_device *phydev)
>         } while (!of_node && dev_walker);
>
>         if (of_node) {
> +               bool update = false;
> +
> +               if (phy_interface_is_rgmii(phydev)) {
> +                       result = ksz9031_config_rgmii_delay(phydev);
> +                       if (result < 0)
> +                               return result;
> +               }
> +
>                 ksz9031_of_load_skew_values(phydev, of_node,
>                                 MII_KSZ9031RN_CLK_PAD_SKEW, 5,
> -                               clk_skews, 2);
> +                               clk_skews, 2, &update);
>
>                 ksz9031_of_load_skew_values(phydev, of_node,
>                                 MII_KSZ9031RN_CONTROL_PAD_SKEW, 4,
> -                               control_skews, 2);
> +                               control_skews, 2, &update);
>
>                 ksz9031_of_load_skew_values(phydev, of_node,
>                                 MII_KSZ9031RN_RX_DATA_PAD_SKEW, 4,
> -                               rx_data_skews, 4);
> +                               rx_data_skews, 4, &update);
>
>                 ksz9031_of_load_skew_values(phydev, of_node,
>                                 MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4,
> -                               tx_data_skews, 4);
> +                               tx_data_skews, 4, &update);
> +
> +               if (update && phydev->interface != PHY_INTERFACE_MODE_RGMII)
> +                       phydev_warn(phydev,
> +                                   "*-skew-ps values should be used only with phy-mode = \"rgmii\"\n");

This triggers on Renesas Salvator-X(S):

    Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00:
*-skew-ps values should be used only with phy-mode = "rgmii"

which uses:

        phy-mode = "rgmii-txid";

and:

        rxc-skew-ps = <1500>;

If I understand Documentation/devicetree/bindings/net/ethernet-controller.yaml
correctly:

      # RX and TX delays are added by the MAC when required
      - rgmii

i.e. any *-skew-ps can be specified.

      # RGMII with internal RX and TX delays provided by the PHY,
      # the MAC should not add the RX or TX delays in this case
      - rgmii-id

i.e. *-skew-ps must not be specified.

      # RGMII with internal RX delay provided by the PHY, the MAC
      # should not add an RX delay in this case
      - rgmii-rxid

i.e. it's still OK to specify tx*-skew-ps values here...

      # RGMII with internal TX delay provided by the PHY, the MAC
      # should not add an TX delay in this case
      - rgmii-txid

... and rx*-skew-ps values here?
Is my understanding correct, and should the check be updated to take into
account rxid and txid?

BTW, the example in Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
still specifies *-skew-ps values with phy-mode = "rgmii-id", so I think
that should be updated, too.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-28 15:28 ` Geert Uytterhoeven
@ 2020-04-28 15:47   ` Andrew Lunn
  2020-04-28 16:16     ` Philippe Schenker
  0 siblings, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2020-04-28 15:47 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Oleksij Rempel, Florian Fainelli, Heiner Kallweit, David Jander,
	David S. Miller, Sascha Hauer, Linux Kernel Mailing List, netdev,
	Philippe Schenker, Russell King, Sergei Shtylyov, Linux-Renesas

On Tue, Apr 28, 2020 at 05:28:30PM +0200, Geert Uytterhoeven wrote:
> This triggers on Renesas Salvator-X(S):
> 
>     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00:
> *-skew-ps values should be used only with phy-mode = "rgmii"
> 
> which uses:
> 
>         phy-mode = "rgmii-txid";
> 
> and:
> 
>         rxc-skew-ps = <1500>;
> 
> If I understand Documentation/devicetree/bindings/net/ethernet-controller.yaml
> correctly:

Hi Geert

Checking for skews which might contradict the PHY-mode is new. I think
this is the first PHY driver to do it. So i'm not too surprised it has
triggered a warning, or there is contradictory documentation.

Your use cases is reasonable. Have the normal transmit delay, and a
bit shorted receive delay. So we should allow it. It just makes the
validation code more complex :-(

	   Andrew

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-28 15:47   ` Andrew Lunn
@ 2020-04-28 16:16     ` Philippe Schenker
  2020-04-29  8:45       ` Geert Uytterhoeven
  0 siblings, 1 reply; 30+ messages in thread
From: Philippe Schenker @ 2020-04-28 16:16 UTC (permalink / raw)
  To: andrew, geert
  Cc: sergei.shtylyov, kernel, davem, hkallweit1, linux, linux-kernel,
	david, netdev, o.rempel, f.fainelli, linux-renesas-soc

On Tue, 2020-04-28 at 17:47 +0200, Andrew Lunn wrote:
> On Tue, Apr 28, 2020 at 05:28:30PM +0200, Geert Uytterhoeven wrote:
> > This triggers on Renesas Salvator-X(S):
> > 
> >     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00:
> > *-skew-ps values should be used only with phy-mode = "rgmii"
> > 
> > which uses:
> > 
> >         phy-mode = "rgmii-txid";
> > 
> > and:
> > 
> >         rxc-skew-ps = <1500>;
> > 
> > If I understand Documentation/devicetree/bindings/net/ethernet-
> > controller.yaml
> > correctly:
> 
> Hi Geert
> 
> Checking for skews which might contradict the PHY-mode is new. I think
> this is the first PHY driver to do it. So i'm not too surprised it has
> triggered a warning, or there is contradictory documentation.
> 
> Your use cases is reasonable. Have the normal transmit delay, and a
> bit shorted receive delay. So we should allow it. It just makes the
> validation code more complex :-(
> 
> 	   Andrew

Hello Geert and Andrew

I reviewed Oleksij's patch that introduced this warning. I just want to
explain our thinking why this is a good thing, but yes maybe we change
that warning a little bit until it lands in mainline.

The KSZ9031 driver didn't support for proper phy-modes until now as it
don't have dedicated registers to control tx and rx delays. With
Oleksij's patch this delay is now done accordingly in skew registers as
best as possible. If you now also set the rxc-skew-ps registers those
values you previously set with rgmii-txid or rxid get overwritten.

We chose the warning to occur on phy-modes 'rgmii-id', 'rgmii-rxid' and
'rgmii-txid' as on those, with the 'rxc-skew-ps' value present,
overwriting skew values could occur and you end up with values you do
not wanted. We thought, that most of the boards have just 'rgmii' set in
phy-mode with specific skew-values present.

@Geert if you actually want the PHY to apply RXC and TXC delays just
insert 'rgmii-id' in your DT and remove those *-skew-ps values. If you
need custom timing due to PCB routing it was thought out to use the phy-
mode 'rgmii' and do the whole required timing with the *-skew-ps values.

@Andrew This warning might be not the best solution but we should
definitely warn that values might get overwritten from what was intended
with e.g. 'rgmii-txid'. The out-of-reset behaviour of the PHY actually
is 'rgmii-txid' so we may also throw now warning if this mode is set.
What is your oppinion?

Regards,
Philippe

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-28 16:16     ` Philippe Schenker
@ 2020-04-29  8:45       ` Geert Uytterhoeven
  2020-04-29  9:26         ` Oleksij Rempel
  2020-04-29 10:02         ` Philippe Schenker
  0 siblings, 2 replies; 30+ messages in thread
From: Geert Uytterhoeven @ 2020-04-29  8:45 UTC (permalink / raw)
  To: Philippe Schenker
  Cc: andrew, sergei.shtylyov, kernel, davem, hkallweit1, linux,
	linux-kernel, david, netdev, o.rempel, f.fainelli,
	linux-renesas-soc, Kazuya Mizuguchi

Hi Philippe,

On Tue, Apr 28, 2020 at 6:16 PM Philippe Schenker
<philippe.schenker@toradex.com> wrote:
> On Tue, 2020-04-28 at 17:47 +0200, Andrew Lunn wrote:
> > On Tue, Apr 28, 2020 at 05:28:30PM +0200, Geert Uytterhoeven wrote:
> > > This triggers on Renesas Salvator-X(S):
> > >
> > >     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00:
> > > *-skew-ps values should be used only with phy-mode = "rgmii"
> > >
> > > which uses:
> > >
> > >         phy-mode = "rgmii-txid";
> > >
> > > and:
> > >
> > >         rxc-skew-ps = <1500>;
> > >
> > > If I understand Documentation/devicetree/bindings/net/ethernet-
> > > controller.yaml
> > > correctly:
> >
> > Checking for skews which might contradict the PHY-mode is new. I think
> > this is the first PHY driver to do it. So i'm not too surprised it has
> > triggered a warning, or there is contradictory documentation.
> >
> > Your use cases is reasonable. Have the normal transmit delay, and a
> > bit shorted receive delay. So we should allow it. It just makes the
> > validation code more complex :-(
>
> I reviewed Oleksij's patch that introduced this warning. I just want to
> explain our thinking why this is a good thing, but yes maybe we change
> that warning a little bit until it lands in mainline.
>
> The KSZ9031 driver didn't support for proper phy-modes until now as it
> don't have dedicated registers to control tx and rx delays. With
> Oleksij's patch this delay is now done accordingly in skew registers as
> best as possible. If you now also set the rxc-skew-ps registers those
> values you previously set with rgmii-txid or rxid get overwritten.
>
> We chose the warning to occur on phy-modes 'rgmii-id', 'rgmii-rxid' and
> 'rgmii-txid' as on those, with the 'rxc-skew-ps' value present,
> overwriting skew values could occur and you end up with values you do
> not wanted. We thought, that most of the boards have just 'rgmii' set in
> phy-mode with specific skew-values present.
>
> @Geert if you actually want the PHY to apply RXC and TXC delays just
> insert 'rgmii-id' in your DT and remove those *-skew-ps values. If you

That seems to work for me, but of course doesn't take into account PCB
routing.

> need custom timing due to PCB routing it was thought out to use the phy-
> mode 'rgmii' and do the whole required timing with the *-skew-ps values.

That mean we do have to provide all values again?
Using "rgmii" without any skew values makes DHCP fail on R-Car H3 ES2.0,
M3-W (ES1.0), and M3-N (ES1.0). Interestingly, DHCP still works on R-Car
H3 ES1.0.

Note that I'm not too-familiar with the actual skew values needed
(CC Mizuguchi-san).

Related commits:
  - 0e45da1c6ea6b186 ("arm64: dts: r8a7795: salvator-x: Fix
EthernetAVB PHY timing")
  - dda3887907d74338 ("arm64: dts: r8a7795: Use rgmii-txid phy-mode
for EthernetAVB")
  - 7eda14afb8843a0d ("arm64: dts: renesas: r8a77990: ebisu: Fix
EthernetAVB phy mode to rgmii")

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-29  8:45       ` Geert Uytterhoeven
@ 2020-04-29  9:26         ` Oleksij Rempel
  2020-05-27 19:11           ` Geert Uytterhoeven
  2020-04-29 10:02         ` Philippe Schenker
  1 sibling, 1 reply; 30+ messages in thread
From: Oleksij Rempel @ 2020-04-29  9:26 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Philippe Schenker, andrew, sergei.shtylyov, kernel, davem,
	hkallweit1, linux, linux-kernel, david, netdev, f.fainelli,
	linux-renesas-soc, Kazuya Mizuguchi

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

Hi Geert,

On Wed, Apr 29, 2020 at 10:45:35AM +0200, Geert Uytterhoeven wrote:
> Hi Philippe,
> 
> On Tue, Apr 28, 2020 at 6:16 PM Philippe Schenker
> <philippe.schenker@toradex.com> wrote:
> > On Tue, 2020-04-28 at 17:47 +0200, Andrew Lunn wrote:
> > > On Tue, Apr 28, 2020 at 05:28:30PM +0200, Geert Uytterhoeven wrote:
> > > > This triggers on Renesas Salvator-X(S):
> > > >
> > > >     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00:
> > > > *-skew-ps values should be used only with phy-mode = "rgmii"
> > > >
> > > > which uses:
> > > >
> > > >         phy-mode = "rgmii-txid";
> > > >
> > > > and:
> > > >
> > > >         rxc-skew-ps = <1500>;
> > > >
> > > > If I understand Documentation/devicetree/bindings/net/ethernet-
> > > > controller.yaml
> > > > correctly:
> > >
> > > Checking for skews which might contradict the PHY-mode is new. I think
> > > this is the first PHY driver to do it. So i'm not too surprised it has
> > > triggered a warning, or there is contradictory documentation.
> > >
> > > Your use cases is reasonable. Have the normal transmit delay, and a
> > > bit shorted receive delay. So we should allow it. It just makes the
> > > validation code more complex :-(
> >
> > I reviewed Oleksij's patch that introduced this warning. I just want to
> > explain our thinking why this is a good thing, but yes maybe we change
> > that warning a little bit until it lands in mainline.
> >
> > The KSZ9031 driver didn't support for proper phy-modes until now as it
> > don't have dedicated registers to control tx and rx delays. With
> > Oleksij's patch this delay is now done accordingly in skew registers as
> > best as possible. If you now also set the rxc-skew-ps registers those
> > values you previously set with rgmii-txid or rxid get overwritten.
> >
> > We chose the warning to occur on phy-modes 'rgmii-id', 'rgmii-rxid' and
> > 'rgmii-txid' as on those, with the 'rxc-skew-ps' value present,
> > overwriting skew values could occur and you end up with values you do
> > not wanted. We thought, that most of the boards have just 'rgmii' set in
> > phy-mode with specific skew-values present.
> >
> > @Geert if you actually want the PHY to apply RXC and TXC delays just
> > insert 'rgmii-id' in your DT and remove those *-skew-ps values. If you
> 
> That seems to work for me, but of course doesn't take into account PCB
> routing.

On boards with simple design, the clock lines have nearly same length as data
lines. To provide needed clock delay, you should make clock line ~17
centimeter longer than data lines. Or configure PHY or MAC side to
provide needed delay.
Since "phy-mode = "rgmii-txid"" was ignored till my patch. And the
"rxc-skew-ps = <1500>" will add a delay of 0.6 nano seconds. Your
configuration was:
TX delay = 1.2ns
RX delay = 0.6ns

Is it really reflects the configuration of you PCB?

> > need custom timing due to PCB routing it was thought out to use the phy-
> > mode 'rgmii' and do the whole required timing with the *-skew-ps values.
> 
> That mean we do have to provide all values again?

No. Using proper phy-mode should be enough. If you using default TX dealy and
configuring RX delay manually, the phy-mode = "rgmii-id" is
the right choice for you.

> Using "rgmii" without any skew values makes DHCP fail on R-Car H3 ES2.0,
> M3-W (ES1.0), and M3-N (ES1.0). Interestingly, DHCP still works on R-Car
> H3 ES1.0.

The TX delay affects MAC to PHY path. The RX delay affects PHY to MAC
path. On my HW, disabling TX delays, didn't affected the communication
in any measurable way. Even with clock line length is equal to the data
lines length. So, it may work just on the edge of the spec.

> Note that I'm not too-familiar with the actual skew values needed
> (CC Mizuguchi-san).
> 
> Related commits:
>   - 0e45da1c6ea6b186 ("arm64: dts: r8a7795: salvator-x: Fix
> EthernetAVB PHY timing")
>   - dda3887907d74338 ("arm64: dts: r8a7795: Use rgmii-txid phy-mode
> for EthernetAVB")
>   - 7eda14afb8843a0d ("arm64: dts: renesas: r8a77990: ebisu: Fix
> EthernetAVB phy mode to rgmii")

Regards,
Oleksij.
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-29  8:45       ` Geert Uytterhoeven
  2020-04-29  9:26         ` Oleksij Rempel
@ 2020-04-29 10:02         ` Philippe Schenker
  1 sibling, 0 replies; 30+ messages in thread
From: Philippe Schenker @ 2020-04-29 10:02 UTC (permalink / raw)
  To: geert
  Cc: andrew, linux, kernel, hkallweit1, davem, linux-kernel,
	sergei.shtylyov, netdev, david, f.fainelli, linux-renesas-soc,
	kazuya.mizuguchi.ks

On Wed, 2020-04-29 at 10:45 +0200, Geert Uytterhoeven wrote:
> Hi Philippe,
> 
> On Tue, Apr 28, 2020 at 6:16 PM Philippe Schenker
> <philippe.schenker@toradex.com> wrote:
> > On Tue, 2020-04-28 at 17:47 +0200, Andrew Lunn wrote:
> > > On Tue, Apr 28, 2020 at 05:28:30PM +0200, Geert Uytterhoeven
> > > wrote:
> > > > This triggers on Renesas Salvator-X(S):
> > > > 
> > > >     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00:
> > > > *-skew-ps values should be used only with phy-mode = "rgmii"
> > > > 
> > > > which uses:
> > > > 
> > > >         phy-mode = "rgmii-txid";
> > > > 
> > > > and:
> > > > 
> > > >         rxc-skew-ps = <1500>;
> > > > 
> > > > If I understand Documentation/devicetree/bindings/net/ethernet-
> > > > controller.yaml
> > > > correctly:
> > > 
> > > Checking for skews which might contradict the PHY-mode is new. I
> > > think
> > > this is the first PHY driver to do it. So i'm not too surprised it
> > > has
> > > triggered a warning, or there is contradictory documentation.
> > > 
> > > Your use cases is reasonable. Have the normal transmit delay, and
> > > a
> > > bit shorted receive delay. So we should allow it. It just makes
> > > the
> > > validation code more complex :-(
> > 
> > I reviewed Oleksij's patch that introduced this warning. I just want
> > to
> > explain our thinking why this is a good thing, but yes maybe we
> > change
> > that warning a little bit until it lands in mainline.
> > 
> > The KSZ9031 driver didn't support for proper phy-modes until now as
> > it
> > don't have dedicated registers to control tx and rx delays. With
> > Oleksij's patch this delay is now done accordingly in skew registers
> > as
> > best as possible. If you now also set the rxc-skew-ps registers
> > those
> > values you previously set with rgmii-txid or rxid get overwritten.
> > 
> > We chose the warning to occur on phy-modes 'rgmii-id', 'rgmii-rxid'
> > and
> > 'rgmii-txid' as on those, with the 'rxc-skew-ps' value present,
> > overwriting skew values could occur and you end up with values you
> > do
> > not wanted. We thought, that most of the boards have just 'rgmii'
> > set in
> > phy-mode with specific skew-values present.
> > 
> > @Geert if you actually want the PHY to apply RXC and TXC delays just
> > insert 'rgmii-id' in your DT and remove those *-skew-ps values. If
> > you
> 
> That seems to work for me, but of course doesn't take into account PCB
> routing.
> 
> > need custom timing due to PCB routing it was thought out to use the
> > phy-
> > mode 'rgmii' and do the whole required timing with the *-skew-ps
> > values.
> 
> That mean we do have to provide all values again?

In the case that you have not length-matched rgmii signals on the PCB I
would advise you to check the skew settings closely. Otherwise you might
end up with values that work on the border and may fail on the full
temperature-range.

If the length is not off by huge amounts, rgmii-id
should work fine.

> Using "rgmii" without any skew values makes DHCP fail on R-Car H3
> ES2.0,

That sounds like the R-Car H3 ES2.0 is not providing a RXC delay.

> M3-W (ES1.0), and M3-N (ES1.0). Interestingly, DHCP still works on R-
> Car
> H3 ES1.0.
> 
> Note that I'm not too-familiar with the actual skew values needed
> (CC Mizuguchi-san).
> 
> Related commits:
>   - 0e45da1c6ea6b186 ("arm64: dts: r8a7795: salvator-x: Fix
> EthernetAVB PHY timing")
>   - dda3887907d74338 ("arm64: dts: r8a7795: Use rgmii-txid phy-mode
> for EthernetAVB")
>   - 7eda14afb8843a0d ("arm64: dts: renesas: r8a77990: ebisu: Fix
> EthernetAVB phy mode to rgmii")
> 
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a
> hacker. But
> when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-22  7:21 [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY Oleksij Rempel
                   ` (3 preceding siblings ...)
  2020-04-28 15:28 ` Geert Uytterhoeven
@ 2020-05-05 18:26 ` Grygorii Strashko
  2020-05-06  4:51   ` Oleksij Rempel
  2020-07-10 22:36 ` Alexandre Belloni
  5 siblings, 1 reply; 30+ messages in thread
From: Grygorii Strashko @ 2020-05-05 18:26 UTC (permalink / raw)
  To: Oleksij Rempel, Andrew Lunn, Florian Fainelli, Heiner Kallweit
  Cc: David Jander, David S. Miller, kernel, linux-kernel, netdev,
	Philippe Schenker, Russell King



On 22/04/2020 10:21, Oleksij Rempel wrote:
> Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid, rgmii-rxid.
> 
> This PHY has an internal RX delay of 1.2ns and no delay for TX.
> 
> The pad skew registers allow to set the total TX delay to max 1.38ns and
> the total RX delay to max of 2.58ns (configurable 1.38ns + build in
> 1.2ns) and a minimal delay of 0ns.
> 
> According to the RGMII v1.3 specification the delay provided by PCB traces
> should be between 1.5ns and 2.0ns. The RGMII v2.0 allows to provide this
> delay by MAC or PHY. So, we configure this PHY to the best values we can
> get by this HW: TX delay to 1.38ns (max supported value) and RX delay to
> 1.80ns (best calculated delay)
> 
> The phy-modes can still be fine tuned/overwritten by *-skew-ps
> device tree properties described in:
> Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> changes v3:
> - change delay on RX line to 1.80ns
> - add warning if *-skew-ps properties are used together with not rgmii
>    mode.
> 
> changes v2:
> - change RX_ID value from 0x1a to 0xa. The overflow bit was detected by
>    FIELD_PREP() build check.
>    Reported-by: kbuild test robot <lkp@intel.com>
> 
>   drivers/net/phy/micrel.c | 128 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 123 insertions(+), 5 deletions(-)
> 

This patch broke networking on at least 5 TI boards:
am572x-idk
am571x-idk
am43xx-hsevm
am43xx-gpevm
am437x-idk

am57xx I can fix.

am437x need to investigate.

-- 
Best regards,
grygorii

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-05-05 18:26 ` Grygorii Strashko
@ 2020-05-06  4:51   ` Oleksij Rempel
  0 siblings, 0 replies; 30+ messages in thread
From: Oleksij Rempel @ 2020-05-06  4:51 UTC (permalink / raw)
  To: Grygorii Strashko
  Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David Jander,
	David S. Miller, kernel, linux-kernel, netdev, Philippe Schenker,
	Russell King

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

Hi Grygorii,

On Tue, May 05, 2020 at 09:26:46PM +0300, Grygorii Strashko wrote:
> 
> 
> On 22/04/2020 10:21, Oleksij Rempel wrote:
> > Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid, rgmii-rxid.
> > 
> > This PHY has an internal RX delay of 1.2ns and no delay for TX.
> > 
> > The pad skew registers allow to set the total TX delay to max 1.38ns and
> > the total RX delay to max of 2.58ns (configurable 1.38ns + build in
> > 1.2ns) and a minimal delay of 0ns.
> > 
> > According to the RGMII v1.3 specification the delay provided by PCB traces
> > should be between 1.5ns and 2.0ns. The RGMII v2.0 allows to provide this
> > delay by MAC or PHY. So, we configure this PHY to the best values we can
> > get by this HW: TX delay to 1.38ns (max supported value) and RX delay to
> > 1.80ns (best calculated delay)
> > 
> > The phy-modes can still be fine tuned/overwritten by *-skew-ps
> > device tree properties described in:
> > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> > changes v3:
> > - change delay on RX line to 1.80ns
> > - add warning if *-skew-ps properties are used together with not rgmii
> >    mode.
> > 
> > changes v2:
> > - change RX_ID value from 0x1a to 0xa. The overflow bit was detected by
> >    FIELD_PREP() build check.
> >    Reported-by: kbuild test robot <lkp@intel.com>
> > 
> >   drivers/net/phy/micrel.c | 128 +++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 123 insertions(+), 5 deletions(-)
> > 
> 
> This patch broke networking on at least 5 TI boards:
> am572x-idk
> am571x-idk
> am43xx-hsevm
> am43xx-gpevm
> am437x-idk
> 
> am57xx I can fix.
> 
> am437x need to investigate.

Please try:
	phy-mode = "rgmii-txid"

I assume your boards are using "rgmii" which should be used only if you
boards have extra long clk traces.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-29  9:26         ` Oleksij Rempel
@ 2020-05-27 19:11           ` Geert Uytterhoeven
  2020-05-27 20:52             ` Andrew Lunn
  2020-05-28  8:20             ` Philippe Schenker
  0 siblings, 2 replies; 30+ messages in thread
From: Geert Uytterhoeven @ 2020-05-27 19:11 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Philippe Schenker, andrew, sergei.shtylyov, kernel, davem,
	hkallweit1, linux, linux-kernel, david, netdev, f.fainelli,
	linux-renesas-soc, Kazuya Mizuguchi, Grygorii Strashko

Hi Oleksij,

On Wed, Apr 29, 2020 at 11:26 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> On Wed, Apr 29, 2020 at 10:45:35AM +0200, Geert Uytterhoeven wrote:
> > On Tue, Apr 28, 2020 at 6:16 PM Philippe Schenker
> > <philippe.schenker@toradex.com> wrote:
> > > On Tue, 2020-04-28 at 17:47 +0200, Andrew Lunn wrote:
> > > > On Tue, Apr 28, 2020 at 05:28:30PM +0200, Geert Uytterhoeven wrote:
> > > > > This triggers on Renesas Salvator-X(S):
> > > > >
> > > > >     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00:
> > > > > *-skew-ps values should be used only with phy-mode = "rgmii"
> > > > >
> > > > > which uses:
> > > > >
> > > > >         phy-mode = "rgmii-txid";
> > > > >
> > > > > and:
> > > > >
> > > > >         rxc-skew-ps = <1500>;
> > > > >
> > > > > If I understand Documentation/devicetree/bindings/net/ethernet-
> > > > > controller.yaml
> > > > > correctly:
> > > >
> > > > Checking for skews which might contradict the PHY-mode is new. I think
> > > > this is the first PHY driver to do it. So i'm not too surprised it has
> > > > triggered a warning, or there is contradictory documentation.
> > > >
> > > > Your use cases is reasonable. Have the normal transmit delay, and a
> > > > bit shorted receive delay. So we should allow it. It just makes the
> > > > validation code more complex :-(
> > >
> > > I reviewed Oleksij's patch that introduced this warning. I just want to
> > > explain our thinking why this is a good thing, but yes maybe we change
> > > that warning a little bit until it lands in mainline.
> > >
> > > The KSZ9031 driver didn't support for proper phy-modes until now as it
> > > don't have dedicated registers to control tx and rx delays. With
> > > Oleksij's patch this delay is now done accordingly in skew registers as
> > > best as possible. If you now also set the rxc-skew-ps registers those
> > > values you previously set with rgmii-txid or rxid get overwritten.

While I don't claim that the new implementation is incorrect, my biggest
gripe is that this change breaks existing setups (cfr. Grygorii's report,
plus see below).  People fine-tuned the parameters in their DTS files
according to the old driver behavior, and now have to update their DTBs,
which violates DTB backwards-compatibility rules.
I know it's ugly, but I'm afraid the only backwards-compatible solution
is to add a new DT property to indicate if the new rules apply.

> > > We chose the warning to occur on phy-modes 'rgmii-id', 'rgmii-rxid' and
> > > 'rgmii-txid' as on those, with the 'rxc-skew-ps' value present,
> > > overwriting skew values could occur and you end up with values you do
> > > not wanted. We thought, that most of the boards have just 'rgmii' set in
> > > phy-mode with specific skew-values present.
> > >
> > > @Geert if you actually want the PHY to apply RXC and TXC delays just
> > > insert 'rgmii-id' in your DT and remove those *-skew-ps values. If you
> >
> > That seems to work for me, but of course doesn't take into account PCB
> > routing.

Of course I talked too soon.  Both with the existing DTS that triggers
the warning, and after changing the mode to "rgmii-id", and dropping the
*-skew-ps values, Ethernet became flaky on R-Car M3-W ES1.0.  While the
system still boots, it boots very slow.
Using nuttcp, I discovered TX performance dropped from ca. 400 Mbps to
0.1-0.3 Mbps, while RX performance looks unaffected.

So I did some more testing:
  1. Plain "rgmii-txid" and "rgmii" break the network completely, on all
     R-Car Gen3 platforms,
  2. "rgmii-id" and "rgmii-rxid" work, but cause slowness on R-Car M3-W,
  3. "rgmii" with *-skew-ps values that match the old values (default
     420 for everything, but default 900 for txc-skew-ps, and the 1500
     override for rxc-skew-ps), behaves exactly the same as "rgmii-id",
  4. "rgmii-txid" with *-skew-ps values that match the old values does
work, i.e.
     adding to arch/arm64/boot/dts/renesas/salvator-common.dtsi:
     +               rxd0-skew-ps = <420>;
     +               rxd1-skew-ps = <420>;
     +               rxd2-skew-ps = <420>;
     +               rxd3-skew-ps = <420>;
     +               rxdv-skew-ps = <420>;
     +               txc-skew-ps = <900>;
     +               txd0-skew-ps = <420>;
     +               txd1-skew-ps = <420>;
     +               txd2-skew-ps = <420>;
     +               txd3-skew-ps = <420>;
     +               txen-skew-ps = <420>;

You may wonder what's the difference between 3 and 4? It's not just the
PHY driver that looks at phy-mode!
drivers/net/ethernet/renesas/ravb_main.c:ravb_set_delay_mode() also
does, and configures an additional TX clock delay of 1.8 ns if TXID is
enabled.  Doing so fixes R-Car M3-W, but doesn't seem to be needed,
or harm, on R-Car H3 ES2.0 and R-Car M3-N.

> > Using "rgmii" without any skew values makes DHCP fail on R-Car H3 ES2.0,
> > M3-W (ES1.0), and M3-N (ES1.0). Interestingly, DHCP still works on R-Car
> > H3 ES1.0.

FTR, the reason R-Car H3 ES1.0 is not affected is that the driver limits
its maximum speed to 100 Mbps, due to a hardware erratum.

So, how to proceed?
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-05-27 19:11           ` Geert Uytterhoeven
@ 2020-05-27 20:52             ` Andrew Lunn
  2020-05-28 13:10               ` Geert Uytterhoeven
  2020-05-28  8:20             ` Philippe Schenker
  1 sibling, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2020-05-27 20:52 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Oleksij Rempel, Philippe Schenker, sergei.shtylyov, kernel,
	davem, hkallweit1, linux, linux-kernel, david, netdev,
	f.fainelli, linux-renesas-soc, Kazuya Mizuguchi,
	Grygorii Strashko

> You may wonder what's the difference between 3 and 4? It's not just the
> PHY driver that looks at phy-mode!
> drivers/net/ethernet/renesas/ravb_main.c:ravb_set_delay_mode() also
> does, and configures an additional TX clock delay of 1.8 ns if TXID is
> enabled.

Hi Geert

That sounds like a MAC bug. Either the MAC insert the delay, or the
PHY does. If the MAC decides it is going to insert the delay, it
should be masking what it passes to phylib so that the PHY does not
add a second delay.

This whole area of RGMII delays has a number of historical bugs, which
often counter act each other. So you fix one, and it break somewhere
else.

In this case, not allowing skews for plain RGMII is probably being too
strict. We probably should relax that constrain in this case, for this
PHY driver.

    Andrew

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-05-27 19:11           ` Geert Uytterhoeven
  2020-05-27 20:52             ` Andrew Lunn
@ 2020-05-28  8:20             ` Philippe Schenker
  2020-05-28 12:51               ` Geert Uytterhoeven
  1 sibling, 1 reply; 30+ messages in thread
From: Philippe Schenker @ 2020-05-28  8:20 UTC (permalink / raw)
  To: o.rempel, geert
  Cc: andrew, linux, kernel, hkallweit1, davem, grygorii.strashko,
	linux-kernel, sergei.shtylyov, netdev, david, f.fainelli,
	linux-renesas-soc, kazuya.mizuguchi.ks

On Wed, 2020-05-27 at 21:11 +0200, Geert Uytterhoeven wrote:
> Hi Oleksij,
> 
> On Wed, Apr 29, 2020 at 11:26 AM Oleksij Rempel <
> o.rempel@pengutronix.de> wrote:
> > On Wed, Apr 29, 2020 at 10:45:35AM +0200, Geert Uytterhoeven wrote:
> > > On Tue, Apr 28, 2020 at 6:16 PM Philippe Schenker
> > > <philippe.schenker@toradex.com> wrote:
> > > > On Tue, 2020-04-28 at 17:47 +0200, Andrew Lunn wrote:
> > > > > On Tue, Apr 28, 2020 at 05:28:30PM +0200, Geert Uytterhoeven
> > > > > wrote:
> > > > > > This triggers on Renesas Salvator-X(S):
> > > > > > 
> > > > > >     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-
> > > > > > ffffffff:00:
> > > > > > *-skew-ps values should be used only with phy-mode = "rgmii"
> > > > > > 
> > > > > > which uses:
> > > > > > 
> > > > > >         phy-mode = "rgmii-txid";
> > > > > > 
> > > > > > and:
> > > > > > 
> > > > > >         rxc-skew-ps = <1500>;
> > > > > > 
> > > > > > If I understand
> > > > > > Documentation/devicetree/bindings/net/ethernet-
> > > > > > controller.yaml
> > > > > > correctly:
> > > > > 
> > > > > Checking for skews which might contradict the PHY-mode is new.
> > > > > I think
> > > > > this is the first PHY driver to do it. So i'm not too
> > > > > surprised it has
> > > > > triggered a warning, or there is contradictory documentation.
> > > > > 
> > > > > Your use cases is reasonable. Have the normal transmit delay,
> > > > > and a
> > > > > bit shorted receive delay. So we should allow it. It just
> > > > > makes the
> > > > > validation code more complex :-(
> > > > 
> > > > I reviewed Oleksij's patch that introduced this warning. I just
> > > > want to
> > > > explain our thinking why this is a good thing, but yes maybe we
> > > > change
> > > > that warning a little bit until it lands in mainline.
> > > > 
> > > > The KSZ9031 driver didn't support for proper phy-modes until now
> > > > as it
> > > > don't have dedicated registers to control tx and rx delays. With
> > > > Oleksij's patch this delay is now done accordingly in skew
> > > > registers as
> > > > best as possible. If you now also set the rxc-skew-ps registers
> > > > those
> > > > values you previously set with rgmii-txid or rxid get
> > > > overwritten.
> 
> While I don't claim that the new implementation is incorrect, my
> biggest
> gripe is that this change breaks existing setups (cfr. Grygorii's
> report,
> plus see below).  People fine-tuned the parameters in their DTS files
> according to the old driver behavior, and now have to update their
> DTBs,
> which violates DTB backwards-compatibility rules.
> I know it's ugly, but I'm afraid the only backwards-compatible
> solution
> is to add a new DT property to indicate if the new rules apply.
> 
> > > > We chose the warning to occur on phy-modes 'rgmii-id', 'rgmii-
> > > > rxid' and
> > > > 'rgmii-txid' as on those, with the 'rxc-skew-ps' value present,
> > > > overwriting skew values could occur and you end up with values
> > > > you do
> > > > not wanted. We thought, that most of the boards have just
> > > > 'rgmii' set in
> > > > phy-mode with specific skew-values present.
> > > > 
> > > > @Geert if you actually want the PHY to apply RXC and TXC delays
> > > > just
> > > > insert 'rgmii-id' in your DT and remove those *-skew-ps values.
> > > > If you
> > > 
> > > That seems to work for me, but of course doesn't take into account
> > > PCB
> > > routing.
> 
> Of course I talked too soon.  Both with the existing DTS that triggers
> the warning, and after changing the mode to "rgmii-id", and dropping
> the
> *-skew-ps values, Ethernet became flaky on R-Car M3-W ES1.0.  While
> the
> system still boots, it boots very slow.
> Using nuttcp, I discovered TX performance dropped from ca. 400 Mbps to
> 0.1-0.3 Mbps, while RX performance looks unaffected.
> 
> So I did some more testing:
>   1. Plain "rgmii-txid" and "rgmii" break the network completely, on
> all
>      R-Car Gen3 platforms,
>   2. "rgmii-id" and "rgmii-rxid" work, but cause slowness on R-Car M3-
> W,
>   3. "rgmii" with *-skew-ps values that match the old values (default
>      420 for everything, but default 900 for txc-skew-ps, and the 1500
>      override for rxc-skew-ps), behaves exactly the same as "rgmii-
> id",
>   4. "rgmii-txid" with *-skew-ps values that match the old values does
> work, i.e.
>      adding to arch/arm64/boot/dts/renesas/salvator-common.dtsi:
>      +               rxd0-skew-ps = <420>;
>      +               rxd1-skew-ps = <420>;
>      +               rxd2-skew-ps = <420>;
>      +               rxd3-skew-ps = <420>;
>      +               rxdv-skew-ps = <420>;
>      +               txc-skew-ps = <900>;
>      +               txd0-skew-ps = <420>;
>      +               txd1-skew-ps = <420>;
>      +               txd2-skew-ps = <420>;
>      +               txd3-skew-ps = <420>;
>      +               txen-skew-ps = <420>;
> 
> You may wonder what's the difference between 3 and 4? It's not just
> the
> PHY driver that looks at phy-mode!
> drivers/net/ethernet/renesas/ravb_main.c:ravb_set_delay_mode() also
> does, and configures an additional TX clock delay of 1.8 ns if TXID is
> enabled.  Doing so fixes R-Car M3-W, but doesn't seem to be needed,
> or harm, on R-Car H3 ES2.0 and R-Car M3-N.

Hi Geert,

Sorry for chiming in on this topic but I also did make my thoughts about
this implementation.

The documentation in Documentation/devicetree/bindings/net/ethernet-
controller.yaml clearly states, that rgmii-id is meaning the delay is
provided by the PHY and MAC should not add anything in this case.

Best Regards,
Philippe

> 
> > > Using "rgmii" without any skew values makes DHCP fail on R-Car H3
> > > ES2.0,
> > > M3-W (ES1.0), and M3-N (ES1.0). Interestingly, DHCP still works on
> > > R-Car
> > > H3 ES1.0.
> 
> FTR, the reason R-Car H3 ES1.0 is not affected is that the driver
> limits
> its maximum speed to 100 Mbps, due to a hardware erratum.
> 
> So, how to proceed?
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-05-28  8:20             ` Philippe Schenker
@ 2020-05-28 12:51               ` Geert Uytterhoeven
  2020-05-28 23:31                 ` Florian Fainelli
  0 siblings, 1 reply; 30+ messages in thread
From: Geert Uytterhoeven @ 2020-05-28 12:51 UTC (permalink / raw)
  To: Philippe Schenker
  Cc: o.rempel, andrew, linux, kernel, hkallweit1, davem,
	grygorii.strashko, linux-kernel, sergei.shtylyov, netdev, david,
	f.fainelli, linux-renesas-soc, kazuya.mizuguchi.ks

Hi Philippe,

On Thu, May 28, 2020 at 10:20 AM Philippe Schenker
<philippe.schenker@toradex.com> wrote:
> On Wed, 2020-05-27 at 21:11 +0200, Geert Uytterhoeven wrote:
> > On Wed, Apr 29, 2020 at 11:26 AM Oleksij Rempel <
> > o.rempel@pengutronix.de> wrote:
> > > On Wed, Apr 29, 2020 at 10:45:35AM +0200, Geert Uytterhoeven wrote:
> > > > On Tue, Apr 28, 2020 at 6:16 PM Philippe Schenker
> > > > <philippe.schenker@toradex.com> wrote:
> > > > > On Tue, 2020-04-28 at 17:47 +0200, Andrew Lunn wrote:
> > > > > > On Tue, Apr 28, 2020 at 05:28:30PM +0200, Geert Uytterhoeven
> > > > > > wrote:
> > > > > > > This triggers on Renesas Salvator-X(S):
> > > > > > >
> > > > > > >     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-
> > > > > > > ffffffff:00:
> > > > > > > *-skew-ps values should be used only with phy-mode = "rgmii"
> > > > > > >
> > > > > > > which uses:
> > > > > > >
> > > > > > >         phy-mode = "rgmii-txid";
> > > > > > >
> > > > > > > and:
> > > > > > >
> > > > > > >         rxc-skew-ps = <1500>;
> > > > > > >
> > > > > > > If I understand
> > > > > > > Documentation/devicetree/bindings/net/ethernet-
> > > > > > > controller.yaml
> > > > > > > correctly:
> > > > > >
> > > > > > Checking for skews which might contradict the PHY-mode is new.
> > > > > > I think
> > > > > > this is the first PHY driver to do it. So i'm not too
> > > > > > surprised it has
> > > > > > triggered a warning, or there is contradictory documentation.
> > > > > >
> > > > > > Your use cases is reasonable. Have the normal transmit delay,
> > > > > > and a
> > > > > > bit shorted receive delay. So we should allow it. It just
> > > > > > makes the
> > > > > > validation code more complex :-(
> > > > >
> > > > > I reviewed Oleksij's patch that introduced this warning. I just
> > > > > want to
> > > > > explain our thinking why this is a good thing, but yes maybe we
> > > > > change
> > > > > that warning a little bit until it lands in mainline.
> > > > >
> > > > > The KSZ9031 driver didn't support for proper phy-modes until now
> > > > > as it
> > > > > don't have dedicated registers to control tx and rx delays. With
> > > > > Oleksij's patch this delay is now done accordingly in skew
> > > > > registers as
> > > > > best as possible. If you now also set the rxc-skew-ps registers
> > > > > those
> > > > > values you previously set with rgmii-txid or rxid get
> > > > > overwritten.
> >
> > While I don't claim that the new implementation is incorrect, my
> > biggest
> > gripe is that this change breaks existing setups (cfr. Grygorii's
> > report,
> > plus see below).  People fine-tuned the parameters in their DTS files
> > according to the old driver behavior, and now have to update their
> > DTBs,
> > which violates DTB backwards-compatibility rules.
> > I know it's ugly, but I'm afraid the only backwards-compatible
> > solution
> > is to add a new DT property to indicate if the new rules apply.
> >
> > > > > We chose the warning to occur on phy-modes 'rgmii-id', 'rgmii-
> > > > > rxid' and
> > > > > 'rgmii-txid' as on those, with the 'rxc-skew-ps' value present,
> > > > > overwriting skew values could occur and you end up with values
> > > > > you do
> > > > > not wanted. We thought, that most of the boards have just
> > > > > 'rgmii' set in
> > > > > phy-mode with specific skew-values present.
> > > > >
> > > > > @Geert if you actually want the PHY to apply RXC and TXC delays
> > > > > just
> > > > > insert 'rgmii-id' in your DT and remove those *-skew-ps values.
> > > > > If you
> > > >
> > > > That seems to work for me, but of course doesn't take into account
> > > > PCB
> > > > routing.
> >
> > Of course I talked too soon.  Both with the existing DTS that triggers
> > the warning, and after changing the mode to "rgmii-id", and dropping
> > the
> > *-skew-ps values, Ethernet became flaky on R-Car M3-W ES1.0.  While
> > the
> > system still boots, it boots very slow.
> > Using nuttcp, I discovered TX performance dropped from ca. 400 Mbps to
> > 0.1-0.3 Mbps, while RX performance looks unaffected.
> >
> > So I did some more testing:
> >   1. Plain "rgmii-txid" and "rgmii" break the network completely, on
> > all
> >      R-Car Gen3 platforms,
> >   2. "rgmii-id" and "rgmii-rxid" work, but cause slowness on R-Car M3-
> > W,
> >   3. "rgmii" with *-skew-ps values that match the old values (default
> >      420 for everything, but default 900 for txc-skew-ps, and the 1500
> >      override for rxc-skew-ps), behaves exactly the same as "rgmii-
> > id",
> >   4. "rgmii-txid" with *-skew-ps values that match the old values does
> > work, i.e.
> >      adding to arch/arm64/boot/dts/renesas/salvator-common.dtsi:
> >      +               rxd0-skew-ps = <420>;
> >      +               rxd1-skew-ps = <420>;
> >      +               rxd2-skew-ps = <420>;
> >      +               rxd3-skew-ps = <420>;
> >      +               rxdv-skew-ps = <420>;
> >      +               txc-skew-ps = <900>;
> >      +               txd0-skew-ps = <420>;
> >      +               txd1-skew-ps = <420>;
> >      +               txd2-skew-ps = <420>;
> >      +               txd3-skew-ps = <420>;
> >      +               txen-skew-ps = <420>;
> >
> > You may wonder what's the difference between 3 and 4? It's not just
> > the
> > PHY driver that looks at phy-mode!
> > drivers/net/ethernet/renesas/ravb_main.c:ravb_set_delay_mode() also
> > does, and configures an additional TX clock delay of 1.8 ns if TXID is
> > enabled.  Doing so fixes R-Car M3-W, but doesn't seem to be needed,
> > or harm, on R-Car H3 ES2.0 and R-Car M3-N.
>
> Sorry for chiming in on this topic but I also did make my thoughts about
> this implementation.
>
> The documentation in Documentation/devicetree/bindings/net/ethernet-
> controller.yaml clearly states, that rgmii-id is meaning the delay is
> provided by the PHY and MAC should not add anything in this case.

Thank you for your very valuable comment!
That means the semantics are clear, and is the reason behind the existence
of properties like "amlogic,tx-delay-ns", which do apply to the MAC.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-05-27 20:52             ` Andrew Lunn
@ 2020-05-28 13:10               ` Geert Uytterhoeven
  2020-05-28 16:08                 ` Andrew Lunn
  0 siblings, 1 reply; 30+ messages in thread
From: Geert Uytterhoeven @ 2020-05-28 13:10 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Oleksij Rempel, Philippe Schenker, sergei.shtylyov, kernel,
	davem, hkallweit1, linux, linux-kernel, david, netdev,
	f.fainelli, linux-renesas-soc, Kazuya Mizuguchi,
	Grygorii Strashko

Hi Andrew,

On Wed, May 27, 2020 at 10:52 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > You may wonder what's the difference between 3 and 4? It's not just the
> > PHY driver that looks at phy-mode!
> > drivers/net/ethernet/renesas/ravb_main.c:ravb_set_delay_mode() also
> > does, and configures an additional TX clock delay of 1.8 ns if TXID is
> > enabled.
>
> That sounds like a MAC bug. Either the MAC insert the delay, or the
> PHY does. If the MAC decides it is going to insert the delay, it
> should be masking what it passes to phylib so that the PHY does not
> add a second delay.

And so I gave this a try, and modified the ravb driver to pass "rgmii"
to the PHY if it has inserted a delay.
That fixes the speed issue on R-Car M3-W!
And gets rid of the "*-skew-ps values should be used only with..."
message.

I also tried if I can get rid of "rxc-skew-ps = <1500>". After dropping
the property, DHCP failed.  Compensating by changing the PHY mode in DT
from "rgmii-txid" to "rgmii-id" makes it work again.

However, given Philippe's comment that the rgmi-*id apply to the PHY
only, I think we need new DT properties for enabling MAC internal delays.

> This whole area of RGMII delays has a number of historical bugs, which
> often counter act each other. So you fix one, and it break somewhere
> else.

Indeed...

> In this case, not allowing skews for plain RGMII is probably being too
> strict. We probably should relax that constrain in this case, for this
> PHY driver.

That description is not quite correct: the driver expects skews for
plain RGMII only. For RGMII-*ID, it prints a warning, but still applies
the supplied skew values.


To fix the issue, I came up with the following problem statement and
plan:

A. Old behavior:

  1. ravb acts upon "rgmii-*id" (on SoCs that support it[1]),
  2. ksz9031 ignored "rgmii-*id", using hardware defaults for skew
     values.

B. New behavior (broken):

  1. ravb acts upon "rgmii-*id",
  2. ksz9031 acts upon "rgmii-*id".

C. Quick fix for v5.8 (workaround, backwards-compatible with old DTB):

  1. ravb acts upon "rgmii-*id", but passes "rgmii" to phy,
  2. ksz9031 acts upon "rgmi", using new "rgmii" skew values.

D. Long-term fix:
  1. Check if new boolean "renesas,[rt]x-delay"[2] values are
      specified in DTB.
       No: ravb acts upon "rgmii-*id", but passes "rgmii" to phy, for
           backwards-compatibility,
       Yes: ravb enables TX clock delay of 2.0 ns and/or RX clock delay
            of 1.8 ns, based on "renesas,[rt]x-delay" values, and passes
            the unmodified interface type to phy,
  2. ksz9031 acts upon "rgmii*",
  3. Salvator-X(S) DTS makes things explicit by changing it from

        phy-mode = "rgmii-txid";
        rxc-skew-ps = <1500>;

     to:

        phy-mode = "rgmii";
        renesas,rx-delay = <false>;
        renesas,tx-delay = <true>;
        rxc-skew-ps = <1500>;

     or:

        phy-mode = "rgmii";
        renesas,rx-delay = <true>;
        renesas,tx-delay = <true>;

[2] Should we use numerical "renesas,[rt]x-delay-ps" instead?
     The only supported values are 0 and 2000 (TX) or 1800 (RX).

The ULCB boards are very similar to Salvator-X(S), so I guess they
behave the same, and are thus affected.

Unfortunately there are other boards that use R-Car Gen3 EtherAVB:
  - The Silicon Linux sub board for CAT874 (CAT875) connects EtherAVB to
    an RTL8211E PHY.  As it uses the "rgmii" mode, it should not be
    affected.
  - The HiHope RZ/G2M sub board connects EtherAVB to an RTL8211E PHY.
    It uses the "rgmii-txid" mode, so it will be affacted by modifying
    the ravb driver.
  - Eagle and V3MSK connect EtherAVB to a KSZ9031, and use the "rgmii-id"
    mode with rxc-skew-ps = <1500>, so they are affected.
  - Ebisu and Draak connect EtherAVB to a KSZ9031, and use the "rgmii"
    mode with rxc-skew-ps = <1500>.  However, they're limited to 100
    Mbps, as EtherAVB on R-Car E3 and D3 do not support TX clock
    internal delay mode[1], and the delays provided by KSZ9031 clock pad
    skew were deemed insufficient.

Obviously, the affected boards will need testing (I only have
Salvator-X(S) and Ebisu).

Does the above make sense?
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-05-28 13:10               ` Geert Uytterhoeven
@ 2020-05-28 16:08                 ` Andrew Lunn
  2020-05-29  4:59                   ` Oleksij Rempel
  2020-05-29 10:17                   ` Geert Uytterhoeven
  0 siblings, 2 replies; 30+ messages in thread
From: Andrew Lunn @ 2020-05-28 16:08 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Oleksij Rempel, Philippe Schenker, sergei.shtylyov, kernel,
	davem, hkallweit1, linux, linux-kernel, david, netdev,
	f.fainelli, linux-renesas-soc, Kazuya Mizuguchi,
	Grygorii Strashko

On Thu, May 28, 2020 at 03:10:06PM +0200, Geert Uytterhoeven wrote:
> Hi Andrew,
> 
> On Wed, May 27, 2020 at 10:52 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > You may wonder what's the difference between 3 and 4? It's not just the
> > > PHY driver that looks at phy-mode!
> > > drivers/net/ethernet/renesas/ravb_main.c:ravb_set_delay_mode() also
> > > does, and configures an additional TX clock delay of 1.8 ns if TXID is
> > > enabled.
> >
> > That sounds like a MAC bug. Either the MAC insert the delay, or the
> > PHY does. If the MAC decides it is going to insert the delay, it
> > should be masking what it passes to phylib so that the PHY does not
> > add a second delay.
> 
> And so I gave this a try, and modified the ravb driver to pass "rgmii"
> to the PHY if it has inserted a delay.
> That fixes the speed issue on R-Car M3-W!
> And gets rid of the "*-skew-ps values should be used only with..."
> message.
> 
> I also tried if I can get rid of "rxc-skew-ps = <1500>". After dropping
> the property, DHCP failed.  Compensating by changing the PHY mode in DT
> from "rgmii-txid" to "rgmii-id" makes it work again.

In general, i suggest that the PHY implements the delay, not the MAC.
Most PHYs support it, where as most MACs don't. It keeps maintenance
and understanding easier, if everything is the same. But there are
cases where the PHY does not have the needed support, and the MAC does
the delays.

> However, given Philippe's comment that the rgmi-*id apply to the PHY
> only, I think we need new DT properties for enabling MAC internal delays.

Do you actually need MAC internal delays?

> That description is not quite correct: the driver expects skews for
> plain RGMII only. For RGMII-*ID, it prints a warning, but still applies
> the supplied skew values.

O.K. so not so bad.

> 
> To fix the issue, I came up with the following problem statement and
> plan:
> 
> A. Old behavior:
> 
>   1. ravb acts upon "rgmii-*id" (on SoCs that support it[1]),
>   2. ksz9031 ignored "rgmii-*id", using hardware defaults for skew
>      values.

So two bugs which cancelled each other out :-)

> B. New behavior (broken):
> 
>   1. ravb acts upon "rgmii-*id",
>   2. ksz9031 acts upon "rgmii-*id".
> 
> C. Quick fix for v5.8 (workaround, backwards-compatible with old DTB):
> 
>   1. ravb acts upon "rgmii-*id", but passes "rgmii" to phy,
>   2. ksz9031 acts upon "rgmi", using new "rgmii" skew values.
> 
> D. Long-term fix:

I don't know if it is possible, but i would prefer that ravb does
nothing and the PHY does the delay. The question is, can you get to
this state without more things breaking?

     Andrew

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-05-28 12:51               ` Geert Uytterhoeven
@ 2020-05-28 23:31                 ` Florian Fainelli
  0 siblings, 0 replies; 30+ messages in thread
From: Florian Fainelli @ 2020-05-28 23:31 UTC (permalink / raw)
  To: Geert Uytterhoeven, Philippe Schenker
  Cc: o.rempel, andrew, linux, kernel, hkallweit1, davem,
	grygorii.strashko, linux-kernel, sergei.shtylyov, netdev, david,
	linux-renesas-soc, kazuya.mizuguchi.ks



On 5/28/2020 5:51 AM, Geert Uytterhoeven wrote:
> Hi Philippe,
> 
> On Thu, May 28, 2020 at 10:20 AM Philippe Schenker
> <philippe.schenker@toradex.com> wrote:
>> On Wed, 2020-05-27 at 21:11 +0200, Geert Uytterhoeven wrote:
>>> On Wed, Apr 29, 2020 at 11:26 AM Oleksij Rempel <
>>> o.rempel@pengutronix.de> wrote:
>>>> On Wed, Apr 29, 2020 at 10:45:35AM +0200, Geert Uytterhoeven wrote:
>>>>> On Tue, Apr 28, 2020 at 6:16 PM Philippe Schenker
>>>>> <philippe.schenker@toradex.com> wrote:
>>>>>> On Tue, 2020-04-28 at 17:47 +0200, Andrew Lunn wrote:
>>>>>>> On Tue, Apr 28, 2020 at 05:28:30PM +0200, Geert Uytterhoeven
>>>>>>> wrote:
>>>>>>>> This triggers on Renesas Salvator-X(S):
>>>>>>>>
>>>>>>>>     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-
>>>>>>>> ffffffff:00:
>>>>>>>> *-skew-ps values should be used only with phy-mode = "rgmii"
>>>>>>>>
>>>>>>>> which uses:
>>>>>>>>
>>>>>>>>         phy-mode = "rgmii-txid";
>>>>>>>>
>>>>>>>> and:
>>>>>>>>
>>>>>>>>         rxc-skew-ps = <1500>;
>>>>>>>>
>>>>>>>> If I understand
>>>>>>>> Documentation/devicetree/bindings/net/ethernet-
>>>>>>>> controller.yaml
>>>>>>>> correctly:
>>>>>>>
>>>>>>> Checking for skews which might contradict the PHY-mode is new.
>>>>>>> I think
>>>>>>> this is the first PHY driver to do it. So i'm not too
>>>>>>> surprised it has
>>>>>>> triggered a warning, or there is contradictory documentation.
>>>>>>>
>>>>>>> Your use cases is reasonable. Have the normal transmit delay,
>>>>>>> and a
>>>>>>> bit shorted receive delay. So we should allow it. It just
>>>>>>> makes the
>>>>>>> validation code more complex :-(
>>>>>>
>>>>>> I reviewed Oleksij's patch that introduced this warning. I just
>>>>>> want to
>>>>>> explain our thinking why this is a good thing, but yes maybe we
>>>>>> change
>>>>>> that warning a little bit until it lands in mainline.
>>>>>>
>>>>>> The KSZ9031 driver didn't support for proper phy-modes until now
>>>>>> as it
>>>>>> don't have dedicated registers to control tx and rx delays. With
>>>>>> Oleksij's patch this delay is now done accordingly in skew
>>>>>> registers as
>>>>>> best as possible. If you now also set the rxc-skew-ps registers
>>>>>> those
>>>>>> values you previously set with rgmii-txid or rxid get
>>>>>> overwritten.
>>>
>>> While I don't claim that the new implementation is incorrect, my
>>> biggest
>>> gripe is that this change breaks existing setups (cfr. Grygorii's
>>> report,
>>> plus see below).  People fine-tuned the parameters in their DTS files
>>> according to the old driver behavior, and now have to update their
>>> DTBs,
>>> which violates DTB backwards-compatibility rules.
>>> I know it's ugly, but I'm afraid the only backwards-compatible
>>> solution
>>> is to add a new DT property to indicate if the new rules apply.
>>>
>>>>>> We chose the warning to occur on phy-modes 'rgmii-id', 'rgmii-
>>>>>> rxid' and
>>>>>> 'rgmii-txid' as on those, with the 'rxc-skew-ps' value present,
>>>>>> overwriting skew values could occur and you end up with values
>>>>>> you do
>>>>>> not wanted. We thought, that most of the boards have just
>>>>>> 'rgmii' set in
>>>>>> phy-mode with specific skew-values present.
>>>>>>
>>>>>> @Geert if you actually want the PHY to apply RXC and TXC delays
>>>>>> just
>>>>>> insert 'rgmii-id' in your DT and remove those *-skew-ps values.
>>>>>> If you
>>>>>
>>>>> That seems to work for me, but of course doesn't take into account
>>>>> PCB
>>>>> routing.
>>>
>>> Of course I talked too soon.  Both with the existing DTS that triggers
>>> the warning, and after changing the mode to "rgmii-id", and dropping
>>> the
>>> *-skew-ps values, Ethernet became flaky on R-Car M3-W ES1.0.  While
>>> the
>>> system still boots, it boots very slow.
>>> Using nuttcp, I discovered TX performance dropped from ca. 400 Mbps to
>>> 0.1-0.3 Mbps, while RX performance looks unaffected.
>>>
>>> So I did some more testing:
>>>   1. Plain "rgmii-txid" and "rgmii" break the network completely, on
>>> all
>>>      R-Car Gen3 platforms,
>>>   2. "rgmii-id" and "rgmii-rxid" work, but cause slowness on R-Car M3-
>>> W,
>>>   3. "rgmii" with *-skew-ps values that match the old values (default
>>>      420 for everything, but default 900 for txc-skew-ps, and the 1500
>>>      override for rxc-skew-ps), behaves exactly the same as "rgmii-
>>> id",
>>>   4. "rgmii-txid" with *-skew-ps values that match the old values does
>>> work, i.e.
>>>      adding to arch/arm64/boot/dts/renesas/salvator-common.dtsi:
>>>      +               rxd0-skew-ps = <420>;
>>>      +               rxd1-skew-ps = <420>;
>>>      +               rxd2-skew-ps = <420>;
>>>      +               rxd3-skew-ps = <420>;
>>>      +               rxdv-skew-ps = <420>;
>>>      +               txc-skew-ps = <900>;
>>>      +               txd0-skew-ps = <420>;
>>>      +               txd1-skew-ps = <420>;
>>>      +               txd2-skew-ps = <420>;
>>>      +               txd3-skew-ps = <420>;
>>>      +               txen-skew-ps = <420>;
>>>
>>> You may wonder what's the difference between 3 and 4? It's not just
>>> the
>>> PHY driver that looks at phy-mode!
>>> drivers/net/ethernet/renesas/ravb_main.c:ravb_set_delay_mode() also
>>> does, and configures an additional TX clock delay of 1.8 ns if TXID is
>>> enabled.  Doing so fixes R-Car M3-W, but doesn't seem to be needed,
>>> or harm, on R-Car H3 ES2.0 and R-Car M3-N.
>>
>> Sorry for chiming in on this topic but I also did make my thoughts about
>> this implementation.
>>
>> The documentation in Documentation/devicetree/bindings/net/ethernet-
>> controller.yaml clearly states, that rgmii-id is meaning the delay is
>> provided by the PHY and MAC should not add anything in this case.
> 
> Thank you for your very valuable comment!
> That means the semantics are clear, and is the reason behind the existence
> of properties like "amlogic,tx-delay-ns", which do apply to the MAC.

They are clear now, but they were not always clear which is why it is
possible that some Ethernet MACs act on the phy_interface_t value when
they should not. There is not a good way to guard against such things
other than reviewing drivers carefully, RGMII was not designed with plug
and play in mind, just reduced pin count.
-- 
Florian

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-05-28 16:08                 ` Andrew Lunn
@ 2020-05-29  4:59                   ` Oleksij Rempel
  2020-05-29 10:17                   ` Geert Uytterhoeven
  1 sibling, 0 replies; 30+ messages in thread
From: Oleksij Rempel @ 2020-05-29  4:59 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Geert Uytterhoeven, Philippe Schenker, sergei.shtylyov, kernel,
	davem, hkallweit1, linux, linux-kernel, david, netdev,
	f.fainelli, linux-renesas-soc, Kazuya Mizuguchi,
	Grygorii Strashko

On Thu, May 28, 2020 at 06:08:39PM +0200, Andrew Lunn wrote:
> On Thu, May 28, 2020 at 03:10:06PM +0200, Geert Uytterhoeven wrote:
> > Hi Andrew,
> > 
> > On Wed, May 27, 2020 at 10:52 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > > You may wonder what's the difference between 3 and 4? It's not just the
> > > > PHY driver that looks at phy-mode!
> > > > drivers/net/ethernet/renesas/ravb_main.c:ravb_set_delay_mode() also
> > > > does, and configures an additional TX clock delay of 1.8 ns if TXID is
> > > > enabled.
> > >
> > > That sounds like a MAC bug. Either the MAC insert the delay, or the
> > > PHY does. If the MAC decides it is going to insert the delay, it
> > > should be masking what it passes to phylib so that the PHY does not
> > > add a second delay.
> > 
> > And so I gave this a try, and modified the ravb driver to pass "rgmii"
> > to the PHY if it has inserted a delay.
> > That fixes the speed issue on R-Car M3-W!
> > And gets rid of the "*-skew-ps values should be used only with..."
> > message.
> > 
> > I also tried if I can get rid of "rxc-skew-ps = <1500>". After dropping
> > the property, DHCP failed.  Compensating by changing the PHY mode in DT
> > from "rgmii-txid" to "rgmii-id" makes it work again.
> 
> In general, i suggest that the PHY implements the delay, not the MAC.
> Most PHYs support it, where as most MACs don't. It keeps maintenance
> and understanding easier, if everything is the same. But there are
> cases where the PHY does not have the needed support, and the MAC does
> the delays.
> 
> > However, given Philippe's comment that the rgmi-*id apply to the PHY
> > only, I think we need new DT properties for enabling MAC internal delays.
> 
> Do you actually need MAC internal delays?
> 
> > That description is not quite correct: the driver expects skews for
> > plain RGMII only. For RGMII-*ID, it prints a warning, but still applies
> > the supplied skew values.
> 
> O.K. so not so bad.
> 
> > 
> > To fix the issue, I came up with the following problem statement and
> > plan:
> > 
> > A. Old behavior:
> > 
> >   1. ravb acts upon "rgmii-*id" (on SoCs that support it[1]),
> >   2. ksz9031 ignored "rgmii-*id", using hardware defaults for skew
> >      values.
> 
> So two bugs which cancelled each other out :-)
> 
> > B. New behavior (broken):
> > 
> >   1. ravb acts upon "rgmii-*id",
> >   2. ksz9031 acts upon "rgmii-*id".
> > 
> > C. Quick fix for v5.8 (workaround, backwards-compatible with old DTB):
> > 
> >   1. ravb acts upon "rgmii-*id", but passes "rgmii" to phy,
> >   2. ksz9031 acts upon "rgmi", using new "rgmii" skew values.
> > 
> > D. Long-term fix:
> 
> I don't know if it is possible, but i would prefer that ravb does
> nothing and the PHY does the delay. The question is, can you get to
> this state without more things breaking?

Some MACs, for example the Atheros AG71XX support delay configuration as
well. But it support also the clock direction. It means (please correct me if
i'm wrong), the MAC can be configured to act as PHY. The same is about
switches, the MAC attached to CPU is act as a PHY and should care about proper
delay configuration.

Regards,
Oleksij

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-05-28 16:08                 ` Andrew Lunn
  2020-05-29  4:59                   ` Oleksij Rempel
@ 2020-05-29 10:17                   ` Geert Uytterhoeven
  1 sibling, 0 replies; 30+ messages in thread
From: Geert Uytterhoeven @ 2020-05-29 10:17 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Oleksij Rempel, Philippe Schenker, sergei.shtylyov, kernel,
	davem, hkallweit1, linux, linux-kernel, david, netdev,
	f.fainelli, linux-renesas-soc, Kazuya Mizuguchi,
	Grygorii Strashko

Hi Andrew,

On Thu, May 28, 2020 at 6:08 PM Andrew Lunn <andrew@lunn.ch> wrote:
> On Thu, May 28, 2020 at 03:10:06PM +0200, Geert Uytterhoeven wrote:
> > On Wed, May 27, 2020 at 10:52 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > > You may wonder what's the difference between 3 and 4? It's not just the
> > > > PHY driver that looks at phy-mode!
> > > > drivers/net/ethernet/renesas/ravb_main.c:ravb_set_delay_mode() also
> > > > does, and configures an additional TX clock delay of 1.8 ns if TXID is
> > > > enabled.
> > >
> > > That sounds like a MAC bug. Either the MAC insert the delay, or the
> > > PHY does. If the MAC decides it is going to insert the delay, it
> > > should be masking what it passes to phylib so that the PHY does not
> > > add a second delay.
> >
> > And so I gave this a try, and modified the ravb driver to pass "rgmii"
> > to the PHY if it has inserted a delay.
> > That fixes the speed issue on R-Car M3-W!
> > And gets rid of the "*-skew-ps values should be used only with..."
> > message.
> >
> > I also tried if I can get rid of "rxc-skew-ps = <1500>". After dropping
> > the property, DHCP failed.  Compensating by changing the PHY mode in DT
> > from "rgmii-txid" to "rgmii-id" makes it work again.
>
> In general, i suggest that the PHY implements the delay, not the MAC.
> Most PHYs support it, where as most MACs don't. It keeps maintenance
> and understanding easier, if everything is the same. But there are
> cases where the PHY does not have the needed support, and the MAC does
> the delays.

I can confirm disabling the MAC delay ("phy-mode = "rgmii""), and adding
a PHY delay ("txc-skew-ps = <1500>") also fixes the slowness on
Salvator-X with R-Car M3-W ES1.0.

However, I would like to be a cit cautious here: on Ebisu with R-Car E3,
the hardware engineers advised to add "max-speed = <100>", as EtherAVB
on R-Car E3 does not support the MAC delay, and the KSZ9031 does not
allow sufficient delay, leading to unreliable communication.
Nevertheless, I never had problems without that limitation, and 1 Gbps
still seems to work after removing it, with and without "txc-skew-ps =
<1500>".

> > However, given Philippe's comment that the rgmi-*id apply to the PHY
> > only, I think we need new DT properties for enabling MAC internal delays.
>
> Do you actually need MAC internal delays?

Given the Ebisu issue, I think we do.
Note that the EtherAVB MAC TX delay, when enabled, is 2.0 ns, and
KSZ9031 supports 0..1860 ps, with 900 ps being the centerpoint, so AFAIU
that is -900..960 ps, i.e. much less than 2.0 ns.

> > To fix the issue, I came up with the following problem statement and
> > plan:
> >
> > A. Old behavior:
> >
> >   1. ravb acts upon "rgmii-*id" (on SoCs that support it[1]),
> >   2. ksz9031 ignored "rgmii-*id", using hardware defaults for skew
> >      values.
>
> So two bugs which cancelled each other out :-)
>
> > B. New behavior (broken):
> >
> >   1. ravb acts upon "rgmii-*id",
> >   2. ksz9031 acts upon "rgmii-*id".
> >
> > C. Quick fix for v5.8 (workaround, backwards-compatible with old DTB):
> >
> >   1. ravb acts upon "rgmii-*id", but passes "rgmii" to phy,
> >   2. ksz9031 acts upon "rgmi", using new "rgmii" skew values.
> >
> > D. Long-term fix:
>
> I don't know if it is possible, but i would prefer that ravb does
> nothing and the PHY does the delay. The question is, can you get to
> this state without more things breaking?

While that seems to work for me, the delay would be a bit too small to work
reliably, according to the hardware engineers.

Hence my proposal for C now, to fix the regressions, and D later.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-22  7:21 [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY Oleksij Rempel
                   ` (4 preceding siblings ...)
  2020-05-05 18:26 ` Grygorii Strashko
@ 2020-07-10 22:36 ` Alexandre Belloni
  2020-07-10 22:54   ` Andrew Lunn
  5 siblings, 1 reply; 30+ messages in thread
From: Alexandre Belloni @ 2020-07-10 22:36 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David Jander,
	David S. Miller, kernel, linux-kernel, netdev, Philippe Schenker,
	Russell King

Hi Oleksij,

This patch breaks Ethernet on the sama5d3 Xplained and I have not been
able to unbreak it. Note tat If you want to test, networking has been
further broken in v5.8-rc1 but should be properly working in v5.8-rc4
after reverting this patch.

On 22/04/2020 09:21:37+0200, Oleksij Rempel wrote:
> Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid, rgmii-rxid.
> 
> This PHY has an internal RX delay of 1.2ns and no delay for TX.
> 
> The pad skew registers allow to set the total TX delay to max 1.38ns and
> the total RX delay to max of 2.58ns (configurable 1.38ns + build in
> 1.2ns) and a minimal delay of 0ns.
> 
> According to the RGMII v1.3 specification the delay provided by PCB traces
> should be between 1.5ns and 2.0ns. The RGMII v2.0 allows to provide this
> delay by MAC or PHY. So, we configure this PHY to the best values we can
> get by this HW: TX delay to 1.38ns (max supported value) and RX delay to
> 1.80ns (best calculated delay)
> 
> The phy-modes can still be fine tuned/overwritten by *-skew-ps
> device tree properties described in:
> Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> changes v3:
> - change delay on RX line to 1.80ns
> - add warning if *-skew-ps properties are used together with not rgmii
>   mode. 
> 
> changes v2:
> - change RX_ID value from 0x1a to 0xa. The overflow bit was detected by
>   FIELD_PREP() build check.
>   Reported-by: kbuild test robot <lkp@intel.com>
> 
>  drivers/net/phy/micrel.c | 128 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 123 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 05d20343b8161..045783eb4bc70 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -19,6 +19,7 @@
>   *			 ksz9477
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/phy.h>
> @@ -490,9 +491,50 @@ static int ksz9021_config_init(struct phy_device *phydev)
>  
>  /* MMD Address 0x2 */
>  #define MII_KSZ9031RN_CONTROL_PAD_SKEW	4
> +#define MII_KSZ9031RN_RX_CTL_M		GENMASK(7, 4)
> +#define MII_KSZ9031RN_TX_CTL_M		GENMASK(3, 0)
> +
>  #define MII_KSZ9031RN_RX_DATA_PAD_SKEW	5
> +#define MII_KSZ9031RN_RXD3		GENMASK(15, 12)
> +#define MII_KSZ9031RN_RXD2		GENMASK(11, 8)
> +#define MII_KSZ9031RN_RXD1		GENMASK(7, 4)
> +#define MII_KSZ9031RN_RXD0		GENMASK(3, 0)
> +
>  #define MII_KSZ9031RN_TX_DATA_PAD_SKEW	6
> +#define MII_KSZ9031RN_TXD3		GENMASK(15, 12)
> +#define MII_KSZ9031RN_TXD2		GENMASK(11, 8)
> +#define MII_KSZ9031RN_TXD1		GENMASK(7, 4)
> +#define MII_KSZ9031RN_TXD0		GENMASK(3, 0)
> +
>  #define MII_KSZ9031RN_CLK_PAD_SKEW	8
> +#define MII_KSZ9031RN_GTX_CLK		GENMASK(9, 5)
> +#define MII_KSZ9031RN_RX_CLK		GENMASK(4, 0)
> +
> +/* KSZ9031 has internal RGMII_IDRX = 1.2ns and RGMII_IDTX = 0ns. To
> + * provide different RGMII options we need to configure delay offset
> + * for each pad relative to build in delay.
> + */
> +/* keep rx as "No delay adjustment" and set rx_clk to +0.60ns to get delays of
> + * 1.80ns
> + */
> +#define RX_ID				0x7
> +#define RX_CLK_ID			0x19
> +
> +/* set rx to +0.30ns and rx_clk to -0.90ns to compensate the
> + * internal 1.2ns delay.
> + */
> +#define RX_ND				0xc
> +#define RX_CLK_ND			0x0
> +
> +/* set tx to -0.42ns and tx_clk to +0.96ns to get 1.38ns delay */
> +#define TX_ID				0x0
> +#define TX_CLK_ID			0x1f
> +
> +/* set tx and tx_clk to "No delay adjustment" to keep 0ns
> + * dealy
> + */
> +#define TX_ND				0x7
> +#define TX_CLK_ND			0xf
>  
>  /* MMD Address 0x1C */
>  #define MII_KSZ9031RN_EDPD		0x23
> @@ -501,7 +543,8 @@ static int ksz9021_config_init(struct phy_device *phydev)
>  static int ksz9031_of_load_skew_values(struct phy_device *phydev,
>  				       const struct device_node *of_node,
>  				       u16 reg, size_t field_sz,
> -				       const char *field[], u8 numfields)
> +				       const char *field[], u8 numfields,
> +				       bool *update)
>  {
>  	int val[4] = {-1, -2, -3, -4};
>  	int matches = 0;
> @@ -517,6 +560,8 @@ static int ksz9031_of_load_skew_values(struct phy_device *phydev,
>  	if (!matches)
>  		return 0;
>  
> +	*update |= true;
> +
>  	if (matches < numfields)
>  		newval = phy_read_mmd(phydev, 2, reg);
>  	else
> @@ -565,6 +610,67 @@ static int ksz9031_enable_edpd(struct phy_device *phydev)
>  			     reg | MII_KSZ9031RN_EDPD_ENABLE);
>  }
>  
> +static int ksz9031_config_rgmii_delay(struct phy_device *phydev)
> +{
> +	u16 rx, tx, rx_clk, tx_clk;
> +	int ret;
> +
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		tx = TX_ND;
> +		tx_clk = TX_CLK_ND;
> +		rx = RX_ND;
> +		rx_clk = RX_CLK_ND;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		tx = TX_ID;
> +		tx_clk = TX_CLK_ID;
> +		rx = RX_ID;
> +		rx_clk = RX_CLK_ID;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		tx = TX_ND;
> +		tx_clk = TX_CLK_ND;
> +		rx = RX_ID;
> +		rx_clk = RX_CLK_ID;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		tx = TX_ID;
> +		tx_clk = TX_CLK_ID;
> +		rx = RX_ND;
> +		rx_clk = RX_CLK_ND;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_CONTROL_PAD_SKEW,
> +			    FIELD_PREP(MII_KSZ9031RN_RX_CTL_M, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TX_CTL_M, tx));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_RX_DATA_PAD_SKEW,
> +			    FIELD_PREP(MII_KSZ9031RN_RXD3, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_RXD2, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_RXD1, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_RXD0, rx));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_TX_DATA_PAD_SKEW,
> +			    FIELD_PREP(MII_KSZ9031RN_TXD3, tx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TXD2, tx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TXD1, tx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TXD0, tx));
> +	if (ret < 0)
> +		return ret;
> +
> +	return phy_write_mmd(phydev, 2, MII_KSZ9031RN_CLK_PAD_SKEW,
> +			     FIELD_PREP(MII_KSZ9031RN_GTX_CLK, tx_clk) |
> +			     FIELD_PREP(MII_KSZ9031RN_RX_CLK, rx_clk));
> +}
> +
>  static int ksz9031_config_init(struct phy_device *phydev)
>  {
>  	const struct device *dev = &phydev->mdio.dev;
> @@ -597,21 +703,33 @@ static int ksz9031_config_init(struct phy_device *phydev)
>  	} while (!of_node && dev_walker);
>  
>  	if (of_node) {
> +		bool update = false;
> +
> +		if (phy_interface_is_rgmii(phydev)) {
> +			result = ksz9031_config_rgmii_delay(phydev);
> +			if (result < 0)
> +				return result;
> +		}
> +
>  		ksz9031_of_load_skew_values(phydev, of_node,
>  				MII_KSZ9031RN_CLK_PAD_SKEW, 5,
> -				clk_skews, 2);
> +				clk_skews, 2, &update);
>  
>  		ksz9031_of_load_skew_values(phydev, of_node,
>  				MII_KSZ9031RN_CONTROL_PAD_SKEW, 4,
> -				control_skews, 2);
> +				control_skews, 2, &update);
>  
>  		ksz9031_of_load_skew_values(phydev, of_node,
>  				MII_KSZ9031RN_RX_DATA_PAD_SKEW, 4,
> -				rx_data_skews, 4);
> +				rx_data_skews, 4, &update);
>  
>  		ksz9031_of_load_skew_values(phydev, of_node,
>  				MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4,
> -				tx_data_skews, 4);
> +				tx_data_skews, 4, &update);
> +
> +		if (update && phydev->interface != PHY_INTERFACE_MODE_RGMII)
> +			phydev_warn(phydev,
> +				    "*-skew-ps values should be used only with phy-mode = \"rgmii\"\n");
>  
>  		/* Silicon Errata Sheet (DS80000691D or DS80000692D):
>  		 * When the device links in the 1000BASE-T slave mode only,
> -- 
> 2.26.0.rc2
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-07-10 22:36 ` Alexandre Belloni
@ 2020-07-10 22:54   ` Andrew Lunn
  2020-07-10 23:25     ` Alexandre Belloni
  0 siblings, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2020-07-10 22:54 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Oleksij Rempel, Florian Fainelli, Heiner Kallweit, David Jander,
	David S. Miller, kernel, linux-kernel, netdev, Philippe Schenker,
	Russell King

On Sat, Jul 11, 2020 at 12:36:10AM +0200, Alexandre Belloni wrote:
> Hi Oleksij,
> 
> This patch breaks Ethernet on the sama5d3 Xplained and I have not been
> able to unbreak it.

Hi Alexandre

                        macb0: ethernet@f0028000 {
                                phy-mode = "rgmii";
                                #address-cells = <1>;
                                #size-cells = <0>;
                                status = "okay";

                                ethernet-phy@7 {
                                        reg = <0x7>;
                                };
                        };

So DT says it wants rgmii. How are the delays being added? Could the
bootloader be configuring the PHY into rgmii-id mode, which is now
getting cleared? Or by strapping of pins on the PHY?

Also, looking at macb_main.c is seen:

       if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
                val = 0;
                if (bp->phy_interface == PHY_INTERFACE_MODE_RGMII)
                        val = GEM_BIT(RGMII);
                else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
                         (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
                        val = MACB_BIT(RMII);
                else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
                        val = MACB_BIT(MII);

                if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
                        val |= MACB_BIT(CLKEN);

                macb_or_gem_writel(bp, USRIO, val);
        }

I don't know if this applies for your hardware, but if you tried
fixing the PHY by setting phy-mode to "rgmii-id", it could be macb
then did not set GEM_BIT(RGMII) and so broken even more?

Rather than bp->phy_interface == PHY_INTERFACE_MODE_RGMII,
phy_interface_mode_is_rgmii() might work better.

      Andrew

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-07-10 22:54   ` Andrew Lunn
@ 2020-07-10 23:25     ` Alexandre Belloni
  0 siblings, 0 replies; 30+ messages in thread
From: Alexandre Belloni @ 2020-07-10 23:25 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Oleksij Rempel, Florian Fainelli, Heiner Kallweit, David Jander,
	David S. Miller, kernel, linux-kernel, netdev, Philippe Schenker,
	Russell King

On 11/07/2020 00:54:53+0200, Andrew Lunn wrote:
> On Sat, Jul 11, 2020 at 12:36:10AM +0200, Alexandre Belloni wrote:
> > Hi Oleksij,
> > 
> > This patch breaks Ethernet on the sama5d3 Xplained and I have not been
> > able to unbreak it.
> 
> Hi Alexandre
> 
>                         macb0: ethernet@f0028000 {
>                                 phy-mode = "rgmii";
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
>                                 status = "okay";
> 
>                                 ethernet-phy@7 {
>                                         reg = <0x7>;
>                                 };
>                         };
> 
> So DT says it wants rgmii. How are the delays being added? Could the
> bootloader be configuring the PHY into rgmii-id mode, which is now
> getting cleared? Or by strapping of pins on the PHY?
> 
> Also, looking at macb_main.c is seen:
> 
>        if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
>                 val = 0;
>                 if (bp->phy_interface == PHY_INTERFACE_MODE_RGMII)
>                         val = GEM_BIT(RGMII);
>                 else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
>                          (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>                         val = MACB_BIT(RMII);
>                 else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>                         val = MACB_BIT(MII);
> 
>                 if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
>                         val |= MACB_BIT(CLKEN);
> 
>                 macb_or_gem_writel(bp, USRIO, val);
>         }
> 
> I don't know if this applies for your hardware, but if you tried
> fixing the PHY by setting phy-mode to "rgmii-id", it could be macb
> then did not set GEM_BIT(RGMII) and so broken even more?
> 

This is exactly what happens. I'll send patches. Thanks Andrew!

> Rather than bp->phy_interface == PHY_INTERFACE_MODE_RGMII,
> phy_interface_mode_is_rgmii() might work better.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-07  9:36 Oleksij Rempel
  2020-04-07 10:57 ` Philippe Schenker
@ 2020-04-07 20:13 ` David Miller
  1 sibling, 0 replies; 30+ messages in thread
From: David Miller @ 2020-04-07 20:13 UTC (permalink / raw)
  To: o.rempel
  Cc: andrew, f.fainelli, hkallweit1, david, kernel, linux-kernel,
	netdev, philippe.schenker, linux


Please resubmit when net-next opens back up, thank you.

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-07 11:02   ` Marc Kleine-Budde
@ 2020-04-07 12:34     ` Philippe Schenker
  0 siblings, 0 replies; 30+ messages in thread
From: Philippe Schenker @ 2020-04-07 12:34 UTC (permalink / raw)
  To: o.rempel, mkl, hkallweit1, f.fainelli, andrew
  Cc: linux, kernel, netdev, linux-kernel, david, davem

On Tue, 2020-04-07 at 13:02 +0200, Marc Kleine-Budde wrote:
> On 4/7/20 12:57 PM, Philippe Schenker wrote:
> > On Tue, 2020-04-07 at 11:36 +0200, Oleksij Rempel wrote:
> > > Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid,
> > > rgmii-rxid.
> > > 
> > > This PHY has an internal RX delay of 1.2ns and no delay for TX.
> > > 
> > > The pad skew registers allow to set the total TX delay to max
> > > 1.38ns
> > > and
> > > the total RX delay to max of 2.58ns (configurable 1.38ns + build
> > > in
> > > 1.2ns) and a minimal delay of 0ns.
> > > 
> > > According to the RGMII v1.3 specification the delay provided by
> > > PCB
> > > traces
> > > should be between 1.5ns and 2.0ns. The RGMII v2.0 allows to
> > > provide
> > > this
> > > delay by MAC or PHY. So, we configure this PHY to the best values
> > > we
> > > can
> > > get by this HW: TX delay to 1.38ns (max supported value) and RX
> > > delay
> > > to
> > > 1.80ns (best calculated delay)
> > > 
> > > The phy-modes can still be fine tuned/overwritten by *-skew-ps
> > > device tree properties described in:
> > > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > 
> > Make sure you do not exceet 80 chars with your phydev_warn. Besides
> > that:
> 
> Warning and Error strings should not be wrapped, so that you can
> better
> "grep" for them.

Didn't knew that, thanks!
> 
> regards,
> Marc
> 

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-07 10:57 ` Philippe Schenker
  2020-04-07 11:02   ` Marc Kleine-Budde
@ 2020-04-07 11:05   ` Russell King - ARM Linux admin
  1 sibling, 0 replies; 30+ messages in thread
From: Russell King - ARM Linux admin @ 2020-04-07 11:05 UTC (permalink / raw)
  To: Philippe Schenker
  Cc: o.rempel, hkallweit1, f.fainelli, andrew, kernel, davem, david,
	netdev, linux-kernel

On Tue, Apr 07, 2020 at 10:57:07AM +0000, Philippe Schenker wrote:
> On Tue, 2020-04-07 at 11:36 +0200, Oleksij Rempel wrote:
> > Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid,
> > rgmii-rxid.
> > 
> > This PHY has an internal RX delay of 1.2ns and no delay for TX.
> > 
> > The pad skew registers allow to set the total TX delay to max 1.38ns
> > and
> > the total RX delay to max of 2.58ns (configurable 1.38ns + build in
> > 1.2ns) and a minimal delay of 0ns.
> > 
> > According to the RGMII v1.3 specification the delay provided by PCB
> > traces
> > should be between 1.5ns and 2.0ns. The RGMII v2.0 allows to provide
> > this
> > delay by MAC or PHY. So, we configure this PHY to the best values we
> > can
> > get by this HW: TX delay to 1.38ns (max supported value) and RX delay
> > to
> > 1.80ns (best calculated delay)
> > 
> > The phy-modes can still be fine tuned/overwritten by *-skew-ps
> > device tree properties described in:
> > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> 
> Make sure you do not exceet 80 chars with your phydev_warn. Besides
> that:

There are exceptions to the 80 column rule. From coding-style.rst:

Statements longer than 80 columns will be broken into sensible chunks,
unless exceeding 80 columns significantly increases readability and
does not hide information. Descendants are always substantially shorter
than the parent and are placed substantially to the right. The same
applies to function headers with a long argument list. *However, never
break user-visible strings such as printk messages, because that breaks
the ability to grep for them.*

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-07 10:57 ` Philippe Schenker
@ 2020-04-07 11:02   ` Marc Kleine-Budde
  2020-04-07 12:34     ` Philippe Schenker
  2020-04-07 11:05   ` Russell King - ARM Linux admin
  1 sibling, 1 reply; 30+ messages in thread
From: Marc Kleine-Budde @ 2020-04-07 11:02 UTC (permalink / raw)
  To: Philippe Schenker, o.rempel, hkallweit1, f.fainelli, andrew
  Cc: netdev, linux-kernel, linux, kernel, david, davem

On 4/7/20 12:57 PM, Philippe Schenker wrote:
> On Tue, 2020-04-07 at 11:36 +0200, Oleksij Rempel wrote:
>> Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid,
>> rgmii-rxid.
>>
>> This PHY has an internal RX delay of 1.2ns and no delay for TX.
>>
>> The pad skew registers allow to set the total TX delay to max 1.38ns
>> and
>> the total RX delay to max of 2.58ns (configurable 1.38ns + build in
>> 1.2ns) and a minimal delay of 0ns.
>>
>> According to the RGMII v1.3 specification the delay provided by PCB
>> traces
>> should be between 1.5ns and 2.0ns. The RGMII v2.0 allows to provide
>> this
>> delay by MAC or PHY. So, we configure this PHY to the best values we
>> can
>> get by this HW: TX delay to 1.38ns (max supported value) and RX delay
>> to
>> 1.80ns (best calculated delay)
>>
>> The phy-modes can still be fine tuned/overwritten by *-skew-ps
>> device tree properties described in:
>> Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> 
> Make sure you do not exceet 80 chars with your phydev_warn. Besides
> that:

Warning and Error strings should not be wrapped, so that you can better
"grep" for them.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

* Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
  2020-04-07  9:36 Oleksij Rempel
@ 2020-04-07 10:57 ` Philippe Schenker
  2020-04-07 11:02   ` Marc Kleine-Budde
  2020-04-07 11:05   ` Russell King - ARM Linux admin
  2020-04-07 20:13 ` David Miller
  1 sibling, 2 replies; 30+ messages in thread
From: Philippe Schenker @ 2020-04-07 10:57 UTC (permalink / raw)
  To: o.rempel, hkallweit1, f.fainelli, andrew
  Cc: linux, kernel, davem, david, netdev, linux-kernel

On Tue, 2020-04-07 at 11:36 +0200, Oleksij Rempel wrote:
> Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid,
> rgmii-rxid.
> 
> This PHY has an internal RX delay of 1.2ns and no delay for TX.
> 
> The pad skew registers allow to set the total TX delay to max 1.38ns
> and
> the total RX delay to max of 2.58ns (configurable 1.38ns + build in
> 1.2ns) and a minimal delay of 0ns.
> 
> According to the RGMII v1.3 specification the delay provided by PCB
> traces
> should be between 1.5ns and 2.0ns. The RGMII v2.0 allows to provide
> this
> delay by MAC or PHY. So, we configure this PHY to the best values we
> can
> get by this HW: TX delay to 1.38ns (max supported value) and RX delay
> to
> 1.80ns (best calculated delay)
> 
> The phy-modes can still be fine tuned/overwritten by *-skew-ps
> device tree properties described in:
> Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Make sure you do not exceet 80 chars with your phydev_warn. Besides
that:

Reviewed-by: Philippe Schenker <philippe.schenker@toradex.com>

> ---
> changes v3:
> - change delay on RX line to 1.80ns
> - add warning if *-skew-ps properties are used together with not rgmii
>   mode. 
> 
> changes v2:
> - change RX_ID value from 0x1a to 0xa. The overflow bit was detected
> by
>   FIELD_PREP() build check.
>   Reported-by: kbuild test robot <lkp@intel.com>
> 
>  drivers/net/phy/micrel.c | 128 +++++++++++++++++++++++++++++++++++++-
> -
>  1 file changed, 123 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 05d20343b8161..045783eb4bc70 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -19,6 +19,7 @@
>   *			 ksz9477
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/phy.h>
> @@ -490,9 +491,50 @@ static int ksz9021_config_init(struct phy_device
> *phydev)
>  
>  /* MMD Address 0x2 */
>  #define MII_KSZ9031RN_CONTROL_PAD_SKEW	4
> +#define MII_KSZ9031RN_RX_CTL_M		GENMASK(7, 4)
> +#define MII_KSZ9031RN_TX_CTL_M		GENMASK(3, 0)
> +
>  #define MII_KSZ9031RN_RX_DATA_PAD_SKEW	5
> +#define MII_KSZ9031RN_RXD3		GENMASK(15, 12)
> +#define MII_KSZ9031RN_RXD2		GENMASK(11, 8)
> +#define MII_KSZ9031RN_RXD1		GENMASK(7, 4)
> +#define MII_KSZ9031RN_RXD0		GENMASK(3, 0)
> +
>  #define MII_KSZ9031RN_TX_DATA_PAD_SKEW	6
> +#define MII_KSZ9031RN_TXD3		GENMASK(15, 12)
> +#define MII_KSZ9031RN_TXD2		GENMASK(11, 8)
> +#define MII_KSZ9031RN_TXD1		GENMASK(7, 4)
> +#define MII_KSZ9031RN_TXD0		GENMASK(3, 0)
> +
>  #define MII_KSZ9031RN_CLK_PAD_SKEW	8
> +#define MII_KSZ9031RN_GTX_CLK		GENMASK(9, 5)
> +#define MII_KSZ9031RN_RX_CLK		GENMASK(4, 0)
> +
> +/* KSZ9031 has internal RGMII_IDRX = 1.2ns and RGMII_IDTX = 0ns. To
> + * provide different RGMII options we need to configure delay offset
> + * for each pad relative to build in delay.
> + */
> +/* keep rx as "No delay adjustment" and set rx_clk to +0.60ns to get
> delays of
> + * 1.80ns
> + */
> +#define RX_ID				0x7
> +#define RX_CLK_ID			0x19
> +
> +/* set rx to +0.30ns and rx_clk to -0.90ns to compensate the
> + * internal 1.2ns delay.
> + */
> +#define RX_ND				0xc
> +#define RX_CLK_ND			0x0
> +
> +/* set tx to -0.42ns and tx_clk to +0.96ns to get 1.38ns delay */
> +#define TX_ID				0x0
> +#define TX_CLK_ID			0x1f
> +
> +/* set tx and tx_clk to "No delay adjustment" to keep 0ns
> + * dealy
> + */
> +#define TX_ND				0x7
> +#define TX_CLK_ND			0xf
>  
>  /* MMD Address 0x1C */
>  #define MII_KSZ9031RN_EDPD		0x23
> @@ -501,7 +543,8 @@ static int ksz9021_config_init(struct phy_device
> *phydev)
>  static int ksz9031_of_load_skew_values(struct phy_device *phydev,
>  				       const struct device_node
> *of_node,
>  				       u16 reg, size_t field_sz,
> -				       const char *field[], u8
> numfields)
> +				       const char *field[], u8
> numfields,
> +				       bool *update)
>  {
>  	int val[4] = {-1, -2, -3, -4};
>  	int matches = 0;
> @@ -517,6 +560,8 @@ static int ksz9031_of_load_skew_values(struct
> phy_device *phydev,
>  	if (!matches)
>  		return 0;
>  
> +	*update |= true;
> +
>  	if (matches < numfields)
>  		newval = phy_read_mmd(phydev, 2, reg);
>  	else
> @@ -565,6 +610,67 @@ static int ksz9031_enable_edpd(struct phy_device
> *phydev)
>  			     reg | MII_KSZ9031RN_EDPD_ENABLE);
>  }
>  
> +static int ksz9031_config_rgmii_delay(struct phy_device *phydev)
> +{
> +	u16 rx, tx, rx_clk, tx_clk;
> +	int ret;
> +
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		tx = TX_ND;
> +		tx_clk = TX_CLK_ND;
> +		rx = RX_ND;
> +		rx_clk = RX_CLK_ND;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		tx = TX_ID;
> +		tx_clk = TX_CLK_ID;
> +		rx = RX_ID;
> +		rx_clk = RX_CLK_ID;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		tx = TX_ND;
> +		tx_clk = TX_CLK_ND;
> +		rx = RX_ID;
> +		rx_clk = RX_CLK_ID;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		tx = TX_ID;
> +		tx_clk = TX_CLK_ID;
> +		rx = RX_ND;
> +		rx_clk = RX_CLK_ND;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_CONTROL_PAD_SKEW,
> +			    FIELD_PREP(MII_KSZ9031RN_RX_CTL_M, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TX_CTL_M, tx));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_RX_DATA_PAD_SKEW,
> +			    FIELD_PREP(MII_KSZ9031RN_RXD3, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_RXD2, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_RXD1, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_RXD0, rx));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_TX_DATA_PAD_SKEW,
> +			    FIELD_PREP(MII_KSZ9031RN_TXD3, tx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TXD2, tx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TXD1, tx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TXD0, tx));
> +	if (ret < 0)
> +		return ret;
> +
> +	return phy_write_mmd(phydev, 2, MII_KSZ9031RN_CLK_PAD_SKEW,
> +			     FIELD_PREP(MII_KSZ9031RN_GTX_CLK, tx_clk) |
> +			     FIELD_PREP(MII_KSZ9031RN_RX_CLK, rx_clk));
> +}
> +
>  static int ksz9031_config_init(struct phy_device *phydev)
>  {
>  	const struct device *dev = &phydev->mdio.dev;
> @@ -597,21 +703,33 @@ static int ksz9031_config_init(struct phy_device
> *phydev)
>  	} while (!of_node && dev_walker);
>  
>  	if (of_node) {
> +		bool update = false;
> +
> +		if (phy_interface_is_rgmii(phydev)) {
> +			result = ksz9031_config_rgmii_delay(phydev);
> +			if (result < 0)
> +				return result;
> +		}
> +
>  		ksz9031_of_load_skew_values(phydev, of_node,
>  				MII_KSZ9031RN_CLK_PAD_SKEW, 5,
> -				clk_skews, 2);
> +				clk_skews, 2, &update);
>  
>  		ksz9031_of_load_skew_values(phydev, of_node,
>  				MII_KSZ9031RN_CONTROL_PAD_SKEW, 4,
> -				control_skews, 2);
> +				control_skews, 2, &update);
>  
>  		ksz9031_of_load_skew_values(phydev, of_node,
>  				MII_KSZ9031RN_RX_DATA_PAD_SKEW, 4,
> -				rx_data_skews, 4);
> +				rx_data_skews, 4, &update);
>  
>  		ksz9031_of_load_skew_values(phydev, of_node,
>  				MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4,
> -				tx_data_skews, 4);
> +				tx_data_skews, 4, &update);
> +
> +		if (update && phydev->interface !=
> PHY_INTERFACE_MODE_RGMII)
> +			phydev_warn(phydev,
> +				    "*-skew-ps values should be used
> only with phy-mode = \"rgmii\"\n");
>  
>  		/* Silicon Errata Sheet (DS80000691D or DS80000692D):
>  		 * When the device links in the 1000BASE-T slave mode
> only,

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

* [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY
@ 2020-04-07  9:36 Oleksij Rempel
  2020-04-07 10:57 ` Philippe Schenker
  2020-04-07 20:13 ` David Miller
  0 siblings, 2 replies; 30+ messages in thread
From: Oleksij Rempel @ 2020-04-07  9:36 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Heiner Kallweit
  Cc: Oleksij Rempel, David Jander, David S. Miller, kernel,
	linux-kernel, netdev, Philippe Schenker, Russell King

Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid, rgmii-rxid.

This PHY has an internal RX delay of 1.2ns and no delay for TX.

The pad skew registers allow to set the total TX delay to max 1.38ns and
the total RX delay to max of 2.58ns (configurable 1.38ns + build in
1.2ns) and a minimal delay of 0ns.

According to the RGMII v1.3 specification the delay provided by PCB traces
should be between 1.5ns and 2.0ns. The RGMII v2.0 allows to provide this
delay by MAC or PHY. So, we configure this PHY to the best values we can
get by this HW: TX delay to 1.38ns (max supported value) and RX delay to
1.80ns (best calculated delay)

The phy-modes can still be fine tuned/overwritten by *-skew-ps
device tree properties described in:
Documentation/devicetree/bindings/net/micrel-ksz90x1.txt

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
changes v3:
- change delay on RX line to 1.80ns
- add warning if *-skew-ps properties are used together with not rgmii
  mode. 

changes v2:
- change RX_ID value from 0x1a to 0xa. The overflow bit was detected by
  FIELD_PREP() build check.
  Reported-by: kbuild test robot <lkp@intel.com>

 drivers/net/phy/micrel.c | 128 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 123 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 05d20343b8161..045783eb4bc70 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -19,6 +19,7 @@
  *			 ksz9477
  */
 
+#include <linux/bitfield.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/phy.h>
@@ -490,9 +491,50 @@ static int ksz9021_config_init(struct phy_device *phydev)
 
 /* MMD Address 0x2 */
 #define MII_KSZ9031RN_CONTROL_PAD_SKEW	4
+#define MII_KSZ9031RN_RX_CTL_M		GENMASK(7, 4)
+#define MII_KSZ9031RN_TX_CTL_M		GENMASK(3, 0)
+
 #define MII_KSZ9031RN_RX_DATA_PAD_SKEW	5
+#define MII_KSZ9031RN_RXD3		GENMASK(15, 12)
+#define MII_KSZ9031RN_RXD2		GENMASK(11, 8)
+#define MII_KSZ9031RN_RXD1		GENMASK(7, 4)
+#define MII_KSZ9031RN_RXD0		GENMASK(3, 0)
+
 #define MII_KSZ9031RN_TX_DATA_PAD_SKEW	6
+#define MII_KSZ9031RN_TXD3		GENMASK(15, 12)
+#define MII_KSZ9031RN_TXD2		GENMASK(11, 8)
+#define MII_KSZ9031RN_TXD1		GENMASK(7, 4)
+#define MII_KSZ9031RN_TXD0		GENMASK(3, 0)
+
 #define MII_KSZ9031RN_CLK_PAD_SKEW	8
+#define MII_KSZ9031RN_GTX_CLK		GENMASK(9, 5)
+#define MII_KSZ9031RN_RX_CLK		GENMASK(4, 0)
+
+/* KSZ9031 has internal RGMII_IDRX = 1.2ns and RGMII_IDTX = 0ns. To
+ * provide different RGMII options we need to configure delay offset
+ * for each pad relative to build in delay.
+ */
+/* keep rx as "No delay adjustment" and set rx_clk to +0.60ns to get delays of
+ * 1.80ns
+ */
+#define RX_ID				0x7
+#define RX_CLK_ID			0x19
+
+/* set rx to +0.30ns and rx_clk to -0.90ns to compensate the
+ * internal 1.2ns delay.
+ */
+#define RX_ND				0xc
+#define RX_CLK_ND			0x0
+
+/* set tx to -0.42ns and tx_clk to +0.96ns to get 1.38ns delay */
+#define TX_ID				0x0
+#define TX_CLK_ID			0x1f
+
+/* set tx and tx_clk to "No delay adjustment" to keep 0ns
+ * dealy
+ */
+#define TX_ND				0x7
+#define TX_CLK_ND			0xf
 
 /* MMD Address 0x1C */
 #define MII_KSZ9031RN_EDPD		0x23
@@ -501,7 +543,8 @@ static int ksz9021_config_init(struct phy_device *phydev)
 static int ksz9031_of_load_skew_values(struct phy_device *phydev,
 				       const struct device_node *of_node,
 				       u16 reg, size_t field_sz,
-				       const char *field[], u8 numfields)
+				       const char *field[], u8 numfields,
+				       bool *update)
 {
 	int val[4] = {-1, -2, -3, -4};
 	int matches = 0;
@@ -517,6 +560,8 @@ static int ksz9031_of_load_skew_values(struct phy_device *phydev,
 	if (!matches)
 		return 0;
 
+	*update |= true;
+
 	if (matches < numfields)
 		newval = phy_read_mmd(phydev, 2, reg);
 	else
@@ -565,6 +610,67 @@ static int ksz9031_enable_edpd(struct phy_device *phydev)
 			     reg | MII_KSZ9031RN_EDPD_ENABLE);
 }
 
+static int ksz9031_config_rgmii_delay(struct phy_device *phydev)
+{
+	u16 rx, tx, rx_clk, tx_clk;
+	int ret;
+
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		tx = TX_ND;
+		tx_clk = TX_CLK_ND;
+		rx = RX_ND;
+		rx_clk = RX_CLK_ND;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		tx = TX_ID;
+		tx_clk = TX_CLK_ID;
+		rx = RX_ID;
+		rx_clk = RX_CLK_ID;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		tx = TX_ND;
+		tx_clk = TX_CLK_ND;
+		rx = RX_ID;
+		rx_clk = RX_CLK_ID;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		tx = TX_ID;
+		tx_clk = TX_CLK_ID;
+		rx = RX_ND;
+		rx_clk = RX_CLK_ND;
+		break;
+	default:
+		return 0;
+	}
+
+	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_CONTROL_PAD_SKEW,
+			    FIELD_PREP(MII_KSZ9031RN_RX_CTL_M, rx) |
+			    FIELD_PREP(MII_KSZ9031RN_TX_CTL_M, tx));
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_RX_DATA_PAD_SKEW,
+			    FIELD_PREP(MII_KSZ9031RN_RXD3, rx) |
+			    FIELD_PREP(MII_KSZ9031RN_RXD2, rx) |
+			    FIELD_PREP(MII_KSZ9031RN_RXD1, rx) |
+			    FIELD_PREP(MII_KSZ9031RN_RXD0, rx));
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_TX_DATA_PAD_SKEW,
+			    FIELD_PREP(MII_KSZ9031RN_TXD3, tx) |
+			    FIELD_PREP(MII_KSZ9031RN_TXD2, tx) |
+			    FIELD_PREP(MII_KSZ9031RN_TXD1, tx) |
+			    FIELD_PREP(MII_KSZ9031RN_TXD0, tx));
+	if (ret < 0)
+		return ret;
+
+	return phy_write_mmd(phydev, 2, MII_KSZ9031RN_CLK_PAD_SKEW,
+			     FIELD_PREP(MII_KSZ9031RN_GTX_CLK, tx_clk) |
+			     FIELD_PREP(MII_KSZ9031RN_RX_CLK, rx_clk));
+}
+
 static int ksz9031_config_init(struct phy_device *phydev)
 {
 	const struct device *dev = &phydev->mdio.dev;
@@ -597,21 +703,33 @@ static int ksz9031_config_init(struct phy_device *phydev)
 	} while (!of_node && dev_walker);
 
 	if (of_node) {
+		bool update = false;
+
+		if (phy_interface_is_rgmii(phydev)) {
+			result = ksz9031_config_rgmii_delay(phydev);
+			if (result < 0)
+				return result;
+		}
+
 		ksz9031_of_load_skew_values(phydev, of_node,
 				MII_KSZ9031RN_CLK_PAD_SKEW, 5,
-				clk_skews, 2);
+				clk_skews, 2, &update);
 
 		ksz9031_of_load_skew_values(phydev, of_node,
 				MII_KSZ9031RN_CONTROL_PAD_SKEW, 4,
-				control_skews, 2);
+				control_skews, 2, &update);
 
 		ksz9031_of_load_skew_values(phydev, of_node,
 				MII_KSZ9031RN_RX_DATA_PAD_SKEW, 4,
-				rx_data_skews, 4);
+				rx_data_skews, 4, &update);
 
 		ksz9031_of_load_skew_values(phydev, of_node,
 				MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4,
-				tx_data_skews, 4);
+				tx_data_skews, 4, &update);
+
+		if (update && phydev->interface != PHY_INTERFACE_MODE_RGMII)
+			phydev_warn(phydev,
+				    "*-skew-ps values should be used only with phy-mode = \"rgmii\"\n");
 
 		/* Silicon Errata Sheet (DS80000691D or DS80000692D):
 		 * When the device links in the 1000BASE-T slave mode only,
-- 
2.26.0.rc2


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

end of thread, other threads:[~2020-07-10 23:25 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-22  7:21 [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY Oleksij Rempel
2020-04-22  8:48 ` Philippe Schenker
2020-04-22 13:39 ` Andrew Lunn
2020-04-23  2:39 ` David Miller
2020-04-28 15:28 ` Geert Uytterhoeven
2020-04-28 15:47   ` Andrew Lunn
2020-04-28 16:16     ` Philippe Schenker
2020-04-29  8:45       ` Geert Uytterhoeven
2020-04-29  9:26         ` Oleksij Rempel
2020-05-27 19:11           ` Geert Uytterhoeven
2020-05-27 20:52             ` Andrew Lunn
2020-05-28 13:10               ` Geert Uytterhoeven
2020-05-28 16:08                 ` Andrew Lunn
2020-05-29  4:59                   ` Oleksij Rempel
2020-05-29 10:17                   ` Geert Uytterhoeven
2020-05-28  8:20             ` Philippe Schenker
2020-05-28 12:51               ` Geert Uytterhoeven
2020-05-28 23:31                 ` Florian Fainelli
2020-04-29 10:02         ` Philippe Schenker
2020-05-05 18:26 ` Grygorii Strashko
2020-05-06  4:51   ` Oleksij Rempel
2020-07-10 22:36 ` Alexandre Belloni
2020-07-10 22:54   ` Andrew Lunn
2020-07-10 23:25     ` Alexandre Belloni
  -- strict thread matches above, loose matches on Subject: below --
2020-04-07  9:36 Oleksij Rempel
2020-04-07 10:57 ` Philippe Schenker
2020-04-07 11:02   ` Marc Kleine-Budde
2020-04-07 12:34     ` Philippe Schenker
2020-04-07 11:05   ` Russell King - ARM Linux admin
2020-04-07 20:13 ` David Miller

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.