Skip to contentSkip to conversation

Add support for 10G Ethernet SerDes on MT7988

RFC PATCH Superseded Daniel Golle 4 participants 30 messages Dec 6, 2023 – Dec 7, 2023

Subsystems (9)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Daniel Golle

author maintainer
#0 lore raw

v2 cover Add support for 10G Ethernet SerDes on MT7988

This series aims to add support for GMAC2 and GMAC3 of the MediaTek MT7988 SoC. While the vendor SDK stuffs all this into their Ethernet driver, I've tried to seperate things into a PHY driver, a PCS driver as well as changes to the existing Ethernet and LynxI PCS driver.

 +--------------+   +----------------+   +------------------+
 |              +---|  USXGMII PCS   |---+                  |
 | Ethernet MAC |   +----------------+   | PEXTP SerDes PHY |
 |              +---|   SGMII PCS    |---+                  |
 +--------------+   +----------------+   +------------------+

Alltogether this allows using GMAC2 and GMAC3 with all possible interface modes, including in-band-status if needed.

Note that this series depends on patch "dt-bindings: clock: mediatek: add MT7988 clock IDs" being merged before.

https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=807227

Changes since RFC v1:
- drop patch inhibiting SGMII AN in 2500Base-X mode
- make pcs-mtk-lynxi a proper platform driver
- ... hence allowing to remove all the wrappers from the usxgmii driver
- attach PEXTP to MAC instead of to USXGMII PCS

Daniel Golle (8):
dt-bindings: phy: mediatek,xfi-pextp: add new bindings
phy: add driver for MediaTek pextp 10GE SerDes PHY
net: pcs: pcs-mtk-lynxi: add platform driver for MT7988
dt-bindings: net: pcs: add bindings for MediaTek USXGMII PCS
net: pcs: add driver for MediaTek USXGMII PCS
dt-bindings: net: mediatek: remove wrongly added clocks and SerDes
dt-bindings: net: mediatek,net: fix and complete mt7988-eth binding
net: ethernet: mtk_eth_soc: add paths and SerDes modes for MT7988

 .../devicetree/bindings/net/mediatek,net.yaml | 180 ++++++--
 .../bindings/net/pcs/mediatek,usxgmii.yaml    |  60 +++
 .../bindings/phy/mediatek,xfi-pextp.yaml      |  80 ++++
 MAINTAINERS                                   |   3 +
 drivers/net/ethernet/mediatek/mtk_eth_path.c  | 122 +++++-
 drivers/net/ethernet/mediatek/mtk_eth_soc.c   | 284 ++++++++++--
 drivers/net/ethernet/mediatek/mtk_eth_soc.h   | 107 ++++-
 drivers/net/pcs/Kconfig                       |  11 +
 drivers/net/pcs/Makefile                      |   1 +
 drivers/net/pcs/pcs-mtk-lynxi.c               | 170 ++++++-
 drivers/net/pcs/pcs-mtk-usxgmii.c             | 413 ++++++++++++++++++
 drivers/phy/mediatek/Kconfig                  |  11 +
 drivers/phy/mediatek/Makefile                 |   1 +
 drivers/phy/mediatek/phy-mtk-pextp.c          | 365 ++++++++++++++++
 include/linux/pcs/pcs-mtk-lynxi.h             |   1 +
 include/linux/pcs/pcs-mtk-usxgmii.h           |  26 ++
 16 files changed, 1746 insertions(+), 89 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml
 create mode 100644 Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml
 create mode 100644 drivers/net/pcs/pcs-mtk-usxgmii.c
 create mode 100644 drivers/phy/mediatek/phy-mtk-pextp.c
 create mode 100644 include/linux/pcs/pcs-mtk-usxgmii.h
Signature · 1 line
2.43.0
author maintainer
#1 lore raw

RFC PATCH v2 1/8 dt-bindings: phy: mediatek,xfi-pextp: add new bindings

Add bindings for the MediaTek PEXTP Ethernet SerDes PHY found in the MediaTek MT7988 SoC which can operate at various interfaces modes:

* USXGMII
* 10GBase-R
* 5GBase-R
* 2500Base-X
* 1000Base-X
* Cisco SGMII (MAC side)

---
 .../bindings/phy/mediatek,xfi-pextp.yaml      | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml
Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml new file +80 -0
new file mode 100644
index 0000000000000..58c93368141e2
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/mediatek,xfi-pextp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek XFI PEXTP SerDes PHY
+
+maintainers:
+  - Daniel Golle <[email protected]>
+
+description:
+  The MediaTek XFI PEXTP SerDes PHY provides the physical SerDes lanes
+  used by the MediaTek USXGMII PCS.
+
+properties:
+  $nodename:
+    pattern: "^phy@[0-9a-f]+$"
+
+  compatible:
+    const: mediatek,mt7988-xfi-pextp
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: XFI PHY clock
+      - description: XFI register clock
+
+  clock-names:
+    items:
+      - const: "xfipll"
+      - const: "topxtal"
+
+  resets:
+    items:
+      - description: PEXTP reset
+
+  mediatek,usxgmii-performance-errata:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      USXGMII0 on MT7988 suffers from a performance problem in 10GBase-R
+      mode which needs a work-around in the driver. The work-around is
+      enabled using this flag.
+
+  "#phy-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - resets
+  - "#phy-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/mediatek,mt7988-clk.h>
+    #include <dt-bindings/reset/mediatek,mt7988-resets.h>
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      phy@11f20000 {
+        compatible = "mediatek,mt7988-xfi-pextp";
+        reg = <0 0x11f20000 0 0x10000>;
+        clocks = <&xfi_pll CLK_XFIPLL_PLL_EN>,
+                 <&topckgen CLK_TOP_XFI_PHY_0_XTAL_SEL>;
+        clock-names = "xfipll", "topxtal";
+        resets = <&watchdog MT7988_TOPRGU_XFI_PEXTP0_GRST>;
+        mediatek,usxgmii-performance-errata;
+        #phy-cells = <0>;
+      };
+    };
+
+...
Signature · 1 line
2.43.0
maintainer
#2 lore raw

Re: [RFC PATCH v2 1/8] dt-bindings: phy: mediatek,xfi-pextp: add new bindings

Quoted 10 lines · Daniel Golle

Add bindings for the MediaTek PEXTP Ethernet SerDes PHY found in the MediaTek MT7988 SoC which can operate at various interfaces modes:

* USXGMII
* 10GBase-R
* 5GBase-R
* 2500Base-X
* 1000Base-X
* Cisco SGMII (MAC side)

Quoted 6 lines
---
 .../bindings/phy/mediatek,xfi-pextp.yaml      | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml:33:16: [error] string value is redundantly quoted with any quotes (quoted-strings) ./Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml:34:16: [error] string value is redundantly quoted with any quotes (quoted-strings)

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.example.dts:18:18: fatal error: dt-bindings/clock/mediatek,mt7988-clk.h: No such file or directory
18 | #include <dt-bindings/clock/mediatek,mt7988-clk.h>

      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

compilation terminated.
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.example.dtb] Error 1 make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1424: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/f57cc2bfadca35219707a0af507ad6e2e354e311.1701826319.git.daniel@makrotopia.org

The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.

author maintainer
#3 lore raw

RFC PATCH v2 2/8 phy: add driver for MediaTek pextp 10GE SerDes PHY

In reply to Daniel Golle

Add driver for MediaTek's pextp 10 Gigabit/s Ethernet SerDes PHY which can be found in the MT7988 SoC.

The PHY can operates only in PHY_MODE_ETHERNET, the submode is one of PHY_INTERFACE_MODE_* corresponding to the supported modes:

* USXGMII
* 10GBase-R
* 5GBase-R
* 2500Base-X
* 1000Base-X
* Cisco SGMII (MAC side)

In order to work-around a performance issue present on the first of two PEXTP present in MT7988 special tuning is applied which can be selected by adding the mediatek,usxgmii-performance-errata property to the device tree node.

There is no documentation what-so-ever for the pextp registers and this driver is based on a GPL licensed implementation found in MediaTek's SDK.

---
 MAINTAINERS                          |   1 +
 drivers/phy/mediatek/Kconfig         |  11 +
 drivers/phy/mediatek/Makefile        |   1 +
 drivers/phy/mediatek/phy-mtk-pextp.c | 365 +++++++++++++++++++++++++++
 4 files changed, 378 insertions(+)
 create mode 100644 drivers/phy/mediatek/phy-mtk-pextp.c
MAINTAINERS +1 -0
index 1ef2a16370619..1ea4555013a4d 100644
@@ -13497,6 +13497,7 @@ L:	[email protected]
 S:	Maintained
 F:	drivers/net/phy/mediatek-ge-soc.c
 F:	drivers/net/phy/mediatek-ge.c
+F:	drivers/phy/mediatek/phy-mtk-pextp.c
 
 MEDIATEK I2C CONTROLLER DRIVER
 M:	Qii Wang <[email protected]>
drivers/phy/mediatek/Kconfig +11 -0
index 3125ecb5d119f..a7749a6d96541 100644
@@ -13,6 +13,17 @@ config PHY_MTK_PCIE
 	  callback for PCIe GEN3 port, it supports software efuse
 	  initialization.
 
+config PHY_MTK_PEXTP
+	tristate "MediaTek PEXTP Driver"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	depends on OF && OF_ADDRESS
+	depends on HAS_IOMEM
+	select GENERIC_PHY
+	help
+	  Say 'Y' here to add support for MediaTek pextp PHY driver.
+	  The driver provides access to the Ethernet SerDes PHY supporting
+	  various 1GE, 2.5GE, 5GE and 10GE modes.
+
 config PHY_MTK_TPHY
 	tristate "MediaTek T-PHY Driver"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
drivers/phy/mediatek/Makefile +1 -0
index c9a50395533eb..ca60c7b9b02ac 100644
@@ -8,6 +8,7 @@ obj-$(CONFIG_PHY_MTK_PCIE)		+= phy-mtk-pcie.o
 obj-$(CONFIG_PHY_MTK_TPHY)		+= phy-mtk-tphy.o
 obj-$(CONFIG_PHY_MTK_UFS)		+= phy-mtk-ufs.o
 obj-$(CONFIG_PHY_MTK_XSPHY)		+= phy-mtk-xsphy.o
+obj-$(CONFIG_PHY_MTK_PEXTP)		+= phy-mtk-pextp.o
 
 phy-mtk-hdmi-drv-y			:= phy-mtk-hdmi.o
 phy-mtk-hdmi-drv-y			+= phy-mtk-hdmi-mt2701.o
drivers/phy/mediatek/phy-mtk-pextp.c new file +365 -0

365 lines not on this page · raw message

Signature · 1 line
2.43.0
#4 lore raw

Re: [RFC PATCH v2 2/8] phy: add driver for MediaTek pextp 10GE SerDes PHY

Hello Daniel,

Quoted 24 lines · On Wed, 6 Dec 2023 01:44:08 +0000 Daniel Golle <[email protected]> wrote:

Add driver for MediaTek's pextp 10 Gigabit/s Ethernet SerDes PHY which can be found in the MT7988 SoC.

The PHY can operates only in PHY_MODE_ETHERNET, the submode is one of PHY_INTERFACE_MODE_* corresponding to the supported modes:

* USXGMII
* 10GBase-R
* 5GBase-R
* 2500Base-X
* 1000Base-X
* Cisco SGMII (MAC side)

In order to work-around a performance issue present on the first of two PEXTP present in MT7988 special tuning is applied which can be selected by adding the mediatek,usxgmii-performance-errata property to the device tree node.

There is no documentation what-so-ever for the pextp registers and this driver is based on a GPL licensed implementation found in MediaTek's SDK.

---

[...]

drivers/phy/mediatek/phy-mtk-pextp.c line 284

Quoted 12 lines
drivers/phy/mediatek/phy-mtk-pextp.c new file +12 -0
+
+static int mtk_pextp_power_on(struct phy *phy)
+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+	int ret;
+
+	ret = clk_prepare_enable(pextp->clk[0]);
+	if (ret)
+		return ret;
+
+	return clk_prepare_enable(pextp->clk[1]);
+}

clk_bulk operations could be used here

drivers/phy/mediatek/phy-mtk-pextp.c line 294

Quoted 9 lines
drivers/phy/mediatek/phy-mtk-pextp.c new file +9 -0
+static int mtk_pextp_power_off(struct phy *phy)
+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+
+	clk_disable_unprepare(pextp->clk[1]);
+	clk_disable_unprepare(pextp->clk[0]);
+
+	return 0;
+}

Here

Quoted 24 lines
drivers/phy/mediatek/phy-mtk-pextp.c new file +24 -0
+static const struct phy_ops mtk_pextp_ops = {
+	.power_on	= mtk_pextp_power_on,
+	.power_off	= mtk_pextp_power_off,
+	.set_mode	= mtk_pextp_set_mode,
+	.reset		= mtk_pextp_reset,
+	.owner		= THIS_MODULE,
+};
+
+static int mtk_pextp_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct phy_provider *phy_provider;
+	struct mtk_pextp_phy *pextp;
+	struct phy *phy;
+
+	if (!np)
+		return -ENODEV;
+
+	pextp = devm_kzalloc(&pdev->dev, sizeof(*pextp), GFP_KERNEL);
+	if (!pextp)
+		return -ENOMEM;
+
+	pextp->base = devm_of_iomap(&pdev->dev, np, 0, NULL);
+	if (!pextp->base)

drivers/phy/mediatek/phy-mtk-pextp.c line 329

Quoted 10 lines
drivers/phy/mediatek/phy-mtk-pextp.c new file +10 -0
+		return -EIO;
+
+	pextp->dev = &pdev->dev;
+	pextp->clk[0] = devm_clk_get(&pdev->dev, "topxtal");
+	if (IS_ERR(pextp->clk[0]))
+		return PTR_ERR(pextp->clk[0]);
+
+	pextp->clk[1] = devm_clk_get(&pdev->dev, "xfipll");
+	if (IS_ERR(pextp->clk[1]))
+		return PTR_ERR(pextp->clk[1]);

