[PATCH net-next 0/2] Add support for PIC64-HPSC/HX MDIO controller

[PATCH net-next 0/2] Add support for PIC64-HPSC/HX MDIO controller

[Charles Perry]

Hello,

This series adds a driver for the two MDIO controllers of PIC64-HPSC/HX.
The hardware supports C22 and C45 but only C22 is implemented for now.

This MDIO hardware is based on a Microsemi design supported in Linux by
mdio-mscc-miim.c. However, The register interface is completely different
with pic64hpsc, hence the need for a separate driver.

The documentation recommends an input clock of 156.25MHz and a prescaler of
39, which yields an MDIO clock of 1.95MHz. This is the clock configuration
I've used in my tests.

This was tested on Microchip HB1301 evalkit which has a VSC8574 and a
VSC8541.

Thanks,
Charles

Charles Perry (2):
  dt-bindings: net: document Microchip PIC64-HPSC/HX MDIO controller
  net: mdio: add a driver for PIC64-HPSC/HX MDIO controller

 .../net/microchip,pic64hpsc-mdio.yaml         |  61 ++++++
 drivers/net/mdio/Kconfig                      |   7 +
 drivers/net/mdio/Makefile                     |   1 +
 drivers/net/mdio/mdio-pic64hpsc.c             | 207 ++++++++++++++++++
 4 files changed, 276 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/microchip,pic64hpsc-mdio.yaml
 create mode 100644 drivers/net/mdio/mdio-pic64hpsc.c

-- 
2.47.3

[PATCH net-next 1/2] dt-bindings: net: document Microchip PIC64-HPSC/HX MDIO controller

[Charles Perry]

This MDIO hardware is based on a Microsemi design supported in Linux by
mdio-mscc-miim.c. However, The register interface is completely different
with pic64hpsc, hence the need for separate documentation.

The hardware supports C22 and C45.

The documentation recommends an input clock of 156.25MHz and a prescaler
of 39, which yields an MDIO clock of 1.95MHz.

The hardware supports an interrupt pin to signal transaction completion
which is not strictly needed as the software can also poll a "TRIGGER"
bit for this.

Signed-off-by: Charles Perry <charles.perry@microchip.com>
---
 .../net/microchip,pic64hpsc-mdio.yaml         | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/microchip,pic64hpsc-mdio.yaml

diff --git a/Documentation/devicetree/bindings/net/microchip,pic64hpsc-mdio.yaml b/Documentation/devicetree/bindings/net/microchip,pic64hpsc-mdio.yaml
new file mode 100644
index 000000000000..21c76199c11b
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/microchip,pic64hpsc-mdio.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/microchip,pic64hpsc-mdio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip PIC64-HPSC/HX MDIO controller
+
+maintainers:
+  - Charles Perry <charles.perry@microchip.com>
+
+description: |
+  Microchip PIC64-HPSC/HX SoCs have two MDIO bus controller. This MDIO bus
+  controller supports C22 and C45 register access. It is named "MDIO Initiator"
+  in the documentation.
+
+allOf:
+  - $ref: mdio.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - const: microchip,pic64hpsc-mdio
+      - items:
+          - const: microchip,pic64hx-mdio
+          - const: microchip,pic64hpsc-mdio
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-frequency: true
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        mdio@4000C21E000 {
+            compatible = "microchip,pic64hpsc-mdio";
+            reg = <0x400 0x0C21E000 0x0 0x1000>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            phy0: ethernet-phy@0 {
+                reg = <0>;
+            };
+        };
+    };
-- 
2.47.3

Re: [PATCH net-next 1/2] dt-bindings: net: document Microchip PIC64-HPSC/HX MDIO controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2789 bytes --]

On Tue, Mar 17, 2026 at 11:46:09AM -0700, Charles Perry wrote:
> This MDIO hardware is based on a Microsemi design supported in Linux by
> mdio-mscc-miim.c. However, The register interface is completely different
> with pic64hpsc, hence the need for separate documentation.
> 
> The hardware supports C22 and C45.
> 
> The documentation recommends an input clock of 156.25MHz and a prescaler
> of 39, which yields an MDIO clock of 1.95MHz.
> 
> The hardware supports an interrupt pin to signal transaction completion
> which is not strictly needed as the software can also poll a "TRIGGER"
> bit for this.
> 
> Signed-off-by: Charles Perry <charles.perry@microchip.com>
> ---
>  .../net/microchip,pic64hpsc-mdio.yaml         | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/microchip,pic64hpsc-mdio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/microchip,pic64hpsc-mdio.yaml b/Documentation/devicetree/bindings/net/microchip,pic64hpsc-mdio.yaml
> new file mode 100644
> index 000000000000..21c76199c11b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/microchip,pic64hpsc-mdio.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/microchip,pic64hpsc-mdio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip PIC64-HPSC/HX MDIO controller
> +
> +maintainers:
> +  - Charles Perry <charles.perry@microchip.com>
> +
> +description: |
> +  Microchip PIC64-HPSC/HX SoCs have two MDIO bus controller. This MDIO bus
> +  controller supports C22 and C45 register access. It is named "MDIO Initiator"
> +  in the documentation.
> +
> +allOf:
> +  - $ref: mdio.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: microchip,pic64hpsc-mdio
> +      - items:
> +          - const: microchip,pic64hx-mdio
> +          - const: microchip,pic64hpsc-mdio
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-frequency: true

Does this genuinely have no constraints?

> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        mdio@4000C21E000 {
> +            compatible = "microchip,pic64hpsc-mdio";
> +            reg = <0x400 0x0C21E000 0x0 0x1000>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            phy0: ethernet-phy@0 {
> +                reg = <0>;
> +            };
> +        };
> +    };
> -- 
> 2.47.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next 1/2] dt-bindings: net: document Microchip PIC64-HPSC/HX MDIO controller

[Charles Perry]

