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

[net-next PATCH v1] 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 dpaa2_find_phy_device() to find phy_dev
that is later connected to mac->phylink.

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

---

 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 114 +++++++++++++-----
 1 file changed, 86 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..163da735ab29 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,54 @@ 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 *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)) {
+				dev_info(dev, "_ADR returned status 0x%x\n", status);
+				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,11 +245,41 @@ bool dpaa2_mac_is_type_fixed(struct fsl_mc_device *dpmac_dev,
 	return fixed;
 }
 
+static struct phy_device *dpaa2_find_phy_device(struct fwnode_handle *fwnode)
+{
+	struct fwnode_reference_args args;
+	struct platform_device *pdev;
+	struct mii_bus *mdio;
+	struct device *dev;
+	acpi_status status;
+	int addr;
+	int err;
+
+	status = acpi_node_get_property_reference(fwnode, "mdio-handle",
+						  0, &args);
+
+	if (ACPI_FAILURE(status) || !is_acpi_device_node(args.fwnode))
+		return NULL;
+
+	dev = bus_find_device_by_fwnode(&platform_bus_type, args.fwnode);
+	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);
+}
+
 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 +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(&dpmac_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,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 = dpaa2_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 v1] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

[Ioana Ciornei]

> Subject: [net-next PATCH v1] 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 dpaa2_find_phy_device() to find phy_dev that is
> later connected to mac->phylink.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> 
> ---
> 
>  .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 114 +++++++++++++-----
>  1 file changed, 86 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..163da735ab29 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,54 @@ 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 *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;


Hi Calvin,

Unfortunately, this is breaking the OF use case.

[    4.236045] fsl_dpaa2_eth dpni.0 (unnamed net_device) (uninitialized): No dpmac@17 node found.              
[    4.245646] fsl_dpaa2_eth dpni.0 (unnamed net_device) (uninitialized): Error connecting to the MAC endpoint 
[    4.331921] fsl_dpaa2_eth dpni.0: fsl_mc_driver_probe failed: -19                                           

You replaced of_find_node_by_name() which searches the entire DTS
file (hence the NULL first parameter) with device_get_named_child_node(dev->parent, ..)
which only searches starting with the dev->parent device. In this case, the
parent device is dprc.1 (the root container) which is not probing on the
device tree so the associated fwnode_handle is NULL.

Regards,
Ioana

> +
> +		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)) {
> +				dev_info(dev, "_ADR returned status 0x%x\n",
> status);
> +				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,11 +245,41 @@ bool dpaa2_mac_is_type_fixed(struct fsl_mc_device
> *dpmac_dev,
>  	return fixed;
>  }
> 
> +static struct phy_device *dpaa2_find_phy_device(struct fwnode_handle
> +*fwnode) {
> +	struct fwnode_reference_args args;
> +	struct platform_device *pdev;
> +	struct mii_bus *mdio;
> +	struct device *dev;
> +	acpi_status status;
> +	int addr;
> +	int err;
> +
> +	status = acpi_node_get_property_reference(fwnode, "mdio-handle",
> +						  0, &args);
> +
> +	if (ACPI_FAILURE(status) || !is_acpi_device_node(args.fwnode))
> +		return NULL;
> +
> +	dev = bus_find_device_by_fwnode(&platform_bus_type, args.fwnode);
> +	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);
> +}
> +
>  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 +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(&dpmac_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,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 = dpaa2_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 v1] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

[Calvin Johnson]

Hi Ioana,

On Thu, Jun 25, 2020 at 07:59:19AM +0000, Ioana Ciornei wrote:
> > Subject: [net-next PATCH v1] 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 dpaa2_find_phy_device() to find phy_dev that is
> > later connected to mac->phylink.
> > 
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > 
> > ---
> > 
> >  .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 114 +++++++++++++-----
> >  1 file changed, 86 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..163da735ab29 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,54 @@ 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 *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;
> 
> 
> Hi Calvin,
> 
> Unfortunately, this is breaking the OF use case.
> 
> [    4.236045] fsl_dpaa2_eth dpni.0 (unnamed net_device) (uninitialized): No dpmac@17 node found.              
> [    4.245646] fsl_dpaa2_eth dpni.0 (unnamed net_device) (uninitialized): Error connecting to the MAC endpoint 
> [    4.331921] fsl_dpaa2_eth dpni.0: fsl_mc_driver_probe failed: -19                                           
> 
> You replaced of_find_node_by_name() which searches the entire DTS
> file (hence the NULL first parameter) with device_get_named_child_node(dev->parent, ..)
> which only searches starting with the dev->parent device. In this case, the
> parent device is dprc.1 (the root container) which is not probing on the
> device tree so the associated fwnode_handle is NULL.

