[U-Boot] [EXT] Re: [PATCH v3 1/2] dm: mdio: add a uclass for MDIO
Joe Hershberger
joe.hershberger at ni.com
Mon Jul 9 20:52:05 UTC 2018
On Thu, Jul 5, 2018 at 2:18 AM, Ken Ma <make at marvell.com> wrote:
> Hi Joe
>
> Please see my comments inline, thanks a lot for your kind and careful review!
>
> Yours,
> Ken
>
> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershberger at ni.com]
> Sent: 2018年6月20日 4:59
> To: Ken Ma <make at marvell.com>; Simon Glass <sjg at chromium.org>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Andy Shevchenko <andriy.shevchenko at linux.intel.com>; Eugeniy Paltsev <Eugeniy.Paltsev at synopsys.com>; Michal Simek <michal.simek at xilinx.com>; Alexander Graf <agraf at suse.de>; Joe Hershberger <joe.hershberger at ni.com>; Heinrich Schuchardt <xypron.glpk at gmx.de>; Stefan Roese <sr at denx.de>; Chris Packham <judge.packham at gmail.com>
> Subject: [EXT] Re: [U-Boot] [PATCH v3 1/2] dm: mdio: add a uclass for MDIO
>
> External Email
>
> ----------------------------------------------------------------------
> On Tue, Jun 12, 2018 at 11:33 PM, <make at marvell.com> wrote:
>> From: Ken Ma <make at 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 at marvell.com>
>> Reviewed-by: sjg at chromium.org, joe.hershberger at ni.com
>> ---
>>
>> Changes in v3:
>> - Move mdio uclass implementation to driver/net folder;
>> - Replace flat-tree functions with livetree functions and update codes
>> and comments to be consistent with driver-model codes style;
>> - Put struct mii_dev to uclass platdata to avoid the mdio alloc and
>> let driver model framework to alloc the memroy automatically,
>> meanwhile the mii bus link initialization is added,
>>
>> Changes in v2: None
>>
>> MAINTAINERS | 1 +
>> doc/device-tree-bindings/net/mdio-bus.txt | 54 ++++++++++++++
>> drivers/Kconfig | 2 +
>> drivers/net/Makefile | 1 +
>> drivers/net/mdio/Kconfig | 18 +++++
>> drivers/net/mdio/Makefile | 6 ++
>> drivers/net/mdio/mdio-uclass.c | 112 ++++++++++++++++++++++++++++++
>> include/dm/uclass-id.h | 1 +
>> include/net/mdio.h | 62 +++++++++++++++++
>> 9 files changed, 257 insertions(+)
>> create mode 100644 doc/device-tree-bindings/net/mdio-bus.txt
>> create mode 100644 drivers/net/mdio/Kconfig create mode 100644
>> drivers/net/mdio/Makefile create mode 100644
>> drivers/net/mdio/mdio-uclass.c create mode 100644 include/net/mdio.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS index 642c448..9e1fc83 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -335,6 +335,7 @@ M: Simon Glass <sjg at chromium.org>
>
> I think it makes sense for me to maintain this, unless Simon really wants it. He's not even Cc'ed, so maybe he doesn't know you are assigning it to him? Adding him.
> [Ken] Thank you for your remind, at first I do not put mdio under driver/net and I could not find a person to mainline it, so I add Simon as maintainer. I will update the maintainer to you. Thanks a lot!
>
>> S: Maintained
>> T: git git://git.denx.de/u-boot-dm.git
>> F: drivers/core/
>> +F: drivers/net/mdio/
>> F: include/dm/
>> F: test/dm/
>>
>> diff --git a/doc/device-tree-bindings/net/mdio-bus.txt
>> b/doc/device-tree-bindings/net/mdio-bus.txt
>> new file mode 100644
>> index 0000000..68d8b25
>> --- /dev/null
>> +++ b/doc/device-tree-bindings/net/mdio-bus.txt
>
> Is it reasonable to use Documentation/devicetree/bindings/net/mdio.txt
> from Linux?
> [Ken] Linux struct mii_bus implements reset_delay_us and reset_gpiod as optional properties while u-boot struct mii_bus has no such fields at all.
> Once uboot struct mii_bus contains reset_delay_us and reset_gpiod, we can add the related part in mdio_uclass.c and mdio_bus.txt.
While we don' support those optional properties, I still think the
bindings need to be shared. Optional need not be supported, so you not
supporting it is no problem. It can still be defined how it should
look if it was supported.
> BTW, I add optional property of mdio name to the txt.
That's fine. I think it's something that would be reasonable to add in
Linux as well.
> And u-boot mdio is based on uclass and actually it's a bus, so I follow spi_bus.txt(in uboot) naming and name it to mdio_bus.txt.
It's still fundamentally a bus in both cases and we are trying to keep
them as shared as possible. I don't see anything compelling so far why
they need to diverge from the outset. Also, as Michal commented, it
should be in a separate patch before the support is added.
>> @@ -0,0 +1,54 @@
>> +MDIO (Management Data Input/Output) busses
>> +
>> +MDIO busses can be described with a node for the MDIO master device
>> +and a set of child nodes for each phy on the bus.
>> +
>> +The MDIO node requires the following properties:
>> +- #address-cells - number of cells required to define phy address on
>> + the MDIO bus.
>> +- #size-cells - should be zero.
>> +- compatible - name of MDIO bus controller following generic names
>> + recommended practice.
>> +- reg - address and length of the MDIO register.
>> +
>> +Optional property:
>> +- mdio-name - MDIO bus name
>> +
>> +The child nodes of the MDIO driver are the individual PHY devices
>> +connected to this MDIO bus. They must have a "reg" property given the
>> +PHY address on the MDIO bus.
>> +- reg - (required) phy address in MDIO bus.
>> +
>> +Example for cp110 MDIO node at the SoC level:
>> + cp0_mdio: mdio at 12a200 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "marvell,orion-mdio";
>> + reg = <0x12a200 0x10>;
>> + mdio-name = "cp0-mdio";
>> + };
>> +
>> + cp0_xmdio: mdio at 12a600 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "marvell,xmdio";
>> + reg = <0x12a600 0x200>;
>> + mdio-name = "cp0-xmdio";
>> + };
>> +
>> +And at the board level, example for armada-8040-mcbin board:
>> + &cp0_mdio {
>> + ge_phy: ethernet-phy at 0 {
>> + reg = <0>;
>> + };
>> + };
>> +
>> + &cp0_xmdio {
>> + phy0: ethernet-phy at 0 {
>> + reg = <0>;
>> + };
>> +
>> + phy8: ethernet-phy at 8 {
>> + reg = <8>;
>> + };
>> + };
>> diff --git a/drivers/Kconfig b/drivers/Kconfig index 9e21b28..0e0982c
>> 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -54,6 +54,8 @@ source "drivers/mtd/Kconfig"
>>
>> source "drivers/net/Kconfig"
>>
>> +source "drivers/net/mdio/Kconfig"
>
> Seems like this should be in drivers/net/Kconfig.
> [Ken] Actually in kernel, MDIO is implemented in driver/net/phy, and then "driver/net/kocnifg" sources "drivers/net/phy/Kconfig".
> Since we implement mdio as an UCLASS, I think it's better to provide a single folder with name of "mdio" but not "phy", meanwhile "driver/net/Kconfig" can "drivers/net/mdio/Kconfig".
Yes, that's exactly what I was asking for. Source it in
driver/net/Kconfig, not in driver/Kconfig.
>> +
>> source "drivers/nvme/Kconfig"
>>
>> source "drivers/pci/Kconfig"
>> diff --git a/drivers/net/Makefile b/drivers/net/Makefile index
>> 584bfdf..1cda03f 100644
>> --- a/drivers/net/Makefile
>> +++ b/drivers/net/Makefile
>> @@ -70,3 +70,4 @@ obj-$(CONFIG_VSC9953) += vsc9953.o
>> obj-$(CONFIG_PIC32_ETH) += pic32_mdio.o pic32_eth.o
>> obj-$(CONFIG_DWC_ETH_QOS) += dwc_eth_qos.o
>> obj-$(CONFIG_FSL_PFE) += pfe_eth/
>> +obj-$(CONFIG_MDIO) += mdio/
>> diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig new
>> file mode 100644 index 0000000..533cc4a
>> --- /dev/null
>> +++ b/drivers/net/mdio/Kconfig
>> @@ -0,0 +1,18 @@
>> +#
>> +# MDIO infrastructure and drivers
>> +#
>> +
>> +menu "MDIO Support"
>> +
>> +config MDIO
>> + bool "Enable MDIO(Management Data Input/Output) drivers"
>
> Put a space before the open paren.
> [Ken] Thanks a lot for your kind remind!
>
>> + depends on DM
>
> I think this should default to enabled. Maybe that can wait until there is more adoption of adding DM MDIO support to MAC drivers.
>
>> + help
>> + Enable driver model for MDIO access.
>> + 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/net/mdio/Makefile b/drivers/net/mdio/Makefile new
>> file mode 100644 index 0000000..45ae502
>> --- /dev/null
>> +++ b/drivers/net/mdio/Makefile
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0+
>> +#
>> +# Copyright (C) 2018 Marvell International Ltd.
>> +# Author: Ken Ma<make at marvell.com>
>> +
>> +obj-$(CONFIG_MDIO) += mdio-uclass.o
>> diff --git a/drivers/net/mdio/mdio-uclass.c
>> b/drivers/net/mdio/mdio-uclass.c new file mode 100644 index
>> 0000000..5a23d21
>> --- /dev/null
>> +++ b/drivers/net/mdio/mdio-uclass.c
>> @@ -0,0 +1,112 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>
> Don't use C++ style comments.
> [Ken] This is the latest GPL license Naming Conventions for C files, please refer to Licenses/README.
> C source: // SPDX-License-Identifier: <SPDX License Expression>
> C header: /* SPDX-License-Identifier: <SPDX License Expression> */
> ASM: /* SPDX-License-Identifier: <SPDX License Expression> */
> scripts: # SPDX-License-Identifier: <SPDX License Expression>
> .rst: .. SPDX-License-Identifier: <SPDX License Expression>
> .dts{i}: // SPDX-License-Identifier: <SPDX License Expression>
Argh... not sure why that was the decision, but carry on...
>
>> +/*
>> + * Copyright (C) 2018 Marvell International Ltd.
>> + * Author: Ken Ma<make at 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 <net/mdio.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +int mdio_mii_bus_get(struct udevice *mdio_dev, struct mii_dev **busp)
>> +{
>> + *busp = (struct mii_dev *)dev_get_uclass_platdata(mdio_dev);
>> +
>> + return 0;
>> +}
>> +
>> +int mdio_device_get_from_phy(ofnode phy_node, struct udevice **devp)
>> +{
>> + ofnode mdio_node;
>> +
>> + mdio_node = ofnode_get_parent(phy_node);
>> + return uclass_get_device_by_ofnode(UCLASS_MDIO, mdio_node,
>> +devp); }
>> +
>> +int mdio_mii_bus_get_from_phy(ofnode phy_node, struct mii_dev **busp)
>> +{
>> + struct udevice *mdio_dev;
>> + int ret;
>> +
>> + ret = mdio_device_get_from_phy(phy_node, &mdio_dev);
>> + if (ret)
>> + return ret;
>> +
>> + *busp = (struct mii_dev *)dev_get_uclass_platdata(mdio_dev);
>> +
>> + return 0;
>> +}
>> +
>> +int mdio_device_get_from_eth(struct udevice *eth, struct udevice
>> +**devp) {
>> + struct ofnode_phandle_args phy_args;
>> + int ret;
>> +
>> + ret = dev_read_phandle_with_args(eth, "phy", NULL, 0, 0, &phy_args);
>> + if (!ret)
>> + return mdio_device_get_from_phy(phy_args.node, devp);
>> +
>> + /*
>> + * If there is no phy reference under the ethernet fdt node,
>> + * it is not an error since the ethernet device may do not use
>> + * mode; so in this case, the output mdio device pointer is set
>> + * as NULL.
>> + */
>> + *devp = NULL;
>> + return 0;
>> +}
>> +
>> +int mdio_mii_bus_get_from_eth(struct udevice *eth, struct mii_dev
>> +**busp) {
>> + struct udevice *mdio_dev;
>> + int ret;
>> +
>> + ret = mdio_device_get_from_eth(eth, &mdio_dev);
>> + if (ret)
>> + return ret;
>> +
>> + if (mdio_dev)
>> + *busp = (struct mii_dev *)dev_get_uclass_platdata(mdio_dev);
>> + else
>> + *busp = NULL;
>> +
>> + return 0;
>> +}
>> +
>> +static int mdio_uclass_pre_probe(struct udevice *dev) {
>> + struct mii_dev *bus = (struct mii_dev *)dev_get_uclass_platdata(dev);
>> + const char *name;
>> +
>> + /* initialize mii_dev struct fields */
>
> Include a comment that you are implementing mdio_alloc() setup here.
> [Ken] Thanks, I will update.
>
>> + INIT_LIST_HEAD(&bus->link);
>> +
>> + name = fdt_getprop(gd->fdt_blob, dev_of_offset(dev),
>> + "mdio-name", NULL);
>> + if (name)
>> + strncpy(bus->name, name, MDIO_NAME_LEN);
>> +
>> + return 0;
>> +}
>> +
>> +static int mdio_uclass_post_probe(struct udevice *dev) {
>> + struct mii_dev *bus = (struct mii_dev
>> +*)dev_get_uclass_platdata(dev);
>> +
>> + return mdio_register(bus);
>> +}
>> +
>> +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..504decd 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, /* Management Data Input/Output(MDIO) device */
>
> Again, include a space before '('.
> [Ken] Thanks a lot for your kind remind!
>
>> UCLASS_MISC, /* Miscellaneous device */
>> UCLASS_MMC, /* SD / MMC card or chip */
>> UCLASS_MOD_EXP, /* RSA Mod Exp device */
>> diff --git a/include/net/mdio.h b/include/net/mdio.h new file mode
>> 100644 index 0000000..5d8d703
>> --- /dev/null
>> +++ b/include/net/mdio.h
>> @@ -0,0 +1,62 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * Copyright (C) 2018 Marvell International Ltd.
>> + * Author: Ken Ma<make at marvell.com>
>> + */
>> +
>> +#ifndef _MDIO_H_
>> +#define _MDIO_H_
>> +
>> +#include <dm.h> /* Because we dereference struct udevice here */
>
> Me thinks that comment is incorrect. Please delete it.
> [Ken] Thanks, I will update it.
>> +#include <phy.h>
>> +
>> +/**
>> + * mdio_mii_bus_get() - Get mii bus from mdio udevice
>> + *
>> + * @mdio_dev: mdio udevice
>> + * @busp: returns mii bus
>> + * @returns 0 on success, error code otherwise.
>> + */
>> +int mdio_mii_bus_get(struct udevice *mdio_dev, struct mii_dev
>> +**busp);
>> +
>> +/**
>> + * mdio_device_get_from_phy() - Get the mdio udevice which the phy
>> +belongs to
>> + *
>> + * @phy_node: phy node offset
>> + * @devp: returns mdio udevice
>> + * @returns 0 on success, error code otherwise.
>> + */
>> +int mdio_device_get_from_phy(ofnode 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
>> + * @busp: returns mii bus
>> + * @returns 0 on success, error code otherwise.
>> + */
>> +int mdio_mii_bus_get_from_phy(ofnode phy_node, struct mii_dev
>> +**busp);
>> +
>> +/**
>> + * 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: returns mdio udevice
>> + * @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
>> + * @busp: returns mii bus
>> + * @returns 0 on success, error code otherwise.
>> + */
>> +int mdio_mii_bus_get_from_eth(struct udevice *eth, struct mii_dev
>> +**busp);
>> +
>> +#endif /* _MDIO_H_ */
>> --
>> 1.9.1
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
More information about the U-Boot
mailing list