On Wed, Mar 18, 2026 at 05:48:08PM +0000, Conor Dooley wrote:
> On Tue, Mar 17, 2026 at 11:46:09AM -0700, Charles Perry wrote:
> > This MDIO hardware is based on a Microsemi design supported in Linux by
> > mdio-mscc-miim.c. However, The register interface is completely different
> > with pic64hpsc, hence the need for separate documentation.
> > 
> > The hardware supports C22 and C45.
> > 
> > The documentation recommends an input clock of 156.25MHz and a prescaler
> > of 39, which yields an MDIO clock of 1.95MHz.
> > 
> > The hardware supports an interrupt pin to signal transaction completion
> > which is not strictly needed as the software can also poll a "TRIGGER"
> > bit for this.
> > 
> > Signed-off-by: Charles Perry <charles.perry@microchip.com>
> > ---
> >  .../net/microchip,pic64hpsc-mdio.yaml         | 61 +++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/microchip,pic64hpsc-mdio.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/net/microchip,pic64hpsc-mdio.yaml b/Documentation/devicetree/bindings/net/microchip,pic64hpsc-mdio.yaml
> > new file mode 100644
> > index 000000000000..21c76199c11b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/microchip,pic64hpsc-mdio.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/microchip,pic64hpsc-mdio.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Microchip PIC64-HPSC/HX MDIO controller
> > +
> > +maintainers:
> > +  - Charles Perry <charles.perry@microchip.com>
> > +
> > +description: |
> > +  Microchip PIC64-HPSC/HX SoCs have two MDIO bus controller. This MDIO bus
> > +  controller supports C22 and C45 register access. It is named "MDIO Initiator"
> > +  in the documentation.
> > +
> > +allOf:
> > +  - $ref: mdio.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - const: microchip,pic64hpsc-mdio
> > +      - items:
> > +          - const: microchip,pic64hx-mdio
> > +          - const: microchip,pic64hpsc-mdio
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-frequency: true
> 
> Does this genuinely have no constraints?

It's going to divide the input frequency by 2 to 512 (the prescaler is 8
bit long), so assuming an input clock of 156.25 MHz, the bounds are 305KHz
to 78MHz. The standard is 2.5MHz.

I can add a maximum and minimum here since I do have some validation on
this in the driver which will bail out if this is out of bound.

Thanks,
Charles

Re: [PATCH net-next 1/2] dt-bindings: net: document Microchip PIC64-HPSC/HX MDIO controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]

On Wed, Mar 18, 2026 at 02:23:23PM -0700, Charles Perry wrote:
> > > +
> > > +  clock-frequency: true
> > 
> > Does this genuinely have no constraints?
> 
> It's going to divide the input frequency by 2 to 512 (the prescaler is 8
> bit long), so assuming an input clock of 156.25 MHz, the bounds are 305KHz
> to 78MHz. The standard is 2.5MHz.
> 
> I can add a maximum and minimum here since I do have some validation on
> this in the driver which will bail out if this is out of bound.


That sounds like a good idea, thanks.

pw-bot: changes-requested

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next 1/2] dt-bindings: net: document Microchip PIC64-HPSC/HX MDIO controller

[Charles Perry]

On Thu, Mar 19, 2026 at 01:35:52AM +0000, Conor Dooley wrote:
> On Wed, Mar 18, 2026 at 02:23:23PM -0700, Charles Perry wrote:
> > > > +
> > > > +  clock-frequency: true
> > > 
> > > Does this genuinely have no constraints?
> > 
> > It's going to divide the input frequency by 2 to 512 (the prescaler is 8
> > bit long), so assuming an input clock of 156.25 MHz, the bounds are 305KHz
> > to 78MHz. The standard is 2.5MHz.
> > 
> > I can add a maximum and minimum here since I do have some validation on
> > this in the driver which will bail out if this is out of bound.

Hello Conor,

I have second doubt about this. The minimum and maximum depend on the input
clock frequency which might change if someone uses a different crystal or
clock config. So for that reason, I thinks it's better to not specify the
bounds, because there's no way to know for sure what they when building the
device tree.

I will however add a "default: 2500000" following some discussions with
Andrew Lunn.


Thanks,
Charles

> 
> 
> That sounds like a good idea, thanks.
> 
> pw-bot: changes-requested
> 
> Cheers,
> Conor.

Re: [PATCH net-next 1/2] dt-bindings: net: document Microchip PIC64-HPSC/HX MDIO controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

On Mon, Mar 23, 2026 at 06:38:06AM -0700, Charles Perry wrote:
> On Thu, Mar 19, 2026 at 01:35:52AM +0000, Conor Dooley wrote:
> > On Wed, Mar 18, 2026 at 02:23:23PM -0700, Charles Perry wrote:
> > > > > +
> > > > > +  clock-frequency: true
> > > > 
> > > > Does this genuinely have no constraints?
> > > 
> > > It's going to divide the input frequency by 2 to 512 (the prescaler is 8
> > > bit long), so assuming an input clock of 156.25 MHz, the bounds are 305KHz
> > > to 78MHz. The standard is 2.5MHz.
> > > 
> > > I can add a maximum and minimum here since I do have some validation on
> > > this in the driver which will bail out if this is out of bound.
> 
> Hello Conor,
> 
> I have second doubt about this. The minimum and maximum depend on the input
> clock frequency which might change if someone uses a different crystal or
> clock config. So for that reason, I thinks it's better to not specify the
> bounds, because there's no way to know for sure what they when building the
> device tree.

Okay, sure.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next 1/2] dt-bindings: net: document Microchip PIC64-HPSC/HX MDIO controller

[Andrew Lunn]

> The hardware supports an interrupt pin to signal transaction completion
> which is not strictly needed as the software can also poll a "TRIGGER"
> bit for this.

Experience with the FEC is that using interrupts is slower, but you
save some CPU load. The vast majority of MDIO devices poll.

     Andrew

Re: [PATCH net-next 1/2] dt-bindings: net: document Microchip PIC64-HPSC/HX MDIO controller

[Andrew Lunn]

> The hardware supports an interrupt pin to signal transaction completion
> which is not strictly needed as the software can also poll a "TRIGGER"
> bit for this.

If the interrupt always exists, it is better to have it in DT. I
assume it will be in the SoC .dtsi file?

Always requiring it makes the code simpler when somebody adds support
for interrupts. You don't need all the _optional_ and dealing with it
being missing etc.

      Andrew

Re: [PATCH net-next 1/2] dt-bindings: net: document Microchip PIC64-HPSC/HX MDIO controller

[Charles Perry]

On Thu, Mar 19, 2026 at 05:59:36PM +0100, Andrew Lunn wrote:
> > The hardware supports an interrupt pin to signal transaction completion
> > which is not strictly needed as the software can also poll a "TRIGGER"
> > bit for this.
> 
> If the interrupt always exists, it is better to have it in DT. I
> assume it will be in the SoC .dtsi file?
> 

Yes, I have that interrupt in my .dtsi.

> Always requiring it makes the code simpler when somebody adds support
> for interrupts. You don't need all the _optional_ and dealing with it
> being missing etc.
> 

Good point, I'll add that to the required properties.

Thanks,
Charles

[PATCH net-next 2/2] net: mdio: add a driver for PIC64-HPSC/HX MDIO controller

