[PATCH net-next v2 0/4] RGMII Internal delay common property

[PATCH net-next v2 0/4] RGMII Internal delay common property

[Dan Murphy]

Hello

The RGMII internal delay is a common setting found in most RGMII capable PHY
devices.  It was found that many vendor specific device tree properties exist
to do the same function. This creates a common property to be used for PHY's
that have tunable internal delays for the Rx and Tx paths.

Dan Murphy (4):
  dt-bindings: net: Add tx and rx internal delays
  net: phy: Add a helper to return the index for of the internal delay
  dt-bindings: net: Add RGMII internal delay for DP83869
  net: dp83869: Add RGMII internal delay configuration

 .../bindings/net/ethernet-controller.yaml     |  14 +++
 .../devicetree/bindings/net/ti,dp83869.yaml   |  16 +++
 drivers/net/phy/dp83869.c                     | 101 ++++++++++++++++++
 drivers/net/phy/phy_device.c                  |  45 ++++++++
 include/linux/phy.h                           |   2 +
 5 files changed, 178 insertions(+)

-- 
2.26.2

[PATCH net-next v2 1/4] dt-bindings: net: Add tx and rx internal delays

[Dan Murphy]

tx-internal-delays and rx-internal-delays are a common setting for RGMII
capable devices.

These properties are used when the phy-mode or phy-controller is set to
rgmii-id, rgmii-rxid or rgmii-txid.  These modes indicate to the
controller that the PHY will add the internal delay for the connection.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---

v2 - updated to add -ps

 .../bindings/net/ethernet-controller.yaml          | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index ac471b60ed6a..70702a4ef5e8 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -143,6 +143,20 @@ properties:
       Specifies the PHY management type. If auto is set and fixed-link
       is not specified, it uses MDIO for management.
 
+  rx-internal-delay-ps:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description: |
+      RGMII Receive PHY Clock Delay defined in pico seconds.  This is used for
+      PHY's that have configurable RX internal delays.  This property is only
+      used when the phy-mode or phy-connection-type is rgmii-id or rgmii-rxid.
+
+  tx-internal-delay-ps:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description: |
+      RGMII Transmit PHY Clock Delay defined in pico seconds.  This is used for
+      PHY's that have configurable TX internal delays.  This property is only
+      used when the phy-mode or phy-connection-type is rgmii-id or rgmii-txid.
+
   fixed-link:
     allOf:
       - if:
-- 
2.26.2

[PATCH net-next v2 2/4] net: phy: Add a helper to return the index for of the internal delay

[Dan Murphy]

Add a helper function that will return the index in the array for the
passed in internal delay value.  The helper requires the array, size and
delay value.

The helper will then return the index for the exact match or return the
index for the index to the closest smaller value.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/phy_device.c | 45 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  2 ++
 2 files changed, 47 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7481135d27ab..40f53b379d2b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2661,6 +2661,51 @@ void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause)
 }
 EXPORT_SYMBOL(phy_get_pause);
 
+/**
+ * phy_get_delay_index - returns the index of the internal delay
+ * @phydev: phy_device struct
+ * @delay_values: array of delays the PHY supports
+ * @size: the size of the delay array
+ * @delay: the delay to be looked up
+ *
+ * Returns the index within the array of internal delay passed in.
+ */
+int phy_get_delay_index(struct phy_device *phydev, int *delay_values, int size,
+			int delay)
+{
+	int i;
+
+	if (size <= 0)
+		return -EINVAL;
+
+	if (delay <= delay_values[0])
+		return 0;
+
+	if (delay > delay_values[size - 1])
+		return size - 1;
+
+	for (i = 0; i < size; i++) {
+		if (delay == delay_values[i])
+			return i;
+
+		/* Find an approximate index by looking up the table */
+		if (delay > delay_values[i - 1] &&
+		    delay < delay_values[i]) {
+			if (delay - delay_values[i - 1] < delay_values[i] - delay)
+				return i - 1;
+			else
+				return i;
+		}
+
+	}
+
+	phydev_err(phydev, "error finding internal delay index for %d\n",
+		   delay);
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(phy_get_delay_index);
+
 static bool phy_drv_supports_irq(struct phy_driver *phydrv)
 {
 	return phydrv->config_intr && phydrv->ack_interrupt;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2bcdf19ed3b4..73552612c189 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1408,6 +1408,8 @@ void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
 bool phy_validate_pause(struct phy_device *phydev,
 			struct ethtool_pauseparam *pp);
 void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause);
+int phy_get_delay_index(struct phy_device *phydev, int *delay_values,
+			int size, int delay);
 void phy_resolve_pause(unsigned long *local_adv, unsigned long *partner_adv,
 		       bool *tx_pause, bool *rx_pause);
 
-- 
2.26.2

[PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Dan Murphy]

Add the internal delay values into the header and update the binding
with the internal delay properties.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../devicetree/bindings/net/ti,dp83869.yaml      | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83869.yaml b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
index 5b69ef03bbf7..2971dd3fc039 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83869.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
@@ -64,6 +64,20 @@ properties:
        Operational mode for the PHY.  If this is not set then the operational
        mode is set by the straps. see dt-bindings/net/ti-dp83869.h for values
 
+  rx-internal-delay-ps:
+    $ref: "#/properties/rx-internal-delay-ps"
+    description: Delay is in pico seconds
+    enum: [ 250, 500, 750, 1000, 1250, 1500, 1750, 2000, 2250, 2500, 2750, 3000,
+            3250, 3500, 3750, 4000 ]
+    default: 2000
+
+  tx-internal-delay-ps:
+    $ref: "#/properties/tx-internal-delay-ps"
+    description: Delay is in pico seconds
+    enum: [ 250, 500, 750, 1000, 1250, 1500, 1750, 2000, 2250, 2500, 2750, 3000,
+            3250, 3500, 3750, 4000 ]
+    default: 2000
+
 required:
   - reg
 
@@ -80,5 +94,7 @@ examples:
         ti,op-mode = <DP83869_RGMII_COPPER_ETHERNET>;
         ti,max-output-impedance = "true";
         ti,clk-output-sel = <DP83869_CLK_O_SEL_CHN_A_RCLK>;
+        rx-internal-delay-ps = <2000>;
+        tx-internal-delay-ps = <2000>;
       };
     };
-- 
2.26.2

[PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration

[Dan Murphy]

Add RGMII internal delay configuration for Rx and Tx.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/dp83869.c | 101 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index cfb22a21a2e6..a9008d32e2b6 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -99,6 +99,14 @@
 #define DP83869_OP_MODE_MII			BIT(5)
 #define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
 
+/* RGMIIDCTL bits */
+#define DP83869_RGMII_TX_CLK_DELAY_SHIFT	4
+#define DP83869_RGMII_CLK_DELAY_INV		0
+
+static int dp83869_internal_delay[] = {250, 500, 750, 1000, 1250, 1500, 1750,
+				       2000, 2250, 2500, 2750, 3000, 3250,
+				       3500, 3750, 4000};
+
 enum {
 	DP83869_PORT_MIRRORING_KEEP,
 	DP83869_PORT_MIRRORING_EN,
@@ -108,6 +116,8 @@ enum {
 struct dp83869_private {
 	int tx_fifo_depth;
 	int rx_fifo_depth;
+	u32 rx_id_delay;
+	u32 tx_id_delay;
 	int io_impedance;
 	int port_mirroring;
 	bool rxctrl_strap_quirk;
@@ -182,6 +192,7 @@ static int dp83869_of_init(struct phy_device *phydev)
 	struct dp83869_private *dp83869 = phydev->priv;
 	struct device *dev = &phydev->mdio.dev;
 	struct device_node *of_node = dev->of_node;
+	int delay_size = ARRAY_SIZE(dp83869_internal_delay);
 	int ret;
 
 	if (!of_node)
@@ -232,6 +243,26 @@ static int dp83869_of_init(struct phy_device *phydev)
 				 &dp83869->tx_fifo_depth))
 		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
 
+	dp83869->rx_id_delay = DP83869_RGMII_CLK_DELAY_INV;
+	ret = of_property_read_u32(of_node, "rx-internal-delay-ps",
+				   &dp83869->rx_id_delay);
+	if (!ret && dp83869->rx_id_delay > dp83869_internal_delay[delay_size]) {
+		phydev_err(phydev,
+			   "rx-internal-delay value of %u out of range\n",
+			   dp83869->rx_id_delay);
+		return -EINVAL;
+	}
+
+	dp83869->tx_id_delay = DP83869_RGMII_CLK_DELAY_INV;
+	ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
+				   &dp83869->tx_id_delay);
+	if (!ret && dp83869->tx_id_delay > dp83869_internal_delay[delay_size]) {
+		phydev_err(phydev,
+			   "tx-internal-delay value of %u out of range\n",
+			   dp83869->tx_id_delay);
+		return -EINVAL;
+	}
+
 	return ret;
 }
 #else
