Hi, On 25/02/2025 13:48, Chintan Vankar wrote: > CPSW driver is defined as UCLASS_MISC driver which needs to be probed > explicitly. Define bind method for CPSW driver to scan and bind > ethernet-ports with UCLASS_ETH driver which will eventually probe CPSW > driver and avoids probing CPSW driver explicitly.
Thank you for doing this. Overall it looks good. Just some cosmetic changes suggested below. > > Signed-off-by: Chintan Vankar <c-van...@ti.com> > --- > > Link to v2: > https://lore.kernel.org/r/20250219104831.2315464-3-c-van...@ti.com/ > > Changes from v2 to v3: > - Removed if condition not required in CPSW driver as suggested by > Alexander Sverdlin. > > drivers/net/ti/am65-cpsw-nuss.c | 120 ++++++++++++++++++-------------- > 1 file changed, 67 insertions(+), 53 deletions(-) > > diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c > index c70b42f6bcc..10513cf92f2 100644 > --- a/drivers/net/ti/am65-cpsw-nuss.c > +++ b/drivers/net/ti/am65-cpsw-nuss.c > @@ -667,6 +667,59 @@ static int am65_cpsw_ofdata_parse_phy(struct udevice > *dev) > return 0; > } > > +static int am65_cpsw_probe_nuss(struct udevice *dev) > +{ > + struct am65_cpsw_common *cpsw_common = dev_get_priv(dev); > + int ret, i; > + > + cpsw_common->dev = dev; > + cpsw_common->ss_base = dev_read_addr(dev); > + if (cpsw_common->ss_base == FDT_ADDR_T_NONE) > + return -EINVAL; > + > + ret = power_domain_get_by_index(dev, &cpsw_common->pwrdmn, 0); > + if (ret) { > + dev_err(dev, "failed to get pwrdmn: %d\n", ret); > + return ret; > + } > + > + ret = clk_get_by_name(dev, "fck", &cpsw_common->fclk); > + if (ret) { > + power_domain_free(&cpsw_common->pwrdmn); > + dev_err(dev, "failed to get clock %d\n", ret); > + return ret; > + } > + > + cpsw_common->cpsw_base = cpsw_common->ss_base + AM65_CPSW_CPSW_NU_BASE; > + cpsw_common->ale_base = cpsw_common->cpsw_base + > + AM65_CPSW_CPSW_NU_ALE_BASE; > + > + for (i = 0; i < AM65_CPSW_CPSWNU_MAX_PORTS; i++) { > + struct am65_cpsw_port *port = &cpsw_common->ports[i]; > + > + port->port_base = cpsw_common->cpsw_base + > + AM65_CPSW_CPSW_NU_PORTS_OFFSET + > + (i * AM65_CPSW_CPSW_NU_PORTS_OFFSET); > + port->port_sgmii_base = cpsw_common->ss_base + > + (i * AM65_CPSW_SGMII_BASE); > + port->macsl_base = port->port_base + > + AM65_CPSW_CPSW_NU_PORT_MACSL_OFFSET; > + } > + > + cpsw_common->bus_freq = > + dev_read_u32_default(dev, "bus_freq", > + AM65_CPSW_MDIO_BUS_FREQ_DEF); > + > + dev_info(dev, "K3 CPSW: nuss_ver: 0x%08X cpsw_ver: 0x%08X ale_ver: > 0x%08X Ports:%u\n", > + readl(cpsw_common->ss_base), > + readl(cpsw_common->cpsw_base), > + readl(cpsw_common->ale_base), > + cpsw_common->port_num); > + > + power_domain_free(&cpsw_common->pwrdmn); > + return ret; > +} > + Instead of moving the am65_cpsw_probe_nuss() please leave it where it was and add the new am65_cpsw_nuss_bind() after it. This way it will show only what really changed in diff and is easier to review. > static int am65_cpsw_port_probe(struct udevice *dev) > { > struct am65_cpsw_priv *priv = dev_get_priv(dev); > @@ -697,45 +750,30 @@ out: > return ret; > } > > -static int am65_cpsw_probe_nuss(struct udevice *dev) > +static int am65_cpsw_nuss_bind(struct udevice *dev) > { > struct am65_cpsw_common *cpsw_common = dev_get_priv(dev); > - ofnode ports_np, node; > - int ret, i; > + struct uclass_driver *drv; > struct udevice *port_dev; > + ofnode ports_np, node; > + int ret; > > - cpsw_common->dev = dev; > - cpsw_common->ss_base = dev_read_addr(dev); > - if (cpsw_common->ss_base == FDT_ADDR_T_NONE) > - return -EINVAL; > - > - ret = power_domain_get_by_index(dev, &cpsw_common->pwrdmn, 0); > - if (ret) { > - dev_err(dev, "failed to get pwrdmn: %d\n", ret); > - return ret; > - } > - > - ret = clk_get_by_name(dev, "fck", &cpsw_common->fclk); > - if (ret) { > - power_domain_free(&cpsw_common->pwrdmn); > - dev_err(dev, "failed to get clock %d\n", ret); > - return ret; > + drv = lists_uclass_lookup(UCLASS_ETH); > + if (!drv) { > + puts("Cannot find eth driver"); > + return -ENOENT; I'm not sure why this check is required. drv is not used anywhere. > } > > - cpsw_common->cpsw_base = cpsw_common->ss_base + AM65_CPSW_CPSW_NU_BASE; > - cpsw_common->ale_base = cpsw_common->cpsw_base + > - AM65_CPSW_CPSW_NU_ALE_BASE; > - > + cpsw_common->dev = dev; > ports_np = dev_read_subnode(dev, "ethernet-ports"); > if (!ofnode_valid(ports_np)) { > - ret = -ENOENT; > - goto out; > + return -ENOENT; > } > > ofnode_for_each_subnode(node, ports_np) { > const char *node_name; > - u32 port_id; > bool disabled; > + u32 port_id; > > node_name = ofnode_get_name(node); > > @@ -745,14 +783,13 @@ static int am65_cpsw_probe_nuss(struct udevice *dev) > if (ret) { > dev_err(dev, "%s: failed to get port_id (%d)\n", > node_name, ret); > - goto out; > + return ret; > } > > if (port_id >= AM65_CPSW_CPSWNU_MAX_PORTS) { > dev_err(dev, "%s: invalid port_id (%d)\n", > node_name, port_id); > - ret = -EINVAL; > - goto out; > + return -EINVAL; > } > cpsw_common->port_num++; > > @@ -768,30 +805,6 @@ static int am65_cpsw_probe_nuss(struct udevice *dev) > dev_err(dev, "Failed to bind to %s node\n", > ofnode_get_name(node)); > } > > - for (i = 0; i < AM65_CPSW_CPSWNU_MAX_PORTS; i++) { > - struct am65_cpsw_port *port = &cpsw_common->ports[i]; > - > - port->port_base = cpsw_common->cpsw_base + > - AM65_CPSW_CPSW_NU_PORTS_OFFSET + > - (i * AM65_CPSW_CPSW_NU_PORTS_OFFSET); > - port->port_sgmii_base = cpsw_common->ss_base + > - (i * AM65_CPSW_SGMII_BASE); > - port->macsl_base = port->port_base + > - AM65_CPSW_CPSW_NU_PORT_MACSL_OFFSET; > - } > - > - cpsw_common->bus_freq = > - dev_read_u32_default(dev, "bus_freq", > - AM65_CPSW_MDIO_BUS_FREQ_DEF); > - > - dev_info(dev, "K3 CPSW: nuss_ver: 0x%08X cpsw_ver: 0x%08X ale_ver: > 0x%08X Ports:%u\n", > - readl(cpsw_common->ss_base), > - readl(cpsw_common->cpsw_base), > - readl(cpsw_common->ale_base), > - cpsw_common->port_num); > - > -out: > - power_domain_free(&cpsw_common->pwrdmn); > return ret; > } > > @@ -806,6 +819,7 @@ U_BOOT_DRIVER(am65_cpsw_nuss) = { > .name = "am65_cpsw_nuss", > .id = UCLASS_MISC, > .of_match = am65_cpsw_nuss_ids, > + .bind = am65_cpsw_nuss_bind, > .probe = am65_cpsw_probe_nuss, > .priv_auto = sizeof(struct am65_cpsw_common), > }; -- cheers, -roger
