Re: [PATCH net-next 2/2] net: dsa: mt7530: add EN7528 support

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] ID_EN7528 writes to an MT7531-specific register, MT7531_DBG_CNT(i), inside mt7531_setup_common().
- [Medium] Missing stabilization delay after hardware reset of PHYs before issuing MDIO writes.

Pre-existing issues:
- [High] ID_AN7583 incorrectly falls back to MT7530_MIRROR_EN (BIT 3) while using the MT7531_CFC register format, failing port mirroring and corrupting CPU packet forwarding logic.
- [High] ID_EN7581 and ID_AN7583 are missing from the MT753X_FORCE_MODE macro, causing them to use the MT7530 bitmask instead of the MT7531 bitmask.
--

--- Patch [2]: [PATCH net-next 2/2] net: dsa: mt7530: add EN7528 support ---
commit 6e316acbfa461fc0e2739fe9d461c2511b9c012c
Author: Ahmed Naseef <naseefkm@gmail.com>

net: dsa: mt7530: add EN7528 support
    
This patch adds support for the EcoNet EN7528 SoC switch. It introduces the
ID_EN7528 variant, reusing mt7988_setup() and indirect PHY accessors, while
handling the switch core as an MT7530 derivative rather than MT7531.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3c2a3029b10cf..2aecad77af9ad 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c

[ ... ]

> @@ -3342,6 +3375,17 @@ static int mt7988_setup(struct dsa_switch *ds)
>  	/* Reset the switch PHYs */
>  	mt7530_write(priv, MT7530_SYS_CTRL, SYS_CTRL_PHY_RST);
>  
> +	/* The EN7528 LAN GPHYs advertise EEE by default, but negotiating EEE
> +	 * with common link partners (e.g. Realtek GbE NICs) results in an
> +	 * unstable link with dropped frames. Disable EEE advertisement on
> +	 * them.
> +	 */
> +	if (priv->id == ID_EN7528)
> +		for (i = EN7528_GPHY_BASE;
> +		     i < EN7528_GPHY_BASE + EN7528_NUM_GPHYS; i++)
> +			mt7531_ind_c45_phy_write(priv, i, MDIO_MMD_AN,
> +						 MDIO_AN_EEE_ADV, 0);

[Severity: Medium]
Does this sequence need a stabilization delay before issuing MDIO writes?

The switch PHYs are hardware reset via SYS_CTRL_PHY_RST just before this,
and hardware PHYs typically require a few milliseconds to exit the reset
state and initialize their management logic.

Could issuing MDIO C45 writes immediately after asserting the reset result in
the PHYs ignoring the transaction or the MDIO controller timing out?

>  
>  	return mt7531_setup_common(ds);

[Severity: Medium]
Does mt7531_setup_common() write to MT7531-specific registers that shouldn't
be accessed by the EN7528?

The commit message states that the EN7528 switch core is an MT7530 derivative
and not an MT7531 derivative. However, mt7988_setup() returns by calling
mt7531_setup_common(), which executes:

mt7531_setup_common() {
    ...
    for (i = 0; i < priv->ds->num_ports; i++) {
        ...
        mt7530_set(priv, MT7531_DBG_CNT(i), MT7531_DIS_CLR);

Could writing MT7531_DIS_CLR to the MT7531_DBG_CNT(i) offset on an MT7530
derivative cause hardware errors or state corruption?

>  }
>  
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index dd33b0df3419e..5f1e841f42c0e 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h

[ ... ]

> @@ -21,6 +21,7 @@ enum mt753x_id {
>  	ID_MT7988 = 3,
>  	ID_EN7581 = 4,
>  	ID_AN7583 = 5,
> +	ID_EN7528 = 6,

[Severity: High]
This isn't a bug introduced by this patch, but are ID_AN7583 and ID_EN7581
missing from the MT753X_MIRROR_EN and MT753X_FORCE_MODE macros?

In drivers/net/dsa/mt7530.h, MT753X_MIRROR_EN is defined as:

#define MT753X_MIRROR_EN(id)        ((id == ID_MT7531 || \
                                      id == ID_MT7988 || \
                                      id == ID_EN7581) ? \
                                     MT7531_MIRROR_EN : MT7530_MIRROR_EN)

Because ID_AN7583 is omitted, it falls back to MT7530_MIRROR_EN (BIT 3).
When mt753x_port_mirror_add() uses the MT7531_CFC register format for
ID_AN7583, it applies BIT 3. In MT7531_CFC, BIT 3 is part of
MT7531_CPU_PMAP_MASK, which controls CPU packet forwarding logic. 

Does this break port mirroring and corrupt the CPU forward control for
ID_AN7583?

Similarly, MT753X_FORCE_MODE is defined as:

#define MT753X_FORCE_MODE(id)       ((id == ID_MT7531 || \
                                      id == ID_MT7988) ? \
                                     MT7531_FORCE_MODE_MASK : \
                                     MT7530_FORCE_MODE)

Both ID_EN7581 and ID_AN7583 are missing here. When mt7531_setup_common()
attempts to force all port links down during setup, it applies
MT7530_FORCE_MODE (BIT 15) instead of MT7531_FORCE_MODE_MASK. 

Does this fail to correctly disable the port links during switch
initialization?

>  };
>  
>  #define	NUM_TRGMII_CTRL			5

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783680864.git.naseefkm@gmail.com?part=2