[net-next PATCH v2 00/14] ACPI support for dpaa2 driver

[net-next PATCH v2 00/14] ACPI support for dpaa2 driver

[Calvin Johnson]

This patch set provides ACPI support to DPAA2 network drivers.

It also introduces new fwnode based APIs to support phylink and phy
layers
    Following functions are defined:
      phylink_fwnode_phy_connect()
      fwnode_mdiobus_register_phy()
      fwnode_mdiobus_register()
      fwnode_get_phy_id()
      fwnode_phy_find_device()
      device_phy_find_device()
      fwnode_get_phy_node()
      fwnode_mdio_find_device()
      fwnode_get_id()

    First one helps in connecting phy to phylink instance.
    Next three helps in getting phy_id and registering phy to mdiobus
    Next two help in finding a phy on a mdiobus.
    Next one helps in getting phy_node from a fwnode.
    Last one is used to get fwnode ID.

    Corresponding OF functions are refactored.
    END


Changes in v2:
- Updated with more description in document
- use reverse christmas tree ordering for local variables
- Refactor OF functions to use fwnode functions

Calvin Johnson (14):
  Documentation: ACPI: DSD: Document MDIO PHY
  net: phy: Introduce phy related fwnode functions
  of: mdio: Refactor of_phy_find_device()
  net: phy: Introduce fwnode_get_phy_id()
  of: mdio: Refactor of_get_phy_id()
  net: mdiobus: Introduce fwnode_mdiobus_register_phy()
  of: mdio: Refactor of_mdiobus_register_phy()
  net: mdiobus: Introduce fwnode_mdiobus_register()
  net/fsl: Use fwnode_mdiobus_register()
  device property: Introduce fwnode_get_id()
  phylink: introduce phylink_fwnode_phy_connect()
  net: phylink: Refactor phylink_of_phy_connect()
  net: phy: Introduce fwnode_mdio_find_device()
  net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

 Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++
 drivers/base/property.c                       |  26 ++++
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  |  86 +++++++-----
 drivers/net/ethernet/freescale/xgmac_mdio.c   |  14 +-
 drivers/net/mdio/of_mdio.c                    |  79 +----------
 drivers/net/phy/mdio_bus.c                    | 116 ++++++++++++++++
 drivers/net/phy/phy_device.c                  | 108 +++++++++++++++
 drivers/net/phy/phylink.c                     |  49 ++++---
 include/linux/mdio.h                          |   2 +
 include/linux/of_mdio.h                       |   6 +-
 include/linux/phy.h                           |  32 +++++
 include/linux/phylink.h                       |   3 +
 include/linux/property.h                      |   1 +
 13 files changed, 519 insertions(+), 132 deletions(-)
 create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst

-- 
2.17.1

[net-next PATCH v2 01/14] Documentation: ACPI: DSD: Document MDIO PHY

[Calvin Johnson]

Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
provide them to be connected to MAC.

Describe properties "phy-handle" and "phy-mode".

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2:
- Updated with more description in document

 Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++
 1 file changed, 129 insertions(+)
 create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst

diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
new file mode 100644
index 000000000000..a2e4fdcdbf53
--- /dev/null
+++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
@@ -0,0 +1,129 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=========================
+MDIO bus and PHYs in ACPI
+=========================
+
+The PHYs on an MDIO bus [1] are probed and registered using
+fwnode_mdiobus_register_phy().
+Later, for connecting these PHYs to MAC, the PHYs registered on the
+mdiobus have to be referenced.
+
+UUID given below should be used as mentioned in the "Device Properties
+UUID For _DSD" [2] document.
+   - UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301
+
+This document introduces two _DSD properties that are to be used
+for PHYs on the MDIO bus.[3]
+
+phy-handle
+----------
+For each MAC node, a device property "phy-handle" is used to reference
+the PHY that is registered on an MDIO bus. This is mandatory for
+network interfaces that have PHYs connected to MAC via MDIO bus.
+
+During the MDIO bus driver initialization, PHYs on this bus are probed
+using the _ADR object as shown below and are registered on the mdio bus.
+
+::
+      Scope(\_SB.MDI0)
+      {
+        Device(PHY1) {
+          Name (_ADR, 0x1)
+        } // end of PHY1
+
+        Device(PHY2) {
+          Name (_ADR, 0x2)
+        } // end of PHY2
+      }
+
+Later, during the MAC driver initialization, the registered PHY devices
+have to be retrieved from the mdio bus. For this, MAC driver needs
+reference to the previously registered PHYs which are provided
+using reference to the device as {\_SB.MDI0.PHY1}.
+
+phy-mode
+--------
+The "phy-mode" _DSD property is used to describe the connection to
+the PHY. The valid values for "phy-mode" are defined in [4].
+
+
+An ASL example of this is shown below.
+
+DSDT entry for MDIO node
+------------------------
+The MDIO bus has an SoC component(mdio controller) and a platform
+component(PHYs on the mdiobus).
+
+a) Silicon Component
+This node describes the MDIO controller,MDI0
+--------------------------------------------
+::
+	Scope(_SB)
+	{
+	  Device(MDI0) {
+	    Name(_HID, "NXP0006")
+	    Name(_CCA, 1)
+	    Name(_UID, 0)
+	    Name(_CRS, ResourceTemplate() {
+	      Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)
+	      Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
+	       {
+		 MDI0_IT
+	       }
+	    }) // end of _CRS for MDI0
+	  } // end of MDI0
+	}
+
+b) Platform Component
+This node defines the PHYs that are connected to the MDIO bus, MDI0
+-------------------------------------------------------------------
+::
+	Scope(\_SB.MDI0)
+	{
+	  Device(PHY1) {
+	    Name (_ADR, 0x1)
+	  } // end of PHY1
+
+	  Device(PHY2) {
+	    Name (_ADR, 0x2)
+	  } // end of PHY2
+	}
+
+
+Below are the MAC nodes where PHY nodes are referenced.
+phy-mode and phy-handle are used as explained earlier.
+------------------------------------------------------
+::
+	Scope(\_SB.MCE0.PR17)
+	{
+	  Name (_DSD, Package () {
+	     ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		 Package () {
+		     Package (2) {"phy-mode", "rgmii-id"},
+		     Package (2) {"phy-handle", \_SB.MDI0.PHY1}
+	      }
+	   })
+	}
+
+	Scope(\_SB.MCE0.PR18)
+	{
+	  Name (_DSD, Package () {
+	    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		Package () {
+		    Package (2) {"phy-mode", "rgmii-id"},
+		    Package (2) {"phy-handle", \_SB.MDI0.PHY2}}
+	    }
+	  })
+	}
+
+References
+==========
+
+[1] Documentation/networking/phy.rst
+
+[2] https://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
+
+[3] Documentation/firmware-guide/acpi/DSD-properties-rules.rst
+
+[4] Documentation/devicetree/bindings/net/ethernet-controller.yaml
-- 
2.17.1

[net-next PATCH v2 02/14] net: phy: Introduce phy related fwnode functions

[Calvin Johnson]

Define fwnode_phy_find_device() to iterate an mdiobus and find the
phy device of the provided phy fwnode. Additionally define
device_phy_find_device() to find phy device of provided device.

Define fwnode_get_phy_node() to get phy_node using named reference.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2:
- use reverse christmas tree ordering for local variables

 drivers/net/phy/phy_device.c | 64 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          | 20 +++++++++++
 2 files changed, 84 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 80c2e646c093..c153273606c1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -9,6 +9,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/acpi.h>
 #include <linux/bitmap.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
@@ -2829,6 +2830,69 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
 	return phydrv->config_intr && phydrv->handle_interrupt;
 }
 
