Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

[Wang, Dongsheng]

On 2018/10/26 3:24, Andrew Lunn wrote:
> On Thu, Oct 25, 2018 at 06:09:15PM +0800, Wang Dongsheng wrote:
>> Use "phy-handle" to porint an internal MDIO device port.
> Hi Dongsheng
>
> You are basically defining how all future ACPI based MAC drivers get
> access to their PHY. This needs to become part of the ACPI standard,
> etc.
>
> This code should not be hidden away in the emac driver. It needs to be
> placed somewhere public so other drivers can use it. And it needs good
> documentation, including an example of what needs to go into the ACPI
> tables, etc.
Hi Andrew

I saw AppliedMicro(apm) xgene has used "phy-handle" for ACPI method, so
I guess "phy-handle" has become part of the ACPI standard. But I cannot
make sure.
I tried to confirm the property is defined in the document(Like DT
binding). However, I did not find any documentation on the description
property definition on the UEFI/ACPICA website.
But when I was reading Documentation/acpi/DSD-properties-rules.txt, my
understanding is we should try to conform to DT bindings. So maybe ACPI
doesn't have such a document, just DT bindings.

Cheers,
Dongsheng

> Thanks
> 	Andrew
>

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

[Timur Tabi]

On 10/25/18 9:18 PM, Wang, Dongsheng wrote:
> But when I was reading Documentation/acpi/DSD-properties-rules.txt, my
> understanding is we should try to conform to DT bindings. So maybe ACPI
> doesn't have such a document, just DT bindings.

There was an attempt to document DSDs, but it was abandoned after a while.

https://github.com/ahs3/dsd

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

[Wang, Dongsheng]

On 2018/10/26 10:37, Timur Tabi wrote:
> On 10/25/18 9:18 PM, Wang, Dongsheng wrote:
>> But when I was reading Documentation/acpi/DSD-properties-rules.txt, my
>> understanding is we should try to conform to DT bindings. So maybe ACPI
>> doesn't have such a document, just DT bindings.
> There was an attempt to document DSDs, but it was abandoned after a while.
>
> https://github.com/ahs3/dsd
>

Yes, here's a database concept, and I asked some Intel guys, the answer
I got was there is no such database or document. :(


Cheers,

Dongsheng

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

[Andrew Lunn]

On Fri, Oct 26, 2018 at 03:04:25AM +0000, Wang, Dongsheng wrote:
> On 2018/10/26 10:37, Timur Tabi wrote:
> > On 10/25/18 9:18 PM, Wang, Dongsheng wrote:
> >> But when I was reading Documentation/acpi/DSD-properties-rules.txt, my
> >> understanding is we should try to conform to DT bindings. So maybe ACPI
> >> doesn't have such a document, just DT bindings.
> > There was an attempt to document DSDs, but it was abandoned after a while.
> >
> > https://github.com/ahs3/dsd
> >
> 
> Yes, here's a database concept, and I asked some Intel guys, the answer
> I got was there is no such database or document. :(

Hi Dongsheng

If there is no clear documentation for ACPI, it becomes even more
important that the xgene code is refactored into a central location,
and you make use of it. We really need to avoid every ACPI ethernet
driver doing its own thing.

       Thanks
	Andrew

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

[Wang, Dongsheng]

On 2018/10/26 21:12, Andrew Lunn wrote:
> On Fri, Oct 26, 2018 at 03:04:25AM +0000, Wang, Dongsheng wrote:
>> On 2018/10/26 10:37, Timur Tabi wrote:
>>> On 10/25/18 9:18 PM, Wang, Dongsheng wrote:
>>>> But when I was reading Documentation/acpi/DSD-properties-rules.txt, my
>>>> understanding is we should try to conform to DT bindings. So maybe ACPI
>>>> doesn't have such a document, just DT bindings.
>>> There was an attempt to document DSDs, but it was abandoned after a while.
>>>
>>> https://github.com/ahs3/dsd
>>>
>> Yes, here's a database concept, and I asked some Intel guys, the answer
>> I got was there is no such database or document. :(
> Hi Dongsheng
>
> If there is no clear documentation for ACPI, it becomes even more
> important that the xgene code is refactored into a central location,
> and you make use of it. We really need to avoid every ACPI ethernet
> driver doing its own thing.

However, without a document specifying MDIO and phy-handle, it is almost
difficult for us to do this. Because maybe the ACPI device or property
corresponding to each platform is different.
Just like APM looks different to us. APM's MDIO adev doesn't describe
the concept of port, and our platform does. Besides, I cannot get the
ACPI table of APM or other manufacturers.
The table of ACPI cannot be obtained from kernel source as easily as DT.
We can't know without a platform to do ACPI dump. Unless some of the
manufacturers have pushed the table to upstream.
So I think we might have a hard time doing this without a document. And
it's likely that this work involves code modifications by BIOS vendors.

Cheers,
Dongsheng

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

[Andrew Lunn]

On Mon, Oct 29, 2018 at 02:39:36AM +0000, Wang, Dongsheng wrote:
> On 2018/10/26 21:12, Andrew Lunn wrote:
> > On Fri, Oct 26, 2018 at 03:04:25AM +0000, Wang, Dongsheng wrote:
> >> On 2018/10/26 10:37, Timur Tabi wrote:
> >>> On 10/25/18 9:18 PM, Wang, Dongsheng wrote:
> >>>> But when I was reading Documentation/acpi/DSD-properties-rules.txt, my
> >>>> understanding is we should try to conform to DT bindings. So maybe ACPI
> >>>> doesn't have such a document, just DT bindings.
> >>> There was an attempt to document DSDs, but it was abandoned after a while.
> >>>
> >>> https://github.com/ahs3/dsd
> >>>
> >> Yes, here's a database concept, and I asked some Intel guys, the answer
> >> I got was there is no such database or document. :(
> > Hi Dongsheng
> >
> > If there is no clear documentation for ACPI, it becomes even more
> > important that the xgene code is refactored into a central location,
> > and you make use of it. We really need to avoid every ACPI ethernet
> > driver doing its own thing.
> 
> However, without a document specifying MDIO and phy-handle, it is almost
> difficult for us to do this. Because maybe the ACPI device or property
> corresponding to each platform is different.
> Just like APM looks different to us. APM's MDIO adev doesn't describe
> the concept of port, and our platform does. Besides, I cannot get the
> ACPI table of APM or other manufacturers.
> The table of ACPI cannot be obtained from kernel source as easily as DT.
> We can't know without a platform to do ACPI dump. Unless some of the
> manufacturers have pushed the table to upstream.
> So I think we might have a hard time doing this without a document. And
> it's likely that this work involves code modifications by BIOS vendors.

Hi Dongsheng

There are two different options here.

1) Everybody does their own thing, ignoring what everybody else has
done, and invents their own wheel. There is no shared code, no shared
description, everybody has their own bugs, etc. ACPI as a standard is
pointless for Ethernet MDIOs and PHYs because it is not a standard,
everybody does something different.

2) Somebody takes the time to design a concept for Ethernet PHYs and
MDIO busses using ACPI. They implement the common code, try to modify
any existing users if possible, and submit the whole thing to become
part of ACPI 6.3.

I would really prefer we go the second route here. It is more initial
effort, but in the long run, everybody benefits.

	Andrew

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

[Wang, Dongsheng]

On 2018/10/29 20:40, Andrew Lunn wrote:
> On Mon, Oct 29, 2018 at 02:39:36AM +0000, Wang, Dongsheng wrote:
>> On 2018/10/26 21:12, Andrew Lunn wrote:
>>> On Fri, Oct 26, 2018 at 03:04:25AM +0000, Wang, Dongsheng wrote:
>>>> On 2018/10/26 10:37, Timur Tabi wrote:
>>>>> On 10/25/18 9:18 PM, Wang, Dongsheng wrote:
>>>>>> But when I was reading Documentation/acpi/DSD-properties-rules.txt, my
>>>>>> understanding is we should try to conform to DT bindings. So maybe ACPI
>>>>>> doesn't have such a document, just DT bindings.
>>>>> There was an attempt to document DSDs, but it was abandoned after a while.
>>>>>
>>>>> https://github.com/ahs3/dsd
>>>>>
>>>> Yes, here's a database concept, and I asked some Intel guys, the answer
>>>> I got was there is no such database or document. :(
>>> Hi Dongsheng
>>>
>>> If there is no clear documentation for ACPI, it becomes even more
>>> important that the xgene code is refactored into a central location,
>>> and you make use of it. We really need to avoid every ACPI ethernet
>>> driver doing its own thing.
>> However, without a document specifying MDIO and phy-handle, it is almost
>> difficult for us to do this. Because maybe the ACPI device or property
>> corresponding to each platform is different.
>> Just like APM looks different to us. APM's MDIO adev doesn't describe
>> the concept of port, and our platform does. Besides, I cannot get the
>> ACPI table of APM or other manufacturers.
>> The table of ACPI cannot be obtained from kernel source as easily as DT.
>> We can't know without a platform to do ACPI dump. Unless some of the
>> manufacturers have pushed the table to upstream.
>> So I think we might have a hard time doing this without a document. And
>> it's likely that this work involves code modifications by BIOS vendors.
> Hi Dongsheng
>
> There are two different options here.
>
> 1) Everybody does their own thing, ignoring what everybody else has
> done, and invents their own wheel. There is no shared code, no shared
> description, everybody has their own bugs, etc. ACPI as a standard is
> pointless for Ethernet MDIOs and PHYs because it is not a standard,
> everybody does something different.
>
> 2) Somebody takes the time to design a concept for Ethernet PHYs and
> MDIO busses using ACPI. They implement the common code, try to modify
> any existing users if possible, and submit the whole thing to become
> part of ACPI 6.3.
>
> I would really prefer we go the second route here. It is more initial
> effort, but in the long run, everybody benefits.

Yes, I also would like the second one. I will reply in a few days.

Cheers,
Dongsheng

> 	Andrew
>

[RFC PATCH 0/3] acpi: Add acpi mdio support code

[Wang Dongsheng]

Originally I just push "phy-handle" support for ACPI on the QCOM QDF2400
platform. After some discussion and following Andrew's advice, I send
out with a generic version of ACPI.

Current there is no clear documentation about MDIO/PHY for ACPI, so when
I reading some documents about ACPI [1], I think we just need to reuse the
DT binding in the ACPI.[2]. However, this series of patches are not
fully compatible with all contents specified in DT binding.

The most important thing about this iseries is link the phy device and
fwnode of acpi. Besides, we need to carry out bus scan at the mdio
register. Therefore, I am not compatible with more DT binding properties
in this series of patches. More support will be in the follow-up patches
support, or some people do the support.

Example:
Based on ACPI doc:
    Documentation/acpi/dsd/data-node-references.txt
    Documentation/acpi/dsd/graph.txt
With _DSD device properties we can finally do this:
    Device (MDIO) {
        Name (_DSD, Package () {
            ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () { Package () { "ethernet-phy@0", PHY0 }, }
        })
        Name (PHY0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "reg", 0x0 }, }
        })
    }

    Device (MACO) {
        Name (_DSD, Package () {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "phy-handle", \_SB.MDIO, "ethernet-phy@0" }, }
        })
    }