@@ -270,6 +301,29 @@ static int dp83869_configure_rgmii(struct phy_device *phydev,
 	return ret;
 }
 
+static int dp83869_verify_rgmii_cfg(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+
+	/* RX delay *must* be specified if internal delay of RX is used. */
+	if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	     phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) &&
+	     dp83869->rx_id_delay == DP83869_RGMII_CLK_DELAY_INV) {
+		phydev_err(phydev, "ti,rx-internal-delay must be specified\n");
+		return -EINVAL;
+	}
+
+	/* TX delay *must* be specified if internal delay of TX is used. */
+	if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	     phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) &&
+	     dp83869->tx_id_delay == DP83869_RGMII_CLK_DELAY_INV) {
+		phydev_err(phydev, "ti,tx-internal-delay must be specified\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int dp83869_configure_mode(struct phy_device *phydev,
 				  struct dp83869_private *dp83869)
 {
@@ -371,6 +425,12 @@ static int dp83869_config_init(struct phy_device *phydev)
 {
 	struct dp83869_private *dp83869 = phydev->priv;
 	int ret, val;
+	int delay_size = ARRAY_SIZE(dp83869_internal_delay);
+	int delay = 0;
+
+	ret = dp83869_verify_rgmii_cfg(phydev);
+	if (ret)
+		return ret;
 
 	ret = dp83869_configure_mode(phydev, dp83869);
 	if (ret)
@@ -394,6 +454,47 @@ static int dp83869_config_init(struct phy_device *phydev)
 				     dp83869->clk_output_sel <<
 				     DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT);
 
+	if (phy_interface_is_rgmii(phydev)) {
+		val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL);
+
+		val &= ~(DP83869_RGMII_TX_CLK_DELAY_EN | DP83869_RGMII_RX_CLK_DELAY_EN);
+		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
+			val |= (DP83869_RGMII_TX_CLK_DELAY_EN | DP83869_RGMII_RX_CLK_DELAY_EN);
+
+		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+			val |= DP83869_RGMII_TX_CLK_DELAY_EN;
+
+		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+			val |= DP83869_RGMII_RX_CLK_DELAY_EN;
+
+		phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL, val);
+
+		if (dp83869->rx_id_delay) {
+			val = phy_get_delay_index(phydev,
+						  &dp83869_internal_delay[0],
+						  delay_size,
+						  dp83869->rx_id_delay);
+			if (val < 0)
+				return val;
+
+			delay |= val;
+		}
+
+		if (dp83869->tx_id_delay) {
+			val = phy_get_delay_index(phydev,
+						  &dp83869_internal_delay[0],
+						  delay_size,
+						  dp83869->tx_id_delay);
+			if (val < 0)
+				return val;
+
+			delay |= val << DP83869_RGMII_TX_CLK_DELAY_SHIFT;
+		}
+
+		phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIIDCTL,
+			      delay);
+	}
+
 	return ret;
 }
 
-- 
2.26.2

Re: [PATCH net-next v2 2/4] net: phy: Add a helper to return the index for of the internal delay

[Florian Fainelli]

