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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 0

- [High] shikra_data (drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-
  ethqos.c:337) provides no .rgmii_por/.num_rgmii_por table, while the…
- [Medium] Adding the core-owned "stmmaceth" clock to shikra_noc_clks[]
  decouples that clock's programmed rate and enable state from the…

--

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

Adds a qcom,shikra-ethqos compatible with a new shikra_data descriptor that
declares three NOC clocks (axi-noc, pcie-tile-axi-noc, stmmaceth) at
120 MHz and a 36-bit DMA address width.  A couple of questions below about
the new descriptor.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 8871d28594116..c474f1e5a0437 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 },

[Severity: Medium]
Is it safe to put the core-owned "stmmaceth" clock into this table, given
that the 120 MHz rate and the OPP/VDD_CX vote then come from different
owners?

On the resume path the core enables the clock before this driver raises the
vote:

stmmac_platform.c:stmmac_bus_clks_config() {
	...
	ret = clk_prepare_enable(plat_dat->stmmac_clk);
	...
	if (plat_dat->clks_config) {
		ret = plat_dat->clks_config(plat_dat->bsp_priv, enabled);
	...
}

ethqos_clks_config(true) is the only place that calls
dev_pm_opp_set_rate(dev, ethqos->noc_clk_rates[0]), so for that window
stmmaceth is running at the previously programmed 120 MHz with the vote at
zero.  The same inversion happens at probe, where
devm_stmmac_probe_config_dt() enables stmmaceth before
qcom_ethqos_init_noc_clks() installs the OPP.

At detach the devm actions unwind in the opposite order to how they were
registered, so ethqos_clks_disable() runs before
devm_stmmac_remove_config_dt():

	} else {
		if (ethqos->num_noc_clks)
			clk_bulk_disable_unprepare(ethqos->num_noc_clks,
						   ethqos->noc_clks);
		clk_disable_unprepare(ethqos->link_clk);
		if (ethqos->num_noc_clks)
			dev_pm_opp_set_rate(&ethqos->pdev->dev, 0);
	}

The bulk disable only drops the glue's extra reference, the core still holds
one, so does this withdraw the performance-state vote while stmmaceth is
still running at 120 MHz?

The mid-sequence failure paths in ethqos_clks_config() look similar: if
clk_set_rate() fails for a later index, or clk_bulk_prepare_enable() fails,
only dev_pm_opp_set_rate(dev, 0) is issued and the rates already raised are
not rolled back.  The original stmmaceth rate is never recorded or restored
either.

> +};
> +
> +static const struct ethqos_emac_driver_data shikra_data = {
> +	.dma_addr_width = 36,
> +	.has_emac_ge_3 = true,
> +	.noc_clk_cfg = shikra_noc_clks,
> +	.num_noc_clks = ARRAY_SIZE(shikra_noc_clks),
> +	.rgmii_config_loopback_en = false,

[Severity: High]
Was a .rgmii_por / .num_rgmii_por table meant to be part of this patch?

All the other descriptors (emac_v2_1_0_data, emac_v2_3_0_data,
emac_v3_0_0_data, emac_v4_0_0_data) supply one, and shikra_data leaves
.link_clk_name unset, so qcom_ethqos_probe() ends up doing
devm_clk_get(dev, "rgmii"):

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

With num_rgmii_por == 0 the reset loop at the top of
ethqos_fix_mac_speed_rgmii() does nothing:

	/* 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);

so RGMII_IO_MACRO_CONFIG, SDCC_HC_REG_DLL_CONFIG, SDCC_HC_REG_DDR_CONFIG and
RGMII_IO_MACRO_CONFIG2 are never brought back to a baseline before the
speed-specific programming, which is read-modify-write.

Can that leave stale bits across a speed change?  For has_emac_ge_3 (true
here) at 10/100M, ethqos_rgmii_macro_init() arms the extended RX-clock delay
path:

		rgmii_setmask(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY,
			      SDCC_HC_REG_DDR_CONFIG);

		rgmii_setmask(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN,
			      SDCC_HC_REG_DDR_CONFIG);
	} else {

and the 1000M branch only programs the other path:

		rgmii_updatel(ethqos, SDCC_DDR_CONFIG_PRG_RCLK_DLY,
			      FIELD_PREP(SDCC_DDR_CONFIG_PRG_RCLK_DLY,
					 prg_rclk_dly), SDCC_HC_REG_DDR_CONFIG);

		rgmii_setmask(ethqos, SDCC_DDR_CONFIG_PRG_DLY_EN,
			      SDCC_HC_REG_DDR_CONFIG);

EXT_PRG_RCLK_DLY_EN is never cleared, so after 100M -> 1000M both delay
paths are enabled.  On the other platforms the POR write of 0x80040800 to
SDCC_HC_REG_DDR_CONFIG clears the extended path first.

Two related fields also look like they depend on the POR table:

- SDCC_DDR_CONFIG_TCXO_CYCLES_CNT is written nowhere else in the driver, yet
  the 1G delay values are derived from "TCXO period * TCXO_CYCLES_CNT /
  (2 * RX delay ns)", which assumes the POR value of 4.
- SDCC_HC_REG_DLL_CONFIG is never restored to its 0x2004642c baseline, and
  CDR_EN / CDR_EXT_EN set by ethqos_dll_configure() at 1G are not cleared
  when dropping to 10/100M, so residual values feed the DLL_RST/PDN toggle
  and the SDC4_STATUS_DLL_LOCK poll.
- RGMII_CONFIG_MAX_SPD_PRG_2 / MAX_SPD_PRG_9 are set for 100M/10M and not
  cleared for 1000M.

If Shikra is in fact not an RGMII part, then the descriptor and the binding
added in the same series look inconsistent the other way: the binding pins
the fourth clock to const "rgmii" and the descriptor sets the RGMII-only
.rgmii_config_loopback_en field.  Which of the two is intended?

> +	.dwmac4_addrs = {

[ ... ]

> +};
> +

[ ... ]

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