[PATCH net-next 0/2] net: phy: dp83822: Add support for GPIO2 clock output

[PATCH net-next 0/2] net: phy: dp83822: Add support for GPIO2 clock output

[Dimitri Fedrau via B4 Relay]

The DP83822 has several clock configuration options for pins GPIO1, GPIO2
and GPIO3. Clock options include:
  - MAC IF clock
  - XI clock
  - Free-Running clock
  - Recovered clock
This patch adds support for GPIO2, the support for GPIO1 and GPIO3 can be
easily added if needed. Code and device tree bindings are derived from
dp83867 which has a similar feature.

Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
---
Dimitri Fedrau (2):
      dt-bindings: net: dp83822: Add support for GPIO2 clock output
      net: phy: dp83822: Add support for GPIO2 clock output

 .../devicetree/bindings/net/ti,dp83822.yaml        |  7 ++++
 drivers/net/phy/dp83822.c                          | 44 ++++++++++++++++++++++
 include/dt-bindings/net/ti-dp83822.h               | 21 +++++++++++
 3 files changed, 72 insertions(+)
---
base-commit: 7ea2745766d776866cfbc981b21ed3cfdf50124e
change-id: 20241206-dp83822-gpio2-clk-out-cd9cf63e37df

Best regards,
-- 
Dimitri Fedrau <dimitri.fedrau@liebherr.com>

[PATCH net-next 1/2] dt-bindings: net: dp83822: Add support for GPIO2 clock output

[Dimitri Fedrau via B4 Relay]

From: Dimitri Fedrau <dimitri.fedrau@liebherr.com>

The GPIO2 pin on the DP83822 can be configured as clock output. Add
binding to support this feature.

Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
---
 .../devicetree/bindings/net/ti,dp83822.yaml         |  7 +++++++
 include/dt-bindings/net/ti-dp83822.h                | 21 +++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
index 784866ea392b2083e93d8dc9aaea93b70dc80934..4a4dc794f21162c6a61c3daeeffa08e666034679 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
@@ -96,6 +96,13 @@ properties:
       - master
       - slave
 
+  ti,gpio2-clk-out:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+       DP83822 PHY only.
+       Muxing option for GPIO2 pin. See dt-bindings/net/ti-dp83822.h for
+       applicable values. When omitted, the PHY's default will be left as is.
+
 required:
   - reg
 
diff --git a/include/dt-bindings/net/ti-dp83822.h b/include/dt-bindings/net/ti-dp83822.h
new file mode 100644
index 0000000000000000000000000000000000000000..d569c90618b7bcae9ffe44eb041f7dae2e74e5d1
--- /dev/null
+++ b/include/dt-bindings/net/ti-dp83822.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
+/*
+ * Device Tree constants for the Texas Instruments DP83822 PHY
+ *
+ * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH
+ *
+ * Author: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
+ */
+
+#ifndef _DT_BINDINGS_TI_DP83822_H
+#define _DT_BINDINGS_TI_DP83822_H
+
+/* IO_MUX_GPIO_CTRL - Clock source selection */
+#define DP83822_CLK_SRC_MAC_IF			0x0
+#define DP83822_CLK_SRC_XI			0x1
+#define DP83822_CLK_SRC_INT_REF			0x2
+#define DP83822_CLK_SRC_RMII_MASTER_MODE_REF	0x4
+#define DP83822_CLK_SRC_FREE_RUNNING		0x6
+#define DP83822_CLK_SRC_RECOVERED		0x7
+
+#endif

-- 
2.39.5

[PATCH net-next 2/2] net: phy: dp83822: Add support for GPIO2 clock output

[Dimitri Fedrau via B4 Relay]

From: Dimitri Fedrau <dimitri.fedrau@liebherr.com>

The GPIO2 pin on the DP83822 can be configured as clock output. Add support
for configuration via DT.

Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
---
 drivers/net/phy/dp83822.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index cf8b6d0bfaa9812eee98c612c0d4259d87da7572..2abb066c3b8fe701c551c278ba31a08f9cb3fc15 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -14,6 +14,8 @@
 #include <linux/netdevice.h>
 #include <linux/bitfield.h>
 
+#include <dt-bindings/net/ti-dp83822.h>
+
 #define DP83822_PHY_ID	        0x2000a240
 #define DP83825S_PHY_ID		0x2000a140
 #define DP83825I_PHY_ID		0x2000a150