I had tested with UEFI+net-next tree and didn't encounter any such issue. Let
me see how to resolve this.

Thanks
Calvin

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2854 bytes --]

Hi Calvin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Calvin-Johnson/net-dpaa2-mac-Add-ACPI-support-for-DPAA2-MAC-driver/20200625-123653
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 147373d968f1c1b5d6bb71e4e8b7495eeb9cdcae
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c: In function 'dpaa2_mac_get_node':
>> drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c:52:13: error: implicit declaration of function 'acpi_evaluate_integer'; did you mean 'acpi_evaluate_object'? [-Werror=implicit-function-declaration]
      52 |    status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
         |             ^~~~~~~~~~~~~~~~~~~~~
         |             acpi_evaluate_object
   cc1: some warnings being treated as errors

vim +52 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c

    26	
    27	/* Caller must call of_node_put on the returned value */
    28	static struct fwnode_handle *dpaa2_mac_get_node(struct device *dev,
    29							u16 dpmac_id)
    30	{
    31		struct fwnode_handle *dpmacs, *dpmac = NULL;
    32		unsigned long long adr;
    33		acpi_status status;
    34		int err;
    35		u32 id;
    36	
    37		if (is_of_node(dev->parent->fwnode)) {
    38			dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
    39			if (!dpmacs)
    40				return NULL;
    41	
    42			while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
    43				err = fwnode_property_read_u32(dpmac, "reg", &id);
    44				if (err)
    45					continue;
    46				if (id == dpmac_id)
    47					return dpmac;
    48			}
    49	
    50		} else if (is_acpi_node(dev->parent->fwnode)) {
    51			device_for_each_child_node(dev->parent, dpmac) {
  > 52				status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
    53							       "_ADR", NULL, &adr);
    54				if (ACPI_FAILURE(status)) {
    55					dev_info(dev, "_ADR returned status 0x%x\n", status);
    56					continue;
    57				} else {
    58					id = (u32)adr;
    59					if (id == dpmac_id)
    60						return dpmac;
    61				}
    62			}
    63		}
    64		return NULL;
    65	}
    66	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69759 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2931 bytes --]

Hi Calvin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Calvin-Johnson/net-dpaa2-mac-Add-ACPI-support-for-DPAA2-MAC-driver/20200625-123653
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 147373d968f1c1b5d6bb71e4e8b7495eeb9cdcae
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c: In function 'dpaa2_mac_get_node':
>> drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c:52:13: error: implicit declaration of function 'acpi_evaluate_integer'; did you mean 'acpi_evaluate_object'? [-Werror=implicit-function-declaration]
      52 |    status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
         |             ^~~~~~~~~~~~~~~~~~~~~
         |             acpi_evaluate_object
   cc1: some warnings being treated as errors

vim +52 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c

    26	
    27	/* Caller must call of_node_put on the returned value */
    28	static struct fwnode_handle *dpaa2_mac_get_node(struct device *dev,
    29							u16 dpmac_id)
    30	{
    31		struct fwnode_handle *dpmacs, *dpmac = NULL;
    32		unsigned long long adr;
    33		acpi_status status;
    34		int err;
    35		u32 id;
    36	
    37		if (is_of_node(dev->parent->fwnode)) {
    38			dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
    39			if (!dpmacs)
    40				return NULL;
    41	
    42			while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
    43				err = fwnode_property_read_u32(dpmac, "reg", &id);
    44				if (err)
    45					continue;
    46				if (id == dpmac_id)
    47					return dpmac;
    48			}
    49	
    50		} else if (is_acpi_node(dev->parent->fwnode)) {
    51			device_for_each_child_node(dev->parent, dpmac) {
  > 52				status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
    53							       "_ADR", NULL, &adr);
    54				if (ACPI_FAILURE(status)) {
    55					dev_info(dev, "_ADR returned status 0x%x\n", status);
    56					continue;
    57				} else {
    58					id = (u32)adr;
    59					if (id == dpmac_id)
    60						return dpmac;
    61				}
    62			}
    63		}
    64		return NULL;
    65	}
    66	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 69759 bytes --]

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

