Re: [PATCH net-next v2 1/8] net: dsa: realtek: rtl8365mb: detect RTL8367S-VB

[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: 1 · 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] Not a final-state bug: a later patch in this series fixes it.
- [Low] The file header comment in
  drivers/net/dsa/realtek/rtl8365mb_main.c adds RTL8367S-VB to the list…
- [Low] The driver now recognises RTL8367S-VB (chip_id 0x6642, chip_ver
  0x0010) under the existing compatible "realtek,rtl8365mb", and its…

--

net: dsa: realtek: rtl8365mb: detect RTL8367S-VB

This adds detection of the RTL8367S-VB (chip_id 0x6642, chip_ver 0x0010).
It introduces a family C / family D split in the chip_info table. The
RTL8367S-VB entry is tagged as family D with SGMII/HSGMII on ext_int 0,
and priv->num_ports is capped at 8 for family D.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index da06f952a053d..80fc551992195 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
> @@ -81,6 +81,7 @@
>   *  - RTL8367RB-VB
>   *  - RTL8367SB
>   *  - RTL8367S
> + *  - RTL8367S-VB
>   *  - RTL8370MB
>   *  - RTL8310SR
>   *

[Severity: Low]
This isn't a bug, but the header comment now puts RTL8367S-VB in the list
introduced by "According to documentation from Realtek, the family should
include the following chips". That list describes the single RTL8367C
family the driver targets.

The same comment block also separates out chips that "require the rtl8367d
vendor driver". The new chip_info entry tags RTL8367S-VB as
RTL8365MB_FAMILY_D, so the comment and the code disagree about the
chip's family.

The comment above RTL8365MB_LEARN_LIMIT_MAX also still describes one
family:

  /* Valid for the whole family except RTL8370B, which has 4160 entries.
   * RTL8370B is mentioned in vendor code but it might not even belong
   * to the same RTL8367C family.
   */

The 2112 limit now applies to family D chips as well.

Could these comments be updated to cover the family D split? The mismatch
is still there at the end of the series.

[ ... ]

> @@ -777,6 +786,19 @@ static const struct rtl8365mb_chip_info rtl8365mb_chip_infos[] = {
>  		.jam_table = rtl8365mb_init_jam_8365mb_vc,
>  		.jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc),
>  	},
> +	{
> +		.name = "RTL8367S-VB",
> +		.chip_id = 0x6642,
> +		.chip_ver = 0x0010,
> +		.family = RTL8365MB_FAMILY_D,
> +		.extints = {
> +			{ 6, 0, PHY_INTF(SGMII) | PHY_INTF(HSGMII) },

[Severity: Medium]
On RTL8367S-VB the SerDes sits on ext_int id 0 (port 6) and RGMII sits on
id 1 (port 7). Is rtl8365mb_pcs_config() still correct for this layout at
this point in the series? It hardcodes the family C interface ID:

#define RTL8365MB_SDS_EXT_INTERFACE_ID		1

rtl8365mb_pcs_config() {
	const int id = RTL8365MB_SDS_EXT_INTERFACE_ID;
	...
	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);
	...
}

RTL8365MB_DIGITAL_INTERFACE_SELECT_REG0 (0x1305) holds EXT0 in bits 3:0
and EXT1 in bits 7:4. On family D this write would therefore put the
SGMII/HSGMII mode into the nibble for port 7's RGMII interface.

rtl8365mb_ext_config_forcemode() works from a different interface. It
forces the link through RTL8365MB_DIGITAL_INTERFACE_FORCE_REG(extint->id),
and extint->id is 0 here.

rtl8365mb_sds_probe_option() doesn't check the family either. If the
family C option register reads non-zero, sds_supported gets set and
phylink can pick SGMII or 2500base-x on port 6:

rtl8365mb_setup()
  rtl8365mb_sds_probe_option()
    mb->sds_supported = true;
...
rtl8365mb_pcs_config()
  regmap_update_bits(..., DIGITAL_INTERFACE_SELECT_REG(1), ...)

Could this break port 7 RGMII, or leave the SerDes mode unset on
RTL8367S-VB?

A later patch in the series, "net: dsa: realtek: rtl8365mb: add family D
SDS13 PCS support", appears to fix this. It adds a family D branch to
rtl8365mb_pcs_config() that skips the DIGITAL_INTERFACE_SELECT write, and
it makes rtl8365mb_sds_probe_option() return early for family D.

Could SerDes modes stay disabled for family D in this patch, so the
intermediate commits remain bisectable?

> +			{ 7, 1, PHY_INTF(MII) | PHY_INTF(TMII) |
> +				PHY_INTF(RMII) | PHY_INTF(RGMII) },
> +		},
> +		.jam_table = rtl8365mb_init_jam_8365mb_vc,
> +		.jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc),
> +	},
>  };

[Severity: Low]
The driver now matches RTL8367S-VB under the existing realtek,rtl8365mb
compatible. Documentation/devicetree/bindings/net/dsa/realtek.yaml wasn't
updated, though, and its model list still reads:

      realtek,rtl8365mb:
        Use with models RTL8363NB, RTL8363NB-VB, RTL8363SC, RTL8363SC-VB,
        RTL8364NB, RTL8364NB-VB, RTL8365MB, RTL8366SC, RTL8367RB-VB, RTL8367S,
        RTL8367SB, RTL8370MB, RTL8310SR

Should RTL8367S-VB be added to that list? None of the later patches in
the series update the binding.

[ ... ]

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