[PATCH 0/9] net: ethernet: mtk_eth_soc: various enhancements

[PATCH 0/9] net: ethernet: mtk_eth_soc: various enhancements

[Daniel Golle]

This series brings a variety of fixes and enhancements for mtk_eth_soc,
adds support for the MT7981 SoC and facilitates sharing the SGMII PCS
code between mtk_eth_soc and mt7530.

Daniel Golle (9):
  net: ethernet: mtk_eth_soc: add support for MT7981 SoC
  net: ethernet: mtk_eth_soc: set MDIO bus clock frequency
  net: ethernet: mtk_eth_soc: reset PCS state
  net: ethernet: mtk_eth_soc: only write values if needed
  net: ethernet: mtk_eth_soc: fix RX data corruption issue
  net: ethernet: mtk_eth_soc: ppe: add support for flow accounting
  net: pcs: add driver for MediaTek SGMII PCS
  net: ethernet: mtk_eth_soc: switch to external PCS driver
  net: dsa: mt7530: use external PCS driver

 MAINTAINERS                                   |   7 +
 drivers/net/dsa/Kconfig                       |   2 +
 drivers/net/dsa/mt7530.c                      | 278 ++++------------
 drivers/net/dsa/mt7530.h                      |  43 +--
 drivers/net/ethernet/mediatek/Kconfig         |   2 +
 drivers/net/ethernet/mediatek/mtk_eth_path.c  |  14 +-
 drivers/net/ethernet/mediatek/mtk_eth_soc.c   |  69 +++-
 drivers/net/ethernet/mediatek/mtk_eth_soc.h   |  98 ++----
 drivers/net/ethernet/mediatek/mtk_ppe.c       | 110 +++++-
 drivers/net/ethernet/mediatek/mtk_ppe.h       |  24 +-
 .../net/ethernet/mediatek/mtk_ppe_debugfs.c   |   9 +-
 .../net/ethernet/mediatek/mtk_ppe_offload.c   |   7 +
 drivers/net/ethernet/mediatek/mtk_ppe_regs.h  |  14 +
 drivers/net/ethernet/mediatek/mtk_sgmii.c     | 190 ++---------
 drivers/net/pcs/Kconfig                       |   6 +
 drivers/net/pcs/Makefile                      |   1 +
 drivers/net/pcs/pcs-mtk.c                     | 314 ++++++++++++++++++
 include/linux/pcs/pcs-mtk.h                   |  13 +
 18 files changed, 707 insertions(+), 494 deletions(-)
 create mode 100644 drivers/net/pcs/pcs-mtk.c
 create mode 100644 include/linux/pcs/pcs-mtk.h


base-commit: 4fafd96910add124586b549ad005dcd179de8a18
-- 
2.39.1

[PATCH 1/9] net: ethernet: mtk_eth_soc: add support for MT7981 SoC

[Daniel Golle]

The MediaTek MT7981 SoC comes two 1G/2.5G SGMII, just like MT7986.

In addition MT7981 comes with a built-in 1000Base-T PHY which can be
used with GMAC1.

As many MT7981 boards make use of swapping SGMII phase and neutral, add
new device-tree attribute 'mediatek,pn_swap' to support them.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_path.c | 14 +++++++--
 drivers/net/ethernet/mediatek/mtk_eth_soc.c  | 21 +++++++++++++
 drivers/net/ethernet/mediatek/mtk_eth_soc.h  | 31 ++++++++++++++++++++
 drivers/net/ethernet/mediatek/mtk_sgmii.c    | 10 +++++++
 4 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_path.c b/drivers/net/ethernet/mediatek/mtk_eth_path.c
index 72648535a14d..317e447f4991 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_path.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_path.c
@@ -96,12 +96,20 @@ static int set_mux_gmac2_gmac0_to_gephy(struct mtk_eth *eth, int path)
 
 static int set_mux_u3_gmac2_to_qphy(struct mtk_eth *eth, int path)
 {
-	unsigned int val = 0;
+	unsigned int val = 0, mask = 0, reg = 0;
 	bool updated = true;
 
 	switch (path) {
 	case MTK_ETH_PATH_GMAC2_SGMII:
-		val = CO_QPHY_SEL;
+		if (MTK_HAS_CAPS(eth->soc->caps, MTK_U3_COPHY_V2)) {
+			reg = USB_PHY_SWITCH_REG;
+			val = SGMII_QPHY_SEL;
+			mask = QPHY_SEL_MASK;
+		} else {
+			reg = INFRA_MISC2;
+			val = CO_QPHY_SEL;
+			mask = val;
+		}
 		break;
 	default:
 		updated = false;
@@ -109,7 +117,7 @@ static int set_mux_u3_gmac2_to_qphy(struct mtk_eth *eth, int path)
 	}
 
 	if (updated)
-		regmap_update_bits(eth->infra, INFRA_MISC2, CO_QPHY_SEL, val);
+		regmap_update_bits(eth->infra, reg, mask, val);
 
 	dev_dbg(eth->dev, "path %s in %s updated = %d\n",
 		mtk_eth_path_name(path), __func__, updated);
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index f1cb1efc94cf..a44ffff48c7b 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -4852,6 +4852,26 @@ static const struct mtk_soc_data mt7629_data = {
 	},
 };
 