And here as well.

Thanks,

Maxime

author maintainer
#5 lore raw

RFC PATCH v2 3/8 net: pcs: pcs-mtk-lynxi: add platform driver for MT7988

In reply to Daniel Golle

Introduce a proper platform MFD driver for the LynxI (H)SGMII PCS which is going to initially be used for the MT7988 SoC.

---
 drivers/net/pcs/pcs-mtk-lynxi.c   | 170 ++++++++++++++++++++++++++++--
 include/linux/pcs/pcs-mtk-lynxi.h |   1 +
 2 files changed, 160 insertions(+), 11 deletions(-)
drivers/net/pcs/pcs-mtk-lynxi.c +159 -11
index 8501dd365279b..558d6351399b5 100644
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2018-2019 MediaTek Inc.
-/* A library for MediaTek SGMII circuit
+/* A library and platform driver for the MediaTek LynxI SGMII circuit
  *
  * Author: Sean Wang <[email protected]>
  * Author: Alexander Couzens <[email protected]>
@@ -8,11 +8,16 @@
  *
  */
 
+#include <linux/clk.h>
 #include <linux/mdio.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/pcs/pcs-mtk-lynxi.h>
 #include <linux/phylink.h>
+#include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/reset.h>
 
 /* SGMII subsystem config registers */
 /* BMCR (low 16) BMSR (high 16) */
@@ -65,6 +70,8 @@
 #define SGMII_PN_SWAP_MASK		GENMASK(1, 0)
 #define SGMII_PN_SWAP_TX_RX		(BIT(0) | BIT(1))
 
+#define MTK_NETSYS_V3_AMA_RGC3		0x128
+
 /* struct mtk_pcs_lynxi -  This structure holds each sgmii regmap andassociated
  *                         data
  * @regmap:                The register map pointing at the range used to setup
@@ -81,6 +88,10 @@ struct mtk_pcs_lynxi {
 	phy_interface_t		interface;
 	struct			phylink_pcs pcs;
 	u32			flags;
+	struct reset_control	*rstc;
+	struct clk		*sgmii_sel;
+	struct clk		*sgmii_rx;
+	struct clk		*sgmii_tx;
 };
 
 static struct mtk_pcs_lynxi *pcs_to_mtk_pcs_lynxi(struct phylink_pcs *pcs)
@@ -102,6 +113,17 @@ static void mtk_pcs_lynxi_get_state(struct phylink_pcs *pcs,
 					 FIELD_GET(SGMII_LPA, adv));
 }
 
+static void mtk_sgmii_reset(struct mtk_pcs_lynxi *mpcs)
+{
+	if (!mpcs->rstc)
+		return;
+
+	reset_control_assert(mpcs->rstc);
+	usleep_range(100, 500);
+	reset_control_deassert(mpcs->rstc);
+	mdelay(10);
+}
+
 static int mtk_pcs_lynxi_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 				phy_interface_t interface,
 				const unsigned long *advertising,
@@ -147,6 +169,7 @@ static int mtk_pcs_lynxi_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 				SGMII_PHYA_PWD);
 
 		/* Reset SGMII PCS state */
+		mtk_sgmii_reset(mpcs);
 		regmap_set_bits(mpcs->regmap, SGMSYS_RESERVED_0,
 				SGMII_SW_RESET);
 
@@ -233,10 +256,29 @@ static void mtk_pcs_lynxi_link_up(struct phylink_pcs *pcs,
 	}
 }
 
+static int mtk_pcs_lynxi_enable(struct phylink_pcs *pcs)
+{
+	struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
+
+	if (mpcs->sgmii_tx && mpcs->sgmii_rx) {
+		clk_prepare_enable(mpcs->sgmii_rx);
+		clk_prepare_enable(mpcs->sgmii_tx);
+	}
+
+	return 0;
+}
+
 static void mtk_pcs_lynxi_disable(struct phylink_pcs *pcs)
 {
 	struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
 
+	regmap_set_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
+
+	if (mpcs->sgmii_tx && mpcs->sgmii_rx) {
+		clk_disable_unprepare(mpcs->sgmii_tx);
+		clk_disable_unprepare(mpcs->sgmii_rx);
+	}
+
 	mpcs->interface = PHY_INTERFACE_MODE_NA;
 }
 
@@ -246,11 +288,12 @@ static const struct phylink_pcs_ops mtk_pcs_lynxi_ops = {
 	.pcs_an_restart = mtk_pcs_lynxi_restart_an,
 	.pcs_link_up = mtk_pcs_lynxi_link_up,
 	.pcs_disable = mtk_pcs_lynxi_disable,
+	.pcs_enable = mtk_pcs_lynxi_enable,
 };
 
-struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
-					 struct regmap *regmap, u32 ana_rgc3,
-					 u32 flags)
+static struct phylink_pcs *mtk_pcs_lynxi_init(struct device *dev, struct regmap *regmap,
+					      u32 ana_rgc3, u32 flags,
+					      struct mtk_pcs_lynxi *prealloc)
 {
 	struct mtk_pcs_lynxi *mpcs;
 	u32 id, ver;
@@ -258,29 +301,33 @@ struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
 
 	ret = regmap_read(regmap, SGMSYS_PCS_DEVICE_ID, &id);
 	if (ret < 0)
-		return NULL;
+		return ERR_PTR(ret);
 
 	if (id != SGMII_LYNXI_DEV_ID) {
 		dev_err(dev, "unknown PCS device id %08x\n", id);
-		return NULL;
+		return ERR_PTR(-ENODEV);
 	}
 
 	ret = regmap_read(regmap, SGMSYS_PCS_SCRATCH, &ver);
 	if (ret < 0)
-		return NULL;
+		return ERR_PTR(ret);
 
 	ver = FIELD_GET(SGMII_DEV_VERSION, ver);
 	if (ver != 0x1) {
 		dev_err(dev, "unknown PCS device version %04x\n", ver);
-		return NULL;
+		return ERR_PTR(-ENODEV);
 	}
 
 	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;
+	if (prealloc) {
+		mpcs = prealloc;
+	} else {
+		mpcs = kzalloc(sizeof(*mpcs), GFP_KERNEL);
+		if (!mpcs)
+			return ERR_PTR(-ENOMEM);
+	};
 
 	mpcs->ana_rgc3 = ana_rgc3;
 	mpcs->regmap = regmap;
@@ -291,6 +338,13 @@ struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
 	mpcs->interface = PHY_INTERFACE_MODE_NA;
 
 	return &mpcs->pcs;
+};
+
+struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
+					 struct regmap *regmap, u32 ana_rgc3,
+					 u32 flags)
+{
+	return mtk_pcs_lynxi_init(dev, regmap, ana_rgc3, flags, NULL);
 }
 EXPORT_SYMBOL(mtk_pcs_lynxi_create);
 
@@ -303,4 +357,98 @@ void mtk_pcs_lynxi_destroy(struct phylink_pcs *pcs)
 }
 EXPORT_SYMBOL(mtk_pcs_lynxi_destroy);
 
+static int mtk_pcs_lynxi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct mtk_pcs_lynxi *mpcs;
+	struct phylink_pcs *pcs;
+	struct regmap *regmap;
+	u32 flags = 0;
+
+	mpcs = devm_kzalloc(dev, sizeof(*mpcs), GFP_KERNEL);
+	if (!mpcs)
+		return -ENOMEM;
+
+	regmap = syscon_node_to_regmap(np->parent);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	if (of_property_read_bool(np->parent, "mediatek,pnswap"))
+		flags |= MTK_SGMII_FLAG_PN_SWAP;
+
+	mpcs->rstc = of_reset_control_get_shared(np->parent, NULL);
+	if (IS_ERR(mpcs->rstc))
+		return PTR_ERR(mpcs->rstc);
+
+	reset_control_deassert(mpcs->rstc);
+	mpcs->sgmii_sel = devm_clk_get_enabled(dev, "sgmii_sel");
+	if (IS_ERR(mpcs->sgmii_sel))
+		return PTR_ERR(mpcs->sgmii_sel);
+
+	mpcs->sgmii_rx = devm_clk_get(dev, "sgmii_rx");
+	if (IS_ERR(mpcs->sgmii_rx))
+		return PTR_ERR(mpcs->sgmii_rx);
+
+	mpcs->sgmii_tx = devm_clk_get(dev, "sgmii_tx");
+	if (IS_ERR(mpcs->sgmii_tx))
+		return PTR_ERR(mpcs->sgmii_tx);
+
+	pcs = mtk_pcs_lynxi_init(dev, regmap, (uintptr_t)of_device_get_match_data(dev),
+				 flags, mpcs);
+	if (IS_ERR(pcs))
+		return PTR_ERR(pcs);
+
+	regmap_set_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
+
+	platform_set_drvdata(pdev, mpcs);
+
+	return 0;
+}
+
+static const struct of_device_id mtk_pcs_lynxi_of_match[] = {
+	{ .compatible = "mediatek,mt7988-sgmii", .data = (void *)MTK_NETSYS_V3_AMA_RGC3 },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mtk_pcs_lynxi_of_match);
+
+struct phylink_pcs *mtk_pcs_lynxi_select_pcs(struct device_node *np, phy_interface_t mode)
+{
+	struct platform_device *pdev;
+	struct mtk_pcs_lynxi *mpcs;
+
+	if (!np)
+		return NULL;
+
+	if (!of_device_is_available(np))
+		return ERR_PTR(-ENODEV);
+
+	if (!of_match_node(mtk_pcs_lynxi_of_match, np))
+		return ERR_PTR(-EINVAL);
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev || !platform_get_drvdata(pdev)) {
+		if (pdev)
+			put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	mpcs = platform_get_drvdata(pdev);
+	put_device(&pdev->dev);
+
+	return &mpcs->pcs;
+}
+EXPORT_SYMBOL(mtk_pcs_lynxi_select_pcs);
+
+static struct platform_driver mtk_pcs_lynxi_driver = {
+	.driver = {
+		.name = "mtk-pcs-lynxi",
+		.of_match_table = mtk_pcs_lynxi_of_match,
+	},
+	.probe = mtk_pcs_lynxi_probe,
+};
+module_platform_driver(mtk_pcs_lynxi_driver);
+
+MODULE_AUTHOR("Daniel Golle <[email protected]>");
+MODULE_DESCRIPTION("MediaTek LynxI HSGMII PCS");
 MODULE_LICENSE("GPL");
include/linux/pcs/pcs-mtk-lynxi.h +1 -0
index be3b4ab32f4a7..5db19d6a7261f 100644
@@ -10,4 +10,5 @@ struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
 					 struct regmap *regmap,
 					 u32 ana_rgc3, u32 flags);
 void mtk_pcs_lynxi_destroy(struct phylink_pcs *pcs);
+struct phylink_pcs *mtk_pcs_lynxi_select_pcs(struct device_node *np, phy_interface_t mode);
 #endif
Signature · 1 line
2.43.0
#6 lore raw

Re: [RFC PATCH v2 3/8] net: pcs: pcs-mtk-lynxi: add platform driver for MT7988

Hello Daniel,

My two cents below :

Quoted 4 lines · On Wed, 6 Dec 2023 01:44:17 +0000 Daniel Golle <[email protected]> wrote:

Introduce a proper platform MFD driver for the LynxI (H)SGMII PCS which is going to initially be used for the MT7988 SoC.

[ ... ]