+/**
+ * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
+ * phy_fwnode.
+ * @phy_fwnode: Pointer to the phy's fwnode.
+ *
+ * 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 *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+	struct mdio_device *mdiodev;
+	struct device *d;
+
+	if (!phy_fwnode)
+		return NULL;
+
+	d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode);
+	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(fwnode_phy_find_device);
+
+/**
+ * device_phy_find_device - For the given device, get the phy_device
+ * @dev: Pointer to the given device
+ *
+ * Refer return conditions of fwnode_phy_find_device().
+ */
+struct phy_device *device_phy_find_device(struct device *dev)
+{
+	return fwnode_phy_find_device(dev_fwnode(dev));
+}
+EXPORT_SYMBOL_GPL(device_phy_find_device);
+
+/**
+ * fwnode_get_phy_node - Get the phy_node using the named reference.
+ * @fwnode: Pointer to fwnode from which phy_node has to be obtained.
+ *
+ * Refer return conditions of fwnode_find_reference().
+ * For ACPI, only "phy-handle" is supported. DT supports all the three
+ * named references to the phy node.
+ */
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *phy_node;
+
+	/* Only phy-handle is used for ACPI */
+	phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
+	if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
+		return phy_node;
+	phy_node = fwnode_find_reference(fwnode, "phy", 0);
+	if (IS_ERR(phy_node))
+		phy_node = fwnode_find_reference(fwnode, "phy-device", 0);
+	return phy_node;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
+
 /**
  * phy_probe - probe and init a PHY device
  * @dev: device to probe and init
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 381a95732b6a..7790a9a56d0f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1341,10 +1341,30 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 				     bool is_c45,
 				     struct phy_c45_device_ids *c45_ids);
 #if IS_ENABLED(CONFIG_PHYLIB)
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
+struct phy_device *device_phy_find_device(struct device *dev);
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode);
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
 int phy_device_register(struct phy_device *phy);
 void phy_device_free(struct phy_device *phydev);
 #else
+static inline
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+	return NULL;
+}
+
+static inline struct phy_device *device_phy_find_device(struct device *dev)
+{
+	return NULL;
+}
+
+static inline
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
+{
+	return NULL;
+}
+
 static inline
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 {
-- 
2.17.1

Re: [net-next PATCH v2 02/14] net: phy: Introduce phy related fwnode functions

[Andy Shevchenko]

On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Define fwnode_phy_find_device() to iterate an mdiobus and find the
> phy device of the provided phy fwnode. Additionally define
> device_phy_find_device() to find phy device of provided device.
>
> Define fwnode_get_phy_node() to get phy_node using named reference.

...

> +#include <linux/acpi.h>

Not sure we need this. See below.

...

> +/**
> + * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
> + * phy_fwnode.

Can we keep a summary on one line?

> + * @phy_fwnode: Pointer to the phy's fwnode.
> + *
> + * 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 *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> +{
> +       struct mdio_device *mdiodev;
> +       struct device *d;

> +       if (!phy_fwnode)
> +               return NULL;

Why is this needed?
Perhaps a comment to the function description explains a case when
@phy_fwnode == NULL.

> +       d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode);
> +       if (d) {
> +               mdiodev = to_mdio_device(d);
> +               if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
> +                       return to_phy_device(d);
> +               put_device(d);
> +       }
> +
> +       return NULL;
> +}

...

> + * For ACPI, only "phy-handle" is supported. DT supports all the three
> + * named references to the phy node.

...

> +       /* Only phy-handle is used for ACPI */
> +       phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
> +       if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
> +               return phy_node;

So, what is the problem with going through the rest on ACPI?
Usually we describe the restrictions in the documentation.

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v2 02/14] net: phy: Introduce phy related fwnode functions

[Calvin Johnson]

On Tue, Dec 15, 2020 at 07:23:26PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Define fwnode_phy_find_device() to iterate an mdiobus and find the
> > phy device of the provided phy fwnode. Additionally define
> > device_phy_find_device() to find phy device of provided device.
> >
> > Define fwnode_get_phy_node() to get phy_node using named reference.
> 
> ...
> 
> > +#include <linux/acpi.h>
> 
> Not sure we need this. See below.

This is required to use is_acpi_node().
> 
> ...
> 
> > +/**
> > + * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
> > + * phy_fwnode.
> 
> Can we keep a summary on one line?

Ok
> 
> > + * @phy_fwnode: Pointer to the phy's fwnode.
> > + *
> > + * 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 *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> > +{
> > +       struct mdio_device *mdiodev;
> > +       struct device *d;
> 
> > +       if (!phy_fwnode)
> > +               return NULL;
> 
> Why is this needed?
> Perhaps a comment to the function description explains a case when
> @phy_fwnode == NULL.

I think this function should be modified to follow of_phy_find_device() which
has NULL check. I'll add fwnode_mdio_find_device() also.
Here is a case where of_phy_find_device() is called without checking phy_np.
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qualcomm/emac/emac-phy.c#L145
> 
> > +       d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode);
> > +       if (d) {
> > +               mdiodev = to_mdio_device(d);
> > +               if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
> > +                       return to_phy_device(d);
> > +               put_device(d);
> > +       }
> > +
> > +       return NULL;
> > +}
> 
> ...
> 
> > + * For ACPI, only "phy-handle" is supported. DT supports all the three
> > + * named references to the phy node.
> 
> ...
> 
> > +       /* Only phy-handle is used for ACPI */
> > +       phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
> > +       if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
> > +               return phy_node;
> 
> So, what is the problem with going through the rest on ACPI?
> Usually we describe the restrictions in the documentation.

Others are legacy DT properties which are not intended to be supported
in ACPI. I can add this info in the document.

Thanks for the review, Andy!

Regards
Calvin

[net-next PATCH v2 03/14] of: mdio: Refactor of_phy_find_device()

[Calvin Johnson]

Refactor of_phy_find_device() to use fwnode_phy_find_device().

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2: None

 drivers/net/mdio/of_mdio.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 4daf94bb56a5..fde76bee78b3 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -369,18 +369,7 @@ EXPORT_SYMBOL(of_mdio_find_device);
  */
 struct phy_device *of_phy_find_device(struct device_node *phy_np)
 {
-	struct mdio_device *mdiodev;
-
-	mdiodev = of_mdio_find_device(phy_np);
-	if (!mdiodev)
-		return NULL;
-
-	if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
-		return to_phy_device(&mdiodev->dev);
-
-	put_device(&mdiodev->dev);
-
-	return NULL;
+	return fwnode_phy_find_device(of_fwnode_handle(phy_np));
 }
 EXPORT_SYMBOL(of_phy_find_device);
 
-- 
2.17.1

[net-next PATCH v2 04/14] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

Extract phy_id from compatible string. This will be used by
fwnode_mdiobus_register_phy() to create phy device using the
phy_id.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2: None

 drivers/net/phy/phy_device.c | 21 +++++++++++++++++++++
 include/linux/phy.h          |  5 +++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c153273606c1..6fad89c02c5a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -846,6 +846,27 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
 	return 0;
 }
 
+/* Extract the phy ID from the compatible string of the form
+ * ethernet-phy-idAAAA.BBBB.
+ */
+int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+	unsigned int upper, lower;
+	const char *cp;
+	int ret;
+
+	ret = fwnode_property_read_string(fwnode, "compatible", &cp);
+	if (ret)
+		return ret;
+
+	if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
+		*phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
+		return 0;
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL(fwnode_get_phy_id);
+
 /**
  * get_phy_device - reads the specified PHY device and returns its @phy_device
  *		    struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7790a9a56d0f..10a66b65a008 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1341,6 +1341,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 				     bool is_c45,
 				     struct phy_c45_device_ids *c45_ids);
 #if IS_ENABLED(CONFIG_PHYLIB)
+int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id);
 struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
 struct phy_device *device_phy_find_device(struct device *dev);
 struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode);
@@ -1348,6 +1349,10 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
 int phy_device_register(struct phy_device *phy);
 void phy_device_free(struct phy_device *phydev);
 #else
+static inline int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+	return 0;
+}
 static inline
 struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
 {
-- 
2.17.1

Re: [net-next PATCH v2 04/14] net: phy: Introduce fwnode_get_phy_id()

[Andy Shevchenko]

On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Extract phy_id from compatible string. This will be used by
> fwnode_mdiobus_register_phy() to create phy device using the
> phy_id.

...

> +       if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
> +               *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
> +               return 0;
> +       }
> +       return -EINVAL;

Perhaps traditional pattern, i.e.
       if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) != 2)
               return -EINVAL;

       *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
       return 0;

And perhaps GENMASK() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v2 04/14] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

On Tue, Dec 15, 2020 at 07:28:10PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Extract phy_id from compatible string. This will be used by
> > fwnode_mdiobus_register_phy() to create phy device using the
> > phy_id.
> 
> ...
> 
> > +       if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
> > +               *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
> > +               return 0;
> > +       }
> > +       return -EINVAL;
> 
> Perhaps traditional pattern, i.e.
>        if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) != 2)
>                return -EINVAL;
> 
>        *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
>        return 0;
> 
> And perhaps GENMASK() ?

Sure. Will rewrite accordingly.

Thanks
Calvin

Re: [net-next PATCH v2 04/14] net: phy: Introduce fwnode_get_phy_id()

[Andy Shevchenko]

On Thu, Dec 17, 2020 at 10:28 AM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
> On Tue, Dec 15, 2020 at 07:28:10PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> > <calvin.johnson@oss.nxp.com> wrote:

...

> > > +       if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {

> >        *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);

> > And perhaps GENMASK() ?
>
> Sure. Will rewrite accordingly.

Reading this again I'm now not sure these masks are needed at all.

-- 
With Best Regards,
Andy Shevchenko

[net-next PATCH v2 05/14] of: mdio: Refactor of_get_phy_id()

[Calvin Johnson]

With the introduction of fwnode_get_phy_id(), refactor of_get_phy_id()
to use fwnode equivalent.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2: None

 drivers/net/mdio/of_mdio.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index fde76bee78b3..31e6435dcc9f 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -29,17 +29,7 @@ MODULE_LICENSE("GPL");
  * ethernet-phy-idAAAA.BBBB */
 static int of_get_phy_id(struct device_node *device, u32 *phy_id)
 {
-	struct property *prop;
-	const char *cp;
-	unsigned int upper, lower;
-
-	of_property_for_each_string(device, "compatible", prop, cp) {
-		if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
-			*phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
-			return 0;
-		}
-	}
-	return -EINVAL;
+	return fwnode_get_phy_id(of_fwnode_handle(device), phy_id);
 }
 
 static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
