Hello Oleksij,
On Tue, 5 Nov 2024 10:09:44 +0100
Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
> 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
