| Message ID | 20231126015346.25208-3-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: Support DT PHY package | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
> +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. >
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c index 64ebcb6d235c..c98e9f7fa3d4 100644 --- a/drivers/net/mdio/of_mdio.c +++ b/drivers/net/mdio/of_mdio.c @@ -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; } diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 25dcaa49ab8b..05f2e8e01a03 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -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, diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 823b25bb3e3e..f416f7434697 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -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); diff --git a/include/linux/phy.h b/include/linux/phy.h index 342f750e8a30..80a4adaeb817 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -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;
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(-)