Tested: QDF2400 (ACPI), buildin/insmod/rmmod

[1]:
Documentation/acpi/dsd/data-node-references.txt
Documentation/acpi/dsd/graph.txt

[2]:
Documentation/devicetree/bindings/phy/phy-bindings.txt
https://lore.kernel.org/patchwork/patch/597296/

Wang Dongsheng (3):
  acpi: Add acpi mdio support code
  net: qcom/emac: split phy_config to mdio bus create and get phy device
  net: qcom/emac: add phy-handle support for ACPI

 drivers/acpi/Kconfig                          |   6 +
 drivers/acpi/Makefile                         |   1 +
 drivers/acpi/acpi_mdio.c                      | 167 ++++++++++++++++++
 drivers/net/ethernet/qualcomm/emac/emac-mac.c |  19 +-
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 142 ++++++++++-----
 drivers/net/phy/mdio_bus.c                    |   3 +
 include/linux/acpi_mdio.h                     |  82 +++++++++
 7 files changed, 369 insertions(+), 51 deletions(-)
 create mode 100644 drivers/acpi/acpi_mdio.c
 create mode 100644 include/linux/acpi_mdio.h

-- 
2.18.0

[RFC PATCH 1/3] acpi: Add acpi mdio support code

[Wang Dongsheng]

Add support for parsing the ACPI data node for PHY devices on an MDIO bus.
The current implementation depend on mdio bus scan.
With _DSD device properties we can finally do this:

    Device (MDIO) {
        Name (_DSD, Package () {
            ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () { Package () { "ethernet-phy@0", PHY0 }, }
        })
        Name (PHY0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "reg", 0x0 }, }
        })
    }

    Device (MACO) {
        Name (_DSD, Package () {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "phy-handle", \_SB.MDIO, "ethernet-phy@0" }, }
        })
    }

Documentations:
    The DT "phy-handle" binding that we reuse for ACPI is documented in
    Documentation/devicetree/bindings/phy/phy-bindings.txt

    Documentation/acpi/dsd/data-node-references.txt
    Documentation/acpi/dsd/graph.txt

Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
---
 drivers/acpi/Kconfig       |   6 ++
 drivers/acpi/Makefile      |   1 +
 drivers/acpi/acpi_mdio.c   | 167 +++++++++++++++++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c |   3 +
 include/linux/acpi_mdio.h  |  82 ++++++++++++++++++
 5 files changed, 259 insertions(+)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 9705fc986da9..0fefa3410ce9 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -252,6 +252,12 @@ config ACPI_PROCESSOR_IDLE
 config ACPI_MCFG
 	bool
 
+config ACPI_MDIO
+	def_tristate PHYLIB
+	depends on PHYLIB
+	help
+	  ACPI MDIO bus (Ethernet PHY) accessors
+
 config ACPI_CPPC_LIB
 	bool
 	depends on ACPI_PROCESSOR
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 6d59aa109a91..ec7461a064fc 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -41,6 +41,7 @@ acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
 acpi-y				+= pci_root.o pci_link.o pci_irq.o
 obj-$(CONFIG_ACPI_MCFG)		+= pci_mcfg.o
+acpi-$(CONFIG_ACPI_MDIO)	+= acpi_mdio.o
 acpi-y				+= acpi_lpss.o acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
