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

[PATCH net-next v3 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                     | 91 +++++++++++++++++-
 drivers/net/phy/phy_device.c                  | 94 +++++++++++++++++++
 include/linux/phy.h                           |  2 +
 5 files changed, 214 insertions(+), 3 deletions(-)

-- 
2.26.2

[PATCH net-next v3 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>
---
 .../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 v3 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 | 94 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  2 +
 2 files changed, 96 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6b30d205642f..667b510c8810 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2661,6 +2661,100 @@ void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause)
 }
 EXPORT_SYMBOL(phy_get_pause);
 
+static int phy_find_descending_delay(struct phy_device *phydev,
+				     int *delay_values, int size, int int_delay)
+{
+	int i;
+
+	if (int_delay > delay_values[0] || int_delay < delay_values[size - 1]) {
+		phydev_err(phydev, "Delay %d is out of range\n", int_delay);
+		return -EINVAL;
+	}
+
+	if (int_delay == delay_values[0])
+		return 0;
+
+	for (i = 1; i < size; i++) {
+		if (int_delay == delay_values[i])
+			return i;
+
+		/* Find an approximate index by looking up the table */
+		if (int_delay < delay_values[i - 1] &&
+		    int_delay > delay_values[i]) {
+			if (int_delay - delay_values[i - 1] >
+			    delay_values[i] - int_delay)
+				return i + 1;
+			else
+				return i;
+		}
+	}
+
+	phydev_err(phydev, "error finding internal delay index for %d\n",
+		   int_delay);
+	return -EINVAL;
+}
+
+static int phy_find_ascending_delay(struct phy_device *phydev,
+				    int *delay_values, int size, int int_delay)
+{
+	int i;
+
+	if (int_delay < delay_values[0] || int_delay > delay_values[size - 1]) {
+		phydev_err(phydev, "Delay %d is out of range\n", int_delay);
+		return -EINVAL;
+	}
+
+	if (int_delay == delay_values[0])
+		return 0;
+
+	for (i = 1; i < size; i++) {
+		if (int_delay == delay_values[i])
+			return i;
+
+		/* Find an approximate index by looking up the table */
+		if (int_delay > delay_values[i - 1] &&
+		    int_delay < delay_values[i]) {
+			if (int_delay - delay_values[i - 1] <
+			    delay_values[i] - int_delay)
+				return i - 1;
+			else
+				return i;
+		}
+	}
+
+	phydev_err(phydev, "error finding internal delay index for %d\n",
+		   int_delay);
+	return -EINVAL;
+}
+
+/**
+ * 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
+ * @int_delay: the internal delay to be looked up
+ * @descending: if the delay array is in descending order
+ *
+ * Returns the index within the array of internal delay passed in.
+ * Return errno if the delay is invalid or cannot be found.
+ */
+s32 phy_get_delay_index(struct phy_device *phydev, int *delay_values, int size,
+			int int_delay, bool descending)
+{
+	if (int_delay < 0)
+		return -EINVAL;
+
+	if (size <= 0)
+		return -EINVAL;
+
+	if (descending)
+		return phy_find_descending_delay(phydev, delay_values, size,
+						 int_delay);
+
+	return phy_find_ascending_delay(phydev, delay_values, size, int_delay);
+}
+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..1ba2c132b635 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, bool descending);
 void phy_resolve_pause(unsigned long *local_adv, unsigned long *partner_adv,
 		       bool *tx_pause, bool *rx_pause);
 
-- 
2.26.2

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

[Andrew Lunn]

> +/**
> + * 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
> + * @int_delay: the internal delay to be looked up
> + * @descending: if the delay array is in descending order
> + *
> + * Returns the index within the array of internal delay passed in.
> + * Return errno if the delay is invalid or cannot be found.
> + */
> +s32 phy_get_delay_index(struct phy_device *phydev, int *delay_values, int size,
> +			int int_delay, bool descending)
> +{
> +	if (int_delay < 0)
> +		return -EINVAL;
> +
> +	if (size <= 0)
> +		return -EINVAL;
> +
> +	if (descending)
> +		return phy_find_descending_delay(phydev, delay_values, size,
> +						 int_delay);
> +
> +	return phy_find_ascending_delay(phydev, delay_values, size, int_delay);
> +}
> +EXPORT_SYMBOL(phy_get_delay_index);