drivers/net/pcs/pcs-mtk-lynxi.c static void mtk_pcs_lynxi_link_up(struct phylink_pcs *pcs, · line 265

Quoted 7 lines
drivers/net/pcs/pcs-mtk-lynxi.c +7 -0
+static int mtk_pcs_lynxi_enable(struct phylink_pcs *pcs)
+{
+	struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
+
+	if (mpcs->sgmii_tx && mpcs->sgmii_rx) {
+		clk_prepare_enable(mpcs->sgmii_rx);
+		clk_prepare_enable(mpcs->sgmii_tx);

You can use the clk_bulk_prepare_enable() here

Quoted 4 lines
drivers/net/pcs/pcs-mtk-lynxi.c +4 -0
+	}
+
+	return 0;
+}

drivers/net/pcs/pcs-mtk-lynxi.c static void mtk_pcs_lynxi_link_up(struct phylink_pcs *pcs, · line 279

Quoted 10 lines
drivers/net/pcs/pcs-mtk-lynxi.c +6 -0
+
 static void mtk_pcs_lynxi_disable(struct phylink_pcs *pcs)
 {
 	struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
 
+	regmap_set_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
+
+	if (mpcs->sgmii_tx && mpcs->sgmii_rx) {
+		clk_disable_unprepare(mpcs->sgmii_tx);
+		clk_disable_unprepare(mpcs->sgmii_rx);

and clk_bulk_disable_unprepare() here

[...]

Quoted 26 lines
drivers/net/pcs/pcs-mtk-lynxi.c +26 -0
+static int mtk_pcs_lynxi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct mtk_pcs_lynxi *mpcs;
+	struct phylink_pcs *pcs;
+	struct regmap *regmap;
+	u32 flags = 0;
+
+	mpcs = devm_kzalloc(dev, sizeof(*mpcs), GFP_KERNEL);
+	if (!mpcs)
+		return -ENOMEM;
+
+	regmap = syscon_node_to_regmap(np->parent);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	if (of_property_read_bool(np->parent, "mediatek,pnswap"))
+		flags |= MTK_SGMII_FLAG_PN_SWAP;
+
+	mpcs->rstc = of_reset_control_get_shared(np->parent, NULL);
+	if (IS_ERR(mpcs->rstc))
+		return PTR_ERR(mpcs->rstc);
+
+	reset_control_deassert(mpcs->rstc);
+	mpcs->sgmii_sel = devm_clk_get_enabled(dev, "sgmii_sel");

drivers/net/pcs/pcs-mtk-lynxi.c void mtk_pcs_lynxi_destroy(struct phylink_pcs *pcs) · line 395

Quoted 10 lines
drivers/net/pcs/pcs-mtk-lynxi.c +10 -0
+	if (IS_ERR(mpcs->sgmii_sel))
+		return PTR_ERR(mpcs->sgmii_sel);
+
+	mpcs->sgmii_rx = devm_clk_get(dev, "sgmii_rx");
+	if (IS_ERR(mpcs->sgmii_rx))
+		return PTR_ERR(mpcs->sgmii_rx);
+
+	mpcs->sgmii_tx = devm_clk_get(dev, "sgmii_tx");
+	if (IS_ERR(mpcs->sgmii_tx))
+		return PTR_ERR(mpcs->sgmii_tx);

and clk bulk operations here as well ?

Maxime

#7 lore raw

Re: [RFC PATCH v2 3/8] net: pcs: pcs-mtk-lynxi: add platform driver for MT7988

In reply to Daniel Golle
Quoted 17 lines · Daniel Golle
drivers/net/pcs/pcs-mtk-lynxi.c +17 -0
+struct phylink_pcs *mtk_pcs_lynxi_select_pcs(struct device_node *np, phy_interface_t mode)
+{
+	struct platform_device *pdev;
+	struct mtk_pcs_lynxi *mpcs;
+
+	if (!np)
+		return NULL;
+
+	if (!of_device_is_available(np))
+		return ERR_PTR(-ENODEV);
+
+	if (!of_match_node(mtk_pcs_lynxi_of_match, np))
+		return ERR_PTR(-EINVAL);
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev || !platform_get_drvdata(pdev)) {
+		if (pdev)

drivers/net/pcs/pcs-mtk-lynxi.c void mtk_pcs_lynxi_destroy(struct phylink_pcs *pcs) · line 441

Quoted 10 lines
drivers/net/pcs/pcs-mtk-lynxi.c +10 -0
+			put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	mpcs = platform_get_drvdata(pdev);
+	put_device(&pdev->dev);
+
+	return &mpcs->pcs;
+}
+EXPORT_SYMBOL(mtk_pcs_lynxi_select_pcs);

If you're going to play games like this, then you must mark the driver with .suppress_bind_attrs = true to remove the bind/unbind attributes in userspace that could wreak havoc with the above - because there is _nothing_ that guarantees that the memory you're returning from this function will remain intact. Basically, it's racy.

Also, I'm not sure I approve of using the "select_pcs" suffix (I haven't spotted _where_ you use this, but returning EPROBE_DEFER to phylink's mac_select_pcs() method doesn't do anything to defer any probe, so that's an entirely misleading error code.

If we are going to have device drivers for PCS, then we need to seriously think about how we look up PCS and return the phylink_pcs pointer - and also how we handle the PCS device going away. None of that should be coded into _any_ PCS driver.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
author maintainer
#8 lore raw

Re: [RFC PATCH v2 3/8] net: pcs: pcs-mtk-lynxi: add platform driver for MT7988

Quoted 26 lines · Russell King (Oracle)
Quoted 25 lines · Daniel Golle
drivers/net/pcs/pcs-mtk-lynxi.c +25 -0
+struct phylink_pcs *mtk_pcs_lynxi_select_pcs(struct device_node *np, phy_interface_t mode)
+{
+	struct platform_device *pdev;
+	struct mtk_pcs_lynxi *mpcs;
+
+	if (!np)
+		return NULL;
+
+	if (!of_device_is_available(np))
+		return ERR_PTR(-ENODEV);
+
+	if (!of_match_node(mtk_pcs_lynxi_of_match, np))
+		return ERR_PTR(-EINVAL);
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev || !platform_get_drvdata(pdev)) {
+		if (pdev)
+			put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	mpcs = platform_get_drvdata(pdev);
+	put_device(&pdev->dev);
+
+	return &mpcs->pcs;
Quoted 8 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+}
+EXPORT_SYMBOL(mtk_pcs_lynxi_select_pcs);

If you're going to play games like this, then you must mark the driver with .suppress_bind_attrs = true to remove the bind/unbind attributes in userspace that could wreak havoc with the above - because there is _nothing_ that guarantees that the memory you're returning from this function will remain intact. Basically, it's racy.

Ack, I've set .suppress_bind_attrs = true in the usxgmii driver but forgot to add it here.

Quoted 4 lines

Also, I'm not sure I approve of using the "select_pcs" suffix (I haven't spotted _where_ you use this, but returning EPROBE_DEFER to phylink's mac_select_pcs() method doesn't do anything to defer any probe, so that's an entirely misleading error code.

EPROBE_DEFER is handled when the function is called by mtk_add_mac() during probe of the Ethernet driver -- which we do want to postpone in case the PCS hasn't been probed yet as at this point that's the best we can do without adding lots of intrastructure to dynamically attach the PCS later on...

But true, later the function is being called by mac_select_pcs() and what ever it returns is returned to the caller of mac_select_pcs(). If you think it's better to return ENODEV (or EAGAIN?) I can change that -- from what I could tell, the only error which receives special handling by phylink is -EOPNOTSUPP, everything else just gets passed- through to the callers.

Quoted 4 lines

If we are going to have device drivers for PCS, then we need to seriously think about how we look up PCS and return the phylink_pcs pointer - and also how we handle the PCS device going away. None of that should be coded into _any_ PCS driver.

I agree -- just wasn't up to design and implement all that at once.

author maintainer
#9 lore raw

RFC PATCH v2 4/8 dt-bindings: net: pcs: add bindings for MediaTek USXGMII PCS

In reply to Daniel Golle

MediaTek's USXGMII can be found in the MT7988 SoC. We need to access it in order to configure and monitor the Ethernet SerDes link in USXGMII, 10GBase-R and 5GBase-R mode. By including a wrapped legacy 1000Base-X/2500Base-X/Cisco SGMII LynxI PCS as well, those interface modes are also available.

---
 .../bindings/net/pcs/mediatek,usxgmii.yaml    | 100 ++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml
Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml new file +100 -0
new file mode 100644
index 0000000000000..9a798c0d2fdd7
@@ -0,0 +1,100 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pcs/mediatek,usxgmii.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek USXGMII PCS
+
+maintainers:
+  - Daniel Golle <[email protected]>
+
+description:
+  The MediaTek USXGMII PCS provides physical link control and status
+  for USXGMII, 10GBase-R and 5GBase-R links on the SerDes interfaces
+  provided by the PEXTP PHY.
+  In order to also support legacy 2500Base-X, 1000Base-X and Cisco
+  SGMII an existing mediatek,*-sgmiisys LynxI PCS is wrapped to
+  provide those interfaces modes on the same SerDes interfaces shared
+  with the USXGMII PCS.
+
+properties:
+  $nodename:
+    pattern: "^pcs@[0-9a-f]+$"
+
+  compatible:
+    const: mediatek,mt7988-usxgmiisys
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: USXGMII top-level clock
+      - description: SGMII top-level clock
+      - description: SGMII subsystem TX clock
+      - description: SGMII subsystem RX clock
+      - description: XFI PLL clock
+
+  clock-names:
+    items:
+      - const: usxgmii
+      - const: sgmii_sel
+      - const: sgmii_tx
+      - const: sgmii_rx
+      - const: xfi_pll
+
+  phys:
+    items:
+      - description: PEXTP SerDes PHY
+
+  mediatek,sgmiisys:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the syscon node of the corresponding SGMII LynxI PCS.
+
+  resets:
+    items:
+      - description: XFI reset
+      - description: SGMII reset
+
+  reset-names:
+    items:
+      - const: xfi
+      - const: sgmii
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - phys
+  - mediatek,sgmiisys
+  - resets
+  - reset-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/mediatek,mt7988-clk.h>
+    #include <dt-bindings/reset/mediatek,mt7988-resets.h>
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+        usxgmiisys0: pcs@10080000 {
+          compatible = "mediatek,mt7988-usxgmiisys";
+          reg = <0 0x10080000 0 0x1000>;
+          clocks = <&topckgen CLK_TOP_USXGMII_SBUS_0_SEL>,
+                   <&topckgen CLK_TOP_SGM_0_SEL>,
+                   <&sgmiisys0 CLK_SGM0_TX_EN>,
+                   <&sgmiisys0 CLK_SGM0_RX_EN>,
+                   <&xfi_pll CLK_XFIPLL_PLL_EN>;
+          clock-names = "usxgmii", "sgmii_sel", "sgmii_tx", "sgmii_rx", "xfi_pll";
+          resets = <&watchdog MT7988_TOPRGU_XFI0_GRST>,
+                   <&watchdog MT7988_TOPRGU_SGMII0_GRST>;
+          reset-names = "xfi", "sgmii";
+          phys = <&xfi_pextp0>;
+          mediatek,sgmiisys = <&sgmiisys0>;
+        };
+    };
Signature · 1 line
2.43.0
maintainer
#10 lore raw

Re: [RFC PATCH v2 4/8] dt-bindings: net: pcs: add bindings for MediaTek USXGMII PCS

Quoted 6 lines · Daniel Golle

MediaTek's USXGMII can be found in the MT7988 SoC. We need to access it in order to configure and monitor the Ethernet SerDes link in USXGMII, 10GBase-R and 5GBase-R mode. By including a wrapped legacy 1000Base-X/2500Base-X/Cisco SGMII LynxI PCS as well, those interface modes are also available.

Quoted 6 lines
---
 .../bindings/net/pcs/mediatek,usxgmii.yaml    | 100 ++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.example.dts:18:18: fatal error: dt-bindings/clock/mediatek,mt7988-clk.h: No such file or directory make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.example.dtb] Error 1

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/14c3eb3022fac2af105950eb161990ecfb17c016.1701826319.git.daniel@makrotopia.org

The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.

author maintainer
#11 lore raw

RFC PATCH v2 5/8 net: pcs: add driver for MediaTek USXGMII PCS

In reply to Daniel Golle

Add driver for USXGMII PCS found in the MediaTek MT7988 SoC and supporting USXGMII, 10GBase-R and 5GBase-R interface modes. In order to support Cisco SGMII, 1000Base-X and 2500Base-X via the also present LynxI PCS create a wrapped PCS taking care of the components shared between the new USXGMII PCS and the legacy LynxI PCS.

---
 .../bindings/net/pcs/mediatek,usxgmii.yaml    |  46 +-
 MAINTAINERS                                   |   2 +
 drivers/net/pcs/Kconfig                       |  11 +
 drivers/net/pcs/Makefile                      |   1 +
 drivers/net/pcs/pcs-mtk-usxgmii.c             | 413 ++++++++++++++++++
 include/linux/pcs/pcs-mtk-usxgmii.h           |  26 ++
 6 files changed, 456 insertions(+), 43 deletions(-)
 create mode 100644 drivers/net/pcs/pcs-mtk-usxgmii.c
 create mode 100644 include/linux/pcs/pcs-mtk-usxgmii.h
Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml +3 -43
index 9a798c0d2fdd7..0cdaa3545edb0 100644
@@ -31,70 +31,30 @@ properties:
   clocks:
     items:
       - description: USXGMII top-level clock
-      - description: SGMII top-level clock
-      - description: SGMII subsystem TX clock
-      - description: SGMII subsystem RX clock
-      - description: XFI PLL clock
-
-  clock-names:
-    items:
-      - const: usxgmii
-      - const: sgmii_sel
-      - const: sgmii_tx
-      - const: sgmii_rx
-      - const: xfi_pll
-
-  phys:
-    items:
-      - description: PEXTP SerDes PHY
-
-  mediatek,sgmiisys:
-    $ref: /schemas/types.yaml#/definitions/phandle
-    description:
-      Phandle to the syscon node of the corresponding SGMII LynxI PCS.
 
   resets:
     items:
       - description: XFI reset
-      - description: SGMII reset
-
-  reset-names:
-    items:
-      - const: xfi
-      - const: sgmii
 
 required:
   - compatible
   - reg
   - clocks
-  - clock-names
-  - phys
-  - mediatek,sgmiisys
   - resets
-  - reset-names
 
 additionalProperties: false
 
 examples:
   - |
     #include <dt-bindings/clock/mediatek,mt7988-clk.h>
-    #include <dt-bindings/reset/mediatek,mt7988-resets.h>
+    #define MT7988_TOPRGU_XFI0_GRST 12
     soc {
       #address-cells = <2>;
       #size-cells = <2>;
         usxgmiisys0: pcs@10080000 {
           compatible = "mediatek,mt7988-usxgmiisys";
           reg = <0 0x10080000 0 0x1000>;
-          clocks = <&topckgen CLK_TOP_USXGMII_SBUS_0_SEL>,
-                   <&topckgen CLK_TOP_SGM_0_SEL>,
-                   <&sgmiisys0 CLK_SGM0_TX_EN>,
-                   <&sgmiisys0 CLK_SGM0_RX_EN>,
-                   <&xfi_pll CLK_XFIPLL_PLL_EN>;
-          clock-names = "usxgmii", "sgmii_sel", "sgmii_tx", "sgmii_rx", "xfi_pll";
-          resets = <&watchdog MT7988_TOPRGU_XFI0_GRST>,
-                   <&watchdog MT7988_TOPRGU_SGMII0_GRST>;
-          reset-names = "xfi", "sgmii";
-          phys = <&xfi_pextp0>;
-          mediatek,sgmiisys = <&sgmiisys0>;
+          clocks = <&topckgen CLK_TOP_USXGMII_SBUS_0_SEL>;
+          resets = <&watchdog MT7988_TOPRGU_XFI0_GRST>;
         };
     };
MAINTAINERS +2 -0
index 1ea4555013a4d..13fe7ffb10f7d 100644
@@ -13487,7 +13487,9 @@ M:	Daniel Golle <[email protected]>
 L:	[email protected]
 S:	Maintained
 F:	drivers/net/pcs/pcs-mtk-lynxi.c
+F:	drivers/net/pcs/pcs-mtk-usxgmii.c
 F:	include/linux/pcs/pcs-mtk-lynxi.h
+F:	include/linux/pcs/pcs-mtk-usxgmii.h
 
 MEDIATEK ETHERNET PHY DRIVERS
 M:	Daniel Golle <[email protected]>
drivers/net/pcs/Kconfig +11 -0
index 87cf308fc6d8b..55a6865bdaba3 100644
@@ -25,6 +25,17 @@ config PCS_MTK_LYNXI
 	  This module provides helpers to phylink for managing the LynxI PCS
 	  which is part of MediaTek's SoC and Ethernet switch ICs.
 
+config PCS_MTK_USXGMII
+	tristate "MediaTek USXGMII PCS"
+	select PCS_MTK_LYNXI
+	select PHY_MTK_PEXTP
+	select PHYLINK
+	help
+	  This module provides a driver for MediaTek's USXGMII PCS supporting
+	  10GBase-R, 5GBase-R and USXGMII interface modes.
+	  1000Base-X, 2500Base-X and Cisco SGMII are supported on the same
+	  differential pairs via an embedded LynxI PHY.
+
 config PCS_RZN1_MIIC
 	tristate "Renesas RZ/N1 MII converter"
 	depends on OF && (ARCH_RZN1 || COMPILE_TEST)
drivers/net/pcs/Makefile +1 -0
index fb1694192ae63..cc355152ca1ca 100644
@@ -6,4 +6,5 @@ pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-nxp.o pcs-xpcs-wx.o
 obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
 obj-$(CONFIG_PCS_LYNX)		+= pcs-lynx.o
 obj-$(CONFIG_PCS_MTK_LYNXI)	+= pcs-mtk-lynxi.o
+obj-$(CONFIG_PCS_MTK_USXGMII)	+= pcs-mtk-usxgmii.o
 obj-$(CONFIG_PCS_RZN1_MIIC)	+= pcs-rzn1-miic.o
drivers/net/pcs/pcs-mtk-usxgmii.c new file +413 -0

413 lines not on this page · raw message

include/linux/pcs/pcs-mtk-usxgmii.h new file +26 -0
new file mode 100644
index 0000000000000..346b88bdf7a6e
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_PCS_MTK_USXGMII_H
+#define __LINUX_PCS_MTK_USXGMII_H
+
+#include <linux/phylink.h>
+
+/**
+ * mtk_usxgmii_select_pcs() - Get MediaTek PCS instance
+ * @np:		Pointer to device node indentifying a MediaTek USXGMII PCS
+ * @mode:	Ethernet PHY interface mode
+ *
+ * Return PCS identified by a device node and the PHY interface mode in use
+ *
+ * Return:	Pointer to phylink PCS instance of NULL
+ */
+#if IS_ENABLED(CONFIG_PCS_MTK_USXGMII)
+struct phylink_pcs *mtk_usxgmii_select_pcs(struct device_node *np, phy_interface_t mode);
+#else
+static inline struct phylink_pcs *mtk_usxgmii_select_pcs(struct device_node *np,
+							 phy_interface_t mode)
+{
+	return NULL;
+}
+#endif /* IS_ENABLED(CONFIG_PCS_MTK_USXGMII) */
+
+#endif /* __LINUX_PCS_MTK_USXGMII_H */
Signature · 1 line
2.43.0
maintainer
#12 lore raw

Re: [RFC PATCH v2 5/8] net: pcs: add driver for MediaTek USXGMII PCS

Quoted 7 lines · Daniel Golle

Add driver for USXGMII PCS found in the MediaTek MT7988 SoC and supporting USXGMII, 10GBase-R and 5GBase-R interface modes. In order to support Cisco SGMII, 1000Base-X and 2500Base-X via the also present LynxI PCS create a wrapped PCS taking care of the components shared between the new USXGMII PCS and the legacy LynxI PCS.

Quoted 11 lines
---
 .../bindings/net/pcs/mediatek,usxgmii.yaml    |  46 +-
 MAINTAINERS                                   |   2 +
 drivers/net/pcs/Kconfig                       |  11 +
 drivers/net/pcs/Makefile                      |   1 +
 drivers/net/pcs/pcs-mtk-usxgmii.c             | 413 ++++++++++++++++++
 include/linux/pcs/pcs-mtk-usxgmii.h           |  26 ++
 6 files changed, 456 insertions(+), 43 deletions(-)
 create mode 100644 drivers/net/pcs/pcs-mtk-usxgmii.c
 create mode 100644 include/linux/pcs/pcs-mtk-usxgmii.h

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/3cd8af5e44554c2db2d7898494ee813967206bd9.1701826319.git.daniel@makrotopia.org

The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.

#13 lore raw

Re: [RFC PATCH v2 5/8] net: pcs: add driver for MediaTek USXGMII PCS

In reply to Daniel Golle

Hello Daniel,

Quoted 8 lines · On Wed, 6 Dec 2023 01:44:38 +0000 Daniel Golle <[email protected]> wrote:

Add driver for USXGMII PCS found in the MediaTek MT7988 SoC and supporting USXGMII, 10GBase-R and 5GBase-R interface modes. In order to support Cisco SGMII, 1000Base-X and 2500Base-X via the also present LynxI PCS create a wrapped PCS taking care of the components shared between the new USXGMII PCS and the legacy LynxI PCS.

---

[...]

drivers/net/pcs/pcs-mtk-usxgmii.c line 114

Quoted 9 lines
drivers/net/pcs/pcs-mtk-usxgmii.c new file +9 -0
+
+static int mtk_usxgmii_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
+				  phy_interface_t interface,
+				  const unsigned long *advertising,
+				  bool permit_pause_to_mac)
+{
+	struct mtk_usxgmii_pcs *mpcs = pcs_to_mtk_usxgmii_pcs(pcs);
+	unsigned int an_ctrl = 0, link_timer = 0, xfi_mode = 0, adapt_mode = 0;
+	bool mode_changed = false;

Reverse christmas tree ordering can be used here (longest lines first)

Quoted 82 lines
drivers/net/pcs/pcs-mtk-usxgmii.c new file +82 -0
+
+	if (interface == PHY_INTERFACE_MODE_USXGMII) {
+		an_ctrl = FIELD_PREP(USXGMII_AN_SYNC_CNT, 0x1FF) | USXGMII_AN_ENABLE;
+		link_timer = FIELD_PREP(USXGMII_LINK_TIMER_IDLE_DETECT, 0x7B) |
+			     FIELD_PREP(USXGMII_LINK_TIMER_COMP_ACK_DETECT, 0x7B) |
+			     FIELD_PREP(USXGMII_LINK_TIMER_AN_RESTART, 0x7B);
+		xfi_mode = FIELD_PREP(USXGMII_XFI_RX_MODE, USXGMII_XFI_MODE_10G) |
+			   FIELD_PREP(USXGMII_XFI_TX_MODE, USXGMII_XFI_MODE_10G);
+	} else if (interface == PHY_INTERFACE_MODE_10GBASER) {
+		an_ctrl = FIELD_PREP(USXGMII_AN_SYNC_CNT, 0x1FF);
+		link_timer = FIELD_PREP(USXGMII_LINK_TIMER_IDLE_DETECT, 0x7B) |
+			     FIELD_PREP(USXGMII_LINK_TIMER_COMP_ACK_DETECT, 0x7B) |
+			     FIELD_PREP(USXGMII_LINK_TIMER_AN_RESTART, 0x7B);
+		xfi_mode = FIELD_PREP(USXGMII_XFI_RX_MODE, USXGMII_XFI_MODE_10G) |
+			   FIELD_PREP(USXGMII_XFI_TX_MODE, USXGMII_XFI_MODE_10G);
+		adapt_mode = USXGMII_RATE_UPDATE_MODE;
+	} else if (interface == PHY_INTERFACE_MODE_5GBASER) {
+		an_ctrl = FIELD_PREP(USXGMII_AN_SYNC_CNT, 0xFF);
+		link_timer = FIELD_PREP(USXGMII_LINK_TIMER_IDLE_DETECT, 0x3D) |
+			     FIELD_PREP(USXGMII_LINK_TIMER_COMP_ACK_DETECT, 0x3D) |
+			     FIELD_PREP(USXGMII_LINK_TIMER_AN_RESTART, 0x3D);
+		xfi_mode = FIELD_PREP(USXGMII_XFI_RX_MODE, USXGMII_XFI_MODE_5G) |
+			   FIELD_PREP(USXGMII_XFI_TX_MODE, USXGMII_XFI_MODE_5G);
+		adapt_mode = USXGMII_RATE_UPDATE_MODE;
+	} else {
+		return -EINVAL;
+	}
+
+	adapt_mode |= FIELD_PREP(USXGMII_RATE_ADAPT_MODE, USXGMII_RATE_ADAPT_MODE_X1);
+
+	if (mpcs->interface != interface) {
+		mpcs->interface = interface;
+		mode_changed = true;
+	}
+
+	mtk_usxgmii_reset(mpcs);
+
+	/* Setup USXGMII AN ctrl */
+	mtk_m32(mpcs, RG_PCS_AN_CTRL0,
+		USXGMII_AN_SYNC_CNT | USXGMII_AN_ENABLE,
+		an_ctrl);
+
+	mtk_m32(mpcs, RG_PCS_AN_CTRL2,
+		USXGMII_LINK_TIMER_IDLE_DETECT |
+		USXGMII_LINK_TIMER_COMP_ACK_DETECT |
+		USXGMII_LINK_TIMER_AN_RESTART,
+		link_timer);
+
+	mpcs->neg_mode = neg_mode;
+
+	/* Gated MAC CK */
+	mtk_m32(mpcs, RG_PHY_TOP_SPEED_CTRL1,
+		USXGMII_MAC_CK_GATED, USXGMII_MAC_CK_GATED);
+
+	/* Enable interface force mode */
+	mtk_m32(mpcs, RG_PHY_TOP_SPEED_CTRL1,
+		USXGMII_IF_FORCE_EN, USXGMII_IF_FORCE_EN);
+
+	/* Setup USXGMII adapt mode */
+	mtk_m32(mpcs, RG_PHY_TOP_SPEED_CTRL1,
+		USXGMII_RATE_UPDATE_MODE | USXGMII_RATE_ADAPT_MODE,
+		adapt_mode);
+
+	/* Setup USXGMII speed */
+	mtk_m32(mpcs, RG_PHY_TOP_SPEED_CTRL1,
+		USXGMII_XFI_RX_MODE | USXGMII_XFI_TX_MODE,
+		xfi_mode);
+
+	usleep_range(1, 10);
+
+	/* Un-gated MAC CK */
+	mtk_m32(mpcs, RG_PHY_TOP_SPEED_CTRL1, USXGMII_MAC_CK_GATED, 0);
+
+	usleep_range(1, 10);
+
+	/* Disable interface force mode for the AN mode */
+	if (an_ctrl & USXGMII_AN_ENABLE)
+		mtk_m32(mpcs, RG_PHY_TOP_SPEED_CTRL1, USXGMII_IF_FORCE_EN, 0);
+
+	return mode_changed;
+}
+

[...]

Quoted 11 lines
drivers/net/pcs/pcs-mtk-usxgmii.c new file +11 -0
+static void mtk_usxgmii_pcs_get_state(struct phylink_pcs *pcs,
+				      struct phylink_link_state *state)
+{
+	struct mtk_usxgmii_pcs *mpcs = pcs_to_mtk_usxgmii_pcs(pcs);
+
+	/* Refresh USXGMII link status by toggling RG_PCS_AN_STATUS_UPDATE */
+	mtk_m32(mpcs, RG_PCS_RX_STATUS0, RG_PCS_RX_STATUS_UPDATE,
+		RG_PCS_RX_STATUS_UPDATE);
+	ndelay(1020);
+	mtk_m32(mpcs, RG_PCS_RX_STATUS0, RG_PCS_RX_STATUS_UPDATE, 0);
+	ndelay(1020);

drivers/net/pcs/pcs-mtk-usxgmii.c line 284

Quoted 10 lines
drivers/net/pcs/pcs-mtk-usxgmii.c new file +10 -0
+
+	/* Read USXGMII link status */
+	state->link = FIELD_GET(RG_PCS_RX_LINK_STATUS,
+				mtk_r32(mpcs, RG_PCS_RX_STATUS0));
+
+	/* Continuously repeat re-configuration sequence until link comes up */
+	if (!state->link) {
+		mtk_usxgmii_pcs_config(pcs, mpcs->neg_mode,
+				       state->interface, NULL, false);
+		return;

.pcs_get_state() isn't called only for link state polling,but also when querying the link state from ethtool, from phylink_ethtool_ksettings_get().

As mtk_usxgmii_pcs_config triggers a pcs reset and reconfiguration, won't this disrupt the link ?

Thanks,

Maxime

#14 lore raw

Re: [RFC PATCH v2 5/8] net: pcs: add driver for MediaTek USXGMII PCS

Quoted 15 lines · Maxime Chevallier

drivers/net/pcs/pcs-mtk-usxgmii.c line 284

Quoted 9 lines · On Wed, 6 Dec 2023 01:44:38 +0000 Daniel Golle <[email protected]> wrote:
drivers/net/pcs/pcs-mtk-usxgmii.c new file +9 -0
+	/* Read USXGMII link status */
+	state->link = FIELD_GET(RG_PCS_RX_LINK_STATUS,
+				mtk_r32(mpcs, RG_PCS_RX_STATUS0));
+
+	/* Continuously repeat re-configuration sequence until link comes up */
+	if (!state->link) {
+		mtk_usxgmii_pcs_config(pcs, mpcs->neg_mode,
+				       state->interface, NULL, false);
+		return;

.pcs_get_state() isn't called only for link state polling,but also when querying the link state from ethtool, from phylink_ethtool_ksettings_get().

Quoted 2 lines

As mtk_usxgmii_pcs_config triggers a pcs reset and reconfiguration, won't this disrupt the link ?

Highly likely if there's a race, but note that mtk_usxgmii_pcs_config() only gets called if the link is *down*. I guess some IPs need a bit of kicking to work properly.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
#15 lore raw

Re: [RFC PATCH v2 5/8] net: pcs: add driver for MediaTek USXGMII PCS

Quoted 16 lines · On Wed, 6 Dec 2023 17:58:08 +0000 "Russell King (Oracle)"…
Quoted 15 lines · Maxime Chevallier

drivers/net/pcs/pcs-mtk-usxgmii.c line 284

Quoted 9 lines · On Wed, 6 Dec 2023 01:44:38 +0000 Daniel Golle <[email protected]> wrote:
drivers/net/pcs/pcs-mtk-usxgmii.c new file +9 -0
+	/* Read USXGMII link status */
+	state->link = FIELD_GET(RG_PCS_RX_LINK_STATUS,
+				mtk_r32(mpcs, RG_PCS_RX_STATUS0));
+
+	/* Continuously repeat re-configuration sequence until link comes up */
+	if (!state->link) {
+		mtk_usxgmii_pcs_config(pcs, mpcs->neg_mode,
+				       state->interface, NULL, false);
+		return;  

.pcs_get_state() isn't called only for link state polling,but also when querying the link state from ethtool, from phylink_ethtool_ksettings_get().

Quoted 7 lines
Quoted 2 lines

As mtk_usxgmii_pcs_config triggers a pcs reset and reconfiguration, won't this disrupt the link ?

Highly likely if there's a race, but note that mtk_usxgmii_pcs_config() only gets called if the link is *down*. I guess some IPs need a bit of kicking to work properly.

Ah right that's true, it should be OK then.

Thanks,

Maxime

maintainer
#16 lore raw

Re: [RFC PATCH v2 5/8] net: pcs: add driver for MediaTek USXGMII PCS

In reply to Daniel Golle
Quoted 9 lines · Daniel Golle

Add driver for USXGMII PCS found in the MediaTek MT7988 SoC and supporting USXGMII, 10GBase-R and 5GBase-R interface modes. In order to support Cisco SGMII, 1000Base-X and 2500Base-X via the also present LynxI PCS create a wrapped PCS taking care of the components shared between the new USXGMII PCS and the legacy LynxI PCS.

---
 .../bindings/net/pcs/mediatek,usxgmii.yaml    |  46 +-

Why are you changing the binding you just added?

In any case, bindings are separate patches.

Quoted 8 lines
 MAINTAINERS                                   |   2 +
 drivers/net/pcs/Kconfig                       |  11 +
 drivers/net/pcs/Makefile                      |   1 +
 drivers/net/pcs/pcs-mtk-usxgmii.c             | 413 ++++++++++++++++++
 include/linux/pcs/pcs-mtk-usxgmii.h           |  26 ++
 6 files changed, 456 insertions(+), 43 deletions(-)
 create mode 100644 drivers/net/pcs/pcs-mtk-usxgmii.c
 create mode 100644 include/linux/pcs/pcs-mtk-usxgmii.h
author maintainer
#17 lore raw

Re: [RFC PATCH v2 5/8] net: pcs: add driver for MediaTek USXGMII PCS

Quoted 7 lines · Rob Herring
Quoted 6 lines · Daniel Golle

Add driver for USXGMII PCS found in the MediaTek MT7988 SoC and supporting USXGMII, 10GBase-R and 5GBase-R interface modes. In order to support Cisco SGMII, 1000Base-X and 2500Base-X via the also present LynxI PCS create a wrapped PCS taking care of the components shared between the new USXGMII PCS and the legacy LynxI PCS.

Quoted 5 lines
Quoted 3 lines
---
 .../bindings/net/pcs/mediatek,usxgmii.yaml    |  46 +-

Why are you changing the binding you just added?

Oh, that change slipped into the wrong commit by accident, sorry about that, I will fix it in the next iteration.

Quoted 11 lines

In any case, bindings are separate patches.

Quoted 8 lines
 MAINTAINERS                                   |   2 +
 drivers/net/pcs/Kconfig                       |  11 +
 drivers/net/pcs/Makefile                      |   1 +
 drivers/net/pcs/pcs-mtk-usxgmii.c             | 413 ++++++++++++++++++
 include/linux/pcs/pcs-mtk-usxgmii.h           |  26 ++
 6 files changed, 456 insertions(+), 43 deletions(-)
 create mode 100644 drivers/net/pcs/pcs-mtk-usxgmii.c
 create mode 100644 include/linux/pcs/pcs-mtk-usxgmii.h
#18 lore raw

Re: [RFC PATCH v2 5/8] net: pcs: add driver for MediaTek USXGMII PCS

In reply to Daniel Golle
Quoted 17 lines · Daniel Golle
drivers/net/pcs/pcs-mtk-usxgmii.c new file +17 -0
+struct phylink_pcs *mtk_usxgmii_select_pcs(struct device_node *np, phy_interface_t mode)
+{
+	struct platform_device *pdev;
+	struct mtk_usxgmii_pcs *mpcs;
+
+	if (!np)
+		return NULL;
+
+	if (!of_device_is_available(np))
+		return ERR_PTR(-ENODEV);
+
+	if (!of_match_node(mtk_usxgmii_of_mtable, np))
+		return ERR_PTR(-EINVAL);
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev || !platform_get_drvdata(pdev)) {
+		if (pdev)

drivers/net/pcs/pcs-mtk-usxgmii.c line 398

Quoted 10 lines
drivers/net/pcs/pcs-mtk-usxgmii.c new file +10 -0
+			put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	mpcs = platform_get_drvdata(pdev);
+	put_device(&pdev->dev);
+
+	return &mpcs->pcs;
+}
+EXPORT_SYMBOL(mtk_usxgmii_select_pcs);

All the same arguments apply here as per the lynxi driver.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
author maintainer
#19 lore raw

RFC PATCH v2 6/8 dt-bindings: net: mediatek: remove wrongly added clocks and SerDes

In reply to Daniel Golle

Several clocks as well as both sgmiisys phandles were added by mistake to the Ethernet bindings for MT7988.

This happened because the vendor driver which served as a reference uses a high number of syscon phandles to access various parts of the SoC which wasn't acceptable upstream. Hence several parts which have never previously been supported (such SerDes PHY and USXGMII PCS) have been moved to separate drivers which also result in a much more sane device tree.

Quickly align the bindings with the upcoming reality of the drivers actually adding full support for this SoC.

Fixes c94a9aabec36 ("dt-bindings: net: mediatek,net: add mt7988-eth binding")
---
 .../devicetree/bindings/net/mediatek,net.yaml | 32 ++++---------------
 1 file changed, 7 insertions(+), 25 deletions(-)
Documentation/devicetree/bindings/net/mediatek,net.yaml +7 -25
index e74502a0afe86..030d106bc7d3f 100644
@@ -337,32 +337,23 @@ allOf:
           minItems: 4
 
         clocks:
-          minItems: 34
-          maxItems: 34
+          minItems: 24
+          maxItems: 24
 
         clock-names:
           items:
-            - const: crypto
+            - const: xgp1
+            - const: xgp2
+            - const: xgp3
             - const: fe
             - const: gp2
             - const: gp1
             - const: gp3
+            - const: esw
+            - const: crypto
             - const: ethwarp_wocpu2
             - const: ethwarp_wocpu1
             - const: ethwarp_wocpu0
-            - const: esw
-            - const: netsys0
-            - const: netsys1
-            - const: sgmii_tx250m
-            - const: sgmii_rx250m
-            - const: sgmii2_tx250m
-            - const: sgmii2_rx250m
-            - const: top_usxgmii0_sel
-            - const: top_usxgmii1_sel
-            - const: top_sgm0_sel
-            - const: top_sgm1_sel
-            - const: top_xfi_phy0_xtal_sel
-            - const: top_xfi_phy1_xtal_sel
             - const: top_eth_gmii_sel
             - const: top_eth_refck_50m_sel
             - const: top_eth_sys_200m_sel
@@ -375,15 +366,6 @@ allOf:
             - const: top_netsys_sync_250m_sel
             - const: top_netsys_ppefb_250m_sel
             - const: top_netsys_warp_sel
-            - const: wocpu1
-            - const: wocpu0
-            - const: xgp1
-            - const: xgp2
-            - const: xgp3
-
-        mediatek,sgmiisys:
-          minItems: 2
-          maxItems: 2
 
 patternProperties:
   "^mac@[0-1]$":
Signature · 1 line
2.43.0
maintainer
#20 lore raw

Re: [RFC PATCH v2 6/8] dt-bindings: net: mediatek: remove wrongly added clocks and SerDes

Quoted 10 lines · Daniel Golle

Several clocks as well as both sgmiisys phandles were added by mistake to the Ethernet bindings for MT7988.

This happened because the vendor driver which served as a reference uses a high number of syscon phandles to access various parts of the SoC which wasn't acceptable upstream. Hence several parts which have never previously been supported (such SerDes PHY and USXGMII PCS) have been moved to separate drivers which also result in a much more sane device tree.

Quoted 9 lines

Quickly align the bindings with the upcoming reality of the drivers actually adding full support for this SoC.

Fixes c94a9aabec36 ("dt-bindings: net: mediatek,net: add mt7988-eth binding")
---
 .../devicetree/bindings/net/mediatek,net.yaml | 32 ++++---------------
 1 file changed, 7 insertions(+), 25 deletions(-)

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.example.dts:18:18: fatal error: dt-bindings/clock/mediatek,mt7988-clk.h: No such file or directory make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.example.dtb] Error 1

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/e2214e904cde9a9d64ec1e87ac38560f7ac35a1f.1701826319.git.daniel@makrotopia.org

The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.

author maintainer
#21 lore raw

RFC PATCH v2 7/8 dt-bindings: net: mediatek,net: fix and complete mt7988-eth binding

In reply to Daniel Golle

Complete support for MT7988 which comes with 3 MACs, SRAM for DMA descriptors and uses a dedicated PCS for the SerDes units.

Fixes c94a9aabec36 ("dt-bindings: net: mediatek,net: add mt7988-eth binding")
---
 .../devicetree/bindings/net/mediatek,net.yaml | 148 +++++++++++++++++-
 1 file changed, 146 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/net/mediatek,net.yaml +146 -2
index 030d106bc7d3f..ca0667c51c1c2 100644
@@ -28,7 +28,10 @@ properties:
       - ralink,rt5350-eth
 
   reg:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: Base of registers used to program the ethernet controller
+      - description: SRAM region used for DMA descriptors
 
   clocks: true
   clock-names: true
@@ -115,6 +118,9 @@ allOf:
               - mediatek,mt7623-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           maxItems: 3
 
@@ -149,6 +155,9 @@ allOf:
               - mediatek,mt7621-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           maxItems: 1
 
@@ -174,6 +183,9 @@ allOf:
             const: mediatek,mt7622-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           maxItems: 3
 
@@ -215,6 +227,9 @@ allOf:
             const: mediatek,mt7629-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           maxItems: 3
 
@@ -257,6 +272,9 @@ allOf:
             const: mediatek,mt7981-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           minItems: 4
 
@@ -295,6 +313,9 @@ allOf:
             const: mediatek,mt7986-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           minItems: 4
 
@@ -333,8 +354,13 @@ allOf:
             const: mediatek,mt7988-eth
     then:
       properties:
+        reg:
+          maxItems: 2
+          minItems: 2
+
         interrupts:
           minItems: 4
+          maxItems: 4
 
         clocks:
           minItems: 24
@@ -368,7 +394,7 @@ allOf:
             - const: top_netsys_warp_sel
 
 patternProperties:
-  "^mac@[0-1]$":
+  "^mac@[0-2]$":
     type: object
     unevaluatedProperties: false
     allOf:
@@ -382,6 +408,9 @@ patternProperties:
       reg:
         maxItems: 1
 
+      phys:
+        maxItems: 1
+
     required:
       - reg
       - compatible
@@ -559,3 +588,118 @@ examples:
         };
       };
     };
+
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/mediatek,mt7988-clk.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      ethernet@15100000 {
+        compatible = "mediatek,mt7988-eth";
+        reg = <0 0x15100000 0 0x80000>, <0 0x15400000 0 0x380000>;
+        interrupts = <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>;
+
+        clocks = <&ethsys CLK_ETHDMA_XGP1_EN>,
+                 <&ethsys CLK_ETHDMA_XGP2_EN>,
+                 <&ethsys CLK_ETHDMA_XGP3_EN>,
+                 <&ethsys CLK_ETHDMA_FE_EN>,
+                 <&ethsys CLK_ETHDMA_GP2_EN>,
+                 <&ethsys CLK_ETHDMA_GP1_EN>,
+                 <&ethsys CLK_ETHDMA_GP3_EN>,
+                 <&ethsys CLK_ETHDMA_ESW_EN>,
+                 <&ethsys CLK_ETHDMA_CRYPT0_EN>,
+                 <&ethwarp CLK_ETHWARP_WOCPU2_EN>,
+                 <&ethwarp CLK_ETHWARP_WOCPU1_EN>,
+                 <&ethwarp CLK_ETHWARP_WOCPU0_EN>,
+                 <&topckgen CLK_TOP_ETH_GMII_SEL>,
+                 <&topckgen CLK_TOP_ETH_REFCK_50M_SEL>,
+                 <&topckgen CLK_TOP_ETH_SYS_200M_SEL>,
+                 <&topckgen CLK_TOP_ETH_SYS_SEL>,
+                 <&topckgen CLK_TOP_ETH_XGMII_SEL>,
+                 <&topckgen CLK_TOP_ETH_MII_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_500M_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_PAO_2X_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_SYNC_250M_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_PPEFB_250M_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_WARP_SEL>;
+
+        clock-names = "xgp1", "xgp2", "xgp3", "fe", "gp2", "gp1",
+                      "gp3", "esw", "crypto",
+                      "ethwarp_wocpu2", "ethwarp_wocpu1",
+                      "ethwarp_wocpu0", "top_eth_gmii_sel",
+                      "top_eth_refck_50m_sel", "top_eth_sys_200m_sel",
+                      "top_eth_sys_sel", "top_eth_xgmii_sel",
+                      "top_eth_mii_sel", "top_netsys_sel",
+                      "top_netsys_500m_sel", "top_netsys_pao_2x_sel",
+                      "top_netsys_sync_250m_sel",
+                      "top_netsys_ppefb_250m_sel",
+                      "top_netsys_warp_sel";
+        assigned-clocks = <&topckgen CLK_TOP_NETSYS_2X_SEL>,
+                          <&topckgen CLK_TOP_NETSYS_GSW_SEL>,
+                          <&topckgen CLK_TOP_USXGMII_SBUS_0_SEL>,
+                          <&topckgen CLK_TOP_USXGMII_SBUS_1_SEL>,
+                          <&topckgen CLK_TOP_SGM_0_SEL>,
+                          <&topckgen CLK_TOP_SGM_1_SEL>;
+        assigned-clock-parents = <&apmixedsys CLK_APMIXED_NET2PLL>,
+                                 <&topckgen CLK_TOP_NET1PLL_D4>,
+                                 <&topckgen CLK_TOP_NET1PLL_D8_D4>,
+                                 <&topckgen CLK_TOP_NET1PLL_D8_D4>,
+                                 <&apmixedsys CLK_APMIXED_SGMPLL>,
+                                 <&apmixedsys CLK_APMIXED_SGMPLL>;
+        mediatek,ethsys = <&ethsys>;
+        mediatek,infracfg = <&topmisc>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mac@0 {
+          compatible = "mediatek,eth-mac";
+          reg = <0>;
+          phy-mode = "internal"; /* CPU port of built-in 1GE switch */
+
+          fixed-link {
+            speed = <10000>;
+            full-duplex;
+            pause;
+          };
+        };
+
+        mac@1 {
+          compatible = "mediatek,eth-mac";
+          reg = <1>;
+          phy-handle = <&int_2p5g_phy>;
+        };
+
+        mac@2 {
+          compatible = "mediatek,eth-mac";
+          reg = <2>;
+          pcs-handle = <&usxgmiisys0>;
+          phy-handle = <&phy0>;
+        };
+
+        mdio_bus: mdio-bus {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          /* external PHY */
+          phy0: ethernet-phy@0 {
+            reg = <0>;
+            compatible = "ethernet-phy-ieee802.3-c45";
+          };
+
+          /* internal 2.5G PHY */
+          int_2p5g_phy: ethernet-phy@15 {
+            reg = <15>;
+            compatible = "ethernet-phy-ieee802.3-c45";
+            phy-mode = "internal";
+          };
+        };
+      };
+    };
Signature · 1 line
2.43.0
maintainer
#22 lore raw

