[RFC net-next PATCH v2 0/2] ACPI support for xgmac_mdio and dpaa2-mac drivers

[RFC net-next PATCH v2 0/2] ACPI support for xgmac_mdio and dpaa2-mac drivers

[Calvin Johnson]

Following other network drivers that supports ACPI,
v2 of this patchset uses non-DT APIs to register mdiobus,
register PHYs, create phylink and connect phy to mac.

This patchset is dependent on fsl-mc-bus patch:
https://lkml.org/lkml/2020/1/28/91

Two helper functions are borrowed from an old patch by Marcin
Wojtas:(mdio_bus: Introduce fwnode MDIO helpers).
https://lkml.org/lkml/2017/12/18/211

Changes in v2:
- Use IS_ERR_OR_NULL for priv->mdio_base instead of plain NULL check
- Add missing terminator of struct acpi_device_id
- Use device_property_read_bool and avoid redundancy
- Add helper functions xgmac_get_phy_id() and xgmac_mdiobus_register_phy()
- Major change following other network drivers supporting ACPI
- dropped v1 patches 1, 2, 4, 5 and 6 as they are no longer valid
- incorporated other v1 review comments

Calvin Johnson (2):
  net/fsl: add ACPI support for mdio bus
  net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 122 +++++++++++----
 drivers/net/ethernet/freescale/xgmac_mdio.c   | 143 +++++++++++++++---
 2 files changed, 215 insertions(+), 50 deletions(-)

-- 
2.17.1

[RFC net-next PATCH v2 1/2] net/fsl: add ACPI support for mdio bus

[Calvin Johnson]

Add ACPI support for MDIO bus registration while maintaining
the existing DT support.

Extract phy_id using xgmac_get_phy_id() from the compatible
string passed through the ACPI table.

Use ACPI property phy-channel to provide ID of the phy.

Use xgmac_mdiobus_register_phy() to register PHY devices
on the MDIO bus.

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

Changes in v2:
- Use IS_ERR_OR_NULL for priv->mdio_base instead of plain NULL check
- Add missing terminator of struct acpi_device_id
- Use device_property_read_bool and avoid redundancy
- Add helper functions xgmac_get_phy_id() and xgmac_mdiobus_register_phy()

 drivers/net/ethernet/freescale/xgmac_mdio.c | 143 +++++++++++++++++---
 1 file changed, 121 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index c82c85ef5fb3..d3480c0e0cf5 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 2019 NXP
  *
  * Authors: Andy Fleming <afleming@freescale.com>
  *          Timur Tabi <timur@freescale.com>
@@ -11,6 +12,7 @@
  * kind, whether express or implied.
  */
 
+#include <linux/property.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
@@ -20,6 +22,7 @@
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/of_mdio.h>
+#include <linux/acpi.h>
 
 /* Number of microseconds to wait for a register to respond */
 #define TIMEOUT	1000
@@ -241,18 +244,81 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
 	return value;
 }
 
+/* Extract the clause 22 phy ID from the compatible string of the form
+ * ethernet-phy-idAAAA.BBBB
+ */
+static int xgmac_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+	const char *cp;
+	unsigned int upper, lower;
+	int ret;
+
+	ret = fwnode_property_read_string(fwnode, "compatible", &cp);
+	if (!ret) {
+		if (sscanf(cp, "ethernet-phy-id%4x.%4x",
+			   &upper, &lower) == 2) {
+			*phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+
+static int xgmac_mdiobus_register_phy(struct mii_bus *bus,
+				      struct fwnode_handle *child, u32 addr)
+{
+	struct phy_device *phy;
+	bool is_c45 = false;
+	int rc;
+	const char *cp;
+	u32 phy_id;
+
+	fwnode_property_read_string(child, "compatible", &cp);
+	if (!strcmp(cp, "ethernet-phy-ieee802.3-c45"))
+		is_c45 = true;
+
+	if (!is_c45 && !xgmac_get_phy_id(child, &phy_id))
+		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+	else
+		phy = get_phy_device(bus, addr, is_c45);
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
+
+	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(child);
+		return rc;
+	}
+
+	dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
+
+	return 0;
+}
+
 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 resource *res;
 	struct mdio_fsl_priv *priv;
 	int ret;
+	struct fwnode_handle *fwnode;
+	struct fwnode_handle *child;
+	int addr, rc;
 
-	ret = of_address_to_resource(np, 0, &res);
-	if (ret) {
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
 		dev_err(&pdev->dev, "could not obtain address\n");
-		return ret;
+		return -ENODEV;
 	}
 
 	bus = mdiobus_alloc_size(sizeof(struct mdio_fsl_priv));
@@ -263,25 +329,55 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 	bus->read = xgmac_mdio_read;
 	bus->write = xgmac_mdio_write;
 	bus->parent = &pdev->dev;
-	snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res.start);
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res->start);
 
 	/* Set the PHY base address */
 	priv = bus->priv;
