Add support for 10G Ethernet SerDes on MT7988

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)

Signed-off-by Daniel Golle <[email protected]>

---
 .../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 line 45

Quoted 6 lines · Daniel Golle

Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml new file +6 -0

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

Why do you need a property for this if you know that it is present on the MT7988?

Quoted 11 lines · Conor Dooley

Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml line 45

Quoted 6 lines · Daniel Golle

Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml new file +6 -0

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

Why do you need a property for this if you know that it is present on the MT7988?

Because it is only present in one of the two SerDes channels. Channel 0 needs the work-around, Channel 1 doesn't.

See also this commit in the vendor driver for reference[1].

We previously discussed that[2] and it was decided that a property would be the prefered way to represent this as there aren't any other per-instance differences which would justify another compatible.

Because it is only present in one of the two SerDes channels. Channel 0 needs the work-around, Channel 1 doesn't.

Quoted 2 lines

Because it is only present in one of the two SerDes channels. Channel 0 needs the work-around, Channel 1 doesn't.

Does the channel know its own number?

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.

Signed-off-by Daniel Golle <[email protected]>

---

drivers/phy/mediatek/phy-mtk-pextp.c new file +310 -0

+
+	/* Setup operation mode */
+	if (is_10g)
+		iowrite32(0x00c9071c, pextp->base + 0x9024);
+	else
+		iowrite32(0x00d9071c, pextp->base + 0x9024);
+
+	if (is_5g)
+		iowrite32(0xaaa5a5aa, pextp->base + 0x2020);
+	else
+		iowrite32(0xaa8585aa, pextp->base + 0x2020);
+
+	if (is_2p5g || is_5g || is_10g) {
+		iowrite32(0x0c020707, pextp->base + 0x2030);
+		iowrite32(0x0e050f0f, pextp->base + 0x2034);
+		iowrite32(0x00140032, pextp->base + 0x2040);
+	} else {
+		iowrite32(0x0c020207, pextp->base + 0x2030);
+		iowrite32(0x0e05050f, pextp->base + 0x2034);
+		iowrite32(0x00200032, pextp->base + 0x2040);
+	}
+
+	if (is_2p5g || is_10g)
+		iowrite32(0x00c014aa, pextp->base + 0x50f0);
+	else if (is_5g)
+		iowrite32(0x00c018aa, pextp->base + 0x50f0);
+	else
+		iowrite32(0x00c014ba, pextp->base + 0x50f0);
+
+	if (is_5g) {
+		iowrite32(0x3777812b, pextp->base + 0x50e0);
+		iowrite32(0x005c9cff, pextp->base + 0x506c);
+		iowrite32(0x9dfafafa, pextp->base + 0x5070);
+		iowrite32(0x273f3f3f, pextp->base + 0x5074);
+		iowrite32(0xa8883868, pextp->base + 0x5078);
+		iowrite32(0x14661466, pextp->base + 0x507c);
+	} else {
+		iowrite32(0x3777c12b, pextp->base + 0x50e0);
+		iowrite32(0x005f9cff, pextp->base + 0x506c);
+		iowrite32(0x9d9dfafa, pextp->base + 0x5070);
+		iowrite32(0x27273f3f, pextp->base + 0x5074);
+		iowrite32(0xa7883c68, pextp->base + 0x5078);
+		iowrite32(0x11661166, pextp->base + 0x507c);
+	}
+
+	if (is_2p5g || is_10g) {
+		iowrite32(0x0e000aaf, pextp->base + 0x5080);
+		iowrite32(0x08080d0d, pextp->base + 0x5084);
+		iowrite32(0x02030909, pextp->base + 0x5088);
+	} else if (is_5g) {
+		iowrite32(0x0e001abf, pextp->base + 0x5080);
+		iowrite32(0x080b0d0d, pextp->base + 0x5084);
+		iowrite32(0x02050909, pextp->base + 0x5088);
+	} else {
+		iowrite32(0x0e000eaf, pextp->base + 0x5080);
+		iowrite32(0x08080e0d, pextp->base + 0x5084);
+		iowrite32(0x02030b09, pextp->base + 0x5088);
+	}
+
+	if (is_5g) {
+		iowrite32(0x0c000000, pextp->base + 0x50e4);
+		iowrite32(0x04000000, pextp->base + 0x50e8);
+	} else {
+		iowrite32(0x0c0c0000, pextp->base + 0x50e4);
+		iowrite32(0x04040000, pextp->base + 0x50e8);
+	}
+
+	if (is_2p5g || mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x0f0f0c06, pextp->base + 0x50eC);
+	else
+		iowrite32(0x0f0f0606, pextp->base + 0x50eC);
+
+	if (is_5g) {
+		iowrite32(0x50808c8c, pextp->base + 0x50a8);
+		iowrite32(0x18000000, pextp->base + 0x6004);
+	} else {
+		iowrite32(0x506e8c8c, pextp->base + 0x50a8);
+		iowrite32(0x18190000, pextp->base + 0x6004);
+	}
+
+	if (is_10g)
+		iowrite32(0x01423342, pextp->base + 0x00f8);
+	else if (is_5g)
+		iowrite32(0x00a132a1, pextp->base + 0x00f8);
+	else if (is_2p5g)
+		iowrite32(0x009c329c, pextp->base + 0x00f8);
+	else
+		iowrite32(0x00fa32fa, pextp->base + 0x00f8);
+
+	/* Force SGDT_OUT off and select PCS */
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x80201f20, pextp->base + 0x00f4);
+	else
+		iowrite32(0x80201f21, pextp->base + 0x00f4);
+
+	/* Force GLB_CKDET_OUT */
+	iowrite32(0x00050c00, pextp->base + 0x0030);
+
+	/* Force AEQ on */
+	iowrite32(0x02002800, pextp->base + 0x0070);
+	ndelay(1020);
+
+	/* Setup DA default value */
+	iowrite32(0x00000020, pextp->base + 0x30b0);
+	iowrite32(0x00008a01, pextp->base + 0x3028);
+	iowrite32(0x0000a884, pextp->base + 0x302c);
+	iowrite32(0x00083002, pextp->base + 0x3024);
+	if (mtk_interface_mode_is_xgmii(interface)) {
+		iowrite32(0x00022220, pextp->base + 0x3010);
+		iowrite32(0x0f020a01, pextp->base + 0x5064);
+		iowrite32(0x06100600, pextp->base + 0x50b4);
+		if (interface == PHY_INTERFACE_MODE_USXGMII)
+			iowrite32(0x40704000, pextp->base + 0x3048);
+		else
+			iowrite32(0x47684100, pextp->base + 0x3048);
+	} else {
+		iowrite32(0x00011110, pextp->base + 0x3010);
+		iowrite32(0x40704000, pextp->base + 0x3048);
+	}
+
+	if (!mtk_interface_mode_is_xgmii(interface) && !is_2p5g)
+		iowrite32(0x0000c000, pextp->base + 0x3064);
+
+	if (interface != PHY_INTERFACE_MODE_10GBASER) {
+		iowrite32(0xa8000000, pextp->base + 0x3050);
+		iowrite32(0x000000aa, pextp->base + 0x3054);
+	} else {
+		iowrite32(0x00000000, pextp->base + 0x3050);
+		iowrite32(0x00000000, pextp->base + 0x3054);
+	}
+
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x00000f00, pextp->base + 0x306c);
+	else if (is_2p5g)
+		iowrite32(0x22000f00, pextp->base + 0x306c);
+	else
+		iowrite32(0x20200f00, pextp->base + 0x306c);
+
+	if (interface == PHY_INTERFACE_MODE_10GBASER && pextp->da_war)
+		iowrite32(0x0007b400, pextp->base + 0xa008);
+
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x00040000, pextp->base + 0xa060);
+	else
+		iowrite32(0x00050000, pextp->base + 0xa060);
+
+	if (is_10g)
+		iowrite32(0x00000001, pextp->base + 0x90d0);
+	else if (is_5g)
+		iowrite32(0x00000003, pextp->base + 0x90d0);
+	else if (is_2p5g)
+		iowrite32(0x00000005, pextp->base + 0x90d0);
+	else
+		iowrite32(0x00000007, pextp->base + 0x90d0);
+
+	/* Release reset */
+	iowrite32(0x0200e800, pextp->base + 0x0070);
+	usleep_range(150, 500);
+
+	/* Switch to P0 */
+	iowrite32(0x0200c111, pextp->base + 0x0070);
+	ndelay(1020);
+	iowrite32(0x0200c101, pextp->base + 0x0070);
+	usleep_range(15, 50);
+
+	if (mtk_interface_mode_is_xgmii(interface)) {
+		/* Switch to Gen3 */
+		iowrite32(0x0202c111, pextp->base + 0x0070);
+	} else {
+		/* Switch to Gen2 */
+		iowrite32(0x0201c111, pextp->base + 0x0070);
+	}
+	ndelay(1020);
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x0202c101, pextp->base + 0x0070);
+	else
+		iowrite32(0x0201c101, pextp->base + 0x0070);
+	usleep_range(100, 500);
+	iowrite32(0x00000030, pextp->base + 0x30b0);
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x80201f00, pextp->base + 0x00f4);
+	else
+		iowrite32(0x80201f01, pextp->base + 0x00f4);
+
+	iowrite32(0x30000000, pextp->base + 0x3040);
+	usleep_range(400, 1000);
+}
+
+static int mtk_pextp_set_mode(struct phy *phy, enum phy_mode mode, int submode)
+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+
+	if (mode != PHY_MODE_ETHERNET)
+		return -EINVAL;
+
+	switch (submode) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_2500BASEX:
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_5GBASER:
+	case PHY_INTERFACE_MODE_10GBASER:
+	case PHY_INTERFACE_MODE_USXGMII:
+		mtk_pextp_setup(pextp, submode);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mtk_pextp_reset(struct phy *phy)
+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+
+	reset_control_assert(pextp->reset);
+	usleep_range(100, 500);
+	reset_control_deassert(pextp->reset);
+	usleep_range(1, 10);
+
+	return 0;
+}
+
+static int mtk_pextp_power_on(struct phy *phy)
+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+
+	return clk_bulk_prepare_enable(MTK_PEXTP_NUM_CLOCKS, pextp->clocks);
+}
+
+static int mtk_pextp_power_off(struct phy *phy)
+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+
+	clk_bulk_disable_unprepare(MTK_PEXTP_NUM_CLOCKS, pextp->clocks);
+
+	return 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)
+		return -EIO;
+
+	pextp->dev = &pdev->dev;
+
+	pextp->clocks[0].id = "topxtal";
+	pextp->clocks[0].clk = devm_clk_get(&pdev->dev, pextp->clocks[0].id);
+	if (IS_ERR(pextp->clocks[0].clk))
+		return PTR_ERR(pextp->clocks[0].clk);
+
+	pextp->clocks[1].id = "xfipll";
+	pextp->clocks[1].clk = devm_clk_get(&pdev->dev, pextp->clocks[1].id);
+	if (IS_ERR(pextp->clocks[1].clk))
+		return PTR_ERR(pextp->clocks[1].clk);
+
+	pextp->reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(pextp->reset))
+		return PTR_ERR(pextp->reset);
+
+	pextp->da_war = of_property_read_bool(np, "mediatek,usxgmii-performance-errata");
+
+	phy = devm_phy_create(&pdev->dev, NULL, &mtk_pextp_ops);
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
+
+	phy_set_drvdata(phy, pextp);
+
+	phy_provider = devm_of_phy_provider_register(&pdev->dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id mtk_pextp_match[] = {
+	{ .compatible = "mediatek,mt7988-xfi-pextp", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mtk_pextp_match);
+
+static struct platform_driver mtk_pextp_driver = {
+	.probe = mtk_pextp_probe,
+	.driver = {
+		.name = "mtk-pextp",
+		.of_match_table = mtk_pextp_match,
+	},
+};
+module_platform_driver(mtk_pextp_driver);
+
+MODULE_DESCRIPTION("MediaTek pextp SerDes PHY driver");
+MODULE_AUTHOR("Daniel Golle <[email protected]>");
+MODULE_LICENSE("GPL");

Il 12/12/23 04:46, Daniel Golle ha scritto:

Quoted 25 lines

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.

Quoted 2 lines

Signed-off-by Daniel Golle <[email protected]>

---

..snip..

Can anyone from MediaTek **please** define those registers and fields?

In this form, this driver is pretty obscure and nobody knows what it's doing;
please remember that, by actually providing a definition for those registers and
fields, the operation (reliability) of this PHY may be improved and this driver
will be actually maintainable (and possibly support more than one variation of
this PHY in the future with less efforts).

MediaTek?

Regards,
Angelo

Quoted 318 lines

drivers/phy/mediatek/phy-mtk-pextp.c new file +189 -0

+
+	/* Setup operation mode */
+	if (is_10g)
+		iowrite32(0x00c9071c, pextp->base + 0x9024);
+	else
+		iowrite32(0x00d9071c, pextp->base + 0x9024);
+
+	if (is_5g)
+		iowrite32(0xaaa5a5aa, pextp->base + 0x2020);
+	else
+		iowrite32(0xaa8585aa, pextp->base + 0x2020);
+
+	if (is_2p5g || is_5g || is_10g) {
+		iowrite32(0x0c020707, pextp->base + 0x2030);
+		iowrite32(0x0e050f0f, pextp->base + 0x2034);
+		iowrite32(0x00140032, pextp->base + 0x2040);
+	} else {
+		iowrite32(0x0c020207, pextp->base + 0x2030);
+		iowrite32(0x0e05050f, pextp->base + 0x2034);
+		iowrite32(0x00200032, pextp->base + 0x2040);
+	}
+
+	if (is_2p5g || is_10g)
+		iowrite32(0x00c014aa, pextp->base + 0x50f0);
+	else if (is_5g)
+		iowrite32(0x00c018aa, pextp->base + 0x50f0);
+	else
+		iowrite32(0x00c014ba, pextp->base + 0x50f0);
+
+	if (is_5g) {
+		iowrite32(0x3777812b, pextp->base + 0x50e0);
+		iowrite32(0x005c9cff, pextp->base + 0x506c);
+		iowrite32(0x9dfafafa, pextp->base + 0x5070);
+		iowrite32(0x273f3f3f, pextp->base + 0x5074);
+		iowrite32(0xa8883868, pextp->base + 0x5078);
+		iowrite32(0x14661466, pextp->base + 0x507c);
+	} else {
+		iowrite32(0x3777c12b, pextp->base + 0x50e0);
+		iowrite32(0x005f9cff, pextp->base + 0x506c);
+		iowrite32(0x9d9dfafa, pextp->base + 0x5070);
+		iowrite32(0x27273f3f, pextp->base + 0x5074);
+		iowrite32(0xa7883c68, pextp->base + 0x5078);
+		iowrite32(0x11661166, pextp->base + 0x507c);
+	}
+
+	if (is_2p5g || is_10g) {
+		iowrite32(0x0e000aaf, pextp->base + 0x5080);
+		iowrite32(0x08080d0d, pextp->base + 0x5084);
+		iowrite32(0x02030909, pextp->base + 0x5088);
+	} else if (is_5g) {
+		iowrite32(0x0e001abf, pextp->base + 0x5080);
+		iowrite32(0x080b0d0d, pextp->base + 0x5084);
+		iowrite32(0x02050909, pextp->base + 0x5088);
+	} else {
+		iowrite32(0x0e000eaf, pextp->base + 0x5080);
+		iowrite32(0x08080e0d, pextp->base + 0x5084);
+		iowrite32(0x02030b09, pextp->base + 0x5088);
+	}
+
+	if (is_5g) {
+		iowrite32(0x0c000000, pextp->base + 0x50e4);
+		iowrite32(0x04000000, pextp->base + 0x50e8);
+	} else {
+		iowrite32(0x0c0c0000, pextp->base + 0x50e4);
+		iowrite32(0x04040000, pextp->base + 0x50e8);
+	}
+
+	if (is_2p5g || mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x0f0f0c06, pextp->base + 0x50eC);
+	else
+		iowrite32(0x0f0f0606, pextp->base + 0x50eC);
+
+	if (is_5g) {
+		iowrite32(0x50808c8c, pextp->base + 0x50a8);
+		iowrite32(0x18000000, pextp->base + 0x6004);
+	} else {
+		iowrite32(0x506e8c8c, pextp->base + 0x50a8);
+		iowrite32(0x18190000, pextp->base + 0x6004);
+	}
+
+	if (is_10g)
+		iowrite32(0x01423342, pextp->base + 0x00f8);
+	else if (is_5g)
+		iowrite32(0x00a132a1, pextp->base + 0x00f8);
+	else if (is_2p5g)
+		iowrite32(0x009c329c, pextp->base + 0x00f8);
+	else
+		iowrite32(0x00fa32fa, pextp->base + 0x00f8);
+
+	/* Force SGDT_OUT off and select PCS */
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x80201f20, pextp->base + 0x00f4);
+	else
+		iowrite32(0x80201f21, pextp->base + 0x00f4);
+
+	/* Force GLB_CKDET_OUT */
+	iowrite32(0x00050c00, pextp->base + 0x0030);
+
+	/* Force AEQ on */
+	iowrite32(0x02002800, pextp->base + 0x0070);
+	ndelay(1020);
+
+	/* Setup DA default value */
+	iowrite32(0x00000020, pextp->base + 0x30b0);
+	iowrite32(0x00008a01, pextp->base + 0x3028);
+	iowrite32(0x0000a884, pextp->base + 0x302c);
+	iowrite32(0x00083002, pextp->base + 0x3024);
+	if (mtk_interface_mode_is_xgmii(interface)) {
+		iowrite32(0x00022220, pextp->base + 0x3010);
+		iowrite32(0x0f020a01, pextp->base + 0x5064);
+		iowrite32(0x06100600, pextp->base + 0x50b4);
+		if (interface == PHY_INTERFACE_MODE_USXGMII)
+			iowrite32(0x40704000, pextp->base + 0x3048);
+		else
+			iowrite32(0x47684100, pextp->base + 0x3048);
+	} else {
+		iowrite32(0x00011110, pextp->base + 0x3010);
+		iowrite32(0x40704000, pextp->base + 0x3048);
+	}
+
+	if (!mtk_interface_mode_is_xgmii(interface) && !is_2p5g)
+		iowrite32(0x0000c000, pextp->base + 0x3064);
+
+	if (interface != PHY_INTERFACE_MODE_10GBASER) {
+		iowrite32(0xa8000000, pextp->base + 0x3050);
+		iowrite32(0x000000aa, pextp->base + 0x3054);
+	} else {
+		iowrite32(0x00000000, pextp->base + 0x3050);
+		iowrite32(0x00000000, pextp->base + 0x3054);
+	}
+
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x00000f00, pextp->base + 0x306c);
+	else if (is_2p5g)
+		iowrite32(0x22000f00, pextp->base + 0x306c);
+	else
+		iowrite32(0x20200f00, pextp->base + 0x306c);
+
+	if (interface == PHY_INTERFACE_MODE_10GBASER && pextp->da_war)
+		iowrite32(0x0007b400, pextp->base + 0xa008);
+
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x00040000, pextp->base + 0xa060);
+	else
+		iowrite32(0x00050000, pextp->base + 0xa060);
+
+	if (is_10g)
+		iowrite32(0x00000001, pextp->base + 0x90d0);
+	else if (is_5g)
+		iowrite32(0x00000003, pextp->base + 0x90d0);
+	else if (is_2p5g)
+		iowrite32(0x00000005, pextp->base + 0x90d0);
+	else
+		iowrite32(0x00000007, pextp->base + 0x90d0);
+
+	/* Release reset */
+	iowrite32(0x0200e800, pextp->base + 0x0070);
+	usleep_range(150, 500);
+
+	/* Switch to P0 */
+	iowrite32(0x0200c111, pextp->base + 0x0070);
+	ndelay(1020);
+	iowrite32(0x0200c101, pextp->base + 0x0070);
+	usleep_range(15, 50);
+
+	if (mtk_interface_mode_is_xgmii(interface)) {
+		/* Switch to Gen3 */
+		iowrite32(0x0202c111, pextp->base + 0x0070);
+	} else {
+		/* Switch to Gen2 */
+		iowrite32(0x0201c111, pextp->base + 0x0070);
+	}
+	ndelay(1020);
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x0202c101, pextp->base + 0x0070);
+	else
+		iowrite32(0x0201c101, pextp->base + 0x0070);
+	usleep_range(100, 500);
+	iowrite32(0x00000030, pextp->base + 0x30b0);
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x80201f00, pextp->base + 0x00f4);
+	else
+		iowrite32(0x80201f01, pextp->base + 0x00f4);
+
+	iowrite32(0x30000000, pextp->base + 0x3040);
+	usleep_range(400, 1000);
+}
+
+static int mtk_pextp_set_mode(struct phy *phy, enum phy_mode mode,

int submode)