[Charles Perry]

This adds an MDIO driver for PIC64-HPSC/HX. The hardware supports C22
and C45 but only C22 is implemented in this commit.

This MDIO hardware is based on a Microsemi design supported in Linux by
mdio-mscc-miim.c. However, The register interface is completely
different with pic64hpsc, hence the need for a separate driver.

The documentation recommends an input clock of 156.25MHz and a prescaler
of 39, which yields an MDIO clock of 1.95MHz.

The hardware supports an interrupt pin or a "TRIGGER" bit that can be
polled to signal transaction completion. This commit uses polling.

This was tested on Microchip HB1301 evalkit with a VSC8574 and a
VSC8541.

Signed-off-by: Charles Perry <charles.perry@microchip.com>
---
 drivers/net/mdio/Kconfig          |   7 +
 drivers/net/mdio/Makefile         |   1 +
 drivers/net/mdio/mdio-pic64hpsc.c | 207 ++++++++++++++++++++++++++++++
 3 files changed, 215 insertions(+)
 create mode 100644 drivers/net/mdio/mdio-pic64hpsc.c

diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 44380378911b..7bdba8c3ddef 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -146,6 +146,13 @@ config MDIO_OCTEON
 	  buses. It is required by the Octeon and ThunderX ethernet device
 	  drivers on some systems.
 
+config MDIO_PIC64HPSC
+	tristate "PIC64-HPSC/HX MDIO interface support"
+	depends on HAS_IOMEM && OF_MDIO
+	help
+	  This driver supports the MDIO interface found on the PIC64-HPSC/HX
+	  SoCs.
+
 config MDIO_IPQ4019
 	tristate "Qualcomm IPQ4019 MDIO interface support"
 	depends on HAS_IOMEM && OF_MDIO
diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index fbec636700e7..048586746026 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_MDIO_MOXART)		+= mdio-moxart.o
 obj-$(CONFIG_MDIO_MSCC_MIIM)		+= mdio-mscc-miim.o
 obj-$(CONFIG_MDIO_MVUSB)		+= mdio-mvusb.o
 obj-$(CONFIG_MDIO_OCTEON)		+= mdio-octeon.o
+obj-$(CONFIG_MDIO_PIC64HPSC)		+= mdio-pic64hpsc.o
 obj-$(CONFIG_MDIO_REALTEK_RTL9300)	+= mdio-realtek-rtl9300.o
 obj-$(CONFIG_MDIO_REGMAP)		+= mdio-regmap.o
 obj-$(CONFIG_MDIO_SUN4I)		+= mdio-sun4i.o
