Re: [PATCH net-next 3/3] enetc: Add ENETC PF level external MDIO support

[Andrew Lunn]

On Wed, Feb 13, 2019 at 01:02:23PM +0200, Claudiu Manoil wrote:
> Each ENETC PF has its own MDIO interface, the corresponding
> MDIO registers are mapped in the ENETC's Port register block.
> The current patch adds a driver for these PF level MDIO buses,
> so that each PF can manage directly its own external link.
> 
> Signed-off-by: Alex Marginean <alexandru.marginean@xxxxxxx>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@xxxxxxx>
> ---
>  drivers/net/ethernet/freescale/enetc/Makefile     |   3 +-
>  drivers/net/ethernet/freescale/enetc/enetc_mdio.c | 196 ++++++++++++++++++++++
>  drivers/net/ethernet/freescale/enetc/enetc_pf.c   |  13 ++
>  drivers/net/ethernet/freescale/enetc/enetc_pf.h   |   6 +
>  4 files changed, 217 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/freescale/enetc/enetc_mdio.c
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/Makefile b/drivers/net/ethernet/freescale/enetc/Makefile
> index 6976602..7139e41 100644
> --- a/drivers/net/ethernet/freescale/enetc/Makefile
> +++ b/drivers/net/ethernet/freescale/enetc/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_FSL_ENETC) += fsl-enetc.o
> -fsl-enetc-$(CONFIG_FSL_ENETC) += enetc.o enetc_cbdr.o enetc_ethtool.o
> +fsl-enetc-$(CONFIG_FSL_ENETC) += enetc.o enetc_cbdr.o enetc_ethtool.o \
> +				 enetc_mdio.o
>  fsl-enetc-$(CONFIG_PCI_IOV) += enetc_msg.o
>  fsl-enetc-objs := enetc_pf.o $(fsl-enetc-y)
>  
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
> new file mode 100644
> index 0000000..e71b4fd
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
> @@ -0,0 +1,196 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +/* Copyright 2019 NXP */
> +
> +#include <linux/mdio.h>
> +#include <linux/of_mdio.h>
> +
> +#include "enetc_pf.h"
> +
> +struct enetc_mdio_regs {
> +	u32	mdio_cfg;	/* MDIO configuration and status */
> +	u32	mdio_ctl;	/* MDIO control */
> +	u32	mdio_data;	/* MDIO data */
> +	u32	mdio_addr;	/* MDIO address */
> +};
> +
> +#define bus_to_enetc_regs(bus)	(struct enetc_mdio_regs __iomem *)((bus)->priv)
> +
> +#define ENETC_MDIO_REG_OFFSET	0x1c00
> +#define ENETC_MDC_DIV		258
> +#define MDIO_CFG_CLKDIV(x)	((((x) >> 1) & 0xff) << 8)
> +#define MDIO_CFG_BSY		BIT(0)
> +#define MDIO_CFG_RD_ER		BIT(1)
> +#define MDIO_CFG_ENC		BIT(6)
> +#define MDIO_CFG_NEG		BIT(23)
> +#define MDIO_CTL_DEV_ADDR(x)	((x) & 0x1f)
> +#define MDIO_CTL_PORT_ADDR(x)	(((x) & 0x1f) << 5)
> +#define MDIO_CTL_READ		BIT(15)
> +#define MDIO_DATA(x)		((x) & 0xffff)
> +
> +#define TIMEOUT	1000
> +static int enetc_wait_complete(struct device *dev,
> +			       struct enetc_mdio_regs __iomem *regs)
> +{
> +	unsigned int timeout;
> +
> +	timeout = TIMEOUT;
> +	while ((enetc_rd_reg(&regs->mdio_cfg) & MDIO_CFG_BSY) && timeout) {
> +		cpu_relax();
> +		timeout--;
> +	}
> +
> +	if (!timeout) {
> +		dev_err(dev, "timeout waiting for bus to be free\n");
> +		return -ETIMEDOUT;
> +	}

Hi Claudiu

readx_poll_timeout()

> +
> +	return 0;
> +}
> +
> +static int enetc_mdio_write(struct mii_bus *bus, int phy_id, int regnum,
> +			    u16 value)
> +{
> +	struct enetc_mdio_regs __iomem *regs = bus_to_enetc_regs(bus);
> +	u32 mdio_ctl, mdio_cfg;
> +	u16 dev_addr;
> +	int ret;
> +
> +	mdio_cfg = MDIO_CFG_CLKDIV(ENETC_MDC_DIV) | MDIO_CFG_NEG;

What does MDIO_CFG_NEG mean?

> +	if (regnum & MII_ADDR_C45) {
> +		/* clause 45 */

Does the comment add anything useful?

> +		dev_addr = (regnum >> 16) & 0x1f;
> +		mdio_cfg |= MDIO_CFG_ENC;

Maybe MDIO_CFG_ENC could be called MDIO_CFG_C45? Assuming that is what
it actually means?

> +int enetc_mdio_probe(struct enetc_pf *pf)
> +{
> +	struct device *dev = &pf->si->pdev->dev;
> +	struct enetc_mdio_regs __iomem *regs;
> +	struct mii_bus *bus;
> +	int ret;
> +
> +	bus = mdiobus_alloc_size(sizeof(regs));
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->name = "Freescale ENETC MDIO Bus";
> +	bus->read = enetc_mdio_read;
> +	bus->write = enetc_mdio_write;
> +	bus->parent = dev;
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> +
> +	/* store the enetc mdio base address for this bus */
> +	regs = pf->si->hw.port + ENETC_MDIO_REG_OFFSET;
> +	bus->priv = regs;
> +
> +	ret = of_mdiobus_register(bus, dev->of_node);

It is a good idea to have an mdio node which contains the list of
PHYs. You can get into odd situations if you don't do that.

>  
> @@ -770,12 +771,23 @@ static int enetc_of_get_phy(struct enetc_ndev_priv *priv)
>  		priv->phy_node = of_node_get(np);
>  	}
>  
> +	if (!of_phy_is_fixed_link(np)) {
> +		err = enetc_mdio_probe(pf);
> +		if (err) {
> +			dev_err(priv->dev, "MDIO bus registration failed\n");

enetc_mdio_probe() already prints an error message. You don't really
need both.

     Andrew