Hi Ken,

On Wed, Jun 6, 2018 at 8:06 PM <m...@marvell.com> wrote:
>
> From: Ken Ma <m...@marvell.com>

aside, awesome email address for someone in embedded development :)

Some minor comments below

Reviewed-by: Chris Packham <judge.pack...@gmail.com>

> This patch adds a separate driver for the MDIO interface of the
> Marvell Ethernet controllers based on driver model. There are two
> reasons to have a separate driver rather than including it inside
> the MAC driver itself:
>   *) The MDIO interface is shared by all Ethernet ports, so a driver
>      must guarantee non-concurrent accesses to this MDIO interface. The
>      most logical way is to have a separate driver that handles this
>      single MDIO interface, used by all Ethernet ports.
>   *) The MDIO interface is the same between the existing mv643xx_eth
>      driver and the new mvneta/mvpp2 driver. Even though it is for now
>      only used by the mvneta/mvpp2 driver, it will in the future be
>      used by the mv643xx_eth driver as well.

Yay.

In u-boot mv643xx_eth is mvgbe.c. I've been looking at converting it
to DM but the first problem I hit was the MDIO interface so this is
very much welcome. Were you planning on adding an actual mv643xx_eth
driver or is that just a copy/paste from Linux?

>
> This driver supports SMI IEEE for 802.3 Clause 22 and XSMI for IEEE
> 802.3 Clause 45.
>
> This patch also adds device tree binding for marvell MDIO driver.
>
> Signed-off-by: Ken Ma <m...@marvell.com>
> ---
>
>  MAINTAINERS                                    |   1 +
>  arch/arm/Kconfig                               |   1 +
>  doc/device-tree-bindings/mdio/marvell-mdio.txt |  18 ++
>  drivers/mdio/Kconfig                           |  10 ++
>  drivers/mdio/Makefile                          |   1 +
>  drivers/mdio/mvmdio.c                          | 237 
> +++++++++++++++++++++++++
>  6 files changed, 268 insertions(+)
>  create mode 100644 doc/device-tree-bindings/mdio/marvell-mdio.txt
>  create mode 100644 drivers/mdio/mvmdio.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f66a904..fb58f17 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -137,6 +137,7 @@ T:  git git://git.denx.de/u-boot-marvell.git
>  F:     arch/arm/mach-kirkwood/
>  F:     arch/arm/mach-mvebu/
>  F:     drivers/ata/ahci_mvebu.c
> +F:     drivers/mdio/mvmdio.c
>
>  ARM MARVELL PXA
>  M:     Marek Vasut <ma...@denx.de>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index dde422b..aae4570 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -432,6 +432,7 @@ config ARCH_MVEBU
>         select DM_SPI
>         select DM_SPI_FLASH
>         select SPI
> +       select DM_MDIO
>
>  config TARGET_DEVKIT3250
>         bool "Support devkit3250"
> diff --git a/doc/device-tree-bindings/mdio/marvell-mdio.txt 
> b/doc/device-tree-bindings/mdio/marvell-mdio.txt
> new file mode 100644
> index 0000000..55db435
> --- /dev/null
> +++ b/doc/device-tree-bindings/mdio/marvell-mdio.txt
> @@ -0,0 +1,18 @@
> +* Marvell MDIO Ethernet Controller interface
> +
> +The Ethernet controllers of the Marvel Armada 3700 and Armada 7k/8k
> +have an identical unit that provides an interface with the MDIO bus.
> +This driver handles this MDIO interface.
> +
> +Mandatory properties:
> +SoC specific:
> +       - #address-cells: Must be <1>.
> +       - #size-cells: Must be <0>.
> +       - compatible: Should be "marvell,orion-mdio" (for SMI)
> +                               "marvell,xmdio"      (for XSMI)
> +       - reg: Base address and size SMI/XMSI bus.
> +
> +Optional properties:
> +       - mdio-name       - MDIO bus name
> +
> +For example, please refer to "mdio-bus.txt".
> diff --git a/drivers/mdio/Kconfig b/drivers/mdio/Kconfig
> index 76e3758..ce251c5 100644
> --- a/drivers/mdio/Kconfig
> +++ b/drivers/mdio/Kconfig
> @@ -15,4 +15,14 @@ config DM_MDIO
>           udevice or mii bus from its child phy node or
>           an ethernet udevice which the phy belongs to.
>
> +config MVMDIO
> +       bool "Marvell MDIO interface support"
> +       depends on DM_MDIO
> +       select PHYLIB
> +       help
> +         This driver supports the MDIO interface found in the network
> +         interface units of the Marvell EBU SoCs (Kirkwood, Orion5x,
> +         Dove, Armada 370, Armada XP, Armada 37xx and Armada7K/8K/8KP).
> +
> +         This driver is used by the MVPP2 and MVNETA drivers.
>  endmenu
> diff --git a/drivers/mdio/Makefile b/drivers/mdio/Makefile
> index 9b290c0..9f571aa 100644
> --- a/drivers/mdio/Makefile
> +++ b/drivers/mdio/Makefile
> @@ -4,3 +4,4 @@
>  # Author: Ken Ma<m...@marvell.com>
>
>  obj-$(CONFIG_DM_MDIO) += mdio-uclass.o
> +obj-$(CONFIG_MVMDIO) += mvmdio.o
> diff --git a/drivers/mdio/mvmdio.c b/drivers/mdio/mvmdio.c
> new file mode 100644
> index 0000000..b2c6636
> --- /dev/null
> +++ b/drivers/mdio/mvmdio.c
> @@ -0,0 +1,237 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018 Marvell International Ltd.
> + * Author: Ken Ma<m...@marvell.com>
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <dm/device-internal.h>
> +#include <dm/lists.h>
> +#include <miiphy.h>
> +#include <phy.h>
> +#include <asm/io.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define MVMDIO_SMI_DATA_SHIFT          0
> +#define MVMDIO_SMI_PHY_ADDR_SHIFT      16
> +#define MVMDIO_SMI_PHY_REG_SHIFT       21
> +#define MVMDIO_SMI_READ_OPERATION      BIT(26)
> +#define MVMDIO_SMI_WRITE_OPERATION     0
> +#define MVMDIO_SMI_READ_VALID          BIT(27)
> +#define MVMDIO_SMI_BUSY                        BIT(28)
> +
> +#define MVMDIO_XSMI_MGNT_REG           0x0
> +#define MVMDIO_XSMI_PHYADDR_SHIFT      16
> +#define MVMDIO_XSMI_DEVADDR_SHIFT      21
> +#define MVMDIO_XSMI_WRITE_OPERATION    (0x5 << 26)
> +#define MVMDIO_XSMI_READ_OPERATION     (0x7 << 26)
> +#define MVMDIO_XSMI_READ_VALID         BIT(29)
> +#define MVMDIO_XSMI_BUSY               BIT(30)
> +#define MVMDIO_XSMI_ADDR_REG           0x8
> +
> +#define MVMDIO_TIMEOUT                 10000
> +
> +struct mvmdio_priv {
> +       void *mdio_base;
> +};
> +
> +enum mvmdio_bus_type {
> +       BUS_TYPE_SMI,
> +       BUS_TYPE_XSMI
> +};
> +
> +/* Wait for the SMI unit to be ready for another operation */
> +static int mvmdio_smi_wait_ready(struct mii_dev *bus)
> +{
> +       u32 timeout = MVMDIO_TIMEOUT;
> +       struct mvmdio_priv *priv = bus->priv;
> +       u32 smi_reg;
> +
> +       /* Wait till the SMI is not busy */
> +       do {
> +               /* Read smi register */
> +               smi_reg = readl(priv->mdio_base);
> +               if (timeout-- == 0) {
> +                       debug("Error: SMI busy timeout\n");

Should this be dev_err(bus->parent, ...); ?

> +                       return -ETIME;
> +               }
> +       } while (smi_reg & MVMDIO_SMI_BUSY);
> +
> +       return 0;
> +}
> +
> +static int mvmdio_smi_read(struct mii_dev *bus, int addr,
> +                          int devad, int reg)
> +{
> +       struct mvmdio_priv *priv = bus->priv;
> +       u32 val;
> +       int ret;
> +
> +       if (devad != MDIO_DEVAD_NONE)
> +               return -EOPNOTSUPP;

mvgbe has a feature to read back the PHY address via the PHY Address
Register which I assume is configureable via hardware strapping. Looks
like the Armada-38x has the same feature although the mvneta driver
doesn't implement it. Should this new driver include this?

> +
> +       ret = mvmdio_smi_wait_ready(bus);
> +       if (ret < 0)
> +               return ret;
> +
> +       writel(((addr << MVMDIO_SMI_PHY_ADDR_SHIFT) |
> +               (reg << MVMDIO_SMI_PHY_REG_SHIFT)  |
> +               MVMDIO_SMI_READ_OPERATION),
> +              priv->mdio_base);
> +
> +       ret = mvmdio_smi_wait_ready(bus);
> +       if (ret < 0)
> +               return ret;
> +
> +       val = readl(priv->mdio_base);
> +       if (!(val & MVMDIO_SMI_READ_VALID)) {
> +               dev_err(bus->parent, "SMI bus read not valid\n");
> +               return -ENODEV;
> +       }
> +
> +       return val & GENMASK(15, 0);
> +}
> +
> +static int mvmdio_smi_write(struct mii_dev *bus, int addr, int devad,
> +                           int reg, u16 value)
> +{
> +       struct mvmdio_priv *priv = bus->priv;
> +       int ret;
> +
> +       if (devad != MDIO_DEVAD_NONE)
> +               return -EOPNOTSUPP;
> +
> +       ret = mvmdio_smi_wait_ready(bus);
> +       if (ret < 0)
> +               return ret;
> +
> +       writel(((addr << MVMDIO_SMI_PHY_ADDR_SHIFT) |
> +               (reg << MVMDIO_SMI_PHY_REG_SHIFT)  |
> +               MVMDIO_SMI_WRITE_OPERATION            |
> +               (value << MVMDIO_SMI_DATA_SHIFT)),
> +              priv->mdio_base);
> +
> +       return 0;
> +}
> +
> +static int mvmdio_xsmi_wait_ready(struct mii_dev *bus)
> +{
> +       u32 timeout = MVMDIO_TIMEOUT;
> +       struct mvmdio_priv *priv = bus->priv;
> +       u32 xsmi_reg;
> +
> +       /* Wait till the xSMI is not busy */
> +       do {
> +               /* Read xSMI register */
> +               xsmi_reg = readl(priv->mdio_base);
> +               if (timeout-- == 0) {
> +                       debug("xSMI busy time-out\n");

Should this be dev_err(bus->parent, ...); ?

> +                       return -ETIME;
> +               }
> +       } while (xsmi_reg & MVMDIO_XSMI_BUSY);
> +
> +       return 0;
> +}
> +
> +static int mvmdio_xsmi_read(struct mii_dev *bus, int addr,
> +                           int devad, int reg)
> +{
> +       struct mvmdio_priv *priv = bus->priv;
> +       int ret;
> +
> +       if (devad == MDIO_DEVAD_NONE)
> +               return -EOPNOTSUPP;
> +
> +       ret = mvmdio_xsmi_wait_ready(bus);
> +       if (ret < 0)
> +               return ret;
> +
> +       writel(reg & GENMASK(15, 0), priv->mdio_base + MVMDIO_XSMI_ADDR_REG);
> +       writel(((addr << MVMDIO_XSMI_PHYADDR_SHIFT) |
> +               (devad << MVMDIO_XSMI_DEVADDR_SHIFT) |
> +               MVMDIO_XSMI_READ_OPERATION),
> +              priv->mdio_base + MVMDIO_XSMI_MGNT_REG);
> +
> +       ret = mvmdio_xsmi_wait_ready(bus);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (!(readl(priv->mdio_base + MVMDIO_XSMI_MGNT_REG) &
> +             MVMDIO_XSMI_READ_VALID)) {
> +               dev_err(bus->parent, "XSMI bus read not valid\n");
> +               return -ENODEV;
> +       }
> +
> +       return readl(priv->mdio_base + MVMDIO_XSMI_MGNT_REG) & GENMASK(15, 0);
> +}
> +
> +static int mvmdio_xsmi_write(struct mii_dev *bus, int addr, int devad,
> +                            int reg, u16 value)
> +{
> +       struct mvmdio_priv *priv = bus->priv;
> +       int ret;
> +
> +       if (devad == MDIO_DEVAD_NONE)
> +               return -EOPNOTSUPP;
> +
> +       ret = mvmdio_xsmi_wait_ready(bus);
> +       if (ret < 0)
> +               return ret;
> +
> +       writel(reg & GENMASK(15, 0), priv->mdio_base + MVMDIO_XSMI_ADDR_REG);
> +       writel(((addr << MVMDIO_XSMI_PHYADDR_SHIFT) |
> +               (devad << MVMDIO_XSMI_DEVADDR_SHIFT) |
> +               MVMDIO_XSMI_WRITE_OPERATION | value),
> +              priv->mdio_base + MVMDIO_XSMI_MGNT_REG);
> +
> +       return 0;
> +}
> +
> +static int mvmdio_probe(struct udevice *dev)
> +{
> +       struct mii_dev **pbus = dev_get_uclass_platdata(dev);
> +       struct mii_dev *bus = *pbus;
> +       struct mvmdio_priv *priv;
> +       enum mvmdio_bus_type type;
> +
> +       priv = dev_get_priv(dev);
> +       priv->mdio_base = (void *)devfdt_get_addr(dev);
> +       bus->priv = priv;
> +
> +       type = (enum mvmdio_bus_type)dev_get_driver_data(dev);
> +       switch (type) {
> +       case BUS_TYPE_SMI:
> +               bus->read = mvmdio_smi_read;
> +               bus->write = mvmdio_smi_write;
> +               if (!bus->name)
> +                       snprintf(bus->name, MDIO_NAME_LEN,
> +                                "orion-mdio.%p", priv->mdio_base);
> +               break;
> +       case BUS_TYPE_XSMI:
> +               bus->read = mvmdio_xsmi_read;
> +               bus->write = mvmdio_xsmi_write;
> +               if (!bus->name)
> +                       snprintf(bus->name, MDIO_NAME_LEN,
> +                                "xmdio.%p", priv->mdio_base);
> +               break;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id mvmdio_ids[] = {
> +       { .compatible = "marvell,orion-mdio", .data = BUS_TYPE_SMI },
> +       { .compatible = "marvell,xmdio", .data = BUS_TYPE_XSMI },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(mvmdio) = {
> +       .name                   = "mvmdio",
> +       .id                     = UCLASS_MDIO,
> +       .of_match               = mvmdio_ids,
> +       .probe                  = mvmdio_probe,
> +       .priv_auto_alloc_size   = sizeof(struct mvmdio_priv),
> +};
> +
> --
> 1.9.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to