On 5/22/2020 5:25 AM, Dan Murphy wrote:
> Add a helper function that will return the index in the array for the
> passed in internal delay value.  The helper requires the array, size and
> delay value.
> 
> The helper will then return the index for the exact match or return the
> index for the index to the closest smaller value.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/net/phy/phy_device.c | 45 ++++++++++++++++++++++++++++++++++++
>  include/linux/phy.h          |  2 ++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 7481135d27ab..40f53b379d2b 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2661,6 +2661,51 @@ void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause)
>  }
>  EXPORT_SYMBOL(phy_get_pause);
>  
> +/**
> + * phy_get_delay_index - returns the index of the internal delay
> + * @phydev: phy_device struct
> + * @delay_values: array of delays the PHY supports
> + * @size: the size of the delay array
> + * @delay: the delay to be looked up
> + *
> + * Returns the index within the array of internal delay passed in.

Can we consider using s32 for storage that way the various
of_read_property_read_u32() are a natural fit (int works too, but I
would prefer being explicit).

> + */
> +int phy_get_delay_index(struct phy_device *phydev, int *delay_values, int size,
> +			int delay)
> +{
> +	int i;
> +
> +	if (size <= 0)
> +		return -EINVAL;
> +
> +	if (delay <= delay_values[0])
> +		return 0;
> +
> +	if (delay > delay_values[size - 1])
> +		return size - 1;

Does not that assume that the delays are sorted by ascending order, if
so, can you make it clear in the kernel doc?

> +
> +	for (i = 0; i < size; i++) {
> +		if (delay == delay_values[i])
> +			return i;
> +
> +		/* Find an approximate index by looking up the table */
> +		if (delay > delay_values[i - 1] &&

&& i > 0 so you do not accidentally under-run the array?

> +		    delay < delay_values[i]) {
> +			if (delay - delay_values[i - 1] < delay_values[i] - delay)
> +				return i - 1;
> +			else
> +				return i;
> +		}
> +
> +	}
> +
> +	phydev_err(phydev, "error finding internal delay index for %d\n",
> +		   delay);
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(phy_get_delay_index);
> +
>  static bool phy_drv_supports_irq(struct phy_driver *phydrv)
>  {
>  	return phydrv->config_intr && phydrv->ack_interrupt;
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 2bcdf19ed3b4..73552612c189 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1408,6 +1408,8 @@ void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
>  bool phy_validate_pause(struct phy_device *phydev,
>  			struct ethtool_pauseparam *pp);
>  void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause);
> +int phy_get_delay_index(struct phy_device *phydev, int *delay_values,
> +			int size, int delay);
>  void phy_resolve_pause(unsigned long *local_adv, unsigned long *partner_adv,
>  		       bool *tx_pause, bool *rx_pause);
>  
> 

-- 
Florian

Re: [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration

[Florian Fainelli]

On 5/22/2020 5:25 AM, Dan Murphy wrote:
> Add RGMII internal delay configuration for Rx and Tx.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/net/phy/dp83869.c | 101 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
> index cfb22a21a2e6..a9008d32e2b6 100644
> --- a/drivers/net/phy/dp83869.c
> +++ b/drivers/net/phy/dp83869.c
> @@ -99,6 +99,14 @@
>  #define DP83869_OP_MODE_MII			BIT(5)
>  #define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
>  
> +/* RGMIIDCTL bits */
> +#define DP83869_RGMII_TX_CLK_DELAY_SHIFT	4
> +#define DP83869_RGMII_CLK_DELAY_INV		0
> +
> +static int dp83869_internal_delay[] = {250, 500, 750, 1000, 1250, 1500, 1750,
> +				       2000, 2250, 2500, 2750, 3000, 3250,
> +				       3500, 3750, 4000};
> +
>  enum {
>  	DP83869_PORT_MIRRORING_KEEP,
>  	DP83869_PORT_MIRRORING_EN,
> @@ -108,6 +116,8 @@ enum {
>  struct dp83869_private {
>  	int tx_fifo_depth;
>  	int rx_fifo_depth;
> +	u32 rx_id_delay;
> +	u32 tx_id_delay;
>  	int io_impedance;
>  	int port_mirroring;
>  	bool rxctrl_strap_quirk;
> @@ -182,6 +192,7 @@ static int dp83869_of_init(struct phy_device *phydev)
>  	struct dp83869_private *dp83869 = phydev->priv;
>  	struct device *dev = &phydev->mdio.dev;
>  	struct device_node *of_node = dev->of_node;
> +	int delay_size = ARRAY_SIZE(dp83869_internal_delay);
>  	int ret;
>  
>  	if (!of_node)
> @@ -232,6 +243,26 @@ static int dp83869_of_init(struct phy_device *phydev)
>  				 &dp83869->tx_fifo_depth))
>  		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
>  
> +	dp83869->rx_id_delay = DP83869_RGMII_CLK_DELAY_INV;
> +	ret = of_property_read_u32(of_node, "rx-internal-delay-ps",
> +				   &dp83869->rx_id_delay);
> +	if (!ret && dp83869->rx_id_delay > dp83869_internal_delay[delay_size]) {
> +		phydev_err(phydev,
> +			   "rx-internal-delay value of %u out of range\n",
> +			   dp83869->rx_id_delay);
> +		return -EINVAL;
> +	}
> +
> +	dp83869->tx_id_delay = DP83869_RGMII_CLK_DELAY_INV;
> +	ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
> +				   &dp83869->tx_id_delay);
> +	if (!ret && dp83869->tx_id_delay > dp83869_internal_delay[delay_size]) {
> +		phydev_err(phydev,
> +			   "tx-internal-delay value of %u out of range\n",
> +			   dp83869->tx_id_delay);
> +		return -EINVAL;
> +	}

This is the kind of validation that I would be expecting from the PHY
library to do, in fact, since you use Device Tree standard property, I
would expect you only need to pass the maximum delay value and some
storage for your array of delays.

> +
>  	return ret;
>  }
>  #else
> @@ -270,6 +301,29 @@ static int dp83869_configure_rgmii(struct phy_device *phydev,
>  	return ret;
>  }
>  
> +static int dp83869_verify_rgmii_cfg(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869 = phydev->priv;
> +
> +	/* RX delay *must* be specified if internal delay of RX is used. */
> +	if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +	     phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) &&
> +	     dp83869->rx_id_delay == DP83869_RGMII_CLK_DELAY_INV) {
> +		phydev_err(phydev, "ti,rx-internal-delay must be specified\n");
> +		return -EINVAL;
> +	}
> +
> +	/* TX delay *must* be specified if internal delay of TX is used. */
> +	if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +	     phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) &&
> +	     dp83869->tx_id_delay == DP83869_RGMII_CLK_DELAY_INV) {
> +		phydev_err(phydev, "ti,tx-internal-delay must be specified\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int dp83869_configure_mode(struct phy_device *phydev,
>  				  struct dp83869_private *dp83869)
>  {
> @@ -371,6 +425,12 @@ static int dp83869_config_init(struct phy_device *phydev)
>  {
>  	struct dp83869_private *dp83869 = phydev->priv;
>  	int ret, val;
> +	int delay_size = ARRAY_SIZE(dp83869_internal_delay);
> +	int delay = 0;
> +
> +	ret = dp83869_verify_rgmii_cfg(phydev);
> +	if (ret)
> +		return ret;
>  
>  	ret = dp83869_configure_mode(phydev, dp83869);
>  	if (ret)
> @@ -394,6 +454,47 @@ static int dp83869_config_init(struct phy_device *phydev)
>  				     dp83869->clk_output_sel <<
>  				     DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT);
>  
> +	if (phy_interface_is_rgmii(phydev)) {
> +		val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL);
> +
> +		val &= ~(DP83869_RGMII_TX_CLK_DELAY_EN | DP83869_RGMII_RX_CLK_DELAY_EN);
> +		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> +			val |= (DP83869_RGMII_TX_CLK_DELAY_EN | DP83869_RGMII_RX_CLK_DELAY_EN);
> +
> +		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> +			val |= DP83869_RGMII_TX_CLK_DELAY_EN;
> +
> +		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
> +			val |= DP83869_RGMII_RX_CLK_DELAY_EN;
> +
> +		phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL, val);
> +
> +		if (dp83869->rx_id_delay) {
> +			val = phy_get_delay_index(phydev,
> +						  &dp83869_internal_delay[0],
> +						  delay_size,
> +						  dp83869->rx_id_delay);
> +			if (val < 0)
> +				return val;
> +
> +			delay |= val;

Don't you need to do a bitwise AND with the maximum delay value
supported by the range since you do a Read/Modify/Write operation here?

> +		}
> +
> +		if (dp83869->tx_id_delay) {
> +			val = phy_get_delay_index(phydev,
> +						  &dp83869_internal_delay[0],
> +						  delay_size,
> +						  dp83869->tx_id_delay);
> +			if (val < 0)
> +				return val;
> +
> +			delay |= val << DP83869_RGMII_TX_CLK_DELAY_SHIFT;

Likewise.

> +		}
> +
> +		phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIIDCTL,
> +			      delay);
> +	}
> +
>  	return ret;
>  }
>  
> 

-- 
Florian

Re: [PATCH net-next v2 2/4] net: phy: Add a helper to return the index for of the internal delay

[Dan Murphy]

Florian

