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

[Landen Chao <landen.chao@mediatek.com>]

On Thu, 2019-12-12 at 11:57 +0800, Florian Fainelli wrote:
> 
> 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?
All internal PHY addresses can be used to access the same PHY_DEV1F
group of registers.

I think the port "0" here must be changed to reference the "first
internal phy address" to prevent the situation you mentioned.
> 
> [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.
Sure. Thanks for your advice.
> 
> [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?
According to the user manual I have, it only provides the configure 10M
+100M+1000M AN mode/1000M force mode/2500M force mode, so I mapping them
to SGMII/1000BaseX/2500BaseX. The user friendly part of this IP wraps
too much detail to map to the spec. I'll try to dig it out.
> 
> [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;
> 
> ?
As the user manual mentioned, it is more likely to be:
 	case PHY_INTERFACE_MODE_2500BASEX:
 		phylink_set(mask, 2500baseX_Full);
 		break;
 	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.
Sure, I'll add "MT7531_" as the prefix.
> 
> > +
> > +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.
I'll add "MT7531_" as the prefix.

Landen