Re: [PATCH net-next v13 09/10] net: stmmac: qcom-ethqos: factor out linux-level setup into a separate function
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-07-24 22:46:07
Also in:
imx, linux-amlogic, linux-arm-kernel, linux-arm-msm, linux-devicetree, linux-mips, linux-renesas-soc, linux-riscv, linux-rockchip, linux-sunxi, lkml
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.
quoted hunk ↗ jump to hunk
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? [ ... ]
quoted hunk ↗ jump to hunk
@@ -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?