-- 
2.17.1

[net-next PATCH v2 06/14] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

[Calvin Johnson]

Introduce fwnode_mdiobus_register_phy() to register PHYs on the
mdiobus. From the compatible string, identify whether the PHY is
c45 and based on this create a PHY device instance which is
registered on the mdiobus.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2: None

 drivers/net/phy/mdio_bus.c | 66 ++++++++++++++++++++++++++++++++++++++
 include/linux/mdio.h       |  2 ++
 2 files changed, 68 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 2b42e46066b4..3361a1a86e97 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -106,6 +106,72 @@ int mdiobus_unregister_device(struct mdio_device *mdiodev)
 }
 EXPORT_SYMBOL(mdiobus_unregister_device);
 
+int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+				struct fwnode_handle *child, u32 addr)
+{
+	struct mii_timestamper *mii_ts;
+	struct phy_device *phy;
+	const char *cp;
+	bool is_c45;
+	u32 phy_id;
+	int rc;
+
+	if (is_of_node(child)) {
+		mii_ts = of_find_mii_timestamper(to_of_node(child));
+		if (IS_ERR(mii_ts))
+			return PTR_ERR(mii_ts);
+	}
+
+	rc = fwnode_property_read_string(child, "compatible", &cp);
+	is_c45 = !(rc || strcmp(cp, "ethernet-phy-ieee802.3-c45"));
+
+	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
+		phy = get_phy_device(bus, addr, is_c45);
+	else
+		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+	if (IS_ERR(phy)) {
+		if (mii_ts && is_of_node(child))
+			unregister_mii_timestamper(mii_ts);
+		return PTR_ERR(phy);
+	}
+
+	if (is_acpi_node(child)) {
+		phy->irq = bus->irq[addr];
+
+		/* Associate the fwnode with the device structure so it
+		 * can be looked up later.
+		 */
+		phy->mdio.dev.fwnode = child;
+
+		/* All data is now stored in the phy struct, so register it */
+		rc = phy_device_register(phy);
+		if (rc) {
+			phy_device_free(phy);
+			fwnode_handle_put(phy->mdio.dev.fwnode);
+			return rc;
+		}
+
+		dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
+	} else if (is_of_node(child)) {
+		rc = of_mdiobus_phy_device_register(bus, phy, to_of_node(child), addr);
+		if (rc) {
+			if (mii_ts)
+				unregister_mii_timestamper(mii_ts);
+			phy_device_free(phy);
+			return rc;
+		}
+
+		/* phy->mii_ts may already be defined by the PHY driver. A
+		 * mii_timestamper probed via the device tree will still have
+		 * precedence.
+		 */
+		if (mii_ts)
+			phy->mii_ts = mii_ts;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(fwnode_mdiobus_register_phy);
+
 struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr)
 {
 	struct mdio_device *mdiodev = bus->mdio_map[addr];
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index dbd69b3d170b..f138ec333725 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -368,6 +368,8 @@ int mdiobus_register_device(struct mdio_device *mdiodev);
 int mdiobus_unregister_device(struct mdio_device *mdiodev);
 bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);
 struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr);
+int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+				      struct fwnode_handle *child, u32 addr);
 
 /**
  * mdio_module_driver() - Helper macro for registering mdio drivers
-- 
2.17.1

Re: [net-next PATCH v2 06/14] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

[Andy Shevchenko]

On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Introduce fwnode_mdiobus_register_phy() to register PHYs on the
> mdiobus. From the compatible string, identify whether the PHY is
> c45 and based on this create a PHY device instance which is
> registered on the mdiobus.

...

> +int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> +                               struct fwnode_handle *child, u32 addr)
> +{
> +       struct mii_timestamper *mii_ts;
> +       struct phy_device *phy;
> +       const char *cp;
> +       bool is_c45;
> +       u32 phy_id;
> +       int rc;

> +       if (is_of_node(child)) {
> +               mii_ts = of_find_mii_timestamper(to_of_node(child));
> +               if (IS_ERR(mii_ts))
> +                       return PTR_ERR(mii_ts);
> +       }

Perhaps

               mii_ts = of_find_mii_timestamper(to_of_node(child));

> +
> +       rc = fwnode_property_read_string(child, "compatible", &cp);
> +       is_c45 = !(rc || strcmp(cp, "ethernet-phy-ieee802.3-c45"));
> +
> +       if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> +               phy = get_phy_device(bus, addr, is_c45);
> +       else
> +               phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> +       if (IS_ERR(phy)) {

> +               if (mii_ts && is_of_node(child))
> +                       unregister_mii_timestamper(mii_ts);

if (!IS_ERR_OR_NULL(mii_ts))
 ...

However it points to the question why unregister() doesn't handle the
above cases.
I would expect unconditional call to unregister() here.

> +               return PTR_ERR(phy);
> +       }
> +
> +       if (is_acpi_node(child)) {
> +               phy->irq = bus->irq[addr];
> +
> +               /* Associate the fwnode with the device structure so it
> +                * can be looked up later.
> +                */
> +               phy->mdio.dev.fwnode = child;
> +
> +               /* All data is now stored in the phy struct, so register it */
> +               rc = phy_device_register(phy);
> +               if (rc) {
> +                       phy_device_free(phy);
> +                       fwnode_handle_put(phy->mdio.dev.fwnode);
> +                       return rc;
> +               }
> +
> +               dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
> +       } else if (is_of_node(child)) {
> +               rc = of_mdiobus_phy_device_register(bus, phy, to_of_node(child), addr);
> +               if (rc) {

> +                       if (mii_ts)
> +                               unregister_mii_timestamper(mii_ts);

Ditto.

> +                       phy_device_free(phy);
> +                       return rc;
> +               }
> +
> +               /* phy->mii_ts may already be defined by the PHY driver. A
> +                * mii_timestamper probed via the device tree will still have
> +                * precedence.
> +                */

> +               if (mii_ts)
> +                       phy->mii_ts = mii_ts;

How is that defined? Do you need to do something with an old pointer?

> +       }
> +       return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v2 06/14] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

[Calvin Johnson]

