[permalink] [raw]

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

On Thu, 2019-12-12 at 03:27 +0800, Andrew Lunn wrote:
> > Do you also hint at using the same number of parameters for
> > mt7531_ind_c22_phy_read() and mt7531_ind_c45_phy_read()?
>
> That is up to you. It just seems like your MDIO bus can do both C22
> and C45. And somebody might connect an external C45 PHY, so you might
> as well support it.
Yes, it is better to support external C45 PHY. Thanks for your advice.

Landen
>
> Andrew

2019-12-12 16:25:12

by Landen Chao (趙皎宏)

[permalink] [raw]

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

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 <[email protected]>
> > Signed-off-by: Sean Wang <[email protected]>
> > ---
>
> [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