diff --git a/drivers/acpi/acpi_mdio.c b/drivers/acpi/acpi_mdio.c
new file mode 100644
index 000000000000..293bf9a63197
--- /dev/null
+++ b/drivers/acpi/acpi_mdio.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Lots of code in this file is copy from drivers/of/of_mdio.c
+// Copyright (c) 2018 Huaxintong Semiconductor Technology Co., Ltd.
+
+#include <linux/acpi.h>
+#include <linux/acpi_mdio.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/netdevice.h>
+#include <linux/err.h>
+#include <linux/phy.h>
+#include <linux/phy_fixed.h>
+
+/* Helper function for acpi_phy_find_device */
+static int phy_match(struct device *dev, void *fwnode)
+{
+	return dev->fwnode == fwnode;
+}
+
+/**
+ * acpi_phy_find_device - Give a PHY fwnode, find the phy_device
+ * @fwnode: Pointer to the phy's acpi data node
+ *
+ * If successful, returns a pointer to the phy_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure.
+ */
+struct phy_device *acpi_phy_find_device(struct fwnode_handle *fwnode)
+{
+	struct device *d;
+	struct mdio_device *mdiodev;
+
+	if (!fwnode)
+		return NULL;
+
+	d = bus_find_device(&mdio_bus_type, NULL, fwnode, phy_match);
+	if (d) {
+		mdiodev = to_mdio_device(d);
+		if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
+			return to_phy_device(d);
+		put_device(d);
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(acpi_phy_find_device);
+
+static int do_acpi_mdiodev_match(struct fwnode_handle *fwnode,
+				 struct mdio_device *mdiodev)
+{
+	struct device *dev = &mdiodev->dev;
+	struct fwnode_handle *child_node;
+	int addr;
+	int ret;
+
+	fwnode_for_each_child_node(fwnode, child_node) {
+		do {
+			addr = acpi_mdio_parse_addr(dev, child_node);
+			if (addr < 0)
+				break;
+
+			if (mdiodev->addr != addr)
+				break;
+
+			dev->fwnode = child_node;
+			return 0;
+		} while (0);
+
+		/* Walk hierarchical extension data nodes */
+		ret = do_acpi_mdiodev_match(child_node, mdiodev);
+		if (!ret)
+			return 0;
+	}
+
+	return -ENODEV;
+}
+
+/* Walk the list of subnodes of a mdio bus and look for a node that
+ * matches the mdio device's address with its 'reg' property. If
+ * found, set the fwnode pointer for the mdio device.
+ */
+void acpi_mdiobus_link_mdiodev(struct mii_bus *bus,
+			       struct mdio_device *mdiodev)
+{
+	struct device *dev = &mdiodev->dev;
+
+	if (dev->fwnode || !bus->dev.fwnode)
+		return;
+
+	if (!has_acpi_companion(&bus->dev))
+		return;
+
+	do_acpi_mdiodev_match(bus->dev.fwnode, mdiodev);
+}
+
+/**
+ * acpi_phy_connect - Connect to the phy
+ * @dev: pointer to net_device claiming the phy
+ * @fwnode: Pointer to ACPI data node for the PHY
+ * @hndlr: Link state callback for the network device
+ * @flags: flags to pass to the PHY
+ * @iface: PHY data interface type
+ *
+ * If successful, returns a pointer to the phy_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure. The
+ * refcount must be dropped by calling phy_disconnect() or phy_detach().
+ */
+struct phy_device *acpi_phy_connect(struct net_device *dev,
+				    struct fwnode_handle *fwnode,
+				    void (*hndlr)(struct net_device *),
+				    u32 flags,
+				    phy_interface_t iface)
+{
+	struct phy_device *phy = acpi_phy_find_device(fwnode);
+	int ret;
+
+	if (!phy)
+		return NULL;
+
+	phy->dev_flags = flags;
+
+	ret = phy_connect_direct(dev, phy, hndlr, iface);
+
+	/* refcount is held by phy_connect_direct() on success */
+	put_device(&phy->mdio.dev);
+
+	return ret ? NULL : phy;
+}
+EXPORT_SYMBOL(acpi_phy_connect);
+
+static int acpi_mdio_node_verify(struct fwnode_handle *fwnode)
+{
+	return is_acpi_device_node(fwnode) ? 0 : -ENODEV;
+}
+
+static int fwnode_mdiobus_verify_node(struct fwnode_handle *fwnode)
+{
+	if (!is_acpi_node(fwnode))
+		return -ENODEV;
+	return acpi_mdio_node_verify(fwnode);
+}
+
+/**
+ * acpi_mdiobus_register - Register mii_bus and create PHYs
+ * @mdio: pointer to mii_bus structure
+ * @fwnode: pointer to fw_node of MDIO bus.
+ *
+ * This function registers the mii_bus structure and scan the phy_devices
+ * for each child node of @fwnode.
+ */
+int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
+{
+	int ret;
+
+	if (!fwnode)
+		return mdiobus_register(mdio);
+
+	ret = fwnode_mdiobus_verify_node(fwnode);
+	if (ret)
+		return ret;
+
+	/* Scan PHYs on MDIO bus */
+	mdio->phy_mask = 0;
+	mdio->dev.fwnode = fwnode;
+
+	/* Register the MDIO bus */
+	return mdiobus_register(mdio);
+}
+EXPORT_SYMBOL(acpi_mdiobus_register);
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 2e59a8419b17..d7bca2145d0f 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -13,6 +13,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/acpi_mdio.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/errno.h>
@@ -516,6 +517,8 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
 	 * in the bus node, and set the of_node pointer in this case.
 	 */
 	of_mdiobus_link_mdiodev(bus, &phydev->mdio);
+	/* Link the phy device with ACPI phy fwnode. */
+	acpi_mdiobus_link_mdiodev(bus, &phydev->mdio);
 
 	err = phy_device_register(phydev);
 	if (err) {
diff --git a/include/linux/acpi_mdio.h b/include/linux/acpi_mdio.h
new file mode 100644
index 000000000000..1a4a30258ebc
--- /dev/null
+++ b/include/linux/acpi_mdio.h
@@ -0,0 +1,82 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+// Copyright (c) 2018 Huaxintong Semiconductor Technology Co., Ltd.
+
+#ifndef __LINUX_ACPI_MDIO_H
+#define __LINUX_ACPI_MDIO_H
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/phy.h>
+#include <linux/property.h>
+
+#if IS_ENABLED(CONFIG_ACPI_MDIO)
+static inline int acpi_mdio_parse_addr(struct device *dev,
+				       const struct fwnode_handle *fwnode)
+{
+	u32 addr;
+
+	if (!is_acpi_data_node(fwnode))
+		return -ENODEV;
+
+	if (!fwnode_property_present(fwnode, "reg"))
+		return -ENODEV;
+
+	if (fwnode_property_read_u32(fwnode, "reg", &addr)) {
+		dev_err(dev, "Invalid PHY address\n");
+		return -ENODEV;
+	}
+
+	/* A PHY must have a reg property in the range [0-31] */
+	if (addr >= PHY_MAX_ADDR) {
+		dev_err(dev, "PHY address %i is too large\n", addr);
+		return -EINVAL;
+	}
+
+	return addr;
+}
+
+struct phy_device *acpi_phy_find_device(struct fwnode_handle *fwnode);
+struct phy_device *acpi_phy_connect(struct net_device *dev,
+				    struct fwnode_handle *fwnode,
+				    void (*hndlr)(struct net_device *),
+				    u32 flags, phy_interface_t iface);
+int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode);
+void acpi_mdiobus_link_mdiodev(struct mii_bus *bus,
+			       struct mdio_device *mdiodev);
+#else
+static inline int acpi_mdio_parse_addr(struct device *dev,
+				       const struct fwnode_handle *fwnode)
+{
+	return -EINVAL;
+}
+
+static inline struct phy_device *
+acpi_phy_find_device(struct fwnode_handle *fwnode)
+{
+	return NULL;
+}
+
+static inline struct phy_device *
+acpi_phy_connect(struct net_device *dev, struct fwnode_handle *fwnode,
+		 void (*hndlr)(struct net_device *), u32 flags,
+		 phy_interface_t iface)
+{
+	return NULL;
+}
+
+static inline int acpi_mdiobus_register(struct mii_bus *mdio,
+					struct fwnode_handle *fwnode)
+{
+	return -ENODEV;
+}
+
+static inline void
+acpi_mdiobus_link_mdiodev(struct mii_bus *bus, struct mdio_device *mdiodev) { }
+#endif
+
+static inline struct fwnode_handle *acpi_get_phy_node(struct phy_device *phydev)
+{
+	return !phydev ? NULL : phydev->mdio.dev.fwnode;
+}
+
+#endif /* __LINUX_ACPI_MDIO_H */
-- 
2.18.0

[RFC PATCH 2/3] net: qcom/emac: split phy_config to mdio bus create and get phy device

[Wang Dongsheng]

This patch separate emac_mdio_bus_create and emac_get_phydev from
emac_phy_config, and do some codes clean.

Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 96 +++++++++++--------
 1 file changed, 56 insertions(+), 40 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index 53dbf1e163a8..8289fdda4be7 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -96,15 +96,15 @@ static int emac_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val)
 	return 0;
 }
 
