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

[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: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In rgmii-id mode at 100M and 10M, ethqos_set_clk_tx_rate() now
  calls clk_set_rate(link_clk, 25000000) or clk_set_rate(link_clk,…
- [Medium] net: stmmac: qcom-ethqos: rgmii_setmask() on multi-bit
  RGMII_CONFIG2_RSVD_CONFIG15
- [Low] The commit is titled and described as a fix for a defect in the
  current tree, but it has no Fixes: tag.

--

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

In rgmii-id mode, this commit powers down the DLL, sets DDR bypass and
programs the IO_MACRO through a new ethqos_rgmii_id_macro_init() helper,
so the MAC adds no delay of its own. It also stops ethqos_set_clk_tx_rate()
from doubling the link clock at 100M/10M in rgmii-id mode.

> net: stmmac: qcom-ethqos: fix RGMII_ID mode to use DLL bypass
>
> When "rgmii-id" is selected the PHY supplies both TX and RX delays, so
> the MAC must not add its own. The driver currently falls through to the
> generic DLL initialisation path which programs it to add a delay.

[ ... ]

> Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

[Severity: Low]
Should this have a Fixes: tag?

Before this patch, rgmii-id went through the generic DLL path in
ethqos_fix_mac_speed_rgmii() and then ethqos_rgmii_macro_init(). Only the
TX phase shift was suppressed there.

The MAC RX delay was still programmed:

  - at 1G: PRG_RCLK_DLY of 57 or 115, plus PRG_DLY_EN
  - at 10/100: EXT_PRG_RCLK_DLY_CODE of 5, plus EXT_PRG_RCLK_DLY and
    EXT_PRG_RCLK_DLY_EN

The PHY adds its own RX delay in this mode, so the RX delay was applied
twice.

A likely candidate is:

Fixes: 164a9ebe9742 ("net: stmmac: dwmac-qcom-ethqos: Respect phy-mode and TX delay")

That commit made rgmii-id handling cover the TX delay only and left the
MAC RX delay in place.

> 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

[ ... ]

> @@ -183,7 +186,15 @@ static int ethqos_set_clk_tx_rate(void *bsp_priv, struct clk *clk_tx_i,
>  	if (rate < 0)
>  		return rate;
>  
> -	return clk_set_rate(ethqos->link_clk, rate * 2);
> +	/* Clock Rate Requirements:
> +	 * MAC added delay: 250/50/5 Mhz for 1G/100M/10M
> +	 * No MAC delay (DLL bypass): 250/25/2.5 Mhz for 1G/100M/10M
> +	 */
> +	if (ethqos->phy_mode != PHY_INTERFACE_MODE_RGMII_ID ||
> +	    speed == SPEED_1000)
> +		rate *= 2;
> +
> +	return clk_set_rate(ethqos->link_clk, rate);

[Severity: Medium]
Can qcs404 and sc8280xp actually produce 25 MHz and 2.5 MHz on the link
clock?

On qcs404 the "rgmii" link clock is GCC_ETH_RGMII_CLK. Its parent's
frequency table only has these entries:

drivers/clk/qcom/gcc-qcs404.c:ftbl_emac_clk_src
    5 MHz, 50 MHz, 125 MHz, 250 MHz

On sc8280xp it is GCC_EMAC0_RGMII_CLK, and the lowest entry is 50 MHz:

drivers/clk/qcom/gcc-sc8280xp.c:ftbl_gcc_emac0_rgmii_clk_src
    50 MHz, 125 MHz, 250 MHz

Both branch clocks have CLK_SET_RATE_PARENT. The parent RCGs round up
rather than fail:

drivers/clk/qcom/clk-rcg2.c:clk_rcg2_determine_rate() {
	...
	return _freq_tbl_determine_rate(hw, rcg->freq_tbl, req, CEIL);
}

So in rgmii-id mode at 100M/10M, the request would silently become 50 MHz
or 5 MHz on qcs404, and 50 MHz on sc8280xp. clk_set_rate() would still
return 0.

Below 1G, ethqos_rgmii_id_macro_init() clears DDR_MODE and
TX_CLK_PHASE_SHIFT_EN and sets RSVD_CONFIG15, which assumes the 1x clock.
That gives a link clock at 2x the required rate at 100M on both SoCs and at
10M on qcs404. At 10M on sc8280xp it is 20x. No error is reported in any
of these cases.

A later patch in this series, "net: stmmac: qcom-ethqos: warn about legacy
RGMII PHY modes", tells users of these boards (qcs404-evb-4000 and
sa8540p-ride) to move to rgmii-id DTs, which puts them on this path.
Shikra, sm8150, sc8180x and qcs615 do have 25 MHz and 2.5 MHz entries.

Should the rate actually applied be checked here, for example with
clk_get_rate() after clk_set_rate()? Or does rgmii-id below 1G need
different handling on these SoCs?

[ ... ]

> @@ -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)
> +{
> +	rgmii_clrmask(ethqos, RGMII_CONFIG2_TX_TO_RX_LOOPBACK_EN,
> +		      RGMII_IO_MACRO_CONFIG2);
> +
> +	if (speed == SPEED_1000)
> +		rgmii_setmask(ethqos, RGMII_CONFIG_DDR_MODE, RGMII_IO_MACRO_CONFIG);
> +	else
> +		rgmii_clrmask(ethqos, RGMII_CONFIG_DDR_MODE, RGMII_IO_MACRO_CONFIG);
> +	rgmii_setmask(ethqos, RGMII_CONFIG_BYPASS_TX_ID_EN, RGMII_IO_MACRO_CONFIG);
> +	rgmii_clrmask(ethqos, RGMII_CONFIG_POS_NEG_DATA_SEL, RGMII_IO_MACRO_CONFIG);
> +	rgmii_clrmask(ethqos, RGMII_CONFIG_PROG_SWAP, RGMII_IO_MACRO_CONFIG);
> +
> +	if (ethqos->has_emac_ge_3)
> +		rgmii_clrmask(ethqos, RGMII_CONFIG2_DATA_DIVIDE_CLK_SEL,
> +			      RGMII_IO_MACRO_CONFIG2);
> +	else
> +		rgmii_setmask(ethqos, RGMII_CONFIG2_DATA_DIVIDE_CLK_SEL,
> +			      RGMII_IO_MACRO_CONFIG2);
> +
> +	rgmii_clrmask(ethqos, RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN,
> +		      RGMII_IO_MACRO_CONFIG2);
> +
> +	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);
> +
> +	if (ethqos->rgmii_config_loopback_en)
> +		rgmii_setmask(ethqos, RGMII_CONFIG_LOOPBACK_EN, RGMII_IO_MACRO_CONFIG);
> +	else
> +		rgmii_clrmask(ethqos, RGMII_CONFIG_LOOPBACK_EN, RGMII_IO_MACRO_CONFIG);
> +
> +	rgmii_setmask(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, RGMII_IO_MACRO_CONFIG2);
> +}
> +
>  static void ethqos_fix_mac_speed_rgmii(void *bsp_priv,
>  				       phy_interface_t interface, int speed,
>  				       unsigned int mode)