On Tue, Dec 15, 2020 at 07:33:40PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Introduce fwnode_mdiobus_register_phy() to register PHYs on the
> > mdiobus. From the compatible string, identify whether the PHY is
> > c45 and based on this create a PHY device instance which is
> > registered on the mdiobus.
> 
> ...
> 
> > +int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> > +                               struct fwnode_handle *child, u32 addr)
> > +{
> > +       struct mii_timestamper *mii_ts;
> > +       struct phy_device *phy;
> > +       const char *cp;
> > +       bool is_c45;
> > +       u32 phy_id;
> > +       int rc;
> 
> > +       if (is_of_node(child)) {
> > +               mii_ts = of_find_mii_timestamper(to_of_node(child));
> > +               if (IS_ERR(mii_ts))
> > +                       return PTR_ERR(mii_ts);
> > +       }
> 
> Perhaps
> 
>                mii_ts = of_find_mii_timestamper(to_of_node(child));
> 
> > +
> > +       rc = fwnode_property_read_string(child, "compatible", &cp);
> > +       is_c45 = !(rc || strcmp(cp, "ethernet-phy-ieee802.3-c45"));
> > +
> > +       if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> > +               phy = get_phy_device(bus, addr, is_c45);
> > +       else
> > +               phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> > +       if (IS_ERR(phy)) {
> 
> > +               if (mii_ts && is_of_node(child))
> > +                       unregister_mii_timestamper(mii_ts);
> 
> if (!IS_ERR_OR_NULL(mii_ts))
>  ...
> 
> However it points to the question why unregister() doesn't handle the
> above cases.
> I would expect unconditional call to unregister() here.

This is following the logic defined in of_mdiobus_register_phy().
https://elixir.bootlin.com/linux/latest/source/drivers/net/mdio/of_mdio.c#L107

	mii_ts = of_find_mii_timestamper(child);
	if (IS_ERR(mii_ts))
		return PTR_ERR(mii_ts);

I think the logic above is correct because this function returns only if
of_find_mii_timestamper() returns error. On NULL return, it proceeds further.

> 
> > +               return PTR_ERR(phy);
> > +       }
> > +
> > +       if (is_acpi_node(child)) {
> > +               phy->irq = bus->irq[addr];
> > +
> > +               /* Associate the fwnode with the device structure so it
> > +                * can be looked up later.
> > +                */
> > +               phy->mdio.dev.fwnode = child;
> > +
> > +               /* All data is now stored in the phy struct, so register it */
> > +               rc = phy_device_register(phy);
> > +               if (rc) {
> > +                       phy_device_free(phy);
> > +                       fwnode_handle_put(phy->mdio.dev.fwnode);
> > +                       return rc;
> > +               }
> > +
> > +               dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
> > +       } else if (is_of_node(child)) {
> > +               rc = of_mdiobus_phy_device_register(bus, phy, to_of_node(child), addr);
> > +               if (rc) {
> 
> > +                       if (mii_ts)
> > +                               unregister_mii_timestamper(mii_ts);
> 
> Ditto.
> 
> > +                       phy_device_free(phy);
> > +                       return rc;
> > +               }
> > +
> > +               /* phy->mii_ts may already be defined by the PHY driver. A
> > +                * mii_timestamper probed via the device tree will still have
> > +                * precedence.
> > +                */
> 
> > +               if (mii_ts)
> > +                       phy->mii_ts = mii_ts;
> 
> How is that defined? Do you need to do something with an old pointer?

As the comment says, I think PHY drivers which got invoked before calling
of_mdiobus_register_phy() may have defined phy->mii_ts.

> 
> > +       }
> > +       return 0;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [net-next PATCH v2 06/14] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

[Andy Shevchenko]

On Fri, Dec 18, 2020 at 7:34 AM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> On Tue, Dec 15, 2020 at 07:33:40PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> > <calvin.johnson@oss.nxp.com> wrote:

...

> > > +               /* phy->mii_ts may already be defined by the PHY driver. A
> > > +                * mii_timestamper probed via the device tree will still have
> > > +                * precedence.
> > > +                */
> >
> > > +               if (mii_ts)
> > > +                       phy->mii_ts = mii_ts;
> >
> > How is that defined? Do you need to do something with an old pointer?
>
> As the comment says, I think PHY drivers which got invoked before calling
> of_mdiobus_register_phy() may have defined phy->mii_ts.

What I meant here is that the old pointer might need some care (freeing?).

> > > +       }

-- 
With Best Regards,
Andy Shevchenko

[net-next PATCH v2 07/14] of: mdio: Refactor of_mdiobus_register_phy()

[Calvin Johnson]

Refactor of_mdiobus_register_phy() to use fwnode_mdiobus_register_phy().

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2: None

 drivers/net/mdio/of_mdio.c | 43 +++-----------------------------------
 include/linux/of_mdio.h    |  6 +++++-
 2 files changed, 8 insertions(+), 41 deletions(-)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 31e6435dcc9f..fa914d285271 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -32,7 +32,7 @@ static int of_get_phy_id(struct device_node *device, u32 *phy_id)
 	return fwnode_get_phy_id(of_fwnode_handle(device), phy_id);
 }
 
-static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
+struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
 {
 	struct of_phandle_args arg;
 	int err;
@@ -49,6 +49,7 @@ static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
 
 	return register_mii_timestamper(arg.np, arg.args[0]);
 }
+EXPORT_SYMBOL(of_find_mii_timestamper);
 
 int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy,
 			      struct device_node *child, u32 addr)
@@ -97,45 +98,7 @@ EXPORT_SYMBOL(of_mdiobus_phy_device_register);
 static int of_mdiobus_register_phy(struct mii_bus *mdio,
 				    struct device_node *child, u32 addr)
 {
-	struct mii_timestamper *mii_ts;
-	struct phy_device *phy;
-	bool is_c45;
-	int rc;
-	u32 phy_id;
-
-	mii_ts = of_find_mii_timestamper(child);
-	if (IS_ERR(mii_ts))
-		return PTR_ERR(mii_ts);
-
-	is_c45 = of_device_is_compatible(child,
-					 "ethernet-phy-ieee802.3-c45");
-
-	if (!is_c45 && !of_get_phy_id(child, &phy_id))
-		phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
-	else
-		phy = get_phy_device(mdio, addr, is_c45);
-	if (IS_ERR(phy)) {
-		if (mii_ts)
-			unregister_mii_timestamper(mii_ts);
-		return PTR_ERR(phy);
-	}
-
-	rc = of_mdiobus_phy_device_register(mdio, phy, child, addr);
-	if (rc) {
-		if (mii_ts)
-			unregister_mii_timestamper(mii_ts);
-		phy_device_free(phy);
-		return rc;
-	}
-
-	/* phy->mii_ts may already be defined by the PHY driver. A
-	 * mii_timestamper probed via the device tree will still have
-	 * precedence.
-	 */
-	if (mii_ts)
-		phy->mii_ts = mii_ts;
-
-	return 0;
+	return fwnode_mdiobus_register_phy(mdio, of_fwnode_handle(child), addr);
 }
 
 static int of_mdiobus_register_device(struct mii_bus *mdio,
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index cfe8c607a628..3b66016f18aa 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -34,6 +34,7 @@ struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np);
 int of_phy_register_fixed_link(struct device_node *np);
 void of_phy_deregister_fixed_link(struct device_node *np);
 bool of_phy_is_fixed_link(struct device_node *np);
+struct mii_timestamper *of_find_mii_timestamper(struct device_node *np);
 int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy,
 				   struct device_node *child, u32 addr);
 
@@ -128,7 +129,10 @@ static inline bool of_phy_is_fixed_link(struct device_node *np)
 {
 	return false;
 }
-
+static inline struct mii_timestamper *of_find_mii_timestamper(struct device_node *np)
+{
+	return NULL;
+}
 static inline int of_mdiobus_phy_device_register(struct mii_bus *mdio,
 					    struct phy_device *phy,
 					    struct device_node *child, u32 addr)
-- 
2.17.1

[net-next PATCH v2 08/14] net: mdiobus: Introduce fwnode_mdiobus_register()

[Calvin Johnson]

Introduce fwnode_mdiobus_register() to register PHYs on the  mdiobus.
If the fwnode is DT node, then call of_mdiobus_register().
If it is an ACPI node, then:
	- disable auto probing of mdiobus
	- register mdiobus
	- save fwnode to mdio structure
	- loop over child nodes & register a phy_device for each PHY

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2: None

 drivers/net/phy/mdio_bus.c | 50 ++++++++++++++++++++++++++++++++++++++
 include/linux/phy.h        |  1 +
 2 files changed, 51 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 3361a1a86e97..e7ad34908936 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -8,6 +8,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/acpi.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/errno.h>
@@ -567,6 +568,55 @@ static int mdiobus_create_device(struct mii_bus *bus,
 	return ret;
 }
 