-/* Configure the MDIO bus and connect the external PHY */
-int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
+static int emac_mdio_bus_create(struct platform_device *pdev,
+				struct emac_adapter *adpt)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct mii_bus *mii_bus;
 	int ret;
 
 	/* Create the mii_bus object for talking to the MDIO bus */
-	adpt->mii_bus = mii_bus = devm_mdiobus_alloc(&pdev->dev);
+	mii_bus = devm_mdiobus_alloc(&pdev->dev);
 	if (!mii_bus)
 		return -ENOMEM;
 
@@ -115,50 +115,66 @@ int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
 	mii_bus->parent = &pdev->dev;
 	mii_bus->priv = adpt;
 
-	if (has_acpi_companion(&pdev->dev)) {
-		u32 phy_addr;
-
-		ret = mdiobus_register(mii_bus);
-		if (ret) {
-			dev_err(&pdev->dev, "could not register mdio bus\n");
-			return ret;
-		}
-		ret = device_property_read_u32(&pdev->dev, "phy-channel",
-					       &phy_addr);
-		if (ret)
-			/* If we can't read a valid phy address, then assume
-			 * that there is only one phy on this mdio bus.
-			 */
-			adpt->phydev = phy_find_first(mii_bus);
-		else
-			adpt->phydev = mdiobus_get_phy(mii_bus, phy_addr);
-
-		/* of_phy_find_device() claims a reference to the phydev,
-		 * so we do that here manually as well. When the driver
-		 * later unloads, it can unilaterally drop the reference
-		 * without worrying about ACPI vs DT.
-		 */
-		if (adpt->phydev)
-			get_device(&adpt->phydev->mdio.dev);
-	} else {
-		struct device_node *phy_np;
-
-		ret = of_mdiobus_register(mii_bus, np);
-		if (ret) {
-			dev_err(&pdev->dev, "could not register mdio bus\n");
-			return ret;
-		}
+	ret = of_mdiobus_register(mii_bus, has_acpi_companion(&pdev->dev) ?
+				  NULL : np);
+	if (ret)
+		dev_err(&pdev->dev, "Could not register mdio bus\n");
+
+	adpt->mii_bus = ret ? NULL : mii_bus;
+	return ret;
+}
+
+static int emac_get_phydev(struct platform_device *pdev,
+			   struct emac_adapter *adpt)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct mii_bus *bus = adpt->mii_bus;
+	struct device_node *phy_np;
+	struct phy_device *phydev;
 
+	u32 phy_addr;
+	int ret;
+
+	if (!has_acpi_companion(&pdev->dev)) {
 		phy_np = of_parse_phandle(np, "phy-handle", 0);
 		adpt->phydev = of_phy_find_device(phy_np);
 		of_node_put(phy_np);
+		return adpt->phydev ? 0 : -ENODEV;
 	}
 
-	if (!adpt->phydev) {
-		dev_err(&pdev->dev, "could not find external phy\n");
-		mdiobus_unregister(mii_bus);
+	ret = device_property_read_u32(&pdev->dev, "phy-channel",
+				       &phy_addr);
+	/* If we can't read a valid phy address, then assume
+	 * that there is only one phy on this mdio bus.
+	 */
+	phydev = ret ? phy_find_first(bus) : mdiobus_get_phy(bus, phy_addr);
+	if (!phydev)
 		return -ENODEV;
-	}
 
+	/* of_phy_find_device() claims a reference to the phydev,
+	 * so we do that here manually as well. When the driver
+	 * later unloads, it can unilaterally drop the reference
+	 * without worrying about ACPI vs DT.
+	 */
+	get_device(&phydev->mdio.dev);
+	adpt->phydev = phydev;
 	return 0;
 }
+
+/* Configure the MDIO bus and connect the external PHY */
+int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
+{
+	int ret;
+
+	ret = emac_mdio_bus_create(pdev, adpt);
+	if (ret)
+		return ret;
+
+	ret = emac_get_phydev(pdev, adpt);
+	if (ret) {
+		dev_err(&pdev->dev, "Could not find external phy\n");
+		mdiobus_unregister(adpt->mii_bus);
+	}
+
+	return ret;
+}
-- 
2.18.0

[RFC PATCH 3/3] net: qcom/emac: add phy-handle support for ACPI

[Wang Dongsheng]

Use "phy-handle" to point to an internal MDIO device port.

Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 19 ++---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 78 ++++++++++++++-----
 2 files changed, 70 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 031f6e6ee9c1..74cfe7b95bb3 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -13,6 +13,7 @@
 /* Qualcomm Technologies, Inc. EMAC Ethernet Controller MAC layer support
  */
 
+#include <linux/acpi_mdio.h>
 #include <linux/tcp.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
@@ -939,28 +940,28 @@ static void emac_adjust_link(struct net_device *netdev)
 int emac_mac_up(struct emac_adapter *adpt)
 {
 	struct net_device *netdev = adpt->netdev;
-	int ret;
+	struct phy_device *phydev = adpt->phydev;
+	struct fwnode_handle *phy_np = acpi_get_phy_node(phydev);
 
 	emac_mac_rx_tx_ring_reset_all(adpt);
 	emac_mac_config(adpt);
 	emac_mac_rx_descs_refill(adpt, &adpt->rx_q);
 
-	adpt->phydev->irq = PHY_POLL;
-	ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link,
-				 PHY_INTERFACE_MODE_SGMII);
-	if (ret) {
+	phydev->irq = PHY_POLL;
+	phydev = acpi_phy_connect(netdev, phy_np, emac_adjust_link,
+				  0, PHY_INTERFACE_MODE_SGMII);
+	if (!phydev) {
 		netdev_err(adpt->netdev, "could not connect phy\n");
-		return ret;
+		return -ENODEV;
 	}
 
-	phy_attached_print(adpt->phydev, NULL);
+	phy_attached_print(phydev, NULL);
 
 	/* enable mac irq */
 	writel((u32)~DIS_INT, adpt->base + EMAC_INT_STATUS);
 	writel(adpt->irq.mask, adpt->base + EMAC_INT_MASK);
 
-	phy_start(adpt->phydev);
-
+	phy_start(phydev);
 	napi_enable(&adpt->rx_q.napi);
 	netif_start_queue(netdev);
 
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index 8289fdda4be7..6616014292b0 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -17,6 +17,7 @@
 #include <linux/phy.h>
 #include <linux/iopoll.h>
 #include <linux/acpi.h>
+#include <linux/acpi_mdio.h>
 #include "emac.h"
 
 /* EMAC base register offsets */
@@ -96,10 +97,60 @@ static int emac_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val)
 	return 0;
 }
 