[Andy Shevchenko]

On Thu, Jun 25, 2020 at 2:06 PM kernel test robot <lkp@intel.com> wrote:

>     50          } else if (is_acpi_node(dev->parent->fwnode)) {

Hmm... Is it a device or data node?

>     51                  device_for_each_child_node(dev->parent, dpmac) {
>   > 52                          status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
>     53                                                         "_ADR", NULL, &adr);
>     54                          if (ACPI_FAILURE(status)) {
>     55                                  dev_info(dev, "_ADR returned status 0x%x\n", status);
>     56                                  continue;
>     57                          } else {
>     58                                  id = (u32)adr;
>     59                                  if (id == dpmac_id)
>     60                                          return dpmac;
>     61                          }
>     62                  }

Can't you use

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

?

>     63          }


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

[-- Attachment #1: Type: text/plain, Size: 1145 bytes --]

On Thu, Jun 25, 2020 at 2:06 PM kernel test robot <lkp@intel.com> wrote:

>     50          } else if (is_acpi_node(dev->parent->fwnode)) {

Hmm... Is it a device or data node?

>     51                  device_for_each_child_node(dev->parent, dpmac) {
>   > 52                          status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
>     53                                                         "_ADR", NULL, &adr);
>     54                          if (ACPI_FAILURE(status)) {
>     55                                  dev_info(dev, "_ADR returned status 0x%x\n", status);
>     56                                  continue;
>     57                          } else {
>     58                                  id = (u32)adr;
>     59                                  if (id == dpmac_id)
>     60                                          return dpmac;
>     61                          }
>     62                  }

Can't you use

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

?

>     63          }


-- 
With Best Regards,
Andy Shevchenko

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

[Andrew Lunn]

On Thu, Jun 25, 2020 at 10:05:38AM +0530, Calvin Johnson wrote:

> +static struct phy_device *dpaa2_find_phy_device(struct fwnode_handle *fwnode)
> +{
> +	struct fwnode_reference_args args;
> +	struct platform_device *pdev;
> +	struct mii_bus *mdio;
> +	struct device *dev;
> +	acpi_status status;
> +	int addr;
> +	int err;
> +
> +	status = acpi_node_get_property_reference(fwnode, "mdio-handle",
> +						  0, &args);
> +
> +	if (ACPI_FAILURE(status) || !is_acpi_device_node(args.fwnode))
> +		return NULL;
> +
> +	dev = bus_find_device_by_fwnode(&platform_bus_type, args.fwnode);
> +	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);
> +}

Hi Calvin

I think this needs putting somewhere global, since you are effectively
defines how all ACPI MACs will find their PHY. This becomes the
defacto standard until the ACPI standards committee comes along and
tells you, you are doing it wrong, it should be like....

Does Linux have a location to document all its defacto standard ACPI
stuff? It would be good to document this somewhere.

       Andrew

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

[Calvin Johnson]

On Thu, Jun 25, 2020 at 09:42:11PM +0200, Andrew Lunn wrote:
> On Thu, Jun 25, 2020 at 10:05:38AM +0530, Calvin Johnson wrote:
> 
> > +static struct phy_device *dpaa2_find_phy_device(struct fwnode_handle *fwnode)
> > +{
> > +	struct fwnode_reference_args args;
> > +	struct platform_device *pdev;
> > +	struct mii_bus *mdio;
> > +	struct device *dev;
> > +	acpi_status status;
> > +	int addr;
> > +	int err;
> > +
> > +	status = acpi_node_get_property_reference(fwnode, "mdio-handle",
> > +						  0, &args);
> > +
> > +	if (ACPI_FAILURE(status) || !is_acpi_device_node(args.fwnode))
> > +		return NULL;
> > +
> > +	dev = bus_find_device_by_fwnode(&platform_bus_type, args.fwnode);
> > +	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);
> > +}
> 
> Hi Calvin
> 
> I think this needs putting somewhere global, since you are effectively
> defines how all ACPI MACs will find their PHY. This becomes the
> defacto standard until the ACPI standards committee comes along and
> tells you, you are doing it wrong, it should be like....
> 
> Does Linux have a location to document all its defacto standard ACPI
> stuff? It would be good to document this somewhere.