Re: [RFC PATCH v2 7/8] dt-bindings: net: mediatek,net: fix and complete mt7988-eth binding

Quoted 9 lines · Daniel Golle

Complete support for MT7988 which comes with 3 MACs, SRAM for DMA descriptors and uses a dedicated PCS for the SerDes units.

Fixes c94a9aabec36 ("dt-bindings: net: mediatek,net: add mt7988-eth binding")
---
 .../devicetree/bindings/net/mediatek,net.yaml | 148 +++++++++++++++++-
 1 file changed, 146 insertions(+), 2 deletions(-)

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/net/mediatek,net.example.dts:231:18: fatal error: dt-bindings/clock/mediatek,mt7988-clk.h: No such file or directory
231 | #include <dt-bindings/clock/mediatek,mt7988-clk.h>
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/net/mediatek,net.example.dtb] Error 1

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/567c6aaa64ecb4872056bc0105c70153fd9d9b50.1701826319.git.daniel@makrotopia.org

The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.

maintainer
#23 lore raw

Re: [RFC PATCH v2 7/8] dt-bindings: net: mediatek,net: fix and complete mt7988-eth binding

In reply to Daniel Golle
Quoted 14 lines · Daniel Golle

Complete support for MT7988 which comes with 3 MACs, SRAM for DMA descriptors and uses a dedicated PCS for the SerDes units.

