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@xxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxx>

Thank you Maxime,
Charles

> 
> Maxime
>