+static const struct mtk_soc_data mt7981_data = {
+	.reg_map = &mt7986_reg_map,
+	.ana_rgc3 = 0x128,
+	.caps = MT7981_CAPS,
+	.hw_features = MTK_HW_FEATURES,
+	.required_clks = MT7981_CLKS_BITMAP,
+	.required_pctl = false,
+	.offload_version = 2,
+	.hash_offset = 4,
+	.foe_entry_size = sizeof(struct mtk_foe_entry),
+	.txrx = {
+		.txd_size = sizeof(struct mtk_tx_dma_v2),
+		.rxd_size = sizeof(struct mtk_rx_dma_v2),
+		.rx_irq_done_mask = MTK_RX_DONE_INT_V2,
+		.rx_dma_l4_valid = RX_DMA_L4_VALID_V2,
+		.dma_max_len = MTK_TX_DMA_BUF_LEN_V2,
+		.dma_len_offset = 8,
+	},
+};
+
 static const struct mtk_soc_data mt7986_data = {
 	.reg_map = &mt7986_reg_map,
 	.ana_rgc3 = 0x128,
@@ -4894,6 +4914,7 @@ const struct of_device_id of_mtk_match[] = {
 	{ .compatible = "mediatek,mt7622-eth", .data = &mt7622_data},
 	{ .compatible = "mediatek,mt7623-eth", .data = &mt7623_data},
 	{ .compatible = "mediatek,mt7629-eth", .data = &mt7629_data},
+	{ .compatible = "mediatek,mt7981-eth", .data = &mt7981_data},
 	{ .compatible = "mediatek,mt7986-eth", .data = &mt7986_data},
 	{ .compatible = "ralink,rt5350-eth", .data = &rt5350_data},
 	{},
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index afc9d52e79bf..7230dcb29315 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -551,11 +551,22 @@
 #define SGMSYS_QPHY_PWR_STATE_CTRL 0xe8
 #define	SGMII_PHYA_PWD		BIT(4)
 
+/* Register to QPHY wrapper control */
+#define SGMSYS_QPHY_WRAP_CTRL	0xec
+#define SGMII_PN_SWAP_MASK	GENMASK(1, 0)
+#define SGMII_PN_SWAP_TX_RX	(BIT(0) | BIT(1))
+#define MTK_SGMII_FLAG_PN_SWAP	BIT(0)
+
 /* Infrasys subsystem config registers */
 #define INFRA_MISC2            0x70c
 #define CO_QPHY_SEL            BIT(0)
 #define GEPHY_MAC_SEL          BIT(1)
 
+/* Top misc registers */
+#define USB_PHY_SWITCH_REG	0x218
+#define QPHY_SEL_MASK		GENMASK(1, 0)
+#define SGMII_QPHY_SEL		0x2
+
 /* MT7628/88 specific stuff */
 #define MT7628_PDMA_OFFSET	0x0800
 #define MT7628_SDM_OFFSET	0x0c00
@@ -736,6 +747,17 @@ enum mtk_clks_map {
 				 BIT(MTK_CLK_SGMII2_CDR_FB) | \
 				 BIT(MTK_CLK_SGMII_CK) | \
 				 BIT(MTK_CLK_ETH2PLL) | BIT(MTK_CLK_SGMIITOP))
+#define MT7981_CLKS_BITMAP	(BIT(MTK_CLK_FE) | BIT(MTK_CLK_GP2) | BIT(MTK_CLK_GP1) | \
+				 BIT(MTK_CLK_WOCPU0) | \
+				 BIT(MTK_CLK_SGMII_TX_250M) | \
+				 BIT(MTK_CLK_SGMII_RX_250M) | \
+				 BIT(MTK_CLK_SGMII_CDR_REF) | \
+				 BIT(MTK_CLK_SGMII_CDR_FB) | \
+				 BIT(MTK_CLK_SGMII2_TX_250M) | \
+				 BIT(MTK_CLK_SGMII2_RX_250M) | \
+				 BIT(MTK_CLK_SGMII2_CDR_REF) | \
+				 BIT(MTK_CLK_SGMII2_CDR_FB) | \
+				 BIT(MTK_CLK_SGMII_CK))
 #define MT7986_CLKS_BITMAP	(BIT(MTK_CLK_FE) | BIT(MTK_CLK_GP2) | BIT(MTK_CLK_GP1) | \
 				 BIT(MTK_CLK_WOCPU1) | BIT(MTK_CLK_WOCPU0) | \
 				 BIT(MTK_CLK_SGMII_TX_250M) | \
@@ -849,6 +871,7 @@ enum mkt_eth_capabilities {
 	MTK_NETSYS_V2_BIT,
 	MTK_SOC_MT7628_BIT,
 	MTK_RSTCTRL_PPE1_BIT,
+	MTK_U3_COPHY_V2_BIT,
 
 	/* MUX BITS*/
 	MTK_ETH_MUX_GDM1_TO_GMAC1_ESW_BIT,
@@ -883,6 +906,7 @@ enum mkt_eth_capabilities {
 #define MTK_NETSYS_V2		BIT(MTK_NETSYS_V2_BIT)
 #define MTK_SOC_MT7628		BIT(MTK_SOC_MT7628_BIT)
 #define MTK_RSTCTRL_PPE1	BIT(MTK_RSTCTRL_PPE1_BIT)
+#define MTK_U3_COPHY_V2		BIT(MTK_U3_COPHY_V2_BIT)
 
 #define MTK_ETH_MUX_GDM1_TO_GMAC1_ESW		\
 	BIT(MTK_ETH_MUX_GDM1_TO_GMAC1_ESW_BIT)
@@ -955,6 +979,11 @@ enum mkt_eth_capabilities {
 		      MTK_MUX_U3_GMAC2_TO_QPHY | \
 		      MTK_MUX_GMAC12_TO_GEPHY_SGMII | MTK_QDMA)
 
+#define MT7981_CAPS  (MTK_GMAC1_SGMII | MTK_GMAC2_SGMII | MTK_GMAC2_GEPHY | \
+		      MTK_MUX_GMAC12_TO_GEPHY_SGMII | MTK_QDMA | \
+		      MTK_MUX_U3_GMAC2_TO_QPHY | MTK_U3_COPHY_V2 | \
+		      MTK_NETSYS_V2 | MTK_RSTCTRL_PPE1)
+
 #define MT7986_CAPS  (MTK_GMAC1_SGMII | MTK_GMAC2_SGMII | \
 		      MTK_MUX_GMAC12_TO_GEPHY_SGMII | MTK_QDMA | \
 		      MTK_NETSYS_V2 | MTK_RSTCTRL_PPE1)
@@ -1068,12 +1097,14 @@ struct mtk_soc_data {
  * @ana_rgc3:          The offset refers to register ANA_RGC3 related to regmap
  * @interface:         Currently configured interface mode
  * @pcs:               Phylink PCS structure
+ * @flags:             Flags indicating hardware properties
  */
 struct mtk_pcs {
 	struct regmap	*regmap;
 	u32             ana_rgc3;
 	phy_interface_t	interface;
 	struct phylink_pcs pcs;
+	u32		flags;
 };
 
 /* struct mtk_sgmii -  This is the structure holding sgmii regmap and its
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index bb00de1003ac..0fe403db23c9 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -88,6 +88,11 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 		regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
 				   SGMII_PHYA_PWD, SGMII_PHYA_PWD);
 
+		if (mpcs->flags & MTK_SGMII_FLAG_PN_SWAP)
+			regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_WRAP_CTRL,
+					   SGMII_PN_SWAP_MASK,
+					   SGMII_PN_SWAP_TX_RX);
+
 		if (interface == PHY_INTERFACE_MODE_2500BASEX)
 			rgc3 = RG_PHY_SPEED_3_125G;
 		else
@@ -182,6 +187,11 @@ int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)
 
 		ss->pcs[i].ana_rgc3 = ana_rgc3;
 		ss->pcs[i].regmap = syscon_node_to_regmap(np);
+
+		ss->pcs[i].flags = 0;
+		if (of_property_read_bool(np, "mediatek,pn_swap"))
+			ss->pcs[i].flags |= MTK_SGMII_FLAG_PN_SWAP;
+
 		of_node_put(np);
 		if (IS_ERR(ss->pcs[i].regmap))
 			return PTR_ERR(ss->pcs[i].regmap);
-- 
2.39.1

Re: [PATCH 1/9] net: ethernet: mtk_eth_soc: add support for MT7981 SoC

[Andrew Lunn]

On Fri, Feb 03, 2023 at 07:00:21AM +0000, Daniel Golle wrote:
> The MediaTek MT7981 SoC comes two 1G/2.5G SGMII, just like MT7986.
> 
> In addition MT7981 comes with a built-in 1000Base-T PHY which can be
> used with GMAC1.
> 
> As many MT7981 boards make use of swapping SGMII phase and neutral, add
> new device-tree attribute 'mediatek,pn_swap' to support them.

Is this new property documented in the device tree binding?

   Andrew

Re: [PATCH 1/9] net: ethernet: mtk_eth_soc: add support for MT7981 SoC

[Vladimir Oltean]

On Fri, Feb 03, 2023 at 07:00:21AM +0000, Daniel Golle wrote:
> The MediaTek MT7981 SoC comes two 1G/2.5G SGMII, just like MT7986.
> 
> In addition MT7981 comes with a built-in 1000Base-T PHY which can be
> used with GMAC1.
> 
> As many MT7981 boards make use of swapping SGMII phase and neutral, add
> new device-tree attribute 'mediatek,pn_swap' to support them.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---

Phase and neutral? What is this, a power plug?
Since SGMII uses differential signaling, I wonder if this isn't about
the polarity of the TX lane (which pin carries the Positive signal and
which the Negative one).

I think there is room for a more general device tree property name than
"mediatek,pn_swap". The Designware XPCS also supports this, see
DW_VR_MII_DIG_CTRL2_TX_POL_INV and the comments in
nxp_sja1105_sgmii_pma_config().

Re: [PATCH 1/9] net: ethernet: mtk_eth_soc: add support for MT7981 SoC

[Vladimir Oltean]

On Fri, Feb 03, 2023 at 07:00:21AM +0000, Daniel Golle wrote:
> The MediaTek MT7981 SoC comes two 1G/2.5G SGMII, just like MT7986.

comes /with/ two 1G/2.5G SGMII /ports/

[PATCH 2/9] net: ethernet: mtk_eth_soc: set MDIO bus clock frequency

[Daniel Golle]

Set MDIO bus clock frequency and allow setting a custom maximum
frequency from device tree.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 25 +++++++++++++++++++++
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |  5 +++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index a44ffff48c7b..9050423821dc 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -790,7 +790,9 @@ static const struct phylink_mac_ops mtk_phylink_ops = {
 static int mtk_mdio_init(struct mtk_eth *eth)
 {
 	struct device_node *mii_np;
+	int clk = 25000000, max_clk = 2500000, divider = 1;
 	int ret;
+	u32 val;
 
 	mii_np = of_get_child_by_name(eth->dev->of_node, "mdio-bus");
 	if (!mii_np) {
@@ -818,6 +820,29 @@ static int mtk_mdio_init(struct mtk_eth *eth)
 	eth->mii_bus->parent = eth->dev;
 
 	snprintf(eth->mii_bus->id, MII_BUS_ID_SIZE, "%pOFn", mii_np);
+
+	if (!of_property_read_u32(mii_np, "clock-frequency", &val))
+		max_clk = val;
+
+	while (clk / divider > max_clk) {
+		if (divider >= 63)
+			break;
+
+		divider++;
+	};
+
+	val = mtk_r32(eth, MTK_PPSC);
+	val |= PPSC_MDC_TURBO;
+	mtk_w32(eth, val, MTK_PPSC);
+
+	/* Configure MDC Divider */
+	val = mtk_r32(eth, MTK_PPSC);
+	val &= ~PPSC_MDC_CFG;
+	val |= FIELD_PREP(PPSC_MDC_CFG, divider);
+	mtk_w32(eth, val, MTK_PPSC);
+
+	dev_dbg(eth->dev, "MDC is running on %d Hz\n", clk / divider);
+
 	ret = of_mdiobus_register(eth->mii_bus, mii_np);
 
 err_put_node:
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 7230dcb29315..724815ae18a0 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -363,6 +363,11 @@
 #define RX_DMA_VTAG_V2		BIT(0)
 #define RX_DMA_L4_VALID_V2	BIT(2)
 
+/* PHY Polling and SMI Master Control registers */
+#define MTK_PPSC		0x10000
+#define PPSC_MDC_CFG		GENMASK(29, 24)
+#define PPSC_MDC_TURBO		BIT(20)
+
 /* PHY Indirect Access Control registers */
 #define MTK_PHY_IAC		0x10004
 #define PHY_IAC_ACCESS		BIT(31)
-- 
2.39.1

Re: [PATCH 2/9] net: ethernet: mtk_eth_soc: set MDIO bus clock frequency

[Andrew Lunn]

>  static int mtk_mdio_init(struct mtk_eth *eth)
>  {
>  	struct device_node *mii_np;
> +	int clk = 25000000, max_clk = 2500000, divider = 1;
>  	int ret;
> +	u32 val;

Reverse Christmas tree please.

> +
> +	if (!of_property_read_u32(mii_np, "clock-frequency", &val))
> +		max_clk = val;
> +
> +	while (clk / divider > max_clk) {
> +		if (divider >= 63)
> +			break;
> +
> +		divider++;
> +	};

Please add some range checks here. Return -EINVAL if val > max_clock.
Also, if divider = 63 indicating the requested clock is too slow.

> +
> +	val = mtk_r32(eth, MTK_PPSC);
> +	val |= PPSC_MDC_TURBO;
> +	mtk_w32(eth, val, MTK_PPSC);
> +
> +	/* Configure MDC Divider */
> +	val = mtk_r32(eth, MTK_PPSC);
> +	val &= ~PPSC_MDC_CFG;
> +	val |= FIELD_PREP(PPSC_MDC_CFG, divider);
> +	mtk_w32(eth, val, MTK_PPSC);

Can these two writes to MTK_PPSC be combined into one? 

val |= FIELD_PREP(PPSC_MDC_CFG, divider) | PPSC_MDC_TURBO;

    Andrew

Re: [PATCH 2/9] net: ethernet: mtk_eth_soc: set MDIO bus clock frequency

[Vladimir Oltean]

On Fri, Feb 03, 2023 at 07:01:01AM +0000, Daniel Golle wrote:
> Set MDIO bus clock frequency and allow setting a custom maximum
> frequency from device tree.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 25 +++++++++++++++++++++
>  drivers/net/ethernet/mediatek/mtk_eth_soc.h |  5 +++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index a44ffff48c7b..9050423821dc 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -790,7 +790,9 @@ static const struct phylink_mac_ops mtk_phylink_ops = {
>  static int mtk_mdio_init(struct mtk_eth *eth)
>  {
>  	struct device_node *mii_np;
> +	int clk = 25000000, max_clk = 2500000, divider = 1;

Would be good if constant values (clk) weren't put in variables.

>  	int ret;
> +	u32 val;
>  
>  	mii_np = of_get_child_by_name(eth->dev->of_node, "mdio-bus");
>  	if (!mii_np) {
> @@ -818,6 +820,29 @@ static int mtk_mdio_init(struct mtk_eth *eth)
>  	eth->mii_bus->parent = eth->dev;
>  
>  	snprintf(eth->mii_bus->id, MII_BUS_ID_SIZE, "%pOFn", mii_np);
> +
> +	if (!of_property_read_u32(mii_np, "clock-frequency", &val))
> +		max_clk = val;

Checking for valid range? There should also probably be a dt-bindings
patch for this.

> +
> +	while (clk / divider > max_clk) {
> +		if (divider >= 63)
> +			break;
> +
> +		divider++;
> +	};

uhm, "divider = min(DIV_ROUND_UP(25000000, max_clk), 63);"? I don't
think the compiler is smart enough to optimize away this loop.

> +
> +	val = mtk_r32(eth, MTK_PPSC);
> +	val |= PPSC_MDC_TURBO;
> +	mtk_w32(eth, val, MTK_PPSC);

What does "TURBO" do and why do you set it unconditionally?

> +
> +	/* Configure MDC Divider */
> +	val = mtk_r32(eth, MTK_PPSC);
> +	val &= ~PPSC_MDC_CFG;
> +	val |= FIELD_PREP(PPSC_MDC_CFG, divider);
> +	mtk_w32(eth, val, MTK_PPSC);
> +
> +	dev_dbg(eth->dev, "MDC is running on %d Hz\n", clk / divider);
> +
>  	ret = of_mdiobus_register(eth->mii_bus, mii_np);
>  
>  err_put_node:
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> index 7230dcb29315..724815ae18a0 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> @@ -363,6 +363,11 @@
>  #define RX_DMA_VTAG_V2		BIT(0)
>  #define RX_DMA_L4_VALID_V2	BIT(2)
>  
> +/* PHY Polling and SMI Master Control registers */
> +#define MTK_PPSC		0x10000
> +#define PPSC_MDC_CFG		GENMASK(29, 24)
> +#define PPSC_MDC_TURBO		BIT(20)
> +
>  /* PHY Indirect Access Control registers */
>  #define MTK_PHY_IAC		0x10004
>  #define PHY_IAC_ACCESS		BIT(31)
> -- 
> 2.39.1
> 

Re: [PATCH 2/9] net: ethernet: mtk_eth_soc: set MDIO bus clock frequency

[Andrew Lunn]

> Checking for valid range? There should also probably be a dt-bindings
> patch for this.

This is a common mdio property in mdio.yaml. So there should not be
any need for a driver specific dt-binding.

    Andrew

Re: [PATCH 2/9] net: ethernet: mtk_eth_soc: set MDIO bus clock frequency

[Vladimir Oltean]

On Fri, Feb 03, 2023 at 11:15:15PM +0100, Andrew Lunn wrote:
> > Checking for valid range? There should also probably be a dt-bindings
> > patch for this.
> 
> This is a common mdio property in mdio.yaml. So there should not be
> any need for a driver specific dt-binding.

I meant it would probably be informative if the dt schema for this MDIO
controller had a "default: $val", "minimum: $val" and "maximum: $val".

[PATCH 3/9] net: ethernet: mtk_eth_soc: reset PCS state

[Daniel Golle]

Reset PCS state when changing interface mode.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.h | 4 ++++
 drivers/net/ethernet/mediatek/mtk_sgmii.c   | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 724815ae18a0..d39717582a85 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -546,6 +546,10 @@
 #define SGMII_SEND_AN_ERROR_EN		BIT(11)
 #define SGMII_IF_MODE_MASK		GENMASK(5, 1)
 
+/* Register to reset SGMII design */
+#define SGMII_RESERVED_0	0x34
+#define SGMII_SW_RESET		BIT(0)
+
 /* Register to set SGMII speed, ANA RG_ Control Signals III*/
 #define SGMSYS_ANA_RG_CS3	0x2028
 #define RG_PHY_SPEED_MASK	(BIT(2) | BIT(3))
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index 0fe403db23c9..f3cf66a23e72 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -88,6 +88,10 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 		regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
 				   SGMII_PHYA_PWD, SGMII_PHYA_PWD);
 
+		/* Reset SGMII PCS state */
+		regmap_update_bits(mpcs->regmap, SGMII_RESERVED_0,
+				   SGMII_SW_RESET, SGMII_SW_RESET);
+
 		if (mpcs->flags & MTK_SGMII_FLAG_PN_SWAP)
 			regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_WRAP_CTRL,
 					   SGMII_PN_SWAP_MASK,
-- 
2.39.1

[PATCH 4/9] net: ethernet: mtk_eth_soc: only write values if needed

[Daniel Golle]

Only restart auto-negotiation and write link timer if actually
necessary.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/ethernet/mediatek/mtk_sgmii.c | 24 +++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index f3cf66a23e72..58b5f2f70a66 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -41,17 +41,13 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
 	unsigned int rgc3, sgm_mode, bmcr;
 	int advertise, link_timer;
-	bool changed, use_an;
+	bool mode_changed = false, changed, use_an;
 
 	advertise = phylink_mii_c22_pcs_encode_advertisement(interface,
 							     advertising);
 	if (advertise < 0)
 		return advertise;
 
-	link_timer = phylink_get_link_timer_ns(interface);
-	if (link_timer < 0)
-		return link_timer;
-
 	/* Clearing IF_MODE_BIT0 switches the PCS to BASE-X mode, and
 	 * we assume that fixes it's speed at bitrate = line rate (in
 	 * other words, 1000Mbps or 2500Mbps).
@@ -77,8 +73,7 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 	}
 
 	if (use_an) {
-		/* FIXME: Do we need to set AN_RESTART here? */
-		bmcr = SGMII_AN_RESTART | SGMII_AN_ENABLE;
+		bmcr = SGMII_AN_ENABLE;
 	} else {
 		bmcr = 0;
 	}
@@ -106,16 +101,21 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 		regmap_update_bits(mpcs->regmap, mpcs->ana_rgc3,
 				   RG_PHY_SPEED_3_125G, rgc3);
 
+		/* Setup the link timer and QPHY power up inside SGMIISYS */
+		link_timer = phylink_get_link_timer_ns(interface);
+		if (link_timer < 0)
+			return link_timer;
+
+		regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, link_timer / 2 / 8);
+
 		mpcs->interface = interface;
+		mode_changed = true;
 	}
 
 	/* Update the advertisement, noting whether it has changed */
 	regmap_update_bits_check(mpcs->regmap, SGMSYS_PCS_ADVERTISE,
 				 SGMII_ADVERTISE, advertise, &changed);
 
-	/* Setup the link timer and QPHY power up inside SGMIISYS */
-	regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, link_timer / 2 / 8);
-
 	/* Update the sgmsys mode register */
 	regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
 			   SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN |
@@ -123,7 +123,7 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 
 	/* Update the BMCR */
 	regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
-			   SGMII_AN_RESTART | SGMII_AN_ENABLE, bmcr);
+			   SGMII_AN_ENABLE, bmcr);
 
 	/* Release PHYA power down state
 	 * Only removing bit SGMII_PHYA_PWD isn't enough.
@@ -137,7 +137,7 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 	usleep_range(50, 100);
 	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
 
-	return changed;
+	return changed || mode_changed;
 }
 
 static void mtk_pcs_restart_an(struct phylink_pcs *pcs)
-- 
2.39.1

Re: [PATCH 4/9] net: ethernet: mtk_eth_soc: only write values if needed

[Andrew Lunn]

On Fri, Feb 03, 2023 at 07:02:01AM +0000, Daniel Golle wrote:
> Only restart auto-negotiation and write link timer if actually
> necessary.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/net/ethernet/mediatek/mtk_sgmii.c | 24 +++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> index f3cf66a23e72..58b5f2f70a66 100644
> --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
> +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> @@ -41,17 +41,13 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
>  	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
>  	unsigned int rgc3, sgm_mode, bmcr;
>  	int advertise, link_timer;
> -	bool changed, use_an;
> +	bool mode_changed = false, changed, use_an;

Reverse Christmas tree .... Please check all the patches.

	Andrew

[PATCH 5/9] net: ethernet: mtk_eth_soc: fix RX data corruption issue

[Daniel Golle]

Also set bit 12 when setting up MAC MCR, as MediaTek SDK did the same
change stating:
"If without this patch, kernel might receive invalid packets that are
corrupted by GMAC."[1]

[1]: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/d8a2975939a12686c4a95c40db21efdc3f821f63
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +-
 drivers/net/ethernet/mediatek/mtk_eth_soc.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 9050423821dc..f09cd6a132c9 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -615,7 +615,7 @@ static int mtk_mac_finish(struct phylink_config *config, unsigned int mode,
 	/* Setup gmac */
 	mcr_cur = mtk_r32(mac->hw, MTK_MAC_MCR(mac->id));
 	mcr_new = mcr_cur;
-	mcr_new |= MAC_MCR_IPG_CFG | MAC_MCR_FORCE_MODE |
+	mcr_new |= MAC_MCR_IPG_CFG | MAC_MCR_BIT_12 | MAC_MCR_FORCE_MODE |
 		   MAC_MCR_BACKOFF_EN | MAC_MCR_BACKPR_EN | MAC_MCR_FORCE_LINK;
 
 	/* Only update control register when needed! */
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index d39717582a85..20d8ea20f164 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -402,6 +402,7 @@
 #define MAC_MCR_FORCE_MODE	BIT(15)
 #define MAC_MCR_TX_EN		BIT(14)
 #define MAC_MCR_RX_EN		BIT(13)
+#define MAC_MCR_BIT_12		BIT(12)
 #define MAC_MCR_BACKOFF_EN	BIT(9)
 #define MAC_MCR_BACKPR_EN	BIT(8)
 #define MAC_MCR_FORCE_RX_FC	BIT(5)
-- 
2.39.1

Re: [PATCH 5/9] net: ethernet: mtk_eth_soc: fix RX data corruption issue

[Andrew Lunn]

On Fri, Feb 03, 2023 at 07:02:28AM +0000, Daniel Golle wrote:
> Also set bit 12 when setting up MAC MCR, as MediaTek SDK did the same
> change stating:
> "If without this patch, kernel might receive invalid packets that are
> corrupted by GMAC."[1]

Do you have any idea what this bit means? It would be nice to have a
more meaningful description.

     Andrew

[PATCH 6/9] net: ethernet: mtk_eth_soc: ppe: add support for flow accounting

[Daniel Golle]

The PPE units found in MT7622 and newer support packet and byte
accounting of hw-offloaded flows. Add support for reading those counters
as found in MediaTek's SDK[1], make them accessible via debugfs and add
them to the flow offload stats.