On 5/22/20 11:11 AM, Florian Fainelli wrote:
>
> On 5/22/2020 5:25 AM, Dan Murphy wrote:
>> Add a helper function that will return the index in the array for the
>> passed in internal delay value.  The helper requires the array, size and
>> delay value.
>>
>> The helper will then return the index for the exact match or return the
>> index for the index to the closest smaller value.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   drivers/net/phy/phy_device.c | 45 ++++++++++++++++++++++++++++++++++++
>>   include/linux/phy.h          |  2 ++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 7481135d27ab..40f53b379d2b 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -2661,6 +2661,51 @@ void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause)
>>   }
>>   EXPORT_SYMBOL(phy_get_pause);
>>   
>> +/**
>> + * phy_get_delay_index - returns the index of the internal delay
>> + * @phydev: phy_device struct
>> + * @delay_values: array of delays the PHY supports
>> + * @size: the size of the delay array
>> + * @delay: the delay to be looked up
>> + *
>> + * Returns the index within the array of internal delay passed in.
> Can we consider using s32 for storage that way the various
> of_read_property_read_u32() are a natural fit (int works too, but I
> would prefer being explicit).

Ack

>
>> + */
>> +int phy_get_delay_index(struct phy_device *phydev, int *delay_values, int size,
>> +			int delay)
>> +{
>> +	int i;
>> +
>> +	if (size <= 0)
>> +		return -EINVAL;
>> +
>> +	if (delay <= delay_values[0])
>> +		return 0;
>> +
>> +	if (delay > delay_values[size - 1])
>> +		return size - 1;
> Does not that assume that the delays are sorted by ascending order, if
> so, can you make it clear in the kernel doc?

Yes I guess it does.  I can add this to the k doc


>
>> +
>> +	for (i = 0; i < size; i++) {
>> +		if (delay == delay_values[i])
>> +			return i;
>> +
>> +		/* Find an approximate index by looking up the table */
>> +		if (delay > delay_values[i - 1] &&
> && i > 0 so you do not accidentally under-run the array?

Yes and no it maybe better to start the for loop with i being 
initialized to 1 since the zeroth element is already validated above.

Dan

Re: [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration

[Dan Murphy]

Florian

On 5/22/20 11:13 AM, Florian Fainelli wrote:
>
> On 5/22/2020 5:25 AM, Dan Murphy wrote:
>> Add RGMII internal delay configuration for Rx and Tx.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   drivers/net/phy/dp83869.c | 101 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 101 insertions(+)
>>
>> diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
>> index cfb22a21a2e6..a9008d32e2b6 100644
>> --- a/drivers/net/phy/dp83869.c
>> +++ b/drivers/net/phy/dp83869.c
>> @@ -99,6 +99,14 @@
>>   #define DP83869_OP_MODE_MII			BIT(5)
>>   #define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
>>   
>> +/* RGMIIDCTL bits */
>> +#define DP83869_RGMII_TX_CLK_DELAY_SHIFT	4
>> +#define DP83869_RGMII_CLK_DELAY_INV		0
>> +
>> +static int dp83869_internal_delay[] = {250, 500, 750, 1000, 1250, 1500, 1750,
>> +				       2000, 2250, 2500, 2750, 3000, 3250,
>> +				       3500, 3750, 4000};
>> +
>>   enum {
>>   	DP83869_PORT_MIRRORING_KEEP,
>>   	DP83869_PORT_MIRRORING_EN,
>> @@ -108,6 +116,8 @@ enum {
>>   struct dp83869_private {
>>   	int tx_fifo_depth;
>>   	int rx_fifo_depth;
>> +	u32 rx_id_delay;
>> +	u32 tx_id_delay;
>>   	int io_impedance;
>>   	int port_mirroring;
>>   	bool rxctrl_strap_quirk;
>> @@ -182,6 +192,7 @@ static int dp83869_of_init(struct phy_device *phydev)
>>   	struct dp83869_private *dp83869 = phydev->priv;
>>   	struct device *dev = &phydev->mdio.dev;
>>   	struct device_node *of_node = dev->of_node;
>> +	int delay_size = ARRAY_SIZE(dp83869_internal_delay);
>>   	int ret;
>>   
>>   	if (!of_node)
>> @@ -232,6 +243,26 @@ static int dp83869_of_init(struct phy_device *phydev)
>>   				 &dp83869->tx_fifo_depth))
>>   		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
>>   
>> +	dp83869->rx_id_delay = DP83869_RGMII_CLK_DELAY_INV;
>> +	ret = of_property_read_u32(of_node, "rx-internal-delay-ps",
>> +				   &dp83869->rx_id_delay);
>> +	if (!ret && dp83869->rx_id_delay > dp83869_internal_delay[delay_size]) {
>> +		phydev_err(phydev,
>> +			   "rx-internal-delay value of %u out of range\n",
>> +			   dp83869->rx_id_delay);
>> +		return -EINVAL;
>> +	}
>> +
>> +	dp83869->tx_id_delay = DP83869_RGMII_CLK_DELAY_INV;
>> +	ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
>> +				   &dp83869->tx_id_delay);
>> +	if (!ret && dp83869->tx_id_delay > dp83869_internal_delay[delay_size]) {
>> +		phydev_err(phydev,
>> +			   "tx-internal-delay value of %u out of range\n",
>> +			   dp83869->tx_id_delay);
>> +		return -EINVAL;
>> +	}
> This is the kind of validation that I would be expecting from the PHY
> library to do, in fact, since you use Device Tree standard property, I
> would expect you only need to pass the maximum delay value and some
> storage for your array of delays.

Actually the PHY library will return either the 0th index if the value 
is to small or the max index if the value is to large

based on the array passed in so maybe this check is unnecessary.


>> +
>>   	return ret;
>>   }
>>   #else
>> @@ -270,6 +301,29 @@ static int dp83869_configure_rgmii(struct phy_device *phydev,
>>   	return ret;
>>   }
>>   
>> +static int dp83869_verify_rgmii_cfg(struct phy_device *phydev)
>> +{
>> +	struct dp83869_private *dp83869 = phydev->priv;
>> +
>> +	/* RX delay *must* be specified if internal delay of RX is used. */
>> +	if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
>> +	     phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) &&
>> +	     dp83869->rx_id_delay == DP83869_RGMII_CLK_DELAY_INV) {
>> +		phydev_err(phydev, "ti,rx-internal-delay must be specified\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* TX delay *must* be specified if internal delay of TX is used. */
>> +	if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
>> +	     phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) &&
>> +	     dp83869->tx_id_delay == DP83869_RGMII_CLK_DELAY_INV) {
>> +		phydev_err(phydev, "ti,tx-internal-delay must be specified\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int dp83869_configure_mode(struct phy_device *phydev,
>>   				  struct dp83869_private *dp83869)
>>   {
>> @@ -371,6 +425,12 @@ static int dp83869_config_init(struct phy_device *phydev)
>>   {
>>   	struct dp83869_private *dp83869 = phydev->priv;
>>   	int ret, val;
>> +	int delay_size = ARRAY_SIZE(dp83869_internal_delay);
>> +	int delay = 0;
>> +
>> +	ret = dp83869_verify_rgmii_cfg(phydev);
>> +	if (ret)
>> +		return ret;
>>   
>>   	ret = dp83869_configure_mode(phydev, dp83869);
>>   	if (ret)
>> @@ -394,6 +454,47 @@ static int dp83869_config_init(struct phy_device *phydev)
>>   				     dp83869->clk_output_sel <<
>>   				     DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT);
>>   
>> +	if (phy_interface_is_rgmii(phydev)) {
>> +		val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL);
>> +
>> +		val &= ~(DP83869_RGMII_TX_CLK_DELAY_EN | DP83869_RGMII_RX_CLK_DELAY_EN);
>> +		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
>> +			val |= (DP83869_RGMII_TX_CLK_DELAY_EN | DP83869_RGMII_RX_CLK_DELAY_EN);
>> +
>> +		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
>> +			val |= DP83869_RGMII_TX_CLK_DELAY_EN;
>> +
>> +		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
>> +			val |= DP83869_RGMII_RX_CLK_DELAY_EN;
>> +
>> +		phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL, val);
>> +
>> +		if (dp83869->rx_id_delay) {
>> +			val = phy_get_delay_index(phydev,
>> +						  &dp83869_internal_delay[0],
>> +						  delay_size,
>> +						  dp83869->rx_id_delay);
>> +			if (val < 0)
>> +				return val;
>> +
>> +			delay |= val;
> Don't you need to do a bitwise AND with the maximum delay value
> supported by the range since you do a Read/Modify/Write operation here?
There is not RMW here.  This is a write to the RGMIIDCTL register.
>
>> +		}
>> +
>> +		if (dp83869->tx_id_delay) {
>> +			val = phy_get_delay_index(phydev,
>> +						  &dp83869_internal_delay[0],
>> +						  delay_size,
>> +						  dp83869->tx_id_delay);
>> +			if (val < 0)
>> +				return val;
>> +
>> +			delay |= val << DP83869_RGMII_TX_CLK_DELAY_SHIFT;
> Likewise.

Same as above

Dan

Re: [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration

[Andrew Lunn]

> > > +	dp83869->tx_id_delay = DP83869_RGMII_CLK_DELAY_INV;
> > > +	ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
> > > +				   &dp83869->tx_id_delay);
> > > +	if (!ret && dp83869->tx_id_delay > dp83869_internal_delay[delay_size]) {
> > > +		phydev_err(phydev,
> > > +			   "tx-internal-delay value of %u out of range\n",
> > > +			   dp83869->tx_id_delay);
> > > +		return -EINVAL;
> > > +	}
> > This is the kind of validation that I would be expecting from the PHY
> > library to do, in fact, since you use Device Tree standard property, I
> > would expect you only need to pass the maximum delay value and some
> > storage for your array of delays.
> 
> Actually the PHY library will return either the 0th index if the value is to
> small or the max index if the value is to large
> 
> based on the array passed in so maybe this check is unnecessary.

Hi Dan

I'm not sure the helper is implementing the best behaviour. Rounded to
the nearest when within the supported range is O.K. But if the request
is outside the range, i would report an error.

Any why is your PHY special, in that is does care about out of range
delays, when others using new the new core helper don't?

	Andrew

Re: [PATCH net-next v2 1/4] dt-bindings: net: Add tx and rx internal delays

[Andrew Lunn]

On Fri, May 22, 2020 at 07:25:31AM -0500, Dan Murphy wrote:
> tx-internal-delays and rx-internal-delays are a common setting for RGMII
> capable devices.
> 
> These properties are used when the phy-mode or phy-controller is set to
> rgmii-id, rgmii-rxid or rgmii-txid.  These modes indicate to the
> controller that the PHY will add the internal delay for the connection.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v2 - updated to add -ps
> 
>  .../bindings/net/ethernet-controller.yaml          | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index ac471b60ed6a..70702a4ef5e8 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -143,6 +143,20 @@ properties:
>        Specifies the PHY management type. If auto is set and fixed-link
>        is not specified, it uses MDIO for management.
>  
> +  rx-internal-delay-ps:
> +    $ref: /schemas/types.yaml#definitions/uint32
> +    description: |
> +      RGMII Receive PHY Clock Delay defined in pico seconds.  This is used for
> +      PHY's that have configurable RX internal delays.  This property is only
> +      used when the phy-mode or phy-connection-type is rgmii-id or rgmii-rxid.

Hi Dan

Please add a comment about rounding to the nearest supported value.

    Andrew

Re: [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration

[Dan Murphy]

Andrew

On 5/23/20 10:09 AM, Andrew Lunn wrote:
>>>> +	dp83869->tx_id_delay = DP83869_RGMII_CLK_DELAY_INV;
>>>> +	ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
>>>> +				   &dp83869->tx_id_delay);
>>>> +	if (!ret && dp83869->tx_id_delay > dp83869_internal_delay[delay_size]) {
>>>> +		phydev_err(phydev,
>>>> +			   "tx-internal-delay value of %u out of range\n",
>>>> +			   dp83869->tx_id_delay);
>>>> +		return -EINVAL;
>>>> +	}
>>> This is the kind of validation that I would be expecting from the PHY
>>> library to do, in fact, since you use Device Tree standard property, I
>>> would expect you only need to pass the maximum delay value and some
>>> storage for your array of delays.
>> Actually the PHY library will return either the 0th index if the value is to
>> small or the max index if the value is to large
>>
>> based on the array passed in so maybe this check is unnecessary.
> Hi Dan
>
> I'm not sure the helper is implementing the best behaviour. Rounded to
> the nearest when within the supported range is O.K. But if the request
> is outside the range, i would report an error.

Hope this email does not bounce.

> Any why is your PHY special, in that is does care about out of range
> delays, when others using new the new core helper don't?

We are not rounding to nearest here.  Basically the helper works to find 
the best match

If the delay passed in is less than or equal to the smallest delay then 
return the smallest delay index

If the delay passed in is greater then the largest delay then return the 
max delay index

Not sure what you mean about this PHY being special.  This helper is not 
PHY specific.

After I think about this more I am thinking a helper may be over kill 
here and the delay to setting should be done within the PHY driver itself

Dan

> 	Andrew

Re: [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration

[Andrew Lunn]

> > Any why is your PHY special, in that is does care about out of range
> > delays, when others using new the new core helper don't?
> 
> We are not rounding to nearest here.  Basically the helper works to find the
> best match
> 
> If the delay passed in is less than or equal to the smallest delay then
> return the smallest delay index
> 
> If the delay passed in is greater then the largest delay then return the max
> delay index

+               /* Find an approximate index by looking up the table */
+               if (delay > delay_values[i - 1] &&
+                   delay < delay_values[i]) {
+                       if (delay - delay_values[i - 1] < delay_values[i] - delay)
+                               return i - 1;
+                       else
+                               return i;

This appears to round to the nearest value when it is not an exact
match.

The documentation is a hint to the DT developer what value to put in
DT. By saying it rounders, the developer does not need to go digging
through the source code to find an exact value, otherwise -EINVAL will
be returned. They can just use the value the HW engineer suggested,
and the PHY will pick whatever is nearest.

> Not sure what you mean about this PHY being special.  This helper is
> not PHY specific.

As you said, if out of range, the helper returns the top/bottom
value. Your PHY is special, the top/bottom value is not good enough,
you throw an error.

The point of helpers is to give uniform behaviour. We have one line
helpers, simply because they give uniform behaviour, rather than have
each driver do it subtlety different. But it also means drivers should
try to not add additional constraints over what the helper already
has, unless it is actually required by the hardware.

> After I think about this more I am thinking a helper may be over kill here
> and the delay to setting should be done within the PHY driver itself

The helper is useful, it will result in uniform handling of rounding
between DT values and what the PHY can actually do. But please also
move your range check and error message inside the helper.

     Andrew

Re: [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration

[Dan Murphy]

Andrew

On 5/23/20 5:07 PM, Andrew Lunn wrote:
>>> Any why is your PHY special, in that is does care about out of range
>>> delays, when others using new the new core helper don't?
>> We are not rounding to nearest here.  Basically the helper works to find the
>> best match
>>
>> If the delay passed in is less than or equal to the smallest delay then
>> return the smallest delay index
>>
>> If the delay passed in is greater then the largest delay then return the max
>> delay index
> +               /* Find an approximate index by looking up the table */
> +               if (delay > delay_values[i - 1] &&
> +                   delay < delay_values[i]) {
> +                       if (delay - delay_values[i - 1] < delay_values[i] - delay)
> +                               return i - 1;
> +                       else
> +                               return i;
>
> This appears to round to the nearest value when it is not an exact
> match.
>
> The documentation is a hint to the DT developer what value to put in
> DT. By saying it rounders, the developer does not need to go digging
> through the source code to find an exact value, otherwise -EINVAL will
> be returned. They can just use the value the HW engineer suggested,
> and the PHY will pick whatever is nearest.
>
>> Not sure what you mean about this PHY being special.  This helper is
>> not PHY specific.
> As you said, if out of range, the helper returns the top/bottom
> value. Your PHY is special, the top/bottom value is not good enough,
> you throw an error.
>
> The point of helpers is to give uniform behaviour. We have one line
> helpers, simply because they give uniform behaviour, rather than have
> each driver do it subtlety different. But it also means drivers should
> try to not add additional constraints over what the helper already
> has, unless it is actually required by the hardware.
>
>> After I think about this more I am thinking a helper may be over kill here
>> and the delay to setting should be done within the PHY driver itself
> The helper is useful, it will result in uniform handling of rounding
> between DT values and what the PHY can actually do. But please also
> move your range check and error message inside the helper.


I re-worked v3 to be a bit more of a helper and incorporated Florian's 
and you comments

Dan


>       Andrew

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Dan Murphy]

Andrew

On 5/20/20 3:44 PM, Andrew Lunn wrote:
>> I think adding it in the core would be a bit of a challenge.  I think each
>> PHY driver needs to handle parsing and validating this property on its own
>> (like fifo-depth).  It is a PHY specific setting.
> fifo-depth yes. But some delays follow a common pattern.
>
> e.g.
> Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
>
> Device Tree Value     Delay   Pad Skew Register Value
>    -----------------------------------------------------
>          0               -840ps          0000
>          200             -720ps          0001
>          400             -600ps          0010
>          600             -480ps          0011
>          800             -360ps          0100
>          1000            -240ps          0101
>          1200            -120ps          0110
>          1400               0ps          0111
>          1600             120ps          1000
>          1800             240ps          1001
>          2000             360ps          1010
>          2200             480ps          1011
>          2400             600ps          1100
>          2600             720ps          1101
>          2800             840ps          1110
>          3000             960ps          1111
>
> Documentation/devicetree/bindings/net/adi,adin.yaml
>
>   adi,rx-internal-delay-ps:
>      description: |
>        RGMII RX Clock Delay used only when PHY operates in RGMII mode with
>        internal delay (phy-mode is 'rgmii-id' or 'rgmii-rxid') in pico-seconds.
>      enum: [ 1600, 1800, 2000, 2200, 2400 ]
>      default: 2000
>
>    adi,tx-internal-delay-ps:
>      description: |
>        RGMII TX Clock Delay used only when PHY operates in RGMII mode with
>        internal delay (phy-mode is 'rgmii-id' or 'rgmii-txid') in pico-seconds.
>      enum: [ 1600, 1800, 2000, 2200, 2400 ]
>      default: 2000
>
> Documentation/devicetree/bindings/net/apm-xgene-enet.txt
>
> - tx-delay: Delay value for RGMII bridge TX clock.
>              Valid values are between 0 to 7, that maps to
>              417, 717, 1020, 1321, 1611, 1913, 2215, 2514 ps
>              Default value is 4, which corresponds to 1611 ps
> - rx-delay: Delay value for RGMII bridge RX clock.
>              Valid values are between 0 to 7, that maps to
>              273, 589, 899, 1222, 1480, 1806, 2147, 2464 ps
>              Default value is 2, which corresponds to 899 ps
>
> You could implement checking against a table of valid values, which is
> something you need for your PHY. You could even consider making it a
> 2D table, and return the register value, not the delay?

So provide a helper function that will just basically parse an array and 
return the indexed value.

The outlier here is the AD device since the index to value is not 1-1 
mapping.  Not sure we need a 2D table like the AD driver.

I actually implemented this in the dp83869 a bit ago and have done this 
in a few other non-PHY drivers.

I guess I can look at making it a utility function in the networking space.

Dan

>
> 	  Andrew

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Andrew Lunn]

> I think adding it in the core would be a bit of a challenge.  I think each
> PHY driver needs to handle parsing and validating this property on its own
> (like fifo-depth).  It is a PHY specific setting.

fifo-depth yes. But some delays follow a common pattern.

e.g.
Documentation/devicetree/bindings/net/micrel-ksz90x1.txt

Device Tree Value     Delay   Pad Skew Register Value
  -----------------------------------------------------
        0               -840ps          0000
        200             -720ps          0001
        400             -600ps          0010
        600             -480ps          0011
        800             -360ps          0100
        1000            -240ps          0101
        1200            -120ps          0110
        1400               0ps          0111
        1600             120ps          1000
        1800             240ps          1001
        2000             360ps          1010
        2200             480ps          1011
        2400             600ps          1100
        2600             720ps          1101
        2800             840ps          1110
        3000             960ps          1111

Documentation/devicetree/bindings/net/adi,adin.yaml

 adi,rx-internal-delay-ps:
    description: |
      RGMII RX Clock Delay used only when PHY operates in RGMII mode with
      internal delay (phy-mode is 'rgmii-id' or 'rgmii-rxid') in pico-seconds.
    enum: [ 1600, 1800, 2000, 2200, 2400 ]
    default: 2000

  adi,tx-internal-delay-ps:
    description: |
      RGMII TX Clock Delay used only when PHY operates in RGMII mode with
      internal delay (phy-mode is 'rgmii-id' or 'rgmii-txid') in pico-seconds.
    enum: [ 1600, 1800, 2000, 2200, 2400 ]
    default: 2000

Documentation/devicetree/bindings/net/apm-xgene-enet.txt

- tx-delay: Delay value for RGMII bridge TX clock.
            Valid values are between 0 to 7, that maps to
            417, 717, 1020, 1321, 1611, 1913, 2215, 2514 ps
            Default value is 4, which corresponds to 1611 ps
- rx-delay: Delay value for RGMII bridge RX clock.
            Valid values are between 0 to 7, that maps to
            273, 589, 899, 1222, 1480, 1806, 2147, 2464 ps
            Default value is 2, which corresponds to 899 ps

You could implement checking against a table of valid values, which is
something you need for your PHY. You could even consider making it a
2D table, and return the register value, not the delay?

	  Andrew

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Dan Murphy]

Andrew

On 5/20/20 2:27 PM, Andrew Lunn wrote:
> Hi Dan
>
>> UGH I think I just got volunteered to do make them common.
> There is code you can copy from PHY drivers. :-)
>
> What would be kind of nice is if the validate was in the core as
> well. Pass a list of possible delays in pS, and it will do a
> phydev_err() if what is in DT does not match one of the listed
> delays. Take a look around at what current drivers do and see if you
> can find a nice abstraction which will work for a few drivers. We
> cannot easily convert existing drivers without breaking DT, but a
> design which works in theory for what we currently have has a good
> chance of working for any new PHY driver.

I think adding it in the core would be a bit of a challenge.  I think 
each PHY driver needs to handle parsing and validating this property on 
its own (like fifo-depth).  It is a PHY specific setting.

Take the DP83867/9 and the ADIN1200/ADIN1300.

The 8386X devices has a delta granularity of 250pS and the AD devices is 
200pS per each setting

And the 867/9 has 3x more values (15) vs only 5 for the AD PHY.

And the Atheros AR803x PHY does use rgmii-id in the yaml, which I guess 
is what you were pointing out, that if set the PHY uses a default 2nS 
delay and it is not configurable.

Same with the Broadcomm.

Ack to not changing already existing drivers which is only 2 the AD PHY 
and the DP83867 PHY.  But I can update the yaml for the 83867 and mark 
the TI specific properties as deprecated in favor of the new properties 
like I did with fifo-depth.

Dan


>       Andrew

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Andrew Lunn]

Hi Dan

> UGH I think I just got volunteered to do make them common.

There is code you can copy from PHY drivers. :-)

What would be kind of nice is if the validate was in the core as
well. Pass a list of possible delays in pS, and it will do a
phydev_err() if what is in DT does not match one of the listed
delays. Take a look around at what current drivers do and see if you
can find a nice abstraction which will work for a few drivers. We
cannot easily convert existing drivers without breaking DT, but a
design which works in theory for what we currently have has a good
chance of working for any new PHY driver.

     Andrew

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Dan Murphy]

Florian

On 5/20/20 12:45 PM, Florian Fainelli wrote:
>
> On 5/20/2020 10:20 AM, Dan Murphy wrote:
>> Andrew/Florian
>>
>> On 5/20/20 11:43 AM, Andrew Lunn wrote:
>>>> I am interested in knowing where that is documented.  I want to RTM I
>>>> grepped for a few different words but came up empty
>>> Hi Dan
>>>
>>> It probably is not well documented, but one example would be
>>>
>>> Documentation/devicetree/bindings/net/ethernet-controller.yaml
>>>
>>> says:
>>>
>>>         # RX and TX delays are added by the MAC when required
>>>         - rgmii
>>>
>>>         # 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
>>>
>>>         # RGMII with internal RX delay provided by the PHY, the MAC
>>>         # should not add an RX delay in this case
>>>         - rgmii-rxid
>>>
>>>         # RGMII with internal TX delay provided by the PHY, the MAC
>>>         # should not add an TX delay in this case
>>>
>>>         Andrew
>> OKI I read that.  I also looked at a couple other drivers too.
>>
>> I am wondering if rx-internal-delay and tx-internal-delay should become
>> a common property like tx/rx fifo-depth
>>> And properly document how to use it or at least the expectation on use.
> Yes they should, and they should have an unit associated with the name.


UGH I think I just got volunteered to do make them common.

Dan

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Florian Fainelli]

