Re: [PATCH net-next v13 09/10] net: stmmac: qcom-ethqos: factor out linux-level setup into a separate function

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: stmmac: qcom-ethqos: factor out linux-level setup into a separate function

This refactor extends ethqos_emac_driver_data with a setup() callback,
implements it for the existing models, and moves operations not required
in SCMI mode into it, in preparation for firmware-controlled 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 43384960278b..3af1608fc21c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -5,6 +5,7 @@
>  #include <linux/of.h>
>  #include <linux/of_net.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
>  #include <linux/phy.h>
>  #include <linux/phy/phy.h>

[Low]
Is there a reason linux/pm_domain.h is added here?  Looking through
this commit, no pm_domain symbols (pm_genpd_*, dev_pm_domain_*,
PD_FLAG_*, etc.) appear to be referenced.

The header seems to belong with the follow-up patch that introduces
devm_pm_domain_attach_list() and struct dev_pm_domain_list usage.

Would it be preferable to move this include into the patch that
actually needs it, so each patch remains self-contained?

[ ... ]

> @@ -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;
>  	plat_dat->bsp_priv = ethqos;
> -	plat_dat->set_clk_tx_rate = ethqos_set_clk_tx_rate;
> -	plat_dat->dump_debug_regs = rgmii_dump;
>  	plat_dat->ptp_clk_freq_config = ethqos_ptp_clk_freq_config;
>  	plat_dat->core_type = DWMAC_CORE_GMAC4;
>  	if (data->has_emac_ge_3)

[Low]
Was the comment above plat_dat->clk_csr = STMMAC_CSR_60_100M; intended
to be dropped?

The assignment itself is preserved, but the rationale for choosing
STMMAC_CSR_60_100M (firmware-controlled AHB clock in the 50-100MHz
range yielding an MDC between 1.19 and 2.38MHz) is now gone.

The commit message describes a mechanical refactor and does not
mention removing this rationale.  Should the comment be kept next to
the assignment, or moved into ethqos_hlos_setup() where the
surrounding clock setup now lives?