[U-Boot] [PATCH v2 1/2] dm: mdio: add a uclass for MDIO

Simon Glass
2018-06-08T21:59:21Z

Hi Ken,

On 6 June 2018 at 18:08, <make@marvell.com> wrote:
> From: Ken Ma <make@marvell.com>
>
> Add a uclass which provides access to MDIO busses and includes
> operations required by MDIO.
> The implementation is based on the existing mii/phy/mdio data
> structures and APIs.
> This patch also adds evice tree binding for MDIO bus.
>
> Signed-off-by: Ken Ma <make@marvell.com>
> ---
>
> Changes in v2: None
>
> MAINTAINERS | 1 +
> doc/device-tree-bindings/mdio/mdio-bus.txt | 54 +++++++++++++
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/mdio/Kconfig | 18 +++++
> drivers/mdio/Makefile | 6 ++
> drivers/mdio/mdio-uclass.c | 119 +++++++++++++++++++++++++++++
> include/dm/uclass-id.h | 1 +
> include/mdio.h | 62 +++++++++++++++
> 9 files changed, 264 insertions(+)
> create mode 100644 doc/device-tree-bindings/mdio/mdio-bus.txt
> create mode 100644 drivers/mdio/Kconfig
> create mode 100644 drivers/mdio/Makefile
> create mode 100644 drivers/mdio/mdio-uclass.c
> create mode 100644 include/mdio.h

When adding a new uclass, please add a sandbox driver and a test in
test/dm/mdio.c

> +menu "MDIO Support"
> +
> +config DM_MDIO

I don't see any CONFIG_MDIO option, so you can just make this MDIO.
The DM_ prefix is for things that are being converted to driver model,
but still have legacy code.

> + bool "Enable Driver Model for MDIO drivers"
> + depends on DM
> + help
> + Enable driver model for MDIO access.

What does MDIO stand for? You should use the full name in help at least once.

> + Drivers provide methods to management data
> + Input/Output.
> + MDIO uclass provides interfaces to get mdio
> + udevice or mii bus from its child phy node or
> + an ethernet udevice which the phy belongs to.
> +
> +endmenu
> diff --git a/drivers/mdio/Makefile b/drivers/mdio/Makefile
> new file mode 100644
> index 0000000..9b290c0
> --- /dev/null
> +++ b/drivers/mdio/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Copyright (C) 2018 Marvell International Ltd.
> +# Author: Ken Ma<make@marvell.com>
> +
> +obj-$(CONFIG_DM_MDIO) += mdio-uclass.o
> diff --git a/drivers/mdio/mdio-uclass.c b/drivers/mdio/mdio-uclass.c
> new file mode 100644
> index 0000000..251776b
> --- /dev/null
> +++ b/drivers/mdio/mdio-uclass.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018 Marvell International Ltd.
> + * Author: Ken Ma<make@marvell.com>
> + */
> +
> +#include <common.h>
> +#include <fdtdec.h>
> +#include <errno.h>
> +#include <dm.h>
> +#include <dm/uclass.h>
> +#include <dm/uclass-internal.h>
> +#include <miiphy.h>
> +#include <mdio.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +int mdio_mii_bus_get(struct udevice *mdio_dev, struct mii_dev **bus)
> +{
> + *bus = *(struct mii_dev **)dev_get_uclass_platdata(mdio_dev);
> +
> + return 0;
> +}
> +
> +int mdio_device_get_from_phy(int phy_node, struct udevice **devp)

Please can you use livetree functions, like dev_read_...() and
ofnode_..() instead the flat-tree functions?

> +{
> + int mdio_off;
> +
> + mdio_off = fdt_parent_offset(gd->fdt_blob, phy_node);
> + return uclass_get_device_by_of_offset(UCLASS_MDIO, mdio_off,
> + devp);
> +}
> +

[..]

