[net-next PATCH v2 0/3] ACPI support for dpaa2 MAC driver.

[net-next PATCH v2 0/3] ACPI support for dpaa2 MAC driver.

[Calvin Johnson]

 This patch series provides ACPI support for dpaa2 MAC driver.
 This also introduces ACPI mechanism to get PHYs registered on a
 MDIO bus and provide them to be connected to MAC.

 This patchset is dependent on the review patches available on:
https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git/log/?h=for-review/acpi-iort-id-rework

 Device Tree can be tested with the below change which is also available in
the above referenced review patches:

--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -931,6 +931,7 @@ static int fsl_mc_bus_probe(struct platform_device *pdev)
        if (error < 0)
                goto error_cleanup_mc_io;

+       mc_bus_dev->dev.fwnode = pdev->dev.fwnode;
        mc->root_mc_bus_dev = mc_bus_dev;
        return 0;


Changes in v2:
    - clean up dpaa2_mac_get_node()
    - introduce find_phy_device()
    - use acpi_find_child_device()

Calvin Johnson (3):
  net: phy: introduce find_phy_device()
  Documentation: ACPI: DSD: Document MDIO PHY
  net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

 Documentation/firmware-guide/acpi/dsd/phy.rst | 40 ++++++++++
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 79 ++++++++++++-------
 drivers/net/phy/phy_device.c                  | 25 ++++++
 include/linux/phy.h                           |  1 +
 4 files changed, 116 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst

-- 
2.17.1

[net-next PATCH v2 1/3] net: phy: introduce find_phy_device()

[Calvin Johnson]

The PHYs on a mdiobus are probed and registered using mdiobus_register().
Later, for connecting these PHYs to MAC, the PHYs registered on the
mdiobus have to be referenced.

For each MAC node, a property "mdio-handle" is used to reference the
MDIO bus on which the PHYs are registered. On getting hold of the MDIO
bus, use find_phy_device() to get the PHY connected to the MAC.

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

Changes in v2: None

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

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index eb1068a77ce1..417000197ab1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -25,6 +25,7 @@
 #include <linux/netdevice.h>
 #include <linux/phy.h>
 #include <linux/phy_led_triggers.h>
+#include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/sfp.h>
 #include <linux/skbuff.h>
@@ -966,6 +967,30 @@ struct phy_device *phy_find_first(struct mii_bus *bus)
 }
 EXPORT_SYMBOL(phy_find_first);
 
+struct phy_device *find_phy_device(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *fwnode_mdio;
+	struct platform_device *pdev;
+	struct mii_bus *mdio;
+	struct device *dev;
+	int addr;
+	int err;
+
+	fwnode_mdio = fwnode_find_reference(fwnode, "mdio-handle", 0);
+	dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode_mdio);
+	if (IS_ERR_OR_NULL(dev))
+		return NULL;
+	pdev =  to_platform_device(dev);
+	mdio = platform_get_drvdata(pdev);
+
+	err = fwnode_property_read_u32(fwnode, "phy-channel", &addr);
+	if (err < 0 || addr < 0 || addr >= PHY_MAX_ADDR)
+		return NULL;
+
+	return mdiobus_get_phy(mdio, addr);
+}
+EXPORT_SYMBOL(find_phy_device);
+
 static void phy_link_change(struct phy_device *phydev, bool up)
 {
 	struct net_device *netdev = phydev->attached_dev;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 101a48fa6750..ef9d7ca5d7ba 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1245,6 +1245,7 @@ int phy_sfp_probe(struct phy_device *phydev,
 struct phy_device *phy_attach(struct net_device *dev, const char *bus_id,
 			      phy_interface_t interface);
 struct phy_device *phy_find_first(struct mii_bus *bus);
+struct phy_device *find_phy_device(struct fwnode_handle *fwnode);
 int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		      u32 flags, phy_interface_t interface);
 int phy_connect_direct(struct net_device *dev, struct phy_device *phydev,
-- 
2.17.1

Re: [net-next PATCH v2 1/3] net: phy: introduce find_phy_device()

[Andy Shevchenko]

On Wed, Jul 1, 2020 at 9:13 AM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> The PHYs on a mdiobus are probed and registered using mdiobus_register().
> Later, for connecting these PHYs to MAC, the PHYs registered on the
> mdiobus have to be referenced.
>
> For each MAC node, a property "mdio-handle" is used to reference the
> MDIO bus on which the PHYs are registered. On getting hold of the MDIO
> bus, use find_phy_device() to get the PHY connected to the MAC.

...

> +       struct platform_device *pdev;

This...

> +       fwnode_mdio = fwnode_find_reference(fwnode, "mdio-handle", 0);
> +       dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode_mdio);

> +       if (IS_ERR_OR_NULL(dev))

IS_ERR()?!

> +               return NULL;

> +       pdev =  to_platform_device(dev);
> +       mdio = platform_get_drvdata(pdev);

...and this can be simple:

mdio = dev_get_drvdata(dev);

> +       err = fwnode_property_read_u32(fwnode, "phy-channel", &addr);
> +       if (err < 0 || addr < 0 || addr >= PHY_MAX_ADDR)
> +               return NULL;

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v2 1/3] net: phy: introduce find_phy_device()

