Re: [PATCH net-next v9 07/11] net: dsa: realtek: rtl8365mb: add family D SDS13 PCS support

[netdev-bot+sashiko@kernel.org]

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

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

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] Family D SGMII at 10/100 Mbit/s: phylink is told port 6
  supports those speeds, but the driver never gives the SDS13 SerDes a…

--

net: dsa: realtek: rtl8365mb: add family D SDS13 PCS support

This commit extends the RTL8367S SGMII/HSGMII PCS code to family D
switches. Their CPU SerDes is reached through the indexed SDS13 window,
and their SDS_MISC layout differs from family C. The patch adds family D
tuning tables and SDS_MISC mux/mode values, always sets sds_supported on
family D, and skips the family C SerDes link-force programming in
pcs_link_up() and mac_link_up().

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index a87253b9c2700..86194d555fd6a 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -1660,6 +1783,12 @@ static void rtl8365mb_pcs_link_up(struct phylink_pcs *pcs,
>  	u32 r_speed;
>  	int ret;
>  
> +	/* Family D forces the external MAC ability from mac_link_up(); its
> +	 * SDS_MISC fields do not share the family C link-force layout.
> +	 */
> +	if (rtl8365mb_is_d(priv))
> +		return;
> +

[Severity: Medium]
On family D, does port 6 work with SGMII at 10 or 100 Mbit/s?

rtl8365mb_sds_probe_option() now always sets mb->sds_supported on family
D. So rtl8365mb_phylink_get_caps() advertises SGMII on port 6 along with
10/100:

    config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
                               MAC_10 | MAC_100 | MAC_1000FD;
    ...
    if (extint->supported_interfaces & RTL8365MB_PHY_INTERFACE_MODE_SGMII &&
        mb->sds_supported)
            __set_bit(PHY_INTERFACE_MODE_SGMII,
                      config->supported_interfaces);

The RTL8367S-VB entry added later in the series gives port 6
PHY_INTF(SGMII) | PHY_INTF(HSGMII).

On family C, rtl8365mb_pcs_link_up() is the only place that programs the
SerDes SGMII_SPD field for 10/100/1000. With this early return, no family
D path appears to give the SDS13 SerDes a rate:

  - rtl8365mb_pcs_config() writes only PA33PC_EN | PA12PC_EN |
    MAC6_SEL_SDS0 | sds_mode into SDS_MISC. It also clears NWAY_EN, and
    pcs_inband_caps() returns LINK_INBAND_DISABLE, so the SerDes cannot
    learn the speed from in-band AN either.
  - rtl8365mb_phylink_mac_link_up() skips the SDS_MISC write on family D.
  - rtl8365mb_ext_config_forcemode() writes only the MAC-side
    RTL8365MB_D_DIGITAL_INTERFACE_FORCE_REG(port) fields.

Suppose an SGMII PHY or an SGMII fixed-link on port 6 resolves to 100 or
10 Mbit/s. The MAC is forced to that speed, but the PCS has no programmed
rate. If family D hardware does not derive the SerDes rate from the MAC
force register, could the link report up and still pass no valid
traffic?

The family D SGMII branch of rtl8365mb_pcs_get_state() also does not read
the speed back from the SerDes. It keeps what phylink passed in and falls
back to 1000/full:

    if (state->speed == SPEED_UNKNOWN)
            state->speed = SPEED_1000;
    if (state->duplex == DUPLEX_UNKNOWN)
            state->duplex = DUPLEX_FULL;

No later patch in the series seems to add family D SerDes speed
programming or to limit family D SGMII to MAC_1000FD. Should family D
SGMII advertise only MAC_1000FD, or should the SerDes rate be programmed
for 10/100?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/9b73858d-fc09-4988-8bab-fd7b88db9412%40yahoo.com