+/**
+ * fwnode_mdiobus_register - Register mii_bus and create PHYs from fwnode
+ * @mdio: pointer to mii_bus structure
+ * @fwnode: pointer to fwnode of MDIO bus.
+ *
+ * This function registers the mii_bus structure and registers a phy_device
+ * for each child node of @fwnode.
+ */
+int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *child;
+	unsigned long long addr;
+	acpi_status status;
+	int ret;
+
+	if (is_of_node(fwnode)) {
+		return of_mdiobus_register(mdio, to_of_node(fwnode));
+	} else if (is_acpi_node(fwnode)) {
+		/* Mask out all PHYs from auto probing. */
+		mdio->phy_mask = ~0;
+		ret = mdiobus_register(mdio);
+		if (ret)
+			return ret;
+
+		mdio->dev.fwnode = fwnode;
+	/* Loop over the child nodes and register a phy_device for each PHY */
+		fwnode_for_each_child_node(fwnode, child) {
+			status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child),
+						       "_ADR", NULL, &addr);
+			if (ACPI_FAILURE(status)) {
+				pr_debug("_ADR returned %d\n", status);
+				continue;
+			}
+
+			if (addr < 0 || addr >= PHY_MAX_ADDR)
+				continue;
+
+			ret = fwnode_mdiobus_register_phy(mdio, child, addr);
+			if (ret == -ENODEV)
+				dev_err(&mdio->dev,
+					"MDIO device at address %lld is missing.\n",
+					addr);
+		}
+		return 0;
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL(fwnode_mdiobus_register);
+
 /**
  * __mdiobus_register - bring up all the PHYs on a given bus and attach them to bus
  * @bus: target mii_bus
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 10a66b65a008..67ea4ca6f76f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -383,6 +383,7 @@ static inline struct mii_bus *mdiobus_alloc(void)
 	return mdiobus_alloc_size(0);
 }
 
+int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode);
 int __mdiobus_register(struct mii_bus *bus, struct module *owner);
 int __devm_mdiobus_register(struct device *dev, struct mii_bus *bus,
 			    struct module *owner);
-- 
2.17.1

Re: [net-next PATCH v2 08/14] net: mdiobus: Introduce fwnode_mdiobus_register()

[Andy Shevchenko]

On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Introduce fwnode_mdiobus_register() to register PHYs on the  mdiobus.
> If the fwnode is DT node, then call of_mdiobus_register().
> If it is an ACPI node, then:
>         - disable auto probing of mdiobus
>         - register mdiobus
>         - save fwnode to mdio structure
>         - loop over child nodes & register a phy_device for each PHY

...

> +/**
> + * fwnode_mdiobus_register - Register mii_bus and create PHYs from fwnode
> + * @mdio: pointer to mii_bus structure
> + * @fwnode: pointer to fwnode of MDIO bus.
> + *
> + * This function registers the mii_bus structure and registers a phy_device
> + * for each child node of @fwnode.
> + */
> +int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
> +{
> +       struct fwnode_handle *child;
> +       unsigned long long addr;
> +       acpi_status status;
> +       int ret;
> +
> +       if (is_of_node(fwnode)) {
> +               return of_mdiobus_register(mdio, to_of_node(fwnode));
> +       } else if (is_acpi_node(fwnode)) {

I would rather see this as simple as

     if (is_of_node(fwnode))
               return of_mdiobus_register(mdio, to_of_node(fwnode));
     if (is_acpi_node(fwnode))
               return acpi_mdiobus_register(mdio, fwnode);

where the latter one is defined somewhere in drivers/acpi/.

> +               /* Mask out all PHYs from auto probing. */
> +               mdio->phy_mask = ~0;
> +               ret = mdiobus_register(mdio);
> +               if (ret)
> +                       return ret;
> +
> +               mdio->dev.fwnode = fwnode;
> +       /* Loop over the child nodes and register a phy_device for each PHY */
> +               fwnode_for_each_child_node(fwnode, child) {

> +                       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child),
> +                                                      "_ADR", NULL, &addr);
> +                       if (ACPI_FAILURE(status)) {

Isn't it fwnode_get_id() now?

> +                               pr_debug("_ADR returned %d\n", status);
> +                               continue;
> +                       }

> +                       if (addr < 0 || addr >= PHY_MAX_ADDR)
> +                               continue;

addr can't be less than 0.

> +                       ret = fwnode_mdiobus_register_phy(mdio, child, addr);
> +                       if (ret == -ENODEV)
> +                               dev_err(&mdio->dev,
> +                                       "MDIO device at address %lld is missing.\n",
> +                                       addr);
> +               }
> +               return 0;
> +       }
> +       return -EINVAL;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v2 08/14] net: mdiobus: Introduce fwnode_mdiobus_register()

[Calvin Johnson]

On Tue, Dec 15, 2020 at 07:53:26PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Introduce fwnode_mdiobus_register() to register PHYs on the  mdiobus.
> > If the fwnode is DT node, then call of_mdiobus_register().
> > If it is an ACPI node, then:
> >         - disable auto probing of mdiobus
> >         - register mdiobus
> >         - save fwnode to mdio structure
> >         - loop over child nodes & register a phy_device for each PHY
> 
> ...
> 
> > +/**
> > + * fwnode_mdiobus_register - Register mii_bus and create PHYs from fwnode
> > + * @mdio: pointer to mii_bus structure
> > + * @fwnode: pointer to fwnode of MDIO bus.
> > + *
> > + * This function registers the mii_bus structure and registers a phy_device
> > + * for each child node of @fwnode.
> > + */
> > +int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
> > +{
> > +       struct fwnode_handle *child;
> > +       unsigned long long addr;
> > +       acpi_status status;
> > +       int ret;
> > +
> > +       if (is_of_node(fwnode)) {
> > +               return of_mdiobus_register(mdio, to_of_node(fwnode));
> > +       } else if (is_acpi_node(fwnode)) {
> 
> I would rather see this as simple as
> 
>      if (is_of_node(fwnode))
>                return of_mdiobus_register(mdio, to_of_node(fwnode));
>      if (is_acpi_node(fwnode))
>                return acpi_mdiobus_register(mdio, fwnode);
> 
> where the latter one is defined somewhere in drivers/acpi/.
Makes sense. I'll do it. But I think it will be better to place
acpi_mdiobus_register() here itself in the network subsystem, maybe
/drivers/net/mdio/acpi_mdio.c.
> 
> > +               /* Mask out all PHYs from auto probing. */
> > +               mdio->phy_mask = ~0;
> > +               ret = mdiobus_register(mdio);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               mdio->dev.fwnode = fwnode;
> > +       /* Loop over the child nodes and register a phy_device for each PHY */
> > +               fwnode_for_each_child_node(fwnode, child) {
> 
> > +                       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child),
> > +                                                      "_ADR", NULL, &addr);
> > +                       if (ACPI_FAILURE(status)) {
> 
> Isn't it fwnode_get_id() now?
Yes. Will change it.
> 
> > +                               pr_debug("_ADR returned %d\n", status);
> > +                               continue;
> > +                       }
> 
> > +                       if (addr < 0 || addr >= PHY_MAX_ADDR)
> > +                               continue;
> 
> addr can't be less than 0.
Yes. will update in v3.
> 
> > +                       ret = fwnode_mdiobus_register_phy(mdio, child, addr);
> > +                       if (ret == -ENODEV)
> > +                               dev_err(&mdio->dev,
> > +                                       "MDIO device at address %lld is missing.\n",
> > +                                       addr);
> > +               }
> > +               return 0;
> > +       }
> > +       return -EINVAL;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [net-next PATCH v2 08/14] net: mdiobus: Introduce fwnode_mdiobus_register()

[Andy Shevchenko]

On Fri, Dec 18, 2020 at 7:40 AM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
> On Tue, Dec 15, 2020 at 07:53:26PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> > <calvin.johnson@oss.nxp.com> wrote:

...

> > I would rather see this as simple as
> >
> >      if (is_of_node(fwnode))
> >                return of_mdiobus_register(mdio, to_of_node(fwnode));
> >      if (is_acpi_node(fwnode))
> >                return acpi_mdiobus_register(mdio, fwnode);
> >
> > where the latter one is defined somewhere in drivers/acpi/.
> Makes sense. I'll do it. But I think it will be better to place
> acpi_mdiobus_register() here itself in the network subsystem, maybe
> /drivers/net/mdio/acpi_mdio.c.

Even better, thanks!

-- 
With Best Regards,
Andy Shevchenko

[net-next PATCH v2 09/14] net/fsl: Use fwnode_mdiobus_register()

[Calvin Johnson]

fwnode_mdiobus_register() internally takes care of both DT
and ACPI cases to register mdiobus. Replace existing
of_mdiobus_register() with fwnode_mdiobus_register().

Note: For both ACPI and DT cases, endianness of MDIO controller
need to be specified using "little-endian" property.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2: None

 drivers/net/ethernet/freescale/xgmac_mdio.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index bfa2826c5545..ae2593abac96 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -2,6 +2,7 @@
  * QorIQ 10G MDIO Controller
  *
  * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2020 NXP
  *
  * Authors: Andy Fleming <afleming@freescale.com>
  *          Timur Tabi <timur@freescale.com>
@@ -11,6 +12,7 @@
  * kind, whether express or implied.
  */
 