> +static int mdio_uclass_pre_probe(struct udevice *dev)
> +{
> + struct mii_dev **pbus = dev_get_uclass_platdata(dev);
> + struct mii_dev *bus;
> + const char *name;
> +
> + bus = mdio_alloc();
> + if (!bus) {
> + printf("Failed to allocate MDIO bus @%p\n",
> + devfdt_get_addr_ptr(dev));

debug()

> + return -1;

Should return a real error, like -ENOMEM

But can you not put struct mii_dev in the private data, to avoid the
alloc? If not, you need a remove() method to free this data.

> + }
> +
> + name = fdt_getprop(gd->fdt_blob, dev_of_offset(dev),
> + "mdio-name", NULL);
> + if (name)
> + strncpy(bus->name, name, MDIO_NAME_LEN);
> + *pbus = bus;
> +
> + return 0;
> +}
> +
> +static int mdio_uclass_post_probe(struct udevice *dev)
> +{
> + struct mii_dev **pbus = dev_get_uclass_platdata(dev);
> +
> + return mdio_register(*pbus);
> +}
> +
> +UCLASS_DRIVER(mdio) = {
> + .id = UCLASS_MDIO,
> + .name = "mdio",
> + .pre_probe = mdio_uclass_pre_probe,
> + .post_probe = mdio_uclass_post_probe,
> + .per_device_platdata_auto_alloc_size = sizeof(struct mii_dev *),
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index d7f9df3..170a0cc 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -49,6 +49,7 @@ enum uclass_id {
> UCLASS_LPC, /* x86 'low pin count' interface */
> UCLASS_MAILBOX, /* Mailbox controller */
> UCLASS_MASS_STORAGE, /* Mass storage device */
> + UCLASS_MDIO, /* MDIO device */

Spell out MDIO if room

> UCLASS_MISC, /* Miscellaneous device */
> UCLASS_MMC, /* SD / MMC card or chip */
> UCLASS_MOD_EXP, /* RSA Mod Exp device */
> diff --git a/include/mdio.h b/include/mdio.h
> new file mode 100644
> index 0000000..50458b1
> --- /dev/null
> +++ b/include/mdio.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2018 Marvell International Ltd.
> + * Author: Ken Ma<make@marvell.com>
> + */
> +
> +#ifndef _MDIO_H_
> +#define _MDIO_H_
> +
> +#include <dm.h> /* Because we dereference struct udevice here */
> +#include <phy.h>
> +
> +/**
> + * mdio_mii_bus_get() - Get mii bus from mdio udevice
> + *
> + * @mdio_dev: mdio udevice
> + * @bus: mii bus

returns mii bus

Also please can you add a 'p' suffix on pointers that are used to return values.

struct mii_dev **busp

That is consistent with how things are done in the driver-model code.

> + * @returns 0 on success, error code otherwise.
> + */
> +int mdio_mii_bus_get(struct udevice *mdio_dev, struct mii_dev **bus);
> +
> +/**
> + * mdio_device_get_from_phy() - Get the mdio udevice which the phy belongs to
> + *
> + * @phy_node: phy node offset

Should use ofnode I think, same below

> + * @devp: mdio udevice
> + * @returns 0 on success, error code otherwise.
> + */
> +int mdio_device_get_from_phy(int phy_node, struct udevice **devp);
> +
> +/**
> + * mdio_mii_bus_get_from_phy() - Get the mii bus which the phy belongs to
> + *
> + * @phy_node: phy node offset
> + * @bus: mii bus
> + * @returns 0 on success, error code otherwise.
> + */
> +int mdio_mii_bus_get_from_phy(int phy_node, struct mii_dev **bus);
> +
> +/**
> + * mdio_device_get_from_eth() - When there is a phy reference of "phy = <&...>"
> + * under an ethernet udevice fdt node, this function can
> + * get the mdio udevice which the phy belongs to
> + *
> + * @dev: the ethernet udevice which contains the phy reference
> + * @devp: mdio udevice

Yes devp is a good name.

> + * @returns 0 on success, error code otherwise.
> + */
> +int mdio_device_get_from_eth(struct udevice *eth, struct udevice **devp);
> +
> +/**
> + * mdio_mii_bus_get_from_eth() - When there is a phy reference of
> + * "phy = <&...>" under an ethernet udevice fdt node, this
> + * function can get the mii bus which the phy belongs to
> + *
> + * @eth: the ethernet udevice which contains the phy reference
> + * @bus: mii bus
> + * @returns 0 on success, error code otherwise.
> + */
> +int mdio_mii_bus_get_from_eth(struct udevice *eth, struct mii_dev **bus);
> +
> +#endif /* _MDIO_H_ */
> --
> 1.9.1
>

Regards,
Simon