[1]: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/bc6a6a375c800dc2b80e1a325a2c732d1737df92
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c   |   8 +-
 drivers/net/ethernet/mediatek/mtk_eth_soc.h   |   1 +
 drivers/net/ethernet/mediatek/mtk_ppe.c       | 110 +++++++++++++++++-
 drivers/net/ethernet/mediatek/mtk_ppe.h       |  24 +++-
 .../net/ethernet/mediatek/mtk_ppe_debugfs.c   |   9 +-
 .../net/ethernet/mediatek/mtk_ppe_offload.c   |   7 ++
 drivers/net/ethernet/mediatek/mtk_ppe_regs.h  |  14 +++
 7 files changed, 168 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index f09cd6a132c9..d50dea1f20f3 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -4708,7 +4708,9 @@ static int mtk_probe(struct platform_device *pdev)
 			u32 ppe_addr = eth->soc->reg_map->ppe_base + i * 0x400;
 
 			eth->ppe[i] = mtk_ppe_init(eth, eth->base + ppe_addr,
-						   eth->soc->offload_version, i);
+						   eth->soc->offload_version, i,
+						   eth->soc->has_accounting);
+
 			if (!eth->ppe[i]) {
 				err = -ENOMEM;
 				goto err_deinit_ppe;
@@ -4830,6 +4832,7 @@ static const struct mtk_soc_data mt7622_data = {
 	.required_pctl = false,
 	.offload_version = 2,
 	.hash_offset = 2,
+	.has_accounting = true,
 	.foe_entry_size = sizeof(struct mtk_foe_entry) - 16,
 	.txrx = {
 		.txd_size = sizeof(struct mtk_tx_dma),
@@ -4867,6 +4870,7 @@ static const struct mtk_soc_data mt7629_data = {
 	.hw_features = MTK_HW_FEATURES,
 	.required_clks = MT7629_CLKS_BITMAP,
 	.required_pctl = false,
+	.has_accounting = true,
 	.txrx = {
 		.txd_size = sizeof(struct mtk_tx_dma),
 		.rxd_size = sizeof(struct mtk_rx_dma),
@@ -4887,6 +4891,7 @@ static const struct mtk_soc_data mt7981_data = {
 	.offload_version = 2,
 	.hash_offset = 4,
 	.foe_entry_size = sizeof(struct mtk_foe_entry),
+	.has_accounting = true,
 	.txrx = {
 		.txd_size = sizeof(struct mtk_tx_dma_v2),
 		.rxd_size = sizeof(struct mtk_rx_dma_v2),
@@ -4907,6 +4912,7 @@ static const struct mtk_soc_data mt7986_data = {
 	.offload_version = 2,
 	.hash_offset = 4,
 	.foe_entry_size = sizeof(struct mtk_foe_entry),
+	.has_accounting = true,
 	.txrx = {
 		.txd_size = sizeof(struct mtk_tx_dma_v2),
 		.rxd_size = sizeof(struct mtk_rx_dma_v2),
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 20d8ea20f164..982482712e0a 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -1085,6 +1085,7 @@ struct mtk_soc_data {
 	u8		hash_offset;
 	u16		foe_entry_size;
 	netdev_features_t hw_features;
+	bool		has_accounting;
 	struct {
 		u32	txd_size;
 		u32	rxd_size;
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe.c b/drivers/net/ethernet/mediatek/mtk_ppe.c
index 6883eb34cd8b..26fa89afc69a 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe.c
@@ -74,6 +74,46 @@ static int mtk_ppe_wait_busy(struct mtk_ppe *ppe)
 	return ret;
 }
 
+static int mtk_ppe_mib_wait_busy(struct mtk_ppe *ppe)
+{
+	int ret;
+	u32 val;
+
+	ret = readl_poll_timeout(ppe->base + MTK_PPE_MIB_SER_CR, val,
+				 !(val & MTK_PPE_MIB_SER_CR_ST),
+				 20, MTK_PPE_WAIT_TIMEOUT_US);
+
+	if (ret)
+		dev_err(ppe->dev, "MIB table busy");
+
+	return ret;
+}
+
+static int mtk_mib_entry_read(struct mtk_ppe *ppe, u16 index, u64 *bytes, u64 *packets)
+{
+	u32 val, cnt_r0, cnt_r1, cnt_r2;
+	u32 byte_cnt_low, byte_cnt_high, pkt_cnt_low, pkt_cnt_high;
+
+	val = FIELD_PREP(MTK_PPE_MIB_SER_CR_ADDR, index) | MTK_PPE_MIB_SER_CR_ST;
+	ppe_w32(ppe, MTK_PPE_MIB_SER_CR, val);
+
+	if (mtk_ppe_mib_wait_busy(ppe))
+		return -ETIMEDOUT;
+
+	cnt_r0 = readl(ppe->base + MTK_PPE_MIB_SER_R0);
+	cnt_r1 = readl(ppe->base + MTK_PPE_MIB_SER_R1);
+	cnt_r2 = readl(ppe->base + MTK_PPE_MIB_SER_R2);
+
+	byte_cnt_low = FIELD_GET(MTK_PPE_MIB_SER_R0_BYTE_CNT_LOW, cnt_r0);
+	byte_cnt_high = FIELD_GET(MTK_PPE_MIB_SER_R1_BYTE_CNT_HIGH, cnt_r1);
+	pkt_cnt_low = FIELD_GET(MTK_PPE_MIB_SER_R1_PKT_CNT_LOW, cnt_r1);
+	pkt_cnt_high = FIELD_GET(MTK_PPE_MIB_SER_R2_PKT_CNT_HIGH, cnt_r2);
+	*bytes = ((u64)byte_cnt_high << 32) | byte_cnt_low;
+	*packets = (pkt_cnt_high << 16) | pkt_cnt_low;
+
+	return 0;
+}
+
 static void mtk_ppe_cache_clear(struct mtk_ppe *ppe)
 {
 	ppe_set(ppe, MTK_PPE_CACHE_CTL, MTK_PPE_CACHE_CTL_CLEAR);
@@ -458,6 +498,13 @@ __mtk_foe_entry_clear(struct mtk_ppe *ppe, struct mtk_flow_entry *entry)
 		hwe->ib1 &= ~MTK_FOE_IB1_STATE;
 		hwe->ib1 |= FIELD_PREP(MTK_FOE_IB1_STATE, MTK_FOE_STATE_INVALID);
 		dma_wmb();
+		if (ppe->accounting) {
+			struct mtk_foe_accounting *acct;
+
+			acct = ppe->acct_table + entry->hash * sizeof(*acct);
+			acct->packets = 0;
+			acct->bytes = 0;
+		}
 	}
 	entry->hash = 0xffff;
 
@@ -565,6 +612,9 @@ __mtk_foe_entry_commit(struct mtk_ppe *ppe, struct mtk_foe_entry *entry,
 	wmb();
 	hwe->ib1 = entry->ib1;
 
+	if (ppe->accounting)
+		*mtk_foe_entry_ib2(eth, hwe) |= MTK_FOE_IB2_MIB_CNT;
+
 	dma_wmb();
 
 	mtk_ppe_cache_clear(ppe);
@@ -756,14 +806,42 @@ int mtk_ppe_prepare_reset(struct mtk_ppe *ppe)
 	return mtk_ppe_wait_busy(ppe);
 }
 
+struct mtk_foe_accounting *mtk_foe_entry_get_mib(struct mtk_ppe *ppe, u32 index,
+						 struct mtk_foe_accounting *diff)
+{
+	struct mtk_foe_accounting *acct;
+	int size = sizeof(struct mtk_foe_accounting);
+	u64 bytes, packets;
+
+	if (!ppe->accounting)
+		return NULL;
+
+	if (mtk_mib_entry_read(ppe, index, &bytes, &packets))
+		return NULL;
+
+	acct = ppe->acct_table + index * size;
+
+	acct->bytes += bytes;
+	acct->packets += packets;
+
+	if (diff) {
+		diff->bytes = bytes;
+		diff->packets = packets;
+	}
+
+	return acct;
+}
+
 struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
-			     int version, int index)
+			     int version, int index, bool accounting)
 {
 	const struct mtk_soc_data *soc = eth->soc;
 	struct device *dev = eth->dev;
 	struct mtk_ppe *ppe;
 	u32 foe_flow_size;
 	void *foe;
+	struct mtk_mib_entry *mib;
+	struct mtk_foe_accounting *acct;
 
 	ppe = devm_kzalloc(dev, sizeof(*ppe), GFP_KERNEL);
 	if (!ppe)
@@ -778,6 +856,7 @@ struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
 	ppe->eth = eth;
 	ppe->dev = dev;
 	ppe->version = version;
+	ppe->accounting = accounting;
 
 	foe = dmam_alloc_coherent(ppe->dev,
 				  MTK_PPE_ENTRIES * soc->foe_entry_size,
@@ -793,6 +872,25 @@ struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
 	if (!ppe->foe_flow)
 		goto err_free_l2_flows;
 
+	if (accounting) {
+		mib = dmam_alloc_coherent(ppe->dev, MTK_PPE_ENTRIES * sizeof(*mib),
+					  &ppe->mib_phys, GFP_KERNEL);
+		if (!mib)
+			return NULL;
+
+		memset(mib, 0, MTK_PPE_ENTRIES * sizeof(*mib));
+
+		ppe->mib_table = mib;
+
+		acct = devm_kzalloc(dev, MTK_PPE_ENTRIES * sizeof(*acct),
+				    GFP_KERNEL);
+
+		if (!acct)
+			return NULL;
+
+		ppe->acct_table = acct;
+	}
+
 	mtk_ppe_debugfs_init(ppe, index);
 
 	return ppe;
@@ -922,6 +1020,16 @@ void mtk_ppe_start(struct mtk_ppe *ppe)
 		ppe_w32(ppe, MTK_PPE_DEFAULT_CPU_PORT1, 0xcb777);
 		ppe_w32(ppe, MTK_PPE_SBW_CTRL, 0x7f);
 	}
+
+	if (ppe->accounting && ppe->mib_phys) {
+		ppe_w32(ppe, MTK_PPE_MIB_TB_BASE, ppe->mib_phys);
+		ppe_m32(ppe, MTK_PPE_MIB_CFG, MTK_PPE_MIB_CFG_EN,
+			MTK_PPE_MIB_CFG_EN);
+		ppe_m32(ppe, MTK_PPE_MIB_CFG, MTK_PPE_MIB_CFG_RD_CLR,
+			MTK_PPE_MIB_CFG_RD_CLR);
+		ppe_m32(ppe, MTK_PPE_MIB_CACHE_CTL, MTK_PPE_MIB_CACHE_CTL_EN,
+			MTK_PPE_MIB_CFG_RD_CLR);
+	}
 }
 
 int mtk_ppe_stop(struct mtk_ppe *ppe)
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe.h b/drivers/net/ethernet/mediatek/mtk_ppe.h
index 5e8bc48252b1..db43fc9762bd 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe.h
+++ b/drivers/net/ethernet/mediatek/mtk_ppe.h
@@ -57,6 +57,7 @@ enum {
 #define MTK_FOE_IB2_MULTICAST		BIT(8)
 
 #define MTK_FOE_IB2_WDMA_QID2		GENMASK(13, 12)
+#define MTK_FOE_IB2_MIB_CNT		BIT(15)
 #define MTK_FOE_IB2_WDMA_DEVIDX		BIT(16)
 #define MTK_FOE_IB2_WDMA_WINFO		BIT(17)
 
@@ -285,16 +286,34 @@ struct mtk_flow_entry {
 	unsigned long cookie;
 };
 
+struct mtk_mib_entry {
+	u32	byt_cnt_l;
+	u16	byt_cnt_h;
+	u32	pkt_cnt_l;
+	u8	pkt_cnt_h;
+	u8	_rsv0;
+	u32	_rsv1;
+} __packed;
+
+struct mtk_foe_accounting {
+	u64	bytes;
+	u64	packets;
+};
+
 struct mtk_ppe {
 	struct mtk_eth *eth;
 	struct device *dev;
 	void __iomem *base;
 	int version;
 	char dirname[5];
+	bool accounting;
 
 	void *foe_table;
 	dma_addr_t foe_phys;
 
+	struct mtk_mib_entry *mib_table;
+	dma_addr_t mib_phys;
+
 	u16 foe_check_time[MTK_PPE_ENTRIES];
 	struct hlist_head *foe_flow;
 
@@ -304,7 +323,8 @@ struct mtk_ppe {
 };
 
 struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
-			     int version, int index);
+			     int version, int index, bool accounting);
+
 void mtk_ppe_deinit(struct mtk_eth *eth);
 void mtk_ppe_start(struct mtk_ppe *ppe);
 int mtk_ppe_stop(struct mtk_ppe *ppe);
@@ -359,5 +379,7 @@ int mtk_foe_entry_commit(struct mtk_ppe *ppe, struct mtk_flow_entry *entry);
 void mtk_foe_entry_clear(struct mtk_ppe *ppe, struct mtk_flow_entry *entry);
 int mtk_foe_entry_idle_time(struct mtk_ppe *ppe, struct mtk_flow_entry *entry);
 int mtk_ppe_debugfs_init(struct mtk_ppe *ppe, int index);
+struct mtk_foe_accounting *mtk_foe_entry_get_mib(struct mtk_ppe *ppe, u32 index,
+						 struct mtk_foe_accounting *diff);
 
 #endif
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c b/drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c
index 391b071bcff3..39775740340b 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c
@@ -82,6 +82,7 @@ mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind)
 		struct mtk_foe_entry *entry = mtk_foe_get_entry(ppe, i);
 		struct mtk_foe_mac_info *l2;
 		struct mtk_flow_addr_info ai = {};
+		struct mtk_foe_accounting *acct;
 		unsigned char h_source[ETH_ALEN];
 		unsigned char h_dest[ETH_ALEN];
 		int type, state;
@@ -95,6 +96,8 @@ mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind)
 		if (bind && state != MTK_FOE_STATE_BIND)
 			continue;
 
+		acct = mtk_foe_entry_get_mib(ppe, i, NULL);
+
 		type = FIELD_GET(MTK_FOE_IB1_PACKET_TYPE, entry->ib1);
 		seq_printf(m, "%05x %s %7s", i,
 			   mtk_foe_entry_state_str(state),
@@ -153,9 +156,11 @@ mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind)
 		*((__be16 *)&h_dest[4]) = htons(l2->dest_mac_lo);
 
 		seq_printf(m, " eth=%pM->%pM etype=%04x"
-			      " vlan=%d,%d ib1=%08x ib2=%08x\n",
+			      " vlan=%d,%d ib1=%08x ib2=%08x"
+			      " packets=%lld bytes=%lld\n",
 			   h_source, h_dest, ntohs(l2->etype),
-			   l2->vlan1, l2->vlan2, entry->ib1, ib2);
+			   l2->vlan1, l2->vlan2, entry->ib1, ib2,
+			   acct->packets, acct->bytes);
 	}
 
 	return 0;
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
index 81afd5ee3fbf..832e11ad9a16 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
@@ -497,6 +497,7 @@ static int
 mtk_flow_offload_stats(struct mtk_eth *eth, struct flow_cls_offload *f)
 {
 	struct mtk_flow_entry *entry;
+	struct mtk_foe_accounting diff;
 	u32 idle;
 
 	entry = rhashtable_lookup(&eth->flow_table, &f->cookie,
@@ -507,6 +508,12 @@ mtk_flow_offload_stats(struct mtk_eth *eth, struct flow_cls_offload *f)
 	idle = mtk_foe_entry_idle_time(eth->ppe[entry->ppe_index], entry);
 	f->stats.lastused = jiffies - idle * HZ;
 
+	if (entry->hash != 0xFFFF) {
+		mtk_foe_entry_get_mib(eth->ppe[entry->ppe_index], entry->hash, &diff);
+		f->stats.pkts += diff.packets;
+		f->stats.bytes += diff.bytes;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_regs.h b/drivers/net/ethernet/mediatek/mtk_ppe_regs.h
index 0fdb983b0a88..a2e61b3eb006 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe_regs.h
+++ b/drivers/net/ethernet/mediatek/mtk_ppe_regs.h
@@ -149,6 +149,20 @@ enum {
 
 #define MTK_PPE_MIB_TB_BASE			0x338
 
+#define MTK_PPE_MIB_SER_CR			0x33C
+#define MTK_PPE_MIB_SER_CR_ST			BIT(16)
+#define MTK_PPE_MIB_SER_CR_ADDR			GENMASK(13, 0)
+
+#define MTK_PPE_MIB_SER_R0			0x340
+#define MTK_PPE_MIB_SER_R0_BYTE_CNT_LOW		GENMASK(31, 0)
+
+#define MTK_PPE_MIB_SER_R1			0x344
+#define MTK_PPE_MIB_SER_R1_PKT_CNT_LOW		GENMASK(31, 16)
+#define MTK_PPE_MIB_SER_R1_BYTE_CNT_HIGH	GENMASK(15, 0)
+
+#define MTK_PPE_MIB_SER_R2			0x348
+#define MTK_PPE_MIB_SER_R2_PKT_CNT_HIGH		GENMASK(23, 0)
+
 #define MTK_PPE_MIB_CACHE_CTL			0x350
 #define MTK_PPE_MIB_CACHE_CTL_EN		BIT(0)
 #define MTK_PPE_MIB_CACHE_CTL_FLUSH		BIT(2)
-- 
2.39.1

Re: [PATCH 6/9] net: ethernet: mtk_eth_soc: ppe: add support for flow accounting

[Vladimir Oltean]

On Fri, Feb 03, 2023 at 07:05:08AM +0000, Daniel Golle wrote:
> The PPE units found in MT7622 and newer support packet and byte
> accounting of hw-offloaded flows. Add support for reading those counters
> as found in MediaTek's SDK[1], make them accessible via debugfs and add
> them to the flow offload stats.
> 
> [1]: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/bc6a6a375c800dc2b80e1a325a2c732d1737df92
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c   |   8 +-
>  drivers/net/ethernet/mediatek/mtk_eth_soc.h   |   1 +
>  drivers/net/ethernet/mediatek/mtk_ppe.c       | 110 +++++++++++++++++-
>  drivers/net/ethernet/mediatek/mtk_ppe.h       |  24 +++-
>  .../net/ethernet/mediatek/mtk_ppe_debugfs.c   |   9 +-
>  .../net/ethernet/mediatek/mtk_ppe_offload.c   |   7 ++
>  drivers/net/ethernet/mediatek/mtk_ppe_regs.h  |  14 +++
>  7 files changed, 168 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index f09cd6a132c9..d50dea1f20f3 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -4708,7 +4708,9 @@ static int mtk_probe(struct platform_device *pdev)
>  			u32 ppe_addr = eth->soc->reg_map->ppe_base + i * 0x400;
>  
>  			eth->ppe[i] = mtk_ppe_init(eth, eth->base + ppe_addr,
> -						   eth->soc->offload_version, i);
> +						   eth->soc->offload_version, i,
> +						   eth->soc->has_accounting);

All arguments mtk_ppe_init() needs (this includes eth->soc->offload_version)
are already available to it. See, first line in mtk_ppe_init() is:

	const struct mtk_soc_data *soc = eth->soc;

> +
>  			if (!eth->ppe[i]) {
>  				err = -ENOMEM;
>  				goto err_deinit_ppe;
> diff --git a/drivers/net/ethernet/mediatek/mtk_ppe.c b/drivers/net/ethernet/mediatek/mtk_ppe.c
> index 6883eb34cd8b..26fa89afc69a 100644
> --- a/drivers/net/ethernet/mediatek/mtk_ppe.c
> +++ b/drivers/net/ethernet/mediatek/mtk_ppe.c
> @@ -74,6 +74,46 @@ static int mtk_ppe_wait_busy(struct mtk_ppe *ppe)
>  	return ret;
>  }
>  
> +static int mtk_ppe_mib_wait_busy(struct mtk_ppe *ppe)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = readl_poll_timeout(ppe->base + MTK_PPE_MIB_SER_CR, val,
> +				 !(val & MTK_PPE_MIB_SER_CR_ST),
> +				 20, MTK_PPE_WAIT_TIMEOUT_US);
> +
> +	if (ret)
> +		dev_err(ppe->dev, "MIB table busy");
> +
> +	return ret;
> +}
> +
> +static int mtk_mib_entry_read(struct mtk_ppe *ppe, u16 index, u64 *bytes, u64 *packets)
> +{
> +	u32 val, cnt_r0, cnt_r1, cnt_r2;
> +	u32 byte_cnt_low, byte_cnt_high, pkt_cnt_low, pkt_cnt_high;
> +
> +	val = FIELD_PREP(MTK_PPE_MIB_SER_CR_ADDR, index) | MTK_PPE_MIB_SER_CR_ST;
> +	ppe_w32(ppe, MTK_PPE_MIB_SER_CR, val);
> +
> +	if (mtk_ppe_mib_wait_busy(ppe))
> +		return -ETIMEDOUT;

err = mtk_ppe_mib_wait_busy();
if (err)
	return err;

> +
> +	cnt_r0 = readl(ppe->base + MTK_PPE_MIB_SER_R0);
> +	cnt_r1 = readl(ppe->base + MTK_PPE_MIB_SER_R1);
> +	cnt_r2 = readl(ppe->base + MTK_PPE_MIB_SER_R2);
> +
> +	byte_cnt_low = FIELD_GET(MTK_PPE_MIB_SER_R0_BYTE_CNT_LOW, cnt_r0);
> +	byte_cnt_high = FIELD_GET(MTK_PPE_MIB_SER_R1_BYTE_CNT_HIGH, cnt_r1);
> +	pkt_cnt_low = FIELD_GET(MTK_PPE_MIB_SER_R1_PKT_CNT_LOW, cnt_r1);
> +	pkt_cnt_high = FIELD_GET(MTK_PPE_MIB_SER_R2_PKT_CNT_HIGH, cnt_r2);
> +	*bytes = ((u64)byte_cnt_high << 32) | byte_cnt_low;
> +	*packets = (pkt_cnt_high << 16) | pkt_cnt_low;
> +
> +	return 0;
> +}
> +
>  static void mtk_ppe_cache_clear(struct mtk_ppe *ppe)
>  {
>  	ppe_set(ppe, MTK_PPE_CACHE_CTL, MTK_PPE_CACHE_CTL_CLEAR);
>  struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
> -			     int version, int index)
> +			     int version, int index, bool accounting)
>  {
>  	const struct mtk_soc_data *soc = eth->soc;
>  	struct device *dev = eth->dev;
>  	struct mtk_ppe *ppe;
>  	u32 foe_flow_size;
>  	void *foe;
> +	struct mtk_mib_entry *mib;
> +	struct mtk_foe_accounting *acct;
>  
>  	ppe = devm_kzalloc(dev, sizeof(*ppe), GFP_KERNEL);
>  	if (!ppe)
> @@ -778,6 +856,7 @@ struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
>  	ppe->eth = eth;
>  	ppe->dev = dev;
>  	ppe->version = version;
> +	ppe->accounting = accounting;
>  
>  	foe = dmam_alloc_coherent(ppe->dev,
>  				  MTK_PPE_ENTRIES * soc->foe_entry_size,
> @@ -793,6 +872,25 @@ struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
>  	if (!ppe->foe_flow)
>  		goto err_free_l2_flows;
>  
> +	if (accounting) {
> +		mib = dmam_alloc_coherent(ppe->dev, MTK_PPE_ENTRIES * sizeof(*mib),
> +					  &ppe->mib_phys, GFP_KERNEL);
> +		if (!mib)
> +			return NULL;
> +
> +		memset(mib, 0, MTK_PPE_ENTRIES * sizeof(*mib));

I remember Jakub pointing out in another email that consistent DMA
memory is already zero-initialized, and it appears in
scripts/coccinelle/api/alloc/zalloc-simple.cocci.

> +
> +		ppe->mib_table = mib;
> +
> +		acct = devm_kzalloc(dev, MTK_PPE_ENTRIES * sizeof(*acct),
> +				    GFP_KERNEL);
> +
> +		if (!acct)
> +			return NULL;
> +
> +		ppe->acct_table = acct;
> +	}
> +
>  	mtk_ppe_debugfs_init(ppe, index);
>  
>  	return ppe;
> @@ -922,6 +1020,16 @@ void mtk_ppe_start(struct mtk_ppe *ppe)
>  		ppe_w32(ppe, MTK_PPE_DEFAULT_CPU_PORT1, 0xcb777);
>  		ppe_w32(ppe, MTK_PPE_SBW_CTRL, 0x7f);
>  	}
> +
> +	if (ppe->accounting && ppe->mib_phys) {
> +		ppe_w32(ppe, MTK_PPE_MIB_TB_BASE, ppe->mib_phys);
> +		ppe_m32(ppe, MTK_PPE_MIB_CFG, MTK_PPE_MIB_CFG_EN,
> +			MTK_PPE_MIB_CFG_EN);
> +		ppe_m32(ppe, MTK_PPE_MIB_CFG, MTK_PPE_MIB_CFG_RD_CLR,
> +			MTK_PPE_MIB_CFG_RD_CLR);
> +		ppe_m32(ppe, MTK_PPE_MIB_CACHE_CTL, MTK_PPE_MIB_CACHE_CTL_EN,
> +			MTK_PPE_MIB_CFG_RD_CLR);
> +	}
>  }
>  
>  int mtk_ppe_stop(struct mtk_ppe *ppe)
> diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c b/drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c
> index 391b071bcff3..39775740340b 100644
> --- a/drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c
> +++ b/drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c
> @@ -82,6 +82,7 @@ mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind)
>  		struct mtk_foe_entry *entry = mtk_foe_get_entry(ppe, i);
>  		struct mtk_foe_mac_info *l2;
>  		struct mtk_flow_addr_info ai = {};
> +		struct mtk_foe_accounting *acct;
>  		unsigned char h_source[ETH_ALEN];
>  		unsigned char h_dest[ETH_ALEN];
>  		int type, state;
> @@ -95,6 +96,8 @@ mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind)
>  		if (bind && state != MTK_FOE_STATE_BIND)
>  			continue;
>  
> +		acct = mtk_foe_entry_get_mib(ppe, i, NULL);

might return NULL

> +
>  		type = FIELD_GET(MTK_FOE_IB1_PACKET_TYPE, entry->ib1);
>  		seq_printf(m, "%05x %s %7s", i,
>  			   mtk_foe_entry_state_str(state),
> @@ -153,9 +156,11 @@ mtk_ppe_debugfs_foe_show(struct seq_file *m, void *private, bool bind)
>  		*((__be16 *)&h_dest[4]) = htons(l2->dest_mac_lo);
>  
>  		seq_printf(m, " eth=%pM->%pM etype=%04x"
> -			      " vlan=%d,%d ib1=%08x ib2=%08x\n",
> +			      " vlan=%d,%d ib1=%08x ib2=%08x"
> +			      " packets=%lld bytes=%lld\n",

%llu

>  			   h_source, h_dest, ntohs(l2->etype),
> -			   l2->vlan1, l2->vlan2, entry->ib1, ib2);
> +			   l2->vlan1, l2->vlan2, entry->ib1, ib2,
> +			   acct->packets, acct->bytes);
>  	}
>  
>  	return 0;
> diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> index 81afd5ee3fbf..832e11ad9a16 100644
> --- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> +++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> @@ -497,6 +497,7 @@ static int
>  mtk_flow_offload_stats(struct mtk_eth *eth, struct flow_cls_offload *f)
>  {
>  	struct mtk_flow_entry *entry;
> +	struct mtk_foe_accounting diff;
>  	u32 idle;
>  
>  	entry = rhashtable_lookup(&eth->flow_table, &f->cookie,
> @@ -507,6 +508,12 @@ mtk_flow_offload_stats(struct mtk_eth *eth, struct flow_cls_offload *f)
>  	idle = mtk_foe_entry_idle_time(eth->ppe[entry->ppe_index], entry);
>  	f->stats.lastused = jiffies - idle * HZ;
>  
> +	if (entry->hash != 0xFFFF) {
> +		mtk_foe_entry_get_mib(eth->ppe[entry->ppe_index], entry->hash, &diff);

If this returns NULL, you don't want to add diff.packets and diff.bytes
to f->stats, because no one bothers to initialize "diff" to all-zeroes.
So it contains junk from kernel stack memory. You might want to avoid this.

> +		f->stats.pkts += diff.packets;
> +		f->stats.bytes += diff.bytes;
> +	}
> +
>  	return 0;
>  }
>  

[PATCH 7/9] net: pcs: add driver for MediaTek SGMII PCS

[Daniel Golle]

The SGMII core found in several MediaTek SoCs is identical to what can
also be found in MediaTek's MT7531 Ethernet switch IC.
As this has not always been clear, both drivers developed different
implementations to deal with the PCS.

Add a dedicated driver, mostly by copying the code now found in the
Ethernet driver. The now redundant code will be removed by a follow-up
commit.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 MAINTAINERS                 |   7 +
 drivers/net/pcs/Kconfig     |   6 +
 drivers/net/pcs/Makefile    |   1 +
 drivers/net/pcs/pcs-mtk.c   | 314 ++++++++++++++++++++++++++++++++++++
 include/linux/pcs/pcs-mtk.h |  13 ++
 5 files changed, 341 insertions(+)
 create mode 100644 drivers/net/pcs/pcs-mtk.c
 create mode 100644 include/linux/pcs/pcs-mtk.h

diff --git a/MAINTAINERS b/MAINTAINERS
index abed40db41f0..a05f0cdcad89 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13000,6 +13000,13 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/mediatek/
 
+MEDIATEK ETHERNET PCS DRIVER
+M:	Daniel Golle <daniel@makrotopia.org>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	drivers/net/pcs/pcs-mtk.c
+F:	include/linux/pcs/pcs-mtk.h
+
 MEDIATEK I2C CONTROLLER DRIVER
 M:	Qii Wang <qii.wang@mediatek.com>
 L:	linux-i2c@vger.kernel.org
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 6e7e6c346a3e..cf65646656e9 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -18,6 +18,12 @@ config PCS_LYNX
 	  This module provides helpers to phylink for managing the Lynx PCS
 	  which is part of the Layerscape and QorIQ Ethernet SERDES.
 
+config PCS_MTK
+	tristate
+	help
+	  This module provides helpers to phylink for managing the LynxI PCS
+	  which is part of MediaTek's SoC and Ethernet switch ICs.
+
 config PCS_RZN1_MIIC
 	tristate "Renesas RZ/N1 MII converter"
 	depends on OF && (ARCH_RZN1 || COMPILE_TEST)
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 4c780d8f2e98..0242d95715c0 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -5,5 +5,6 @@ pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-nxp.o
 
 obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
 obj-$(CONFIG_PCS_LYNX)		+= pcs-lynx.o
+obj-$(CONFIG_PCS_MTK)		+= pcs-mtk.o
 obj-$(CONFIG_PCS_RZN1_MIIC)	+= pcs-rzn1-miic.o
 obj-$(CONFIG_PCS_ALTERA_TSE)	+= pcs-altera-tse.o
diff --git a/drivers/net/pcs/pcs-mtk.c b/drivers/net/pcs/pcs-mtk.c
new file mode 100644
index 000000000000..b4236df10815
--- /dev/null
+++ b/drivers/net/pcs/pcs-mtk.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018-2019 MediaTek Inc.
+/* A library for MediaTek SGMII circuit
+ *
+ * Author: Sean Wang <sean.wang@mediatek.com>
+ * Author: Daniel Golle <daniel@makrotopia.org>
+ *
+ */
+#include <linux/mdio.h>
+#include <linux/phylink.h>
+#include <linux/pcs/pcs-mtk.h>
+#include <linux/of.h>
+#include <linux/phylink.h>
+#include <linux/regmap.h>
+
+/* SGMII subsystem config registers */
+/* BMCR (low 16) BMSR (high 16) */
+#define SGMSYS_PCS_CONTROL_1		0x0
+#define SGMII_BMCR			GENMASK(15, 0)
+#define SGMII_BMSR			GENMASK(31, 16)
+#define SGMII_AN_RESTART		BIT(9)
+#define SGMII_ISOLATE			BIT(10)
+#define SGMII_AN_ENABLE			BIT(12)
+#define SGMII_LINK_STATYS		BIT(18)
+#define SGMII_AN_ABILITY		BIT(19)
+#define SGMII_AN_COMPLETE		BIT(21)
+#define SGMII_PCS_FAULT			BIT(23)
+#define SGMII_AN_EXPANSION_CLR		BIT(30)
+
+#define SGMSYS_PCS_DEVICE_ID		0x4
+#define SGMII_LYNXI_DEV_ID		0x4d544950
+
+#define SGMSYS_PCS_ADVERTISE		0x8
+#define SGMII_ADVERTISE			GENMASK(15, 0)
+#define SGMII_LPA			GENMASK(31, 16)
+
+#define SGMSYS_PCS_SCRATCH		0x14
+#define SGMII_DEV_VERSION		GENMASK(31, 16)
+
+/* Register to programmable link timer, the unit in 2 * 8ns */
+#define SGMSYS_PCS_LINK_TIMER		0x18
+#define SGMII_LINK_TIMER_MASK		GENMASK(19, 0)
+#define SGMII_LINK_TIMER_DEFAULT	(0x186a0 & SGMII_LINK_TIMER_MASK)
+
+/* Register to control remote fault */
+#define SGMSYS_SGMII_MODE		0x20
+#define SGMII_IF_MODE_SGMII		BIT(0)
+#define SGMII_SPEED_DUPLEX_AN		BIT(1)
+#define SGMII_SPEED_MASK		GENMASK(3, 2)
+#define SGMII_SPEED_10			FIELD_PREP(SGMII_SPEED_MASK, 0)
+#define SGMII_SPEED_100			FIELD_PREP(SGMII_SPEED_MASK, 1)
+#define SGMII_SPEED_1000		FIELD_PREP(SGMII_SPEED_MASK, 2)
+#define SGMII_DUPLEX_HALF		BIT(4)
+#define SGMII_REMOTE_FAULT_DIS		BIT(8)
+#define SGMII_CODE_SYNC_SET_VAL		BIT(9)
+#define SGMII_CODE_SYNC_SET_EN		BIT(10)
+#define SGMII_SEND_AN_ERROR_EN		BIT(11)
+
+/* Register to reset SGMII design */
+#define SGMII_RESERVED_0		0x34
+#define SGMII_SW_RESET			BIT(0)
+
+/* Register to set SGMII speed, ANA RG_ Control Signals III */
+#define SGMSYS_ANA_RG_CS3		0x2028
+#define RG_PHY_SPEED_MASK		(BIT(2) | BIT(3))
+#define RG_PHY_SPEED_1_25G		0x0
+#define RG_PHY_SPEED_3_125G		BIT(2)
+
+/* Register to power up QPHY */
+#define SGMSYS_QPHY_PWR_STATE_CTRL	0xe8
+#define	SGMII_PHYA_PWD			BIT(4)
+
+/* Register to QPHY wrapper control */
+#define SGMSYS_QPHY_WRAP_CTRL		0xec
+#define SGMII_PN_SWAP_MASK		GENMASK(1, 0)
+#define SGMII_PN_SWAP_TX_RX		(BIT(0) | BIT(1))
+
+/* struct mtk_pcs -    This structure holds each sgmii regmap and associated
+ *                     data
+ * @regmap:            The register map pointing at the range used to setup
+ *                     SGMII modes
+ * @dev:               Pointer to device owning the PCS
+ * @ana_rgc3:          The offset refers to register ANA_RGC3 related to regmap
+ * @interface:         Currently configured interface mode
+ * @pcs:               Phylink PCS structure
+ * @flags:             Flags indicating hardware properties
+ */
+struct mtk_pcs {
+	struct regmap		*regmap;
+	struct device		*dev;
+	u32			ana_rgc3;
+	phy_interface_t		interface;
+	struct			phylink_pcs pcs;
+	u32			flags;
+};
+
+static struct mtk_pcs *pcs_to_mtk_pcs(struct phylink_pcs *pcs)
+{
+	return container_of(pcs, struct mtk_pcs, pcs);
+}
+
+static void mtk_pcs_get_state(struct phylink_pcs *pcs,
+			      struct phylink_link_state *state)
+{
+	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
+	unsigned int bm, adv;
+
+	/* Read the BMSR and LPA */
+	regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &bm);
+	regmap_read(mpcs->regmap, SGMSYS_PCS_ADVERTISE, &adv);
+
+	phylink_mii_c22_pcs_decode_state(state, FIELD_GET(SGMII_BMSR, bm),
+					 FIELD_GET(SGMII_LPA, adv));
+}
+
+static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+			  phy_interface_t interface,
+			  const unsigned long *advertising,
+			  bool permit_pause_to_mac)
+{
+	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
+	unsigned int rgc3, sgm_mode, bmcr;
+	int advertise, link_timer;
+	bool mode_changed = false, changed, use_an;
+
+	advertise = phylink_mii_c22_pcs_encode_advertisement(interface,
+							     advertising);
+	if (advertise < 0)
+		return advertise;
+
+	/* Clearing IF_MODE_BIT0 switches the PCS to BASE-X mode, and
+	 * we assume that fixes it's speed at bitrate = line rate (in
+	 * other words, 1000Mbps or 2500Mbps).
+	 */
+	if (interface == PHY_INTERFACE_MODE_SGMII) {
+		sgm_mode = SGMII_IF_MODE_SGMII;
+		if (phylink_autoneg_inband(mode)) {
+			sgm_mode |= SGMII_REMOTE_FAULT_DIS |
+				    SGMII_SPEED_DUPLEX_AN;
+			use_an = true;
+		} else {
+			use_an = false;
+		}
+	} else if (phylink_autoneg_inband(mode)) {
+		/* 1000base-X or 2500base-X autoneg */
+		sgm_mode = SGMII_REMOTE_FAULT_DIS;
+		use_an = linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+					   advertising);
+	} else {
+		/* 1000base-X or 2500base-X without autoneg */
+		sgm_mode = 0;
+		use_an = false;
+	}
+
+	if (use_an)
+		bmcr = SGMII_AN_ENABLE;
+	else
+		bmcr = 0;
+
+	if (mpcs->interface != interface) {
+		/* PHYA power down */
+		regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
+				   SGMII_PHYA_PWD, SGMII_PHYA_PWD);
+
+		/* Reset SGMII PCS state */
+		regmap_update_bits(mpcs->regmap, SGMII_RESERVED_0,
+				   SGMII_SW_RESET, SGMII_SW_RESET);
+
+		if (mpcs->flags & MTK_SGMII_FLAG_PN_SWAP)
+			regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_WRAP_CTRL,
+					   SGMII_PN_SWAP_MASK,
+					   SGMII_PN_SWAP_TX_RX);
+
+		if (interface == PHY_INTERFACE_MODE_2500BASEX)
+			rgc3 = RG_PHY_SPEED_3_125G;
+		else
+			rgc3 = 0;
+
+		/* Configure the underlying interface speed */
+		regmap_update_bits(mpcs->regmap, mpcs->ana_rgc3,
+				   RG_PHY_SPEED_MASK, rgc3);
+
+		/* Setup the link timer and QPHY power up inside SGMIISYS */
+		link_timer = phylink_get_link_timer_ns(interface);
+		if (link_timer < 0)
+			return link_timer;
+
+		regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, link_timer / 2 / 8);
+
+		mpcs->interface = interface;
+		mode_changed = true;
+	}
+
+	/* Update the advertisement, noting whether it has changed */
+	regmap_update_bits_check(mpcs->regmap, SGMSYS_PCS_ADVERTISE,
+				 SGMII_ADVERTISE, advertise, &changed);
+
+	/* Update the sgmsys mode register */
+	regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
+			   SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN |
+			   SGMII_IF_MODE_SGMII, sgm_mode);
+
+	/* Update the BMCR */
+	regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
+			   SGMII_AN_ENABLE, bmcr);
+
+	/* Release PHYA power down state
+	 * Only removing bit SGMII_PHYA_PWD isn't enough.
+	 * There are cases when the SGMII_PHYA_PWD register contains 0x9 which
+	 * prevents SGMII from working. The SGMII still shows link but no traffic
+	 * can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was
+	 * taken from a good working state of the SGMII interface.
+	 * Unknown how much the QPHY needs but it is racy without a sleep.
+	 * Tested on mt7622 & mt7986.
+	 */
+	usleep_range(50, 100);
+	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
+
+	return changed || mode_changed;
+}
+
+static void mtk_pcs_restart_an(struct phylink_pcs *pcs)
+{
+	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
+
+	regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
+			   SGMII_AN_RESTART, SGMII_AN_RESTART);
+}
+
+static void mtk_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
+			    phy_interface_t interface, int speed, int duplex)
+{
+	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
+	unsigned int sgm_mode;
+
+	if (!phylink_autoneg_inband(mode)) {
+		/* Force the speed and duplex setting */
+		if (speed == SPEED_10)
+			sgm_mode = SGMII_SPEED_10;
+		else if (speed == SPEED_100)
+			sgm_mode = SGMII_SPEED_100;
+		else
+			sgm_mode = SGMII_SPEED_1000;
+
+		if (duplex != DUPLEX_FULL)
+			sgm_mode |= SGMII_DUPLEX_HALF;
+
+		regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
+				   SGMII_DUPLEX_HALF | SGMII_SPEED_MASK,
+				   sgm_mode);
+	}
+}
+
+static const struct phylink_pcs_ops mtk_pcs_ops = {
+	.pcs_get_state = mtk_pcs_get_state,
+	.pcs_config = mtk_pcs_config,
+	.pcs_an_restart = mtk_pcs_restart_an,
+	.pcs_link_up = mtk_pcs_link_up,
+};
+
+struct phylink_pcs *mtk_pcs_create(struct device *dev, struct regmap *regmap,
+				   u32 ana_rgc3, u32 flags)
+{
+	struct mtk_pcs *mpcs;
+	u32 id, ver;
+	int ret;
+
+	ret = regmap_read(regmap, SGMSYS_PCS_DEVICE_ID, &id);
+	if (ret < 0)
+		return NULL;
+
+	if (id != SGMII_LYNXI_DEV_ID) {
+		dev_err(dev, "unknown PCS device id %08x\n", id);
+		return NULL;
+	}
+
+	ret = regmap_read(regmap, SGMSYS_PCS_SCRATCH, &ver);
+	if (ret < 0)
+		return NULL;
+
+	ver = FIELD_GET(SGMII_DEV_VERSION, ver);
+	if (ver != 0x1) {
+		dev_err(dev, "unknown PCS device version %04x\n", ver);
+		return NULL;
+	}
+
+	dev_dbg(dev, "MediaTek LynxI SGMII PCS (id 0x%08x, ver 0x%04x)\n", id,
+		ver);
+
+	mpcs = kzalloc(sizeof(*mpcs), GFP_KERNEL);
+	if (!mpcs)
+		return NULL;
+
+	mpcs->dev = dev;
+	mpcs->ana_rgc3 = ana_rgc3;
+	mpcs->regmap = regmap;
+	mpcs->flags = flags;
+	mpcs->pcs.ops = &mtk_pcs_ops;
+	mpcs->pcs.poll = true;
+	mpcs->interface = PHY_INTERFACE_MODE_NA;
+
+	return &mpcs->pcs;
+}
+EXPORT_SYMBOL(mtk_pcs_create);
+
+void mtk_pcs_destroy(struct phylink_pcs *pcs)
+{
+	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
+
+	kfree(mpcs);
+}
+EXPORT_SYMBOL(mtk_pcs_destroy);
+
+MODULE_LICENSE("GPL");
diff --git a/include/linux/pcs/pcs-mtk.h b/include/linux/pcs/pcs-mtk.h
new file mode 100644
index 000000000000..d75cd114b859
--- /dev/null
+++ b/include/linux/pcs/pcs-mtk.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_PCS_MTK_H
+#define __LINUX_PCS_MTK_H
+
+#include <linux/phylink.h>
+#include <linux/regmap.h>
+
+#define MTK_SGMII_FLAG_PN_SWAP BIT(0)
+struct phylink_pcs *mtk_pcs_create(struct device *dev, struct regmap *regmap,
+				   u32 ana_rgc3, u32 flags);
+void mtk_pcs_destroy(struct phylink_pcs *pcs);
+
+#endif
-- 
2.39.1

