[PATCH net-next v7 0/6] RGMII Internal delay common property

[PATCH net-next v7 0/6] 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 internal delays for the Rx and Tx paths.

If the internal delay is tunable then the caller needs to pass the internal
delay array and the return will be the index in the array that was found in
the firmware node.

If the internal delay is fixed then the caller only needs to indicate which
delay to return and if the value is defined then the value in the firmware is
returned.

This series contains examples of both a configurable delay and a fixed delay.

Dan Murphy (6):
  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
  dt-bindings: net: dp83822: Add TI dp83822 phy
  net: phy: DP83822: Add ability to advertise Fiber connection

 .../devicetree/bindings/net/ethernet-phy.yaml |  11 ++
 .../devicetree/bindings/net/ti,dp83822.yaml   |  51 +++++++++
 .../devicetree/bindings/net/ti,dp83869.yaml   |  16 ++-
 drivers/net/phy/dp83822.c                     | 108 ++++++++++++++++--
 drivers/net/phy/dp83869.c                     |  53 ++++++++-
 drivers/net/phy/phy_device.c                  |  68 +++++++++++
 include/linux/phy.h                           |   4 +
 7 files changed, 299 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/ti,dp83822.yaml

-- 
2.26.2

[PATCH net-next v7 1/6] 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>
---
 .../devicetree/bindings/net/ethernet-phy.yaml         | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index 9b1f1147ca36..b2887476fe6a 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -162,6 +162,17 @@ properties:
     description:
       Specifies a reference to a node representing a SFP cage.
 
+
+  rx-internal-delay-ps:
+    description: |
+      RGMII Receive PHY Clock Delay defined in pico seconds.  This is used for
+      PHY's that have configurable RX internal delays.
+
+  tx-internal-delay-ps:
+    description: |
+      RGMII Transmit PHY Clock Delay defined in pico seconds.  This is used for
+      PHY's that have configurable TX internal delays.
+
 required:
   - reg
 
-- 
2.26.2

[PATCH net-next v7 2/6] 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 | 68 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  4 +++
 2 files changed, 72 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 04946de74fa0..611d4e68e3c6 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -31,6 +31,7 @@
 #include <linux/mdio.h>
 #include <linux/io.h>
 #include <linux/uaccess.h>
+#include <linux/property.h>
 
 MODULE_DESCRIPTION("PHY library");
 MODULE_AUTHOR("Andy Fleming");
@@ -2657,6 +2658,73 @@ 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
+ * @dev: pointer to the devices device struct
+ * @delay_values: array of delays the PHY supports
+ * @size: the size of the delay array
+ * @is_rx: boolean to indicate to get the rx internal delay
+ *
+ * Returns the index within the array of internal delay passed in.
+ * Or if size == 0 then the delay read from the firmware is returned.
+ * The array must be in ascending order.
+ * Return errno if the delay is invalid or cannot be found.
+ */
+s32 phy_get_internal_delay(struct phy_device *phydev, struct device *dev,
+			   const int *delay_values, int size, bool is_rx)
+{
+	int ret;
+	int i;
+	s32 delay;
+
+	if (is_rx)
+		ret = device_property_read_u32(dev, "rx-internal-delay-ps",
+					       &delay);
+	else
+		ret = device_property_read_u32(dev, "tx-internal-delay-ps",
+					       &delay);
+	if (ret) {
+		phydev_err(phydev, "internal delay not defined\n");
+		return -EINVAL;
+	}
+
+	if (delay < 0)
+		return -EINVAL;
+
+	if (size <= 0)
+		return delay;
+
+	if (delay < delay_values[0] || delay > delay_values[size - 1]) {
+		phydev_err(phydev, "Delay %d is out of range\n", delay);
+		return -EINVAL;
+	}
+
+	if (delay == delay_values[0])
+		return 0;
+
+	for (i = 1; 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_internal_delay);
+
 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 8c05d0fb5c00..917bfd422e06 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1430,6 +1430,10 @@ 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);