On 5/20/2020 10:20 AM, Dan Murphy wrote:
> Andrew/Florian
> 
> On 5/20/20 11:43 AM, Andrew Lunn wrote:
>>> I am interested in knowing where that is documented.  I want to RTM I
>>> grepped for a few different words but came up empty
>> Hi Dan
>>
>> It probably is not well documented, but one example would be
>>
>> Documentation/devicetree/bindings/net/ethernet-controller.yaml
>>
>> says:
>>
>>        # RX and TX delays are added by the MAC when required
>>        - rgmii
>>
>>        # 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
>>
>>        # RGMII with internal RX delay provided by the PHY, the MAC
>>        # should not add an RX delay in this case
>>        - rgmii-rxid
>>
>>        # RGMII with internal TX delay provided by the PHY, the MAC
>>        # should not add an TX delay in this case
>>
>>        Andrew
> 
> OKI I read that.  I also looked at a couple other drivers too.
> 
> I am wondering if rx-internal-delay and tx-internal-delay should become
> a common property like tx/rx fifo-depth
> > And properly document how to use it or at least the expectation on use.

Yes they should, and they should have an unit associated with the name.
-- 
Florian

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Dan Murphy]

Andrew/Florian

On 5/20/20 11:43 AM, Andrew Lunn wrote:
>> I am interested in knowing where that is documented.  I want to RTM I
>> grepped for a few different words but came up empty
> Hi Dan
>
> It probably is not well documented, but one example would be
>
> Documentation/devicetree/bindings/net/ethernet-controller.yaml
>
> says:
>
>        # RX and TX delays are added by the MAC when required
>        - rgmii
>
>        # 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
>
>        # RGMII with internal RX delay provided by the PHY, the MAC
>        # should not add an RX delay in this case
>        - rgmii-rxid
>
>        # RGMII with internal TX delay provided by the PHY, the MAC
>        # should not add an TX delay in this case
>
>        Andrew