Do we really need this ascending vs descending? This array is not
coming from device tree of anything, it is a static list in the PHY
driver. I would just define it needs to be ascending and be done.

	Andrew

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

[Dan Murphy]

Andrew

On 5/26/20 7:42 PM, Andrew Lunn wrote:
>> +/**
>> + * 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
>> + * @int_delay: the internal delay to be looked up
>> + * @descending: if the delay array is in descending order
>> + *
>> + * Returns the index within the array of internal delay passed in.
>> + * Return errno if the delay is invalid or cannot be found.
>> + */
>> +s32 phy_get_delay_index(struct phy_device *phydev, int *delay_values, int size,
>> +			int int_delay, bool descending)
>> +{
>> +	if (int_delay < 0)
>> +		return -EINVAL;
>> +
>> +	if (size <= 0)
>> +		return -EINVAL;
>> +
>> +	if (descending)
>> +		return phy_find_descending_delay(phydev, delay_values, size,
>> +						 int_delay);
>> +
>> +	return phy_find_ascending_delay(phydev, delay_values, size, int_delay);
>> +}
>> +EXPORT_SYMBOL(phy_get_delay_index);
> Do we really need this ascending vs descending? This array is not
> coming from device tree of anything, it is a static list in the PHY
> driver. I would just define it needs to be ascending and be done.

I was thinking about the constraints of having just an ascending array 
helper.

If there is a PHY out there that has a descending delay array then this 
function is not a helper.

Then the PHY driver now has to implement a descending search or extend 
out this helper to do the same.

I can just keep it ascending for now but this helper may need to be 
updated in the future to accommodate any PHYs with descending delay arrays.

Dan


> 	Andrew

[PATCH net-next v3 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 v3 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 | 91 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 88 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index cfb22a21a2e6..1c440e0c0d64 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -64,6 +64,9 @@
 #define DP83869_RGMII_TX_CLK_DELAY_EN		BIT(1)
 #define DP83869_RGMII_RX_CLK_DELAY_EN		BIT(0)
 
+/* RGMIIDCTL */
+#define DP83869_RGMII_CLK_DELAY_SHIFT		4
+
 /* STRAP_STS1 bits */
 #define DP83869_STRAP_OP_MODE_MASK		GENMASK(2, 0)
 #define DP83869_STRAP_STS1_RESERVED		BIT(11)
@@ -78,9 +81,6 @@
 #define DP83869_PHYCR_FIFO_DEPTH_MASK	GENMASK(15, 12)
 #define DP83869_PHYCR_RESERVED_MASK	BIT(11)
 
-/* RGMIIDCTL bits */
-#define DP83869_RGMII_TX_CLK_DELAY_SHIFT	4
-
 /* IO_MUX_CFG bits */
 #define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL	0x1f
 
@@ -99,6 +99,10 @@
 #define DP83869_OP_MODE_MII			BIT(5)
 #define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
 
+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 +112,8 @@ enum {
 struct dp83869_private {
 	int tx_fifo_depth;
 	int rx_fifo_depth;
+	s32 rx_id_delay;
+	s32 tx_id_delay;
 	int io_impedance;
 	int port_mirroring;
 	bool rxctrl_strap_quirk;
@@ -218,6 +224,7 @@ static int dp83869_of_init(struct phy_device *phydev)
 		ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
 		if (ret < 0)
 			return ret;
+
 		if (ret & DP83869_STRAP_MIRROR_ENABLED)
 			dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
 		else
@@ -232,6 +239,20 @@ static int dp83869_of_init(struct phy_device *phydev)
 				 &dp83869->tx_fifo_depth))
 		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
 
+	ret = of_property_read_u32(of_node, "rx-internal-delay-ps",
+				   &dp83869->rx_id_delay);
+	if (ret) {
+		dp83869->rx_id_delay = ret;
+		ret = 0;
+	}
+
+	ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
+				   &dp83869->tx_id_delay);
+	if (ret) {
+		dp83869->tx_id_delay = ret;
+		ret = 0;
+	}
+
 	return ret;
 }
 #else
@@ -367,10 +388,45 @@ static int dp83869_configure_mode(struct phy_device *phydev,
 	return ret;
 }
 
