[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

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

[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

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 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

[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 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 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 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