@@ -33,6 +35,7 @@
 #define MII_DP83822_RCSR	0x17
 #define MII_DP83822_RESET_CTRL	0x1f
 #define MII_DP83822_GENCFG	0x465
+#define MII_DP83822_IOCTRL2	0x463
 #define MII_DP83822_SOR1	0x467
 
 /* DP83826 specific registers */
@@ -106,6 +109,11 @@
 #define DP83822_RX_CLK_SHIFT	BIT(12)
 #define DP83822_TX_CLK_SHIFT	BIT(11)
 
+/* IOCTRL2 bits */
+#define DP83822_IOCTRL2_GPIO2_CLK_SRC		GENMASK(6, 4)
+#define DP83822_IOCTRL2_GPIO2_CTRL		GENMASK(2, 0)
+#define DP83822_IOCTRL2_GPIO2_CTRL_CLK_REF	GENMASK(1, 0)
+
 /* SOR1 mode */
 #define DP83822_STRAP_MODE1	0
 #define DP83822_STRAP_MODE2	BIT(0)
@@ -141,6 +149,8 @@ struct dp83822_private {
 	u8 cfg_dac_minus;
 	u8 cfg_dac_plus;
 	struct ethtool_wolinfo wol;
+	bool set_gpio2_clk_out;
+	u32 gpio2_clk_out;
 };
 
 static int dp83822_config_wol(struct phy_device *phydev,
@@ -415,6 +425,15 @@ static int dp83822_config_init(struct phy_device *phydev)
 	int err = 0;
 	int bmcr;
 
+	if (dp83822->set_gpio2_clk_out)
+		phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83822_IOCTRL2,
+			       DP83822_IOCTRL2_GPIO2_CTRL |
+			       DP83822_IOCTRL2_GPIO2_CLK_SRC,
+			       FIELD_PREP(DP83822_IOCTRL2_GPIO2_CTRL,
+					  DP83822_IOCTRL2_GPIO2_CTRL_CLK_REF) |
+			       FIELD_PREP(DP83822_IOCTRL2_GPIO2_CLK_SRC,
+					  dp83822->gpio2_clk_out));
+
 	if (phy_interface_is_rgmii(phydev)) {
 		rx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0,
 						      true);