Re: [PATCH 7/9] net: pcs: add driver for MediaTek SGMII PCS

[Andrew Lunn]

> index 6e7e6c346a3e..cf65646656e9 100644
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
> @@ -18,6 +18,12 @@ config PCS_LYNX
>  	  This module provides helpers to phylink for managing the Lynx PCS
>  	  which is part of the Layerscape and QorIQ Ethernet SERDES.
>  
> +config PCS_MTK
> +	tristate
> +	help
> +	  This module provides helpers to phylink for managing the LynxI PCS
> +	  which is part of MediaTek's SoC and Ethernet switch ICs.

You should probably have a more specific name, for when MTK produces a
new PCS which is completely different.

Also, how similar is this LynxI PCS to the Lynx PCS?

      Andrew

Re: [PATCH 7/9] net: pcs: add driver for MediaTek SGMII PCS

[Vladimir Oltean]

On Fri, Feb 03, 2023 at 03:14:29PM +0100, Andrew Lunn wrote:
> > index 6e7e6c346a3e..cf65646656e9 100644
> > --- a/drivers/net/pcs/Kconfig
> > +++ b/drivers/net/pcs/Kconfig
> > @@ -18,6 +18,12 @@ config PCS_LYNX
> >  	  This module provides helpers to phylink for managing the Lynx PCS
> >  	  which is part of the Layerscape and QorIQ Ethernet SERDES.
> >  
> > +config PCS_MTK
> > +	tristate
> > +	help
> > +	  This module provides helpers to phylink for managing the LynxI PCS
> > +	  which is part of MediaTek's SoC and Ethernet switch ICs.
> 
> You should probably have a more specific name, for when MTK produces a
> new PCS which is completely different.
> 
> Also, how similar is this LynxI PCS to the Lynx PCS?