Fixes c94a9aabec36 ("dt-bindings: net: mediatek,net: add mt7988-eth binding")
---
 .../devicetree/bindings/net/mediatek,net.yaml | 148 +++++++++++++++++-
 1 file changed, 146 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/net/mediatek,net.yaml +0 -0
index 030d106bc7d3f..ca0667c51c1c2 100644

Documentation/devicetree/bindings/net/mediatek,net.yaml properties: · line 34

Quoted 8 lines
Documentation/devicetree/bindings/net/mediatek,net.yaml +4 -1
@@ -28,7 +28,10 @@ properties:
       - ralink,rt5350-eth
 
   reg:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: Base of registers used to program the ethernet controller
+      - description: SRAM region used for DMA descriptors

Is this a dedicated SRAM for this purpose, or a common one partitioned up. mmio-sram and a phandle is how to do the latter.

Quoted 64 lines
Documentation/devicetree/bindings/net/mediatek,net.yaml +18 -0
   clocks: true
   clock-names: true
@@ -115,6 +118,9 @@ allOf:
               - mediatek,mt7623-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           maxItems: 3
 
@@ -149,6 +155,9 @@ allOf:
               - mediatek,mt7621-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           maxItems: 1
 
@@ -174,6 +183,9 @@ allOf:
             const: mediatek,mt7622-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           maxItems: 3
 