drivers/phy/mediatek/phy-mtk-pextp.c new file +37 -0

+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+
+	if (mode != PHY_MODE_ETHERNET)
+		return -EINVAL;
+
+	switch (submode) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_2500BASEX:
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_5GBASER:
+	case PHY_INTERFACE_MODE_10GBASER:
+	case PHY_INTERFACE_MODE_USXGMII:
+		mtk_pextp_setup(pextp, submode);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mtk_pextp_reset(struct phy *phy)
+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+
+	reset_control_assert(pextp->reset);
+	usleep_range(100, 500);
+	reset_control_deassert(pextp->reset);
+	usleep_range(1, 10);
+
+	return 0;
+}
+
+static int mtk_pextp_power_on(struct phy *phy)
+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+
+	return clk_bulk_prepare_enable(MTK_PEXTP_NUM_CLOCKS, pextp-

Quoted 1 line

clocks);

drivers/phy/mediatek/phy-mtk-pextp.c new file +7 -0

+}
+
+static int mtk_pextp_power_off(struct phy *phy)
+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+
+	clk_bulk_disable_unprepare(MTK_PEXTP_NUM_CLOCKS, pextp-

Quoted 1 line

clocks);

drivers/phy/mediatek/phy-mtk-pextp.c new file +34 -0