Probably not very similar. Here's the Mediatek 32-bit memory map,
translated by me to a 16-bit MDIO memory map:

/* SGMII subsystem config registers */
/* BMCR (low 16) BMSR (high 16) */
#define SGMSYS_PCS_CONTROL_1		0x0		// BMCR at MDIO addr 0x0, BMSR at 0x1, aka standard

#define SGMSYS_PCS_DEVICE_ID		0x4		// PHYSID1 at 0x2, PHYSID2 at 0x3, aka standard

#define SGMSYS_PCS_ADVERTISE		0x8		// MII_ADV at 0x4, MII_LPA at 0x5

#define SGMSYS_PCS_SCRATCH		0x14		// MDIO address 0xa

/* Register to programmable link timer, the unit in 2 * 8ns */
#define SGMSYS_PCS_LINK_TIMER		0x18		// MDIO address 0xc

/* Register to control remote fault */
#define SGMSYS_SGMII_MODE		0x20		// MDIO address 0x10

/* Register to reset SGMII design */
#define SGMII_RESERVED_0		0x34		// MDIO address 0x1a

/* Register to set SGMII speed, ANA RG_ Control Signals III */
#define SGMSYS_ANA_RG_CS3		0x2028		// not sure how to access this through C22, OTOH not used?

/* Register to power up QPHY */
#define SGMSYS_QPHY_PWR_STATE_CTRL	0xe8		// again, not sure how to access through C22


Compared to these definitions for Lynx, the rest being standard regs:

#define LINK_TIMER_LO			0x12
#define LINK_TIMER_HI			0x13
#define IF_MODE				0x14

So the standard bits appear to be common, the vendor extensions different.
When I say common, I only take into consideration the memory map, not
the differences in handling. For example, what Lynx handles as a single
call to phylink_mii_c22_pcs_get_state(), the Mediatek PCS handles as a
call to mtk_pcs_get_state().

Re: [PATCH 7/9] net: pcs: add driver for MediaTek SGMII PCS

[Andrew Lunn]

On Fri, Feb 03, 2023 at 05:00:14PM +0200, Vladimir Oltean wrote:
> On Fri, Feb 03, 2023 at 03:14:29PM +0100, Andrew Lunn wrote:
> > > index 6e7e6c346a3e..cf65646656e9 100644
> > > --- a/drivers/net/pcs/Kconfig
> > > +++ b/drivers/net/pcs/Kconfig
> > > @@ -18,6 +18,12 @@ config PCS_LYNX
> > >  	  This module provides helpers to phylink for managing the Lynx PCS
> > >  	  which is part of the Layerscape and QorIQ Ethernet SERDES.
> > >  
> > > +config PCS_MTK
> > > +	tristate
> > > +	help
> > > +	  This module provides helpers to phylink for managing the LynxI PCS
> > > +	  which is part of MediaTek's SoC and Ethernet switch ICs.
> > 
> > You should probably have a more specific name, for when MTK produces a
> > new PCS which is completely different.
> > 
> > Also, how similar is this LynxI PCS to the Lynx PCS?
> 
> Probably not very similar. Here's the Mediatek 32-bit memory map,
> translated by me to a 16-bit MDIO memory map:

Thanks. Given the similarities in the name, i had to ask...

	Andrew

[PATCH 8/9] net: ethernet: mtk_eth_soc: switch to external PCS driver

[Daniel Golle]

Now that we got a PCS driver, use it and remove the now redundant
PCS code and its header macros from the Ethernet driver.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/ethernet/mediatek/Kconfig       |   2 +
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |  13 +-
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |  80 +-------
 drivers/net/ethernet/mediatek/mtk_sgmii.c   | 200 +++-----------------
 4 files changed, 37 insertions(+), 258 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/Kconfig b/drivers/net/ethernet/mediatek/Kconfig
index 97374fb3ee79..8f9d2f663885 100644
--- a/drivers/net/ethernet/mediatek/Kconfig
+++ b/drivers/net/ethernet/mediatek/Kconfig
@@ -19,6 +19,8 @@ config NET_MEDIATEK_SOC
 	select DIMLIB
 	select PAGE_POOL
 	select PAGE_POOL_STATS