+
+s32 phy_get_internal_delay(struct phy_device *phydev, struct device *dev,
+			   const int *delay_values, int size, bool is_rx);
+
 void phy_resolve_pause(unsigned long *local_adv, unsigned long *partner_adv,
 		       bool *tx_pause, bool *rx_pause);
 
-- 
2.26.2

[PATCH net-next v7 3/6] 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, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83869.yaml b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
index 5b69ef03bbf7..71e90a3e4652 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83869.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
@@ -8,7 +8,7 @@ $schema: "http://devicetree.org/meta-schemas/core.yaml#"
 title: TI DP83869 ethernet PHY
 
 allOf:
-  - $ref: "ethernet-controller.yaml#"
+  - $ref: "ethernet-phy.yaml#"
 
 maintainers:
   - Dan Murphy <dmurphy@ti.com>
@@ -64,6 +64,18 @@ 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:
+    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:
+    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 +92,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 v7 4/6] 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 | 53 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 53ed3abc26c9..2c651bcc440d 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -64,6 +64,10 @@
 #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
+#define DP83869_CLK_DELAY_DEF			7
+
 /* STRAP_STS1 bits */
 #define DP83869_STRAP_OP_MODE_MASK		GENMASK(2, 0)
 #define DP83869_STRAP_STS1_RESERVED		BIT(11)
@@ -78,9 +82,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 +100,10 @@
 #define DP83869_OP_MODE_MII			BIT(5)
 #define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
 
+static const 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 +113,8 @@ enum {
 struct dp83869_private {
 	int tx_fifo_depth;
 	int rx_fifo_depth;
+	s32 rx_int_delay;
+	s32 tx_int_delay;
 	int io_impedance;
 	int port_mirroring;
 	bool rxctrl_strap_quirk;
@@ -182,6 +189,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)
@@ -235,6 +243,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;
 
+	dp83869->rx_int_delay = phy_get_internal_delay(phydev, dev,
+						     &dp83869_internal_delay[0],
+						      delay_size, true);
+	if (dp83869->rx_int_delay < 0)
+		dp83869->rx_int_delay =
+				dp83869_internal_delay[DP83869_CLK_DELAY_DEF];
+
+	dp83869->tx_int_delay = phy_get_internal_delay(phydev, dev,
+						     &dp83869_internal_delay[0],
+						      delay_size, false);
+	if (dp83869->tx_int_delay < 0)
+		dp83869->tx_int_delay =
+				dp83869_internal_delay[DP83869_CLK_DELAY_DEF];
+
 	return ret;
 }
 #else
@@ -397,6 +419,31 @@ 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 = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIIDCTL,
+				    dp83869->rx_int_delay |
+			dp83869->tx_int_delay << DP83869_RGMII_CLK_DELAY_SHIFT);
+		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

[PATCH net-next v7 5/6] dt-bindings: net: dp83822: Add TI dp83822 phy

[Dan Murphy]

Add a dt binding for the TI dp83822 ethernet phy device.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../devicetree/bindings/net/ti,dp83822.yaml   | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ti,dp83822.yaml

diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
new file mode 100644
index 000000000000..09e0e5fd88e3
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)
+# Copyright (C) 2020 Texas Instruments Incorporated
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/net/ti,dp83822.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: TI DP83822 ethernet PHY
+
+allOf:
+  - $ref: "ethernet-phy.yaml#"
+
+maintainers:
+  - Dan Murphy <dmurphy@ti.com>
+
+description: |
+  The DP83822 is a low-power, single-port, 10/100 Mbps Ethernet PHY. It
+  provides all of the physical layer functions needed to transmit and receive
+  data over standard, twisted-pair cables or to connect to an external,
+  fiber-optic transceiver. Additionally, the DP83822 provides flexibility to
+  connect to a MAC through a standard MII, RMII, or RGMII interface
+
+  Specifications about the charger can be found at:
+    http://www.ti.com/lit/ds/symlink/dp83822i.pdf
+
+properties:
+  reg:
+    maxItems: 1
+
+  rx-internal-delay-ps:
+    description: Enables the RX fixed internal delay of 3.5ns.
+    default: 3500
+
+  tx-internal-delay-ps:
+    description: Enables the TX fixed internal delay of 3.5ns.
+    default: 3500
+
+required:
+  - reg
+
+examples:
+  - |
+    mdio0 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      ethphy0: ethernet-phy@0 {
+        reg = <0>;
+        rx-internal-delay-ps = <3500>;
+        tx-internal-delay-ps = <3500>;
+      };
+    };
-- 
2.26.2