OKI I read that.  I also looked at a couple other drivers too.

I am wondering if rx-internal-delay and tx-internal-delay should become 
a common property like tx/rx fifo-depth

And properly document how to use it or at least the expectation on use.

Dan

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Andrew Lunn]

> I am interested in knowing where that is documented.  I want to RTM I
> grepped for a few different words but came up empty

Hi Dan

It probably is not well documented, but one example would be

Documentation/devicetree/bindings/net/ethernet-controller.yaml

says:

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

      # 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

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

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

      Andrew

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Florian Fainelli]

On 5/20/2020 8:56 AM, Dan Murphy wrote:
> Andrew
> 
> On 5/20/20 10:36 AM, Andrew Lunn wrote:
>>>> Hi Dan
>>>>
>>>> Having it required with PHY_INTERFACE_MODE_RGMII_ID or
>>>> PHY_INTERFACE_MODE_RGMII_RXID is pretty unusual. Normally these
>>>> properties are used to fine tune the delay, if the default of 2ns does
>>>> not work.
>>> Also if the MAC phy-mode is configured with RGMII-ID and no internal
>>> delay
>>> values defined wouldn't that be counter intuitive?
>> Most PHYs don't allow the delay to be fine tuned. You just pass for
>> example PHY_INTERFACE_MODE_RGMII_ID to the PHY driver and it enables a
>> 2ns delay. That is what people expect, and is documented.
> 
>> Being able to tune the delay is an optional extra, which some PHYs
>> support, but that is always above and beyond
>> PHY_INTERFACE_MODE_RGMII_ID.
> 
> I am interested in knowing where that is documented.  I want to RTM I
> grepped for a few different words but came up empty
> 
> Since this is a tuneable phy we need to program the ID.  2ns is the
> default value
> 
> Maybe I can change it from Required to Configurable or Used.