+	select PCS_MTK
+	select REGMAP_MMIO
 	help
 	  This driver supports the gigabit ethernet MACs in the
 	  MediaTek SoC family.
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index d50dea1f20f3..5c833b82c93e 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -4081,6 +4081,7 @@ static int mtk_unreg_dev(struct mtk_eth *eth)
 
 static int mtk_cleanup(struct mtk_eth *eth)
 {
+	mtk_sgmii_destroy(eth->sgmii);
 	mtk_unreg_dev(eth);
 	mtk_free_dev(eth);
 	cancel_work_sync(&eth->pending_work);
@@ -4578,6 +4579,7 @@ static int mtk_probe(struct platform_device *pdev)
 		if (!eth->sgmii)
 			return -ENOMEM;
 
+		eth->sgmii->dev = eth->dev;
 		err = mtk_sgmii_init(eth->sgmii, pdev->dev.of_node,
 				     eth->soc->ana_rgc3);
 
@@ -4590,14 +4592,17 @@ static int mtk_probe(struct platform_device *pdev)
 							    "mediatek,pctl");
 		if (IS_ERR(eth->pctl)) {
 			dev_err(&pdev->dev, "no pctl regmap found\n");
-			return PTR_ERR(eth->pctl);
+			err = PTR_ERR(eth->pctl);
+			goto err_destroy_sgmii;
 		}
 	}
 
 	if (MTK_HAS_CAPS(eth->soc->caps, MTK_NETSYS_V2)) {
 		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-		if (!res)
-			return -EINVAL;
+		if (!res) {
+			err = -EINVAL;
+			goto err_destroy_sgmii;
+		}
 	}
 
 	if (eth->soc->offload_version) {
@@ -4749,6 +4754,8 @@ static int mtk_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_destroy_sgmii:
+	mtk_sgmii_destroy(eth->sgmii);
 err_deinit_ppe:
 	mtk_ppe_deinit(eth);
 	mtk_mdio_cleanup(eth);
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 982482712e0a..46d76ca4ad2d 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -508,65 +508,6 @@
 #define ETHSYS_DMA_AG_MAP_QDMA	BIT(1)
 #define ETHSYS_DMA_AG_MAP_PPE	BIT(2)
 
-/* SGMII subsystem config registers */
-/* BMCR (low 16) BMSR (high 16) */
-#define SGMSYS_PCS_CONTROL_1	0x0
-#define SGMII_BMCR		GENMASK(15, 0)
-#define SGMII_BMSR		GENMASK(31, 16)
-#define SGMII_AN_RESTART	BIT(9)
-#define SGMII_ISOLATE		BIT(10)
-#define SGMII_AN_ENABLE		BIT(12)
-#define SGMII_LINK_STATYS	BIT(18)
-#define SGMII_AN_ABILITY	BIT(19)
-#define SGMII_AN_COMPLETE	BIT(21)
-#define SGMII_PCS_FAULT		BIT(23)
-#define SGMII_AN_EXPANSION_CLR	BIT(30)
-
-#define SGMSYS_PCS_ADVERTISE	0x8
-#define SGMII_ADVERTISE		GENMASK(15, 0)
-#define SGMII_LPA		GENMASK(31, 16)
-
-/* Register to programmable link timer, the unit in 2 * 8ns */
-#define SGMSYS_PCS_LINK_TIMER	0x18
-#define SGMII_LINK_TIMER_MASK	GENMASK(19, 0)
-#define SGMII_LINK_TIMER_DEFAULT	(0x186a0 & SGMII_LINK_TIMER_MASK)
-
-/* Register to control remote fault */
-#define SGMSYS_SGMII_MODE		0x20
-#define SGMII_IF_MODE_SGMII		BIT(0)
-#define SGMII_SPEED_DUPLEX_AN		BIT(1)
-#define SGMII_SPEED_MASK		GENMASK(3, 2)
-#define SGMII_SPEED_10			FIELD_PREP(SGMII_SPEED_MASK, 0)
-#define SGMII_SPEED_100			FIELD_PREP(SGMII_SPEED_MASK, 1)
-#define SGMII_SPEED_1000		FIELD_PREP(SGMII_SPEED_MASK, 2)
-#define SGMII_DUPLEX_HALF		BIT(4)
-#define SGMII_IF_MODE_BIT5		BIT(5)
-#define SGMII_REMOTE_FAULT_DIS		BIT(8)
-#define SGMII_CODE_SYNC_SET_VAL		BIT(9)
-#define SGMII_CODE_SYNC_SET_EN		BIT(10)
-#define SGMII_SEND_AN_ERROR_EN		BIT(11)
-#define SGMII_IF_MODE_MASK		GENMASK(5, 1)
-
-/* Register to reset SGMII design */
-#define SGMII_RESERVED_0	0x34
-#define SGMII_SW_RESET		BIT(0)
-
-/* Register to set SGMII speed, ANA RG_ Control Signals III*/
-#define SGMSYS_ANA_RG_CS3	0x2028
-#define RG_PHY_SPEED_MASK	(BIT(2) | BIT(3))
-#define RG_PHY_SPEED_1_25G	0x0
-#define RG_PHY_SPEED_3_125G	BIT(2)
-
-/* Register to power up QPHY */
-#define SGMSYS_QPHY_PWR_STATE_CTRL 0xe8
-#define	SGMII_PHYA_PWD		BIT(4)
-
-/* Register to QPHY wrapper control */
-#define SGMSYS_QPHY_WRAP_CTRL	0xec
-#define SGMII_PN_SWAP_MASK	GENMASK(1, 0)
-#define SGMII_PN_SWAP_TX_RX	(BIT(0) | BIT(1))
-#define MTK_SGMII_FLAG_PN_SWAP	BIT(0)
-
 /* Infrasys subsystem config registers */
 #define INFRA_MISC2            0x70c
 #define CO_QPHY_SEL            BIT(0)
@@ -1101,29 +1042,13 @@ struct mtk_soc_data {
 /* currently no SoC has more than 2 macs */
 #define MTK_MAX_DEVS			2
 
-/* struct mtk_pcs -    This structure holds each sgmii regmap and associated
- *                     data
- * @regmap:            The register map pointing at the range used to setup
- *                     SGMII modes
- * @ana_rgc3:          The offset refers to register ANA_RGC3 related to regmap
- * @interface:         Currently configured interface mode
- * @pcs:               Phylink PCS structure
- * @flags:             Flags indicating hardware properties
- */
-struct mtk_pcs {
-	struct regmap	*regmap;
-	u32             ana_rgc3;
-	phy_interface_t	interface;
-	struct phylink_pcs pcs;
-	u32		flags;
-};
-
 /* struct mtk_sgmii -  This is the structure holding sgmii regmap and its
  *                     characteristics
  * @pcs                Array of individual PCS structures
  */
 struct mtk_sgmii {
-	struct mtk_pcs	pcs[MTK_MAX_DEVS];
+	struct phylink_pcs *pcs[MTK_MAX_DEVS];
+	struct device *dev;
 };
 
 /* struct mtk_eth -	This is the main datasructure for holding the state
@@ -1351,6 +1276,7 @@ u32 mtk_r32(struct mtk_eth *eth, unsigned reg);
 struct phylink_pcs *mtk_sgmii_select_pcs(struct mtk_sgmii *ss, int id);
 int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *np,
 		   u32 ana_rgc3);
+void mtk_sgmii_destroy(struct mtk_sgmii *ss);
 
 int mtk_gmac_sgmii_path_setup(struct mtk_eth *eth, int mac_id);
 int mtk_gmac_gephy_path_setup(struct mtk_eth *eth, int mac_id);
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
index 58b5f2f70a66..d2c320c9746b 100644
--- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
+++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
@@ -10,199 +10,34 @@
 #include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/phylink.h>
+#include <linux/pcs/pcs-mtk.h>
 #include <linux/regmap.h>
 
 #include "mtk_eth_soc.h"
 
-static struct mtk_pcs *pcs_to_mtk_pcs(struct phylink_pcs *pcs)
-{
-	return container_of(pcs, struct mtk_pcs, pcs);
-}
-
-static void mtk_pcs_get_state(struct phylink_pcs *pcs,
-			      struct phylink_link_state *state)
-{
-	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
-	unsigned int bm, adv;
-
-	/* Read the BMSR and LPA */
-	regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &bm);
-	regmap_read(mpcs->regmap, SGMSYS_PCS_ADVERTISE, &adv);
-
-	phylink_mii_c22_pcs_decode_state(state, FIELD_GET(SGMII_BMSR, bm),
-					 FIELD_GET(SGMII_LPA, adv));
-}
-
-static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
-			  phy_interface_t interface,
-			  const unsigned long *advertising,
-			  bool permit_pause_to_mac)
-{
-	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
-	unsigned int rgc3, sgm_mode, bmcr;
-	int advertise, link_timer;
-	bool mode_changed = false, changed, use_an;
-
-	advertise = phylink_mii_c22_pcs_encode_advertisement(interface,
-							     advertising);
-	if (advertise < 0)
-		return advertise;
-
-	/* Clearing IF_MODE_BIT0 switches the PCS to BASE-X mode, and
-	 * we assume that fixes it's speed at bitrate = line rate (in
-	 * other words, 1000Mbps or 2500Mbps).
-	 */
-	if (interface == PHY_INTERFACE_MODE_SGMII) {
-		sgm_mode = SGMII_IF_MODE_SGMII;
-		if (phylink_autoneg_inband(mode)) {
-			sgm_mode |= SGMII_REMOTE_FAULT_DIS |
-				    SGMII_SPEED_DUPLEX_AN;
-			use_an = true;
-		} else {
-			use_an = false;
-		}
-	} else if (phylink_autoneg_inband(mode)) {
-		/* 1000base-X or 2500base-X autoneg */
-		sgm_mode = SGMII_REMOTE_FAULT_DIS;
-		use_an = linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
-					   advertising);
-	} else {
-		/* 1000base-X or 2500base-X without autoneg */
-		sgm_mode = 0;
-		use_an = false;
-	}
-
-	if (use_an) {
-		bmcr = SGMII_AN_ENABLE;
-	} else {
-		bmcr = 0;
-	}
-
-	if (mpcs->interface != interface) {
-		/* PHYA power down */
-		regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
-				   SGMII_PHYA_PWD, SGMII_PHYA_PWD);
-
-		/* Reset SGMII PCS state */
-		regmap_update_bits(mpcs->regmap, SGMII_RESERVED_0,
-				   SGMII_SW_RESET, SGMII_SW_RESET);
-
-		if (mpcs->flags & MTK_SGMII_FLAG_PN_SWAP)
-			regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_WRAP_CTRL,
-					   SGMII_PN_SWAP_MASK,
-					   SGMII_PN_SWAP_TX_RX);
-
-		if (interface == PHY_INTERFACE_MODE_2500BASEX)
-			rgc3 = RG_PHY_SPEED_3_125G;
-		else
-			rgc3 = 0;
-
-		/* Configure the underlying interface speed */
-		regmap_update_bits(mpcs->regmap, mpcs->ana_rgc3,
-				   RG_PHY_SPEED_3_125G, rgc3);
-
-		/* Setup the link timer and QPHY power up inside SGMIISYS */
-		link_timer = phylink_get_link_timer_ns(interface);
-		if (link_timer < 0)
-			return link_timer;
-
-		regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, link_timer / 2 / 8);
-
-		mpcs->interface = interface;
-		mode_changed = true;
-	}
-
-	/* Update the advertisement, noting whether it has changed */
-	regmap_update_bits_check(mpcs->regmap, SGMSYS_PCS_ADVERTISE,
-				 SGMII_ADVERTISE, advertise, &changed);
-
-	/* Update the sgmsys mode register */
-	regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
-			   SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN |
-			   SGMII_IF_MODE_SGMII, sgm_mode);
-
-	/* Update the BMCR */
-	regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
-			   SGMII_AN_ENABLE, bmcr);
-
-	/* Release PHYA power down state
-	 * Only removing bit SGMII_PHYA_PWD isn't enough.
-	 * There are cases when the SGMII_PHYA_PWD register contains 0x9 which
-	 * prevents SGMII from working. The SGMII still shows link but no traffic
-	 * can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was
-	 * taken from a good working state of the SGMII interface.
-	 * Unknown how much the QPHY needs but it is racy without a sleep.
-	 * Tested on mt7622 & mt7986.
-	 */
-	usleep_range(50, 100);
-	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
-
-	return changed || mode_changed;
-}
-
-static void mtk_pcs_restart_an(struct phylink_pcs *pcs)
-{
-	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
-
-	regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
-			   SGMII_AN_RESTART, SGMII_AN_RESTART);
-}
-
-static void mtk_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
-			    phy_interface_t interface, int speed, int duplex)
-{
-	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
-	unsigned int sgm_mode;
-
-	if (!phylink_autoneg_inband(mode)) {
-		/* Force the speed and duplex setting */
-		if (speed == SPEED_10)
-			sgm_mode = SGMII_SPEED_10;
-		else if (speed == SPEED_100)
-			sgm_mode = SGMII_SPEED_100;
-		else
-			sgm_mode = SGMII_SPEED_1000;
-
-		if (duplex != DUPLEX_FULL)
-			sgm_mode |= SGMII_DUPLEX_HALF;
-
-		regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
-				   SGMII_DUPLEX_HALF | SGMII_SPEED_MASK,
-				   sgm_mode);
-	}
-}
-
-static const struct phylink_pcs_ops mtk_pcs_ops = {
-	.pcs_get_state = mtk_pcs_get_state,
-	.pcs_config = mtk_pcs_config,
-	.pcs_an_restart = mtk_pcs_restart_an,
-	.pcs_link_up = mtk_pcs_link_up,
-};
-
 int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)
 {
 	struct device_node *np;
 	int i;
+	u32 flags;
+	struct regmap *regmap;
 
 	for (i = 0; i < MTK_MAX_DEVS; i++) {
 		np = of_parse_phandle(r, "mediatek,sgmiisys", i);
 		if (!np)
 			break;
 
-		ss->pcs[i].ana_rgc3 = ana_rgc3;
-		ss->pcs[i].regmap = syscon_node_to_regmap(np);
-
-		ss->pcs[i].flags = 0;
+		flags = 0;
 		if (of_property_read_bool(np, "pn_swap"))
-			ss->pcs[i].flags |= MTK_SGMII_FLAG_PN_SWAP;
+			flags |= MTK_SGMII_FLAG_PN_SWAP;
 
 		of_node_put(np);
-		if (IS_ERR(ss->pcs[i].regmap))
-			return PTR_ERR(ss->pcs[i].regmap);
 
-		ss->pcs[i].pcs.ops = &mtk_pcs_ops;
-		ss->pcs[i].pcs.poll = true;
-		ss->pcs[i].interface = PHY_INTERFACE_MODE_NA;
+		regmap = syscon_node_to_regmap(np);
+		if (IS_ERR(regmap))
+			return PTR_ERR(regmap);
+
+		ss->pcs[i] = mtk_pcs_create(ss->dev, regmap, ana_rgc3, flags);
 	}
 
 	return 0;
@@ -210,8 +45,17 @@ int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)
 
 struct phylink_pcs *mtk_sgmii_select_pcs(struct mtk_sgmii *ss, int id)
 {
-	if (!ss->pcs[id].regmap)
-		return NULL;
+	return ss->pcs[id];
+}
+
+void mtk_sgmii_destroy(struct mtk_sgmii *ss)
+{
+	int i;
+
+	if (!ss)
+		return;
 
-	return &ss->pcs[id].pcs;
+	for (i = 0; i < MTK_MAX_DEVS; i++)
+		if (ss->pcs[i])
+			mtk_pcs_destroy(ss->pcs[i]);
 }
-- 
2.39.1

Re: [PATCH 8/9] net: ethernet: mtk_eth_soc: switch to external PCS driver

[Bjørn Mork]

Daniel Golle <daniel@makrotopia.org> writes:

> -		ss->pcs[i].ana_rgc3 = ana_rgc3;
> -		ss->pcs[i].regmap = syscon_node_to_regmap(np);
> -
> -		ss->pcs[i].flags = 0;
> +		flags = 0;
>  		if (of_property_read_bool(np, "pn_swap"))
> -			ss->pcs[i].flags |= MTK_SGMII_FLAG_PN_SWAP;
> +			flags |= MTK_SGMII_FLAG_PN_SWAP;
>  

patch 1 added "mediatek,pn_swap" so this doesn't apply.  We've all done
last minute cleanups - never a good idea :-)


Bjørn

Re: [PATCH 8/9] net: ethernet: mtk_eth_soc: switch to external PCS driver

[Vladimir Oltean]

On Fri, Feb 03, 2023 at 07:06:10AM +0000, Daniel Golle wrote:
>  int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)
>  {
>  	struct device_node *np;
>  	int i;
> +	u32 flags;
> +	struct regmap *regmap;
>  
>  	for (i = 0; i < MTK_MAX_DEVS; i++) {
>  		np = of_parse_phandle(r, "mediatek,sgmiisys", i);
>  		if (!np)
>  			break;
>  
> -		ss->pcs[i].ana_rgc3 = ana_rgc3;
> -		ss->pcs[i].regmap = syscon_node_to_regmap(np);
> -
> -		ss->pcs[i].flags = 0;
> +		flags = 0;
>  		if (of_property_read_bool(np, "pn_swap"))
> -			ss->pcs[i].flags |= MTK_SGMII_FLAG_PN_SWAP;
> +			flags |= MTK_SGMII_FLAG_PN_SWAP;
>  
>  		of_node_put(np);
> -		if (IS_ERR(ss->pcs[i].regmap))
> -			return PTR_ERR(ss->pcs[i].regmap);
>  
> -		ss->pcs[i].pcs.ops = &mtk_pcs_ops;
> -		ss->pcs[i].pcs.poll = true;
> -		ss->pcs[i].interface = PHY_INTERFACE_MODE_NA;
> +		regmap = syscon_node_to_regmap(np);

Not supposed to use "np" after of_node_put().

> +		if (IS_ERR(regmap))
> +			return PTR_ERR(regmap);
> +
> +		ss->pcs[i] = mtk_pcs_create(ss->dev, regmap, ana_rgc3, flags);
>  	}

[PATCH 9/9] net: dsa: mt7530: use external PCS driver

[Daniel Golle]

Implement regmap access wrappers, for now only to be used by the
pcs-mtk driver.
Make use of external PCS driver and drop the reduntant implementation
in mt7530.c.
As a nice side effect the SGMII registers can now also more easily be
inspected for debugging via /sys/kernel/debug/regmap.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/dsa/Kconfig  |   2 +
 drivers/net/dsa/mt7530.c | 278 ++++++++++-----------------------------
 drivers/net/dsa/mt7530.h |  43 +-----
 3 files changed, 72 insertions(+), 251 deletions(-)

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index f6f3b43dfb06..61b6608e45d6 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -38,6 +38,8 @@ config NET_DSA_MT7530
 	tristate "MediaTek MT7530 and MT7531 Ethernet switch support"
 	select NET_DSA_TAG_MTK
 	select MEDIATEK_GE_PHY
+	select PCS_MTK
+	select REGMAP
 	help
 	  This enables support for the MediaTek MT7530 and MT7531 Ethernet
 	  switch chips. Multi-chip module MT7530 in MT7621AT, MT7621DAT,
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 616b21c90d05..fb8ec7e479d1 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -14,6 +14,7 @@
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
 #include <linux/of_platform.h>
+#include <linux/pcs/pcs-mtk.h>
 #include <linux/phylink.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
@@ -2555,128 +2556,11 @@ static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
 	return 0;
 }
 
-static void mt7531_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
-			       phy_interface_t interface, int speed, int duplex)
-{
-	struct mt7530_priv *priv = pcs_to_mt753x_pcs(pcs)->priv;
-	int port = pcs_to_mt753x_pcs(pcs)->port;
-	unsigned int val;
-
-	/* For adjusting speed and duplex of SGMII force mode. */
-	if (interface != PHY_INTERFACE_MODE_SGMII ||
-	    phylink_autoneg_inband(mode))
-		return;
-
-	/* SGMII force mode setting */
-	val = mt7530_read(priv, MT7531_SGMII_MODE(port));
-	val &= ~MT7531_SGMII_IF_MODE_MASK;
-
-	switch (speed) {
-	case SPEED_10:
-		val |= MT7531_SGMII_FORCE_SPEED_10;
-		break;
-	case SPEED_100:
-		val |= MT7531_SGMII_FORCE_SPEED_100;
-		break;
-	case SPEED_1000:
-		val |= MT7531_SGMII_FORCE_SPEED_1000;
-		break;
-	}
-
-	/* MT7531 SGMII 1G force mode can only work in full duplex mode,
-	 * no matter MT7531_SGMII_FORCE_HALF_DUPLEX is set or not.
-	 *
-	 * The speed check is unnecessary as the MAC capabilities apply
-	 * this restriction. --rmk
-	 */
-	if ((speed == SPEED_10 || speed == SPEED_100) &&
-	    duplex != DUPLEX_FULL)
-		val |= MT7531_SGMII_FORCE_HALF_DUPLEX;
-
-	mt7530_write(priv, MT7531_SGMII_MODE(port), val);
-}
-
 static bool mt753x_is_mac_port(u32 port)
 {
 	return (port == 5 || port == 6);
 }
 