Hi Andrew

As you know, making this code generic would bring us back to waiting for
ACPI team's approval which is very difficult to get as the ACPI doesn't
have any opinion on MDIO bus.

Like other ACPI ethernet drivers, can't we keep it local to this driver to
avoid the above issue?

I can add more documentation to this function itself.
If we plan to make this approach generic, then it may have to be put in:
Documentation/firmware-guide/acpi/

Please advice.

Thanks
Calvin

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

[Andrew Lunn]

> Hi Andrew
> 
> As you know, making this code generic would bring us back to waiting for
> ACPI team's approval which is very difficult to get as the ACPI doesn't
> have any opinion on MDIO bus.
> 
> Like other ACPI ethernet drivers, can't we keep it local to this driver to
> avoid the above issue?

Hi Calvin

That does not scale. Every driver doing its own thing. Each having its
own bugs, maintenance overheads, documentation problems, no meta
validation of the ACPI tables because every table is different,
etc. Where is the Advanced in that? It sounds more like Primitive,
Chaotic, Antiquated?

Plus having it generic means there is one place which needs
modifications when the ACPI standards committee does decide how this
should be done.

> If we plan to make this approach generic, then it may have to be put in:
> Documentation/firmware-guide/acpi/

So looking in this directory, we have defacto standards,
e.g. linux/Documentation/firmware-guide/acpi/dsd/leds.rst.

So lets add another defacto standard, how you find a PHY.

   Andrew

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

[Calvin Johnson]

Hi Andrew

On Fri, Jun 26, 2020 at 03:39:42PM +0200, Andrew Lunn wrote:
> > Hi Andrew
> > 
> > As you know, making this code generic would bring us back to waiting for
> > ACPI team's approval which is very difficult to get as the ACPI doesn't
> > have any opinion on MDIO bus.
> > 
> > Like other ACPI ethernet drivers, can't we keep it local to this driver to
> > avoid the above issue?
> 
> Hi Calvin
> 
> That does not scale. Every driver doing its own thing. Each having its
> own bugs, maintenance overheads, documentation problems, no meta
> validation of the ACPI tables because every table is different,
> etc. Where is the Advanced in that? It sounds more like Primitive,
> Chaotic, Antiquated?
> 
> Plus having it generic means there is one place which needs
> modifications when the ACPI standards committee does decide how this
> should be done.

I'm very much aligned with your thoughts. Making this generic is the right
approach according to me. Is it sufficient, if we get net subsystem approval?

> 
> > If we plan to make this approach generic, then it may have to be put in:
> > Documentation/firmware-guide/acpi/
> 
> So looking in this directory, we have defacto standards,
> e.g. linux/Documentation/firmware-guide/acpi/dsd/leds.rst.
> 
> So lets add another defacto standard, how you find a PHY.
Sure will add.

Thanks
Calvin

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

[Calvin Johnson]

On Thu, Jun 25, 2020 at 10:29:15PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 25, 2020 at 2:06 PM kernel test robot <lkp@intel.com> wrote:
> 
> >     50          } else if (is_acpi_node(dev->parent->fwnode)) {
> 
> Hmm... Is it a device or data node?

Device node.

> 
> >     51                  device_for_each_child_node(dev->parent, dpmac) {
> >   > 52                          status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
> >     53                                                         "_ADR", NULL, &adr);
> >     54                          if (ACPI_FAILURE(status)) {
> >     55                                  dev_info(dev, "_ADR returned status 0x%x\n", status);
> >     56                                  continue;
> >     57                          } else {
> >     58                                  id = (u32)adr;
> >     59                                  if (id == dpmac_id)
> >     60                                          return dpmac;
> >     61                          }
> >     62                  }
> 
> Can't you use
> 
>  adev = acpi_find_child_device(ACPI_COMPANION(dev->parent), dpmac_id, false);
>  if (adev)
>   return ...

Thanks. It looks better.
> 
> ?
> 
> >     63          }
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko