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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: stmmac: qcom-ethqos: missing rgmii_por in shikra_data
--

commit 8e4d72d76d93b838333086304a2ba657b2b5e8e1
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

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

This patch adds support for the Qualcomm Shikra ETHQOS controller by
registering the qcom,shikra-ethqos compatible string and defining the
shikra_data driver data descriptor with its three NOC clocks and 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 9c602755532e1..a4e083a8517d0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -307,6 +307,22 @@ static const struct ethqos_emac_driver_data emac_v4_0_0_data = {
>  	.dwmac4_addrs = &ethqos_dwmac4_addrs,
>  };
>  
> +static const char * const shikra_noc_clk_names[] = {
> +	"stmmaceth",
> +	"axi-noc",
> +	"pcie-tile-axi-noc",
> +	NULL,
> +};
> +
> +static const struct ethqos_emac_driver_data shikra_data = {
> +	.dma_addr_width = 36,
> +	.has_emac_ge_3 = true,
> +	.noc_clk_names = shikra_noc_clk_names,
> +	.num_noc_clks = ARRAY_SIZE(shikra_noc_clk_names) - 1,
> +	.rgmii_config_loopback_en = false,
> +	.dwmac4_addrs = &ethqos_dwmac4_addrs,
> +};

[Severity: Medium]
Should shikra_data also populate .rgmii_por and .num_rgmii_por like all the
other struct ethqos_emac_driver_data descriptors (emac_v2_3_0_data,
emac_v2_1_0_data, emac_v3_0_0_data, and emac_v4_0_0_data) in
drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c?

With .rgmii_por and .num_rgmii_por omitted, qcom_ethqos_probe() leaves
ethqos->rgmii_por as NULL and ethqos->num_rgmii_por as 0 while registering
ethqos_fix_mac_speed_rgmii() as plat_dat->fix_mac_speed. When phylink brings
up or changes the RGMII link speed, the POR reset loop in
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);

Subsequent configuration in ethqos_fix_mac_speed_rgmii() and
ethqos_rgmii_id_macro_init() relies on read-modify-write updates assuming
RGMII_IO_MACRO_CONFIG, SDCC_HC_REG_DLL_CONFIG, SDCC_HC_REG_DDR_CONFIG,
SDCC_HC_REG_DLL_CONFIG2, SDCC_USR_CTL, and RGMII_IO_MACRO_CONFIG2 were first
reset to their baseline POR defaults.

Without this reset, non-POR state left by a bootloader or kexec is never
cleared in any RGMII mode (including PHY_INTERFACE_MODE_RGMII_ID, where
ethqos_rgmii_id_macro_init() relies on POR defaults for fields such as
RGMII_CONFIG_INTF_SEL, RGMII_CONFIG_MAX_SPD_PRG_2, and
RGMII_CONFIG_MAX_SPD_PRG_9). Additionally, in PHY_INTERFACE_MODE_RGMII,
PHY_INTERFACE_MODE_RGMII_RXID, and PHY_INTERFACE_MODE_RGMII_TXID modes, speed
transitions between 1000M and 100M/10M will leave stale bits set from the
previous speed (such as SDCC_DLL_CONFIG_DLL_EN, SDCC_DLL_CONFIG_CDR_EN,
SDCC_DLL_CONFIG_CDR_EXT_EN, and SDCC_DDR_CONFIG_PRG_RCLK_DLY when switching
from 1000M to 100M/10M, or SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_EN,
SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY, and SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE
when switching from 100M/10M to 1000M).

[ ... ]

> @@ -947,6 +963,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://sashiko.dev/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f@oss.qualcomm.com?part=9