Re: [PATCH net-next 9/9] net: stmmac: qcom-ethqos: add Shikra EMAC support

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 3 · Low: 0

- [Medium] shikra_data (drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-
  ethqos.c:337-359) is the only ETHQOS platform descriptor without…
- [Medium] Shikra is the first compatible with a nonzero num_noc_clks, so
  qcom_ethqos_probe() now calls qcom_ethqos_init_noc_clks(), which ends…
- [Medium] The PTP clock rate (`clk_ptp_rate`) is captured from the
  `stmmaceth` clock before its rate is updated to 120MHz, leading to…

--

net: stmmac: qcom-ethqos: add Shikra EMAC support

Adds a qcom,shikra-ethqos compatible backed by a new shikra_data
descriptor.  The descriptor enables three NOC clocks (axi-noc,
pcie-tile-axi-noc, stmmaceth) at 120 MHz and sets a 36-bit DMA address
width.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index a31541506a4c9..f96680456472d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -328,6 +328,36 @@ static const struct ethqos_emac_driver_data emac_v4_0_0_data = {
>  	},
>  };
>  
> +static const struct ethqos_noc_clk_cfg shikra_noc_clks[] = {
> +	{ "axi-noc", 120000000 },
> +	{ "pcie-tile-axi-noc", 120000000 },
> +	{ "stmmaceth", 120000000 },
> +};
> +
> +static const struct ethqos_emac_driver_data shikra_data = {
> +	.dma_addr_width = 36,
> +	.has_emac_ge_3 = true,

[Severity: Medium]
Note that this list programs the main MAC clock ("stmmaceth") to
120 MHz, and shikra_data is the first descriptor to do so.  That
interacts badly with how the PTP reference rate is captured earlier in
probe.

qcom_ethqos_probe() calls devm_stmmac_probe_config_dt() before any of
the clock rates are touched, and stmmac_probe_config_dt() has the
fall-back path:

	plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref");
	if (IS_ERR(plat->clk_ptp_ref)) {
		plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
		plat->clk_ptp_ref = NULL;
		dev_info(&pdev->dev, "PTP uses main clock\n");

so plat_dat->clk_ptp_rate is a snapshot of whatever rate the bootloader
left on stmmaceth.  Only afterwards do qcom_ethqos_init_noc_clks() and
ethqos_clks_config() move that same clock to the 120 MHz requested
above.  And ethqos_ptp_clk_freq_config() cannot repair the value,
because it bails out first thing when the fall-back was taken:

	if (!plat_dat->clk_ptp_ref)
		return;

Does that leave stmmac using a stale clk_ptp_rate for the sub-second
increment and addend programming, i.e. PTP timestamps that advance at
the wrong rate?  If the boot rate happens to be 0, the check in
stmmac_init_tstamp_counter() rejects it outright ("Invalid PTP clock
rate") and hwtstamping fails.

Admittedly this needs a DT that omits ptp_ref, which the
qcom,shikra-ethqos binding does not allow, so the window is narrow.
Still, since the core explicitly supports the fall-back, it would be
good to know the intent here: is ptp_ref mandatory for Shikra by
construction, or should the stmmaceth rate be established before
devm_stmmac_probe_config_dt() runs (or clk_ptp_rate re-read after the
rate change) so the two cannot disagree?

[Severity: Medium]
Is the missing .rgmii_por / .num_rgmii_por pair intentional here?  This is
the only descriptor in the file without a POR table; emac_v2_1_0_data,
emac_v2_3_0_data, emac_v3_0_0_data and emac_v4_0_0_data all supply a
six-entry table.

Since .link_clk_name is also unset, qcom_ethqos_probe() asks for the clock
named "rgmii":

	ethqos->link_clk = devm_clk_get(dev, data->link_clk_name ?: "rgmii");

so this instance takes the ethqos_fix_mac_speed_rgmii() path, where the
POR restore loop runs zero times:

	/* Reset to POR values and enable clk */
	for (i = 0; i < ethqos->num_rgmii_por; i++)
		rgmii_writel(ethqos, ethqos->rgmii_por[i].value,
			     ethqos->rgmii_por[i].offset);

With num_rgmii_por == 0, RGMII_IO_MACRO_CONFIG, RGMII_IO_MACRO_CONFIG2,
SDCC_HC_REG_DLL_CONFIG(2), SDCC_HC_REG_DDR_CONFIG and SDCC_USR_CTL are
never returned to a known baseline before the speed-specific
read-modify-write sequence runs.

Can that leave a stale delay setting across a speed change?  In
ethqos_rgmii_macro_init(), the non-1000 Mbps branch does:

	rgmii_setmask(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN,
		      SDCC_HC_REG_DDR_CONFIG);

while the SPEED_1000 branch only programs PRG_RCLK_DLY and sets
PRG_DLY_EN.  Neither branch clears the other branch's enable bit; on the
existing ge_3 platforms the POR write (DDR_CONFIG = 0x80040800, with
EXT_PRG_RCLK_DLY_EN clear) is what clears it.  Does a 10/100 to 1000 Mbps
renegotiation on Shikra therefore end up with the external RX clock delay
path enabled together with the 1 G programmed delay?  The first link-up
would likewise be configured on top of whatever the bootloader left in the
IO macro and DLL.

Alternatively, if Shikra is not an RGMII part, then the "rgmii" link clock
name and the absent .needs_sgmii_loopback look inconsistent with
emac_v4_0_0_data.  The commit message only says "similar to previous
platforms", which does not say which of the two it is.

> +	.noc_clk_cfg = shikra_noc_clks,
> +	.num_noc_clks = ARRAY_SIZE(shikra_noc_clks),

[Severity: Medium]
This is the first descriptor with a nonzero num_noc_clks, so it is also the
first to activate the NOC/OPP voting path in qcom_ethqos_probe().  Can that
leave a performance state vote behind on a probe failure?

qcom_ethqos_init_noc_clks() ends with:

	ret = dev_pm_opp_set_rate(dev, data->noc_clk_cfg[0].rate);
	if (ret)
		return dev_err_probe(dev, ret, "Failed to set initial NOC OPP rate\n");

which programs axi-noc and, through the required-opps, raises the device's
genpd performance state.  The paired teardown is only installed later:

	ethqos->link_clk = devm_clk_get(dev, 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");

	ret = ethqos_clks_config(ethqos, true);
	if (ret)
		return ret;

	ret = devm_add_action_or_reset(dev, ethqos_clks_disable, ethqos);

So if devm_clk_get() returns -EPROBE_DEFER (rgmii clock provider not yet
registered) or -ENOENT on an incomplete DT, probe returns before
ethqos_clks_disable is registered and dev_pm_opp_set_rate(dev, 0) is never
issued.

Does the devm cleanup undo the vote?  _opp_table_kref_release() in
drivers/opp/core.c drops current_opp, clears the OF table and puts the
clock, but performs no OPP transition:

	if (opp_table->current_opp)
		dev_pm_opp_put(opp_table->current_opp);
	_of_clear_opp_table(opp_table);
	/* Release automatically acquired single clk */
	if (!IS_ERR(opp_table->clk))
		clk_put(opp_table->clk);

And on the genpd side, genpd_dev_pm_detach() only drops the attach-time
default pstate:

	/* Drop the default performance state */
	if (dev_gpd_data(dev)->default_pstate) {
		dev_pm_genpd_set_performance_state(dev, 0);
		dev_gpd_data(dev)->default_pstate = 0;
	}

which is 0 here because the required-opps live on the OPP table entries,
and genpd_remove_device() does not re-evaluate genpd->performance_state.
Does that mean the elevated VDD_CX and 120 MHz axi-noc vote survives with
no driver bound?

A narrower variant is the dev_pm_opp_set_rate() failure inside
ethqos_clks_config(true), which returns the error without lowering the vote
taken in qcom_ethqos_init_noc_clks().

Would it work to register the cleanup action (or call
dev_pm_opp_set_rate(dev, 0)) immediately after the initial vote, or to move
the initial vote into the existing ethqos_clks_config() sequence where
failures already lower it?

> +	.rgmii_config_loopback_en = false,
> +	.dwmac4_addrs = {

[ ... ]

> +	},
> +};
> +

[ ... ]

> @@ -957,6 +987,7 @@ static const struct of_device_id qcom_ethqos_match[] = {
>  	{ .compatible = "qcom,qcs404-ethqos", .data = &emac_v2_3_0_data},
>  	{ .compatible = "qcom,sa8775p-ethqos", .data = &emac_v4_0_0_data},
>  	{ .compatible = "qcom,sc8280xp-ethqos", .data = &emac_v3_0_0_data},
> +	{ .compatible = "qcom,shikra-ethqos", .data = &shikra_data},
>  	{ .compatible = "qcom,sm8150-ethqos", .data = &emac_v2_1_0_data},
>  	{ }
>  };

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035%40oss.qualcomm.com