diff --git a/drivers/net/mdio/mdio-pic64hpsc.c b/drivers/net/mdio/mdio-pic64hpsc.c
new file mode 100644
index 000000000000..1128b3a86804
--- /dev/null
+++ b/drivers/net/mdio/mdio-pic64hpsc.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Microchip PIC64-HPSC/HX MDIO controller driver
+ *
+ * Copyright (c) 2026 Microchip Technology Inc. and its subsidiaries.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_mdio.h>
+#include <linux/platform_device.h>
+
+#define MDIO_REG_PRESCALER     0x20
+#define MDIO_CFG_PRESCALE_MASK GENMASK(7, 0)
+
+#define MDIO_REG_FRAME_CFG_1 0x24
+#define MDIO_WDATA_MASK	     GENMASK(15, 0)
+
+#define MDIO_REG_FRAME_CFG_2	 0x28
+#define MDIO_TRIGGER_BIT	 BIT(31)
+#define MDIO_REG_DEV_ADDR_MASK	 GENMASK(20, 16)
+#define MDIO_PHY_PRT_ADDR_MASK	 GENMASK(8, 4)
+#define MDIO_OPERATION_MASK	 GENMASK(3, 2)
+#define MDIO_START_OF_FRAME_MASK GENMASK(1, 0)
+
+/* Possible value of MDIO_OPERATION_MASK */
+#define MDIO_OPERATION_WRITE BIT(0)
+#define MDIO_OPERATION_READ  BIT(1)
+
+#define MDIO_REG_FRAME_STATUS 0x2C
+#define MDIO_READOK_BIT	      BIT(24)
+#define MDIO_RDATA_MASK	      GENMASK(15, 0)
+
+#define MDIO_INT_I_ADDR 0x30
+#define MDIO_INT_I_BIT	BIT(0)
+
+#define MDIO_INT_E_ADDR 0x34
+#define MDIO_INT_E_BIT	BIT(0)
+
+struct pic64hpsc_mdio_dev {
+	void __iomem *regs;
+};
+
+static int pic64hpsc_mdio_wait_trigger(struct mii_bus *bus)
+{
+	struct pic64hpsc_mdio_dev *priv = bus->priv;
+	u32 val;
+	int ret;
+
+	/* The MDIO_TRIGGER bit returns 0 when a transaction has completed. */
+	ret = readl_poll_timeout(priv->regs + MDIO_REG_FRAME_CFG_2, val,
+				 !(val & MDIO_TRIGGER_BIT), 50, 10000);
+
+	if (ret < 0)
+		dev_dbg(&bus->dev, "TRIGGER bit timeout: %x\n", val);
+
+	return ret;
+}
+
+static int pic64hpsc_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
+{
+	struct pic64hpsc_mdio_dev *priv = bus->priv;
+	u32 val;
+	int ret;
+
+	ret = pic64hpsc_mdio_wait_trigger(bus);
+	if (ret)
+		return ret;
+
+	writel(MDIO_TRIGGER_BIT | FIELD_PREP(MDIO_REG_DEV_ADDR_MASK, regnum) |
+		       FIELD_PREP(MDIO_PHY_PRT_ADDR_MASK, mii_id) |
+		       FIELD_PREP(MDIO_OPERATION_MASK, MDIO_OPERATION_READ) |
+		       FIELD_PREP(MDIO_START_OF_FRAME_MASK, 1),
+	       priv->regs + MDIO_REG_FRAME_CFG_2);
+
+	ret = pic64hpsc_mdio_wait_trigger(bus);
+	if (ret)
+		return ret;
+
+	val = readl(priv->regs + MDIO_REG_FRAME_STATUS);
+
+	/* The MDIO_READOK is a 1-bit value reflecting the inverse of the MDIO
+	 * bus value captured during the 2nd TA cycle. A PHY/Port should drive
+	 * the MDIO bus with a logic 0 on the 2nd TA cycle, however, the
+	 * PHY/Port could optionally drive a logic 1, to communicate a read
+	 * failure. This feature is optional, not defined by the 802.3 standard
+	 * and not supported in standard external PHYs.
+	 */
+	if (!(bus->phy_ignore_ta_mask & 1 << mii_id) &&
+	    !FIELD_GET(MDIO_READOK_BIT, val)) {
+		dev_dbg(&bus->dev, "READOK bit cleared\n");
+		return -EIO;
+	}
+
+	ret = FIELD_GET(MDIO_RDATA_MASK, val);
+
+	return ret;
+}
+
+static int pic64hpsc_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
+				u16 value)
+{
+	struct pic64hpsc_mdio_dev *priv = bus->priv;
+	int ret;
+
+	ret = pic64hpsc_mdio_wait_trigger(bus);
+	if (ret < 0)
+		return ret;
+
+	writel(FIELD_PREP(MDIO_WDATA_MASK, value),
+	       priv->regs + MDIO_REG_FRAME_CFG_1);
+
+	writel(MDIO_TRIGGER_BIT | FIELD_PREP(MDIO_REG_DEV_ADDR_MASK, regnum) |
+		       FIELD_PREP(MDIO_PHY_PRT_ADDR_MASK, mii_id) |
+		       FIELD_PREP(MDIO_OPERATION_MASK, MDIO_OPERATION_WRITE) |
+		       FIELD_PREP(MDIO_START_OF_FRAME_MASK, 1),
+	       priv->regs + MDIO_REG_FRAME_CFG_2);
+
+	return 0;
+}
+
+static int pic64hpsc_mdio_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct pic64hpsc_mdio_dev *priv;
+	struct mii_bus *bus;
+	unsigned long rate;
+	struct clk *clk;
+	u32 bus_freq;
+	u32 div;
+	int ret;
+
+	bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
+	if (!bus)
+		return -ENOMEM;
+
+	priv = bus->priv;
+
+	priv->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->regs))
+		return PTR_ERR(priv->regs);
+
+	bus->name = KBUILD_MODNAME;
+	bus->read = pic64hpsc_mdio_read;
+	bus->write = pic64hpsc_mdio_write;
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
+	bus->parent = dev;
+
+	clk = devm_clk_get_optional_enabled(dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	of_property_read_u32(np, "clock-frequency", &bus_freq);
+
+	if (bus_freq) {
+		if (!clk) {
+			dev_err(dev,
+				"cannot use clock-frequency without a clock\n");
+			return -EINVAL;
+		}
+
+		rate = clk_get_rate(clk);
+
+		div = DIV_ROUND_UP(rate, 2 * bus_freq) - 1;
+		if (div == 0 || div & ~MDIO_CFG_PRESCALE_MASK) {
+			dev_err(dev, "Incorrect MDIO clock frequency\n");
+			return -EINVAL;
+		}
+
+		dev_dbg(dev, "rate=%lu bus_freq=%u real_bus_freq=%lu div=%u\n",
+			rate, bus_freq, rate / (2 * (1 + div)), div);
+		writel(div, priv->regs + MDIO_REG_PRESCALER);
+	}
+
+	ret = devm_of_mdiobus_register(dev, bus, np);
+	if (ret) {
+		dev_err(dev, "Cannot register MDIO bus (%d)\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, bus);
+
+	return 0;
+}
+
+static const struct of_device_id pic64hpsc_mdio_match[] = {
+	{ .compatible = "microchip,pic64hpsc-mdio" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, pic64hpsc_mdio_match);
+
+static struct platform_driver pic64hpsc_mdio_driver = {
+	.probe = pic64hpsc_mdio_probe,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = pic64hpsc_mdio_match,
+	},
+};
+module_platform_driver(pic64hpsc_mdio_driver);
+
+MODULE_AUTHOR("Charles Perry <charles.perry@microchip.com>");
+MODULE_DESCRIPTION("Microchip PIC64-HPSC/HX MDIO driver");
+MODULE_LICENSE("GPL");
-- 
2.47.3

Re: [PATCH net-next 2/2] net: mdio: add a driver for PIC64-HPSC/HX MDIO controller

[Maxime Chevallier]

Hi Charles,

On 17/03/2026 19:46, Charles Perry wrote:
> This adds an MDIO driver for PIC64-HPSC/HX. The hardware supports C22
> and C45 but only C22 is implemented in this commit.
> 
> This MDIO hardware is based on a Microsemi design supported in Linux by
> mdio-mscc-miim.c. However, The register interface is completely
> different with pic64hpsc, hence the need for a separate driver.
> 
> The documentation recommends an input clock of 156.25MHz and a prescaler
> of 39, which yields an MDIO clock of 1.95MHz.
> 
> The hardware supports an interrupt pin or a "TRIGGER" bit that can be
> polled to signal transaction completion. This commit uses polling.
> 
> This was tested on Microchip HB1301 evalkit with a VSC8574 and a
> VSC8541.
> 
> Signed-off-by: Charles Perry <charles.perry@microchip.com>
> ---
>  drivers/net/mdio/Kconfig          |   7 +
>  drivers/net/mdio/Makefile         |   1 +
>  drivers/net/mdio/mdio-pic64hpsc.c | 207 ++++++++++++++++++++++++++++++
>  3 files changed, 215 insertions(+)
>  create mode 100644 drivers/net/mdio/mdio-pic64hpsc.c
> 
> diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
> index 44380378911b..7bdba8c3ddef 100644
> --- a/drivers/net/mdio/Kconfig
> +++ b/drivers/net/mdio/Kconfig
> @@ -146,6 +146,13 @@ config MDIO_OCTEON
>  	  buses. It is required by the Octeon and ThunderX ethernet device
>  	  drivers on some systems.
>  
> +config MDIO_PIC64HPSC
> +	tristate "PIC64-HPSC/HX MDIO interface support"
> +	depends on HAS_IOMEM && OF_MDIO
> +	help
> +	  This driver supports the MDIO interface found on the PIC64-HPSC/HX
> +	  SoCs.
> +
>  config MDIO_IPQ4019
>  	tristate "Qualcomm IPQ4019 MDIO interface support"
>  	depends on HAS_IOMEM && OF_MDIO
> diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
> index fbec636700e7..048586746026 100644
> --- a/drivers/net/mdio/Makefile
> +++ b/drivers/net/mdio/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_MDIO_MOXART)		+= mdio-moxart.o
>  obj-$(CONFIG_MDIO_MSCC_MIIM)		+= mdio-mscc-miim.o
>  obj-$(CONFIG_MDIO_MVUSB)		+= mdio-mvusb.o
>  obj-$(CONFIG_MDIO_OCTEON)		+= mdio-octeon.o
> +obj-$(CONFIG_MDIO_PIC64HPSC)		+= mdio-pic64hpsc.o
>  obj-$(CONFIG_MDIO_REALTEK_RTL9300)	+= mdio-realtek-rtl9300.o
>  obj-$(CONFIG_MDIO_REGMAP)		+= mdio-regmap.o
>  obj-$(CONFIG_MDIO_SUN4I)		+= mdio-sun4i.o
> diff --git a/drivers/net/mdio/mdio-pic64hpsc.c b/drivers/net/mdio/mdio-pic64hpsc.c
> new file mode 100644
> index 000000000000..1128b3a86804
> --- /dev/null
> +++ b/drivers/net/mdio/mdio-pic64hpsc.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Microchip PIC64-HPSC/HX MDIO controller driver
> + *
> + * Copyright (c) 2026 Microchip Technology Inc. and its subsidiaries.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_mdio.h>
> +#include <linux/platform_device.h>
> +
> +#define MDIO_REG_PRESCALER     0x20
> +#define MDIO_CFG_PRESCALE_MASK GENMASK(7, 0)
> +
> +#define MDIO_REG_FRAME_CFG_1 0x24
> +#define MDIO_WDATA_MASK	     GENMASK(15, 0)
> +
> +#define MDIO_REG_FRAME_CFG_2	 0x28
> +#define MDIO_TRIGGER_BIT	 BIT(31)
> +#define MDIO_REG_DEV_ADDR_MASK	 GENMASK(20, 16)
> +#define MDIO_PHY_PRT_ADDR_MASK	 GENMASK(8, 4)
> +#define MDIO_OPERATION_MASK	 GENMASK(3, 2)
> +#define MDIO_START_OF_FRAME_MASK GENMASK(1, 0)
> +
> +/* Possible value of MDIO_OPERATION_MASK */
> +#define MDIO_OPERATION_WRITE BIT(0)
> +#define MDIO_OPERATION_READ  BIT(1)
> +
> +#define MDIO_REG_FRAME_STATUS 0x2C
> +#define MDIO_READOK_BIT	      BIT(24)
> +#define MDIO_RDATA_MASK	      GENMASK(15, 0)
> +
> +#define MDIO_INT_I_ADDR 0x30
> +#define MDIO_INT_I_BIT	BIT(0)
> +
> +#define MDIO_INT_E_ADDR 0x34
> +#define MDIO_INT_E_BIT	BIT(0)

Thes INT_I/E don't seem to be used, you can drop them

> +
> +struct pic64hpsc_mdio_dev {
> +	void __iomem *regs;
> +};
> +
> +static int pic64hpsc_mdio_wait_trigger(struct mii_bus *bus)
> +{
> +	struct pic64hpsc_mdio_dev *priv = bus->priv;
> +	u32 val;
> +	int ret;
> +
> +	/* The MDIO_TRIGGER bit returns 0 when a transaction has completed. */
> +	ret = readl_poll_timeout(priv->regs + MDIO_REG_FRAME_CFG_2, val,
> +				 !(val & MDIO_TRIGGER_BIT), 50, 10000);
> +
> +	if (ret < 0)
> +		dev_dbg(&bus->dev, "TRIGGER bit timeout: %x\n", val);
> +
> +	return ret;
> +}
> +
> +static int pic64hpsc_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> +{
> +	struct pic64hpsc_mdio_dev *priv = bus->priv;
> +	u32 val;
> +	int ret;
> +
> +	ret = pic64hpsc_mdio_wait_trigger(bus);
> +	if (ret)
> +		return ret;
> +
> +	writel(MDIO_TRIGGER_BIT | FIELD_PREP(MDIO_REG_DEV_ADDR_MASK, regnum) |
> +		       FIELD_PREP(MDIO_PHY_PRT_ADDR_MASK, mii_id) |
> +		       FIELD_PREP(MDIO_OPERATION_MASK, MDIO_OPERATION_READ) |
> +		       FIELD_PREP(MDIO_START_OF_FRAME_MASK, 1),
> +	       priv->regs + MDIO_REG_FRAME_CFG_2);
> +
> +	ret = pic64hpsc_mdio_wait_trigger(bus);
> +	if (ret)
> +		return ret;
> +
> +	val = readl(priv->regs + MDIO_REG_FRAME_STATUS);
> +
> +	/* The MDIO_READOK is a 1-bit value reflecting the inverse of the MDIO
> +	 * bus value captured during the 2nd TA cycle. A PHY/Port should drive
> +	 * the MDIO bus with a logic 0 on the 2nd TA cycle, however, the
> +	 * PHY/Port could optionally drive a logic 1, to communicate a read
> +	 * failure. This feature is optional, not defined by the 802.3 standard
> +	 * and not supported in standard external PHYs.
> +	 */
> +	if (!(bus->phy_ignore_ta_mask & 1 << mii_id) &&
> +	    !FIELD_GET(MDIO_READOK_BIT, val)) {
> +		dev_dbg(&bus->dev, "READOK bit cleared\n");
> +		return -EIO;
> +	}
> +
> +	ret = FIELD_GET(MDIO_RDATA_MASK, val);
> +
> +	return ret;
> +}
> +
> +static int pic64hpsc_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> +				u16 value)
> +{
> +	struct pic64hpsc_mdio_dev *priv = bus->priv;
> +	int ret;
> +
> +	ret = pic64hpsc_mdio_wait_trigger(bus);
> +	if (ret < 0)
> +		return ret;
> +
> +	writel(FIELD_PREP(MDIO_WDATA_MASK, value),
> +	       priv->regs + MDIO_REG_FRAME_CFG_1);
> +
> +	writel(MDIO_TRIGGER_BIT | FIELD_PREP(MDIO_REG_DEV_ADDR_MASK, regnum) |
> +		       FIELD_PREP(MDIO_PHY_PRT_ADDR_MASK, mii_id) |
> +		       FIELD_PREP(MDIO_OPERATION_MASK, MDIO_OPERATION_WRITE) |
> +		       FIELD_PREP(MDIO_START_OF_FRAME_MASK, 1),
> +	       priv->regs + MDIO_REG_FRAME_CFG_2);
> +
> +	return 0;
> +}
> +
> +static int pic64hpsc_mdio_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct pic64hpsc_mdio_dev *priv;
> +	struct mii_bus *bus;
> +	unsigned long rate;
> +	struct clk *clk;
> +	u32 bus_freq;
> +	u32 div;
> +	int ret;
> +
> +	bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	priv = bus->priv;
> +
> +	priv->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->regs))
> +		return PTR_ERR(priv->regs);
> +
> +	bus->name = KBUILD_MODNAME;
> +	bus->read = pic64hpsc_mdio_read;
> +	bus->write = pic64hpsc_mdio_write;