-static int mt7531_sgmii_setup_mode_force(struct mt7530_priv *priv, u32 port,
-					 phy_interface_t interface)
-{
-	u32 val;
-
-	if (!mt753x_is_mac_port(port))
-		return -EINVAL;
-
-	mt7530_set(priv, MT7531_QPHY_PWR_STATE_CTRL(port),
-		   MT7531_SGMII_PHYA_PWD);
-
-	val = mt7530_read(priv, MT7531_PHYA_CTRL_SIGNAL3(port));
-	val &= ~MT7531_RG_TPHY_SPEED_MASK;
-	/* Setup 2.5 times faster clock for 2.5Gbps data speeds with 10B/8B
-	 * encoding.
-	 */
-	val |= (interface == PHY_INTERFACE_MODE_2500BASEX) ?
-		MT7531_RG_TPHY_SPEED_3_125G : MT7531_RG_TPHY_SPEED_1_25G;
-	mt7530_write(priv, MT7531_PHYA_CTRL_SIGNAL3(port), val);
-
-	mt7530_clear(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_ENABLE);
-
-	/* MT7531 SGMII 1G and 2.5G force mode can only work in full duplex
-	 * mode, no matter MT7531_SGMII_FORCE_HALF_DUPLEX is set or not.
-	 */
-	mt7530_rmw(priv, MT7531_SGMII_MODE(port),
-		   MT7531_SGMII_IF_MODE_MASK | MT7531_SGMII_REMOTE_FAULT_DIS,
-		   MT7531_SGMII_FORCE_SPEED_1000);
-
-	mt7530_write(priv, MT7531_QPHY_PWR_STATE_CTRL(port), 0);
-
-	return 0;
-}
-
-static int mt7531_sgmii_setup_mode_an(struct mt7530_priv *priv, int port,
-				      phy_interface_t interface)
-{
-	if (!mt753x_is_mac_port(port))
-		return -EINVAL;
-
-	mt7530_set(priv, MT7531_QPHY_PWR_STATE_CTRL(port),
-		   MT7531_SGMII_PHYA_PWD);
-
-	mt7530_rmw(priv, MT7531_PHYA_CTRL_SIGNAL3(port),
-		   MT7531_RG_TPHY_SPEED_MASK, MT7531_RG_TPHY_SPEED_1_25G);
-
-	mt7530_set(priv, MT7531_SGMII_MODE(port),
-		   MT7531_SGMII_REMOTE_FAULT_DIS |
-		   MT7531_SGMII_SPEED_DUPLEX_AN);
-
-	mt7530_rmw(priv, MT7531_PCS_SPEED_ABILITY(port),
-		   MT7531_SGMII_TX_CONFIG_MASK, 1);
-
-	mt7530_set(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_ENABLE);
-
-	mt7530_set(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_RESTART);
-
-	mt7530_write(priv, MT7531_QPHY_PWR_STATE_CTRL(port), 0);
-
-	return 0;
-}
-
-static void mt7531_pcs_an_restart(struct phylink_pcs *pcs)
-{
-	struct mt7530_priv *priv = pcs_to_mt753x_pcs(pcs)->priv;
-	int port = pcs_to_mt753x_pcs(pcs)->port;
-	u32 val;
-
-	/* Only restart AN when AN is enabled */
-	val = mt7530_read(priv, MT7531_PCS_CONTROL_1(port));
-	if (val & MT7531_SGMII_AN_ENABLE) {
-		val |= MT7531_SGMII_AN_RESTART;
-		mt7530_write(priv, MT7531_PCS_CONTROL_1(port), val);
-	}
-}
-
 static int
 mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		  phy_interface_t interface)
@@ -2699,11 +2583,11 @@ mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		phydev = dp->slave->phydev;
 		return mt7531_rgmii_setup(priv, port, interface, phydev);
 	case PHY_INTERFACE_MODE_SGMII:
-		return mt7531_sgmii_setup_mode_an(priv, port, interface);
 	case PHY_INTERFACE_MODE_NA:
 	case PHY_INTERFACE_MODE_1000BASEX:
 	case PHY_INTERFACE_MODE_2500BASEX:
-		return mt7531_sgmii_setup_mode_force(priv, port, interface);
+		/* handled in SGMII PCS driver */
+		return 0;
 	default:
 		return -EINVAL;
 	}
@@ -2728,11 +2612,14 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,
 
 	switch (interface) {
 	case PHY_INTERFACE_MODE_TRGMII:
+		return &priv->pcs[port].pcs;
 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_1000BASEX:
 	case PHY_INTERFACE_MODE_2500BASEX:
-		return &priv->pcs[port].pcs;
+		if (!mt753x_is_mac_port(port))
+			return ERR_PTR(-EINVAL);
 
+		return priv->sgmii_pcs[port - 5];
 	default:
 		return NULL;
 	}
@@ -2970,86 +2857,6 @@ static void mt7530_pcs_get_state(struct phylink_pcs *pcs,
 		state->pause |= MLO_PAUSE_TX;
 }
 
-static int
-mt7531_sgmii_pcs_get_state_an(struct mt7530_priv *priv, int port,
-			      struct phylink_link_state *state)
-{
-	u32 status, val;
-	u16 config_reg;
-
-	status = mt7530_read(priv, MT7531_PCS_CONTROL_1(port));
-	state->link = !!(status & MT7531_SGMII_LINK_STATUS);
-	state->an_complete = !!(status & MT7531_SGMII_AN_COMPLETE);
-	if (state->interface == PHY_INTERFACE_MODE_SGMII &&
-	    (status & MT7531_SGMII_AN_ENABLE)) {
-		val = mt7530_read(priv, MT7531_PCS_SPEED_ABILITY(port));
-		config_reg = val >> 16;
-
-		switch (config_reg & LPA_SGMII_SPD_MASK) {
-		case LPA_SGMII_1000:
-			state->speed = SPEED_1000;
-			break;
-		case LPA_SGMII_100:
-			state->speed = SPEED_100;
-			break;
-		case LPA_SGMII_10:
-			state->speed = SPEED_10;
-			break;
-		default:
-			dev_err(priv->dev, "invalid sgmii PHY speed\n");
-			state->link = false;
-			return -EINVAL;
-		}
-
-		if (config_reg & LPA_SGMII_FULL_DUPLEX)
-			state->duplex = DUPLEX_FULL;
-		else
-			state->duplex = DUPLEX_HALF;
-	}
-
-	return 0;
-}
-
-static void
-mt7531_sgmii_pcs_get_state_inband(struct mt7530_priv *priv, int port,
-				  struct phylink_link_state *state)
-{
-	unsigned int val;
-
-	val = mt7530_read(priv, MT7531_PCS_CONTROL_1(port));
-	state->link = !!(val & MT7531_SGMII_LINK_STATUS);
-	if (!state->link)
-		return;
-
-	state->an_complete = state->link;
-
-	if (state->interface == PHY_INTERFACE_MODE_2500BASEX)
-		state->speed = SPEED_2500;
-	else
-		state->speed = SPEED_1000;
-
-	state->duplex = DUPLEX_FULL;
-	state->pause = MLO_PAUSE_NONE;
-}
-
-static void mt7531_pcs_get_state(struct phylink_pcs *pcs,
-				 struct phylink_link_state *state)
-{
-	struct mt7530_priv *priv = pcs_to_mt753x_pcs(pcs)->priv;
-	int port = pcs_to_mt753x_pcs(pcs)->port;
-
-	if (state->interface == PHY_INTERFACE_MODE_SGMII) {
-		mt7531_sgmii_pcs_get_state_an(priv, port, state);
-		return;
-	} else if ((state->interface == PHY_INTERFACE_MODE_1000BASEX) ||
-		   (state->interface == PHY_INTERFACE_MODE_2500BASEX)) {
-		mt7531_sgmii_pcs_get_state_inband(priv, port, state);
-		return;
-	}
-
-	state->link = false;
-}
-
 static int mt753x_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 			     phy_interface_t interface,
 			     const unsigned long *advertising,
@@ -3069,18 +2876,57 @@ static const struct phylink_pcs_ops mt7530_pcs_ops = {
 	.pcs_an_restart = mt7530_pcs_an_restart,
 };
 
-static const struct phylink_pcs_ops mt7531_pcs_ops = {
-	.pcs_validate = mt753x_pcs_validate,
-	.pcs_get_state = mt7531_pcs_get_state,
-	.pcs_config = mt753x_pcs_config,
-	.pcs_an_restart = mt7531_pcs_an_restart,
-	.pcs_link_up = mt7531_pcs_link_up,
+static int mt7530_regmap_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct mt7530_priv *priv = context;
+
+	*val = mt7530_read(priv, reg);
+	return 0;
+};
+
+static int mt7530_regmap_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct mt7530_priv *priv = context;
+
+	mt7530_write(priv, reg, val);
+	return 0;
+};
+
+static int mt7530_regmap_update_bits(void *context, unsigned int reg,
+				     unsigned int mask, unsigned int val)
+{
+	struct mt7530_priv *priv = context;
+
+	mt7530_rmw(priv, reg, mask, val);
+	return 0;
+};
+
+static const struct regmap_bus mt7531_regmap_bus = {
+	.reg_write = mt7530_regmap_write,
+	.reg_read = mt7530_regmap_read,
+	.reg_update_bits = mt7530_regmap_update_bits,
+};
+
+#define MT7531_PCS_REGMAP_CONFIG(_name, _reg_base) \
+	{				\
+		.name = _name,		\
+		.reg_bits = 16,		\
+		.val_bits = 32,		\
+		.reg_stride = 4,	\
+		.reg_base = _reg_base,	\
+		.max_register = 0x17c,	\
+	}
+
+static const struct regmap_config mt7531_pcs_config[] = {
+	MT7531_PCS_REGMAP_CONFIG("port5", MT7531_SGMII_REG_BASE(5)),
+	MT7531_PCS_REGMAP_CONFIG("port6", MT7531_SGMII_REG_BASE(6)),
 };
 
 static int
 mt753x_setup(struct dsa_switch *ds)
 {
 	struct mt7530_priv *priv = ds->priv;
+	struct regmap *regmap;
 	int i, ret;
 
 	/* Initialise the PCS devices */
@@ -3088,8 +2934,6 @@ mt753x_setup(struct dsa_switch *ds)
 		priv->pcs[i].pcs.ops = priv->info->pcs_ops;
 		priv->pcs[i].priv = priv;
 		priv->pcs[i].port = i;
-		if (mt753x_is_mac_port(i))
-			priv->pcs[i].pcs.poll = 1;
 	}
 
 	ret = priv->info->sw_setup(ds);
@@ -3104,6 +2948,15 @@ mt753x_setup(struct dsa_switch *ds)
 	if (ret && priv->irq)
 		mt7530_free_irq_common(priv);
 
+	if (priv->id == ID_MT7531)
+		for (i = 0; i < 2; ++i) {
+			regmap = devm_regmap_init(ds->dev,
+						  &mt7531_regmap_bus, priv,
+						  &mt7531_pcs_config[i]);
+			priv->sgmii_pcs[i] = mtk_pcs_create(ds->dev, regmap,
+							    0x128, 0);
+		}
+
 	return ret;
 }
 
@@ -3199,7 +3052,7 @@ static const struct mt753x_info mt753x_table[] = {
 	},
 	[ID_MT7531] = {
 		.id = ID_MT7531,
-		.pcs_ops = &mt7531_pcs_ops,
+		.pcs_ops = &mt7530_pcs_ops,
 		.sw_setup = mt7531_setup,
 		.phy_read_c22 = mt7531_ind_c22_phy_read,
 		.phy_write_c22 = mt7531_ind_c22_phy_write,
@@ -3309,7 +3162,7 @@ static void
 mt7530_remove(struct mdio_device *mdiodev)
 {
 	struct mt7530_priv *priv = dev_get_drvdata(&mdiodev->dev);
-	int ret = 0;
+	int ret = 0, i;
 
 	if (!priv)
 		return;
@@ -3328,6 +3181,11 @@ mt7530_remove(struct mdio_device *mdiodev)
 		mt7530_free_irq(priv);
 
 	dsa_unregister_switch(priv->ds);
+
+	for (i = 0; i < 2; ++i)
+		if (priv->sgmii_pcs[i])
+			mtk_pcs_destroy(priv->sgmii_pcs[i]);
+
 	mutex_destroy(&priv->reg_mutex);
 }
 
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 6b2fc6290ea8..dedbd707634a 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -364,47 +364,7 @@ enum mt7530_vlan_port_acc_frm {
 					 CCR_TX_OCT_CNT_BAD)
 
 /* MT7531 SGMII register group */
-#define MT7531_SGMII_REG_BASE		0x5000
-#define MT7531_SGMII_REG(p, r)		(MT7531_SGMII_REG_BASE + \
-					((p) - 5) * 0x1000 + (r))
-
-/* Register forSGMII PCS_CONTROL_1 */
-#define MT7531_PCS_CONTROL_1(p)		MT7531_SGMII_REG(p, 0x00)
-#define  MT7531_SGMII_LINK_STATUS	BIT(18)
-#define  MT7531_SGMII_AN_ENABLE		BIT(12)
-#define  MT7531_SGMII_AN_RESTART	BIT(9)
-#define  MT7531_SGMII_AN_COMPLETE	BIT(21)
-
-/* Register for SGMII PCS_SPPED_ABILITY */
-#define MT7531_PCS_SPEED_ABILITY(p)	MT7531_SGMII_REG(p, 0x08)
-#define  MT7531_SGMII_TX_CONFIG_MASK	GENMASK(15, 0)
-#define  MT7531_SGMII_TX_CONFIG		BIT(0)
-
-/* Register for SGMII_MODE */
-#define MT7531_SGMII_MODE(p)		MT7531_SGMII_REG(p, 0x20)
-#define  MT7531_SGMII_REMOTE_FAULT_DIS	BIT(8)
-#define  MT7531_SGMII_IF_MODE_MASK	GENMASK(5, 1)
-#define  MT7531_SGMII_FORCE_DUPLEX	BIT(4)
-#define  MT7531_SGMII_FORCE_SPEED_MASK	GENMASK(3, 2)
-#define  MT7531_SGMII_FORCE_SPEED_1000	BIT(3)
-#define  MT7531_SGMII_FORCE_SPEED_100	BIT(2)
-#define  MT7531_SGMII_FORCE_SPEED_10	0
-#define  MT7531_SGMII_SPEED_DUPLEX_AN	BIT(1)
-
-enum mt7531_sgmii_force_duplex {
-	MT7531_SGMII_FORCE_FULL_DUPLEX = 0,
-	MT7531_SGMII_FORCE_HALF_DUPLEX = 0x10,
-};
-
-/* Fields of QPHY_PWR_STATE_CTRL */
-#define MT7531_QPHY_PWR_STATE_CTRL(p)	MT7531_SGMII_REG(p, 0xe8)
-#define  MT7531_SGMII_PHYA_PWD		BIT(4)
-
-/* Values of SGMII SPEED */
-#define MT7531_PHYA_CTRL_SIGNAL3(p)	MT7531_SGMII_REG(p, 0x128)
-#define  MT7531_RG_TPHY_SPEED_MASK	(BIT(2) | BIT(3))
-#define  MT7531_RG_TPHY_SPEED_1_25G	0x0
-#define  MT7531_RG_TPHY_SPEED_3_125G	BIT(2)
+#define MT7531_SGMII_REG_BASE(p)	(0x5000 + ((p) - 5) * 0x1000)
 
 /* Register for system reset */
 #define MT7530_SYS_CTRL			0x7000
@@ -828,6 +788,7 @@ struct mt7530_priv {
 
 	struct mt7530_port	ports[MT7530_NUM_PORTS];
 	struct mt753x_pcs	pcs[MT7530_NUM_PORTS];
+	struct phylink_pcs	*sgmii_pcs[2];
 	/* protect among processes for registers access*/
 	struct mutex reg_mutex;
 	int irq;
-- 
2.39.1

Re: [PATCH 9/9] net: dsa: mt7530: use external PCS driver

[Vladimir Oltean]

On Fri, Feb 03, 2023 at 07:06:53AM +0000, Daniel Golle wrote:
> @@ -2728,11 +2612,14 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,
>  
>  	switch (interface) {
>  	case PHY_INTERFACE_MODE_TRGMII:
> +		return &priv->pcs[port].pcs;
>  	case PHY_INTERFACE_MODE_SGMII:
>  	case PHY_INTERFACE_MODE_1000BASEX:
>  	case PHY_INTERFACE_MODE_2500BASEX:
> -		return &priv->pcs[port].pcs;
> +		if (!mt753x_is_mac_port(port))
> +			return ERR_PTR(-EINVAL);

What is the reason for returning ERR_PTR(-EINVAL) to mac_select_pcs()?

>  
> +		return priv->sgmii_pcs[port - 5];

How about putting the pcs pointer in struct mt7530_port?

>  	default:
>  		return NULL;
>  	}
> @@ -3088,8 +2934,6 @@ mt753x_setup(struct dsa_switch *ds)
>  		priv->pcs[i].pcs.ops = priv->info->pcs_ops;
>  		priv->pcs[i].priv = priv;
>  		priv->pcs[i].port = i;
> -		if (mt753x_is_mac_port(i))
> -			priv->pcs[i].pcs.poll = 1;
>  	}
>  
>  	ret = priv->info->sw_setup(ds);
> @@ -3104,6 +2948,15 @@ mt753x_setup(struct dsa_switch *ds)
>  	if (ret && priv->irq)
>  		mt7530_free_irq_common(priv);