I do not think this is properly documented, it is an established
practice, but it should be clearly documented somewhere, I do not know
whether that belongs in the PHY Device Tree binding or if this belongs
to the PHY documentation.
-- 
Florian

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Dan Murphy]

Andrew

On 5/20/20 10:36 AM, Andrew Lunn wrote:
>>> Hi Dan
>>>
>>> Having it required with PHY_INTERFACE_MODE_RGMII_ID or
>>> PHY_INTERFACE_MODE_RGMII_RXID is pretty unusual. Normally these
>>> properties are used to fine tune the delay, if the default of 2ns does
>>> not work.
>> Also if the MAC phy-mode is configured with RGMII-ID and no internal delay
>> values defined wouldn't that be counter intuitive?
> Most PHYs don't allow the delay to be fine tuned. You just pass for
> example PHY_INTERFACE_MODE_RGMII_ID to the PHY driver and it enables a
> 2ns delay. That is what people expect, and is documented.

> Being able to tune the delay is an optional extra, which some PHYs
> support, but that is always above and beyond
> PHY_INTERFACE_MODE_RGMII_ID.

I am interested in knowing where that is documented.  I want to RTM I 
grepped for a few different words but came up empty

Since this is a tuneable phy we need to program the ID.  2ns is the 
default value

Maybe I can change it from Required to Configurable or Used.

Dan


>       Andrew

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Andrew Lunn]

> > Hi Dan
> > 
> > Having it required with PHY_INTERFACE_MODE_RGMII_ID or
> > PHY_INTERFACE_MODE_RGMII_RXID is pretty unusual. Normally these
> > properties are used to fine tune the delay, if the default of 2ns does
> > not work.
> 
> Also if the MAC phy-mode is configured with RGMII-ID and no internal delay
> values defined wouldn't that be counter intuitive?

Most PHYs don't allow the delay to be fine tuned. You just pass for
example PHY_INTERFACE_MODE_RGMII_ID to the PHY driver and it enables a
2ns delay. That is what people expect, and is documented.

Being able to tune the delay is an optional extra, which some PHYs
support, but that is always above and beyond
PHY_INTERFACE_MODE_RGMII_ID.

     Andrew

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Dan Murphy]

Andrew

On 5/20/20 10:28 AM, Dan Murphy wrote:
> Andrew
>
> On 5/20/20 8:56 AM, Andrew Lunn wrote:
>> On Wed, May 20, 2020 at 07:18:34AM -0500, Dan Murphy wrote:
>>> Add the internal delay values into the header and update the binding
>>> with the internal delay properties.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>>   .../devicetree/bindings/net/ti,dp83869.yaml    | 16 ++++++++++++++++
>>>   include/dt-bindings/net/ti-dp83869.h           | 18 
>>> ++++++++++++++++++
>>>   2 files changed, 34 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83869.yaml 
>>> b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
>>> index 5b69ef03bbf7..344015ab9081 100644
>>> --- a/Documentation/devicetree/bindings/net/ti,dp83869.yaml
>>> +++ b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
>>> @@ -64,6 +64,20 @@ properties:
>>>          Operational mode for the PHY.  If this is not set then the 
>>> operational
>>>          mode is set by the straps. see dt-bindings/net/ti-dp83869.h 
>>> for values
>>>   +  ti,rx-internal-delay:
>>> +    $ref: /schemas/types.yaml#definitions/uint32
>>> +    description: |
>>> +      RGMII Receive Clock Delay - see dt-bindings/net/ti-dp83869.h
>>> +      for applicable values. Required only if interface type is
>>> +      PHY_INTERFACE_MODE_RGMII_ID or PHY_INTERFACE_MODE_RGMII_RXID.
>> Hi Dan
>>
>> Having it required with PHY_INTERFACE_MODE_RGMII_ID or
>> PHY_INTERFACE_MODE_RGMII_RXID is pretty unusual. Normally these
>> properties are used to fine tune the delay, if the default of 2ns does
>> not work.
>
> Also if the MAC phy-mode is configured with RGMII-ID and no internal 
> delay values defined wouldn't that be counter intuitive?
>
> The driver will error out if the RGMII-ID is used and there was no 
> internal delay defined for either rx or tx making either one required.
>
> The MAC node needs to indicate to use the internal delay for RGMII 
> other wise the driver should ignore the internal delay programming as 
> these internal delays are not applicable to SGMII or MII modes.  The 
> RGMII mode can be used if the default 2ns delay is acceptable.
>
> Thus why we are documenting in the binding when the internal delay is 
> required as putting these under "required" is not correct.
>
> Dan
>
This is also the same for the DP83867 PHY as that PHY and the 83869 have 
a few identical features like internal delay, WoL and downshifting.