Is there a plan to eventually add C45 ? if so, I'd put 'c22' somewhere
in the names here.

The rest seems OK to me, so with the extra macros removed,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

Re: [PATCH net-next 2/2] net: mdio: add a driver for PIC64-HPSC/HX MDIO controller

[Charles Perry]

On Wed, Mar 18, 2026 at 10:52:46AM +0100, Maxime Chevallier wrote:
> Hi Charles,
> 
> On 17/03/2026 19:46, Charles Perry wrote:
> > This adds an MDIO driver for PIC64-HPSC/HX. The hardware supports C22
> > and C45 but only C22 is implemented in this commit.
> > 
> > This MDIO hardware is based on a Microsemi design supported in Linux by
> > mdio-mscc-miim.c. However, The register interface is completely
> > different with pic64hpsc, hence the need for a separate driver.
> > 
> > The documentation recommends an input clock of 156.25MHz and a prescaler
> > of 39, which yields an MDIO clock of 1.95MHz.
> > 
> > The hardware supports an interrupt pin or a "TRIGGER" bit that can be
> > polled to signal transaction completion. This commit uses polling.
> > 
> > This was tested on Microchip HB1301 evalkit with a VSC8574 and a
> > VSC8541.
> > 
> > Signed-off-by: Charles Perry <charles.perry@microchip.com>
> > ---
> >  drivers/net/mdio/Kconfig          |   7 +
> >  drivers/net/mdio/Makefile         |   1 +
> >  drivers/net/mdio/mdio-pic64hpsc.c | 207 ++++++++++++++++++++++++++++++
> >  3 files changed, 215 insertions(+)
> >  create mode 100644 drivers/net/mdio/mdio-pic64hpsc.c
> > 
> > diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
> > index 44380378911b..7bdba8c3ddef 100644
> > --- a/drivers/net/mdio/Kconfig
> > +++ b/drivers/net/mdio/Kconfig
> > @@ -146,6 +146,13 @@ config MDIO_OCTEON
> >  	  buses. It is required by the Octeon and ThunderX ethernet device
> >  	  drivers on some systems.
> >  
> > +config MDIO_PIC64HPSC
> > +	tristate "PIC64-HPSC/HX MDIO interface support"
> > +	depends on HAS_IOMEM && OF_MDIO
> > +	help
> > +	  This driver supports the MDIO interface found on the PIC64-HPSC/HX
> > +	  SoCs.
> > +
> >  config MDIO_IPQ4019
> >  	tristate "Qualcomm IPQ4019 MDIO interface support"
> >  	depends on HAS_IOMEM && OF_MDIO
> > diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
> > index fbec636700e7..048586746026 100644
> > --- a/drivers/net/mdio/Makefile
> > +++ b/drivers/net/mdio/Makefile
> > @@ -20,6 +20,7 @@ obj-$(CONFIG_MDIO_MOXART)		+= mdio-moxart.o
> >  obj-$(CONFIG_MDIO_MSCC_MIIM)		+= mdio-mscc-miim.o
> >  obj-$(CONFIG_MDIO_MVUSB)		+= mdio-mvusb.o
> >  obj-$(CONFIG_MDIO_OCTEON)		+= mdio-octeon.o
> > +obj-$(CONFIG_MDIO_PIC64HPSC)		+= mdio-pic64hpsc.o
> >  obj-$(CONFIG_MDIO_REALTEK_RTL9300)	+= mdio-realtek-rtl9300.o
> >  obj-$(CONFIG_MDIO_REGMAP)		+= mdio-regmap.o
> >  obj-$(CONFIG_MDIO_SUN4I)		+= mdio-sun4i.o
> > diff --git a/drivers/net/mdio/mdio-pic64hpsc.c b/drivers/net/mdio/mdio-pic64hpsc.c
> > new file mode 100644
> > index 000000000000..1128b3a86804
> > --- /dev/null
> > +++ b/drivers/net/mdio/mdio-pic64hpsc.c
> > @@ -0,0 +1,207 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Microchip PIC64-HPSC/HX MDIO controller driver
> > + *
> > + * Copyright (c) 2026 Microchip Technology Inc. and its subsidiaries.
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define MDIO_REG_PRESCALER     0x20
> > +#define MDIO_CFG_PRESCALE_MASK GENMASK(7, 0)
> > +
> > +#define MDIO_REG_FRAME_CFG_1 0x24
> > +#define MDIO_WDATA_MASK	     GENMASK(15, 0)
> > +
> > +#define MDIO_REG_FRAME_CFG_2	 0x28
> > +#define MDIO_TRIGGER_BIT	 BIT(31)
> > +#define MDIO_REG_DEV_ADDR_MASK	 GENMASK(20, 16)
> > +#define MDIO_PHY_PRT_ADDR_MASK	 GENMASK(8, 4)
> > +#define MDIO_OPERATION_MASK	 GENMASK(3, 2)
> > +#define MDIO_START_OF_FRAME_MASK GENMASK(1, 0)
> > +
> > +/* Possible value of MDIO_OPERATION_MASK */
> > +#define MDIO_OPERATION_WRITE BIT(0)
> > +#define MDIO_OPERATION_READ  BIT(1)
> > +
> > +#define MDIO_REG_FRAME_STATUS 0x2C
> > +#define MDIO_READOK_BIT	      BIT(24)
> > +#define MDIO_RDATA_MASK	      GENMASK(15, 0)
> > +
> > +#define MDIO_INT_I_ADDR 0x30
> > +#define MDIO_INT_I_BIT	BIT(0)
> > +
> > +#define MDIO_INT_E_ADDR 0x34
> > +#define MDIO_INT_E_BIT	BIT(0)
> 
> Thes INT_I/E don't seem to be used, you can drop them

