diff mbox series

[net-next,RFC,04/14] net: phy: add initial support for PHY package in DT

Message ID 20231120135041.15259-5-ansuelsmth@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: phy: Support DT PHY package | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/codegen success Generated files up to date
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1577 this patch: 1577
netdev/cc_maintainers success CCed 11 of 11 maintainers
netdev/build_clang fail Errors and warnings before: 1170 this patch: 1173
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1614 this patch: 1614
netdev/checkpatch warning WARNING: line length of 84 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc fail Errors and warnings before: 0 this patch: 1
netdev/source_inline success Was 0 now: 0

Commit Message

Christian Marangi Nov. 20, 2023, 1:50 p.m. UTC
Add initial support for PHY package in DT.

Make it easier to define PHY package and describe the global PHY
directly in DT by refereincing them by phandles instead of custom
functions in each PHY driver.

Each PHY in a package needs to be defined in a dedicated node in the
mdio node. This dedicated node needs to have the compatible set to
"ethernet-phy-package" and define "global-phys" and "#global-phy-cells"
respectively to a list of phandle to the global phy to define for the
PHY package and 0 for cells as the phandle won't take any args.

With this defined, the generic PHY probe will join each PHY in this
dedicated node to the package.

PHY driver MUST set the required global PHY count in
.phy_package_global_phy_num to correctly verify that DT define the
correct number of phandle to the required global PHY.

mdio_bus.c and of_mdio.c is updated to now support and parse also
PHY package subnote that have the compatible "phy-package".

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/mdio/of_mdio.c   | 60 ++++++++++++++++++++++-----------
 drivers/net/phy/mdio_bus.c   | 33 ++++++++++++++-----
 drivers/net/phy/phy_device.c | 64 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  5 +++
 4 files changed, 135 insertions(+), 27 deletions(-)

Comments

Simon Horman Nov. 22, 2023, 10:41 a.m. UTC | #1
On Mon, Nov 20, 2023 at 02:50:31PM +0100, Christian Marangi wrote:
> Add initial support for PHY package in DT.
> 
> Make it easier to define PHY package and describe the global PHY
> directly in DT by refereincing them by phandles instead of custom
> functions in each PHY driver.
> 
> Each PHY in a package needs to be defined in a dedicated node in the
> mdio node. This dedicated node needs to have the compatible set to
> "ethernet-phy-package" and define "global-phys" and "#global-phy-cells"
> respectively to a list of phandle to the global phy to define for the
> PHY package and 0 for cells as the phandle won't take any args.
> 
> With this defined, the generic PHY probe will join each PHY in this
> dedicated node to the package.
> 
> PHY driver MUST set the required global PHY count in
> .phy_package_global_phy_num to correctly verify that DT define the
> correct number of phandle to the required global PHY.
> 
> mdio_bus.c and of_mdio.c is updated to now support and parse also
> PHY package subnote that have the compatible "phy-package".
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