+static struct phy_device *
+emac_acpi_get_phydev_from_phy_handle(struct platform_device *pdev)
+{
+	struct fwnode_reference_args args = {0};
+	struct fwnode_handle *fw_node;
+	struct phy_device *phydev;
+	int ret;
+
+	/* Get PHY Port reference from phy-handle */
+	fw_node = acpi_fwnode_handle(ACPI_COMPANION(&pdev->dev));
+	ret = acpi_node_get_property_reference(fw_node, "phy-handle", 0,
+					       &args);
+	if (ret)
+		return ERR_PTR(-ENODEV);
+
+	if (!is_acpi_data_node(args.fwnode))
+		return ERR_PTR(-ENODEV);
+
+	phydev = acpi_phy_find_device(args.fwnode);
+	return phydev ? phydev : ERR_PTR(-ENODEV);
+}
+
+static struct phy_device *
+emac_acpi_get_phydev(struct platform_device *pdev, struct emac_adapter *adpt)
+{
+	struct phy_device *phydev = NULL;
+	int phy_addr;
+	int ret;
+
+	/* Compatible with "phy-channel" */
+	ret = device_property_read_u32(&pdev->dev, "phy-channel",
+				       &phy_addr);
+	if (!ret)
+		phydev = mdiobus_get_phy(adpt->mii_bus, phy_addr);
+	if (phydev)
+		return phydev;
+
+	/* Get PHY Port reference from phy-handle */
+	phydev = emac_acpi_get_phydev_from_phy_handle(pdev);
+	if (!IS_ERR(phydev))
+		return phydev;
+
+	/* If we can't read a valid phy address from "phy-channel"/"phy-handle",
+	 * then assume that there is only one phy on local mdio bus.
+	 */
+	phydev = phy_find_first(adpt->mii_bus);
+	return phydev ? phydev : ERR_PTR(-ENODEV);
+}
+
 static int emac_mdio_bus_create(struct platform_device *pdev,
 				struct emac_adapter *adpt)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct fwnode_handle *fwnode = pdev->dev.fwnode;
 	struct mii_bus *mii_bus;
 	int ret;
 