[Severity: Medium]
Does the programming guide really expect all of bits 31:17 in
RGMII_IO_MACRO_CONFIG2 to be set at 100M and 10M?

RGMII_CONFIG2_RSVD_CONFIG15 is GENMASK(31, 17), which is a 15-bit
field. The suffix looks like the field width, as it is for
RGMII_CONFIG_MAX_SPD_PRG_9 (GENMASK(16, 8)) and
RGMII_CONFIG_MAX_SPD_PRG_2 (GENMASK(7, 6)).

rgmii_setmask() is just rgmii_updatel(ethqos, mask, mask, offset). So
below 1G, ethqos_rgmii_id_macro_init() writes 0x7fff into this field,
which is 0xfffe0000 in RGMII_IO_MACRO_CONFIG2.

The existing ethqos_rgmii_macro_init() path only ever clears this
field, so this is the first place a non-zero value is written to it.

If the hardware expects a specific value there, for example 1, then
10M/100M links in rgmii-id mode would run with every reserved bit in
that range set. The effect of that is unknown, and it could break
those link speeds.

Should this be rgmii_updatel() with
FIELD_PREP(RGMII_CONFIG2_RSVD_CONFIG15, val) for the value the guide
gives? If all ones really is what the guide asks for, could you add a
comment saying so? Otherwise it reads like a single-bit setmask applied
to a multi-bit field by mistake.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f%40oss.qualcomm.com