+static int dp83869_get_delay(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	int delay_size = ARRAY_SIZE(dp83869_internal_delay);
+	int tx_delay = 0;
+	int rx_delay = 0;
+
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
+	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
+		tx_delay = phy_get_delay_index(phydev,
+					       &dp83869_internal_delay[0],
+					       delay_size, dp83869->tx_id_delay,
+					       false);
+		if (tx_delay < 0) {
+			phydev_err(phydev, "Tx internal delay is invalid\n");
+			return tx_delay;
+		}
+	}
+
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
+	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
+		rx_delay = phy_get_delay_index(phydev,
+					       &dp83869_internal_delay[0],
+					       delay_size, dp83869->rx_id_delay,
+					       false);
+		if (rx_delay < 0) {
+			phydev_err(phydev, "Rx internal delay is invalid\n");
+			return rx_delay;
+		}
+	}
+
+	return rx_delay | tx_delay << DP83869_RGMII_CLK_DELAY_SHIFT;
+}
+
 static int dp83869_config_init(struct phy_device *phydev)
 {
 	struct dp83869_private *dp83869 = phydev->priv;
 	int ret, val;
+	int delay;
 
 	ret = dp83869_configure_mode(phydev, dp83869);
 	if (ret)
@@ -394,6 +450,35 @@ 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)) {
+		ret = dp83869_get_delay(phydev);
+		if (ret < 0)
+			return ret;
+
+		delay = ret;
+		ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIIDCTL,
+				    delay);
+		if (ret)
+			return ret;
+
+		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;
+
+		ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL,
+				    val);
+	}
+
 	return ret;
 }
 
-- 
2.26.2

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

[Andrew Lunn]

> @@ -218,6 +224,7 @@ static int dp83869_of_init(struct phy_device *phydev)
>  		ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
>  		if (ret < 0)
>  			return ret;
> +
>  		if (ret & DP83869_STRAP_MIRROR_ENABLED)
>  			dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
>  		else

This random white space change does not belong in this patch.

> @@ -232,6 +239,20 @@ static int dp83869_of_init(struct phy_device *phydev)
>  				 &dp83869->tx_fifo_depth))
>  		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
>  
> +	ret = of_property_read_u32(of_node, "rx-internal-delay-ps",
> +				   &dp83869->rx_id_delay);
> +	if (ret) {
> +		dp83869->rx_id_delay = ret;
> +		ret = 0;
> +	}

This looks odd.

If this optional property is not found, -EINVAL will be returned. It
could also return -ENODATA. You then assign this error value to
dp83869->rx_id_delay? I would of expected you to assign 2000, the
default value?

> +
> +	ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
> +				   &dp83869->tx_id_delay);
> +	if (ret) {
> +		dp83869->tx_id_delay = ret;
> +		ret = 0;
> +	}
> +
>  	return ret;
>  }
>  #else
> @@ -367,10 +388,45 @@ static int dp83869_configure_mode(struct phy_device *phydev,
>  	return ret;
>  }
>  
> +static int dp83869_get_delay(struct phy_device *phydev)
> +{
> +	struct dp83869_private *dp83869 = phydev->priv;
> +	int delay_size = ARRAY_SIZE(dp83869_internal_delay);
> +	int tx_delay = 0;
> +	int rx_delay = 0;
> +
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
> +		tx_delay = phy_get_delay_index(phydev,
> +					       &dp83869_internal_delay[0],
> +					       delay_size, dp83869->tx_id_delay,
> +					       false);
> +		if (tx_delay < 0) {
> +			phydev_err(phydev, "Tx internal delay is invalid\n");
> +			return tx_delay;
> +		}
> +	}
> +
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
> +		rx_delay = phy_get_delay_index(phydev,
> +					       &dp83869_internal_delay[0],
> +					       delay_size, dp83869->rx_id_delay,
> +					       false);
> +		if (rx_delay < 0) {
> +			phydev_err(phydev, "Rx internal delay is invalid\n");
> +			return rx_delay;
> +		}
> +	}

So any PHY using these properties is going to pretty much reproduce
this code. Meaning is should all be in a helper.

     Andrew

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

[Dan Murphy]

Andrew