+#include <linux/acpi.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
@@ -243,10 +245,9 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
 
 static int xgmac_mdio_probe(struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
-	struct mii_bus *bus;
-	struct resource *res;
 	struct mdio_fsl_priv *priv;
+	struct resource *res;
+	struct mii_bus *bus;
 	int ret;
 
 	/* In DPAA-1, MDIO is one of the many FMan sub-devices. The FMan
@@ -279,13 +280,14 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 		goto err_ioremap;
 	}
 
+	/* For both ACPI and DT cases, endianness of MDIO controller
+	 *  need to be specified using "little-endian" property.
+	 */
 	priv->is_little_endian = device_property_read_bool(&pdev->dev,
 							   "little-endian");
-
 	priv->has_a011043 = device_property_read_bool(&pdev->dev,
 						      "fsl,erratum-a011043");
-
-	ret = of_mdiobus_register(bus, np);
+	ret = fwnode_mdiobus_register(bus, pdev->dev.fwnode);
 	if (ret) {
 		dev_err(&pdev->dev, "cannot register MDIO bus\n");
 		goto err_registration;
-- 
2.17.1

Re: [net-next PATCH v2 09/14] net/fsl: Use fwnode_mdiobus_register()

[Andy Shevchenko]

On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> fwnode_mdiobus_register() internally takes care of both DT
> and ACPI cases to register mdiobus. Replace existing
> of_mdiobus_register() with fwnode_mdiobus_register().
>
> Note: For both ACPI and DT cases, endianness of MDIO controller
> need to be specified using "little-endian" property.

...

> @@ -2,6 +2,7 @@
>   * QorIQ 10G MDIO Controller
>   *
>   * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2020 NXP
>   *
>   * Authors: Andy Fleming <afleming@freescale.com>
>   *          Timur Tabi <timur@freescale.com>
> @@ -11,6 +12,7 @@

I guess this...

>         priv->is_little_endian = device_property_read_bool(&pdev->dev,
>                                                            "little-endian");
> -
>         priv->has_a011043 = device_property_read_bool(&pdev->dev,
>                                                       "fsl,erratum-a011043");

...this...

> -

...and this changes can go to a separate patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v2 09/14] net/fsl: Use fwnode_mdiobus_register()

[Calvin Johnson]

On Tue, Dec 15, 2020 at 07:55:01PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > fwnode_mdiobus_register() internally takes care of both DT
> > and ACPI cases to register mdiobus. Replace existing
> > of_mdiobus_register() with fwnode_mdiobus_register().
> >
> > Note: For both ACPI and DT cases, endianness of MDIO controller
> > need to be specified using "little-endian" property.
> 
> ...
> 
> > @@ -2,6 +2,7 @@
> >   * QorIQ 10G MDIO Controller
> >   *
> >   * Copyright 2012 Freescale Semiconductor, Inc.
> > + * Copyright 2020 NXP
> >   *
> >   * Authors: Andy Fleming <afleming@freescale.com>
> >   *          Timur Tabi <timur@freescale.com>
> > @@ -11,6 +12,7 @@
> 
> I guess this...
> 
> >         priv->is_little_endian = device_property_read_bool(&pdev->dev,
> >                                                            "little-endian");
> > -
> >         priv->has_a011043 = device_property_read_bool(&pdev->dev,
> >                                                       "fsl,erratum-a011043");
> 
> ...this...
> 
> > -
> 
> ...and this changes can go to a separate patch.

I think I'll remove the unnecessary cleanup.

Regards
Calvin

[net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()

[Calvin Johnson]

Using fwnode_get_id(), get the reg property value for DT node
and get the _ADR object value for ACPI node.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2: None

 drivers/base/property.c  | 26 ++++++++++++++++++++++++++
 include/linux/property.h |  1 +
 2 files changed, 27 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 4c43d30145c6..1c50e17ae879 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -580,6 +580,32 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
 	return fwnode_call_ptr_op(fwnode, get_name_prefix);
 }
 
+/**
+ * fwnode_get_id - Get the id of a fwnode.
+ * @fwnode: firmware node
+ * @id: id of the fwnode
+ *
+ * Returns 0 on success or a negative errno.
+ */
+int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
+{
+	unsigned long long adr;
+	acpi_status status;
+
+	if (is_of_node(fwnode)) {
+		return of_property_read_u32(to_of_node(fwnode), "reg", id);
+	} else if (is_acpi_node(fwnode)) {
+		status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
+					       METHOD_NAME__ADR, NULL, &adr);
+		if (ACPI_FAILURE(status))
+			return -ENODATA;
+		*id = (u32)adr;
+		return 0;
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_id);
+
 /**
  * fwnode_get_parent - Return parent firwmare node
  * @fwnode: Firmware whose parent is retrieved
diff --git a/include/linux/property.h b/include/linux/property.h
index 2d4542629d80..92d405cf2b07 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -82,6 +82,7 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
 
 const char *fwnode_get_name(const struct fwnode_handle *fwnode);
 const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
+int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id);
 struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_next_parent(
 	struct fwnode_handle *fwnode);
-- 
2.17.1

Re: [net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()

[Laurent Pinchart]

Hi Calvin,

Thank you for the patch.

On Tue, Dec 15, 2020 at 10:13:11PM +0530, Calvin Johnson wrote:
> Using fwnode_get_id(), get the reg property value for DT node
> and get the _ADR object value for ACPI node.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
> 
> Changes in v2: None
> 
>  drivers/base/property.c  | 26 ++++++++++++++++++++++++++
>  include/linux/property.h |  1 +
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 4c43d30145c6..1c50e17ae879 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -580,6 +580,32 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
>  	return fwnode_call_ptr_op(fwnode, get_name_prefix);
>  }
>  
> +/**
> + * fwnode_get_id - Get the id of a fwnode.
> + * @fwnode: firmware node
> + * @id: id of the fwnode
> + *

Is the concept of fwnode ID documented clearly somewhere ? I think this
function should otherwise have more documentation, at least to explain
what the ID is.

> + * Returns 0 on success or a negative errno.
> + */
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> +{
> +	unsigned long long adr;
> +	acpi_status status;
> +
> +	if (is_of_node(fwnode)) {
> +		return of_property_read_u32(to_of_node(fwnode), "reg", id);
> +	} else if (is_acpi_node(fwnode)) {
> +		status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> +					       METHOD_NAME__ADR, NULL, &adr);
> +		if (ACPI_FAILURE(status))
> +			return -ENODATA;

Would it make sense to standardize error codes ? of_property_read_u32()
can return -EINVAL, -ENODATA or -EOVERFLOW. I don't think the caller of
this function would be very interested to tell those three cases apart.
Maybe we should return -EINVAL in all error cases ? Or maybe different
error codes to mean "the backend doesn't support the concept of IDs",
and "the device doesn't have an ID" ?

> +		*id = (u32)adr;
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_get_id);
> +
>  /**
>   * fwnode_get_parent - Return parent firwmare node
>   * @fwnode: Firmware whose parent is retrieved
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 2d4542629d80..92d405cf2b07 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -82,6 +82,7 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
>  
>  const char *fwnode_get_name(const struct fwnode_handle *fwnode);
>  const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id);
>  struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
>  struct fwnode_handle *fwnode_get_next_parent(
>  	struct fwnode_handle *fwnode);

-- 
Regards,

Laurent Pinchart

Re: [net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()

[Andy Shevchenko]

On Tue, Dec 15, 2020 at 7:00 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tue, Dec 15, 2020 at 10:13:11PM +0530, Calvin Johnson wrote:
> > Using fwnode_get_id(), get the reg property value for DT node
> > and get the _ADR object value for ACPI node.

...

> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
>
> Is the concept of fwnode ID documented clearly somewhere ? I think this
> function should otherwise have more documentation, at least to explain
> what the ID is.

I'm afraid that OF has no clear concept of this either. It's usually
used as a unique ID for the children of some device (like MFD) and can
represent a lot of things. But I agree it should be documented.

Rob, any ideas about this?

> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > +     unsigned long long adr;
> > +     acpi_status status;
> > +
> > +     if (is_of_node(fwnode)) {
> > +             return of_property_read_u32(to_of_node(fwnode), "reg", id);
> > +     } else if (is_acpi_node(fwnode)) {
> > +             status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > +                                            METHOD_NAME__ADR, NULL, &adr);
> > +             if (ACPI_FAILURE(status))
> > +                     return -ENODATA;
>
> Would it make sense to standardize error codes ? of_property_read_u32()
> can return -EINVAL, -ENODATA or -EOVERFLOW. I don't think the caller of
> this function would be very interested to tell those three cases apart.
> Maybe we should return -EINVAL in all error cases ? Or maybe different
> error codes to mean "the backend doesn't support the concept of IDs",
> and "the device doesn't have an ID" ?

We may actually get mapping to all three if first we check for the
method/name existence followed by value check.
But I don't think we need to bloat this simple one.

> > +             *id = (u32)adr;
> > +             return 0;
> > +     }
> > +     return -EINVAL;
> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()

[Calvin Johnson]

Hi Laurent,

Thanks for reviewing.
On Tue, Dec 15, 2020 at 07:00:28PM +0200, Laurent Pinchart wrote:
> Hi Calvin,
> 
> Thank you for the patch.
> 
> On Tue, Dec 15, 2020 at 10:13:11PM +0530, Calvin Johnson wrote:
> > Using fwnode_get_id(), get the reg property value for DT node
> > and get the _ADR object value for ACPI node.
> > 
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > ---
> > 
> > Changes in v2: None
> > 
> >  drivers/base/property.c  | 26 ++++++++++++++++++++++++++
> >  include/linux/property.h |  1 +
> >  2 files changed, 27 insertions(+)
> > 
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 4c43d30145c6..1c50e17ae879 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -580,6 +580,32 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
> >  	return fwnode_call_ptr_op(fwnode, get_name_prefix);
> >  }
> >  
> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
> > + *
> 
> Is the concept of fwnode ID documented clearly somewhere ? I think this
> function should otherwise have more documentation, at least to explain
> what the ID is.

Agree. Will add more info here.
> 
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > +	unsigned long long adr;
> > +	acpi_status status;
> > +
> > +	if (is_of_node(fwnode)) {
> > +		return of_property_read_u32(to_of_node(fwnode), "reg", id);
> > +	} else if (is_acpi_node(fwnode)) {
> > +		status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > +					       METHOD_NAME__ADR, NULL, &adr);
> > +		if (ACPI_FAILURE(status))
> > +			return -ENODATA;
> 
> Would it make sense to standardize error codes ? of_property_read_u32()
> can return -EINVAL, -ENODATA or -EOVERFLOW. I don't think the caller of
> this function would be very interested to tell those three cases apart.
> Maybe we should return -EINVAL in all error cases ? Or maybe different
> error codes to mean "the backend doesn't support the concept of IDs",
> and "the device doesn't have an ID" ?

I think it make sense to return just -EINVAL. Will take care in v3.

Thanks
Calvin

Re: [net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()

[Andy Shevchenko]

On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Using fwnode_get_id(), get the reg property value for DT node
> and get the _ADR object value for ACPI node.

and -> or

...

> +/**
> + * fwnode_get_id - Get the id of a fwnode.
> + * @fwnode: firmware node
> + * @id: id of the fwnode
> + *
> + * Returns 0 on success or a negative errno.
> + */
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> +{
> +       unsigned long long adr;
> +       acpi_status status;
> +
> +       if (is_of_node(fwnode)) {
> +               return of_property_read_u32(to_of_node(fwnode), "reg", id);

ACPI nodes can hold reg property as well. I would rather think about

ret = fwnode_property_read_u32(fwnode, "reg", id)
if (!(ret && is_acpi_node(fwnode)))
  return ret;

> +       } else if (is_acpi_node(fwnode)) {

Redundant 'else'

> +               status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> +                                              METHOD_NAME__ADR, NULL, &adr);
> +               if (ACPI_FAILURE(status))
> +                       return -ENODATA;

I'm wondering if it compiles when CONFIG_ACPI=n.

> +               *id = (u32)adr;
> +               return 0;
> +       }

> +       return -EINVAL;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()

[Calvin Johnson]

On Tue, Dec 15, 2020 at 07:45:16PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Using fwnode_get_id(), get the reg property value for DT node
> > and get the _ADR object value for ACPI node.
> 
> and -> or
> 
> ...
> 
> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
> > + *
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > +       unsigned long long adr;
> > +       acpi_status status;
> > +
> > +       if (is_of_node(fwnode)) {
> > +               return of_property_read_u32(to_of_node(fwnode), "reg", id);
> 
> ACPI nodes can hold reg property as well. I would rather think about
> 
> ret = fwnode_property_read_u32(fwnode, "reg", id)
> if (!(ret && is_acpi_node(fwnode)))
>   return ret;
> 
Got it. Will rework on it.
> > +       } else if (is_acpi_node(fwnode)) {
> 
> Redundant 'else'
> 
> > +               status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > +                                              METHOD_NAME__ADR, NULL, &adr);
> > +               if (ACPI_FAILURE(status))
> > +                       return -ENODATA;
> 
> I'm wondering if it compiles when CONFIG_ACPI=n.
Correct. It doesn't compile for non-ACPI case. Will resolve it.

Thanks
Calvin

[net-next PATCH v2 11/14] phylink: introduce phylink_fwnode_phy_connect()

[Calvin Johnson]

Define phylink_fwnode_phy_connect() to connect phy specified by
a fwnode to a phylink instance.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2: None

 drivers/net/phy/phylink.c | 54 +++++++++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |  3 +++
 2 files changed, 57 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 84f6e197f965..389dc3ec165e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -5,6 +5,7 @@
  *
  * Copyright (C) 2015 Russell King
  */
+#include <linux/acpi.h>
 #include <linux/ethtool.h>
 #include <linux/export.h>
 #include <linux/gpio/consumer.h>
@@ -1120,6 +1121,59 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
 }
 EXPORT_SYMBOL_GPL(phylink_of_phy_connect);
 
