| Message ID | 20241105090944.671379-7-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Side MDIO Support for LAN937x Switches | expand |
Hello Oleksij, On Tue, 5 Nov 2024 10:09:44 +0100 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > Introduce ksz_parse_dt_phy_config() to validate and parse PHY > configuration from the device tree for KSZ switches. This function > ensures proper setup of internal PHYs by checking `phy-handle` > properties, verifying expected PHY IDs, and handling parent node > mismatches. Sets the PHY mask on the MII bus if validation is > successful. Returns -EINVAL on configuration errors. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/dsa/microchip/ksz_common.c | 80 ++++++++++++++++++++++++-- > 1 file changed, 74 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > index 3909b55857430..cd1a466504180 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -2373,6 +2373,77 @@ static void ksz_irq_phy_free(struct ksz_device *dev) > irq_dispose_mapping(ds->user_mii_bus->irq[phy]); > } > > +/** > + * ksz_parse_dt_phy_config - Parse and validate PHY configuration from DT > + * @dev: pointer to the KSZ device structure > + * @bus: pointer to the MII bus structure > + * @mdio_np: pointer to the MDIO node in the device tree > + * > + * This function parses and validates PHY configurations for each user port > + * defined in the device tree for a KSZ switch device. It verifies that the > + * `phy-handle` properties are correctly set and that the internal PHYs match > + * expected IDs and parent nodes. Sets up the PHY mask in the MII bus if all > + * validations pass. Logs error messages for any mismatches or missing data. > + * > + * Return: 0 on success, or a negative error code on failure. > + */ > +static int ksz_parse_dt_phy_config(struct ksz_device *dev, struct mii_bus *bus, > + struct device_node *mdio_np) > +{ > + struct device_node *phy_node, *phy_parent_node; > + bool phys_are_valid = true; > + struct dsa_port *dp; > + u32 phy_id; > + int ret; > + > + dsa_switch_for_each_user_port(dp, dev->ds) { > + if (!dev->info->internal_phy[dp->index]) > + continue; > + > + phy_node = of_parse_phandle(dp->dn, "phy-handle", 0); > + if (!phy_node) { > + dev_err(dev->dev, "failed to parse phy-handle for port %d.\n", > + dp->index); > + phys_are_valid = false; > + continue; > + } > + > + phy_parent_node = of_get_parent(phy_node); > + if (!phy_parent_node) { > + dev_err(dev->dev, "failed to get PHY-parent node for port %d\n", > + dp->index); > + phys_are_valid = false; > + } else if (dev->info->internal_phy[dp->index] && > + phy_parent_node != mdio_np) { There's a check a few lines above that guarantees that at this point dev->info->internal_phy[dp->index] will always evaluate as true, so you could simplify that condition a bit :) > + dev_err(dev->dev, "PHY-parent node mismatch for port %d, expected %pOF, got %pOF\n", > + dp->index, mdio_np, phy_parent_node); > + phys_are_valid = false; > + } else { > + ret = of_property_read_u32(phy_node, "reg", &phy_id); > + if (ret < 0) { > + dev_err(dev->dev, "failed to read PHY ID for port %d. Error %d\n", > + dp->index, ret); > + phys_are_valid = false; > + } else if (phy_id != dev->phy_addr_map[dp->index]) { > + dev_err(dev->dev, "PHY ID mismatch for port %d, expected 0x%x, got 0x%x\n", > + dp->index, dev->phy_addr_map[dp->index], > + phy_id); In this context, PHY ID might be a bit misleading, as PHY ID usually refers to the identifier (OUI + model id used at probe to select the driver). May I suggest phy_addr instead ? Thanks, Maxime
Hi Maxime, On Tue, Nov 05, 2024 at 03:28:05PM +0100, Maxime Chevallier wrote: > > + dsa_switch_for_each_user_port(dp, dev->ds) { > > + if (!dev->info->internal_phy[dp->index]) > > + continue; > > + > > + phy_node = of_parse_phandle(dp->dn, "phy-handle", 0); > > + if (!phy_node) { > > + dev_err(dev->dev, "failed to parse phy-handle for port %d.\n", > > + dp->index); > > + phys_are_valid = false; > > + continue; > > + } > > + > > + phy_parent_node = of_get_parent(phy_node); > > + if (!phy_parent_node) { > > + dev_err(dev->dev, "failed to get PHY-parent node for port %d\n", > > + dp->index); > > + phys_are_valid = false; > > + } else if (dev->info->internal_phy[dp->index] && > > + phy_parent_node != mdio_np) { > > There's a check a few lines above that guarantees that at this point > dev->info->internal_phy[dp->index] will always evaluate as true, > so you could simplify that condition a bit :) good point :) > > + dev_err(dev->dev, "PHY-parent node mismatch for port %d, expected %pOF, got %pOF\n", > > + dp->index, mdio_np, phy_parent_node); > > + phys_are_valid = false; > > + } else { > > + ret = of_property_read_u32(phy_node, "reg", &phy_id); > > + if (ret < 0) { > > + dev_err(dev->dev, "failed to read PHY ID for port %d. Error %d\n", > > + dp->index, ret); > > + phys_are_valid = false; > > + } else if (phy_id != dev->phy_addr_map[dp->index]) { > > + dev_err(dev->dev, "PHY ID mismatch for port %d, expected 0x%x, got 0x%x\n", > > + dp->index, dev->phy_addr_map[dp->index], > > + phy_id); > > In this context, PHY ID might be a bit misleading, as PHY ID usually > refers to the identifier (OUI + model id used at probe to select the > driver). May I suggest phy_addr instead ? ack, will rework it.
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 3909b55857430..cd1a466504180 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -2373,6 +2373,77 @@ static void ksz_irq_phy_free(struct ksz_device *dev) irq_dispose_mapping(ds->user_mii_bus->irq[phy]); } +/** + * ksz_parse_dt_phy_config - Parse and validate PHY configuration from DT + * @dev: pointer to the KSZ device structure + * @bus: pointer to the MII bus structure + * @mdio_np: pointer to the MDIO node in the device tree + * + * This function parses and validates PHY configurations for each user port + * defined in the device tree for a KSZ switch device. It verifies that the + * `phy-handle` properties are correctly set and that the internal PHYs match + * expected IDs and parent nodes. Sets up the PHY mask in the MII bus if all + * validations pass. Logs error messages for any mismatches or missing data. + * + * Return: 0 on success, or a negative error code on failure. + */ +static int ksz_parse_dt_phy_config(struct ksz_device *dev, struct mii_bus *bus, + struct device_node *mdio_np) +{ + struct device_node *phy_node, *phy_parent_node; + bool phys_are_valid = true; + struct dsa_port *dp; + u32 phy_id; + int ret; + + dsa_switch_for_each_user_port(dp, dev->ds) { + if (!dev->info->internal_phy[dp->index]) + continue; + + phy_node = of_parse_phandle(dp->dn, "phy-handle", 0); + if (!phy_node) { + dev_err(dev->dev, "failed to parse phy-handle for port %d.\n", + dp->index); + phys_are_valid = false; + continue; + } + + phy_parent_node = of_get_parent(phy_node); + if (!phy_parent_node) { + dev_err(dev->dev, "failed to get PHY-parent node for port %d\n", + dp->index); + phys_are_valid = false; + } else if (dev->info->internal_phy[dp->index] && + phy_parent_node != mdio_np) { + dev_err(dev->dev, "PHY-parent node mismatch for port %d, expected %pOF, got %pOF\n", + dp->index, mdio_np, phy_parent_node); + phys_are_valid = false; + } else { + ret = of_property_read_u32(phy_node, "reg", &phy_id); + if (ret < 0) { + dev_err(dev->dev, "failed to read PHY ID for port %d. Error %d\n", + dp->index, ret); + phys_are_valid = false; + } else if (phy_id != dev->phy_addr_map[dp->index]) { + dev_err(dev->dev, "PHY ID mismatch for port %d, expected 0x%x, got 0x%x\n", + dp->index, dev->phy_addr_map[dp->index], + phy_id); + phys_are_valid = false; + } else { + bus->phy_mask |= BIT(phy_id); + } + } + + of_node_put(phy_node); + of_node_put(phy_parent_node); + } + + if (!phys_are_valid) + return -EINVAL; + + return 0; +} + /** * ksz_mdio_register - Register and configure the MDIO bus for the KSZ device. * @dev: Pointer to the KSZ device structure. @@ -2392,7 +2463,6 @@ static int ksz_mdio_register(struct ksz_device *dev) struct dsa_switch *ds = dev->ds; struct device_node *mdio_np; struct mii_bus *bus; - struct dsa_port *dp; int ret, i; mdio_np = of_get_child_by_name(dev->dev->of_node, "mdio"); @@ -2451,11 +2521,9 @@ static int ksz_mdio_register(struct ksz_device *dev) snprintf(bus->id, MII_BUS_ID_SIZE, "SMI-%d", ds->index); } - dsa_switch_for_each_user_port(dp, dev->ds) { - if (dev->info->internal_phy[dp->index] && - dev->phy_addr_map[dp->index] < PHY_MAX_ADDR) - bus->phy_mask |= BIT(dev->phy_addr_map[dp->index]); - } + ret = ksz_parse_dt_phy_config(dev, bus, mdio_np); + if (ret) + goto put_mdio_node; ds->phys_mii_mask = bus->phy_mask; bus->parent = ds->dev;
Introduce ksz_parse_dt_phy_config() to validate and parse PHY configuration from the device tree for KSZ switches. This function ensures proper setup of internal PHYs by checking `phy-handle` properties, verifying expected PHY IDs, and handling parent node mismatches. Sets the PHY mask on the MII bus if validation is successful. Returns -EINVAL on configuration errors. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/dsa/microchip/ksz_common.c | 80 ++++++++++++++++++++++++-- 1 file changed, 74 insertions(+), 6 deletions(-) -- 2.39.5