...

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index c2bb3f0b9dda..5bf90c49e5bd 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -339,6 +339,8 @@ struct mdio_bus_stats {
>   * phy_package_leave().
>   */
>  struct phy_package_shared {
> +	/* With PHY package defined in DT this points to the PHY package node */
> +	struct device_node *np;
>  	/* addrs list pointer */
>  	/* note that this pointer is shared between different phydevs.
>  	 * It is allocated and freed automatically by phy_package_join() and

Hi Christian,

a minor nit from my side: please add np to the kernel doc for
struct phy_package_shared.

> @@ -888,6 +890,8 @@ struct phy_led {
>   * @flags: A bitfield defining certain other features this PHY
>   *   supports (like interrupts)
>   * @driver_data: Static driver data
> + * @phy_package_global_phy_num: Num of the required global phy
> + *   for PHY package global configuration.
>   *
>   * All functions are optional. If config_aneg or read_status
>   * are not implemented, the phy core uses the genphy versions.

...
Simon Horman Nov. 22, 2023, 10:52 a.m. UTC | #2
On Mon, Nov 20, 2023 at 02:50:31PM +0100, Christian Marangi wrote:
> Add initial support for PHY package in DT.
> 
> Make it easier to define PHY package and describe the global PHY
> directly in DT by refereincing them by phandles instead of custom
> functions in each PHY driver.
> 
> Each PHY in a package needs to be defined in a dedicated node in the
> mdio node. This dedicated node needs to have the compatible set to
> "ethernet-phy-package" and define "global-phys" and "#global-phy-cells"
> respectively to a list of phandle to the global phy to define for the
> PHY package and 0 for cells as the phandle won't take any args.
> 
> With this defined, the generic PHY probe will join each PHY in this
> dedicated node to the package.
> 
> PHY driver MUST set the required global PHY count in
> .phy_package_global_phy_num to correctly verify that DT define the
> correct number of phandle to the required global PHY.
> 
> mdio_bus.c and of_mdio.c is updated to now support and parse also
> PHY package subnote that have the compatible "phy-package".
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Hi Christian,

I was a little hasty in hitting send on my previous message.
Please find some more minor feedback from my side below.

...

> diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
> index 64ebcb6d235c..bb910651118f 100644
> --- a/drivers/net/mdio/of_mdio.c
> +++ b/drivers/net/mdio/of_mdio.c
> @@ -139,6 +139,44 @@ bool of_mdiobus_child_is_phy(struct device_node *child)
>  }
>  EXPORT_SYMBOL(of_mdiobus_child_is_phy);
>  
> +static int __of_mdiobus_parse_phys(struct mii_bus *mdio, struct device_node *np,
> +				   bool *scanphys)
> +{
> +	struct device_node *child;
> +	int addr, rc;
> +
> +	/* Loop over the child nodes and register a phy_device for each phy */
> +	for_each_available_child_of_node(np, child) {
> +		if (of_device_is_compatible(child, "ethernet-phy-package")) {
> +			rc = __of_mdiobus_parse_phys(mdio, child, scanphys);
> +			if (rc && rc != -ENODEV)
> +				return rc;

for_each_available_child_of_node() makes calls to of_node_get() and
of_node_put(), so when jumping out of a loop it is necessary to call
of_node_put(), in this case of_node_put(child).

As flagged by Coccinelle.

Also flagged in of_mdiobus_find_phy() both before and after this patch.

> +
> +			continue;
> +		}
> +
> +		addr = of_mdio_parse_addr(&mdio->dev, child);
> +		if (addr < 0) {
> +			*scanphys = true;
> +			continue;
> +		}
> +
> +		if (of_mdiobus_child_is_phy(child))
> +			rc = of_mdiobus_register_phy(mdio, child, addr);
> +		else
> +			rc = of_mdiobus_register_device(mdio, child, addr);
> +
> +		if (rc == -ENODEV)
> +			dev_err(&mdio->dev,
> +				"MDIO device at address %d is missing.\n",
> +				addr);
> +		else if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * __of_mdiobus_register - Register mii_bus and create PHYs from the device tree
>   * @mdio: pointer to mii_bus structure
> @@ -180,25 +218,9 @@ int __of_mdiobus_register(struct mii_bus *mdio, struct device_node *np,
>  		return rc;
>  
>  	/* Loop over the child nodes and register a phy_device for each phy */
> -	for_each_available_child_of_node(np, child) {
> -		addr = of_mdio_parse_addr(&mdio->dev, child);
> -		if (addr < 0) {
> -			scanphys = true;
> -			continue;
> -		}
> -
> -		if (of_mdiobus_child_is_phy(child))
> -			rc = of_mdiobus_register_phy(mdio, child, addr);
> -		else
> -			rc = of_mdiobus_register_device(mdio, child, addr);
> -
> -		if (rc == -ENODEV)
> -			dev_err(&mdio->dev,
> -				"MDIO device at address %d is missing.\n",
> -				addr);
> -		else if (rc)
> -			goto unregister;
> -	}
> +	rc = __of_mdiobus_parse_phys(mdio, np, &scanphys);
> +	if (rc)
> +		goto unregister;

Jumping to unregister will call of_node_put(child),
however child appears to be uninitialised here.

Flagged by clang-16 W=1 build, and Smatch.

>  
>  	if (!scanphys)
>  		return 0;

...
Christian Marangi Nov. 22, 2023, 6:15 p.m. UTC | #3
On Wed, Nov 22, 2023 at 10:52:43AM +0000, Simon Horman wrote:
> On Mon, Nov 20, 2023 at 02:50:31PM +0100, Christian Marangi wrote:
> > Add initial support for PHY package in DT.
> > 
> > Make it easier to define PHY package and describe the global PHY
> > directly in DT by refereincing them by phandles instead of custom
> > functions in each PHY driver.
> > 
> > Each PHY in a package needs to be defined in a dedicated node in the
> > mdio node. This dedicated node needs to have the compatible set to
> > "ethernet-phy-package" and define "global-phys" and "#global-phy-cells"
> > respectively to a list of phandle to the global phy to define for the
> > PHY package and 0 for cells as the phandle won't take any args.
> > 
> > With this defined, the generic PHY probe will join each PHY in this
> > dedicated node to the package.
> > 
> > PHY driver MUST set the required global PHY count in
> > .phy_package_global_phy_num to correctly verify that DT define the
> > correct number of phandle to the required global PHY.
> > 
> > mdio_bus.c and of_mdio.c is updated to now support and parse also
> > PHY package subnote that have the compatible "phy-package".
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> 
> Hi Christian,
> 
> I was a little hasty in hitting send on my previous message.
> Please find some more minor feedback from my side below.
>

Thanks a lot for the initial review and sorry for the various warning
you had to write about it. I know this was a new concept and that I had
to discuss a lot about the DT structure so I was a bit relaxed in
releasing OF node. Will handle all of them in v2. Again thanks! 

> ...
> 
> > diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
> > index 64ebcb6d235c..bb910651118f 100644
> > --- a/drivers/net/mdio/of_mdio.c
> > +++ b/drivers/net/mdio/of_mdio.c
> > @@ -139,6 +139,44 @@ bool of_mdiobus_child_is_phy(struct device_node *child)
> >  }
> >  EXPORT_SYMBOL(of_mdiobus_child_is_phy);
> >  
> > +static int __of_mdiobus_parse_phys(struct mii_bus *mdio, struct device_node *np,
> > +				   bool *scanphys)
> > +{
> > +	struct device_node *child;
> > +	int addr, rc;
> > +
> > +	/* Loop over the child nodes and register a phy_device for each phy */
> > +	for_each_available_child_of_node(np, child) {
> > +		if (of_device_is_compatible(child, "ethernet-phy-package")) {
> > +			rc = __of_mdiobus_parse_phys(mdio, child, scanphys);
> > +			if (rc && rc != -ENODEV)
> > +				return rc;
> 
> for_each_available_child_of_node() makes calls to of_node_get() and
> of_node_put(), so when jumping out of a loop it is necessary to call
> of_node_put(), in this case of_node_put(child).
> 
> As flagged by Coccinelle.
> 
> Also flagged in of_mdiobus_find_phy() both before and after this patch.
> 
> > +
> > +			continue;
> > +		}
> > +
> > +		addr = of_mdio_parse_addr(&mdio->dev, child);
> > +		if (addr < 0) {
> > +			*scanphys = true;
> > +			continue;
> > +		}
> > +
> > +		if (of_mdiobus_child_is_phy(child))
> > +			rc = of_mdiobus_register_phy(mdio, child, addr);
> > +		else
> > +			rc = of_mdiobus_register_device(mdio, child, addr);
> > +
> > +		if (rc == -ENODEV)
> > +			dev_err(&mdio->dev,
> > +				"MDIO device at address %d is missing.\n",
> > +				addr);
> > +		else if (rc)
> > +			return rc;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * __of_mdiobus_register - Register mii_bus and create PHYs from the device tree
> >   * @mdio: pointer to mii_bus structure
> > @@ -180,25 +218,9 @@ int __of_mdiobus_register(struct mii_bus *mdio, struct device_node *np,
> >  		return rc;
> >  
> >  	/* Loop over the child nodes and register a phy_device for each phy */
> > -	for_each_available_child_of_node(np, child) {
> > -		addr = of_mdio_parse_addr(&mdio->dev, child);
> > -		if (addr < 0) {
> > -			scanphys = true;
> > -			continue;
> > -		}
> > -
> > -		if (of_mdiobus_child_is_phy(child))
> > -			rc = of_mdiobus_register_phy(mdio, child, addr);
> > -		else
> > -			rc = of_mdiobus_register_device(mdio, child, addr);
> > -
> > -		if (rc == -ENODEV)
> > -			dev_err(&mdio->dev,
> > -				"MDIO device at address %d is missing.\n",
> > -				addr);
> > -		else if (rc)
> > -			goto unregister;
> > -	}
> > +	rc = __of_mdiobus_parse_phys(mdio, np, &scanphys);
> > +	if (rc)
> > +		goto unregister;
> 
> Jumping to unregister will call of_node_put(child),
> however child appears to be uninitialised here.
> 
> Flagged by clang-16 W=1 build, and Smatch.
> 
> >  
> >  	if (!scanphys)
> >  		return 0;
> 
> ...
Simon Horman Nov. 22, 2023, 9:14 p.m. UTC | #4
On Wed, Nov 22, 2023 at 07:15:06PM +0100, Christian Marangi wrote:

...

> > Hi Christian,
> > 
> > I was a little hasty in hitting send on my previous message.
> > Please find some more minor feedback from my side below.
> >
> 
> Thanks a lot for the initial review and sorry for the various warning
> you had to write about it. I know this was a new concept and that I had
> to discuss a lot about the DT structure so I was a bit relaxed in
> releasing OF node. Will handle all of them in v2. Again thanks! 

No problem. I'm never sure if this kind of feedback is appropriate for and
RFC or not. And perhaps in this case it would have been better to wait for
at least one more revision. So sorry for the noise at this stage of the
patch-set's development.

...
diff mbox series

Patch

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 64ebcb6d235c..bb910651118f 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -139,6 +139,44 @@  bool of_mdiobus_child_is_phy(struct device_node *child)
 }
 EXPORT_SYMBOL(of_mdiobus_child_is_phy);
 
+static int __of_mdiobus_parse_phys(struct mii_bus *mdio, struct device_node *np,
+				   bool *scanphys)
+{
+	struct device_node *child;
+	int addr, rc;
+
+	/* Loop over the child nodes and register a phy_device for each phy */
+	for_each_available_child_of_node(np, child) {
+		if (of_device_is_compatible(child, "ethernet-phy-package")) {
+			rc = __of_mdiobus_parse_phys(mdio, child, scanphys);
+			if (rc && rc != -ENODEV)
+				return rc;
+
+			continue;
+		}
+
+		addr = of_mdio_parse_addr(&mdio->dev, child);
+		if (addr < 0) {
+			*scanphys = true;
+			continue;
+		}
+
+		if (of_mdiobus_child_is_phy(child))
+			rc = of_mdiobus_register_phy(mdio, child, addr);
+		else
+			rc = of_mdiobus_register_device(mdio, child, addr);
+
+		if (rc == -ENODEV)
+			dev_err(&mdio->dev,
+				"MDIO device at address %d is missing.\n",
+				addr);
+		else if (rc)
+			return rc;
+	}
+
+	return 0;
+}
+
 /**
  * __of_mdiobus_register - Register mii_bus and create PHYs from the device tree
  * @mdio: pointer to mii_bus structure
@@ -180,25 +218,9 @@  int __of_mdiobus_register(struct mii_bus *mdio, struct device_node *np,
 		return rc;
 
 	/* Loop over the child nodes and register a phy_device for each phy */
-	for_each_available_child_of_node(np, child) {
-		addr = of_mdio_parse_addr(&mdio->dev, child);
-		if (addr < 0) {
-			scanphys = true;
-			continue;
-		}
-
-		if (of_mdiobus_child_is_phy(child))
-			rc = of_mdiobus_register_phy(mdio, child, addr);
-		else
-			rc = of_mdiobus_register_device(mdio, child, addr);
-
-		if (rc == -ENODEV)
-			dev_err(&mdio->dev,
-				"MDIO device at address %d is missing.\n",
-				addr);
-		else if (rc)
-			goto unregister;
-	}
+	rc = __of_mdiobus_parse_phys(mdio, np, &scanphys);
+	if (rc)
+		goto unregister;
 
 	if (!scanphys)
 		return 0;
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 25dcaa49ab8b..ecec20fd3fd3 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -455,19 +455,23 @@  EXPORT_SYMBOL(of_mdio_find_bus);
  * found, set the of_node pointer for the mdio device. This allows
  * auto-probed phy devices to be supplied with information passed in
  * via DT.
+ * If a PHY package is found, PHY is searched also there.
  */
-static void of_mdiobus_link_mdiodev(struct mii_bus *bus,
-				    struct mdio_device *mdiodev)
+static int of_mdiobus_find_phy(struct device *dev, struct mdio_device *mdiodev,
+			       struct device_node *np)
 {
-	struct device *dev = &mdiodev->dev;
 	struct device_node *child;
 
-	if (dev->of_node || !bus->dev.of_node)
-		return;
-
-	for_each_available_child_of_node(bus->dev.of_node, child) {
+	for_each_available_child_of_node(np, child) {
 		int addr;
 
+		if (of_device_is_compatible(child, "ethernet-phy-package")) {
+			if (!of_mdiobus_find_phy(dev, mdiodev, child))
+				return 0;
+
+			continue;
+		}
+
 		addr = of_mdio_parse_addr(dev, child);
 		if (addr < 0)
 			continue;
@@ -477,9 +481,22 @@  static void of_mdiobus_link_mdiodev(struct mii_bus *bus,
 			/* The refcount on "child" is passed to the mdio
 			 * device. Do _not_ use of_node_put(child) here.
 			 */
-			return;
+			return 0;
 		}
 	}
+
+	return -ENODEV;
+}
+
+static void of_mdiobus_link_mdiodev(struct mii_bus *bus,
+				    struct mdio_device *mdiodev)
+{
+	struct device *dev = &mdiodev->dev;
+
+	if (dev->of_node || !bus->dev.of_node)
+		return;
+
+	of_mdiobus_find_phy(dev, mdiodev, bus->dev.of_node);
 }
 #else /* !IS_ENABLED(CONFIG_OF_MDIO) */
 static inline void of_mdiobus_link_mdiodev(struct mii_bus *mdio,
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e016dbfb0d27..9ff76d84dca0 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3193,6 +3193,65 @@  static int of_phy_leds(struct phy_device *phydev)
 	return 0;
 }
 
+static int of_phy_package(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct of_phandle_args phy_phandle;
+	struct device_node *package_node;
+	int i, global_phys_num, ret;
+	int *global_phy_addrs;
+
+	if (!node)
+		return 0;
+
+	package_node = of_get_parent(node);
+	if (!package_node)
+		return 0;
+
+	if (!of_device_is_compatible(package_node, "ethernet-phy-package"))
+		return 0;
+
+	ret = of_count_phandle_with_args(package_node, "global-phys", NULL);
+	if (ret < 0)
+		return 0;
+	global_phys_num = ret;
+
+	if (global_phys_num != phydev->drv->phy_package_global_phy_num)
+		return -EINVAL;
+
+	global_phy_addrs = kmalloc_array(global_phys_num, sizeof(*global_phy_addrs),
+					 GFP_KERNEL);
+	if (!global_phy_addrs)
+		return -ENOMEM;
+
+	for (i = 0; i < global_phys_num; i++) {
+		int addr;
+
+		ret = of_parse_phandle_with_args(package_node, "global-phys",
+						 NULL, i, &phy_phandle);
+		if (ret)
+			goto exit;
+
+		ret = of_property_read_u32(phy_phandle.np, "reg", &addr);
+		if (ret)
+			goto exit;
+
+		global_phy_addrs[i] = addr;
+	}
+
+	ret = devm_phy_package_join(&phydev->mdio.dev, phydev, global_phy_addrs,
+				    global_phys_num, 0);
+	if (ret)
+		goto exit;
+
+	phydev->shared->np = package_node;
+
+exit:
+	kfree(global_phy_addrs);
+
+	return ret;
+}
+
 /**
  * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
  * @fwnode: pointer to the mdio_device's fwnode
@@ -3301,6 +3360,11 @@  static int phy_probe(struct device *dev)
 	if (phydrv->flags & PHY_IS_INTERNAL)
 		phydev->is_internal = true;
 
+	/* Parse DT to detect PHY package and join them */
+	err = of_phy_package(phydev);
+	if (err)
+		goto out;
+
 	/* Deassert the reset signal */
 	phy_device_reset(phydev, 0);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c2bb3f0b9dda..5bf90c49e5bd 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -339,6 +339,8 @@  struct mdio_bus_stats {
  * phy_package_leave().
  */
 struct phy_package_shared {
+	/* With PHY package defined in DT this points to the PHY package node */
+	struct device_node *np;
 	/* addrs list pointer */
 	/* note that this pointer is shared between different phydevs.
 	 * It is allocated and freed automatically by phy_package_join() and
@@ -888,6 +890,8 @@  struct phy_led {
  * @flags: A bitfield defining certain other features this PHY
  *   supports (like interrupts)
  * @driver_data: Static driver data
+ * @phy_package_global_phy_num: Num of the required global phy
+ *   for PHY package global configuration.
  *
  * All functions are optional. If config_aneg or read_status
  * are not implemented, the phy core uses the genphy versions.
@@ -905,6 +909,7 @@  struct phy_driver {
 	const unsigned long * const features;
 	u32 flags;
 	const void *driver_data;
+	unsigned int phy_package_global_phy_num;
 
 	/**
 	 * @soft_reset: Called to issue a PHY software reset