Dan


>>
>>      Andrew

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Dan Murphy]

Andrew

On 5/20/20 8:56 AM, Andrew Lunn wrote:
> On Wed, May 20, 2020 at 07:18:34AM -0500, Dan Murphy wrote:
>> Add the internal delay values into the header and update the binding
>> with the internal delay properties.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   .../devicetree/bindings/net/ti,dp83869.yaml    | 16 ++++++++++++++++
>>   include/dt-bindings/net/ti-dp83869.h           | 18 ++++++++++++++++++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83869.yaml b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
>> index 5b69ef03bbf7..344015ab9081 100644
>> --- a/Documentation/devicetree/bindings/net/ti,dp83869.yaml
>> +++ b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
>> @@ -64,6 +64,20 @@ properties:
>>          Operational mode for the PHY.  If this is not set then the operational
>>          mode is set by the straps. see dt-bindings/net/ti-dp83869.h for values
>>   
>> +  ti,rx-internal-delay:
>> +    $ref: /schemas/types.yaml#definitions/uint32
>> +    description: |
>> +      RGMII Receive Clock Delay - see dt-bindings/net/ti-dp83869.h
>> +      for applicable values. Required only if interface type is
>> +      PHY_INTERFACE_MODE_RGMII_ID or PHY_INTERFACE_MODE_RGMII_RXID.
> Hi Dan
>
> Having it required with PHY_INTERFACE_MODE_RGMII_ID or
> PHY_INTERFACE_MODE_RGMII_RXID is pretty unusual. Normally these
> properties are used to fine tune the delay, if the default of 2ns does
> not work.

Also if the MAC phy-mode is configured with RGMII-ID and no internal 
delay values defined wouldn't that be counter intuitive?

The driver will error out if the RGMII-ID is used and there was no 
internal delay defined for either rx or tx making either one required.

The MAC node needs to indicate to use the internal delay for RGMII other 
wise the driver should ignore the internal delay programming as these 
internal delays are not applicable to SGMII or MII modes.  The RGMII 
mode can be used if the default 2ns delay is acceptable.

Thus why we are documenting in the binding when the internal delay is 
required as putting these under "required" is not correct.

Dan

>
>      Andrew

Re: [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Andrew Lunn]

On Wed, May 20, 2020 at 07:18:34AM -0500, Dan Murphy wrote:
> Add the internal delay values into the header and update the binding
> with the internal delay properties.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  .../devicetree/bindings/net/ti,dp83869.yaml    | 16 ++++++++++++++++
>  include/dt-bindings/net/ti-dp83869.h           | 18 ++++++++++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,dp83869.yaml b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
> index 5b69ef03bbf7..344015ab9081 100644
> --- a/Documentation/devicetree/bindings/net/ti,dp83869.yaml
> +++ b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
> @@ -64,6 +64,20 @@ properties:
>         Operational mode for the PHY.  If this is not set then the operational
>         mode is set by the straps. see dt-bindings/net/ti-dp83869.h for values
>  
> +  ti,rx-internal-delay:
> +    $ref: /schemas/types.yaml#definitions/uint32
> +    description: |
> +      RGMII Receive Clock Delay - see dt-bindings/net/ti-dp83869.h
> +      for applicable values. Required only if interface type is
> +      PHY_INTERFACE_MODE_RGMII_ID or PHY_INTERFACE_MODE_RGMII_RXID.

Hi Dan

Having it required with PHY_INTERFACE_MODE_RGMII_ID or
PHY_INTERFACE_MODE_RGMII_RXID is pretty unusual. Normally these
properties are used to fine tune the delay, if the default of 2ns does
not work.

    Andrew

[PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Dan Murphy]

Add the internal delay values into the header and update the binding
with the internal delay properties.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../devicetree/bindings/net/ti,dp83869.yaml    | 16 ++++++++++++++++
 include/dt-bindings/net/ti-dp83869.h           | 18 ++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83869.yaml b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
index 5b69ef03bbf7..344015ab9081 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83869.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
@@ -64,6 +64,20 @@ properties:
        Operational mode for the PHY.  If this is not set then the operational
        mode is set by the straps. see dt-bindings/net/ti-dp83869.h for values
 
+  ti,rx-internal-delay:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description: |
+      RGMII Receive Clock Delay - see dt-bindings/net/ti-dp83869.h
+      for applicable values. Required only if interface type is
+      PHY_INTERFACE_MODE_RGMII_ID or PHY_INTERFACE_MODE_RGMII_RXID.
+
+  ti,tx-internal-delay:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description: |
+      RGMII Transmit Clock Delay - see dt-bindings/net/ti-dp83869.h
+      for applicable values. Required only if interface type is
+      PHY_INTERFACE_MODE_RGMII_ID or PHY_INTERFACE_MODE_RGMII_TXID.
+
 required:
   - reg
 
@@ -80,5 +94,7 @@ examples:
         ti,op-mode = <DP83869_RGMII_COPPER_ETHERNET>;
         ti,max-output-impedance = "true";
         ti,clk-output-sel = <DP83869_CLK_O_SEL_CHN_A_RCLK>;
+        ti,rx-internal-delay = <DP83869_RGMIIDCTL_2_25_NS>;
+        ti,tx-internal-delay = <DP83869_RGMIIDCTL_2_75_NS>;
       };
     };
diff --git a/include/dt-bindings/net/ti-dp83869.h b/include/dt-bindings/net/ti-dp83869.h
index 218b1a64e975..77d104a40f1f 100644
--- a/include/dt-bindings/net/ti-dp83869.h
+++ b/include/dt-bindings/net/ti-dp83869.h
@@ -16,6 +16,24 @@
 #define DP83869_PHYCR_FIFO_DEPTH_6_B_NIB	0x02
 #define DP83869_PHYCR_FIFO_DEPTH_8_B_NIB	0x03
 
+/* RGMIIDCTL internal delay for rx and tx */
+#define	DP83869_RGMIIDCTL_250_PS	0x0
+#define	DP83869_RGMIIDCTL_500_PS	0x1
+#define	DP83869_RGMIIDCTL_750_PS	0x2
+#define	DP83869_RGMIIDCTL_1_NS		0x3
+#define	DP83869_RGMIIDCTL_1_25_NS	0x4
+#define	DP83869_RGMIIDCTL_1_50_NS	0x5
+#define	DP83869_RGMIIDCTL_1_75_NS	0x6
+#define	DP83869_RGMIIDCTL_2_00_NS	0x7
+#define	DP83869_RGMIIDCTL_2_25_NS	0x8
+#define	DP83869_RGMIIDCTL_2_50_NS	0x9
+#define	DP83869_RGMIIDCTL_2_75_NS	0xa
+#define	DP83869_RGMIIDCTL_3_00_NS	0xb
+#define	DP83869_RGMIIDCTL_3_25_NS	0xc
+#define	DP83869_RGMIIDCTL_3_50_NS	0xd
+#define	DP83869_RGMIIDCTL_3_75_NS	0xe
+#define	DP83869_RGMIIDCTL_4_00_NS	0xf
+
 /* IO_MUX_CFG - Clock output selection */
 #define DP83869_CLK_O_SEL_CHN_A_RCLK		0x0
 #define DP83869_CLK_O_SEL_CHN_B_RCLK		0x1
-- 
2.26.2