Ok

> 
> > +
> > +struct pic64hpsc_mdio_dev {
> > +	void __iomem *regs;
> > +};
> > +
> > +static int pic64hpsc_mdio_wait_trigger(struct mii_bus *bus)
> > +{
> > +	struct pic64hpsc_mdio_dev *priv = bus->priv;
> > +	u32 val;
> > +	int ret;
> > +
> > +	/* The MDIO_TRIGGER bit returns 0 when a transaction has completed. */
> > +	ret = readl_poll_timeout(priv->regs + MDIO_REG_FRAME_CFG_2, val,
> > +				 !(val & MDIO_TRIGGER_BIT), 50, 10000);
> > +
> > +	if (ret < 0)
> > +		dev_dbg(&bus->dev, "TRIGGER bit timeout: %x\n", val);
> > +
> > +	return ret;
> > +}
> > +
> > +static int pic64hpsc_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> > +{
> > +	struct pic64hpsc_mdio_dev *priv = bus->priv;
> > +	u32 val;
> > +	int ret;
> > +
> > +	ret = pic64hpsc_mdio_wait_trigger(bus);
> > +	if (ret)
> > +		return ret;
> > +
> > +	writel(MDIO_TRIGGER_BIT | FIELD_PREP(MDIO_REG_DEV_ADDR_MASK, regnum) |
> > +		       FIELD_PREP(MDIO_PHY_PRT_ADDR_MASK, mii_id) |
> > +		       FIELD_PREP(MDIO_OPERATION_MASK, MDIO_OPERATION_READ) |
> > +		       FIELD_PREP(MDIO_START_OF_FRAME_MASK, 1),
> > +	       priv->regs + MDIO_REG_FRAME_CFG_2);
> > +
> > +	ret = pic64hpsc_mdio_wait_trigger(bus);
> > +	if (ret)
> > +		return ret;
> > +
> > +	val = readl(priv->regs + MDIO_REG_FRAME_STATUS);
> > +
> > +	/* The MDIO_READOK is a 1-bit value reflecting the inverse of the MDIO
> > +	 * bus value captured during the 2nd TA cycle. A PHY/Port should drive
> > +	 * the MDIO bus with a logic 0 on the 2nd TA cycle, however, the
> > +	 * PHY/Port could optionally drive a logic 1, to communicate a read
> > +	 * failure. This feature is optional, not defined by the 802.3 standard
> > +	 * and not supported in standard external PHYs.
> > +	 */
> > +	if (!(bus->phy_ignore_ta_mask & 1 << mii_id) &&
> > +	    !FIELD_GET(MDIO_READOK_BIT, val)) {
> > +		dev_dbg(&bus->dev, "READOK bit cleared\n");
> > +		return -EIO;
> > +	}
> > +
> > +	ret = FIELD_GET(MDIO_RDATA_MASK, val);
> > +
> > +	return ret;
> > +}
> > +
> > +static int pic64hpsc_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> > +				u16 value)
> > +{
> > +	struct pic64hpsc_mdio_dev *priv = bus->priv;
> > +	int ret;
> > +
> > +	ret = pic64hpsc_mdio_wait_trigger(bus);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	writel(FIELD_PREP(MDIO_WDATA_MASK, value),
> > +	       priv->regs + MDIO_REG_FRAME_CFG_1);
> > +
> > +	writel(MDIO_TRIGGER_BIT | FIELD_PREP(MDIO_REG_DEV_ADDR_MASK, regnum) |
> > +		       FIELD_PREP(MDIO_PHY_PRT_ADDR_MASK, mii_id) |
> > +		       FIELD_PREP(MDIO_OPERATION_MASK, MDIO_OPERATION_WRITE) |
> > +		       FIELD_PREP(MDIO_START_OF_FRAME_MASK, 1),
> > +	       priv->regs + MDIO_REG_FRAME_CFG_2);
> > +
> > +	return 0;
> > +}
> > +
> > +static int pic64hpsc_mdio_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct device *dev = &pdev->dev;
> > +	struct pic64hpsc_mdio_dev *priv;
> > +	struct mii_bus *bus;
> > +	unsigned long rate;
> > +	struct clk *clk;
> > +	u32 bus_freq;
> > +	u32 div;
> > +	int ret;
> > +
> > +	bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	priv = bus->priv;
> > +
> > +	priv->regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->regs))
> > +		return PTR_ERR(priv->regs);
> > +
> > +	bus->name = KBUILD_MODNAME;
> > +	bus->read = pic64hpsc_mdio_read;
> > +	bus->write = pic64hpsc_mdio_write;
> 
> Is there a plan to eventually add C45 ? if so, I'd put 'c22' somewhere
> in the names here.