On 5/26/20 7:52 PM, Andrew Lunn wrote:
>> @@ -218,6 +224,7 @@ static int dp83869_of_init(struct phy_device *phydev)
>>   		ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
>>   		if (ret < 0)
>>   			return ret;
>> +
>>   		if (ret & DP83869_STRAP_MIRROR_ENABLED)
>>   			dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
>>   		else
> This random white space change does not belong in this patch.

OK


>> @@ -232,6 +239,20 @@ static int dp83869_of_init(struct phy_device *phydev)
>>   				 &dp83869->tx_fifo_depth))
>>   		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
>>   
>> +	ret = of_property_read_u32(of_node, "rx-internal-delay-ps",
>> +				   &dp83869->rx_id_delay);
>> +	if (ret) {
>> +		dp83869->rx_id_delay = ret;
>> +		ret = 0;
>> +	}
> This looks odd.
>
> If this optional property is not found, -EINVAL will be returned. It
> could also return -ENODATA. You then assign this error value to
> dp83869->rx_id_delay? I would of expected you to assign 2000, the
> default value?

Well the driver cannot assume this is the intended value.

If the dt defines rgmii-rx/tx-id then these values are required not 
optional.  That was the discussion on the binding.

I set these to errno because when config_init is called the driver 
verifies that the values are valid and present and if they

are not then the PHY will fail to init.

If we set the delay to default then the PHY may be programmed with the 
wrong delay.


>> +
>> +	ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
>> +				   &dp83869->tx_id_delay);
>> +	if (ret) {
>> +		dp83869->tx_id_delay = ret;
>> +		ret = 0;
>> +	}
>> +
>>   	return ret;
>>   }
>>   #else
>> @@ -367,10 +388,45 @@ static int dp83869_configure_mode(struct phy_device *phydev,
>>   	return ret;
>>   }
>>   
>> +static int dp83869_get_delay(struct phy_device *phydev)
>> +{
>> +	struct dp83869_private *dp83869 = phydev->priv;
>> +	int delay_size = ARRAY_SIZE(dp83869_internal_delay);
>> +	int tx_delay = 0;
>> +	int rx_delay = 0;
>> +
>> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
>> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
>> +		tx_delay = phy_get_delay_index(phydev,
>> +					       &dp83869_internal_delay[0],
>> +					       delay_size, dp83869->tx_id_delay,
>> +					       false);
>> +		if (tx_delay < 0) {
>> +			phydev_err(phydev, "Tx internal delay is invalid\n");
>> +			return tx_delay;
>> +		}
>> +	}
>> +
>> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
>> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
>> +		rx_delay = phy_get_delay_index(phydev,
>> +					       &dp83869_internal_delay[0],
>> +					       delay_size, dp83869->rx_id_delay,
>> +					       false);
>> +		if (rx_delay < 0) {
>> +			phydev_err(phydev, "Rx internal delay is invalid\n");
>> +			return rx_delay;
>> +		}
>> +	}
> So any PHY using these properties is going to pretty much reproduce
> this code. Meaning is should all be in a helper.

The issue here is that the phy_mode may only be rgmii-txid so you only 
want to find the tx_delay and return.

Same with the RXID.  How is the helper supposed to know what delay to 
return and look for?

The PHY also only needs to use the helper if the PHY is in certain modes.

And the decision to use the checks is really based on the PHY driver.

Not sure if other PHYs delays require both delays to be set or if the 
delays are independent.

The helper cannot assume this.

Dan


>
>       Andrew

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

[Andrew Lunn]

> If the dt defines rgmii-rx/tx-id then these values are required not
> optional.  That was the discussion on the binding.

How many times do i need to say it. They are optional. If not
specified, default to 2ns.