@@ -115,8 +166,8 @@ static int emac_mdio_bus_create(struct platform_device *pdev,
 	mii_bus->parent = &pdev->dev;
 	mii_bus->priv = adpt;
 
-	ret = of_mdiobus_register(mii_bus, has_acpi_companion(&pdev->dev) ?
-				  NULL : np);
+	ret = is_of_node(fwnode) ? of_mdiobus_register(mii_bus, np) :
+				   acpi_mdiobus_register(mii_bus, fwnode);
 	if (ret)
 		dev_err(&pdev->dev, "Could not register mdio bus\n");
 
@@ -128,13 +179,9 @@ static int emac_get_phydev(struct platform_device *pdev,
 			   struct emac_adapter *adpt)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct mii_bus *bus = adpt->mii_bus;
 	struct device_node *phy_np;
 	struct phy_device *phydev;
 
-	u32 phy_addr;
-	int ret;
-
 	if (!has_acpi_companion(&pdev->dev)) {
 		phy_np = of_parse_phandle(np, "phy-handle", 0);
 		adpt->phydev = of_phy_find_device(phy_np);
@@ -142,14 +189,9 @@ static int emac_get_phydev(struct platform_device *pdev,
 		return adpt->phydev ? 0 : -ENODEV;
 	}
 
-	ret = device_property_read_u32(&pdev->dev, "phy-channel",
-				       &phy_addr);
-	/* If we can't read a valid phy address, then assume
-	 * that there is only one phy on this mdio bus.
-	 */
-	phydev = ret ? phy_find_first(bus) : mdiobus_get_phy(bus, phy_addr);
-	if (!phydev)
-		return -ENODEV;
+	phydev = emac_acpi_get_phydev(pdev, adpt);
+	if (IS_ERR(phydev))
+		return PTR_ERR(phydev);
 
 	/* of_phy_find_device() claims a reference to the phydev,
 	 * so we do that here manually as well. When the driver
@@ -171,10 +213,10 @@ int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
 		return ret;
 
 	ret = emac_get_phydev(pdev, adpt);
-	if (ret) {
-		dev_err(&pdev->dev, "Could not find external phy\n");
-		mdiobus_unregister(adpt->mii_bus);
-	}
+	if (!ret)
+		return 0;
 
+	dev_err(&pdev->dev, "Could not find external phy\n");
+	mdiobus_unregister(adpt->mii_bus);
 	return ret;
 }
-- 
2.18.0

Re: [RFC PATCH 1/3] acpi: Add acpi mdio support code

[Rafael J. Wysocki]

On Thursday, November 8, 2018 8:22:16 AM CET Wang Dongsheng wrote:
> Add support for parsing the ACPI data node for PHY devices on an MDIO bus.
> The current implementation depend on mdio bus scan.
> With _DSD device properties we can finally do this:
> 
>     Device (MDIO) {
>         Name (_DSD, Package () {
>             ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>             Package () { Package () { "ethernet-phy@0", PHY0 }, }
>         })
>         Name (PHY0, Package() {
>             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>             Package () { Package () { "reg", 0x0 }, }
>         })
>     }
> 
>     Device (MACO) {
>         Name (_DSD, Package () {
>             ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>             Package () { Package () { "phy-handle", \_SB.MDIO, "ethernet-phy@0" }, }
>         })
>     }
> 
> Documentations:
>     The DT "phy-handle" binding that we reuse for ACPI is documented in
>     Documentation/devicetree/bindings/phy/phy-bindings.txt
> 
>     Documentation/acpi/dsd/data-node-references.txt
>     Documentation/acpi/dsd/graph.txt
> 
> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
> ---
>  drivers/acpi/Kconfig       |   6 ++
>  drivers/acpi/Makefile      |   1 +
>  drivers/acpi/acpi_mdio.c   | 167 +++++++++++++++++++++++++++++++++++++
>  drivers/net/phy/mdio_bus.c |   3 +
>  include/linux/acpi_mdio.h  |  82 ++++++++++++++++++
>  5 files changed, 259 insertions(+)
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 9705fc986da9..0fefa3410ce9 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -252,6 +252,12 @@ config ACPI_PROCESSOR_IDLE
>  config ACPI_MCFG
>  	bool
>  
> +config ACPI_MDIO
> +	def_tristate PHYLIB
> +	depends on PHYLIB
> +	help
> +	  ACPI MDIO bus (Ethernet PHY) accessors
> +
>  config ACPI_CPPC_LIB
>  	bool
>  	depends on ACPI_PROCESSOR
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 6d59aa109a91..ec7461a064fc 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -41,6 +41,7 @@ acpi-y				+= ec.o
>  acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
>  acpi-y				+= pci_root.o pci_link.o pci_irq.o
>  obj-$(CONFIG_ACPI_MCFG)		+= pci_mcfg.o
> +acpi-$(CONFIG_ACPI_MDIO)	+= acpi_mdio.o
>  acpi-y				+= acpi_lpss.o acpi_apd.o
>  acpi-y				+= acpi_platform.o
>  acpi-y				+= acpi_pnp.o
> diff --git a/drivers/acpi/acpi_mdio.c b/drivers/acpi/acpi_mdio.c
> new file mode 100644
> index 000000000000..293bf9a63197
> --- /dev/null
> +++ b/drivers/acpi/acpi_mdio.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Lots of code in this file is copy from drivers/of/of_mdio.c

Would it be possible to re-factor that code and share it instead?

Thanks,
Rafael

Re: [RFC PATCH 1/3] acpi: Add acpi mdio support code

[Wang, Dongsheng]

On 2018/11/8 15:44, Rafael J. Wysocki wrote:
> On Thursday, November 8, 2018 8:22:16 AM CET Wang Dongsheng wrote:
>> Add support for parsing the ACPI data node for PHY devices on an MDIO bus.
>> The current implementation depend on mdio bus scan.
>> With _DSD device properties we can finally do this:
>>
>>     Device (MDIO) {
>>         Name (_DSD, Package () {
>>             ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>>             Package () { Package () { "ethernet-phy@0", PHY0 }, }
>>         })
>>         Name (PHY0, Package() {
>>             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>             Package () { Package () { "reg", 0x0 }, }
>>         })
>>     }
>>
>>     Device (MACO) {
>>         Name (_DSD, Package () {
>>             ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>             Package () { Package () { "phy-handle", \_SB.MDIO, "ethernet-phy@0" }, }
>>         })
>>     }
>>
>> Documentations:
>>     The DT "phy-handle" binding that we reuse for ACPI is documented in
>>     Documentation/devicetree/bindings/phy/phy-bindings.txt
>>
>>     Documentation/acpi/dsd/data-node-references.txt
>>     Documentation/acpi/dsd/graph.txt
>>
>> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
>> ---
>>  drivers/acpi/Kconfig       |   6 ++
>>  drivers/acpi/Makefile      |   1 +
>>  drivers/acpi/acpi_mdio.c   | 167 +++++++++++++++++++++++++++++++++++++
>>  drivers/net/phy/mdio_bus.c |   3 +
>>  include/linux/acpi_mdio.h  |  82 ++++++++++++++++++
>>  5 files changed, 259 insertions(+)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 9705fc986da9..0fefa3410ce9 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -252,6 +252,12 @@ config ACPI_PROCESSOR_IDLE
>>  config ACPI_MCFG
>>  	bool
>>  
>> +config ACPI_MDIO
>> +	def_tristate PHYLIB
>> +	depends on PHYLIB
>> +	help
>> +	  ACPI MDIO bus (Ethernet PHY) accessors
>> +
>>  config ACPI_CPPC_LIB
>>  	bool
>>  	depends on ACPI_PROCESSOR
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index 6d59aa109a91..ec7461a064fc 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -41,6 +41,7 @@ acpi-y				+= ec.o
>>  acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
>>  acpi-y				+= pci_root.o pci_link.o pci_irq.o
>>  obj-$(CONFIG_ACPI_MCFG)		+= pci_mcfg.o
>> +acpi-$(CONFIG_ACPI_MDIO)	+= acpi_mdio.o
>>  acpi-y				+= acpi_lpss.o acpi_apd.o
>>  acpi-y				+= acpi_platform.o
>>  acpi-y				+= acpi_pnp.o
>> diff --git a/drivers/acpi/acpi_mdio.c b/drivers/acpi/acpi_mdio.c
>> new file mode 100644
>> index 000000000000..293bf9a63197
>> --- /dev/null
>> +++ b/drivers/acpi/acpi_mdio.c
>> @@ -0,0 +1,167 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +// Lots of code in this file is copy from drivers/of/of_mdio.c
> Would it be possible to re-factor that code and share it instead?

I thought about it, we can actually do it with fwnode.

But I don't have a lot of concentrate to do this. I'm going to focus on
a few other things...:(

Maybe in the second half of 2019 I can re-factor them if there is no one
re-factor them.


Cheers,

Dongsheng


> Thanks,
> Rafael
>
>

Re: [RFC PATCH 1/3] acpi: Add acpi mdio support code

[Rafael J. Wysocki]

On Thursday, November 8, 2018 8:55:47 AM CET Wang, Dongsheng wrote:
> On 2018/11/8 15:44, Rafael J. Wysocki wrote:
> > On Thursday, November 8, 2018 8:22:16 AM CET Wang Dongsheng wrote:
> >> Add support for parsing the ACPI data node for PHY devices on an MDIO bus.
> >> The current implementation depend on mdio bus scan.
> >> With _DSD device properties we can finally do this:
> >>
> >>     Device (MDIO) {
> >>         Name (_DSD, Package () {
> >>             ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> >>             Package () { Package () { "ethernet-phy@0", PHY0 }, }
> >>         })
> >>         Name (PHY0, Package() {
> >>             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >>             Package () { Package () { "reg", 0x0 }, }
> >>         })
> >>     }
> >>
> >>     Device (MACO) {
> >>         Name (_DSD, Package () {
> >>             ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >>             Package () { Package () { "phy-handle", \_SB.MDIO, "ethernet-phy@0" }, }
> >>         })
> >>     }
> >>
> >> Documentations:
> >>     The DT "phy-handle" binding that we reuse for ACPI is documented in
> >>     Documentation/devicetree/bindings/phy/phy-bindings.txt
> >>
> >>     Documentation/acpi/dsd/data-node-references.txt
> >>     Documentation/acpi/dsd/graph.txt
> >>
> >> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
> >> ---
> >>  drivers/acpi/Kconfig       |   6 ++
> >>  drivers/acpi/Makefile      |   1 +
> >>  drivers/acpi/acpi_mdio.c   | 167 +++++++++++++++++++++++++++++++++++++
> >>  drivers/net/phy/mdio_bus.c |   3 +
> >>  include/linux/acpi_mdio.h  |  82 ++++++++++++++++++
> >>  5 files changed, 259 insertions(+)
> >>
> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >> index 9705fc986da9..0fefa3410ce9 100644
> >> --- a/drivers/acpi/Kconfig
> >> +++ b/drivers/acpi/Kconfig
> >> @@ -252,6 +252,12 @@ config ACPI_PROCESSOR_IDLE
> >>  config ACPI_MCFG
> >>  	bool
> >>  
> >> +config ACPI_MDIO
> >> +	def_tristate PHYLIB
> >> +	depends on PHYLIB
> >> +	help
> >> +	  ACPI MDIO bus (Ethernet PHY) accessors
> >> +
> >>  config ACPI_CPPC_LIB
> >>  	bool
> >>  	depends on ACPI_PROCESSOR
> >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> >> index 6d59aa109a91..ec7461a064fc 100644
> >> --- a/drivers/acpi/Makefile
> >> +++ b/drivers/acpi/Makefile
> >> @@ -41,6 +41,7 @@ acpi-y				+= ec.o
> >>  acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
> >>  acpi-y				+= pci_root.o pci_link.o pci_irq.o
> >>  obj-$(CONFIG_ACPI_MCFG)		+= pci_mcfg.o
> >> +acpi-$(CONFIG_ACPI_MDIO)	+= acpi_mdio.o
> >>  acpi-y				+= acpi_lpss.o acpi_apd.o
> >>  acpi-y				+= acpi_platform.o
> >>  acpi-y				+= acpi_pnp.o
> >> diff --git a/drivers/acpi/acpi_mdio.c b/drivers/acpi/acpi_mdio.c
> >> new file mode 100644
> >> index 000000000000..293bf9a63197
> >> --- /dev/null
> >> +++ b/drivers/acpi/acpi_mdio.c
> >> @@ -0,0 +1,167 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +// Lots of code in this file is copy from drivers/of/of_mdio.c
> > Would it be possible to re-factor that code and share it instead?
> 
> I thought about it, we can actually do it with fwnode.
> 
> But I don't have a lot of concentrate to do this. I'm going to focus on
> a few other things...:(
> 
> Maybe in the second half of 2019 I can re-factor them if there is no one
> re-factor them.

Well, I'd rather avoid code duplication upfront if possible.

Thanks,
Rafael

Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code

[Andrew Lunn]

On Thu, Nov 08, 2018 at 03:21:29PM +0800, Wang Dongsheng wrote:
> Originally I just push "phy-handle" support for ACPI on the QCOM QDF2400
> platform. After some discussion and following Andrew's advice, I send
> out with a generic version of ACPI.
> 
> Current there is no clear documentation about MDIO/PHY for ACPI, so when
> I reading some documents about ACPI [1], I think we just need to reuse the
> DT binding in the ACPI.[2]. However, this series of patches are not
> fully compatible with all contents specified in DT binding.
> 
> The most important thing about this iseries is link the phy device and
> fwnode of acpi. Besides, we need to carry out bus scan at the mdio
> register. Therefore, I am not compatible with more DT binding properties
> in this series of patches. More support will be in the follow-up patches
> support, or some people do the support.
> 
> Example:
> Based on ACPI doc:
>     Documentation/acpi/dsd/data-node-references.txt
>     Documentation/acpi/dsd/graph.txt
> With _DSD device properties we can finally do this:
>     Device (MDIO) {
>         Name (_DSD, Package () {
>             ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>             Package () { Package () { "ethernet-phy@0", PHY0 }, }
>         })
>         Name (PHY0, Package() {
>             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>             Package () { Package () { "reg", 0x0 }, }
>         })

I don't know much about ACPI. I do know DT. MDIO busses can have
multiple PHYs on them. Is the following valid to list two PHYs?

     Device (MDIO) {
         Name (_DSD, Package () {
             ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
             Package () { Package () { "ethernet-phy@0", PHY0 }, }
         })
         Name (PHY0, Package() {
             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
             Package () { Package () { "reg", 0x0 }, }
         })
         Name (_DSD, Package () {
             ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
             Package () { Package () { "ethernet-phy@10", PHY1 }, }
         })
         Name (PHY1, Package() {
             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
             Package () { Package () { "reg", 0x10 }, }
         })
     }


An MDIO bus can also have more than PHYs on them. There can be
Ethernet switches. Broadcom also have some with generic PHY devices on
them, and other odd things. That means whatever is on an MDIO bus is a
device in the Linux device model. How does that work? Do we need some
form Device (PHY) {}?

     Device (MDIO) {
         Device (PHY) {
             Name (_DSD, Package () {
                 ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
                 Package () { Package () { "ethernet-phy@0", PHY0 }, }
             })
             Name (PHY0, Package() {
                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                 Package () { Package () { "reg", 0x0 }, }
             })
         }
         Device (PHY) {
             Name (_DSD, Package () {
                 ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
                 Package () { Package () { "ethernet-phy@10", PHY1 }, }
             })
             Name (PHY1, Package() {
                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                 Package () { Package () { "reg", 0x10 }, }
             })
         Device (SWITCH) {
             Name (_DSD, Package () {
                 ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
                 Package () { Package () { "switch@11", SWITCH0 }, }
             })
             Name (SWITCH0, Package() {
                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                 Package () { Package () { "reg", 0x11 }, }
             })
         }
      }

I'm just trying to ensure whatever is defined is flexible enough that
we really can later support everything which DT does. We have PHYs on
MDIO busses, inside switches, which are on MDIO busses, which are
inside Ethernet interfaces, etc.

An MDIO bus is very similar to an i2c bus. How is that described in
ACPI? Anything we can learn from that?

      Thanks
         Andrew

Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code

[Timur Tabi]

On 11/8/18 5:23 PM, Andrew Lunn wrote:
> I don't know much about ACPI. I do know DT. MDIO busses can have
> multiple PHYs on them. Is the following valid to list two PHYs?
> 
>       Device (MDIO) {
>           Name (_DSD, Package () {
>               ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>               Package () { Package () { "ethernet-phy@0", PHY0 }, }
>           })
>           Name (PHY0, Package() {
>               ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>               Package () { Package () { "reg", 0x0 }, }
>           })
>           Name (_DSD, Package () {
>               ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>               Package () { Package () { "ethernet-phy@10", PHY1 }, }
>           })
>           Name (PHY1, Package() {
>               ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>               Package () { Package () { "reg", 0x10 }, }
>           })
>       }

You can't have the same DSD twice.  It would need to look like this:

          Name (PHY1, Package() {
              ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
              Package () { Package () { "reg", 0, 0x10 }, }
          })

Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code

[Wang, Dongsheng]

Hi Andrew,

On 2018/11/9 7:23, Andrew Lunn wrote:
> On Thu, Nov 08, 2018 at 03:21:29PM +0800, Wang Dongsheng wrote:
>> Originally I just push "phy-handle" support for ACPI on the QCOM QDF2400
>> platform. After some discussion and following Andrew's advice, I send
>> out with a generic version of ACPI.
>>
>> Current there is no clear documentation about MDIO/PHY for ACPI, so when
>> I reading some documents about ACPI [1], I think we just need to reuse the
>> DT binding in the ACPI.[2]. However, this series of patches are not
>> fully compatible with all contents specified in DT binding.
>>
>> The most important thing about this iseries is link the phy device and
>> fwnode of acpi. Besides, we need to carry out bus scan at the mdio
>> register. Therefore, I am not compatible with more DT binding properties
>> in this series of patches. More support will be in the follow-up patches
>> support, or some people do the support.
>>
>> Example:
>> Based on ACPI doc:
>>     Documentation/acpi/dsd/data-node-references.txt
>>     Documentation/acpi/dsd/graph.txt
>> With _DSD device properties we can finally do this:
>>     Device (MDIO) {
>>         Name (_DSD, Package () {
>>             ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>>             Package () { Package () { "ethernet-phy@0", PHY0 }, }
>>         })
>>         Name (PHY0, Package() {
>>             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>             Package () { Package () { "reg", 0x0 }, }
>>         })
> I don't know much about ACPI. I do know DT. MDIO busses can have
> multiple PHYs on them. Is the following valid to list two PHYs?
>
>      Device (MDIO) {
>          Name (_DSD, Package () {
>              ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>              Package () { Package () { "ethernet-phy@0", PHY0 }, }
>          })
>          Name (PHY0, Package() {
>              ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>              Package () { Package () { "reg", 0x0 }, }
>          })
>          Name (_DSD, Package () {
>              ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>              Package () { Package () { "ethernet-phy@10", PHY1 }, }
>          })
>          Name (PHY1, Package() {
>              ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>              Package () { Package () { "reg", 0x10 }, }
>          })
>      }

Multiple PHYs example:


    Device (MDIO)
    {
        Name (_DSD, Package () {
        ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () {

                Package () { "ethernet-phy@0", PHY0 },

                Package () { "ethernet-phy@1", PHY1 },
                ...

                ...

            }
        })
       
        Name (PHY0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "reg", 0x0 },
            }
        })


        Name (PHY1, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "reg", 0x1 },
            }
        })
   }

    Device (MAC0)

    {

        // _DSD: Device-Specific Data
        Name (_DSD, Package (0x02) {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device
Properties for _DSD */,
            Package () {
                Package () { "phy-handle", Package () { \_SB.MDIO,
"ethernet-phy@0" } },
                ...
                ...
            }
        })

    }


    Device (MAC1)

    {

        // _DSD: Device-Specific Data
        Name (_DSD, Package (0x02) {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device
Properties for _DSD */,
            Package () {
                Package () { "phy-handle", Package () { \_SB.MDIO,
"ethernet-phy@1" } },
                ...
                ...
            }
        })

    }