[PATCH net-next v7 6/6] net: phy: DP83822: Add ability to advertise Fiber connection

[Dan Murphy]

The DP83822 can be configured to use the RGMII interface. There are
independent fixed 3.5ns clock shift (aka internal delay) for the TX and RX
paths. This allow either one to be set if the MII interface is RGMII and
the value is set in the firmware node.

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

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index 1dd19d0cb269..49574c3df9ca 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -26,7 +26,9 @@
 #define MII_DP83822_PHYSCR	0x11
 #define MII_DP83822_MISR1	0x12
 #define MII_DP83822_MISR2	0x13
+#define MII_DP83822_RCSR	0x17
 #define MII_DP83822_RESET_CTRL	0x1f
+#define MII_DP83822_GENCFG	0x465
 
 #define DP83822_HW_RESET	BIT(15)
 #define DP83822_SW_RESET	BIT(14)
@@ -77,6 +79,15 @@
 #define DP83822_WOL_INDICATION_SEL BIT(8)
 #define DP83822_WOL_CLR_INDICATION BIT(11)
 
+/* RSCR bits */
+#define DP83822_RX_CLK_SHIFT	BIT(12)
+#define DP83822_TX_CLK_SHIFT	BIT(11)
+
+struct dp83822_private {
+	int rx_int_delay;
+	int tx_int_delay;
+};
+
 static int dp83822_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -255,7 +266,7 @@ static int dp83822_config_intr(struct phy_device *phydev)
 	return phy_write(phydev, MII_DP83822_PHYSCR, physcr_status);
 }
 
-static int dp83822_config_init(struct phy_device *phydev)
+static int dp8382x_disable_wol(struct phy_device *phydev)
 {
 	int value = DP83822_WOL_EN | DP83822_WOL_MAGIC_EN |
 		    DP83822_WOL_SECURE_ON;
@@ -264,6 +275,32 @@ static int dp83822_config_init(struct phy_device *phydev)
 				  MII_DP83822_WOL_CFG, value);
 }
 
+static int dp83822_config_init(struct phy_device *phydev)
+{
+	struct dp83822_private *dp83822 = phydev->priv;
+	int rgmii_delay;
+	int err = 0;
+
+	if (phy_interface_is_rgmii(phydev)) {
+		if (dp83822->rx_int_delay)
+			rgmii_delay = DP83822_RX_CLK_SHIFT;
+
+		if (dp83822->tx_int_delay)
+			rgmii_delay |= DP83822_TX_CLK_SHIFT;
+
+		if (rgmii_delay)
+			err = phy_set_bits_mmd(phydev, DP83822_DEVADDR,
+					       MII_DP83822_RCSR, rgmii_delay);
+	}
+
+	return dp8382x_disable_wol(phydev);
+}
+
+static int dp8382x_config_init(struct phy_device *phydev)
+{
+	return dp8382x_disable_wol(phydev);
+}
+
 static int dp83822_phy_reset(struct phy_device *phydev)
 {
 	int err;
@@ -272,7 +309,46 @@ static int dp83822_phy_reset(struct phy_device *phydev)
 	if (err < 0)
 		return err;
 
-	dp83822_config_init(phydev);
+	return phydev->drv->config_init(phydev);
+}
+
+#ifdef CONFIG_OF_MDIO
+static int dp83822_of_init(struct phy_device *phydev)
+{
+	struct dp83822_private *dp83822 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+
+	dp83822->rx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0,
+						       true);
+	if (dp83822->rx_int_delay < 0)
+		dp83822->rx_int_delay = 0;
+
+	dp83822->tx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0,
+						       false);
+	if (dp83822->tx_int_delay < 0)
+		dp83822->tx_int_delay = 0;
+
+	return 0;
+}
+#else
+static int dp83822_of_init(struct phy_device *phydev)
+{
+	return 0;
+}
+#endif /* CONFIG_OF_MDIO */
+
+static int dp83822_probe(struct phy_device *phydev)
+{
+	struct dp83822_private *dp83822;
+
+	dp83822 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83822),
+			       GFP_KERNEL);
+	if (!dp83822)
+		return -ENOMEM;
+
+	phydev->priv = dp83822;
+
+	dp83822_of_init(phydev);
 
 	return 0;
 }
