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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

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