+/**
+ * phylink_fwnode_phy_connect() - connect the PHY specified in the fwnode.
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @fwnode: a pointer to a &struct fwnode_handle.
+ * @flags: PHY-specific flags to communicate to the PHY device driver
+ *
+ * Connect the phy specified @fwnode to the phylink instance specified
+ * by @pl.
+ *
+ * Returns 0 on success or a negative errno.
+ */
+int phylink_fwnode_phy_connect(struct phylink *pl,
+			       struct fwnode_handle *fwnode,
+			       u32 flags)
+{
+	struct fwnode_handle *phy_fwnode;
+	struct phy_device *phy_dev;
+	int ret;
+
+	if (is_of_node(fwnode)) {
+		/* Fixed links and 802.3z are handled without needing a PHY */
+		if (pl->cfg_link_an_mode == MLO_AN_FIXED ||
+		    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
+		     phy_interface_mode_is_8023z(pl->link_interface)))
+			return 0;
+	}
+
+	phy_fwnode = fwnode_get_phy_node(fwnode);
+	if (IS_ERR(phy_fwnode)) {
+		if (pl->cfg_link_an_mode == MLO_AN_PHY)
+			return -ENODEV;
+		return 0;
+	}
+
+	phy_dev = fwnode_phy_find_device(phy_fwnode);
+	/* We're done with the phy_node handle */
+	fwnode_handle_put(phy_fwnode);
+	if (!phy_dev)
+		return -ENODEV;
+
+	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
+				pl->link_interface);
+	if (ret)
+		return ret;
+
+	ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface);
+	if (ret)
+		phy_detach(phy_dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phylink_fwnode_phy_connect);
+
 /**
  * phylink_disconnect_phy() - disconnect any PHY attached to the phylink
  *   instance.
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index d81a714cfbbd..75d4f99090fd 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -439,6 +439,9 @@ void phylink_destroy(struct phylink *);
 
 int phylink_connect_phy(struct phylink *, struct phy_device *);
 int phylink_of_phy_connect(struct phylink *, struct device_node *, u32 flags);
+int phylink_fwnode_phy_connect(struct phylink *pl,
+			       struct fwnode_handle *fwnode,
+			       u32 flags);
 void phylink_disconnect_phy(struct phylink *);
 
 void phylink_mac_change(struct phylink *, bool up);
-- 
2.17.1

[net-next PATCH v2 12/14] net: phylink: Refactor phylink_of_phy_connect()

[Calvin Johnson]

Refactor phylink_of_phy_connect() to use phylink_fwnode_phy_connect().

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2: None

 drivers/net/phy/phylink.c | 39 +--------------------------------------
 1 file changed, 1 insertion(+), 38 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 389dc3ec165e..26f014f0ad42 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1080,44 +1080,7 @@ EXPORT_SYMBOL_GPL(phylink_connect_phy);
 int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
 			   u32 flags)
 {
-	struct device_node *phy_node;
-	struct phy_device *phy_dev;
-	int ret;
-
-	/* Fixed links and 802.3z are handled without needing a PHY */
-	if (pl->cfg_link_an_mode == MLO_AN_FIXED ||
-	    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
-	     phy_interface_mode_is_8023z(pl->link_interface)))
-		return 0;
-
-	phy_node = of_parse_phandle(dn, "phy-handle", 0);
-	if (!phy_node)
-		phy_node = of_parse_phandle(dn, "phy", 0);
-	if (!phy_node)
-		phy_node = of_parse_phandle(dn, "phy-device", 0);
-
-	if (!phy_node) {
-		if (pl->cfg_link_an_mode == MLO_AN_PHY)
-			return -ENODEV;
-		return 0;
-	}
-
-	phy_dev = of_phy_find_device(phy_node);
-	/* We're done with the phy_node handle */
-	of_node_put(phy_node);
-	if (!phy_dev)
-		return -ENODEV;
-
-	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
-				pl->link_interface);
-	if (ret)
-		return ret;
-
-	ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface);
-	if (ret)
-		phy_detach(phy_dev);
-
-	return ret;
+	return phylink_fwnode_phy_connect(pl, of_fwnode_handle(dn), flags);
 }
 EXPORT_SYMBOL_GPL(phylink_of_phy_connect);
 
-- 
2.17.1

[net-next PATCH v2 13/14] net: phy: Introduce fwnode_mdio_find_device()

[Calvin Johnson]