You need to patch the previous code to "return ret".

>  
> +	if (priv->id == ID_MT7531)

if the code block below is multi-line (which it is), put braces here too

or can return early if priv->id != ID_MT7531, and this reduces the
indentation by one level.

> +		for (i = 0; i < 2; ++i) {

could also iterate over all ports and ignore those which have
!mt753x_is_mac_port(port)

> +			regmap = devm_regmap_init(ds->dev,
> +						  &mt7531_regmap_bus, priv,
> +						  &mt7531_pcs_config[i]);

can fail

> +			priv->sgmii_pcs[i] = mtk_pcs_create(ds->dev, regmap,
> +							    0x128, 0);

can fail

Don't forget to do error teardown which isn't leaky

0x128 comes from the old definition of MT7531_PHYA_CTRL_SIGNAL3(port),
so please keep a macro of some sorts to denote the offset of ana_rgc3
for MT7531, rather than just this obscure magic number.

> +		}
> +
>  	return ret;
>  }
>  
> @@ -3199,7 +3052,7 @@ static const struct mt753x_info mt753x_table[] = {
>  	},
>  	[ID_MT7531] = {
>  		.id = ID_MT7531,
> -		.pcs_ops = &mt7531_pcs_ops,
> +		.pcs_ops = &mt7530_pcs_ops,
>  		.sw_setup = mt7531_setup,
>  		.phy_read_c22 = mt7531_ind_c22_phy_read,
>  		.phy_write_c22 = mt7531_ind_c22_phy_write,
> @@ -3309,7 +3162,7 @@ static void
>  mt7530_remove(struct mdio_device *mdiodev)
>  {
>  	struct mt7530_priv *priv = dev_get_drvdata(&mdiodev->dev);
> -	int ret = 0;
> +	int ret = 0, i;
>  
>  	if (!priv)
>  		return;
> @@ -3328,6 +3181,11 @@ mt7530_remove(struct mdio_device *mdiodev)
>  		mt7530_free_irq(priv);
>  
>  	dsa_unregister_switch(priv->ds);
> +
> +	for (i = 0; i < 2; ++i)

There is no ++i in this driver and there are 11 i++, so please be
consistent with what exists.

> +		if (priv->sgmii_pcs[i])
> +			mtk_pcs_destroy(priv->sgmii_pcs[i]);
> +
>  	mutex_destroy(&priv->reg_mutex);
>  }
>  

Re: [PATCH 9/9] net: dsa: mt7530: use external PCS driver

[Daniel Golle]

Hi Vladimir,

thank you for the review!

On Sat, Feb 04, 2023 at 12:19:15AM +0200, Vladimir Oltean wrote:
> On Fri, Feb 03, 2023 at 07:06:53AM +0000, Daniel Golle wrote:
> > @@ -2728,11 +2612,14 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,
> >  
> >  	switch (interface) {
> >  	case PHY_INTERFACE_MODE_TRGMII:
> > +		return &priv->pcs[port].pcs;
> >  	case PHY_INTERFACE_MODE_SGMII:
> >  	case PHY_INTERFACE_MODE_1000BASEX:
> >  	case PHY_INTERFACE_MODE_2500BASEX:
> > -		return &priv->pcs[port].pcs;
> > +		if (!mt753x_is_mac_port(port))
> > +			return ERR_PTR(-EINVAL);
> 
> What is the reason for returning ERR_PTR(-EINVAL) to mac_select_pcs()?

The SerDes PCS units are only available for port 5 and 6. The code
should make sure that the corresponding interface modes are only used
on these two ports, so a BUG_ON(!mt753x_is_mac_port(port)) would also
do the trick, I guess. However, as dsa_port_phylink_mac_select_pcs may
also return ERR_PTR(-EOPNOTSUPP), returning ERR_PTR(-EINVAL) felt like
the right thing to do in that case.
Are you suggesting to use BUG_ON() instead or rather return
ERR_PTR(-EOPNOTSUPP)?


> 
> >  
> > +		return priv->sgmii_pcs[port - 5];
> 
> How about putting the pcs pointer in struct mt7530_port?

There are only two SerDes units available, only for port 5 and port 6.
Hence I wouldn't want to allocate additional unused sizeof(void*) for
ports 0 to 4.

> 
> >  	default:
> >  		return NULL;
> >  	}
> > @@ -3088,8 +2934,6 @@ mt753x_setup(struct dsa_switch *ds)
> >  		priv->pcs[i].pcs.ops = priv->info->pcs_ops;
> >  		priv->pcs[i].priv = priv;
> >  		priv->pcs[i].port = i;
> > -		if (mt753x_is_mac_port(i))
> > -			priv->pcs[i].pcs.poll = 1;
> >  	}
> >  
> >  	ret = priv->info->sw_setup(ds);
> > @@ -3104,6 +2948,15 @@ mt753x_setup(struct dsa_switch *ds)
> >  	if (ret && priv->irq)
> >  		mt7530_free_irq_common(priv);
> 
> You need to patch the previous code to "return ret".
> 
> >  
> > +	if (priv->id == ID_MT7531)
> 
> if the code block below is multi-line (which it is), put braces here too
> 
> or can return early if priv->id != ID_MT7531, and this reduces the
> indentation by one level.
> 
> > +		for (i = 0; i < 2; ++i) {
> 
> could also iterate over all ports and ignore those which have
> !mt753x_is_mac_port(port)
> 
> > +			regmap = devm_regmap_init(ds->dev,
> > +						  &mt7531_regmap_bus, priv,
> > +						  &mt7531_pcs_config[i]);
> 
> can fail
> 
> > +			priv->sgmii_pcs[i] = mtk_pcs_create(ds->dev, regmap,
> > +							    0x128, 0);
> 
> can fail
> 
> Don't forget to do error teardown which isn't leaky
> 
> 0x128 comes from the old definition of MT7531_PHYA_CTRL_SIGNAL3(port),
> so please keep a macro of some sorts to denote the offset of ana_rgc3
> for MT7531, rather than just this obscure magic number.
> 
> > +		}
> > +
> >  	return ret;
> >  }
> >  
> > @@ -3199,7 +3052,7 @@ static const struct mt753x_info mt753x_table[] = {
> >  	},
> >  	[ID_MT7531] = {
> >  		.id = ID_MT7531,
> > -		.pcs_ops = &mt7531_pcs_ops,
> > +		.pcs_ops = &mt7530_pcs_ops,
> >  		.sw_setup = mt7531_setup,
> >  		.phy_read_c22 = mt7531_ind_c22_phy_read,
> >  		.phy_write_c22 = mt7531_ind_c22_phy_write,
> > @@ -3309,7 +3162,7 @@ static void
> >  mt7530_remove(struct mdio_device *mdiodev)
> >  {
> >  	struct mt7530_priv *priv = dev_get_drvdata(&mdiodev->dev);
> > -	int ret = 0;
> > +	int ret = 0, i;
> >  
> >  	if (!priv)
> >  		return;
> > @@ -3328,6 +3181,11 @@ mt7530_remove(struct mdio_device *mdiodev)
> >  		mt7530_free_irq(priv);
> >  
> >  	dsa_unregister_switch(priv->ds);
> > +
> > +	for (i = 0; i < 2; ++i)
> 
> There is no ++i in this driver and there are 11 i++, so please be
> consistent with what exists.

As most likely in all cases a pre-increment is sufficient and less
resource consuming than a post-increment operation we should consider
switching them all to ++i instead of i++.
I will anyway use i++ in v2 for now.

> 
> > +		if (priv->sgmii_pcs[i])
> > +			mtk_pcs_destroy(priv->sgmii_pcs[i]);
> > +
> >  	mutex_destroy(&priv->reg_mutex);
> >  }
> >  

Re: [PATCH 9/9] net: dsa: mt7530: use external PCS driver

[Andrew Lunn]

> The SerDes PCS units are only available for port 5 and 6. The code
> should make sure that the corresponding interface modes are only used
> on these two ports, so a BUG_ON(!mt753x_is_mac_port(port)) would also
> do the trick, I guess. However, as dsa_port_phylink_mac_select_pcs may
> also return ERR_PTR(-EOPNOTSUPP), returning ERR_PTR(-EINVAL) felt like
> the right thing to do in that case.
> Are you suggesting to use BUG_ON() instead or rather return
> ERR_PTR(-EOPNOTSUPP)?

BUG_ON() is considered to mean something which you cannot recover from
has happened, and going further will only cause more file system
corruption, memory corruption, etc. WARN_ON() is better, since it
gives the user a chance to perform a controlled shutdown, so
potentially not loosing files etc.

But even a WARN_ON() seems a bit extreme. If -EINVAL is sufficient to
cause either the whole device, or the port to fail to probe, that
should be enough to get the DT developers attention. Then either a
dev_err() or a dev_dbg() to help narrow down which check failed.

	  Andrew

Re: [PATCH 9/9] net: dsa: mt7530: use external PCS driver

[Russell King (Oracle)]

Hi Daniel,

I haven't reviewed the patches yet, and probably won't for a while.
(I'm recovering from Covid).

On Sat, Feb 04, 2023 at 03:02:00PM +0000, Daniel Golle wrote:
> Hi Vladimir,
> 
> thank you for the review!
> 
> On Sat, Feb 04, 2023 at 12:19:15AM +0200, Vladimir Oltean wrote:
> > On Fri, Feb 03, 2023 at 07:06:53AM +0000, Daniel Golle wrote:
> > > @@ -2728,11 +2612,14 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,
> > >  
> > >  	switch (interface) {
> > >  	case PHY_INTERFACE_MODE_TRGMII:
> > > +		return &priv->pcs[port].pcs;
> > >  	case PHY_INTERFACE_MODE_SGMII:
> > >  	case PHY_INTERFACE_MODE_1000BASEX:
> > >  	case PHY_INTERFACE_MODE_2500BASEX:
> > > -		return &priv->pcs[port].pcs;
> > > +		if (!mt753x_is_mac_port(port))
> > > +			return ERR_PTR(-EINVAL);
> > 
> > What is the reason for returning ERR_PTR(-EINVAL) to mac_select_pcs()?
> 
> The SerDes PCS units are only available for port 5 and 6. The code
> should make sure that the corresponding interface modes are only used
> on these two ports, so a BUG_ON(!mt753x_is_mac_port(port)) would also
> do the trick, I guess. However, as dsa_port_phylink_mac_select_pcs may
> also return ERR_PTR(-EOPNOTSUPP), returning ERR_PTR(-EINVAL) felt like
> the right thing to do in that case.
> Are you suggesting to use BUG_ON() instead or rather return
> ERR_PTR(-EOPNOTSUPP)?

Returning ERR_PTR(-EOPNOTSUPP) from mac_select_pcs() must not be done
conditionally - it means that "this instance does not support the
mac_select_pcs() interface" and phylink will never call it again.

It was implemented to provide DSA a way to tell phylink that the DSA
driver doesn't implement this interface - phylink originally checked
whether mac_ops->mac_select_pcs was NULL, but with DSA's layering,
such a check can only be made by a runtime call.

Returning NULL means "there is no PCS for this interface mode", and
returning any other error code essentially signifies that the
interface mode is not supported (even e.g. GMII or INTERNAL)...
which really *should* be handled by setting supported_interfaces
correctly in the first place.

I would much prefer drivers to just return NULL if they have no PCS,
even for ports where it's not possible, or not implement the
interface over returning some kind of error code.

The only time I would expect mac_select_pcs() to return an error is
where it needed to do something in order to evaluate whether to
return a PCS or not, and it encountered an error while trying to
evaluate that or some truely bizarre situation. E.g. a failed
memory allocation, or "we know that a PCS is required for this but
we're missing it". Something of that ilk.

Anyway, more rest needed.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 9/9] net: dsa: mt7530: use external PCS driver

[Vladimir Oltean]

On Sat, Feb 04, 2023 at 03:02:00PM +0000, Daniel Golle wrote:
> Hi Vladimir,
> 
> thank you for the review!
> 
> On Sat, Feb 04, 2023 at 12:19:15AM +0200, Vladimir Oltean wrote:
> > On Fri, Feb 03, 2023 at 07:06:53AM +0000, Daniel Golle wrote:
> > > @@ -2728,11 +2612,14 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,
> > >  
> > >  	switch (interface) {
> > >  	case PHY_INTERFACE_MODE_TRGMII:
> > > +		return &priv->pcs[port].pcs;
> > >  	case PHY_INTERFACE_MODE_SGMII:
> > >  	case PHY_INTERFACE_MODE_1000BASEX:
> > >  	case PHY_INTERFACE_MODE_2500BASEX:
> > > -		return &priv->pcs[port].pcs;
> > > +		if (!mt753x_is_mac_port(port))
> > > +			return ERR_PTR(-EINVAL);
> > 
> > What is the reason for returning ERR_PTR(-EINVAL) to mac_select_pcs()?
> 
> The SerDes PCS units are only available for port 5 and 6. The code
> should make sure that the corresponding interface modes are only used
> on these two ports, so a BUG_ON(!mt753x_is_mac_port(port)) would also
> do the trick, I guess. However, as dsa_port_phylink_mac_select_pcs may
> also return ERR_PTR(-EOPNOTSUPP), returning ERR_PTR(-EINVAL) felt like
> the right thing to do in that case.
> Are you suggesting to use BUG_ON() instead or rather return
> ERR_PTR(-EOPNOTSUPP)?

I was not suggesting anything, I was just asking why the inconsistency
exists between the handling of SERDES protocols on ports != 5 or 6
(return ERR_PTR(-EINVAL)), and the handling of unknown PHY modes
(return NULL). If you don't want to convey any special message, then
returning NULL to phylink should be sufficient to say there is no PCS.
The driver already ensures that phylink validation fails with wrong PHY
mode on wrong port due to the way in which it populates supported_interfaces
in mt7531_mac_port_get_caps().

Also, no BUG_ON() for the reasons Andrew pointed out.

> > > +		return priv->sgmii_pcs[port - 5];
> > 
> > How about putting the pcs pointer in struct mt7530_port?
> 
> There are only two SerDes units available, only for port 5 and port 6.
> Hence I wouldn't want to allocate additional unused sizeof(void*) for
> ports 0 to 4.

I asked the question fully knowing that there will be no more than 2
ports with a non-NULL PCS pointer. There's a time and place for
(premature) optimizations, but I don't think that the control path of a
switch is one particular place that comes at the top. Between
priv->ports[port].sgmii_pcs and priv->sgmii_pcs[port - 5], my personal
sensibilities for simple and maintainable code would always choose the
former. However I'm not going to cling onto this. Whichever way you prefer.

> > > +	for (i = 0; i < 2; ++i)
> > 
> > There is no ++i in this driver and there are 11 i++, so please be
> > consistent with what exists.
> 
> As most likely in all cases a pre-increment is sufficient and less
> resource consuming than a post-increment operation we should consider
> switching them all to ++i instead of i++.
> I will anyway use i++ in v2 for now.

And what would you suggest is the difference in the compiled code between
"for (i = 0; i < n; i++)" and "for (i = 0; i < n; ++i)"?

Re: [PATCH 0/9] net: ethernet: mtk_eth_soc: various enhancements

[Bjørn Mork]

Daniel Golle <daniel@makrotopia.org> writes:

> This series brings a variety of fixes and enhancements for mtk_eth_soc,
> adds support for the MT7981 SoC and facilitates sharing the SGMII PCS
> code between mtk_eth_soc and mt7530.

Thanks! I've now successfully tested this on an MT7986 board with 2.5G
phys (GPY211C) connected to one of the mtk_eth_soc macs and on port 5 of
the the MT7531.  The series fixes a number of issues for me, including a
mysterious packet drop at 1G only which I believe is related to the
undocumented MAC_MCR_BIT_12 in patch 5.

With Vladimir's (WiP?) "PCS and PHY in-band sync" patch set and your
related implementations in mxl-gpy, mt7530 and mtk_eth_soc to top of
this series, both 2.5G ports are now usable at any speed.

Note that the mt7530.c part of the series depends on 
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git/commit/?id=697c3892d825fb78f42ec8e53bed065dd728db3e
which is not yet in net-next. It appears to be destined for v6.2 so this
is not a problem.  Just mentioning it in case someone else is struggling
with the testing of this series.

Feel free to add

Tested-by: Bjørn Mork <bjorn@mork.no>

to the complete series.


Bjørn