Re: [PATCH net-next v3 4/9] net: stmmac: qcom-ethqos: fix RGMII_ID mode to use DLL bypass

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: stmmac: qcom-ethqos: rgmii_setmask() on multi-bit RGMII_CONFIG2_RSVD_CONFIG15
--

commit 405cfced2fa67ffbf18f898b4b34f3a6a9035507
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

net: stmmac: qcom-ethqos: fix RGMII_ID mode to use DLL bypass

This commit fixes PHY_INTERFACE_MODE_RGMII_ID handling in dwmac-qcom-ethqos by
powering down the DLL, enabling DDR bypass mode, adjusting the link clock rate
for 100M/10M speeds, and programming IO_MACRO via a new
ethqos_rgmii_id_macro_init() helper.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 44270c25d874f..53fb9de4d92c7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -67,6 +67,9 @@
>  /* SDC4_STATUS bits */
>  #define SDC4_STATUS_DLL_LOCK			BIT(7)
>  
> +/* SDCC_USR_CTL bits */
> +#define SDCC_USR_CTL_DDR_BYPASS			BIT(30)
> +
>  /* RGMII_IO_MACRO_CONFIG2 fields */
>  #define RGMII_CONFIG2_RSVD_CONFIG15		GENMASK(31, 17)
>  #define RGMII_CONFIG2_RGMII_CLK_SEL_CFG		BIT(16)

[ ... ]

> @@ -475,6 +485,42 @@ static void ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
>  		      RGMII_IO_MACRO_CONFIG);
>  }
>  
> +static void ethqos_rgmii_id_macro_init(struct qcom_ethqos *ethqos, int speed)
> +{

[ ... ]

> +	if (speed == SPEED_1000)
> +		rgmii_clrmask(ethqos, RGMII_CONFIG2_RSVD_CONFIG15, RGMII_IO_MACRO_CONFIG2);
> +	else
> +		rgmii_setmask(ethqos, RGMII_CONFIG2_RSVD_CONFIG15, RGMII_IO_MACRO_CONFIG2);

[Severity: Medium]
Should this else branch be setting all 15 bits of RGMII_CONFIG2_RSVD_CONFIG15
in RGMII_IO_MACRO_CONFIG2 when configuring 100M or 10M speeds in
PHY_INTERFACE_MODE_RGMII_ID mode?

RGMII_CONFIG2_RSVD_CONFIG15 is defined as GENMASK(31, 17), where the 15 suffix
denotes the 15-bit width of the field (similar to RGMII_CONFIG_MAX_SPD_PRG_9
and RGMII_CONFIG_MAX_SPD_PRG_2).

Because rgmii_setmask() calls rgmii_updatel(ethqos, mask, mask, offset), it
writes 0x7fff into bits 31:17. Should this instead program a specific value into
that field using rgmii_updatel() and FIELD_PREP(RGMII_CONFIG2_RSVD_CONFIG15, ...)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f@oss.qualcomm.com?part=4