Define fwnode_mdio_find_device() to get a pointer to the
mdio_device from fwnode passed to the function.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2: None

 drivers/net/mdio/of_mdio.c   | 11 +----------
 drivers/net/phy/phy_device.c | 23 +++++++++++++++++++++++
 include/linux/phy.h          |  6 ++++++
 3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index fa914d285271..1b561849269e 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -300,16 +300,7 @@ EXPORT_SYMBOL(of_mdiobus_register);
  */
 struct mdio_device *of_mdio_find_device(struct device_node *np)
 {
-	struct device *d;
-
-	if (!np)
-		return NULL;
-
-	d = bus_find_device_by_of_node(&mdio_bus_type, np);
-	if (!d)
-		return NULL;
-
-	return to_mdio_device(d);
+	return fwnode_mdio_find_device(of_fwnode_handle(np));
 }
 EXPORT_SYMBOL(of_mdio_find_device);
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6fad89c02c5a..17d20e6d5416 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2851,6 +2851,29 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
 	return phydrv->config_intr && phydrv->handle_interrupt;
 }
 
+/**
+ * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
+ * @np: pointer to the mdio_device's fwnode
+ *
+ * If successful, returns a pointer to the mdio_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure.
+ * The caller should call put_device() on the mdio_device after its use
+ */
+struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode)
+{
+	struct device *d;
+
+	if (!fwnode)
+		return NULL;
+
+	d = bus_find_device_by_fwnode(&mdio_bus_type, fwnode);
+	if (!d)
+		return NULL;
+
+	return to_mdio_device(d);
+}
+EXPORT_SYMBOL(fwnode_mdio_find_device);
+
 /**
  * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
  * phy_fwnode.
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 67ea4ca6f76f..5a0c290ff0f4 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1343,6 +1343,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 				     struct phy_c45_device_ids *c45_ids);
 #if IS_ENABLED(CONFIG_PHYLIB)
 int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id);
+struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode);
 struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
 struct phy_device *device_phy_find_device(struct device *dev);
 struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode);
@@ -1355,6 +1356,11 @@ static inline int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
 	return 0;
 }
 static inline
+struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode)
+{
+	return 0;
+}
+static inline
 struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
 {
 	return NULL;
-- 
2.17.1

[net-next PATCH v2 14/14] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

[Calvin Johnson]

Modify dpaa2_mac_connect() to support ACPI along with DT.
Modify dpaa2_mac_get_node() to get the dpmac fwnode from either
DT or ACPI.

Replace of_get_phy_mode with fwnode_get_phy_mode to get
phy-mode for a dpmac_node.

Use helper function phylink_fwnode_phy_connect() to find phy_dev and
connect to mac->phylink.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v2:
- Refactor OF functions to use fwnode functions

 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 86 +++++++++++--------
 1 file changed, 50 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index 828c177df03d..c242d5c2a9ed 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -1,6 +1,9 @@
 // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
 /* Copyright 2019 NXP */
 
+#include <linux/acpi.h>
+#include <linux/property.h>
+
 #include "dpaa2-eth.h"
 #include "dpaa2-mac.h"
 
@@ -34,39 +37,47 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)
 	return 0;
 }
 
-/* Caller must call of_node_put on the returned value */
-static struct device_node *dpaa2_mac_get_node(u16 dpmac_id)
+static struct fwnode_handle *dpaa2_mac_get_node(struct device *dev,
+						u16 dpmac_id)
 {
-	struct device_node *dpmacs, *dpmac = NULL;
-	u32 id;
+	struct device_node *dpmacs = NULL;
+	struct fwnode_handle *parent, *child  = NULL;
 	int err;
+	u32 id;
 
-	dpmacs = of_find_node_by_name(NULL, "dpmacs");
-	if (!dpmacs)
-		return NULL;
+	if (is_of_node(dev->parent->fwnode)) {
+		dpmacs = of_find_node_by_name(NULL, "dpmacs");
+		if (!dpmacs)
+			return NULL;
+		parent = of_fwnode_handle(dpmacs);
+	} else if (is_acpi_node(dev->parent->fwnode)) {
+		parent = dev->parent->fwnode;
+	}
 
-	while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
-		err = of_property_read_u32(dpmac, "reg", &id);
-		if (err)
+	fwnode_for_each_child_node(parent, child) {
+		err = fwnode_get_id(child, &id);
+		if (err) {
 			continue;
-		if (id == dpmac_id)
-			break;
+		} else if (id == dpmac_id) {
+			if (is_of_node(dev->parent->fwnode))
+				of_node_put(dpmacs);
+			return child;
+		}
 	}
-
-	of_node_put(dpmacs);
-
-	return dpmac;
+	if (is_of_node(dev->parent->fwnode))
+		of_node_put(dpmacs);
+	return NULL;
 }
 
-static int dpaa2_mac_get_if_mode(struct device_node *node,
+static int dpaa2_mac_get_if_mode(struct fwnode_handle *dpmac_node,
 				 struct dpmac_attr attr)
 {
 	phy_interface_t if_mode;
 	int err;
 
-	err = of_get_phy_mode(node, &if_mode);
-	if (!err)
-		return if_mode;
+	err = fwnode_get_phy_mode(dpmac_node);
+	if (err > 0)
+		return err;
 
 	err = phy_mode(attr.eth_if, &if_mode);
 	if (!err)
@@ -255,26 +266,27 @@ bool dpaa2_mac_is_type_fixed(struct fsl_mc_device *dpmac_dev,
 }
 
 static int dpaa2_pcs_create(struct dpaa2_mac *mac,
-			    struct device_node *dpmac_node, int id)
+			    struct fwnode_handle *dpmac_node,
+			    int id)
 {
 	struct mdio_device *mdiodev;
-	struct device_node *node;
+	struct fwnode_handle *node;
 
-	node = of_parse_phandle(dpmac_node, "pcs-handle", 0);
-	if (!node) {
+	node = fwnode_find_reference(dpmac_node, "pcs-handle", 0);
+	if (IS_ERR(node)) {
 		/* do not error out on old DTS files */
 		netdev_warn(mac->net_dev, "pcs-handle node not found\n");
 		return 0;
 	}
 
-	if (!of_device_is_available(node)) {
+	if (!of_device_is_available(to_of_node(node))) {
 		netdev_err(mac->net_dev, "pcs-handle node not available\n");
-		of_node_put(node);
+		of_node_put(to_of_node(node));
 		return -ENODEV;
 	}
 
-	mdiodev = of_mdio_find_device(node);
-	of_node_put(node);
+	mdiodev = fwnode_mdio_find_device(node);
+	fwnode_handle_put(node);
 	if (!mdiodev)
 		return -EPROBE_DEFER;
 
@@ -304,7 +316,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 {
 	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
 	struct net_device *net_dev = mac->net_dev;
-	struct device_node *dpmac_node;
+	struct fwnode_handle *dpmac_node = NULL;
 	struct phylink *phylink;
 	struct dpmac_attr attr;
 	int err;
@@ -324,7 +336,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 
 	mac->if_link_type = attr.link_type;
 
-	dpmac_node = dpaa2_mac_get_node(attr.id);
+	dpmac_node = dpaa2_mac_get_node(&mac->mc_dev->dev, attr.id);
 	if (!dpmac_node) {
 		netdev_err(net_dev, "No dpmac@%d node found.\n", attr.id);
 		err = -ENODEV;
@@ -342,7 +354,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	 * error out if the interface mode requests them and there is no PHY
 	 * to act upon them
 	 */
-	if (of_phy_is_fixed_link(dpmac_node) &&
+	if (of_phy_is_fixed_link(to_of_node(dpmac_node)) &&
 	    (mac->if_mode == PHY_INTERFACE_MODE_RGMII_ID ||
 	     mac->if_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
 	     mac->if_mode == PHY_INTERFACE_MODE_RGMII_TXID)) {
@@ -362,7 +374,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	mac->phylink_config.type = PHYLINK_NETDEV;
 
 	phylink = phylink_create(&mac->phylink_config,
-				 of_fwnode_handle(dpmac_node), mac->if_mode,
+				 dpmac_node, mac->if_mode,
 				 &dpaa2_mac_phylink_ops);
 	if (IS_ERR(phylink)) {
 		err = PTR_ERR(phylink);
@@ -373,13 +385,14 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	if (mac->pcs)
 		phylink_set_pcs(mac->phylink, &mac->pcs->pcs);
 
-	err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0);
+	err = phylink_fwnode_phy_connect(mac->phylink, dpmac_node, 0);
 	if (err) {
-		netdev_err(net_dev, "phylink_of_phy_connect() = %d\n", err);
+		netdev_err(net_dev, "phylink_fwnode_phy_connect() = %d\n", err);
 		goto err_phylink_destroy;
 	}
 
-	of_node_put(dpmac_node);
+	if (is_of_node(dpmac_node))
+		fwnode_handle_put(dpmac_node);
 
 	return 0;
 
@@ -388,7 +401,8 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 err_pcs_destroy:
 	dpaa2_pcs_destroy(mac);
 err_put_node:
-	of_node_put(dpmac_node);
+	if (is_of_node(dpmac_node))
+		fwnode_handle_put(dpmac_node);
 err_close_dpmac:
 	dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle);
 	return err;
-- 
2.17.1