[net-next,RFC,v3,2/8] net: phy: add initial support for PHY package in DT
Commit Message
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 node name with the
prefix "ethernet-phy-package" and compatible set to
"ethernet-phy-package".
With this defined, the generic PHY probe will join each PHY in this
dedicated node to the package.
PHY package will be joined based on the reg defined in the
ethernet-phy-package node.
mdio_bus.c and of_mdio.c is updated to now support and parse also
PHY package subnote by checking if the node compatible is
"ethernet-phy-package".
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
drivers/net/mdio/of_mdio.c | 68 +++++++++++++++++++++++++-----------
drivers/net/phy/mdio_bus.c | 35 ++++++++++++++-----
drivers/net/phy/phy_device.c | 35 +++++++++++++++++++
include/linux/phy.h | 3 ++
4 files changed, 112 insertions(+), 29 deletions(-)
Comments
> +static int of_phy_package(struct phy_device *phydev)
> +{
> + struct device_node *node = phydev->mdio.dev.of_node;
> + struct device_node *package_node;
> + u32 base_addr;
> + int ret;
> +
> + 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;
> +
> + if (of_property_read_u32(package_node, "reg", &base_addr))
> + return -EINVAL;
> +
> + ret = devm_phy_package_join(&phydev->mdio.dev, phydev,
> + base_addr, 0);
No don't do this. It is just going to lead to errors. The PHY driver
knows how many PHYs are in the package. So it can figure out what the
base address is and create the package. It can add each PHY as they
probe. That cannot go wrong.
If you create the package based on DT you have to validate that the DT
is correct. You need the same information, the base address, how many
packages are in the PHY, etc. So DT gains your nothing except more
potential to get it wrong.
Please use DT just for properties for the package, nothing else.
Andrew
On Tue, Nov 28, 2023 at 01:39:02AM +0100, Andrew Lunn wrote:
> > +static int of_phy_package(struct phy_device *phydev)
> > +{
> > + struct device_node *node = phydev->mdio.dev.of_node;
> > + struct device_node *package_node;
> > + u32 base_addr;
> > + int ret;
> > +
> > + 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;
> > +
> > + if (of_property_read_u32(package_node, "reg", &base_addr))
> > + return -EINVAL;
> > +
> > + ret = devm_phy_package_join(&phydev->mdio.dev, phydev,
> > + base_addr, 0);
>
> No don't do this. It is just going to lead to errors. The PHY driver
> knows how many PHYs are in the package. So it can figure out what the
> base address is and create the package. It can add each PHY as they
> probe. That cannot go wrong.
>
No it can't and we experiment this with a funny implementation on the
QSDK. Maybe I'm confused?
Example on QSDK they were all based on a value first_phy_addr. This was
filled with the first phy addr found (for the package).
All OEM followed a template with declaring all sort of bloat and they
probably have no idea what they are actually putting in DT. We reverse
all the properties and we gave a meaning to all the values...
We quikly notice that the parsing was very fragile and on some strange
device (an example a Xiaomi 36000) the first PHY for the package was
actually not attached to anything. Resulting in all this logic of
"first_phy_addr" failing as by removing the first PHY, the value was set
to a wrong addr resulting in all sort of problems.
This changed a lot (the original series used a more robust way with
phandle, but it was asked to use base_addr and use offset in the PHY
addr, less robust still OK)
If we revert to PHY driver making the PHY package then we lose all kind
of ""automation"" of having a correct base addr. In PHY driver we would
have to manually parse with parent (as we do here) and check the value?
Why not do the thing directly on PHY core?
By making the PHY driver creating the package, we are back on all kind
of bloat in the PHY driver doing the same thing (parsing package nodes,
probe_once check, config_once check) instead of handling them directly
in PHY core.
Also just to point out, the way the current PHY driver derive the base
addr is problematic. All of them use special regs to find the base one
but I can totally see a PHY driver in the future assuming the first PHY
being the first in the package to be probed, set the base addr on the
first PHY and also assuming that it's always define in DT.
If we really don't want the OF parsing in PHY core and autojoin them,
is at least OK to introduce an helper to get the base addr from a PHY
package node structure? (to at least try to minimaze the bloat that PHY
driver will have?)
Also with the OF support dropped, I guess also the added API in this
series has to be dropped. (as they would be called after the first PHY
probe and that is problematic if for some reason only one PHY of the
package is declared in DT) (an alternative might be moving the
probe_once after the PHY is probed as we expect the phy_package_join
call done in the PHY probe call)
> If you create the package based on DT you have to validate that the DT
> is correct. You need the same information, the base address, how many
> packages are in the PHY, etc. So DT gains your nothing except more
> potential to get it wrong.
>
For the sake of package join, only the reg has to be validated and the
validation is just if addrs makes sense. Everything else can be done by
PHY package probe once.
> Please use DT just for properties for the package, nothing else.
>
@@ -139,6 +139,47 @@ 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 = 0;
+
+ /* 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)
+ goto exit;
+
+ 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)
+ goto exit;
+ }
+
+ return 0;
+exit:
+ of_node_put(child);
+ return rc;
+}
+
/**
* __of_mdiobus_register - Register mii_bus and create PHYs from the device tree
* @mdio: pointer to mii_bus structure
@@ -180,25 +221,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;
@@ -227,15 +252,16 @@ int __of_mdiobus_register(struct mii_bus *mdio, struct device_node *np,
if (!rc)
break;
if (rc != -ENODEV)
- goto unregister;
+ goto put_unregister;
}
}
}
return 0;
-unregister:
+put_unregister:
of_node_put(child);
+unregister:
mdiobus_unregister(mdio);
return rc;
}
@@ -455,19 +455,25 @@ 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)) {
+ of_node_put(child);
+ return 0;
+ }
+
+ continue;
+ }
+
addr = of_mdio_parse_addr(dev, child);
if (addr < 0)
continue;
@@ -477,9 +483,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,
@@ -3174,6 +3174,36 @@ 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 device_node *package_node;
+ u32 base_addr;
+ int ret;
+
+ 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;
+
+ if (of_property_read_u32(package_node, "reg", &base_addr))
+ return -EINVAL;
+
+ ret = devm_phy_package_join(&phydev->mdio.dev, phydev,
+ base_addr, 0);
+ if (ret)
+ return ret;
+
+ phydev->shared->np = package_node;
+
+ return ret;
+}
+
/**
* fwnode_mdio_find_device - Given a fwnode, find the mdio_device
* @fwnode: pointer to the mdio_device's fwnode
@@ -3282,6 +3312,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);
@@ -329,6 +329,7 @@ struct mdio_bus_stats {
* struct phy_package_shared - Shared information in PHY packages
* @base_addr: Base PHY address of PHY package used to combine PHYs
* in one package and for offset calculation of phy_package_read/write
+ * @np: Pointer to the Device Node if PHY package defined in DT
* @refcnt: Number of PHYs connected to this shared data
* @flags: Initialization of PHY package
* @priv_size: Size of the shared private data @priv
@@ -340,6 +341,8 @@ struct mdio_bus_stats {
*/
struct phy_package_shared {
int base_addr;
+ /* With PHY package defined in DT this points to the PHY package node */
+ struct device_node *np;
refcount_t refcnt;
unsigned long flags;
size_t priv_size;