Re: [PATCH net-next 4/6] net: dsa: mt7530: Add the support of MT7531 switch

[Florian Fainelli <f.fainelli@gmail.com>]

On 12/10/2019 12:14 AM, Landen Chao wrote:
> Add new support for MT7531:
> 
> MT7531 is the next generation of MT7530. It is also a 7-ports switch with
> 5 giga embedded phys, 2 cpu ports, and the same MAC logic of MT7530. Cpu
> port 6 only supports HSGMII interface. Cpu port 5 supports either RGMII
> or HSGMII in different HW sku. Due to HSGMII interface support, pll, and
> pad setting are different from MT7530. This patch adds different initial
> setting of MT7531.
> 
> Signed-off-by: Landen Chao <landen.chao@mediatek.com>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---

[snip]

> +	/* Enable PHY power, since phy_device has not yet been created
> +	 * provided for phy_[read,write]_mmd_indirect is called, we provide
> +	 * our own mt7531_ind_mmd_phy_[read,write] to complete this
> +	 * function.
> +	 */
> +	val = mt7531_ind_mmd_phy_read(priv, 0, PHY_DEV1F,
> +				      MT7531_PHY_DEV1F_REG_403);
> +	val |= MT7531_PHY_EN_BYPASS_MODE;
> +	val &= ~MT7531_PHY_POWER_OFF;
> +	mt7531_ind_mmd_phy_write(priv, 0, PHY_DEV1F,
> +				 MT7531_PHY_DEV1F_REG_403, val);

You are doing this for port 0 only, is that because this broadcasts to
all internal PHYs as well, or is it enough to somehow do it just for
port 0? It sounds like you might want to make this operation a bit more
scoped, if you have an external PHY that also responds to broadcast MDIO
writes this could possibly cause some unattended effects, no?

[snip]

> +static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port)
> +{
> +	u32 val;
> +
> +	if (port != 5) {
> +		dev_err(priv->dev, "RGMII mode is not available for port %d\n",
> +			port);
> +		return -EINVAL;
> +	}
> +
> +	val = mt7530_read(priv, MT7531_CLKGEN_CTRL);
> +	val |= GP_CLK_EN;
> +	val &= ~GP_MODE_MASK;
> +	val |= GP_MODE(MT7531_GP_MODE_RGMII);
> +	val |= TXCLK_NO_REVERSE;
> +	val |= RXCLK_NO_DELAY;

You actually need to look at the port's phy_interface_t value to
determine whether the delays should be set/clear in either RX or TX
directions.

[snip]

> -	if (phylink_autoneg_inband(mode)) {
> +	if (phylink_autoneg_inband(mode) &&
> +	    state->interface != PHY_INTERFACE_MODE_SGMII) {

So you don't support in-band auto-negotiation for 1000BaseX either?

[snip]

> @@ -1590,9 +2197,20 @@ static void mt753x_phylink_validate(struct dsa_switch *ds, int port,
>  	phylink_set_port_modes(mask);
>  	phylink_set(mask, Autoneg);
>  
> -	if (state->interface == PHY_INTERFACE_MODE_TRGMII) {
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_TRGMII:
>  		phylink_set(mask, 1000baseT_Full);
> -	} else {
> +		break;
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		phylink_set(mask, 1000baseX_Full);
> +		phylink_set(mask, 2500baseX_Full);

Did you intend this to be:

	case PHY_INTERFACE_MODE_2500BASEX:
		phylink_set(mask, 2500baseX_Full);
		/* fall through */
	case PHY_INTERFACE_MODE_1000BASEX:
		phylink_set(mask, 1000baseX_Full);
		break;

?
[snip]

> +/* Register for PHY Indirect Access Control */
> +#define MT7531_PHY_IAC			0x701C
> +#define  PHY_ACS_ST			BIT(31)
> +#define  MDIO_REG_ADDR_MASK		(0x1f << 25)
> +#define  MDIO_PHY_ADDR_MASK		(0x1f << 20)
> +#define  MDIO_CMD_MASK			(0x3 << 18)
> +#define  MDIO_ST_MASK			(0x3 << 16)
> +#define  MDIO_RW_DATA_MASK		(0xffff)
> +#define  MDIO_REG_ADDR(x)		(((x) & 0x1f) << 25)
> +#define  MDIO_DEV_ADDR(x)		(((x) & 0x1f) << 25)
> +#define  MDIO_PHY_ADDR(x)		(((x) & 0x1f) << 20)
> +#define  MDIO_CMD(x)			(((x) & 0x3) << 18)
> +#define  MDIO_ST(x)			(((x) & 0x3) << 16)

I would suggest names that are more scoped because these could easily
collide with existing of future definitions from include/linux/mdio.h.

> +
> +enum mt7531_phy_iac_cmd {
> +	MT7531_MDIO_ADDR = 0,
> +	MT7531_MDIO_WRITE = 1,
> +	MT7531_MDIO_READ = 2,
> +	MT7531_MDIO_READ_CL45 = 3,
> +};
> +
> +/* MDIO_ST: MDIO start field */
> +enum mt7531_mdio_st {
> +	MT7531_MDIO_ST_CL45 = 0,
> +	MT7531_MDIO_ST_CL22 = 1,
> +};
> +
> +#define  MDIO_CL22_READ			(MDIO_ST(MT7531_MDIO_ST_CL22) | \
> +					 MDIO_CMD(MT7531_MDIO_READ))
> +#define  MDIO_CL22_WRITE		(MDIO_ST(MT7531_MDIO_ST_CL22) | \
> +					 MDIO_CMD(MT7531_MDIO_WRITE))
> +#define  MDIO_CL45_ADDR			(MDIO_ST(MT7531_MDIO_ST_CL45) | \
> +					 MDIO_CMD(MT7531_MDIO_ADDR))
> +#define  MDIO_CL45_READ			(MDIO_ST(MT7531_MDIO_ST_CL45) | \
> +					 MDIO_CMD(MT7531_MDIO_READ))
> +#define  MDIO_CL45_WRITE		(MDIO_ST(MT7531_MDIO_ST_CL45) | \
> +					 MDIO_CMD(MT7531_MDIO_WRITE))

Likewise.
-- 
Florian