@@ -215,6 +227,9 @@ allOf:
             const: mediatek,mt7629-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           maxItems: 3
 
@@ -257,6 +272,9 @@ allOf:
             const: mediatek,mt7981-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           minItems: 4
 
@@ -295,6 +313,9 @@ allOf:
             const: mediatek,mt7986-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           minItems: 4
 

Documentation/devicetree/bindings/net/mediatek,net.yaml allOf: · line 358

Quoted 5 lines
Documentation/devicetree/bindings/net/mediatek,net.yaml +2 -0
@@ -333,8 +354,13 @@ allOf:
             const: mediatek,mt7988-eth
     then:
       properties:
+        reg:
+          maxItems: 2

Don't need maxItems here. That's already the max.

Quoted 28 lines
Documentation/devicetree/bindings/net/mediatek,net.yaml +7 -1
+          minItems: 2
+
         interrupts:
           minItems: 4
+          maxItems: 4
 
         clocks:
           minItems: 24
@@ -368,7 +394,7 @@ allOf:
             - const: top_netsys_warp_sel
 
 patternProperties:
-  "^mac@[0-1]$":
+  "^mac@[0-2]$":
     type: object
     unevaluatedProperties: false
     allOf:
@@ -382,6 +408,9 @@ patternProperties:
       reg:
         maxItems: 1
 
+      phys:
+        maxItems: 1
+
     required:
       - reg
       - compatible

Documentation/devicetree/bindings/net/mediatek,net.yaml examples: · line 595

Quoted 8 lines
Documentation/devicetree/bindings/net/mediatek,net.yaml +5 -0
@@ -559,3 +588,118 @@ examples:
         };
       };
     };
+
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/mediatek,mt7988-clk.h>

Why is fixing the binding needing a new example? Is this example really different enough to justify a whole other example?

Quoted 112 lines
Documentation/devicetree/bindings/net/mediatek,net.yaml +110 -0
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      ethernet@15100000 {
+        compatible = "mediatek,mt7988-eth";
+        reg = <0 0x15100000 0 0x80000>, <0 0x15400000 0 0x380000>;
+        interrupts = <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>;
+
+        clocks = <&ethsys CLK_ETHDMA_XGP1_EN>,
+                 <&ethsys CLK_ETHDMA_XGP2_EN>,
+                 <&ethsys CLK_ETHDMA_XGP3_EN>,
+                 <&ethsys CLK_ETHDMA_FE_EN>,
+                 <&ethsys CLK_ETHDMA_GP2_EN>,
+                 <&ethsys CLK_ETHDMA_GP1_EN>,
+                 <&ethsys CLK_ETHDMA_GP3_EN>,
+                 <&ethsys CLK_ETHDMA_ESW_EN>,
+                 <&ethsys CLK_ETHDMA_CRYPT0_EN>,
+                 <&ethwarp CLK_ETHWARP_WOCPU2_EN>,
+                 <&ethwarp CLK_ETHWARP_WOCPU1_EN>,
+                 <&ethwarp CLK_ETHWARP_WOCPU0_EN>,
+                 <&topckgen CLK_TOP_ETH_GMII_SEL>,
+                 <&topckgen CLK_TOP_ETH_REFCK_50M_SEL>,
+                 <&topckgen CLK_TOP_ETH_SYS_200M_SEL>,
+                 <&topckgen CLK_TOP_ETH_SYS_SEL>,
+                 <&topckgen CLK_TOP_ETH_XGMII_SEL>,
+                 <&topckgen CLK_TOP_ETH_MII_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_500M_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_PAO_2X_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_SYNC_250M_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_PPEFB_250M_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_WARP_SEL>;
+
+        clock-names = "xgp1", "xgp2", "xgp3", "fe", "gp2", "gp1",
+                      "gp3", "esw", "crypto",
+                      "ethwarp_wocpu2", "ethwarp_wocpu1",
+                      "ethwarp_wocpu0", "top_eth_gmii_sel",
+                      "top_eth_refck_50m_sel", "top_eth_sys_200m_sel",
+                      "top_eth_sys_sel", "top_eth_xgmii_sel",
+                      "top_eth_mii_sel", "top_netsys_sel",
+                      "top_netsys_500m_sel", "top_netsys_pao_2x_sel",
+                      "top_netsys_sync_250m_sel",
+                      "top_netsys_ppefb_250m_sel",
+                      "top_netsys_warp_sel";
+        assigned-clocks = <&topckgen CLK_TOP_NETSYS_2X_SEL>,
+                          <&topckgen CLK_TOP_NETSYS_GSW_SEL>,
+                          <&topckgen CLK_TOP_USXGMII_SBUS_0_SEL>,
+                          <&topckgen CLK_TOP_USXGMII_SBUS_1_SEL>,
+                          <&topckgen CLK_TOP_SGM_0_SEL>,
+                          <&topckgen CLK_TOP_SGM_1_SEL>;
+        assigned-clock-parents = <&apmixedsys CLK_APMIXED_NET2PLL>,
+                                 <&topckgen CLK_TOP_NET1PLL_D4>,
+                                 <&topckgen CLK_TOP_NET1PLL_D8_D4>,
+                                 <&topckgen CLK_TOP_NET1PLL_D8_D4>,
+                                 <&apmixedsys CLK_APMIXED_SGMPLL>,
+                                 <&apmixedsys CLK_APMIXED_SGMPLL>;
+        mediatek,ethsys = <&ethsys>;
+        mediatek,infracfg = <&topmisc>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mac@0 {
+          compatible = "mediatek,eth-mac";
+          reg = <0>;
+          phy-mode = "internal"; /* CPU port of built-in 1GE switch */
+
+          fixed-link {
+            speed = <10000>;
+            full-duplex;
+            pause;
+          };
+        };
+
+        mac@1 {
+          compatible = "mediatek,eth-mac";
+          reg = <1>;
+          phy-handle = <&int_2p5g_phy>;
+        };
+
+        mac@2 {
+          compatible = "mediatek,eth-mac";
+          reg = <2>;
+          pcs-handle = <&usxgmiisys0>;
+          phy-handle = <&phy0>;
+        };
+
+        mdio_bus: mdio-bus {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          /* external PHY */
+          phy0: ethernet-phy@0 {
+            reg = <0>;
+            compatible = "ethernet-phy-ieee802.3-c45";
+          };
+
+          /* internal 2.5G PHY */
+          int_2p5g_phy: ethernet-phy@15 {
+            reg = <15>;
+            compatible = "ethernet-phy-ieee802.3-c45";
+            phy-mode = "internal";
+          };
+        };
+      };
+    };
Signature · 1 line
2.43.0
author maintainer
#24 lore raw

Re: [RFC PATCH v2 7/8] dt-bindings: net: mediatek,net: fix and complete mt7988-eth binding

Quoted 17 lines · Rob Herring
Quoted 16 lines · Daniel Golle

Complete support for MT7988 which comes with 3 MACs, SRAM for DMA descriptors and uses a dedicated PCS for the SerDes units.

Fixes c94a9aabec36 ("dt-bindings: net: mediatek,net: add mt7988-eth binding")
---
 .../devicetree/bindings/net/mediatek,net.yaml | 148 +++++++++++++++++-
 1 file changed, 146 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/net/mediatek,net.yaml +0 -0
index 030d106bc7d3f..ca0667c51c1c2 100644
@@ -28,7 +28,10 @@ properties:
       - ralink,rt5350-eth
 
Quoted 9 lines

Documentation/devicetree/bindings/net/mediatek,net.yaml properties: · line 34

Quoted 6 lines
Documentation/devicetree/bindings/net/mediatek,net.yaml +4 -1
   reg:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: Base of registers used to program the ethernet controller