@@ -308,6 +384,7 @@ static int dp83822_resume(struct phy_device *phydev)
 		PHY_ID_MATCH_MODEL(_id),			\
 		.name		= (_name),			\
 		/* PHY_BASIC_FEATURES */			\
+		.probe          = dp83822_probe,		\
 		.soft_reset	= dp83822_phy_reset,		\
 		.config_init	= dp83822_config_init,		\
 		.get_wol = dp83822_get_wol,			\
@@ -318,14 +395,29 @@ static int dp83822_resume(struct phy_device *phydev)
 		.resume = dp83822_resume,			\
 	}
 
+#define DP8382X_PHY_DRIVER(_id, _name)				\
+	{							\
+		PHY_ID_MATCH_MODEL(_id),			\
+		.name		= (_name),			\
+		/* PHY_BASIC_FEATURES */			\
+		.soft_reset	= dp83822_phy_reset,		\
+		.config_init	= dp8382x_config_init,		\
+		.get_wol = dp83822_get_wol,			\
+		.set_wol = dp83822_set_wol,			\
+		.ack_interrupt = dp83822_ack_interrupt,		\
+		.config_intr = dp83822_config_intr,		\
+		.suspend = dp83822_suspend,			\
+		.resume = dp83822_resume,			\
+	}
+
 static struct phy_driver dp83822_driver[] = {
 	DP83822_PHY_DRIVER(DP83822_PHY_ID, "TI DP83822"),
-	DP83822_PHY_DRIVER(DP83825I_PHY_ID, "TI DP83825I"),
-	DP83822_PHY_DRIVER(DP83826C_PHY_ID, "TI DP83826C"),
-	DP83822_PHY_DRIVER(DP83826NC_PHY_ID, "TI DP83826NC"),
-	DP83822_PHY_DRIVER(DP83825S_PHY_ID, "TI DP83825S"),
-	DP83822_PHY_DRIVER(DP83825CM_PHY_ID, "TI DP83825M"),
-	DP83822_PHY_DRIVER(DP83825CS_PHY_ID, "TI DP83825CS"),
+	DP8382X_PHY_DRIVER(DP83825I_PHY_ID, "TI DP83825I"),
+	DP8382X_PHY_DRIVER(DP83826C_PHY_ID, "TI DP83826C"),
+	DP8382X_PHY_DRIVER(DP83826NC_PHY_ID, "TI DP83826NC"),
+	DP8382X_PHY_DRIVER(DP83825S_PHY_ID, "TI DP83825S"),
+	DP8382X_PHY_DRIVER(DP83825CM_PHY_ID, "TI DP83825M"),
+	DP8382X_PHY_DRIVER(DP83825CS_PHY_ID, "TI DP83825CS"),
 };
 module_phy_driver(dp83822_driver);
 
-- 
2.26.2

Re: [PATCH net-next v7 6/6] net: phy: DP83822: Add ability to advertise Fiber connection

[Dan Murphy]

All

On 6/17/20 1:20 PM, Dan Murphy wrote:
> The DP83822 can be configured to use the RGMII interface. There are
> independent fixed 3.5ns clock shift (aka internal delay) for the TX and RX
> paths. This allow either one to be set if the MII interface is RGMII and
> the value is set in the firmware node.

$subject is wrong.  I used the 83822 fiber patch as my base as it had 
90% of the work done that I needed for the

internal delay.  I will fix it in v8 after review comments.

Dan

Re: [PATCH net-next v7 6/6] net: phy: DP83822: Add ability to advertise Fiber connection

[Jakub Kicinski]