>
> An MDIO bus can also have more than PHYs on them. There can be
> Ethernet switches. Broadcom also have some with generic PHY devices on
> them, and other odd things. That means whatever is on an MDIO bus is a
> device in the Linux device model. How does that work? Do we need some
> form Device (PHY) {}?

ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b") describes a ACPI data node.

The data node can contain property or pointer.


Let's look at the table I'm using:

Device (MAC1)
{
    // _DSD: Device-Specific Data
    Name (_DSD, Package (0x02) {
        ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device
Properties for _DSD */,
        Package (0x07) {
            Package () { "phy-handle", Package () { \_SB.MAC1.MDIO,
"ethernet-phy@1" } },
            Package () { "dev-refs", \_SB.MAC0 },
            Package () { "refs0-dev", Package () { \_SB.MAC1.MDIO,
"refs@0" } },
            Package () { "refs1-dev", Package () { \_SB.MAC1.MDIO,
"refs@1" } },
            ...
            ...
        }
    })

    Device (MDIO)
    {
        Name (_DSD, Package () {
        ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () {
                Package () { "ethernet-phy@1", PHY1 },
                Package () { "refs@0",  REF0},
                Package () { "refs@1",  REF1},
            }
        })
        
        //Contain a property
        Name (PHY1, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "reg", 0x1 },
            }
        })
        
        //Point to a device
        Name (REF0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "child-refs-dev", \_SB.MAC0 },
            }
        })
        
        //Contain a property and a pointer that point to a device
        Name (REF1, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "property", 0x5 },
                Package () { "child-refs-dev", \_SB.MAC0 },
            }
        })
    }
}


