On Thu, 2019-12-12 at 11:57 +0800, Florian Fainelli wrote:

On 12/10/2019 12:14 AM, Landen Chao wrote:

quoted

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 <redacted>
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---

[snip]

quoted

+	/* 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]

quoted

+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]

quoted

-	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]

quoted

@@ -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]

quoted

+/* 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.

quoted

+
+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