@@ -613,6 +632,7 @@ static int dp83822_of_init(struct phy_device *phydev)
 {
 	struct dp83822_private *dp83822 = phydev->priv;
 	struct device *dev = &phydev->mdio.dev;
+	int ret;
 
 	/* Signal detection for the PHY is only enabled if the FX_EN and the
 	 * SD_EN pins are strapped. Signal detection can only enabled if FX_EN
@@ -625,6 +645,30 @@ static int dp83822_of_init(struct phy_device *phydev)
 		dp83822->fx_enabled = device_property_present(dev,
 							      "ti,fiber-mode");
 
+	ret = of_property_read_u32(dev->of_node, "ti,gpio2-clk-out",
+				   &dp83822->gpio2_clk_out);
+	if (!ret) {
+		dp83822->set_gpio2_clk_out = true;
+		switch (dp83822->gpio2_clk_out) {
+		case DP83822_CLK_SRC_MAC_IF:
+			break;
+		case DP83822_CLK_SRC_XI:
+			break;
+		case DP83822_CLK_SRC_INT_REF:
+			break;
+		case DP83822_CLK_SRC_RMII_MASTER_MODE_REF:
+			break;
+		case DP83822_CLK_SRC_FREE_RUNNING:
+			break;
+		case DP83822_CLK_SRC_RECOVERED:
+			break;
+		default:
+			phydev_err(phydev, "ti,gpio2-clk-out value %u not valid\n",
+				   dp83822->gpio2_clk_out);
+			return -EINVAL;
+		}
+	}
+
 	return 0;
 }
 

-- 
2.39.5

Re: [PATCH net-next 2/2] net: phy: dp83822: Add support for GPIO2 clock output

[Andrew Lunn]

>  #define MII_DP83822_RCSR	0x17
>  #define MII_DP83822_RESET_CTRL	0x1f
>  #define MII_DP83822_GENCFG	0x465
> +#define MII_DP83822_IOCTRL2	0x463
>  #define MII_DP83822_SOR1	0x467

These are sorted, so the MII_DP83822_IOCTRL2 should go before
MII_DP83822_GENCFG.

> +	if (dp83822->set_gpio2_clk_out)
> +		phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83822_IOCTRL2,

I would of preferred MDIO_MMD_VEND2 rather than DP83822_DEVADDR, but
having just this one instance correct would look a bit odd.

> +	ret = of_property_read_u32(dev->of_node, "ti,gpio2-clk-out",
> +				   &dp83822->gpio2_clk_out);
> +	if (!ret) {
> +		dp83822->set_gpio2_clk_out = true;
> +		switch (dp83822->gpio2_clk_out) {
> +		case DP83822_CLK_SRC_MAC_IF:
> +			break;
> +		case DP83822_CLK_SRC_XI:
> +			break;
> +		case DP83822_CLK_SRC_INT_REF:
> +			break;
> +		case DP83822_CLK_SRC_RMII_MASTER_MODE_REF:
> +			break;
> +		case DP83822_CLK_SRC_FREE_RUNNING:
> +			break;
> +		case DP83822_CLK_SRC_RECOVERED:
> +			break;

You can list multiple case statements together, and have one break at
the end.

I would personally also only have:

> +		dp83822->set_gpio2_clk_out = true;

if validation passes, not that it really matters because:


> +		default:
> +			phydev_err(phydev, "ti,gpio2-clk-out value %u not valid\n",
> +				   dp83822->gpio2_clk_out);
> +			return -EINVAL;
> +		}
> +	}


    Andrew

---
pw-bot: cr

Re: [PATCH net-next 2/2] net: phy: dp83822: Add support for GPIO2 clock output

[Dimitri Fedrau]

Am Mon, Dec 09, 2024 at 02:17:04PM +0100 schrieb Andrew Lunn:
> >  #define MII_DP83822_RCSR	0x17
> >  #define MII_DP83822_RESET_CTRL	0x1f
> >  #define MII_DP83822_GENCFG	0x465
> > +#define MII_DP83822_IOCTRL2	0x463
> >  #define MII_DP83822_SOR1	0x467
> 
> These are sorted, so the MII_DP83822_IOCTRL2 should go before
> MII_DP83822_GENCFG.
>
Will fix it.

> > +	if (dp83822->set_gpio2_clk_out)
> > +		phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83822_IOCTRL2,
> 
> I would of preferred MDIO_MMD_VEND2 rather than DP83822_DEVADDR, but
> having just this one instance correct would look a bit odd.
>
Is it worth fixing it in a separate patch, replacing all DP83822_DEVADDR
with MDIO_MMD_VEND2 ?

> > +	ret = of_property_read_u32(dev->of_node, "ti,gpio2-clk-out",
> > +				   &dp83822->gpio2_clk_out);
> > +	if (!ret) {
> > +		dp83822->set_gpio2_clk_out = true;
> > +		switch (dp83822->gpio2_clk_out) {
> > +		case DP83822_CLK_SRC_MAC_IF:
> > +			break;
> > +		case DP83822_CLK_SRC_XI:
> > +			break;
> > +		case DP83822_CLK_SRC_INT_REF:
> > +			break;
> > +		case DP83822_CLK_SRC_RMII_MASTER_MODE_REF:
> > +			break;
> > +		case DP83822_CLK_SRC_FREE_RUNNING:
> > +			break;
> > +		case DP83822_CLK_SRC_RECOVERED:
> > +			break;
> 
> You can list multiple case statements together, and have one break at
> the end.
> 
Will fix it.

> I would personally also only have:
> 
> > +		dp83822->set_gpio2_clk_out = true;
> 
> if validation passes, not that it really matters because:
> 
> 
> > +		default:
> > +			phydev_err(phydev, "ti,gpio2-clk-out value %u not valid\n",
> > +				   dp83822->gpio2_clk_out);
> > +			return -EINVAL;
> > +		}
> > +	}
> 
Ok.

	Dimitri

Re: [PATCH net-next 2/2] net: phy: dp83822: Add support for GPIO2 clock output

[Andrew Lunn]

> > I would of preferred MDIO_MMD_VEND2 rather than DP83822_DEVADDR, but
> > having just this one instance correct would look a bit odd.
> >
> Is it worth fixing it in a separate patch, replacing all DP83822_DEVADDR
> with MDIO_MMD_VEND2 ?

You could do. As a reviewer, i like to be able to quickly see, these
are vendor registers, i don't need to check if they are part of 802.3,
and should have standard names, and be pulled out into a library for
others to share. So if you want to add a patch, i'm happy with that.

	Andrew