-	priv->mdio_base = of_iomap(np, 0);
-	if (!priv->mdio_base) {
+	priv->mdio_base = ioremap(res->start, resource_size(res));
+	if (IS_ERR_OR_NULL(priv->mdio_base)) {
 		ret = -ENOMEM;
 		goto err_ioremap;
 	}
 
-	priv->is_little_endian = of_property_read_bool(pdev->dev.of_node,
-						       "little-endian");
-
-	priv->has_a011043 = of_property_read_bool(pdev->dev.of_node,
-						  "fsl,erratum-a011043");
-
-	ret = of_mdiobus_register(bus, np);
-	if (ret) {
-		dev_err(&pdev->dev, "cannot register MDIO bus\n");
+	priv->is_little_endian = device_property_read_bool(&pdev->dev,
+							   "little-endian");
+
+	priv->has_a011043 = device_property_read_bool(&pdev->dev,
+						      "fsl,erratum-a011043");
+	if (is_of_node(pdev->dev.fwnode)) {
+		ret = of_mdiobus_register(bus, np);
+		if (ret) {
+			dev_err(&pdev->dev, "cannot register MDIO bus\n");
+			goto err_registration;
+		}
+	} else if (is_acpi_node(pdev->dev.fwnode)) {
+		/* Mask out all PHYs from auto probing. */
+		bus->phy_mask = ~0;
+		ret = mdiobus_register(bus);
+		if (ret) {
+			dev_err(&pdev->dev, "mdiobus register err(%d)\n", ret);
+			return ret;
+		}
+
+		fwnode = pdev->dev.fwnode;
+	/* Loop over the child nodes and register a phy_device for each PHY */
+		fwnode_for_each_child_node(fwnode, child) {
+			fwnode_property_read_u32(child, "phy-channel", &addr);
+
+			if (addr < 0 || addr >= PHY_MAX_ADDR) {
+				dev_err(&bus->dev, "Invalid PHY address %i\n", addr);
+				continue;
+			}
+
+			rc = xgmac_mdiobus_register_phy(bus, child, addr);
+			if (rc == -ENODEV)
+				dev_err(&bus->dev,
+					"MDIO device at address %d is missing.\n",
+					addr);
+		}
+	} else {
+		dev_err(&pdev->dev, "Cannot get cfg data from DT or ACPI\n");
+		ret = -ENXIO;
 		goto err_registration;
 	}
 
@@ -290,8 +386,6 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 	return 0;
 
 err_registration:
-	iounmap(priv->mdio_base);
-
 err_ioremap:
 	mdiobus_free(bus);
 
@@ -303,13 +397,12 @@ static int xgmac_mdio_remove(struct platform_device *pdev)
 	struct mii_bus *bus = platform_get_drvdata(pdev);
 
 	mdiobus_unregister(bus);
-	iounmap(bus->priv);
 	mdiobus_free(bus);
 
 	return 0;
 }
 
-static const struct of_device_id xgmac_mdio_match[] = {
+static const struct of_device_id xgmac_mdio_of_match[] = {
 	{
 		.compatible = "fsl,fman-xmdio",
 	},
@@ -318,12 +411,18 @@ static const struct of_device_id xgmac_mdio_match[] = {
 	},
 	{},
 };
-MODULE_DEVICE_TABLE(of, xgmac_mdio_match);
+MODULE_DEVICE_TABLE(of, xgmac_mdio_of_match);
+
+static const struct acpi_device_id xgmac_mdio_acpi_match[] = {
+	{"NXP0006", 0}
+};
+MODULE_DEVICE_TABLE(acpi, xgmac_mdio_acpi_match);
 
 static struct platform_driver xgmac_mdio_driver = {
 	.driver = {
 		.name = "fsl-fman_xmdio",
-		.of_match_table = xgmac_mdio_match,
+		.of_match_table = xgmac_mdio_of_match,
+		.acpi_match_table = ACPI_PTR(xgmac_mdio_acpi_match),
 	},
 	.probe = xgmac_mdio_probe,
 	.remove = xgmac_mdio_remove,
-- 
2.17.1

[RFC net-next PATCH v2 2/2] 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 functions fwnode_phy_match() and
fwnode_phy_find_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:
- Major change following other network drivers supporting ACPI
- dropped v1 patches 1, 2, 4, 5 and 6 as they are no longer valid
- incorporated other v1 review comments

 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 122 ++++++++++++++----
 1 file changed, 94 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index 3ee236c5fc37..5a03da54a67f 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -3,6 +3,9 @@
 
 #include "dpaa2-eth.h"
 #include "dpaa2-mac.h"
+#include <linux/acpi.h>
+#include <linux/phy.h>
+#include <linux/phylink.h>
 
 #define phylink_to_dpaa2_mac(config) \
 	container_of((config), struct dpaa2_mac, phylink_config)
@@ -23,38 +26,56 @@ 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 acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct fwnode_handle *dpmacs, *dpmac = NULL;
+	unsigned long long adr;
+	acpi_status status;
 	int err;
+	u32 id;
 
-	dpmacs = of_find_node_by_name(NULL, "dpmacs");
-	if (!dpmacs)
-		return NULL;
+	if (is_of_node(dev->parent->fwnode)) {
+		dpmacs = device_get_named_child_node(dev->parent, "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;
+		}
 
-	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;
+	} else if (is_acpi_node(dev->parent->fwnode)) {
+		device_for_each_child_node(dev->parent, dpmac) {
+			status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
+						       "_ADR", NULL, &adr);
+			if (ACPI_FAILURE(status)) {
+				pr_debug("_ADR returned %d on %s\n",
+					 status, (char *)buffer.pointer);
+				continue;
+			} else {
+				id = (u32)adr;
+				if (id == dpmac_id)
+					return dpmac;
+			}
+		}
 	}
-
-	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)
@@ -227,13 +248,40 @@ bool dpaa2_mac_is_type_fixed(struct fsl_mc_device *dpmac_dev,
 	return fixed;
 }
 
+static int fwnode_phy_match(struct device *dev, const void *phy_fwnode)
+{
+	return dev->fwnode == phy_fwnode;
+}
+
+static struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+	struct device *d;
+	struct mdio_device *mdiodev;
+
+	if (!phy_fwnode)
+		return NULL;
+
+	d = bus_find_device(&mdio_bus_type, NULL, phy_fwnode, fwnode_phy_match);
+	if (d) {
+		mdiodev = to_mdio_device(d);
+		if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
+			return to_phy_device(d);
+		put_device(d);
+	}
+
+	return NULL;
+}
+
 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 fwnode_reference_args args;