>      Device (MDIO) {
>          Device (PHY) {
>              Name (_DSD, Package () {
>                  ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>                  Package () { Package () { "ethernet-phy@0", PHY0 }, }
>              })
>              Name (PHY0, Package() {
>                  ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                  Package () { Package () { "reg", 0x0 }, }
>              })
>          }
>          Device (PHY) {
>              Name (_DSD, Package () {
>                  ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>                  Package () { Package () { "ethernet-phy@10", PHY1 }, }
>              })
>              Name (PHY1, Package() {
>                  ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                  Package () { Package () { "reg", 0x10 }, }
>              })
>          Device (SWITCH) {
>              Name (_DSD, Package () {
>                  ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>                  Package () { Package () { "switch@11", SWITCH0 }, }
>              })
>              Name (SWITCH0, Package() {
>                  ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                  Package () { Package () { "reg", 0x11 }, }
>              })
>          }

    Device (MDIO)
    {
        Name (_DSD, Package () {
        ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () {
                Package () { "ethernet-phy@1", PHY1 },
                Package () { "switch@0",  SW00 },

                Package () { "switch@1",  SW01 },
            }
        })
        
        //Contain a property
        Name (PHY1, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "reg", 0x1 },
            }
        })
 
        Name (SW00, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "reg", 0x0 },
            }
        })

        Name (SW01, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "property", 0x5 },
            }
        })
    }


>       }
>
> I'm just trying to ensure whatever is defined is flexible enough that
> we really can later support everything which DT does. We have PHYs on
> MDIO busses, inside switches, which are on MDIO busses, which are
> inside Ethernet interfaces, etc.

I think it can be satisfied. See the table I'm using above.


> An MDIO bus is very similar to an i2c bus. How is that described in
> ACPI? Anything we can learn from that?

About the data node, I just read the Kernel Documentation/acpi/dsd/

And others learn from ACPI spec.


Cheers,

Dongsheng

Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code

[Wang, Dongsheng]

On 2018/11/9 7:23, Andrew Lunn wrote:
> I'm just trying to ensure whatever is defined is flexible enough that
> we really can later support everything which DT does. We have PHYs on
> MDIO busses, inside switches, which are on MDIO busses, which are
> inside Ethernet interfaces, etc.
>
> An MDIO bus is very similar to an i2c bus. How is that described in
> ACPI? Anything we can learn from that?

All the documents I've seen are from UEFII/ACPICA and I have not seen any
independent description of MDIO in the ACPI documents. Maybe I missed a
document, too.

I think not all devices need to be described in ACPI. The ACPI defines what
should be included in a device, as well as some special definitions. Our
MDIO
doesn't need a special definition, because it's enough to use a generic ACPI
description. I think that's why MDIO doesn't have a separate description.

Cheers,
Dongsheng

Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code

[Andrew Lunn]

> > I'm just trying to ensure whatever is defined is flexible enough that
> > we really can later support everything which DT does. We have PHYs on
> > MDIO busses, inside switches, which are on MDIO busses, which are
> > inside Ethernet interfaces, etc.
> 
> I think it can be satisfied. See the table I'm using above.

Hi Dongsheng


Since i don't know anything better, i think i have to trust you have
this correct.

It would be good to document this, so that the next person who needs
to add ACPI support for a PHY has some documentation to look at.
Could you add something to Documentation/acpi/dsd?

      Andrew

Re: [RFC PATCH 1/3] acpi: Add acpi mdio support code

[Andrew Lunn]

On Thu, Nov 08, 2018 at 03:22:16PM +0800, Wang Dongsheng wrote:
> Add support for parsing the ACPI data node for PHY devices on an MDIO bus.
> The current implementation depend on mdio bus scan.
> With _DSD device properties we can finally do this:
> 
>     Device (MDIO) {
>         Name (_DSD, Package () {
>             ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>             Package () { Package () { "ethernet-phy@0", PHY0 }, }
>         })
>         Name (PHY0, Package() {
>             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>             Package () { Package () { "reg", 0x0 }, }
>         })
>     }
> 
>     Device (MACO) {
>         Name (_DSD, Package () {
>             ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>             Package () { Package () { "phy-handle", \_SB.MDIO, "ethernet-phy@0" }, }
>         })
>     }
> 
> Documentations:
>     The DT "phy-handle" binding that we reuse for ACPI is documented in
>     Documentation/devicetree/bindings/phy/phy-bindings.txt
> 
>     Documentation/acpi/dsd/data-node-references.txt
>     Documentation/acpi/dsd/graph.txt
> 
> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
> ---
>  drivers/acpi/Kconfig       |   6 ++
>  drivers/acpi/Makefile      |   1 +
>  drivers/acpi/acpi_mdio.c   | 167 +++++++++++++++++++++++++++++++++++++
>  drivers/net/phy/mdio_bus.c |   3 +
>  include/linux/acpi_mdio.h  |  82 ++++++++++++++++++
>  5 files changed, 259 insertions(+)
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 9705fc986da9..0fefa3410ce9 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -252,6 +252,12 @@ config ACPI_PROCESSOR_IDLE
>  config ACPI_MCFG
>  	bool
>  
> +config ACPI_MDIO
> +	def_tristate PHYLIB
> +	depends on PHYLIB
> +	help
> +	  ACPI MDIO bus (Ethernet PHY) accessors
> +
>  config ACPI_CPPC_LIB
>  	bool
>  	depends on ACPI_PROCESSOR
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 6d59aa109a91..ec7461a064fc 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -41,6 +41,7 @@ acpi-y				+= ec.o
>  acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
>  acpi-y				+= pci_root.o pci_link.o pci_irq.o
>  obj-$(CONFIG_ACPI_MCFG)		+= pci_mcfg.o
> +acpi-$(CONFIG_ACPI_MDIO)	+= acpi_mdio.o
>  acpi-y				+= acpi_lpss.o acpi_apd.o
>  acpi-y				+= acpi_platform.o
>  acpi-y				+= acpi_pnp.o
> diff --git a/drivers/acpi/acpi_mdio.c b/drivers/acpi/acpi_mdio.c
> new file mode 100644
> index 000000000000..293bf9a63197
> --- /dev/null

> +++ b/drivers/acpi/acpi_mdio.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Lots of code in this file is copy from drivers/of/of_mdio.c
> +// Copyright (c) 2018 Huaxintong Semiconductor Technology Co., Ltd.

I agree with Rafael here. We should try to refactor and combine this
code. And where possible, we should try to add a uniform fwnode_ API
which underneath either uses OF or ACPI, depending on the type of the
node. We already have fwnode_get_mac_address(), fwmode_irq_get(),
fwnode_get_phy_mode(), so where possible, PHYs should be no different.

      Andrew

Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code

[Wang, Dongsheng]

Hello Rafael,

On 2018/11/13 1:19, Andrew Lunn wrote:
>>> I'm just trying to ensure whatever is defined is flexible enough that
>>> we really can later support everything which DT does. We have PHYs on
>>> MDIO busses, inside switches, which are on MDIO busses, which are
>>> inside Ethernet interfaces, etc.
>> I think it can be satisfied. See the table I'm using above.
> Hi Dongsheng
>
>
> Since i don't know anything better, i think i have to trust you have
> this correct.
>
> It would be good to document this, so that the next person who needs
> to add ACPI support for a PHY has some documentation to look at.
> Could you add something to Documentation/acpi/dsd?

How about Andrew suggestion? I agree with Andrew.

Or We need to add the documentation to Documentation/acpi?
I see that both GPIO and I2C are down here.


Cheers,
Dongsheng