Yes. Ok, will do.

> 
> The rest seems OK to me, so with the extra macros removed,
> 
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thank you Maxime,
Charles

> 
> Maxime
> 

Re: [PATCH net-next 2/2] net: mdio: add a driver for PIC64-HPSC/HX MDIO controller

[Andrew Lunn]

> +static int pic64hpsc_mdio_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct pic64hpsc_mdio_dev *priv;
> +	struct mii_bus *bus;
> +	unsigned long rate;
> +	struct clk *clk;
> +	u32 bus_freq;
> +	u32 div;
> +	int ret;
> +
> +	bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	priv = bus->priv;
> +
> +	priv->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->regs))
> +		return PTR_ERR(priv->regs);
> +
> +	bus->name = KBUILD_MODNAME;
> +	bus->read = pic64hpsc_mdio_read;
> +	bus->write = pic64hpsc_mdio_write;
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> +	bus->parent = dev;
> +
> +	clk = devm_clk_get_optional_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);

What is the use case for not listing the clock? Optional clocks are
generally because it was forgotten about in the initial driver, and
added later. In order to not break backwards compatibility, the clock
needs to be optional.

But this is a new driver. Why not make it required?

> +
> +	of_property_read_u32(np, "clock-frequency", &bus_freq);
> +
> +	if (bus_freq) {
> +		if (!clk) {
> +			dev_err(dev,
> +				"cannot use clock-frequency without a clock\n");
> +			return -EINVAL;
> +		}

And this then gets simpler.

> +
> +		rate = clk_get_rate(clk);
> +
> +		div = DIV_ROUND_UP(rate, 2 * bus_freq) - 1;
> +		if (div == 0 || div & ~MDIO_CFG_PRESCALE_MASK) {
> +			dev_err(dev, "Incorrect MDIO clock frequency\n");

I think "Out of range" is more correct.

	Andrew

Re: [PATCH net-next 2/2] net: mdio: add a driver for PIC64-HPSC/HX MDIO controller

[Charles Perry]

On Thu, Mar 19, 2026 at 05:55:44PM +0100, Andrew Lunn wrote:
> > +static int pic64hpsc_mdio_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct device *dev = &pdev->dev;
> > +	struct pic64hpsc_mdio_dev *priv;
> > +	struct mii_bus *bus;
> > +	unsigned long rate;
> > +	struct clk *clk;
> > +	u32 bus_freq;
> > +	u32 div;
> > +	int ret;
> > +
> > +	bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	priv = bus->priv;
> > +
> > +	priv->regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->regs))
> > +		return PTR_ERR(priv->regs);
> > +
> > +	bus->name = KBUILD_MODNAME;
> > +	bus->read = pic64hpsc_mdio_read;
> > +	bus->write = pic64hpsc_mdio_write;
> > +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> > +	bus->parent = dev;
> > +
> > +	clk = devm_clk_get_optional_enabled(dev, NULL);
> > +	if (IS_ERR(clk))
> > +		return PTR_ERR(clk);
> 
> What is the use case for not listing the clock? Optional clocks are
> generally because it was forgotten about in the initial driver, and
> added later. In order to not break backwards compatibility, the clock
> needs to be optional.
> 
> But this is a new driver. Why not make it required?
> 