+	struct phy_device *phy_dev;
 	struct phylink *phylink;
 	struct dpmac_attr attr;
+	int status;
 	int err;
 
 	err = dpmac_open(mac->mc_io, 0, dpmac_dev->obj_desc.id,
@@ -251,7 +299,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;
@@ -269,7 +317,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 +330,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 +338,38 @@ 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)) {
+		status = acpi_node_get_property_reference(dpmac_node,
+							  "phy-handle",
+							  0, &args);
+		if (ACPI_FAILURE(status))
+			goto err_phylink_destroy;
+		phy_dev = fwnode_phy_find_device(args.fwnode);
+		if (!phy_dev)
+			goto err_phylink_destroy;
+
+		err = phylink_connect_phy(mac->phylink, phy_dev);
+		if (err)
+			phy_detach(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: [RFC net-next PATCH v2 2/2] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

[Russell King - ARM Linux admin]

On Sat, Apr 18, 2020 at 04:24:32PM +0530, Calvin Johnson 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 functions fwnode_phy_match() and
> fwnode_phy_find_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:
> - Major change following other network drivers supporting ACPI
> - dropped v1 patches 1, 2, 4, 5 and 6 as they are no longer valid
> - incorporated other v1 review comments
> 
>  .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 122 ++++++++++++++----
>  1 file changed, 94 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> index 3ee236c5fc37..5a03da54a67f 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> @@ -3,6 +3,9 @@
>  
>  #include "dpaa2-eth.h"
>  #include "dpaa2-mac.h"
> +#include <linux/acpi.h>
> +#include <linux/phy.h>
> +#include <linux/phylink.h>

Why do you need linux/phy.h and linux/phylink.h here?  Please try
building the driver without; you'll find they are already included
via dpaa2-mac.h.

> +static int fwnode_phy_match(struct device *dev, const void *phy_fwnode)
> +{
> +	return dev->fwnode == phy_fwnode;
> +}
> +
> +static struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> +{
> +	struct device *d;
> +	struct mdio_device *mdiodev;
> +
> +	if (!phy_fwnode)
> +		return NULL;
> +
> +	d = bus_find_device(&mdio_bus_type, NULL, phy_fwnode, fwnode_phy_match);
> +	if (d) {
> +		mdiodev = to_mdio_device(d);
> +		if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
> +			return to_phy_device(d);
> +		put_device(d);
> +	}
> +
> +	return NULL;
> +}

This is groping around in the mdio bus implementation details; drivers
must not do this layering violation.  Please propose an interface in
the mdio code to do what you need.

> +
>  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 fwnode_reference_args args;
> +	struct phy_device *phy_dev;
>  	struct phylink *phylink;
>  	struct dpmac_attr attr;
> +	int status;
>  	int err;
>  
>  	err = dpmac_open(mac->mc_io, 0, dpmac_dev->obj_desc.id,
> @@ -251,7 +299,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;
> @@ -269,7 +317,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 +330,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 +338,38 @@ 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)) {
> +		status = acpi_node_get_property_reference(dpmac_node,
> +							  "phy-handle",
> +							  0, &args);
> +		if (ACPI_FAILURE(status))
> +			goto err_phylink_destroy;
> +		phy_dev = fwnode_phy_find_device(args.fwnode);
> +		if (!phy_dev)
> +			goto err_phylink_destroy;
> +
> +		err = phylink_connect_phy(mac->phylink, phy_dev);
> +		if (err)
> +			phy_detach(phy_dev);

phy_detach() reverses the effect of phy_attach_direct().  This is not
the correct cleanup function in this case, because the PHY hasn't been
attached (and phylink_connect_phy() will clean up any effects it has
on error.)  You only need to reverse the effect of your
fwnode_phy_find_device(), which phy_detach() is inappropriate for.

In any case, if this method of getting a PHY is accepted by ACPI folk,
it could be moved into a helper in phylink_fwnode_phy_connect() - and
that really needs to happen before a patch adding this functionality is
acceptable.

> +	}
>  	if (err) {
> -		netdev_err(net_dev, "phylink_of_phy_connect() = %d\n", err);
> +		netdev_err(net_dev, "phylink_fwnode_phy_connect() = %d\n", err);

That's a very misleading change - there is no function named as per your
new name.

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

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [RFC net-next PATCH v2 1/2] net/fsl: add ACPI support for mdio bus

[Russell King - ARM Linux admin]

On Sat, Apr 18, 2020 at 04:24:31PM +0530, Calvin Johnson wrote:
> @@ -241,18 +244,81 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
>  	return value;
>  }
>  
> +/* Extract the clause 22 phy ID from the compatible string of the form
> + * ethernet-phy-idAAAA.BBBB

This comment is incorrect.  What about clause 45 PHYs?

> + */
> +static int xgmac_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
> +{
> +	const char *cp;
> +	unsigned int upper, lower;
> +	int ret;
> +
> +	ret = fwnode_property_read_string(fwnode, "compatible", &cp);
> +	if (!ret) {
> +		if (sscanf(cp, "ethernet-phy-id%4x.%4x",
> +			   &upper, &lower) == 2) {
> +			*phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
> +			return 0;
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +static int xgmac_mdiobus_register_phy(struct mii_bus *bus,
> +				      struct fwnode_handle *child, u32 addr)
> +{
> +	struct phy_device *phy;
> +	bool is_c45 = false;
> +	int rc;
> +	const char *cp;
> +	u32 phy_id;
> +
> +	fwnode_property_read_string(child, "compatible", &cp);
> +	if (!strcmp(cp, "ethernet-phy-ieee802.3-c45"))
> +		is_c45 = true;
> +
> +	if (!is_c45 && !xgmac_get_phy_id(child, &phy_id))
> +		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> +	else
> +		phy = get_phy_device(bus, addr, is_c45);
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);
> +
> +	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(child);
> +		return rc;
> +	}
> +
> +	dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
> +
> +	return 0;

You seem to be duplicating the OF implementation in a private driver,
converting it to fwnode.  This is not how we develop the Linux kernel.
We fix subsystem problems by fixing the subsystems, not by throwing
what should be subsystem code into private drivers.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [RFC net-next PATCH v2 1/2] net/fsl: add ACPI support for mdio bus

[Andy Shevchenko]

On Sat, Apr 18, 2020 at 2:41 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
> On Sat, Apr 18, 2020 at 04:24:31PM +0530, Calvin Johnson wrote:

> You seem to be duplicating the OF implementation in a private driver,
> converting it to fwnode.  This is not how we develop the Linux kernel.
> We fix subsystem problems by fixing the subsystems, not by throwing
> what should be subsystem code into private drivers.

I didn't dive into the details, but I feel same way.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC net-next PATCH v2 1/2] net/fsl: add ACPI support for mdio bus

[Andrew Lunn]

> > +static int xgmac_mdiobus_register_phy(struct mii_bus *bus,
> > +				      struct fwnode_handle *child, u32 addr)
> > +{
> > +	struct phy_device *phy;
> > +	bool is_c45 = false;
> > +	int rc;
> > +	const char *cp;
> > +	u32 phy_id;
> > +
> > +	fwnode_property_read_string(child, "compatible", &cp);
> > +	if (!strcmp(cp, "ethernet-phy-ieee802.3-c45"))
> > +		is_c45 = true;
> > +
> > +	if (!is_c45 && !xgmac_get_phy_id(child, &phy_id))
> > +		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> > +	else
> > +		phy = get_phy_device(bus, addr, is_c45);
> > +	if (IS_ERR(phy))
> > +		return PTR_ERR(phy);
> > +
> > +	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(child);
> > +		return rc;
> > +	}
> > +
> > +	dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
> > +
> > +	return 0;
> 
> You seem to be duplicating the OF implementation in a private driver,
> converting it to fwnode.  This is not how we develop the Linux kernel.
> We fix subsystem problems by fixing the subsystems, not by throwing
> what should be subsystem code into private drivers.

And i think a similar comment was given for v1, but i could be
remembering wrongly.

	    Andrew

Re: [RFC net-next PATCH v2 1/2] net/fsl: add ACPI support for mdio bus

[Andrew Lunn]

> -	ret = of_mdiobus_register(bus, np);

So this is the interesting part. What you really want to be doing is
adding a device_mdiobus_register(bus, dev) to the core. And it needs
to share as much as possible with the of_mdiobus_register()
implementation.

       Andrew

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

[Andrew Lunn]

> -	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)) {
> +		status = acpi_node_get_property_reference(dpmac_node,
> +							  "phy-handle",
> +							  0, &args);
> +		if (ACPI_FAILURE(status))
> +			goto err_phylink_destroy;
> +		phy_dev = fwnode_phy_find_device(args.fwnode);
> +		if (!phy_dev)
> +			goto err_phylink_destroy;
> +
> +		err = phylink_connect_phy(mac->phylink, phy_dev);
> +		if (err)
> +			phy_detach(phy_dev);

So it looks like you need to add a phylink_fwnode_phy_connect(). And
maybe on top of that you need a phylink_device_phy_connect()?

So please stop. Take a step back, look at how the of_, device_,
fwnode_, and acpi_ abstractions all stack on top of each other, then
propose phylib and phylink core changes to implement these
abstractions.

	Andrew

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

[Calvin Johnson]

On Sat, Apr 18, 2020 at 12:38:13PM +0100, Russell King - ARM Linux admin wrote:
> On Sat, Apr 18, 2020 at 04:24:32PM +0530, Calvin Johnson 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 functions fwnode_phy_match() and
> > fwnode_phy_find_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:
> > - Major change following other network drivers supporting ACPI
> > - dropped v1 patches 1, 2, 4, 5 and 6 as they are no longer valid
> > - incorporated other v1 review comments
> > 
> >  .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 122 ++++++++++++++----
> >  1 file changed, 94 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > index 3ee236c5fc37..5a03da54a67f 100644
> > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > @@ -3,6 +3,9 @@
> >  
> >  #include "dpaa2-eth.h"
> >  #include "dpaa2-mac.h"
> > +#include <linux/acpi.h>
> > +#include <linux/phy.h>
> > +#include <linux/phylink.h>
> 
> Why do you need linux/phy.h and linux/phylink.h here?  Please try
> building the driver without; you'll find they are already included
> via dpaa2-mac.h.

You are right. I'll remove them in v3

> > +static int fwnode_phy_match(struct device *dev, const void *phy_fwnode)
> > +{
> > +	return dev->fwnode == phy_fwnode;
> > +}
> > +
> > +static struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> > +{
> > +	struct device *d;
> > +	struct mdio_device *mdiodev;
> > +
> > +	if (!phy_fwnode)
> > +		return NULL;
> > +
> > +	d = bus_find_device(&mdio_bus_type, NULL, phy_fwnode, fwnode_phy_match);
> > +	if (d) {
> > +		mdiodev = to_mdio_device(d);
> > +		if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
> > +			return to_phy_device(d);
> > +		put_device(d);
> > +	}
> > +
> > +	return NULL;
> > +}
> 
> This is groping around in the mdio bus implementation details; drivers
> must not do this layering violation.  Please propose an interface in
> the mdio code to do what you need.

I'll study and propose a solution.

> 
> > +
> >  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 fwnode_reference_args args;
> > +	struct phy_device *phy_dev;
> >  	struct phylink *phylink;
> >  	struct dpmac_attr attr;
> > +	int status;
> >  	int err;
> >  
> >  	err = dpmac_open(mac->mc_io, 0, dpmac_dev->obj_desc.id,
> > @@ -251,7 +299,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;
> > @@ -269,7 +317,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 +330,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 +338,38 @@ 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)) {
> > +		status = acpi_node_get_property_reference(dpmac_node,
> > +							  "phy-handle",
> > +							  0, &args);
> > +		if (ACPI_FAILURE(status))
> > +			goto err_phylink_destroy;
> > +		phy_dev = fwnode_phy_find_device(args.fwnode);
> > +		if (!phy_dev)
> > +			goto err_phylink_destroy;
> > +
> > +		err = phylink_connect_phy(mac->phylink, phy_dev);
> > +		if (err)
> > +			phy_detach(phy_dev);
> 
> phy_detach() reverses the effect of phy_attach_direct().  This is not
> the correct cleanup function in this case, because the PHY hasn't been
> attached (and phylink_connect_phy() will clean up any effects it has
> on error.)  You only need to reverse the effect of your
> fwnode_phy_find_device(), which phy_detach() is inappropriate for.

Got it. I'll repair this part.

> 
> In any case, if this method of getting a PHY is accepted by ACPI folk,
> it could be moved into a helper in phylink_fwnode_phy_connect() - and
> that really needs to happen before a patch adding this functionality is
> acceptable.

There is already similar code in upstream kernel:
https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c#L825
It makes sense to have a generic helper. Will create one.
Hope I can introduce it in the v3 patchset, ofcourse phylink_fwnode_phy_connect will be
defined in a patch before it is called. Let me know if it is not okay.

> 
> > +	}
> >  	if (err) {
> > -		netdev_err(net_dev, "phylink_of_phy_connect() = %d\n", err);
> > +		netdev_err(net_dev, "phylink_fwnode_phy_connect() = %d\n", err);
> 
> That's a very misleading change - there is no function named as per your
> new name.
My bad. Sorry. Will correct it.

Thanks
Calvin

Re: [RFC net-next PATCH v2 1/2] net/fsl: add ACPI support for mdio bus

[Calvin Johnson]

On Sat, Apr 18, 2020 at 12:41:16PM +0100, Russell King - ARM Linux admin wrote:
> On Sat, Apr 18, 2020 at 04:24:31PM +0530, Calvin Johnson wrote:
> > @@ -241,18 +244,81 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
> >  	return value;
> >  }
> >  
> > +/* Extract the clause 22 phy ID from the compatible string of the form
> > + * ethernet-phy-idAAAA.BBBB
> 
> This comment is incorrect.  What about clause 45 PHYs?

Agree. Will correct it.
May be we need a comment update for of_get_phy_id() also.
https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/of/of_mdio.c#L28

<snip>

> > +	/* 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(child);
> > +		return rc;
> > +	}
> > +
> > +	dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
> > +
> > +	return 0;
> 
> You seem to be duplicating the OF implementation in a private driver,
> converting it to fwnode.  This is not how we develop the Linux kernel.
> We fix subsystem problems by fixing the subsystems, not by throwing
> what should be subsystem code into private drivers.

I've used some part of the of_mdiobus_register_phy(). Looks like some
other network drivers using acpi had also taken similar approach.
Anyway, I'll try to make it generic and move out to subsystem.

Thanks
Calvin

Re: [RFC net-next PATCH v2 1/2] net/fsl: add ACPI support for mdio bus

[Calvin Johnson]

On Sat, Apr 18, 2020 at 04:46:06PM +0200, Andrew Lunn wrote:
> > -	ret = of_mdiobus_register(bus, np);
> 
> So this is the interesting part. What you really want to be doing is
> adding a device_mdiobus_register(bus, dev) to the core. And it needs
> to share as much as possible with the of_mdiobus_register()
> implementation.

Sure, will take this approach and submit v3.

Thanks
Calvin