+
+	return 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)
+		return -EIO;
+
+	pextp->dev = &pdev->dev;
+
+	pextp->clocks[0].id = "topxtal";
+	pextp->clocks[0].clk = devm_clk_get(&pdev->dev, pextp-

Quoted 1 line

clocks[0].id);

drivers/phy/mediatek/phy-mtk-pextp.c new file +5 -0

+	if (IS_ERR(pextp->clocks[0].clk))
+		return PTR_ERR(pextp->clocks[0].clk);
+
+	pextp->clocks[1].id = "xfipll";
+	pextp->clocks[1].clk = devm_clk_get(&pdev->dev, pextp-

Quoted 1 line

clocks[1].id);

drivers/phy/mediatek/phy-mtk-pextp.c new file +4 -0

+	if (IS_ERR(pextp->clocks[1].clk))
+		return PTR_ERR(pextp->clocks[1].clk);
+
+	pextp->reset = devm_reset_control_get_exclusive(&pdev->dev,

NULL);

drivers/phy/mediatek/phy-mtk-pextp.c new file +4 -0

+	if (IS_ERR(pextp->reset))
+		return PTR_ERR(pextp->reset);
+
+	pextp->da_war = of_property_read_bool(np, "mediatek,usxgmii-

performance-errata");

drivers/phy/mediatek/phy-mtk-pextp.c new file +8 -0

+
+	phy = devm_phy_create(&pdev->dev, NULL, &mtk_pextp_ops);
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
+
+	phy_set_drvdata(phy, pextp);
+
+	phy_provider = devm_of_phy_provider_register(&pdev->dev,

of_phy_simple_xlate);

drivers/phy/mediatek/phy-mtk-pextp.c new file +22 -0

+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id mtk_pextp_match[] = {
+	{ .compatible = "mediatek,mt7988-xfi-pextp", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mtk_pextp_match);
+
+static struct platform_driver mtk_pextp_driver = {
+	.probe = mtk_pextp_probe,
+	.driver = {
+		.name = "mtk-pextp",
+		.of_match_table = mtk_pextp_match,
+	},
+};
+module_platform_driver(mtk_pextp_driver);
+
+MODULE_DESCRIPTION("MediaTek pextp SerDes PHY driver");
+MODULE_AUTHOR("Daniel Golle <[email protected]>");
+MODULE_LICENSE("GPL");

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.

Signed-off-by Daniel Golle <[email protected]>

---
 MAINTAINERS                          |   1 +
 drivers/phy/mediatek/Kconfig         |  11 +
 drivers/phy/mediatek/Makefile        |   1 +
 drivers/phy/mediatek/phy-mtk-pextp.c | 361 +++++++++++++++++++++++++++
 4 files changed, 374 insertions(+)
 create mode 100644 drivers/phy/mediatek/phy-mtk-pextp.c

MAINTAINERS +1 -0

index 98f7dd0499f17..b2772cfe2a704 100644
@@ -13510,6 +13510,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 +0 -0

index c9a50395533eb..ca60c7b9b02ac 100644

drivers/phy/mediatek/Makefile +0 -0

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

new file mode 100644
index 0000000000000..3ec48cf129105
@@ -0,0 +1,361 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* MediaTek 10GE SerDes PHY driver

drivers/phy/mediatek/phy-mtk-pextp.c new file +98 -0

+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/reset.h>
+#include <linux/phy.h>
+#include <linux/phy/phy.h>
+
+#define MTK_PEXTP_NUM_CLOCKS	2
+
+struct mtk_pextp_phy {
+	void __iomem		*base;
+	struct device		*dev;
+	struct reset_control	*reset;
+	struct clk_bulk_data	clocks[MTK_PEXTP_NUM_CLOCKS];
+	bool			da_war;
+};
+
+static bool mtk_interface_mode_is_xgmii(phy_interface_t interface)
+{
+	switch (interface) {
+	case PHY_INTERFACE_MODE_INTERNAL:
+	case PHY_INTERFACE_MODE_USXGMII:
+	case PHY_INTERFACE_MODE_10GBASER:
+	case PHY_INTERFACE_MODE_5GBASER:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static void mtk_pextp_setup(struct mtk_pextp_phy *pextp, phy_interface_t interface)
+{
+	bool is_10g = (interface == PHY_INTERFACE_MODE_10GBASER ||
+		       interface == PHY_INTERFACE_MODE_USXGMII);
+	bool is_2p5g = (interface == PHY_INTERFACE_MODE_2500BASEX);
+	bool is_5g = (interface == PHY_INTERFACE_MODE_5GBASER);
+
+	dev_dbg(pextp->dev, "setting up for mode %s\n", phy_modes(interface));
+
+	/* Setup operation mode */
+	if (is_10g)
+		iowrite32(0x00c9071c, pextp->base + 0x9024);
+	else
+		iowrite32(0x00d9071c, pextp->base + 0x9024);
+
+	if (is_5g)
+		iowrite32(0xaaa5a5aa, pextp->base + 0x2020);
+	else
+		iowrite32(0xaa8585aa, pextp->base + 0x2020);
+
+	if (is_2p5g || is_5g || is_10g) {
+		iowrite32(0x0c020707, pextp->base + 0x2030);
+		iowrite32(0x0e050f0f, pextp->base + 0x2034);
+		iowrite32(0x00140032, pextp->base + 0x2040);
+	} else {
+		iowrite32(0x0c020207, pextp->base + 0x2030);
+		iowrite32(0x0e05050f, pextp->base + 0x2034);
+		iowrite32(0x00200032, pextp->base + 0x2040);
+	}
+
+	if (is_2p5g || is_10g)
+		iowrite32(0x00c014aa, pextp->base + 0x50f0);
+	else if (is_5g)
+		iowrite32(0x00c018aa, pextp->base + 0x50f0);
+	else
+		iowrite32(0x00c014ba, pextp->base + 0x50f0);
+
+	if (is_5g) {
+		iowrite32(0x3777812b, pextp->base + 0x50e0);
+		iowrite32(0x005c9cff, pextp->base + 0x506c);
+		iowrite32(0x9dfafafa, pextp->base + 0x5070);
+		iowrite32(0x273f3f3f, pextp->base + 0x5074);
+		iowrite32(0xa8883868, pextp->base + 0x5078);
+		iowrite32(0x14661466, pextp->base + 0x507c);
+	} else {
+		iowrite32(0x3777c12b, pextp->base + 0x50e0);
+		iowrite32(0x005f9cff, pextp->base + 0x506c);
+		iowrite32(0x9d9dfafa, pextp->base + 0x5070);
+		iowrite32(0x27273f3f, pextp->base + 0x5074);
+		iowrite32(0xa7883c68, pextp->base + 0x5078);
+		iowrite32(0x11661166, pextp->base + 0x507c);
+	}
+
+	if (is_2p5g || is_10g) {
+		iowrite32(0x0e000aaf, pextp->base + 0x5080);
+		iowrite32(0x08080d0d, pextp->base + 0x5084);
+		iowrite32(0x02030909, pextp->base + 0x5088);
+	} else if (is_5g) {
+		iowrite32(0x0e001abf, pextp->base + 0x5080);
+		iowrite32(0x080b0d0d, pextp->base + 0x5084);
+		iowrite32(0x02050909, pextp->base + 0x5088);
+	} else {
+		iowrite32(0x0e000eaf, pextp->base + 0x5080);
+		iowrite32(0x08080e0d, pextp->base + 0x5084);
+		iowrite32(0x02030b09, pextp->base + 0x5088);
+	}
+

drivers/phy/mediatek/phy-mtk-pextp.c new file +227 -0

+	else
+		iowrite32(0x0f0f0606, pextp->base + 0x50eC);
+
+	if (is_5g) {
+		iowrite32(0x50808c8c, pextp->base + 0x50a8);
+		iowrite32(0x18000000, pextp->base + 0x6004);
+	} else {
+		iowrite32(0x506e8c8c, pextp->base + 0x50a8);
+		iowrite32(0x18190000, pextp->base + 0x6004);
+	}
+
+	if (is_10g)
+		iowrite32(0x01423342, pextp->base + 0x00f8);
+	else if (is_5g)
+		iowrite32(0x00a132a1, pextp->base + 0x00f8);
+	else if (is_2p5g)
+		iowrite32(0x009c329c, pextp->base + 0x00f8);
+	else
+		iowrite32(0x00fa32fa, pextp->base + 0x00f8);
+
+	/* Force SGDT_OUT off and select PCS */
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x80201f20, pextp->base + 0x00f4);
+	else
+		iowrite32(0x80201f21, pextp->base + 0x00f4);
+
+	/* Force GLB_CKDET_OUT */
+	iowrite32(0x00050c00, pextp->base + 0x0030);
+
+	/* Force AEQ on */
+	iowrite32(0x02002800, pextp->base + 0x0070);
+	ndelay(1020);
+
+	/* Setup DA default value */
+	iowrite32(0x00000020, pextp->base + 0x30b0);
+	iowrite32(0x00008a01, pextp->base + 0x3028);
+	iowrite32(0x0000a884, pextp->base + 0x302c);
+	iowrite32(0x00083002, pextp->base + 0x3024);
+	if (mtk_interface_mode_is_xgmii(interface)) {
+		iowrite32(0x00022220, pextp->base + 0x3010);
+		iowrite32(0x0f020a01, pextp->base + 0x5064);
+		iowrite32(0x06100600, pextp->base + 0x50b4);
+		if (interface == PHY_INTERFACE_MODE_USXGMII)
+			iowrite32(0x40704000, pextp->base + 0x3048);
+		else
+			iowrite32(0x47684100, pextp->base + 0x3048);
+	} else {
+		iowrite32(0x00011110, pextp->base + 0x3010);
+		iowrite32(0x40704000, pextp->base + 0x3048);
+	}
+
+	if (!mtk_interface_mode_is_xgmii(interface) && !is_2p5g)
+		iowrite32(0x0000c000, pextp->base + 0x3064);
+
+	if (interface != PHY_INTERFACE_MODE_10GBASER) {
+		iowrite32(0xa8000000, pextp->base + 0x3050);
+		iowrite32(0x000000aa, pextp->base + 0x3054);
+	} else {
+		iowrite32(0x00000000, pextp->base + 0x3050);
+		iowrite32(0x00000000, pextp->base + 0x3054);
+	}
+
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x00000f00, pextp->base + 0x306c);
+	else if (is_2p5g)
+		iowrite32(0x22000f00, pextp->base + 0x306c);
+	else
+		iowrite32(0x20200f00, pextp->base + 0x306c);
+
+	if (interface == PHY_INTERFACE_MODE_10GBASER && pextp->da_war)
+		iowrite32(0x0007b400, pextp->base + 0xa008);
+
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x00040000, pextp->base + 0xa060);
+	else
+		iowrite32(0x00050000, pextp->base + 0xa060);
+
+	if (is_10g)
+		iowrite32(0x00000001, pextp->base + 0x90d0);
+	else if (is_5g)
+		iowrite32(0x00000003, pextp->base + 0x90d0);
+	else if (is_2p5g)
+		iowrite32(0x00000005, pextp->base + 0x90d0);
+	else
+		iowrite32(0x00000007, pextp->base + 0x90d0);
+
+	/* Release reset */
+	iowrite32(0x0200e800, pextp->base + 0x0070);
+	usleep_range(150, 500);
+
+	/* Switch to P0 */
+	iowrite32(0x0200c111, pextp->base + 0x0070);
+	ndelay(1020);
+	iowrite32(0x0200c101, pextp->base + 0x0070);
+	usleep_range(15, 50);
+
+	if (mtk_interface_mode_is_xgmii(interface)) {
+		/* Switch to Gen3 */
+		iowrite32(0x0202c111, pextp->base + 0x0070);
+	} else {
+		/* Switch to Gen2 */
+		iowrite32(0x0201c111, pextp->base + 0x0070);
+	}
+	ndelay(1020);
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x0202c101, pextp->base + 0x0070);
+	else
+		iowrite32(0x0201c101, pextp->base + 0x0070);
+	usleep_range(100, 500);
+	iowrite32(0x00000030, pextp->base + 0x30b0);
+	if (mtk_interface_mode_is_xgmii(interface))
+		iowrite32(0x80201f00, pextp->base + 0x00f4);
+	else
+		iowrite32(0x80201f01, pextp->base + 0x00f4);
+
+	iowrite32(0x30000000, pextp->base + 0x3040);
+	usleep_range(400, 1000);
+}
+
+static int mtk_pextp_set_mode(struct phy *phy, enum phy_mode mode, int submode)
+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+
+	if (mode != PHY_MODE_ETHERNET)
+		return -EINVAL;
+
+	switch (submode) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_2500BASEX:
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_5GBASER:
+	case PHY_INTERFACE_MODE_10GBASER:
+	case PHY_INTERFACE_MODE_USXGMII:
+		mtk_pextp_setup(pextp, submode);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mtk_pextp_reset(struct phy *phy)
+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+
+	reset_control_assert(pextp->reset);
+	usleep_range(100, 500);
+	reset_control_deassert(pextp->reset);
+	usleep_range(1, 10);
+
+	return 0;
+}
+
+static int mtk_pextp_power_on(struct phy *phy)
+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+
+	return clk_bulk_prepare_enable(MTK_PEXTP_NUM_CLOCKS, pextp->clocks);
+}
+
+static int mtk_pextp_power_off(struct phy *phy)
+{
+	struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
+
+	clk_bulk_disable_unprepare(MTK_PEXTP_NUM_CLOCKS, pextp->clocks);
+
+	return 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)
+		return -EIO;
+
+	pextp->dev = &pdev->dev;
+
+	pextp->clocks[0].id = "topxtal";
+	pextp->clocks[0].clk = devm_clk_get(&pdev->dev, pextp->clocks[0].id);
+	if (IS_ERR(pextp->clocks[0].clk))
+		return PTR_ERR(pextp->clocks[0].clk);
+
+	pextp->clocks[1].id = "xfipll";
+	pextp->clocks[1].clk = devm_clk_get(&pdev->dev, pextp->clocks[1].id);
+	if (IS_ERR(pextp->clocks[1].clk))
+		return PTR_ERR(pextp->clocks[1].clk);
+
+	pextp->reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(pextp->reset))
+		return PTR_ERR(pextp->reset);
+
+	pextp->da_war = of_property_read_bool(np, "mediatek,usxgmii-performance-errata");
+
+	phy = devm_phy_create(&pdev->dev, NULL, &mtk_pextp_ops);
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
+
+	phy_set_drvdata(phy, pextp);
+
+	phy_provider = devm_of_phy_provider_register(&pdev->dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id mtk_pextp_match[] = {
+	{ .compatible = "mediatek,mt7988-xfi-pextp", },
+	{ }
+};

drivers/phy/mediatek/phy-mtk-pextp.c new file +4 -0

+
+MODULE_DESCRIPTION("MediaTek pextp SerDes PHY driver");
+MODULE_AUTHOR("Daniel Golle <[email protected]>");
+MODULE_LICENSE("GPL");

Signature · 1 line

2.43.0

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

Signed-off-by Daniel Golle <[email protected]>

drivers/net/pcs/pcs-mtk-lynxi.c +6 -0

+struct phylink_pcs *mtk_pcs_lynxi_get(struct device *dev, struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct mtk_pcs_lynxi *mpcs;
+
+	if (!np)

drivers/net/pcs/pcs-mtk-lynxi.c +9 -0

+
+	return &mpcs->pcs;
+}
+EXPORT_SYMBOL(mtk_pcs_lynxi_get);
+
+void mtk_pcs_lynxi_put(struct phylink_pcs *pcs)
+{
+	struct mtk_pcs_lynxi *cur, *mpcs = NULL;
+

Quoted 4 lines · 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.

Signed-off-by Daniel Golle <[email protected]>

I made some specific suggestions about what I wanted to see for "getting" PCS in the previous review, and I'm disappointed that this patch set is still inventing its own solution.

Quoted 6 lines

(quoted diff) +6 -0

+struct phylink_pcs *mtk_pcs_lynxi_get(struct device *dev, struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct mtk_pcs_lynxi *mpcs;
+
+	if (!np)

Review

Quoted 10 lines

(quoted diff) +10 -0

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

This is racy - as I thought I described before, userspace can unbind the device in one thread, while another thread is calling this function. With just the right timing, this check succeeds, but...

Review

Quoted 6 lines

(quoted diff) +6 -0

+		if (pdev)
+			put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	mpcs = platform_get_drvdata(pdev);

mpcs ends up being read as NULL here. Even if you did manage to get a valid pointer, "mpcs" being devm-alloced could be freed from under you at this point...

Review

Quoted 1 line

(quoted diff) +1 -0

+	device_link_add(dev, mpcs->dev, DL_FLAG_AUTOREMOVE_CONSUMER);

resulting in this accessing memory which has been freed.

The solution would be either to suppress the bind/unbind attributes (provided the underlying struct device can't go away, which probably also means ensuring the same of the MDIO bus. Aternatively, adding a lock around the remove path and around the checking of platform_get_drvdata() down to adding the device link would probably solve it.

However, I come back to my general point - this kind of stuff is hairy. Do we want N different implementations of it in various drivers with subtle bugs, or do we want _one_ implemenatation.

If we go with the one implemenation approach, then we need to think about whether we should be using device links or not. The problem could be for network interfaces where one struct device is associated with multiple network interfaces. Using device links has the unfortunate side effect that if the PCS for one of those network interfaces is removed, _all_ network interfaces disappear.

My original suggestion was to hook into phylink to cause that to take the link down when an in-use PCS gets removed.

Quoted 10 lines

(quoted diff) +9 -0

+
+	return &mpcs->pcs;
+}
+EXPORT_SYMBOL(mtk_pcs_lynxi_get);
+
+void mtk_pcs_lynxi_put(struct phylink_pcs *pcs)
+{
+	struct mtk_pcs_lynxi *cur, *mpcs = NULL;
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (!pcs)
+		return;
+
+	mutex_lock(&instance_mutex);
+	list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
+		if (pcs == &cur->pcs) {
+			mpcs = cur;
+			break;
+		}
+	mutex_unlock(&instance_mutex);

I don't see what this loop gains us, other than checking that the "pcs" is still on the list and hasn't already been removed. If that is all that this is about, then I would suggest:

	bool found = false;

	if (!pcs)
		return;

	mpcs = pcs_to_mtk_pcs_lynxi(pcs);
	mutex_lock(&instance_mutex);
	list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
		if (cur == mpcs) {
			found = true;
			break;
		}
	mutex_unlock(&instance_mutex);

	if (WARN_ON(!found))
		return;

which makes it more obvious why this exists.

Hi Russell,

sorry for the extended time it took me to get back to this patch and the comments you made for it. Understanding the complete scope of the problem took a while (plus there were holidays and other fun things), and also brought up further questions which I hope you can help me find good answers for, see below:

Quoted 57 lines · Russell King (Oracle)

Quoted 4 lines · 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.

Signed-off-by Daniel Golle <[email protected]>

I made some specific suggestions about what I wanted to see for "getting" PCS in the previous review, and I'm disappointed that this patch set is still inventing its own solution.

Quoted 6 lines

(quoted diff) +6 -0

+struct phylink_pcs *mtk_pcs_lynxi_get(struct device *dev, struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct mtk_pcs_lynxi *mpcs;
+
+	if (!np)

Review

Quoted 10 lines

(quoted diff) +10 -0

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

This is racy - as I thought I described before, userspace can unbind the device in one thread, while another thread is calling this function. With just the right timing, this check succeeds, but...

Review

Quoted 6 lines

(quoted diff) +6 -0

+		if (pdev)
+			put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	mpcs = platform_get_drvdata(pdev);

mpcs ends up being read as NULL here. Even if you did manage to get a valid pointer, "mpcs" being devm-alloced could be freed from under you at this point...

Review

Quoted 1 line

(quoted diff) +1 -0

+	device_link_add(dev, mpcs->dev, DL_FLAG_AUTOREMOVE_CONSUMER);

resulting in this accessing memory which has been freed.

The solution would be either to suppress the bind/unbind attributes (provided the underlying struct device can't go away, which probably also means ensuring the same of the MDIO bus. Aternatively, adding a lock around the remove path and around the checking of platform_get_drvdata() down to adding the device link would probably solve it.

However, I come back to my general point - this kind of stuff is hairy. Do we want N different implementations of it in various drivers with subtle bugs, or do we want _one_ implemenatation.

Quoted 6 lines

If we go with the one implemenation approach, then we need to think about whether we should be using device links or not. The problem could be for network interfaces where one struct device is associated with multiple network interfaces. Using device links has the unfortunate side effect that if the PCS for one of those network interfaces is removed, _all_ network interfaces disappear.

I agree, esp. on this MT7988 removal of a PCS which may then not even be in use also impairs connectivity on the built-in gigE DSA switch. It would be nice to try to avoid this.

Quoted 3 lines

My original suggestion was to hook into phylink to cause that to take the link down when an in-use PCS gets removed.

I took a deep dive into how this could be done correctly and how similar things are done for other drivers. Apart from the PCS there often also is a muxing-PHY involved, eg. MediaTek's XFI T-PHY in this case (previously often called "pextp" for no apparent reason) or Marvell's comphy (mvebu-comphy).

So let's try something simple on an already well-supported platform, I thought and grabbed Marvell Armada CN9131-DB running vanilla Linux, and it didn't even take some something racy, but plain:

ip link set eth6 up
cd /sys/bus/platform/drivers/mvebu-comphy
echo f2120000.phy > unbind
echo f4120000.phy > unbind
echo f6120000.phy > unbind
ip link set eth6 down

That was enough. The result is a kernel crash, and the same should apply on popular platforms like the SolidRun MACCHIATOBin and other similar boards.

So this gets me to think that there is a wider problem around non-phylink-managed resources which may disappear while in use by network drivers, and I guess that the same applies in many other places. I don't have a SATA drive connected to that Marvell board, but I can imagine what happens when suddenly removing the comphy instance in charge of the SATA link and then a subsequent SATA hotplug event happens or stuff like that...

Anyway, back to network subsystem and phylink:

Do you agree that we need a way to register (and unregister) PCS providers with phylink, so we don't need *_get_by_of_node implementations in each driver? If so, can you sketch out what the basic requirements for an acceptable solution would be?

Also, do you agree that lack of handling of disappearing resources, such as PHYs (*not* network PHYs, but PHYs as in drivers/phy/*) or syscons, is already a problem right now which should be addressed?

If you imagine phylink to take care of the life-cycle of all link- resources, what is vision about those things other than classic MDIO- connected PHYs?

And well, of course, the easy fix for the example above would be:

drivers/phy/marvell/phy-mvebu-cp110-comphy.c +1 -0

index b0dd133665986..9517c96319595 100644
@@ -1099,6 +1099,7 @@ static const struct of_device_id mvebu_comphy_of_match_table[] = {
 MODULE_DEVICE_TABLE(of, mvebu_comphy_of_match_table);
 
 static struct platform_driver mvebu_comphy_driver = {
+	.suppress_bind_attrs = true,
 	.probe	= mvebu_comphy_probe,
 	.driver	= {
 		.name = "mvebu-comphy",

But that should then apply to every single driver in drivers/phy/...

Quoted 43 lines

Quoted 11 lines

(quoted diff) +9 -0

+
+	return &mpcs->pcs;
+}
+EXPORT_SYMBOL(mtk_pcs_lynxi_get);
+
+void mtk_pcs_lynxi_put(struct phylink_pcs *pcs)
+{
+	struct mtk_pcs_lynxi *cur, *mpcs = NULL;
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (!pcs)
+		return;
+
+	mutex_lock(&instance_mutex);
+	list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
+		if (pcs == &cur->pcs) {
+			mpcs = cur;
+			break;
+		}
+	mutex_unlock(&instance_mutex);

I don't see what this loop gains us, other than checking that the "pcs" is still on the list and hasn't already been removed. If that is all that this is about, then I would suggest:

	bool found = false;

	if (!pcs)
		return;

	mpcs = pcs_to_mtk_pcs_lynxi(pcs);
	mutex_lock(&instance_mutex);
	list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
		if (cur == mpcs) {
			found = true;
			break;
		}
	mutex_unlock(&instance_mutex);

	if (WARN_ON(!found))
		return;

Quoted 1 line

which makes it more obvious why this exists.

The idea was not only to make sure it hasn't been removed, but also to be sure that what ever the private data pointer points to has actually been created by that very driver.

But yes, doing it in the way you suggest will work in the same way, just when having my idea in mind it looks more fishy to use pcs_to_mtk_pcs_lynxi() before we are 100% sure that what we dealing with has previously been created by this driver.

Cheers

Daniel

Hi Russell,
Hi netdev crew,

I haven't received any reply for this email and am still waiting for clarification regarding how inter-driver dependencies should be modelled in that case of mtk_eth_soc. My search for good examples for that in the kernel code was quite frustrating and all I've found are supposedly bugs of that exact cathegory.

Please see my questions mentioned in the previous email I've sent and also a summary of suggested solutions inline below:

Quoted 133 lines · Daniel Golle

Hi Russell,

sorry for the extended time it took me to get back to this patch and the comments you made for it. Understanding the complete scope of the problem took a while (plus there were holidays and other fun things), and also brought up further questions which I hope you can help me find good answers for, see below:

Quoted 53 lines · Russell King (Oracle)

Quoted 4 lines · 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.

Signed-off-by Daniel Golle <[email protected]>

I made some specific suggestions about what I wanted to see for "getting" PCS in the previous review, and I'm disappointed that this patch set is still inventing its own solution.

Quoted 6 lines

(quoted diff) +6 -0

+struct phylink_pcs *mtk_pcs_lynxi_get(struct device *dev, struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct mtk_pcs_lynxi *mpcs;
+
+	if (!np)

Review

Quoted 10 lines

(quoted diff) +10 -0

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

This is racy - as I thought I described before, userspace can unbind the device in one thread, while another thread is calling this function. With just the right timing, this check succeeds, but...

Review

Quoted 6 lines

(quoted diff) +6 -0

+		if (pdev)
+			put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	mpcs = platform_get_drvdata(pdev);

mpcs ends up being read as NULL here. Even if you did manage to get a valid pointer, "mpcs" being devm-alloced could be freed from under you at this point...

Review

Quoted 1 line

(quoted diff) +1 -0

+	device_link_add(dev, mpcs->dev, DL_FLAG_AUTOREMOVE_CONSUMER);

resulting in this accessing memory which has been freed.

The solution would be either to suppress the bind/unbind attributes (provided the underlying struct device can't go away, which probably also means ensuring the same of the MDIO bus. Aternatively, adding a lock around the remove path and around the checking of platform_get_drvdata() down to adding the device link would probably solve it.

However, I come back to my general point - this kind of stuff is hairy. Do we want N different implementations of it in various drivers with subtle bugs, or do we want _one_ implemenatation.

Quoted 6 lines

If we go with the one implemenation approach, then we need to think about whether we should be using device links or not. The problem could be for network interfaces where one struct device is associated with multiple network interfaces. Using device links has the unfortunate side effect that if the PCS for one of those network interfaces is removed, _all_ network interfaces disappear.

I agree, esp. on this MT7988 removal of a PCS which may then not even be in use also impairs connectivity on the built-in gigE DSA switch. It would be nice to try to avoid this.

Quoted 2 lines

My original suggestion was to hook into phylink to cause that to take the link down when an in-use PCS gets removed.

I took a deep dive into how this could be done correctly and how similar things are done for other drivers. Apart from the PCS there often also is a muxing-PHY involved, eg. MediaTek's XFI T-PHY in this case (previously often called "pextp" for no apparent reason) or Marvell's comphy (mvebu-comphy).

So let's try something simple on an already well-supported platform, I thought and grabbed Marvell Armada CN9131-DB running vanilla Linux, and it didn't even take some something racy, but plain:

ip link set eth6 up
cd /sys/bus/platform/drivers/mvebu-comphy
echo f2120000.phy > unbind
echo f4120000.phy > unbind
echo f6120000.phy > unbind
ip link set eth6 down

That was enough. The result is a kernel crash, and the same should apply on popular platforms like the SolidRun MACCHIATOBin and other similar boards.

So this gets me to think that there is a wider problem around non-phylink-managed resources which may disappear while in use by network drivers, and I guess that the same applies in many other places. I don't have a SATA drive connected to that Marvell board, but I can imagine what happens when suddenly removing the comphy instance in charge of the SATA link and then a subsequent SATA hotplug event happens or stuff like that...

Anyway, back to network subsystem and phylink:

Do you agree that we need a way to register (and unregister) PCS providers with phylink, so we don't need *_get_by_of_node implementations in each driver? If so, can you sketch out what the basic requirements for an acceptable solution would be?

Also, do you agree that lack of handling of disappearing resources, such as PHYs (*not* network PHYs, but PHYs as in drivers/phy/*) or syscons, is already a problem right now which should be addressed?

If you imagine phylink to take care of the life-cycle of all link- resources, what is vision about those things other than classic MDIO- connected PHYs?

And well, of course, the easy fix for the example above would be:

drivers/phy/marvell/phy-mvebu-cp110-comphy.c +1 -0

index b0dd133665986..9517c96319595 100644
@@ -1099,6 +1099,7 @@ static const struct of_device_id mvebu_comphy_of_match_table[] = {
  MODULE_DEVICE_TABLE(of, mvebu_comphy_of_match_table);
 
  static struct platform_driver mvebu_comphy_driver = {
+	.suppress_bind_attrs = true,
  	.probe	= mvebu_comphy_probe,
  	.driver	= {
  		.name = "mvebu-comphy",

Quoted 2 lines

But that should then apply to every single driver in drivers/phy/...

My remaining questions are:
- Is it ok to just use .suppress_bind_attrs = true for PCS and PHY drivers to avoid having to care out hot-removal? Those components are anyway built-into the SoC so removing them is physically not possible.

- In case of driver removal (rmmod -f) imho using a device-link would be sufficient to prevent the worst. However, we would have to live with the all-or-nothing nature of that approach, ie. once e.g. the USXGMII driver is being removed, *all* Ethernet interfaces would disappear with it (even those not actually using USXGMII).

If the solutions mentioned above do not sound agreeable, please suggest how a good solution would look like, ideally also share an example of any driver in the kernel where this is done in the way you would like to have things done.

Quoted 56 lines

Quoted 40 lines

Quoted 9 lines

(quoted diff) +9 -0

+
+	return &mpcs->pcs;
+}
+EXPORT_SYMBOL(mtk_pcs_lynxi_get);
+
+void mtk_pcs_lynxi_put(struct phylink_pcs *pcs)
+{
+	struct mtk_pcs_lynxi *cur, *mpcs = NULL;
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (!pcs)
+		return;
+
+	mutex_lock(&instance_mutex);
+	list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
+		if (pcs == &cur->pcs) {
+			mpcs = cur;
+			break;
+		}
+	mutex_unlock(&instance_mutex);

I don't see what this loop gains us, other than checking that the "pcs" is still on the list and hasn't already been removed. If that is all that this is about, then I would suggest:

	bool found = false;

	if (!pcs)
		return;

	mpcs = pcs_to_mtk_pcs_lynxi(pcs);
	mutex_lock(&instance_mutex);
	list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
		if (cur == mpcs) {
			found = true;
			break;
		}
	mutex_unlock(&instance_mutex);

	if (WARN_ON(!found))
		return;

Quoted 1 line

which makes it more obvious why this exists.

The idea was not only to make sure it hasn't been removed, but also to be sure that what ever the private data pointer points to has actually been created by that very driver.

But yes, doing it in the way you suggest will work in the same way, just when having my idea in mind it looks more fishy to use pcs_to_mtk_pcs_lynxi() before we are 100% sure that what we dealing with has previously been created by this driver.

Cheers

Daniel

Gesendet: Mittwoch, 10. Juli 2024 um 13:17 Uhr
Von: "Frank Wunderlich" <[email protected]>
An: "Russell King (Oracle)" <[email protected]>
Cc: "David S. Miller" <[email protected]>, "Eric Dumazet" <[email protected]>, "Jakub Kicinski" <[email protected]>, "Paolo Abeni" <[email protected]>, "Rob Herring" <[email protected]>, "Krzysztof Kozlowski" <[email protected]>, "Conor Dooley" <[email protected]>, "Chunfeng Yun" <[email protected]>, "Vinod Koul" <[email protected]>, "Kishon Vijay Abraham I" <[email protected]>, "Felix Fietkau" <[email protected]>, "John Crispin" <[email protected]>, "Sean Wang" <[email protected]>, "Mark Lee" <[email protected]>, "Lorenzo Bianconi" <[email protected]>, "Matthias Brugger" <[email protected]>, "AngeloGioacchino Del Regno" <[email protected]>, "Andrew Lunn" <[email protected]>, "Heiner Kallweit" <[email protected]>, "Alexander Couzens" <[email protected]>, "Qingfang Deng" <[email protected]>, "SkyLake Huang" <[email protected]>, "Philipp Zabel" <[email protected]>, [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], "Daniel Golle" <[email protected]>
Betreff: Re: [RFC PATCH net-next v3 3/8] net: pcs: pcs-mtk-lynxi: add platform driver for MT7988

Hi Russel / netdev

how can we proceed here? development for mt7988/Bpi-R4 is stalled here because it is unclear what's the right way to go...

We want to avoid much work adding a complete new framework for a "small" change (imho this affects all SFP-like sub-devices and their linking to phylink/pcs driver)...if this really the way to go then some boundary conditions will be helpful.

regards Frank

Am 22.04.24 um 18:24 schrieb Daniel Golle:

Quoted 221 lines

Hi Russell,
Hi netdev crew,

I haven't received any reply for this email and am still waiting for clarification regarding how inter-driver dependencies should be modelled in that case of mtk_eth_soc. My search for good examples for that in the kernel code was quite frustrating and all I've found are supposedly bugs of that exact cathegory.

Please see my questions mentioned in the previous email I've sent and also a summary of suggested solutions inline below:

Quoted 133 lines · Daniel Golle

Hi Russell,

sorry for the extended time it took me to get back to this patch and the comments you made for it. Understanding the complete scope of the problem took a while (plus there were holidays and other fun things), and also brought up further questions which I hope you can help me find good answers for, see below:

Quoted 53 lines · Russell King (Oracle)

Quoted 4 lines · 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.

Signed-off-by Daniel Golle <[email protected]>

I made some specific suggestions about what I wanted to see for "getting" PCS in the previous review, and I'm disappointed that this patch set is still inventing its own solution.

Quoted 6 lines

(quoted diff) +6 -0

+struct phylink_pcs *mtk_pcs_lynxi_get(struct device *dev, struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct mtk_pcs_lynxi *mpcs;
+
+	if (!np)

Review

Quoted 10 lines

(quoted diff) +10 -0

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

This is racy - as I thought I described before, userspace can unbind the device in one thread, while another thread is calling this function. With just the right timing, this check succeeds, but...

Review

Quoted 6 lines

(quoted diff) +6 -0

+		if (pdev)
+			put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	mpcs = platform_get_drvdata(pdev);

mpcs ends up being read as NULL here. Even if you did manage to get a valid pointer, "mpcs" being devm-alloced could be freed from under you at this point...

Review

Quoted 1 line

(quoted diff) +1 -0

+	device_link_add(dev, mpcs->dev, DL_FLAG_AUTOREMOVE_CONSUMER);

resulting in this accessing memory which has been freed.

The solution would be either to suppress the bind/unbind attributes (provided the underlying struct device can't go away, which probably also means ensuring the same of the MDIO bus. Aternatively, adding a lock around the remove path and around the checking of platform_get_drvdata() down to adding the device link would probably solve it.

However, I come back to my general point - this kind of stuff is hairy. Do we want N different implementations of it in various drivers with subtle bugs, or do we want _one_ implemenatation.

Quoted 6 lines

If we go with the one implemenation approach, then we need to think about whether we should be using device links or not. The problem could be for network interfaces where one struct device is associated with multiple network interfaces. Using device links has the unfortunate side effect that if the PCS for one of those network interfaces is removed, _all_ network interfaces disappear.

I agree, esp. on this MT7988 removal of a PCS which may then not even be in use also impairs connectivity on the built-in gigE DSA switch. It would be nice to try to avoid this.

Quoted 2 lines

My original suggestion was to hook into phylink to cause that to take the link down when an in-use PCS gets removed.

I took a deep dive into how this could be done correctly and how similar things are done for other drivers. Apart from the PCS there often also is a muxing-PHY involved, eg. MediaTek's XFI T-PHY in this case (previously often called "pextp" for no apparent reason) or Marvell's comphy (mvebu-comphy).

So let's try something simple on an already well-supported platform, I thought and grabbed Marvell Armada CN9131-DB running vanilla Linux, and it didn't even take some something racy, but plain:

ip link set eth6 up
cd /sys/bus/platform/drivers/mvebu-comphy
echo f2120000.phy > unbind
echo f4120000.phy > unbind
echo f6120000.phy > unbind
ip link set eth6 down

That was enough. The result is a kernel crash, and the same should apply on popular platforms like the SolidRun MACCHIATOBin and other similar boards.

So this gets me to think that there is a wider problem around non-phylink-managed resources which may disappear while in use by network drivers, and I guess that the same applies in many other places. I don't have a SATA drive connected to that Marvell board, but I can imagine what happens when suddenly removing the comphy instance in charge of the SATA link and then a subsequent SATA hotplug event happens or stuff like that...

Anyway, back to network subsystem and phylink:

Do you agree that we need a way to register (and unregister) PCS providers with phylink, so we don't need *_get_by_of_node implementations in each driver? If so, can you sketch out what the basic requirements for an acceptable solution would be?

Also, do you agree that lack of handling of disappearing resources, such as PHYs (*not* network PHYs, but PHYs as in drivers/phy/*) or syscons, is already a problem right now which should be addressed?

If you imagine phylink to take care of the life-cycle of all link- resources, what is vision about those things other than classic MDIO- connected PHYs?

And well, of course, the easy fix for the example above would be:

drivers/phy/marvell/phy-mvebu-cp110-comphy.c +1 -0

index b0dd133665986..9517c96319595 100644
@@ -1099,6 +1099,7 @@ static const struct of_device_id mvebu_comphy_of_match_table[] = {
  MODULE_DEVICE_TABLE(of, mvebu_comphy_of_match_table);
 
  static struct platform_driver mvebu_comphy_driver = {
+	.suppress_bind_attrs = true,
  	.probe	= mvebu_comphy_probe,
  	.driver	= {
  		.name = "mvebu-comphy",

Quoted 2 lines

But that should then apply to every single driver in drivers/phy/...

My remaining questions are:
- Is it ok to just use .suppress_bind_attrs = true for PCS and PHY drivers to avoid having to care out hot-removal? Those components are anyway built-into the SoC so removing them is physically not possible.

- In case of driver removal (rmmod -f) imho using a device-link would be sufficient to prevent the worst. However, we would have to live with the all-or-nothing nature of that approach, ie. once e.g. the USXGMII driver is being removed, *all* Ethernet interfaces would disappear with it (even those not actually using USXGMII).

If the solutions mentioned above do not sound agreeable, please suggest how a good solution would look like, ideally also share an example of any driver in the kernel where this is done in the way you would like to have things done.

Quoted 56 lines

Quoted 40 lines

Quoted 9 lines

(quoted diff) +9 -0

+
+	return &mpcs->pcs;
+}
+EXPORT_SYMBOL(mtk_pcs_lynxi_get);
+
+void mtk_pcs_lynxi_put(struct phylink_pcs *pcs)
+{
+	struct mtk_pcs_lynxi *cur, *mpcs = NULL;
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (!pcs)
+		return;
+
+	mutex_lock(&instance_mutex);
+	list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
+		if (pcs == &cur->pcs) {
+			mpcs = cur;
+			break;
+		}
+	mutex_unlock(&instance_mutex);

I don't see what this loop gains us, other than checking that the "pcs" is still on the list and hasn't already been removed. If that is all that this is about, then I would suggest:

	bool found = false;

	if (!pcs)
		return;

	mpcs = pcs_to_mtk_pcs_lynxi(pcs);
	mutex_lock(&instance_mutex);
	list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
		if (cur == mpcs) {
			found = true;
			break;
		}
	mutex_unlock(&instance_mutex);

	if (WARN_ON(!found))
		return;

Quoted 1 line

which makes it more obvious why this exists.

The idea was not only to make sure it hasn't been removed, but also to be sure that what ever the private data pointer points to has actually been created by that very driver.

But yes, doing it in the way you suggest will work in the same way, just when having my idea in mind it looks more fishy to use pcs_to_mtk_pcs_lynxi() before we are 100% sure that what we dealing with has previously been created by this driver.

Cheers

Daniel

Hi netdev,

we are still stale in adding pcs driver here...can you please show us the way to go??

daniel posted another attempt but this seems also stale.
https://patchwork.kernel.org/project/netdevbpf/patch/ba4e359584a6b3bc4b3470822c42186d5b0856f9.1721910728.git.daniel@makrotopia.org/

@Angelo/Matthias, can you help solving this?

left full quote below....

regards Frank

Quoted 241 lines

Gesendet: Mittwoch, 10. Juli 2024 um 13:17 Uhr
Von: "Frank Wunderlich" <[email protected]>
An: "Russell King (Oracle)" <[email protected]>
Cc: "David S. Miller" <[email protected]>, "Eric Dumazet" <[email protected]>, "Jakub Kicinski" <[email protected]>, "Paolo Abeni" <[email protected]>, "Rob Herring" <[email protected]>, "Krzysztof Kozlowski" <[email protected]>, "Conor Dooley" <[email protected]>, "Chunfeng Yun" <[email protected]>, "Vinod Koul" <[email protected]>, "Kishon Vijay Abraham I" <[email protected]>, "Felix Fietkau" <[email protected]>, "John Crispin" <[email protected]>, "Sean Wang" <[email protected]>, "Mark Lee" <[email protected]>, "Lorenzo Bianconi" <[email protected]>, "Matthias Brugger" <[email protected]>, "AngeloGioacchino Del Regno" <[email protected]>, "Andrew Lunn" <[email protected]>, "Heiner Kallweit" <[email protected]>, "Alexander Couzens" <[email protected]>, "Qingfang Deng" <[email protected]>, "SkyLake Huang" <[email protected]>, "Philipp Zabel" <[email protected]>, [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], "Daniel Golle" <[email protected]>
Betreff: Re: [RFC PATCH net-next v3 3/8] net: pcs: pcs-mtk-lynxi: add platform driver for MT7988

Hi Russel / netdev

how can we proceed here? development for mt7988/Bpi-R4 is stalled here because it is unclear what's the right way to go...

We want to avoid much work adding a complete new framework for a "small" change (imho this affects all SFP-like sub-devices and their linking to phylink/pcs driver)...if this really the way to go then some boundary conditions will be helpful.

regards Frank

Am 22.04.24 um 18:24 schrieb Daniel Golle:

Quoted 221 lines

Hi Russell,
Hi netdev crew,

I haven't received any reply for this email and am still waiting for clarification regarding how inter-driver dependencies should be modelled in that case of mtk_eth_soc. My search for good examples for that in the kernel code was quite frustrating and all I've found are supposedly bugs of that exact cathegory.

Please see my questions mentioned in the previous email I've sent and also a summary of suggested solutions inline below:

Quoted 133 lines · Daniel Golle

Hi Russell,

sorry for the extended time it took me to get back to this patch and the comments you made for it. Understanding the complete scope of the problem took a while (plus there were holidays and other fun things), and also brought up further questions which I hope you can help me find good answers for, see below:

Quoted 53 lines · Russell King (Oracle)

Quoted 4 lines · 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.

Signed-off-by Daniel Golle <[email protected]>

I made some specific suggestions about what I wanted to see for "getting" PCS in the previous review, and I'm disappointed that this patch set is still inventing its own solution.

Quoted 6 lines

(quoted diff) +6 -0

+struct phylink_pcs *mtk_pcs_lynxi_get(struct device *dev, struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct mtk_pcs_lynxi *mpcs;
+
+	if (!np)

Review

Quoted 10 lines

(quoted diff) +10 -0

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

This is racy - as I thought I described before, userspace can unbind the device in one thread, while another thread is calling this function. With just the right timing, this check succeeds, but...

Review

Quoted 6 lines

(quoted diff) +6 -0

+		if (pdev)
+			put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	mpcs = platform_get_drvdata(pdev);

mpcs ends up being read as NULL here. Even if you did manage to get a valid pointer, "mpcs" being devm-alloced could be freed from under you at this point...

Review

Quoted 1 line

(quoted diff) +1 -0

+	device_link_add(dev, mpcs->dev, DL_FLAG_AUTOREMOVE_CONSUMER);

resulting in this accessing memory which has been freed.

The solution would be either to suppress the bind/unbind attributes (provided the underlying struct device can't go away, which probably also means ensuring the same of the MDIO bus. Aternatively, adding a lock around the remove path and around the checking of platform_get_drvdata() down to adding the device link would probably solve it.

However, I come back to my general point - this kind of stuff is hairy. Do we want N different implementations of it in various drivers with subtle bugs, or do we want _one_ implemenatation.

Quoted 6 lines

If we go with the one implemenation approach, then we need to think about whether we should be using device links or not. The problem could be for network interfaces where one struct device is associated with multiple network interfaces. Using device links has the unfortunate side effect that if the PCS for one of those network interfaces is removed, _all_ network interfaces disappear.

I agree, esp. on this MT7988 removal of a PCS which may then not even be in use also impairs connectivity on the built-in gigE DSA switch. It would be nice to try to avoid this.

Quoted 2 lines

My original suggestion was to hook into phylink to cause that to take the link down when an in-use PCS gets removed.

I took a deep dive into how this could be done correctly and how similar things are done for other drivers. Apart from the PCS there often also is a muxing-PHY involved, eg. MediaTek's XFI T-PHY in this case (previously often called "pextp" for no apparent reason) or Marvell's comphy (mvebu-comphy).

So let's try something simple on an already well-supported platform, I thought and grabbed Marvell Armada CN9131-DB running vanilla Linux, and it didn't even take some something racy, but plain:

ip link set eth6 up
cd /sys/bus/platform/drivers/mvebu-comphy
echo f2120000.phy > unbind
echo f4120000.phy > unbind
echo f6120000.phy > unbind
ip link set eth6 down

That was enough. The result is a kernel crash, and the same should apply on popular platforms like the SolidRun MACCHIATOBin and other similar boards.

So this gets me to think that there is a wider problem around non-phylink-managed resources which may disappear while in use by network drivers, and I guess that the same applies in many other places. I don't have a SATA drive connected to that Marvell board, but I can imagine what happens when suddenly removing the comphy instance in charge of the SATA link and then a subsequent SATA hotplug event happens or stuff like that...

Anyway, back to network subsystem and phylink:

Do you agree that we need a way to register (and unregister) PCS providers with phylink, so we don't need *_get_by_of_node implementations in each driver? If so, can you sketch out what the basic requirements for an acceptable solution would be?

Also, do you agree that lack of handling of disappearing resources, such as PHYs (*not* network PHYs, but PHYs as in drivers/phy/*) or syscons, is already a problem right now which should be addressed?

If you imagine phylink to take care of the life-cycle of all link- resources, what is vision about those things other than classic MDIO- connected PHYs?

And well, of course, the easy fix for the example above would be:

drivers/phy/marvell/phy-mvebu-cp110-comphy.c +1 -0

index b0dd133665986..9517c96319595 100644
@@ -1099,6 +1099,7 @@ static const struct of_device_id mvebu_comphy_of_match_table[] = {
  MODULE_DEVICE_TABLE(of, mvebu_comphy_of_match_table);
 
  static struct platform_driver mvebu_comphy_driver = {
+	.suppress_bind_attrs = true,
  	.probe	= mvebu_comphy_probe,
  	.driver	= {
  		.name = "mvebu-comphy",

Quoted 2 lines

But that should then apply to every single driver in drivers/phy/...

My remaining questions are:
- Is it ok to just use .suppress_bind_attrs = true for PCS and PHY drivers to avoid having to care out hot-removal? Those components are anyway built-into the SoC so removing them is physically not possible.

- In case of driver removal (rmmod -f) imho using a device-link would be sufficient to prevent the worst. However, we would have to live with the all-or-nothing nature of that approach, ie. once e.g. the USXGMII driver is being removed, *all* Ethernet interfaces would disappear with it (even those not actually using USXGMII).

If the solutions mentioned above do not sound agreeable, please suggest how a good solution would look like, ideally also share an example of any driver in the kernel where this is done in the way you would like to have things done.

Quoted 56 lines

Quoted 40 lines

Quoted 9 lines

(quoted diff) +9 -0

+
+	return &mpcs->pcs;
+}
+EXPORT_SYMBOL(mtk_pcs_lynxi_get);
+
+void mtk_pcs_lynxi_put(struct phylink_pcs *pcs)
+{
+	struct mtk_pcs_lynxi *cur, *mpcs = NULL;
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (!pcs)
+		return;
+
+	mutex_lock(&instance_mutex);
+	list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
+		if (pcs == &cur->pcs) {
+			mpcs = cur;
+			break;
+		}
+	mutex_unlock(&instance_mutex);

I don't see what this loop gains us, other than checking that the "pcs" is still on the list and hasn't already been removed. If that is all that this is about, then I would suggest:

	bool found = false;

	if (!pcs)
		return;

	mpcs = pcs_to_mtk_pcs_lynxi(pcs);
	mutex_lock(&instance_mutex);
	list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
		if (cur == mpcs) {
			found = true;
			break;
		}
	mutex_unlock(&instance_mutex);

	if (WARN_ON(!found))
		return;

Quoted 1 line

which makes it more obvious why this exists.

The idea was not only to make sure it hasn't been removed, but also to be sure that what ever the private data pointer points to has actually been created by that very driver.

But yes, doing it in the way you suggest will work in the same way, just when having my idea in mind it looks more fishy to use pcs_to_mtk_pcs_lynxi() before we are 100% sure that what we dealing with has previously been created by this driver.

Cheers

Daniel

Gesendet: Freitag, 4. Oktober 2024 um 16:35
Von: "Russell King (Oracle)" <[email protected]>
Betreff: Re: Aw: Re: [RFC PATCH net-next v3 3/8] net: pcs: pcs-mtk-lynxi: add platform driver for MT7988

Hi Frank,

Sorry, but I've not been able to look at this, and I've completely lost all context now. I was diverted onto a high priority work issue for a while (was it from April to end of June) so didn't have much time available for mainline work. I then had a much needed holiday (three weeks) in July. I then had a clear week where I did look at mainline. Since then, I've had two cataract operations that have made being on the computer somewhat difficult, and it is only recently that I'm effectively "back" after what is approximately six months of not having a lot of bandwidth. I've seen the cataract consultant this morning, and just found out that my optometrist appointment for Tuesday is too soon after the cataract operation, and needs to be moved two weeks. The optometrist doesn't have availability then, so it's going to be another four weeks. FFS... I wish I'd known, then I could've made an arrangement with the optometrist months ago for the correct date.

Now, XPCS has introduced a hack in a similar way to what you're trying to do, but I wasn't able to review it, so it went in. We're heading towards the situation where every PCS driver is going to have its own way to look up a PCS registered as a device. This is not going to scale.

We need something better than this - and at the moment that's all I can say because I haven't given it any more thought beyond that so far.

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.

Signed-off-by Daniel Golle <[email protected]>

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

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.

Signed-off-by Daniel Golle <[email protected]>

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

Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml new file +27 -0

new file mode 100644
index 0000000000000..0cdaa3545edb0
@@ -0,0 +1,60 @@
+# 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
+

Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml new file +4 -0

+
+required:
+  - compatible
+  - reg

Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml new file +10 -0

+    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>;
+          resets = <&watchdog MT7988_TOPRGU_XFI0_GRST>;
+        };
+    };

Signature · 1 line

2.43.0

Add driver for USXGMII PCS found in the MediaTek MT7988 SoC and supporting USXGMII, 10GBase-R and 5GBase-R interface modes.

Signed-off-by Daniel Golle <[email protected]>

drivers/net/pcs/pcs-mtk-usxgmii.c +0 -0

include/linux/pcs/pcs-mtk-usxgmii.h new file +0 -0

new file mode 100644
index 0000000000000..ef936d9c5f116

include/linux/pcs/pcs-mtk-usxgmii.h new file +10 -0

+void mtk_usxgmii_pcs_put(struct phylink_pcs *pcs);
+#else
+static inline struct phylink_pcs *mtk_usxgmii_pcs_get(struct device *dev, struct device_node *np)
+{
+	return NULL;
+}
+static inline void mtk_usxgmii_pcs_put(struct phylink_pcs *pcs) { }
+#endif /* IS_ENABLED(CONFIG_PCS_MTK_USXGMII) */
+
+#endif /* __LINUX_PCS_MTK_USXGMII_H */

Signature · 1 line

2.43.0

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")

Signed-off-by Daniel Golle <[email protected]>

---
 .../devicetree/bindings/net/mediatek,net.yaml | 32 ++++---------------
 1 file changed, 7 insertions(+), 25 deletions(-)

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")

Signed-off-by Daniel Golle <[email protected]>

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")

Signed-off-by Daniel Golle <[email protected]>

---
 .../devicetree/bindings/net/mediatek,net.yaml | 148 +++++++++++++++++-
 1 file changed, 146 insertions(+), 2 deletions(-)

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")

Signed-off-by Daniel Golle <[email protected]>

---
 .../devicetree/bindings/net/mediatek,net.yaml | 148 +++++++++++++++++-
 1 file changed, 146 insertions(+), 2 deletions(-)

Documentation/devicetree/bindings/net/mediatek,net.yaml +145 -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,12 @@ allOf:
             const: mediatek,mt7988-eth
     then:
       properties:
+        reg:
+          minItems: 2
+
         interrupts:
           minItems: 4
+          maxItems: 4
 
         clocks:
           minItems: 24

@@ -368,7 +393,7 @@ allOf:
             - const: top_netsys_warp_sel
 
 patternProperties:
-  "^mac@[0-1]$":
+  "^mac@[0-2]$":
     type: object
     unevaluatedProperties: false
     allOf:

@@ -382,6 +407,9 @@ patternProperties:
       reg:
         maxItems: 1
 
+      phys:
+        maxItems: 1
+
     required:
       - reg
       - compatible

@@ -559,3 +587,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>, <&sgmii0>;
+          phys = <&pextp0>;
+        };
+
+        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

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")

Signed-off-by Daniel Golle <[email protected]>

---