[Andrew Lunn]

> +struct phy_device *find_phy_device(struct fwnode_handle *fwnode)

We should consider the naming convention. All phylib phy functions
start with phy_. We already have phy_find_first(), so maybe
phy_find_by_fwnode() to follow the pattern?

> +{
> +	struct fwnode_handle *fwnode_mdio;
> +	struct platform_device *pdev;
> +	struct mii_bus *mdio;
> +	struct device *dev;
> +	int addr;
> +	int err;
> +
> +	fwnode_mdio = fwnode_find_reference(fwnode, "mdio-handle", 0);
> +	dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode_mdio);
> +	if (IS_ERR_OR_NULL(dev))
> +		return NULL;
> +	pdev =  to_platform_device(dev);
> +	mdio = platform_get_drvdata(pdev);

That is a big assumption to make. Please take a look at the
class_find_device_by_*() functions, as used by of_mdio_find_bus(),
mdio_find_bus(), etc.

	Andrew

[net-next PATCH v2 2/3] 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.

An ACPI node property "mdio-handle" is introduced to reference the
MDIO bus on which PHYs are registered with autoprobing method used
by mdiobus_register().

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

Changes in v2: None

 Documentation/firmware-guide/acpi/dsd/phy.rst | 40 +++++++++++++++++++
 1 file changed, 40 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..78dcb0cacc7e
--- /dev/null
+++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
@@ -0,0 +1,40 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=========================
+MDIO bus and PHYs in ACPI
+=========================
+
+The PHYs on a mdiobus are probed and registered using mdiobus_register().
+Later, for connecting these PHYs to MAC, the PHYs registered on the
+mdiobus have to be referenced.
+
+For each MAC node, a property "mdio-handle" is used to reference the
+MDIO bus on which the PHYs are registered. On getting hold of the MDIO
+bus, use find_phy_device() to get the PHY connected to the MAC.
+
+
+An example of this is show below::
+
+	Scope(\_SB.MCE0.PR17) // 1G
+	{
+	  Name (_DSD, Package () {
+	     ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		 Package () {
+		     Package (2) {"phy-channel", 1},
+		     Package (2) {"phy-mode", "rgmii-id"},
+		     Package (2) {"mdio-handle", Package (){\_SB.MDI0}}
+	      }
+	   })
+	}
+
+	Scope(\_SB.MCE0.PR18) // 1G
+	{
+	  Name (_DSD, Package () {
+	    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		Package () {
+		    Package (2) {"phy-channel", 2},
+		    Package (2) {"phy-mode", "rgmii-id"},
+		    Package (2) {"mdio-handle", Package (){\_SB.MDI0}}
+	    }
+	  })
+	}
-- 
2.17.1

Re: [net-next PATCH v2 2/3] Documentation: ACPI: DSD: Document MDIO PHY

[Andy Shevchenko]

On Wed, Jul 1, 2020 at 9:13 AM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
> provide them to be connected to MAC.
>
> An ACPI node property "mdio-handle" is introduced to reference the
> MDIO bus on which PHYs are registered with autoprobing method used
> by mdiobus_register().

...