On Wed, 17 Jun 2020 13:20:19 -0500 Dan Murphy wrote:
> +static int dp83822_config_init(struct phy_device *phydev)
> +{
> +	struct dp83822_private *dp83822 = phydev->priv;
> +	int rgmii_delay;
> +	int err = 0;
> +
> +	if (phy_interface_is_rgmii(phydev)) {
> +		if (dp83822->rx_int_delay)
> +			rgmii_delay = DP83822_RX_CLK_SHIFT;
> +
> +		if (dp83822->tx_int_delay)
> +			rgmii_delay |= DP83822_TX_CLK_SHIFT;
> +
> +		if (rgmii_delay)
> +			err = phy_set_bits_mmd(phydev, DP83822_DEVADDR,
> +					       MII_DP83822_RCSR, rgmii_delay);
> +	}
> +
> +	return dp8382x_disable_wol(phydev);
> +}


drivers/net/phy/dp83822.c: In function dp83822_config_init:
drivers/net/phy/dp83822.c:282:6: warning: variable err set but not used [-Wunused-but-set-variable]
  282 |  int err = 0;
      |      ^~~

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

[Andrew Lunn]

On Wed, Jun 17, 2020 at 01:20:14PM -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>

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

    Andrew

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

[Andrew Lunn]

On Wed, Jun 17, 2020 at 01:20:15PM -0500, 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 | 68 ++++++++++++++++++++++++++++++++++++
>  include/linux/phy.h          |  4 +++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 04946de74fa0..611d4e68e3c6 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -31,6 +31,7 @@
>  #include <linux/mdio.h>
>  #include <linux/io.h>
>  #include <linux/uaccess.h>
> +#include <linux/property.h>
>  
>  MODULE_DESCRIPTION("PHY library");
>  MODULE_AUTHOR("Andy Fleming");
> @@ -2657,6 +2658,73 @@ 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
> + * @dev: pointer to the devices device struct
> + * @delay_values: array of delays the PHY supports
> + * @size: the size of the delay array
> + * @is_rx: boolean to indicate to get the rx internal delay
> + *
> + * Returns the index within the array of internal delay passed in.
> + * Or if size == 0 then the delay read from the firmware is returned.
> + * The array must be in ascending order.
> + * Return errno if the delay is invalid or cannot be found.
> + */
> +s32 phy_get_internal_delay(struct phy_device *phydev, struct device *dev,
> +			   const int *delay_values, int size, bool is_rx)
> +{
> +	int ret;
> +	int i;
> +	s32 delay;
> +
> +	if (is_rx)
> +		ret = device_property_read_u32(dev, "rx-internal-delay-ps",
> +					       &delay);
> +	else
> +		ret = device_property_read_u32(dev, "tx-internal-delay-ps",
> +					       &delay);
> +	if (ret) {
> +		phydev_err(phydev, "internal delay not defined\n");

This is an optional property. So printing an error message seems heavy
handed.

Maybe it would be better to default to 0 if the property is not found,
and continue with the lookup in the table to find what value should be
written for a 0ps delay?

	Andrew

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

[Andrew Lunn]

On Wed, Jun 17, 2020 at 01:20:14PM -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>
> ---
>  .../devicetree/bindings/net/ethernet-phy.yaml         | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index 9b1f1147ca36..b2887476fe6a 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -162,6 +162,17 @@ properties:
>      description:
>        Specifies a reference to a node representing a SFP cage.
>  
> +
> +  rx-internal-delay-ps:
> +    description: |
> +      RGMII Receive PHY Clock Delay defined in pico seconds.  This is used for
> +      PHY's that have configurable RX internal delays.
> +
> +  tx-internal-delay-ps:
> +    description: |
> +      RGMII Transmit PHY Clock Delay defined in pico seconds.  This is used for
> +      PHY's that have configurable TX internal delays.
> +

So in a later patch you have:

default: 2000

That seems to apply that these values only apply when the phy mode
indicates a delay is needed. It would be good to document that here,
when each of these properties will be used. Also, that they default to
2000 when not present.

     Andrew

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

[Dan Murphy]

Andrew

On 6/17/20 8:51 PM, Andrew Lunn wrote:
> On Wed, Jun 17, 2020 at 01:20:15PM -0500, 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 | 68 ++++++++++++++++++++++++++++++++++++
>>   include/linux/phy.h          |  4 +++
>>   2 files changed, 72 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 04946de74fa0..611d4e68e3c6 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -31,6 +31,7 @@
>>   #include <linux/mdio.h>
>>   #include <linux/io.h>
>>   #include <linux/uaccess.h>
>> +#include <linux/property.h>
>>   
>>   MODULE_DESCRIPTION("PHY library");
>>   MODULE_AUTHOR("Andy Fleming");
>> @@ -2657,6 +2658,73 @@ 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
>> + * @dev: pointer to the devices device struct
>> + * @delay_values: array of delays the PHY supports
>> + * @size: the size of the delay array
>> + * @is_rx: boolean to indicate to get the rx internal delay
>> + *
>> + * Returns the index within the array of internal delay passed in.
>> + * Or if size == 0 then the delay read from the firmware is returned.
>> + * The array must be in ascending order.
>> + * Return errno if the delay is invalid or cannot be found.
>> + */
>> +s32 phy_get_internal_delay(struct phy_device *phydev, struct device *dev,
>> +			   const int *delay_values, int size, bool is_rx)
>> +{
>> +	int ret;
>> +	int i;
>> +	s32 delay;
>> +
>> +	if (is_rx)
>> +		ret = device_property_read_u32(dev, "rx-internal-delay-ps",
>> +					       &delay);
>> +	else
>> +		ret = device_property_read_u32(dev, "tx-internal-delay-ps",
>> +					       &delay);
>> +	if (ret) {
>> +		phydev_err(phydev, "internal delay not defined\n");
> This is an optional property. So printing an error message seems heavy
> handed.

I will change this to phydev_info


> Maybe it would be better to default to 0 if the property is not found,
> and continue with the lookup in the table to find what value should be
> written for a 0ps delay?

If the property is not found what would we look up?  The property 
missing to me indicates that the phy is not adding the delay for that path.

If these properties are not present then the delay should not be set by 
the device driver.

This is why I return -EINVAL.  Maybe I should return -ENODATA instead.

Dan

>
> 	Andrew

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

[Dan Murphy]

Andrew

On 6/17/20 9:01 PM, Andrew Lunn wrote:
> On Wed, Jun 17, 2020 at 01:20:14PM -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>
>> ---
>>   .../devicetree/bindings/net/ethernet-phy.yaml         | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
>> index 9b1f1147ca36..b2887476fe6a 100644
>> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
>> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
>> @@ -162,6 +162,17 @@ properties:
>>       description:
>>         Specifies a reference to a node representing a SFP cage.
>>   
>> +
>> +  rx-internal-delay-ps:
>> +    description: |
>> +      RGMII Receive PHY Clock Delay defined in pico seconds.  This is used for
>> +      PHY's that have configurable RX internal delays.
>> +
>> +  tx-internal-delay-ps:
>> +    description: |
>> +      RGMII Transmit PHY Clock Delay defined in pico seconds.  This is used for
>> +      PHY's that have configurable TX internal delays.
>> +
> So in a later patch you have:
>
> default: 2000
>
> That seems to apply that these values only apply when the phy mode
> indicates a delay is needed. It would be good to document that here,
> when each of these properties will be used. Also, that they default to
> 2000 when not present.

The default of 2000ps is for the DP83869 only.  The DP83822 fixed delay 
is 3500ps.

So indicating that the default here is 2000ps for all devices is not 
correct.

But I can add a note that if these properties are present then the delay 
is added by the PHY.

Dan


>       Andrew

Re: [PATCH net-next v7 5/6] dt-bindings: net: dp83822: Add TI dp83822 phy

[Rob Herring]

On Wed, 17 Jun 2020 13:20:18 -0500, Dan Murphy wrote:
> Add a dt binding for the TI dp83822 ethernet phy device.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  .../devicetree/bindings/net/ti,dp83822.yaml   | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/ti,dp83822.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>