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

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 Mamake@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 Mamake@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 Mamake@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