Re: [PATCH net-next v2 7/8] net: dsa: realtek: rtl8365mb: add family D SDS13 PCS support

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 2

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] On family D (RTL8367S-VB), the SerDes path skips every SerDes-
  side speed, duplex and pause step, but phylink is still told the port…
- [Medium] In this commit, the family D `rtl8365mb_pcs_config()` writes
  the SDS_MISC mode field (bits 4:0) straight to SGMII (0x02) or HSGMII…
- [Low] The `struct rtl8365mb` kernel-doc says `@sds_supported` means
  "SerDes tuning parameters match the chip option, so the SerDes…
- [Low] Several comments in the SerDes PCS/MAC path still say the SerDes
  link parameters are forced, or reprogrammed, in…

--

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

This commit extends the RTL8367S SGMII/HSGMII PCS code to family D
switches, whose CPU SerDes is reached through the indexed SDS13 window.
It adds family D tuning tables and SDS_MISC encodings. The family D path
is chosen from the chip-family metadata, and family C is left unchanged.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index 3819a48386006..4cad90f5c0181 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -1398,6 +1435,14 @@ static int rtl8365mb_sds_probe_option(struct realtek_priv *priv)
>  	int ret;
>  	int i;
>  
> +	/* Family D has a fixed SDS13 programming model and does not use the
> +	 * family C option register to select its tuning table.
> +	 */
> +	if (rtl8365mb_get_family(priv) == RTL8365MB_FAMILY_D) {
> +		mb->sds_supported = true;
> +		return 0;
> +	}
> +
>  	/* Nothing to probe if no external interface is wired to the SerDes */
>  	for (i = 0; i < RTL8365MB_MAX_NUM_EXTINTS; i++) {
>  		extint = &mb->chip_info->extints[i];

[Severity: Low]
This isn't a bug, but this early return skips both the extint scan ("Nothing
to probe if no external interface is wired to the SerDes") and the option
register check. The kernel-doc for struct rtl8365mb still says:

 * @sds_supported: SerDes tuning parameters match the chip option, so the
 *                 SerDes interface modes can be advertised

Should the kernel-doc be updated for family D?

rtl8365mb_setup() also calls rtl8365mb_sds_raise_rate_limits() whenever
sds_supported is set. As a result, every family D chip gets its port 6
ingress and egress rate limits rewritten.

RTL8367S-VB does have an SGMII/HSGMII extint on port 6, so nothing changes
today. The commit message says this code can be reused by other family D
boards, though. Would it make sense to keep the extint scan ahead of the
family D shortcut?

[ ... ]

> @@ -1526,34 +1590,53 @@ static int rtl8365mb_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>  
>  	/* Tune the SerDes with vendor-prescribed parameters */
>  	for (i = 0; i < sds_jam_size; i++) {
> -		ret = rtl8365mb_sds_write(priv, sds_jam[i].reg,
> -					  sds_jam[i].val);
> +		ret = rtl8365mb_sds_write(priv, sds_index,
> +					  sds_jam[i].reg, sds_jam[i].val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Family-specific post-tuning configuration */
> +	if (is_d) {
> +		ret = regmap_update_bits(priv->map, RTL8365MB_D_FIBER_CFG2_REG,
> +					 RTL8365MB_D_FIBER_CFG2_RX_DISABLE_MASK,
> +					 RTL8365MB_D_FIBER_CFG2_RX_DISABLE_SDS0);
>  		if (ret)
>  			return ret;
> +
> +		misc_mask = RTL8365MB_D_SDS_MISC_CFG_MASK;
> +		misc_val  = RTL8365MB_D_SDS_MISC_PA33PC_EN |
> +			    RTL8365MB_D_SDS_MISC_PA12PC_EN |
> +			    RTL8365MB_D_SDS_MISC_MAC6_SEL_SDS0 | sds_mode;
> +	} else {

[ ... ]

>  	ret = regmap_update_bits(priv->map, RTL8365MB_SDS_MISC_REG,
> -				 RTL8365MB_SDS_MISC_MAC8_SEL_SGMII_MASK |
> -					 RTL8365MB_SDS_MISC_MAC8_SEL_HSGMII_MASK,
> -				 mode == RTL8365MB_EXT_PORT_MODE_SGMII ?
> -					 RTL8365MB_SDS_MISC_MAC8_SEL_SGMII_MASK :
> -					 RTL8365MB_SDS_MISC_MAC8_SEL_HSGMII_MASK);
> +				 misc_mask, misc_val);
>  	if (ret)
>  		return ret;

[Severity: Medium]
On family D, does this write give the SerDes receiver a mode edge to latch
on?

The mode field (bits 4:0) is written straight to SGMII (0x02) or HSGMII
(0x12). It is never parked at RTL8365MB_D_PORT_SDS_MODE_DISABLE (0x1f)
first.

The register may already hold the target value, either from the bootloader
or from an earlier pcs_config() with the same interface. In that case
regmap_update_bits() writes nothing. Even when the write does happen, it can
land before the far-end MAC is up.

At this point in the series, could the CPU-facing trunk then report link up
on both sides but pass no frames?

The later patch "net: dsa: realtek: rtl8365mb: re-latch the family D SerDes"
seems to fix this with rtl8365mb_sds_relatch_work(). That work parks
SDS_MISC at DISABLE, sleeps 20 ms, restores the target, and retries until
the link status bit is set. Could that logic be folded into this patch, so
this commit works on its own when bisecting?

[ ... ]

> @@ -1576,14 +1661,16 @@ static int rtl8365mb_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>  	/* Keep SGMII in-band autonegotiation disabled: the link parameters are
>  	 * forced from rtl8365mb_pcs_link_up() instead.
>  	 */
> -	ret = rtl8365mb_sds_read(priv, RTL8365MB_SDS_REG_NWAY, &val);
> +	ret = rtl8365mb_sds_read(priv, sds_index,
> +				 RTL8365MB_SDS_REG_NWAY, &val);

[Severity: Low]
This isn't a bug, but this comment is no longer accurate for family D,
because rtl8365mb_pcs_link_up() now returns right away there. Several other
comments say the same thing and are unchanged.

In rtl8365mb_pcs_get_state():

	/* In-band autonegotiation is not implemented, so the link parameters are
	 * forced from rtl8365mb_pcs_link_up(). ...

In rtl8365mb_phylink_mac_link_up():

	/* The SerDes forced link state is programmed by the PCS in
	 * rtl8365mb_pcs_link_up(); here only the MAC external interface force
	 * is configured, for both RGMII and SerDes.
	 */

In rtl8365mb_phylink_mac_link_down():

	 * enough to bring the link down; the SerDes keeps presenting its last
	 * forced state until the next pcs_link_up() reprograms it.

The comment above RTL8365MB_SDS_EXT_INTERFACE_ID also says:

/* The embedded SerDes can only be muxed to external interface 1 (MAC8),
 * which is port 6.
 */

However, the family D chip_info puts the SerDes on extint 0 (MAC6, through
SDS13).

Could these comments be updated to cover the family D path too?

[ ... ]

> @@ -1630,6 +1725,13 @@ static void rtl8365mb_pcs_get_state(struct phylink_pcs *pcs,
>  	if (!state->link)
>  		return;
>  
> +	if (is_d) {
> +		state->duplex = DUPLEX_FULL;
> +		state->speed = state->interface == PHY_INTERFACE_MODE_2500BASEX ?
> +				SPEED_2500 : SPEED_1000;
> +		return;
> +	}
> +
>  	/* The speed and duplex are forced; read them back from the values
>  	 * programmed into the SerDes MISC register.
>  	 */
> @@ -1671,6 +1773,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_get_family(priv) == RTL8365MB_FAMILY_D)
> +		return;
> +

[Severity: Medium]
With this early return, does anything on family D program the SerDes for
10/100 or half duplex?

The family D SDS_MISC fields added here (PA33PC_EN, PA12PC_EN, MAC6_SEL_SDS0
and the 5-bit mode) have no speed or duplex field. pcs_config() also clears
NWAY, so no in-band code word carries the rate either. If the family D MAC
force register alone is enough for these modes, could a comment say so?

Pause has the same gap. The rtl8365mb_phylink_mac_link_up() hunk later in
this patch skips the SDS_MISC TXFC/RXFC write on family D:

	if (rtl8365mb_interface_is_serdes(interface) &&
	    rtl8365mb_get_family(priv) != RTL8365MB_FAMILY_D) {

Nothing replaces that write. The comment just above it says these SerDes
bits, "not the MAC force pause bits, gate pause on the SerDes external
interface".

The family D branch in rtl8365mb_pcs_get_state() above always reports
DUPLEX_FULL, plus SPEED_1000 or SPEED_2500.

When in-band AN is disabled, phylink_mac_pcs_get_state() fills in
state->speed and state->duplex from the forced link_config before it calls
pcs_get_state(). rtl8365mb_pcs_inband_caps() always returns
LINK_INBAND_DISABLE.

Take managed = "in-band-status" with 100/half forced through ethtool. Would
this branch replace those values with 1000/full? That would affect both what
ethtool reports and what the next mac_link_up() forces into the MAC.

Meanwhile, rtl8365mb_phylink_get_caps() still advertises:

	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
				   MAC_10 | MAC_100 | MAC_1000FD;

rtl8365mb_sds_probe_option() now sets sds_supported for every family D
chip. That means SGMII and 2500base-X are offered on the RTL8367S-VB port 6
extint.

Should the family D SerDes port limit its mac_capabilities to what this path
actually handles? Or should the commit message explain these limits? Right
now it only says the "SDS_MISC fields differ from family C", and this
appears unchanged at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/28382f3f-03c1-4606-9b11-86f118abeafe%40yahoo.com