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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch makes rtl8365mb_pcs_link_up() return immediately on
  family D, so on family D the SerDes link is forced only by…
- [Low] net: dsa: realtek: inadvertent clobbering of SDS1 RX disable bit
  in pcs_config

--

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

This patch extends the RTL8367S SGMII/HSGMII PCS code to cover family D
switches. On those parts the CPU SerDes is reached through the indexed
SDS13 window, and the SDS_MISC fields are laid out differently from
family C. It adds family D tuning tables and an index argument to the
SerDes read/write helpers, and it adds family D branches to the PCS and
phylink MAC callbacks.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index f31d72aa0b9a9..d23e80f1d745d 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -1667,6 +1778,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: Low]
This isn't a bug, but with this early return rtl8365mb_pcs_link_up()
programs nothing on family D. On those parts the SerDes link is forced
only by rtl8365mb_phylink_mac_link_up()->rtl8365mb_ext_config_forcemode().

Should two comments that still credit pcs_link_up() be updated too?

The first is the comment ahead of the forcing code 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.
	 */

The second is in rtl8365mb_phylink_mac_link_down():

	/* phylink has no pcs_link_down callback, so on the SerDes path only the
	 * MAC external interface force is reset here. Clearing the MAC force is
	 * enough to bring the link down; the SerDes keeps presenting its last
	 * forced state until the next pcs_link_up() reprograms it.
	 */

This patch updated the matching comments in rtl8365mb_pcs_config(),
rtl8365mb_pcs_get_state() and the pause block of
rtl8365mb_phylink_mac_link_up(). These two are still unchanged at the end
of the series.

[ ... ]

> @@ -1522,34 +1589,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 {
> +		/* Mux the SerDes to MAC8 in the requested mode */
> +		misc_mask = RTL8365MB_SDS_MISC_MAC8_SEL_SGMII_MASK |
> +			    RTL8365MB_SDS_MISC_MAC8_SEL_HSGMII_MASK;
> +		misc_val  = (sds_mode == RTL8365MB_EXT_PORT_MODE_SGMII) ?
> +			    RTL8365MB_SDS_MISC_MAC8_SEL_SGMII_MASK :
> +			    RTL8365MB_SDS_MISC_MAC8_SEL_HSGMII_MASK;
>  	}
>  
> -	/* Mux the SerDes to MAC8 in the requested mode */
>  	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;
>  
> -	val = mode << RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(id);
> -	ret = regmap_update_bits(priv->map,
> -				 RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(id),
> -				 RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(id),
> -				 val);
> -	if (ret)
> -		return ret;
> +	if (!is_d) {
> +		val = sds_mode << RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(id);
> +		ret = regmap_update_bits(priv->map,
> +					 RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(id),
> +					 RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(id),
> +					 val);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/* Take the SerDes out of reset. The vendor driver does this only
>  	 * after the SerDes mux and the interface mode are configured.
>  	 */
> -	ret = rtl8365mb_sds_write(priv, RTL8365MB_SDS_REG_RESET,
> +	ret = rtl8365mb_sds_write(priv, sds_index,
> +				  RTL8365MB_SDS_REG_RESET,
>  				  RTL8365MB_SDS_RESET_DEASSERT);
>  	if (ret)
>  		return ret;

[Severity: Low]
In rtl8365mb_pcs_config(), the family D branch updates
RTL8365MB_D_FIBER_CFG2_REG using RTL8365MB_D_FIBER_CFG2_RX_DISABLE_MASK
as the mask, which is GENMASK(7, 6), but the value written is only
RTL8365MB_D_FIBER_CFG2_RX_DISABLE_SDS0, i.e. BIT(6). So besides
setting bit 6, every call also clears bit 7 of that register.

Going by the field names, bit 7 is the RX disable bit for the second
SerDes (SDS1). Clearing it here re-enables the SDS1 fiber RX path as a
side effect of configuring the CPU SerDes. That happens regardless of
how SDS1 is actually used on the board.

rtl8365mb_ext_config_rgmii() goes out of its way to put SDS1 into the
disabled mode through RTL8365MB_D_REG_SDS1_MISC0 when ext1 is RGMII.
Dropping the SDS1 RX disable bit here seems at odds with that.

Is clearing bit 7 intentional, for example because the vendor RTL8367D
API writes the whole 2-bit field with 0x1? If so, a comment saying so
would help. If not, should the mask be limited to
RTL8365MB_D_FIBER_CFG2_RX_DISABLE_SDS0 so that only the SDS0 bit is
touched?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/38872e6f-1264-4a93-9f81-8212173e909d%40yahoo.com