Re: [PATCH v3 01/15] net: ti: am65-cpsw-nuss: Define bind method for CPSW driver

[Roger Quadros <rogerq@kernel.org>]

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-vankar@ti.com>
> ---
> 
> Link to v2:
> https://lore.kernel.org/r/20250219104831.2315464-3-c-vankar@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