+      - description: SRAM region used for DMA descriptors

Is this a dedicated SRAM for this purpose, or a common one partitioned up. mmio-sram and a phandle is how to do the latter.

The SRAM memory sits just next to the Ethernet and dedicated for this purpose, at least in theory. I'm not aware of the internal details here.

Quoted 64 lines
Quoted 64 lines
Documentation/devicetree/bindings/net/mediatek,net.yaml +18 -0
   clocks: true
   clock-names: true
@@ -115,6 +118,9 @@ allOf:
               - mediatek,mt7623-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           maxItems: 3
 
@@ -149,6 +155,9 @@ allOf:
               - mediatek,mt7621-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           maxItems: 1
 
@@ -174,6 +183,9 @@ allOf:
             const: mediatek,mt7622-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           maxItems: 3
 
@@ -215,6 +227,9 @@ allOf:
             const: mediatek,mt7629-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           maxItems: 3
 
@@ -257,6 +272,9 @@ allOf:
             const: mediatek,mt7981-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           minItems: 4
 
@@ -295,6 +313,9 @@ allOf:
             const: mediatek,mt7986-eth
     then:
       properties:
+        reg:
+          maxItems: 1
+
         interrupts:
           minItems: 4
 
Quoted 8 lines

Documentation/devicetree/bindings/net/mediatek,net.yaml allOf: · line 358

Quoted 6 lines
Documentation/devicetree/bindings/net/mediatek,net.yaml +2 -0
@@ -333,8 +354,13 @@ allOf:
             const: mediatek,mt7988-eth
     then:
       properties:
+        reg:
+          maxItems: 2

Don't need maxItems here. That's already the max.

Ack.

Quoted 33 lines
Quoted 33 lines
Documentation/devicetree/bindings/net/mediatek,net.yaml +8 -1
+          minItems: 2
+
         interrupts:
           minItems: 4
+          maxItems: 4
 
         clocks:
           minItems: 24
@@ -368,7 +394,7 @@ allOf:
             - const: top_netsys_warp_sel
 
 patternProperties:
-  "^mac@[0-1]$":
+  "^mac@[0-2]$":
     type: object
     unevaluatedProperties: false
     allOf:
@@ -382,6 +408,9 @@ patternProperties:
       reg:
         maxItems: 1
 
+      phys:
+        maxItems: 1
+
     required:
       - reg
       - compatible
@@ -559,3 +588,118 @@ examples:
         };
       };
     };
+
Quoted 8 lines

Documentation/devicetree/bindings/net/mediatek,net.yaml examples: · line 595

Quoted 4 lines
Documentation/devicetree/bindings/net/mediatek,net.yaml +4 -0
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/mediatek,mt7988-clk.h>

Why is fixing the binding needing a new example? Is this example really different enough to justify a whole other example?

Starting from MT7988 the PCS needs to be referenced as pcs-handle in the MAC node, so that is a bit different. If you don't think that justifies including the additional example I will drop that.

Quoted 113 lines
Quoted 113 lines
Documentation/devicetree/bindings/net/mediatek,net.yaml +110 -0
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      ethernet@15100000 {
+        compatible = "mediatek,mt7988-eth";
+        reg = <0 0x15100000 0 0x80000>, <0 0x15400000 0 0x380000>;
+        interrupts = <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>;
+
+        clocks = <&ethsys CLK_ETHDMA_XGP1_EN>,
+                 <&ethsys CLK_ETHDMA_XGP2_EN>,
+                 <&ethsys CLK_ETHDMA_XGP3_EN>,
+                 <&ethsys CLK_ETHDMA_FE_EN>,
+                 <&ethsys CLK_ETHDMA_GP2_EN>,
+                 <&ethsys CLK_ETHDMA_GP1_EN>,
+                 <&ethsys CLK_ETHDMA_GP3_EN>,
+                 <&ethsys CLK_ETHDMA_ESW_EN>,
+                 <&ethsys CLK_ETHDMA_CRYPT0_EN>,
+                 <&ethwarp CLK_ETHWARP_WOCPU2_EN>,
+                 <&ethwarp CLK_ETHWARP_WOCPU1_EN>,
+                 <&ethwarp CLK_ETHWARP_WOCPU0_EN>,
+                 <&topckgen CLK_TOP_ETH_GMII_SEL>,
+                 <&topckgen CLK_TOP_ETH_REFCK_50M_SEL>,
+                 <&topckgen CLK_TOP_ETH_SYS_200M_SEL>,
+                 <&topckgen CLK_TOP_ETH_SYS_SEL>,
+                 <&topckgen CLK_TOP_ETH_XGMII_SEL>,
+                 <&topckgen CLK_TOP_ETH_MII_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_500M_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_PAO_2X_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_SYNC_250M_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_PPEFB_250M_SEL>,
+                 <&topckgen CLK_TOP_NETSYS_WARP_SEL>;
+
+        clock-names = "xgp1", "xgp2", "xgp3", "fe", "gp2", "gp1",
+                      "gp3", "esw", "crypto",
+                      "ethwarp_wocpu2", "ethwarp_wocpu1",
+                      "ethwarp_wocpu0", "top_eth_gmii_sel",
+                      "top_eth_refck_50m_sel", "top_eth_sys_200m_sel",
+                      "top_eth_sys_sel", "top_eth_xgmii_sel",
+                      "top_eth_mii_sel", "top_netsys_sel",
+                      "top_netsys_500m_sel", "top_netsys_pao_2x_sel",
+                      "top_netsys_sync_250m_sel",
+                      "top_netsys_ppefb_250m_sel",
+                      "top_netsys_warp_sel";
+        assigned-clocks = <&topckgen CLK_TOP_NETSYS_2X_SEL>,
+                          <&topckgen CLK_TOP_NETSYS_GSW_SEL>,
+                          <&topckgen CLK_TOP_USXGMII_SBUS_0_SEL>,
+                          <&topckgen CLK_TOP_USXGMII_SBUS_1_SEL>,
+                          <&topckgen CLK_TOP_SGM_0_SEL>,
+                          <&topckgen CLK_TOP_SGM_1_SEL>;
+        assigned-clock-parents = <&apmixedsys CLK_APMIXED_NET2PLL>,
+                                 <&topckgen CLK_TOP_NET1PLL_D4>,
+                                 <&topckgen CLK_TOP_NET1PLL_D8_D4>,
+                                 <&topckgen CLK_TOP_NET1PLL_D8_D4>,
+                                 <&apmixedsys CLK_APMIXED_SGMPLL>,
+                                 <&apmixedsys CLK_APMIXED_SGMPLL>;
+        mediatek,ethsys = <&ethsys>;
+        mediatek,infracfg = <&topmisc>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mac@0 {
+          compatible = "mediatek,eth-mac";
+          reg = <0>;
+          phy-mode = "internal"; /* CPU port of built-in 1GE switch */
+
+          fixed-link {
+            speed = <10000>;
+            full-duplex;
+            pause;
+          };
+        };
+
+        mac@1 {
+          compatible = "mediatek,eth-mac";
+          reg = <1>;
+          phy-handle = <&int_2p5g_phy>;
+        };
+
+        mac@2 {
+          compatible = "mediatek,eth-mac";
+          reg = <2>;
+          pcs-handle = <&usxgmiisys0>;
+          phy-handle = <&phy0>;
+        };
+
+        mdio_bus: mdio-bus {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          /* external PHY */
+          phy0: ethernet-phy@0 {
+            reg = <0>;
+            compatible = "ethernet-phy-ieee802.3-c45";
+          };
+
+          /* internal 2.5G PHY */
+          int_2p5g_phy: ethernet-phy@15 {
+            reg = <15>;
+            compatible = "ethernet-phy-ieee802.3-c45";
+            phy-mode = "internal";
+          };
+        };
+      };
+    };
Signature · 1 line
2.43.0
author maintainer
#25 lore raw

RFC PATCH v2 8/8 net: ethernet: mtk_eth_soc: add paths and SerDes modes for MT7988

In reply to Daniel Golle

MT7988 comes with a built-in 2.5G PHY as well as SerDes lanes to connect external PHYs or transceivers in USXGMII, 10GBase-R, 5GBase-R, 2500Base-X, 1000Base-X and Cisco SGMII interface modes.

Implement support for configuring for the new paths to SerDes interfaces and the internal 2.5G PHY.

Add USXGMII PCS driver for 10GBase-R, 5GBase-R and USXGMII mode, and setup the new PHYA on MT7988 to access the also still existing old LynxI PCS for 1000Base-X, 2500Base-X and Cisco SGMII PCS interface modes.

---
 drivers/net/ethernet/mediatek/mtk_eth_path.c | 122 +++++++-
 drivers/net/ethernet/mediatek/mtk_eth_soc.c  | 284 +++++++++++++++++--
 drivers/net/ethernet/mediatek/mtk_eth_soc.h  | 107 +++++--
 3 files changed, 462 insertions(+), 51 deletions(-)
drivers/net/ethernet/mediatek/mtk_eth_path.c +118 -4
index 7c27a19c4d8f4..3f4f4cfe6a233 100644
@@ -31,10 +31,20 @@ static const char *mtk_eth_path_name(u64 path)
 		return "gmac2_rgmii";
 	case MTK_ETH_PATH_GMAC2_SGMII:
 		return "gmac2_sgmii";
+	case MTK_ETH_PATH_GMAC2_2P5GPHY:
+		return "gmac2_2p5gphy";
 	case MTK_ETH_PATH_GMAC2_GEPHY:
 		return "gmac2_gephy";
+	case MTK_ETH_PATH_GMAC3_SGMII:
+		return "gmac3_sgmii";
 	case MTK_ETH_PATH_GDM1_ESW:
 		return "gdm1_esw";
+	case MTK_ETH_PATH_GMAC1_USXGMII:
+		return "gmac1_usxgmii";
+	case MTK_ETH_PATH_GMAC2_USXGMII:
+		return "gmac2_usxgmii";
+	case MTK_ETH_PATH_GMAC3_USXGMII:
+		return "gmac3_usxgmii";
 	default:
 		return "unknown path";
 	}
@@ -127,6 +137,27 @@ static int set_mux_u3_gmac2_to_qphy(struct mtk_eth *eth, u64 path)
 	return 0;
 }
 
+static int set_mux_gmac2_to_2p5gphy(struct mtk_eth *eth, u64 path)
+{
+	int ret;
+
+	if (path == MTK_ETH_PATH_GMAC2_2P5GPHY) {
+		ret = regmap_clear_bits(eth->ethsys, ETHSYS_SYSCFG0, SYSCFG0_SGMII_GMAC2_V2);
+		if (ret)
+			return ret;
+
+		/* Setup mux to 2p5g PHY */
+		ret = regmap_clear_bits(eth->infra, TOP_MISC_NETSYS_PCS_MUX, MUX_G2_USXGMII_SEL);
+		if (ret)
+			return ret;
+
+		dev_dbg(eth->dev, "path %s in %s updated\n",
+			mtk_eth_path_name(path), __func__);
+	}
+
+	return 0;
+}
+
 static int set_mux_gmac1_gmac2_to_sgmii_rgmii(struct mtk_eth *eth, u64 path)
 {
 	unsigned int val = 0;
@@ -165,7 +196,48 @@ static int set_mux_gmac1_gmac2_to_sgmii_rgmii(struct mtk_eth *eth, u64 path)
 	return 0;
 }
 
-static int set_mux_gmac12_to_gephy_sgmii(struct mtk_eth *eth, u64 path)
+static int set_mux_gmac123_to_usxgmii(struct mtk_eth *eth, u64 path)
+{
+	unsigned int val = 0;
+	bool updated = true;
+	int mac_id = 0;
+
+	/* Disable SYSCFG1 SGMII */
+	regmap_read(eth->ethsys, ETHSYS_SYSCFG0, &val);
+
+	switch (path) {
+	case MTK_ETH_PATH_GMAC1_USXGMII:
+		val &= ~(u32)SYSCFG0_SGMII_GMAC1_V2;
+		mac_id = MTK_GMAC1_ID;
+		break;
+	case MTK_ETH_PATH_GMAC2_USXGMII:
+		val &= ~(u32)SYSCFG0_SGMII_GMAC2_V2;
+		mac_id = MTK_GMAC2_ID;
+		break;
+	case MTK_ETH_PATH_GMAC3_USXGMII:
+		val &= ~(u32)SYSCFG0_SGMII_GMAC3_V2;
+		mac_id = MTK_GMAC3_ID;
+		break;
+	default:
+		updated = false;
+	};
+
+	if (updated) {
+		regmap_update_bits(eth->ethsys, ETHSYS_SYSCFG0,
+				   SYSCFG0_SGMII_MASK, val);
+
+		if (mac_id == MTK_GMAC2_ID)
+			regmap_set_bits(eth->infra, TOP_MISC_NETSYS_PCS_MUX,
+					MUX_G2_USXGMII_SEL);
+	}
+
+	dev_dbg(eth->dev, "path %s in %s updated = %d\n",
+		mtk_eth_path_name(path), __func__, updated);
+
+	return 0;
+}
+
+static int set_mux_gmac123_to_gephy_sgmii(struct mtk_eth *eth, u64 path)
 {
 	unsigned int val = 0;
 	bool updated = true;
@@ -182,6 +254,9 @@ static int set_mux_gmac12_to_gephy_sgmii(struct mtk_eth *eth, u64 path)
 	case MTK_ETH_PATH_GMAC2_SGMII:
 		val |= SYSCFG0_SGMII_GMAC2_V2;
 		break;
+	case MTK_ETH_PATH_GMAC3_SGMII:
+		val |= SYSCFG0_SGMII_GMAC3_V2;
+		break;
 	default:
 		updated = false;
 	}