> +                    Package (2) {"mdio-handle", Package (){\_SB.MDI0}}

Reference as a package? Hmm... Is it really possible to have more than
one handle here?

...

> +                   Package (2) {"phy-channel", 2},
> +                   Package (2) {"phy-mode", "rgmii-id"},
> +                   Package (2) {"mdio-handle", Package (){\_SB.MDI0}}

And drop all these 2s. They are counted automatically by `iasl`.

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v2 2/3] Documentation: ACPI: DSD: Document MDIO PHY

[Calvin Johnson]

On Wed, Jul 01, 2020 at 01:27:43PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 1, 2020 at 9:13 AM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
> > provide them to be connected to MAC.
> >
> > An ACPI node property "mdio-handle" is introduced to reference the
> > MDIO bus on which PHYs are registered with autoprobing method used
> > by mdiobus_register().
> 
> ...
> 
> > +                    Package (2) {"mdio-handle", Package (){\_SB.MDI0}}
> 
> Reference as a package? Hmm... Is it really possible to have more than
> one handle here?

I didn't get your question here. Is it becasue of the (2)? I'll remove them
as they are automatically counted.

But if it is about the reference as a package. We've other similar examples.
One of them here:
https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hisilicon/Hi1610/Hi1610AcpiTables/Dsdt/D03Hns.asl#L581

> 
> ...
> 
> > +                   Package (2) {"phy-channel", 2},
> > +                   Package (2) {"phy-mode", "rgmii-id"},
> > +                   Package (2) {"mdio-handle", Package (){\_SB.MDI0}}
> 
> And drop all these 2s. They are counted automatically by `iasl`.
> 
Thanks
Calvin

Re: [net-next PATCH v2 2/3] Documentation: ACPI: DSD: Document MDIO PHY

[Andy Shevchenko]

On Fri, Jul 3, 2020 at 2:36 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
> On Wed, Jul 01, 2020 at 01:27:43PM +0300, Andy Shevchenko wrote:
> > On Wed, Jul 1, 2020 at 9:13 AM Calvin Johnson
> > <calvin.johnson@oss.nxp.com> wrote:

...

> > > +                    Package (2) {"mdio-handle", Package (){\_SB.MDI0}}
> >
> > Reference as a package? Hmm... Is it really possible to have more than
> > one handle here?
>
> I didn't get your question here.

> But if it is about the reference as a package. We've other similar examples.
> One of them here:
> https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hisilicon/Hi1610/Hi1610AcpiTables/Dsdt/D03Hns.asl#L581

Thanks for this. It's not the line I was looking for, but I found
another example that answers my question, i.o.w. you may have several
references in one property.

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v2 2/3] Documentation: ACPI: DSD: Document MDIO PHY

[Randy Dunlap]

On 6/30/20 11:12 PM, Calvin Johnson wrote:
> Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
> provide them to be connected to MAC.
> 
> An ACPI node property "mdio-handle" is introduced to reference the
> MDIO bus on which PHYs are registered with autoprobing method used
> by mdiobus_register().
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
> 
> Changes in v2: None
> 
>  Documentation/firmware-guide/acpi/dsd/phy.rst | 40 +++++++++++++++++++
>  1 file changed, 40 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..78dcb0cacc7e
> --- /dev/null
> +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
> @@ -0,0 +1,40 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=========================
> +MDIO bus and PHYs in ACPI
> +=========================
> +
> +The PHYs on a mdiobus are probed and registered using mdiobus_register().

            on an mdiobus (?)

> +Later, for connecting these PHYs to MAC, the PHYs registered on the
> +mdiobus have to be referenced.
> +
> +For each MAC node, a property "mdio-handle" is used to reference the
> +MDIO bus on which the PHYs are registered. On getting hold of the MDIO
> +bus, use find_phy_device() to get the PHY connected to the MAC.
> +
> +
> +An example of this is show below::

                         shown

> +
> +	Scope(\_SB.MCE0.PR17) // 1G


-- 
~Randy

Re: [net-next PATCH v2 2/3] Documentation: ACPI: DSD: Document MDIO PHY

[Andrew Lunn]