My idea is that if someone wants to use whatever is the hardware default
or what was set by the bootloader, they have an option to do so. For that
reason, I made the clock and the clock-frequency optional. This is
something I can do without if you think it will homogenize better with new
drivers.

Now I just realized that I can achieve this by just making the
clock-frequency optional and not the clock.

Looking at some other MDIO drivers, I can see that there's different
policies on the "clock-frequency" not specified case:

 - mdio-airoha.c: use 2.5MHz if not specified
 - mdio-ipq4019.c: detect if the prescaler is the out of reset value,
                   choose 2.5MHz if that's the case.
 - mdio-mscc-miim.c and mdio-bcm-unimac.c: keep the current settings

I can implement any of the three policy above and don't have strong
opinions about this.

> > +
> > +	of_property_read_u32(np, "clock-frequency", &bus_freq);
> > +
> > +	if (bus_freq) {
> > +		if (!clk) {
> > +			dev_err(dev,
> > +				"cannot use clock-frequency without a clock\n");
> > +			return -EINVAL;
> > +		}
> 
> And this then gets simpler.
> 

Ok, I'll make the clock mandatory.

> > +
> > +		rate = clk_get_rate(clk);
> > +
> > +		div = DIV_ROUND_UP(rate, 2 * bus_freq) - 1;
> > +		if (div == 0 || div & ~MDIO_CFG_PRESCALE_MASK) {
> > +			dev_err(dev, "Incorrect MDIO clock frequency\n");
> 
> I think "Out of range" is more correct.

Ack.

Thank you,
Charles

> 
> 	Andrew

Re: [PATCH net-next 2/2] net: mdio: add a driver for PIC64-HPSC/HX MDIO controller

[Andrew Lunn]

> My idea is that if someone wants to use whatever is the hardware default
> or what was set by the bootloader, they have an option to do so. For that
> reason, I made the clock and the clock-frequency optional. This is
> something I can do without if you think it will homogenize better with new
> drivers.
> 
> Now I just realized that I can achieve this by just making the
> clock-frequency optional and not the clock.

It gets complicated pretty quickly, if you leave things open.

802.3 sets a maximum of 2.5Mhz. When this driver takes over the
hardware, and there is no hint from device tree what frequency to use,
but the hardware is configured to 50Mhz, what should it do? Trust the
bootloader? Or assume the bootloader or something else has messed it
up? Same goes for 1KHz?

Is the hardware default documented in the datasheet? Does it default
to 2.5Mhz?

>  - mdio-airoha.c: use 2.5MHz if not specified

I personally would do this. This keeps you in line with 802.3. Anybody
wanting to do anything else then uses clock-frequency, so the
intention is clearly documented.

	Andrew

Re: [PATCH net-next 2/2] net: mdio: add a driver for PIC64-HPSC/HX MDIO controller

[Charles Perry]

On Thu, Mar 19, 2026 at 08:53:01PM +0100, Andrew Lunn wrote:
> > My idea is that if someone wants to use whatever is the hardware default
> > or what was set by the bootloader, they have an option to do so. For that
> > reason, I made the clock and the clock-frequency optional. This is
> > something I can do without if you think it will homogenize better with new
> > drivers.
> > 
> > Now I just realized that I can achieve this by just making the
> > clock-frequency optional and not the clock.
> 
> It gets complicated pretty quickly, if you leave things open.
> 
> 802.3 sets a maximum of 2.5Mhz. When this driver takes over the
> hardware, and there is no hint from device tree what frequency to use,
> but the hardware is configured to 50Mhz, what should it do? Trust the
> bootloader? Or assume the bootloader or something else has messed it
> up? Same goes for 1KHz?
> 
> Is the hardware default documented in the datasheet? Does it default
> to 2.5Mhz?

No, it defaults to something rather slow: 610 KHz (assuming the input clock
is 156.25MHz, divides by 256)

> 
> >  - mdio-airoha.c: use 2.5MHz if not specified
> 
> I personally would do this. This keeps you in line with 802.3. Anybody
> wanting to do anything else then uses clock-frequency, so the
> intention is clearly documented.

Ok, sounds good, I'll use this policy.

Thanks,
Charles

Re: [PATCH net-next 2/2] net: mdio: add a driver for PIC64-HPSC/HX MDIO controller

[Andrew Lunn]

> +	platform_set_drvdata(pdev, bus);

Does not seem necessary.

	Andrew

Re: [PATCH net-next 2/2] net: mdio: add a driver for PIC64-HPSC/HX MDIO controller

[Charles Perry]

On Thu, Mar 19, 2026 at 05:56:55PM +0100, Andrew Lunn wrote:
> > +	platform_set_drvdata(pdev, bus);
> 
> Does not seem necessary.

Right, I forgot to remove that after I converted everything to devm_.

Thanks,
Charles

> 
> 	Andrew

Re: [PATCH net-next 2/2] net: mdio: add a driver for PIC64-HPSC/HX MDIO controller

[Andrew Lunn]

> +	u32 bus_freq;

> +	of_property_read_u32(np, "clock-frequency", &bus_freq);

clock-frequency is not required. So this can return an error, and
leave bus_freq untouched, which is a stack variable with random
contents.

	Andrew

Re: [PATCH net-next 2/2] net: mdio: add a driver for PIC64-HPSC/HX MDIO controller

[Charles Perry]

On Thu, Mar 19, 2026 at 06:03:09PM +0100, Andrew Lunn wrote:
> > +	u32 bus_freq;
> 
> > +	of_property_read_u32(np, "clock-frequency", &bus_freq);
> 
> clock-frequency is not required. So this can return an error, and
> leave bus_freq untouched, which is a stack variable with random
> contents.

You're right, I'll check the return value.

Thanks,
Charles

> 
> 	Andrew