Re: [PATCH net-next v15 8/9] net: stmmac: qcom-ethqos: factor out linux-level setup into a separate function

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message does not describe the refactor boundary
  accurately.
- [Low] The patch deletes the comment that justified the hard-coded MDC
  divider ('The clocks are controlled by firmware, so we don't know for…

--

net: stmmac: qcom-ethqos: factor out linux-level setup into a separate function

This patch adds a setup() callback to struct ethqos_emac_driver_data. It
implements the callback as ethqos_hlos_setup() for the existing EMAC
variants. The rgmii mapping, link clock handling and several plat_dat
callbacks move from qcom_ethqos_probe() into it, ahead of support for
firmware-controlled (SCMI) EMAC variants.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index ab1bcb7eb18e2..509a5368e69a8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c

[ ... ]

> @@ -692,6 +704,58 @@ static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
>  	netdev_dbg(priv->dev, "PTP rate %lu\n", plat_dat->clk_ptp_rate);
>  }
>  
> +static int ethqos_hlos_setup(struct qcom_ethqos *ethqos,
> +			     struct plat_stmmacenet_data *plat_dat)
> +{
> +	struct platform_device *pdev = ethqos->pdev;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ethqos->rgmii_base = devm_platform_ioremap_resource_byname(pdev, "rgmii");
> +	if (IS_ERR(ethqos->rgmii_base))
> +		return dev_err_probe(dev, PTR_ERR(ethqos->rgmii_base),
> +				     "Failed to map rgmii resource\n");
> +
> +	ethqos->link_clk = devm_clk_get(dev, ethqos->data->link_clk_name ?: "rgmii");
> +	if (IS_ERR(ethqos->link_clk))
> +		return dev_err_probe(dev, PTR_ERR(ethqos->link_clk),
> +				     "Failed to get link_clk\n");
> +
> +	plat_dat->clks_config = ethqos_clks_config;
> +
> +	ret = ethqos_clks_config(ethqos, true);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, ethqos_clks_disable, ethqos);
> +	if (ret)
> +		return ret;
> +

[ ... ]

> +	case PHY_INTERFACE_MODE_2500BASEX:
> +	case PHY_INTERFACE_MODE_SGMII:
> +		plat_dat->fix_mac_speed = ethqos_fix_mac_speed_sgmii;
> +		break;
> +	default:
> +		break;
> +	}

[Severity: Low]
This isn't a bug, but is the commit message accurate about what moves into
the callback? It says the patch will:

  "move all operations not required in SCMI mode into it."

SCMI mode also needs several of the steps moved into ethqos_hlos_setup():

  - mapping the "rgmii" wrapper
  - setting plat_dat->clks_config
  - the first ethqos_clks_config(ethqos, true) call, along with its
    devm_add_action_or_reset(dev, ethqos_clks_disable, ethqos) cleanup
  - selecting ethqos_fix_mac_speed_sgmii for SGMII/2500BASEX

Later in the series, the ethqos_scmi_setup() added by "net: stmmac:
qcom-ethqos: add support for sa8255p" repeats each of these steps. Its own
comment says the wrapper loopback and functional clock bits are required.
Without them, the DMA SW-reset in dwmac4_dma_reset() never completes.

As a result, the same setup code exists in two places instead of once in
qcom_ethqos_probe().

Could the shared steps stay in qcom_ethqos_probe() or move to a common
helper, leaving only the HLOS-only parts in ethqos_hlos_setup()? If not,
could the commit message describe the split more precisely?

[ ... ]

> @@ -765,21 +815,8 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, PTR_ERR(ethqos->serdes_phy),
>  				     "Failed to get serdes phy\n");
>  
> -	ethqos_set_clk_tx_rate(ethqos, NULL, plat_dat->phy_interface,
> -			       SPEED_1000);
> -
> -	qcom_ethqos_set_sgmii_loopback(ethqos, true);
> -	ethqos_set_func_clk_en(ethqos);
> -
> -	/* The clocks are controlled by firmware, so we don't know for certain
> -	 * what clock rate is being used. Hardware documentation mentions that
> -	 * the AHB slave clock will be in the range of 50 to 100MHz, which
> -	 * equates to a MDC between 1.19 and 2.38MHz.
> -	 */
>  	plat_dat->clk_csr = STMMAC_CSR_60_100M;

[Severity: Low]
Was this comment meant to be removed?

qcom_ethqos_probe() still sets plat_dat->clk_csr = STMMAC_CSR_60_100M for
every variant, but the comment explaining the hard-coded MDC divider is
gone.

The commit message describes the change as code motion and does not
mention removing the comment. No later patch in the series restores it.

Could the comment be kept above the clk_csr assignment?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-qcom-sa8255p-emac-v15-0-e82f33720737%40oss.qualcomm.com