> +An example of this is show below::
> +
> +	Scope(\_SB.MCE0.PR17) // 1G
> +	{
> +	  Name (_DSD, Package () {
> +	     ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> +		 Package () {
> +		     Package (2) {"phy-channel", 1},
> +		     Package (2) {"phy-mode", "rgmii-id"},
> +		     Package (2) {"mdio-handle", Package (){\_SB.MDI0}}

Please include the MDIO device node in the example, to make it clearer
how this linking works.

It would also be good to document "phy-mode" in this file.

   Andrew

[net-next PATCH v2 3/3] 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.
Define and use helper function find_phy_device() to find phy_dev
that is later connected to mac->phylink.

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

---

Changes in v2:
    - clean up dpaa2_mac_get_node()
    - introduce find_phy_device()
    - use acpi_find_child_device()

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

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index 3ee236c5fc37..78e8160c9b52 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -3,6 +3,8 @@
 
 #include "dpaa2-eth.h"
 #include "dpaa2-mac.h"
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
 
 #define phylink_to_dpaa2_mac(config) \
 	container_of((config), struct dpaa2_mac, phylink_config)
@@ -23,38 +25,46 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)
 }
 
 /* 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 fwnode_handle *fsl_mc_fwnode = dev->parent->parent->fwnode;
+	struct fwnode_handle *dpmacs, *dpmac = NULL;
+	struct device *fsl_mc = dev->parent->parent;
+	struct acpi_device *adev;
 	int err;
+	u32 id;
 
-	dpmacs = of_find_node_by_name(NULL, "dpmacs");
-	if (!dpmacs)
-		return NULL;
-
-	while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
-		err = of_property_read_u32(dpmac, "reg", &id);
-		if (err)
-			continue;
-		if (id == dpmac_id)
-			break;
+	if (is_of_node(fsl_mc_fwnode)) {
+		dpmacs = device_get_named_child_node(fsl_mc, "dpmacs");
+		if (!dpmacs)
+			return NULL;
+
+		while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
+			err = fwnode_property_read_u32(dpmac, "reg", &id);
+			if (err)
+				continue;
+			if (id == dpmac_id)
+				return dpmac;
+		}
+	} else if (is_acpi_node(fsl_mc_fwnode)) {
+		adev = acpi_find_child_device(ACPI_COMPANION(dev->parent),
+					      dpmac_id, false);
+		if (adev)
+			return (&adev->fwnode);
 	}
-
-	of_node_put(dpmacs);
-
-	return dpmac;
+	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)
@@ -231,7 +241,8 @@ 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 phy_device *phy_dev;
 	struct phylink *phylink;
 	struct dpmac_attr attr;
 	int err;
@@ -251,7 +262,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(&dpmac_dev->dev, attr.id);
 	if (!dpmac_node) {
 		netdev_err(net_dev, "No dpmac@%d node found.\n", attr.id);
 		err = -ENODEV;
@@ -269,7 +280,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)) {
@@ -282,7 +293,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);
@@ -290,20 +301,30 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
 	}
 	mac->phylink = phylink;
 
-	err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0);
+	if (is_of_node(dpmac_node))
+		err = phylink_of_phy_connect(mac->phylink,
+					     to_of_node(dpmac_node), 0);
+	else if (is_acpi_node(dpmac_node)) {
+		phy_dev = find_phy_device(dpmac_node);
+		if (IS_ERR(phy_dev))
+			goto err_phylink_destroy;
+		err = phylink_connect_phy(mac->phylink, phy_dev);
+	}
 	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))
+		of_node_put(to_of_node(dpmac_node));
 
 	return 0;
 
 err_phylink_destroy:
 	phylink_destroy(mac->phylink);
 err_put_node:
-	of_node_put(dpmac_node);
+	if (is_of_node(dpmac_node))
+		of_node_put(to_of_node(dpmac_node));
 err_close_dpmac:
 	dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle);
 	return err;
-- 
2.17.1

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

[Andy Shevchenko]

On Wed, Jul 1, 2020 at 9:13 AM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> 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.
> Define and use helper function find_phy_device() to find phy_dev
> that is later connected to mac->phylink.

...

>  #include "dpaa2-eth.h"
>  #include "dpaa2-mac.h"

> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>

Can we put (more) generic headers atop of (more) private ones?

...

> +       struct fwnode_handle *fsl_mc_fwnode = dev->parent->parent->fwnode;

dev_fwnode() please.

> +       struct fwnode_handle *dpmacs, *dpmac = NULL;
> +       struct device *fsl_mc = dev->parent->parent;

So. something like
       struct device *fsl_mc = dev->parent->parent;
       struct fwnode_handle *fsl_mc_fwnode = dev_fwnode(fsl_mc);

...

> +               dpmacs = device_get_named_child_node(fsl_mc, "dpmacs");

If you have fwnode, why to use device_* API?
               dpmacs = fwnode_get_named_child_node(fsl_mc_fwnode, "dpmacs");

> +               if (!dpmacs)
> +                       return NULL;
> +
> +               while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
> +                       err = fwnode_property_read_u32(dpmac, "reg", &id);
> +                       if (err)
> +                               continue;
> +                       if (id == dpmac_id)
> +                               return dpmac;
> +               }

...

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

is_acpi_device_node() ?

> +               adev = acpi_find_child_device(ACPI_COMPANION(dev->parent),
> +                                             dpmac_id, false);
> +               if (adev)

> +                       return (&adev->fwnode);

No need to have parentheses. Don't we have some special macro to get
fwnode out of ACPI device?

...

> +       err = fwnode_get_phy_mode(dpmac_node);
> +       if (err > 0)
> +               return err;

Positive?! Why? What's going on here?

...

> +       if (is_of_node(dpmac_node))
> +               err = phylink_of_phy_connect(mac->phylink,
> +                                            to_of_node(dpmac_node), 0);
> +       else if (is_acpi_node(dpmac_node)) {
> +               phy_dev = find_phy_device(dpmac_node);
> +               if (IS_ERR(phy_dev))
> +                       goto err_phylink_destroy;
> +               err = phylink_connect_phy(mac->phylink, phy_dev);

Can't you rather provide phylink_fwnode_connect_phy API and drop this
conditional tree entirely?

...

> +       if (is_of_node(dpmac_node))

Redundant.

> +               of_node_put(to_of_node(dpmac_node));

Honestly, looking at this code, I think one needs a bit more time to
get into fwnode paradigm and APIs.

...

> +       if (is_of_node(dpmac_node))

Ditto.

> +               of_node_put(to_of_node(dpmac_node));

-- 
With Best Regards,
Andy Shevchenko

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

[Ioana Ciornei]

> Subject: [net-next PATCH v2 3/3] net: dpaa2-mac: Add ACPI support for DPAA2
> MAC driver
> 
> 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.
> Define and use helper function find_phy_device() to find phy_dev that is later
> connected to mac->phylink.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> 
> ---
> 
> Changes in v2:
>     - clean up dpaa2_mac_get_node()
>     - introduce find_phy_device()
>     - use acpi_find_child_device()
> 
>  .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 79 ++++++++++++-------
>  1 file changed, 50 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> index 3ee236c5fc37..78e8160c9b52 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> @@ -3,6 +3,8 @@
> 
>  #include "dpaa2-eth.h"
>  #include "dpaa2-mac.h"
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> 
>  #define phylink_to_dpaa2_mac(config) \
>  	container_of((config), struct dpaa2_mac, phylink_config) @@ -23,38
> +25,46 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t
> *if_mode)  }
> 
>  /* 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 fwnode_handle *fsl_mc_fwnode = dev->parent->parent-
> >fwnode;
> +	struct fwnode_handle *dpmacs, *dpmac = NULL;
> +	struct device *fsl_mc = dev->parent->parent;
> +	struct acpi_device *adev;
>  	int err;
> +	u32 id;
> 
> -	dpmacs = of_find_node_by_name(NULL, "dpmacs");
> -	if (!dpmacs)
> -		return NULL;
> -
> -	while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
> -		err = of_property_read_u32(dpmac, "reg", &id);
> -		if (err)
> -			continue;
> -		if (id == dpmac_id)
> -			break;
> +	if (is_of_node(fsl_mc_fwnode)) {
> +		dpmacs = device_get_named_child_node(fsl_mc, "dpmacs");
> +		if (!dpmacs)
> +			return NULL;
> +
> +		while ((dpmac = fwnode_get_next_child_node(dpmacs,
> dpmac))) {
> +			err = fwnode_property_read_u32(dpmac, "reg", &id);
> +			if (err)
> +				continue;
> +			if (id == dpmac_id)
> +				return dpmac;
> +		}
> +	} else if (is_acpi_node(fsl_mc_fwnode)) {
> +		adev = acpi_find_child_device(ACPI_COMPANION(dev->parent),
> +					      dpmac_id, false);
> +		if (adev)
> +			return (&adev->fwnode);
>  	}
> -
> -	of_node_put(dpmacs);
> -

This of_node_put() on the 'dpmacs'  node still needs to happen for the OF case.

Ioana

> -	return dpmac;
> +	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)
> @@ -231,7 +241,8 @@ 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 phy_device *phy_dev;
>  	struct phylink *phylink;
>  	struct dpmac_attr attr;
>  	int err;
> @@ -251,7 +262,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(&dpmac_dev->dev, attr.id);
>  	if (!dpmac_node) {
>  		netdev_err(net_dev, "No dpmac@%d node found.\n", attr.id);
>  		err = -ENODEV;
> @@ -269,7 +280,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)) { @@ -
> 282,7 +293,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);
> @@ -290,20 +301,30 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
>  	}
>  	mac->phylink = phylink;
> 
> -	err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0);
> +	if (is_of_node(dpmac_node))
> +		err = phylink_of_phy_connect(mac->phylink,
> +					     to_of_node(dpmac_node), 0);
> +	else if (is_acpi_node(dpmac_node)) {
> +		phy_dev = find_phy_device(dpmac_node);
> +		if (IS_ERR(phy_dev))
> +			goto err_phylink_destroy;
> +		err = phylink_connect_phy(mac->phylink, phy_dev);
> +	}
>  	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))
> +		of_node_put(to_of_node(dpmac_node));
> 
>  	return 0;
> 
>  err_phylink_destroy:
>  	phylink_destroy(mac->phylink);
>  err_put_node:
> -	of_node_put(dpmac_node);
> +	if (is_of_node(dpmac_node))
> +		of_node_put(to_of_node(dpmac_node));
>  err_close_dpmac:
>  	dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle);
>  	return err;
> --
> 2.17.1

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

[Andy Shevchenko]

On Thu, Jul 2, 2020 at 11:48 AM Ioana Ciornei <ioana.ciornei@nxp.com> wrote:
>
> > Subject: [net-next PATCH v2 3/3] net: dpaa2-mac: Add ACPI support for DPAA2
> > MAC driver
> >
> > 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.
> > Define and use helper function find_phy_device() to find phy_dev that is later
> > connected to mac->phylink.

...

> > -     while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
> > -             err = of_property_read_u32(dpmac, "reg", &id);
> > -             if (err)
> > -                     continue;
> > -             if (id == dpmac_id)
> > -                     break;
> > +     if (is_of_node(fsl_mc_fwnode)) {
> > +             dpmacs = device_get_named_child_node(fsl_mc, "dpmacs");
> > +             if (!dpmacs)
> > +                     return NULL;
> > +
> > +             while ((dpmac = fwnode_get_next_child_node(dpmacs,
> > dpmac))) {
> > +                     err = fwnode_property_read_u32(dpmac, "reg", &id);
> > +                     if (err)
> > +                             continue;
> > +                     if (id == dpmac_id)
> > +                             return dpmac;
> > +             }
> > +     } else if (is_acpi_node(fsl_mc_fwnode)) {
> > +             adev = acpi_find_child_device(ACPI_COMPANION(dev->parent),
> > +                                           dpmac_id, false);
> > +             if (adev)
> > +                     return (&adev->fwnode);
> >       }
> > -
> > -     of_node_put(dpmacs);
> > -
>
> This of_node_put() on the 'dpmacs'  node still needs to happen for the OF case.

Actually this also raises the question if ACPI case increases refcount
or not and it should be fixed accordingly (Note, we have to take
reference to fwnode before return in ACPI case and drop reference to
adev).

-- 
With Best Regards,
Andy Shevchenko