> > > +	ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
> > > +				   &dp83869->tx_id_delay);
> > > +	if (ret) {
> > > +		dp83869->tx_id_delay = ret;
> > > +		ret = 0;
> > > +	}
> > > +
> > >   	return ret;
> > >   }
> > >   #else
> > > @@ -367,10 +388,45 @@ static int dp83869_configure_mode(struct phy_device *phydev,
> > >   	return ret;
> > >   }
> > > +static int dp83869_get_delay(struct phy_device *phydev)
> > > +{
> > > +	struct dp83869_private *dp83869 = phydev->priv;
> > > +	int delay_size = ARRAY_SIZE(dp83869_internal_delay);
> > > +	int tx_delay = 0;
> > > +	int rx_delay = 0;
> > > +
> > > +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> > > +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
> > > +		tx_delay = phy_get_delay_index(phydev,
> > > +					       &dp83869_internal_delay[0],
> > > +					       delay_size, dp83869->tx_id_delay,
> > > +					       false);
> > > +		if (tx_delay < 0) {
> > > +			phydev_err(phydev, "Tx internal delay is invalid\n");
> > > +			return tx_delay;
> > > +		}
> > > +	}
> > > +
> > > +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
> > > +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
> > > +		rx_delay = phy_get_delay_index(phydev,
> > > +					       &dp83869_internal_delay[0],
> > > +					       delay_size, dp83869->rx_id_delay,
> > > +					       false);
> > > +		if (rx_delay < 0) {
> > > +			phydev_err(phydev, "Rx internal delay is invalid\n");
> > > +			return rx_delay;
> > > +		}
> > > +	}
> > So any PHY using these properties is going to pretty much reproduce
> > this code. Meaning is should all be in a helper.
> 
> The issue here is that the phy_mode may only be rgmii-txid so you only want
> to find the tx_delay and return.
> 
> Same with the RXID.  How is the helper supposed to know what delay to return
> and look for?

How does this code do it? It looks at the value of interface.

    Andrew

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

[Dan Murphy]

Andrew

On 5/27/20 8:12 AM, Andrew Lunn wrote:
>> If the dt defines rgmii-rx/tx-id then these values are required not
>> optional.  That was the discussion on the binding.
> How many times do i need to say it. They are optional. If not
> specified, default to 2ns.

OK.  I guess then the DP83867 driver is wrong because it specifically 
states in bold

     /* RX delay *must* be specified if internal delay of RX is used. */

It was signed off in commit fafc5db28a2ff


>>>> +	ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
>>>> +				   &dp83869->tx_id_delay);
>>>> +	if (ret) {
>>>> +		dp83869->tx_id_delay = ret;
>>>> +		ret = 0;
>>>> +	}
>>>> +
>>>>    	return ret;
>>>>    }
>>>>    #else
>>>> @@ -367,10 +388,45 @@ static int dp83869_configure_mode(struct phy_device *phydev,
>>>>    	return ret;
>>>>    }
>>>> +static int dp83869_get_delay(struct phy_device *phydev)
>>>> +{
>>>> +	struct dp83869_private *dp83869 = phydev->priv;
>>>> +	int delay_size = ARRAY_SIZE(dp83869_internal_delay);
>>>> +	int tx_delay = 0;
>>>> +	int rx_delay = 0;
>>>> +
>>>> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
>>>> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
>>>> +		tx_delay = phy_get_delay_index(phydev,
>>>> +					       &dp83869_internal_delay[0],
>>>> +					       delay_size, dp83869->tx_id_delay,
>>>> +					       false);
>>>> +		if (tx_delay < 0) {
>>>> +			phydev_err(phydev, "Tx internal delay is invalid\n");
>>>> +			return tx_delay;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
>>>> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
>>>> +		rx_delay = phy_get_delay_index(phydev,
>>>> +					       &dp83869_internal_delay[0],
>>>> +					       delay_size, dp83869->rx_id_delay,
>>>> +					       false);
>>>> +		if (rx_delay < 0) {
>>>> +			phydev_err(phydev, "Rx internal delay is invalid\n");
>>>> +			return rx_delay;
>>>> +		}
>>>> +	}
>>> So any PHY using these properties is going to pretty much reproduce
>>> this code. Meaning is should all be in a helper.
>> The issue here is that the phy_mode may only be rgmii-txid so you only want
>> to find the tx_delay and return.
>>
>> Same with the RXID.  How is the helper supposed to know what delay to return
>> and look for?
> How does this code do it? It looks at the value of interface.

Actually I will be removing this check with setting the delays to default.

Dan


>      Andrew

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

[Andrew Lunn]

> +static int dp83869_internal_delay[] = {250, 500, 750, 1000, 1250, 1500, 1750,
> +				       2000, 2250, 2500, 2750, 3000, 3250,
> +				       3500, 3750, 4000};
> +

You should make this const. Otherwise it takes up twice the space.

    Andrew