@@ -209,6 +284,10 @@ static const struct mtk_eth_muxc mtk_eth_muxc[] = {
 		.name = "mux_u3_gmac2_to_qphy",
 		.cap_bit = MTK_ETH_MUX_U3_GMAC2_TO_QPHY,
 		.set_path = set_mux_u3_gmac2_to_qphy,
+	}, {
+		.name = "mux_gmac2_to_2p5gphy",
+		.cap_bit = MTK_ETH_MUX_GMAC2_TO_2P5GPHY,
+		.set_path = set_mux_gmac2_to_2p5gphy,
 	}, {
 		.name = "mux_gmac1_gmac2_to_sgmii_rgmii",
 		.cap_bit = MTK_ETH_MUX_GMAC1_GMAC2_TO_SGMII_RGMII,
@@ -216,7 +295,15 @@ static const struct mtk_eth_muxc mtk_eth_muxc[] = {
 	}, {
 		.name = "mux_gmac12_to_gephy_sgmii",
 		.cap_bit = MTK_ETH_MUX_GMAC12_TO_GEPHY_SGMII,
-		.set_path = set_mux_gmac12_to_gephy_sgmii,
+		.set_path = set_mux_gmac123_to_gephy_sgmii,
+	}, {
+		.name = "mux_gmac123_to_gephy_sgmii",
+		.cap_bit = MTK_ETH_MUX_GMAC123_TO_GEPHY_SGMII,
+		.set_path = set_mux_gmac123_to_gephy_sgmii,
+	}, {
+		.name = "mux_gmac123_to_usxgmii",
+		.cap_bit = MTK_ETH_MUX_GMAC123_TO_USXGMII,
+		.set_path = set_mux_gmac123_to_usxgmii,
 	},
 };
 
@@ -249,12 +336,39 @@ static int mtk_eth_mux_setup(struct mtk_eth *eth, u64 path)
 	return err;
 }
 
+int mtk_gmac_usxgmii_path_setup(struct mtk_eth *eth, int mac_id)
+{
+	u64 path;
+
+	path = (mac_id == MTK_GMAC1_ID) ?  MTK_ETH_PATH_GMAC1_USXGMII :
+	       (mac_id == MTK_GMAC2_ID) ?  MTK_ETH_PATH_GMAC2_USXGMII :
+					   MTK_ETH_PATH_GMAC3_USXGMII;
+
+	/* Setup proper MUXes along the path */
+	return mtk_eth_mux_setup(eth, path);
+}
+
 int mtk_gmac_sgmii_path_setup(struct mtk_eth *eth, int mac_id)
 {
 	u64 path;
 
-	path = (mac_id == 0) ?  MTK_ETH_PATH_GMAC1_SGMII :
-				MTK_ETH_PATH_GMAC2_SGMII;
+	path = (mac_id == MTK_GMAC1_ID) ? MTK_ETH_PATH_GMAC1_SGMII :
+	       (mac_id == MTK_GMAC2_ID) ? MTK_ETH_PATH_GMAC2_SGMII :
+					  MTK_ETH_PATH_GMAC3_SGMII;
+
+	/* Setup proper MUXes along the path */
+	return mtk_eth_mux_setup(eth, path);
+}
+
+int mtk_gmac_2p5gphy_path_setup(struct mtk_eth *eth, int mac_id)
+{
+	u64 path = 0;
+
+	if (mac_id == MTK_GMAC2_ID)
+		path = MTK_ETH_PATH_GMAC2_2P5GPHY;
+
+	if (!path)
+		return -EINVAL;
 
 	/* Setup proper MUXes along the path */
 	return mtk_eth_mux_setup(eth, path);
drivers/net/ethernet/mediatek/mtk_eth_soc.c +253 -31

424 lines not on this page · raw message

drivers/net/ethernet/mediatek/mtk_eth_soc.h +91 -16

249 lines not on this page · raw message

Signature · 1 line
2.43.0
#26 lore raw

Re: [RFC PATCH v2 8/8] net: ethernet: mtk_eth_soc: add paths and SerDes modes for MT7988

Quoted 4 lines · Daniel Golle
drivers/net/ethernet/mediatek/mtk_eth_soc.c +0 -0
@@ -516,6 +538,21 @@ static struct phylink_pcs *mtk_mac_select_pcs(struct phylink_config *config,
 	struct mtk_eth *eth = mac->hw;
 	unsigned int sid;
 

drivers/net/ethernet/mediatek/mtk_eth_soc.c static struct phylink_pcs *mtk_mac_select_pcs(struct phylink_config *config, · line 550

Quoted 10 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +10 -0
+	if (mtk_is_netsys_v3_or_greater(eth)) {
+		switch (interface) {
+		case PHY_INTERFACE_MODE_1000BASEX:
+		case PHY_INTERFACE_MODE_2500BASEX:
+		case PHY_INTERFACE_MODE_SGMII:
+			return mtk_pcs_lynxi_select_pcs(mac->sgmii_pcs_of_node, interface);
+		case PHY_INTERFACE_MODE_5GBASER:
+		case PHY_INTERFACE_MODE_10GBASER:
+		case PHY_INTERFACE_MODE_USXGMII:
+			return mtk_usxgmii_select_pcs(mac->usxgmii_pcs_of_node, interface);

From what I can see, neither of these two "select_pcs" methods that you're calling makes any use of the "interface" you pass to them. I'm not sure what they _could_ do with it either, given that what you're effectively doing here is getting the phylink_pcs structure from the driver, and each one only has a single phylink_pcs.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
author maintainer
#27 lore raw

Re: [RFC PATCH v2 8/8] net: ethernet: mtk_eth_soc: add paths and SerDes modes for MT7988

Quoted 5 lines · Russell King (Oracle)
Quoted 4 lines · Daniel Golle
drivers/net/ethernet/mediatek/mtk_eth_soc.c +0 -0
@@ -516,6 +538,21 @@ static struct phylink_pcs *mtk_mac_select_pcs(struct phylink_config *config,
 	struct mtk_eth *eth = mac->hw;
 	unsigned int sid;
 
Quoted 16 lines

drivers/net/ethernet/mediatek/mtk_eth_soc.c static struct phylink_pcs *mtk_mac_select_pcs(struct phylink_config *config, · line 550

Quoted 10 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +10 -0
+	if (mtk_is_netsys_v3_or_greater(eth)) {
+		switch (interface) {
+		case PHY_INTERFACE_MODE_1000BASEX:
+		case PHY_INTERFACE_MODE_2500BASEX:
+		case PHY_INTERFACE_MODE_SGMII:
+			return mtk_pcs_lynxi_select_pcs(mac->sgmii_pcs_of_node, interface);
+		case PHY_INTERFACE_MODE_5GBASER:
+		case PHY_INTERFACE_MODE_10GBASER:
+		case PHY_INTERFACE_MODE_USXGMII:
+			return mtk_usxgmii_select_pcs(mac->usxgmii_pcs_of_node, interface);

From what I can see, neither of these two "select_pcs" methods that you're calling makes any use of the "interface" you pass to them. I'm not sure what they _could_ do with it either, given that what you're effectively doing here is getting the phylink_pcs structure from the driver, and each one only has a single phylink_pcs.

Yes, you are right, the interface parameter isn't used, I will drop it from both mtk_*_select_pcs() prototypes.

In the long run we may want something like
struct phylink_pcs *of_pcs_get(struct device_node *np, phy_interface_t interface) provided by a to-be-built drivers/net/pcs/core.c...

#28 lore raw

Re: [RFC PATCH v2 8/8] net: ethernet: mtk_eth_soc: add paths and SerDes modes for MT7988

Quoted 23 lines · Daniel Golle
Quoted 5 lines · Russell King (Oracle)
Quoted 4 lines · Daniel Golle
drivers/net/ethernet/mediatek/mtk_eth_soc.c +0 -0
@@ -516,6 +538,21 @@ static struct phylink_pcs *mtk_mac_select_pcs(struct phylink_config *config,
 	struct mtk_eth *eth = mac->hw;
 	unsigned int sid;
 
Quoted 16 lines

drivers/net/ethernet/mediatek/mtk_eth_soc.c static struct phylink_pcs *mtk_mac_select_pcs(struct phylink_config *config, · line 550

Quoted 10 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +10 -0
+	if (mtk_is_netsys_v3_or_greater(eth)) {
+		switch (interface) {
+		case PHY_INTERFACE_MODE_1000BASEX:
+		case PHY_INTERFACE_MODE_2500BASEX:
+		case PHY_INTERFACE_MODE_SGMII:
+			return mtk_pcs_lynxi_select_pcs(mac->sgmii_pcs_of_node, interface);
+		case PHY_INTERFACE_MODE_5GBASER:
+		case PHY_INTERFACE_MODE_10GBASER:
+		case PHY_INTERFACE_MODE_USXGMII:
+			return mtk_usxgmii_select_pcs(mac->usxgmii_pcs_of_node, interface);

From what I can see, neither of these two "select_pcs" methods that you're calling makes any use of the "interface" you pass to them. I'm not sure what they _could_ do with it either, given that what you're effectively doing here is getting the phylink_pcs structure from the driver, and each one only has a single phylink_pcs.

Quoted 6 lines

Yes, you are right, the interface parameter isn't used, I will drop it from both mtk_*_select_pcs() prototypes.

In the long run we may want something like
struct phylink_pcs *of_pcs_get(struct device_node *np, phy_interface_t interface) provided by a to-be-built drivers/net/pcs/core.c...

Again... it's not as simple as that. As soon as we get into the situation that some _other_ driver becomes responsible for providing the struct phylink_pcs pointer, we _then_ need to have some way of dealing with that device going away.

By that I mean that the memory pointed to returned from such a function that you are proposing above could be freed - or worse could be unmapped from the kernel address space, and the same goes for the operations structure as well - even more so if the "ops" are part of module data and the module is unloaded.

As I know how these discussions go (it's not my first time bringing up these kinds of multi-driver interations), no, locking the module into memory doesn't work, and shows a lack of a full understanding of the problem.

We need to have a way that when a PCS device is removed, that is propagated up the management levels and causes the PCS to be gracefully removed from the network driver (in other words, from phylink).

I won't accept a hack that sticky-plasters around the problem - not for code that I am involved in actively maintaining - and sticky-plastering around this class of problem seems to happen all too often in the kernel.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
author maintainer
#29 lore raw

Re: [RFC PATCH v2 8/8] net: ethernet: mtk_eth_soc: add paths and SerDes modes for MT7988

Hi Russell,

thank for going into my patch in depth and taking your time for an elaborate and constructive answer.

Quoted 27 lines · Russell King (Oracle)
Quoted 26 lines · Daniel Golle
Quoted 5 lines · Russell King (Oracle)
Quoted 4 lines · Daniel Golle
drivers/net/ethernet/mediatek/mtk_eth_soc.c +0 -0
@@ -516,6 +538,21 @@ static struct phylink_pcs *mtk_mac_select_pcs(struct phylink_config *config,
 	struct mtk_eth *eth = mac->hw;
 	unsigned int sid;
 
Quoted 16 lines

drivers/net/ethernet/mediatek/mtk_eth_soc.c static struct phylink_pcs *mtk_mac_select_pcs(struct phylink_config *config, · line 550

Quoted 10 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +10 -0
+	if (mtk_is_netsys_v3_or_greater(eth)) {
+		switch (interface) {
+		case PHY_INTERFACE_MODE_1000BASEX:
+		case PHY_INTERFACE_MODE_2500BASEX:
+		case PHY_INTERFACE_MODE_SGMII:
+			return mtk_pcs_lynxi_select_pcs(mac->sgmii_pcs_of_node, interface);
+		case PHY_INTERFACE_MODE_5GBASER:
+		case PHY_INTERFACE_MODE_10GBASER:
+		case PHY_INTERFACE_MODE_USXGMII:
+			return mtk_usxgmii_select_pcs(mac->usxgmii_pcs_of_node, interface);

From what I can see, neither of these two "select_pcs" methods that you're calling makes any use of the "interface" you pass to them. I'm not sure what they _could_ do with it either, given that what you're effectively doing here is getting the phylink_pcs structure from the driver, and each one only has a single phylink_pcs.

Yes, you are right, the interface parameter isn't used, I will drop it from both mtk_*_select_pcs() prototypes.

Quoted 8 lines
Quoted 3 lines

In the long run we may want something like
struct phylink_pcs *of_pcs_get(struct device_node *np, phy_interface_t interface) provided by a to-be-built drivers/net/pcs/core.c...

Again... it's not as simple as that. As soon as we get into the situation that some _other_ driver becomes responsible for providing the struct phylink_pcs pointer, we _then_ need to have some way of dealing with that device going away.

I assume you are referring to struct phylink_pcs *of_pcs_get(...), right? And true, it'd be quite a complex piece of infrastructure.

Quoted 7 lines

By that I mean that the memory pointed to returned from such a function that you are proposing above could be freed - or worse could be unmapped from the kernel address space, and the same goes for the operations structure as well - even more so if the "ops" are part of module data and the module is unloaded.

Quoted 8 lines

As I know how these discussions go (it's not my first time bringing up these kinds of multi-driver interations), no, locking the module into memory doesn't work, and shows a lack of a full understanding of the problem.

We need to have a way that when a PCS device is removed, that is propagated up the management levels and causes the PCS to be gracefully removed from the network driver (in other words, from phylink).

I see -- spontanous removal of the PCS may not be a practical problem on that very hardware, but from the driver model point of view, it is.

Should the callback for removal be implemented as part of the network driver or are you suggesting to add such infrastructure to phylink?

Quoted 5 lines

I won't accept a hack that sticky-plasters around the problem - not for code that I am involved in actively maintaining - and sticky-plastering around this class of problem seems to happen all too often in the kernel.

Oh yes, I can see that every day when testing various SFP modules with exposed PHYs -- and often get stackdumps thrown at me when I remove them...