net: ethernet: mtk_eth_soc: various enhancements

Introduce DT bindings for the MT7981 SoC to mediatek,net.yaml.

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

---
 .../devicetree/bindings/net/mediatek,net.yaml | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)

Documentation/devicetree/bindings/net/mediatek,net.yaml +31 -0

index 7ef696204c5a..d17e2eb46118 100644
@@ -21,6 +21,7 @@ properties:
       - mediatek,mt7623-eth
       - mediatek,mt7622-eth
       - mediatek,mt7629-eth
+      - mediatek,mt7981-eth
       - mediatek,mt7986-eth
       - ralink,rt5350-eth
 

@@ -206,6 +207,47 @@ allOf:
 
         mediatek,wed: false
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: mediatek,mt7981-eth
+    then:
+      properties:
+        interrupts:
+          minItems: 4
+
+        clocks:
+          minItems: 15
+          maxItems: 15
+
+        clock-names:
+          items:
+            - const: fe
+            - const: gp2
+            - const: gp1
+            - const: wocpu0
+            - const: sgmii_ck
+            - const: sgmii_tx250m
+            - const: sgmii_rx250m
+            - const: sgmii_cdr_ref
+            - const: sgmii_cdr_fb
+            - const: sgmii2_tx250m
+            - const: sgmii2_rx250m
+            - const: sgmii2_cdr_ref
+            - const: sgmii2_cdr_fb
+            - const: netsys0

Add documentation for the newly introduced 'mediatek,pn_swap' property to mediatek,sgmiisys.txt.

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

---
 .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt    | 4 ++++
 1 file changed, 4 insertions(+)

Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt +0 -0

index d2c24c277514..b38dd0fde21d 100644

Quoted 3 lines · Daniel Golle

Add documentation for the newly introduced 'mediatek,pn_swap' property to mediatek,sgmiisys.txt.

Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel.

Quoted 13 lines

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

---
 .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt    | 4 ++++
 1 file changed, 4 insertions(+)

Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt +0 -0

index d2c24c277514..b38dd0fde21d 100644
@@ -14,6 +14,10 @@ Required Properties:
 	- "mediatek,mt7986-sgmiisys_1", "syscon"
 - #clock-cells: Must be 1
 

Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt Required Properties: · line 19

Quoted 3 lines

Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt +3 -0

+Optional Properties:
+
+- mediatek,pn_swap: Invert polarity of the SGMII data lanes.

No:
1. No new properties for TXT bindings,

2. Underscore is not allowed.

3. Does not look like property of this node. This is a clock controller or system controller, not SGMII/phy etc.

Hi Krzysztof,

Quoted 23 lines · Krzysztof Kozlowski

Quoted 3 lines · Daniel Golle

Add documentation for the newly introduced 'mediatek,pn_swap' property to mediatek,sgmiisys.txt.

Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel.

Quoted 13 lines

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

---
 .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt    | 4 ++++
 1 file changed, 4 insertions(+)

Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt +0 -0

index d2c24c277514..b38dd0fde21d 100644
@@ -14,6 +14,10 @@ Required Properties:
 	- "mediatek,mt7986-sgmiisys_1", "syscon"
 - #clock-cells: Must be 1
 

Quoted 6 lines

Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt Required Properties: · line 19

Quoted 3 lines

Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt +3 -0

+Optional Properties:
+
+- mediatek,pn_swap: Invert polarity of the SGMII data lanes.

No:
1. No new properties for TXT bindings,

So I'll have to convert the bindings to YAML, right?

Quoted 2 lines

2. Underscore is not allowed.

Ack, will change the name of the property.

Quoted 2 lines

3. Does not look like property of this node. This is a clock controller or system controller, not SGMII/phy etc.

The register range referred to by this node *does* represent also an SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and are referenced in the node of the Ethernet core, and then used by drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap. (This is the current situation already, and not related to the patchset now adding only a new property to support hardware which needs that)

So: Should I introduce a new binding for the same compatible strings related to the SGMII PHY features? Or is it fine in this case to add this property to the existing binding?

Quoted 10 lines · Daniel Golle

...

Quoted 2 lines

3. Does not look like property of this node. This is a clock controller or system controller, not SGMII/phy etc.

The register range referred to by this node *does* represent also an SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and are referenced in the node of the Ethernet core, and then used by drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap. (This is the current situation already, and not related to the patchset now adding only a new property to support hardware which needs that)

Just because a register is located in syscon block, does not mean that SGMII configuration is a property of this device.

Quoted 5 lines

So: Should I introduce a new binding for the same compatible strings related to the SGMII PHY features? Or is it fine in this case to add this property to the existing binding?

The user of syscon should configure it. I don't think you need new binding. You just have to update the user of this syscon.

Quoted 12 lines · Krzysztof Kozlowski

Quoted 10 lines · Daniel Golle

...

Quoted 2 lines

3. Does not look like property of this node. This is a clock controller or system controller, not SGMII/phy etc.

The register range referred to by this node *does* represent also an SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and are referenced in the node of the Ethernet core, and then used by drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap. (This is the current situation already, and not related to the patchset now adding only a new property to support hardware which needs that)

Quoted 2 lines

Just because a register is located in syscon block, does not mean that SGMII configuration is a property of this device.

It's not just one register, the whole SGMII PCS is located in those mediatek,sgmiisys syscon nodes.

Quoted 7 lines

Quoted 6 lines

So: Should I introduce a new binding for the same compatible strings related to the SGMII PHY features? Or is it fine in this case to add this property to the existing binding?

Quoted 2 lines

The user of syscon should configure it. I don't think you need new binding. You just have to update the user of this syscon.

Excuse my confusion, but it's still not entirely clear to me. So in this case I should add the description of the added propterty of the individual SGMII units (there can be more than one) to Documentation/devicetree/bindings/net/mediatek,net.yaml eventhough the properties are in the sgmiisys syscon nodes?

If so I will have to figure out how to describe properties of other nodes in the binding of the node referencing them. Are there any good examples for that?

Or should the property itself be moved into yet another array of booleans which should be added in the node describing the ethernet controller and referencing these sgmiisys syscons using phandles?

Quoted 16 lines · Daniel Golle

Quoted 14 lines · Krzysztof Kozlowski

Quoted 10 lines · Daniel Golle

...

Quoted 2 lines

3. Does not look like property of this node. This is a clock controller or system controller, not SGMII/phy etc.

The register range referred to by this node *does* represent also an SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and are referenced in the node of the Ethernet core, and then used by drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap. (This is the current situation already, and not related to the patchset now adding only a new property to support hardware which needs that)

Just because a register is located in syscon block, does not mean that SGMII configuration is a property of this device.

Quoted 2 lines

It's not just one register, the whole SGMII PCS is located in those mediatek,sgmiisys syscon nodes.

Then maybe this should be a PCS PHY device instead of adding properties unrelated to clock/system controller? I don't know, currently this binding says it is a provider of clocks...

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

So: Should I introduce a new binding for the same compatible strings related to the SGMII PHY features? Or is it fine in this case to add this property to the existing binding?

Quoted 8 lines

Quoted 2 lines

The user of syscon should configure it. I don't think you need new binding. You just have to update the user of this syscon.

Excuse my confusion, but it's still not entirely clear to me. So in this case I should add the description of the added propterty of the individual SGMII units (there can be more than one) to Documentation/devicetree/bindings/net/mediatek,net.yaml eventhough the properties are in the sgmiisys syscon nodes?

I guess the property should be in the node representing the SGMII. You add it now to the clock (or system) controller, so it does not look right. It's not a property of a clock controller.

Now which node should have this property depends on your devices - which I have no clue about, I read what is in the bindings.

Quoted 5 lines

If so I will have to figure out how to describe properties of other nodes in the binding of the node referencing them. Are there any good examples for that?

phys and pcs'es?

Quoted 5 lines

Or should the property itself be moved into yet another array of booleans which should be added in the node describing the ethernet controller and referencing these sgmiisys syscons using phandles?

Best regards,
Krzysztof

Hi Krzysztof,

thank you for taking the time to review and explain.

Quoted 17 lines · Krzysztof Kozlowski

Quoted 16 lines · Daniel Golle

Quoted 14 lines · Krzysztof Kozlowski

Quoted 10 lines · Daniel Golle

...

Quoted 2 lines

3. Does not look like property of this node. This is a clock controller or system controller, not SGMII/phy etc.

The register range referred to by this node *does* represent also an SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and are referenced in the node of the Ethernet core, and then used by drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap. (This is the current situation already, and not related to the patchset now adding only a new property to support hardware which needs that)

Just because a register is located in syscon block, does not mean that SGMII configuration is a property of this device.

Quoted 6 lines

Quoted 2 lines

It's not just one register, the whole SGMII PCS is located in those mediatek,sgmiisys syscon nodes.

Then maybe this should be a PCS PHY device instead of adding properties unrelated to clock/system controller? I don't know, currently this binding says it is a provider of clocks...

As in reality it is really a clock provider and also SGMII PCS at the same time, maybe we should just update the description of the binding to match that:

Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt +1 -0

index d2c24c2775141..db6f75df200ba 100644
@@ -2,6 +2,7 @@ MediaTek SGMIISYS controller
 ============================
 
 The MediaTek SGMIISYS controller provides various clocks to the system.
+It also represents the SGMII PCS used by the Ethernet core.
 
 Required Properties:
 

See

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/mediatek/mtk_sgmii.c#n179

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt7986a.dtsi#n409

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

So: Should I introduce a new binding for the same compatible strings related to the SGMII PHY features? Or is it fine in this case to add this property to the existing binding?

Quoted 8 lines

Quoted 2 lines

The user of syscon should configure it. I don't think you need new binding. You just have to update the user of this syscon.

Excuse my confusion, but it's still not entirely clear to me. So in this case I should add the description of the added propterty of the individual SGMII units (there can be more than one) to Documentation/devicetree/bindings/net/mediatek,net.yaml eventhough the properties are in the sgmiisys syscon nodes?

Quoted 3 lines

I guess the property should be in the node representing the SGMII. You add it now to the clock (or system) controller, so it does not look right. It's not a property of a clock controller.

Well maybe this node needs to be split then into one node representing only the clock controller and another node representing the SGMII PCS? I'm not sure if this is even possible, some registers in this range represent clocks, other registers are accessed using regmap API in mtk_sgmii.c.

And (see the rest of this series) the exact same SGMII PCS can also be found in MT7531 switch IC which has it's own (a bit odd) way to access 32-bit registers over MDIO, also in this case it is simply not easily possible to represent the SGMII PCS in device tree.

Quoted 3 lines

Now which node should have this property depends on your devices - which I have no clue about, I read what is in the bindings.

There isn't any other node exclusively representing the SGMII PCS. I guess the only other option would be to move the property to the Ethernet controller node, which imho complicates things as it is really a property of an individual SGMII PHY (of which there can be more than one).

Quoted 2 lines

Quoted 2 lines

Quoted 5 lines

Quoted 3 lines

If so I will have to figure out how to describe properties of other nodes in the binding of the node referencing them. Are there any good examples for that?

phys and pcs'es?

Hm, none of the current PCS (or PHY) drivers are represented by a syscon node... (and maybe that's the mistake in first place?)

Quoted 2 lines · Daniel Golle

Hm, none of the current PCS (or PHY) drivers are represented by a syscon node... (and maybe that's the mistake in first place?)

Yes.

Quoted 5 lines · Krzysztof Kozlowski

Quoted 2 lines · Daniel Golle

Hm, none of the current PCS (or PHY) drivers are represented by a syscon node... (and maybe that's the mistake in first place?)

Yes.

Nos, it isn't.

Quoted 8 lines · Russell King (Oracle)

Quoted 5 lines · Krzysztof Kozlowski

Quoted 2 lines · Daniel Golle

Hm, none of the current PCS (or PHY) drivers are represented by a syscon node... (and maybe that's the mistake in first place?)

Yes.

Nos, it isn't.

To expand on this - I have no idea why you consider it a mistake that apparently all PCS aren't represented by a syscon node.

PCS is a sub-block in an ethernet system, just the same as a MAC is a sub-block. PCS can appear in several locations of an ethernet system, but are generally found either side of a serial ethernet link such as 1000base-X, SGMII, USXGMII, 10Gbase-R etc.

So, one can find PCS within an ethernet PHY - and there may be one facing the MAC connection, and there will be another facing the media. We generally do not need to separate these PCS from the PHY itself because we view the PHY as one whole device.

The optional PCS on the MAC side of the link is something that we need to know about, because this has to be configured to talk to the PHY, or to configure and obtain negotiation results from in the case of fibre links.

PCS on the MAC side are not a system level device, they are very much a specific piece of ethernet hardware in the same way that the MAC is, and we don't represent the MAC as a syscon node. There is no reason to do so with PCS.

These PCS on the MAC side tend to be accessed via direct MMIO accesses, or over a MDIO bus.

There's other blocks in the IEEE 802.3 ethernet layering, such as the PMA/PMD module (which for the MAC side we tend to model with the drivers/phy layer) - but again, these also appear in ethernet PHYs in order to produce the electrical signals for e.g. twisted pair ethernet.

So, to effectively state that you consider that PCS should always be represented as a syscon node is rather naieve, and really as a DT reviewer you should not be making such decisions, but soliciting opinions from those who know this subject area in detail _whether_ they are some kind of system controller before making such a decision.

Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt Required Properties: · line 19

Quoted 3 lines · Daniel Golle

Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt +3 -0

+Optional Properties:
+
+- mediatek,pn_swap: Invert polarity of the SGMII data lanes.

No:
3. Does not look like property of this node. This is a clock controller or system controller, not SGMII/phy etc.

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

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

Reset PCS state when changing interface mode.

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

drivers/net/pcs/Kconfig +5 -0

+	select REGMAP
+	help
+	  This module provides helpers to phylink for managing the LynxI PCS
+	  which is part of MediaTek's SoC and Ethernet switch ICs.
+
 config PCS_RZN1_MIIC
 	tristate "Renesas RZ/N1 MII converter"
 	depends on OF && (ARCH_RZN1 || COMPILE_TEST)

drivers/net/pcs/Makefile +1 -0

index 4c780d8f2e98..9b9afd6b1c22 100644
@@ -5,5 +5,6 @@ pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-nxp.o
 
 obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
 obj-$(CONFIG_PCS_LYNX)		+= pcs-lynx.o
+obj-$(CONFIG_PCS_MTK_LYNXI)	+= pcs-mtk-lynxi.o
 obj-$(CONFIG_PCS_RZN1_MIIC)	+= pcs-rzn1-miic.o
 obj-$(CONFIG_PCS_ALTERA_TSE)	+= pcs-altera-tse.o

drivers/net/pcs/pcs-mtk-lynxi.c new file +13 -0

new file mode 100644
index 000000000000..0100def53d45
@@ -0,0 +1,315 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018-2019 MediaTek Inc.
+/* A library for MediaTek SGMII circuit
+ *
+ * Author: Sean Wang <[email protected]>
+ * Author: Daniel Golle <[email protected]>
+ *
+ */
+#include <linux/mdio.h>
+#include <linux/phylink.h>
+#include <linux/pcs/pcs-mtk-lynxi.h>
+#include <linux/of.h>
+#include <linux/phylink.h>

drivers/net/pcs/pcs-mtk-lynxi.c new file +5 -0

+
+#define SGMSYS_PCS_DEVICE_ID		0x4
+#define SGMII_LYNXI_DEV_ID		0x4d544950
+
+#define SGMSYS_PCS_ADVERTISE		0x8

drivers/net/pcs/pcs-mtk-lynxi.c new file +5 -0

+
+/* Register to control remote fault */
+#define SGMSYS_SGMII_MODE		0x20
+#define SGMII_IF_MODE_SGMII		BIT(0)
+#define SGMII_SPEED_DUPLEX_AN		BIT(1)

drivers/net/pcs/pcs-mtk-lynxi.c new file +4 -0

+#define RG_PHY_SPEED_MASK		(BIT(2) | BIT(3))
+#define RG_PHY_SPEED_1_25G		0x0
+#define RG_PHY_SPEED_3_125G		BIT(2)
+

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

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

---
 drivers/net/ethernet/mediatek/Kconfig       |   2 +
 drivers/net/ethernet/mediatek/mtk_eth_soc.c |  13 +-
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |  80 +-------
 drivers/net/ethernet/mediatek/mtk_sgmii.c   | 202 +++-----------------
 4 files changed, 38 insertions(+), 259 deletions(-)

drivers/net/ethernet/mediatek/Kconfig +2 -0

index 97374fb3ee79..da0db417ab69 100644
@@ -19,6 +19,8 @@ config NET_MEDIATEK_SOC
 	select DIMLIB
 	select PAGE_POOL
 	select PAGE_POOL_STATS
+	select PCS_MTK_LYNXI
+	select REGMAP_MMIO
 	help
 	  This driver supports the gigabit ethernet MACs in the
 	  MediaTek SoC family.

drivers/net/ethernet/mediatek/mtk_eth_soc.c +10 -3

index 97df77c7999e..54e85f54d7fc 100644
@@ -4077,6 +4077,7 @@ static int mtk_unreg_dev(struct mtk_eth *eth)
 
 static int mtk_cleanup(struct mtk_eth *eth)
 {
+	mtk_sgmii_destroy(eth->sgmii);
 	mtk_unreg_dev(eth);
 	mtk_free_dev(eth);
 	cancel_work_sync(&eth->pending_work);

@@ -4580,6 +4581,7 @@ static int mtk_probe(struct platform_device *pdev)
 		if (!eth->sgmii)
 			return -ENOMEM;
 
+		eth->sgmii->dev = eth->dev;
 		err = mtk_sgmii_init(eth->sgmii, pdev->dev.of_node,
 				     eth->soc->ana_rgc3);
 

@@ -4592,14 +4594,17 @@ static int mtk_probe(struct platform_device *pdev)
 							    "mediatek,pctl");
 		if (IS_ERR(eth->pctl)) {
 			dev_err(&pdev->dev, "no pctl regmap found\n");
-			return PTR_ERR(eth->pctl);
+			err = PTR_ERR(eth->pctl);
+			goto err_destroy_sgmii;
 		}
 	}
 
 	if (MTK_HAS_CAPS(eth->soc->caps, MTK_NETSYS_V2)) {
 		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-		if (!res)
-			return -EINVAL;
+		if (!res) {
+			err = -EINVAL;
+			goto err_destroy_sgmii;
+		}
 	}
 
 	if (eth->soc->offload_version) {

@@ -4749,6 +4754,8 @@ static int mtk_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_destroy_sgmii:
+	mtk_sgmii_destroy(eth->sgmii);
 err_deinit_ppe:
 	mtk_ppe_deinit(eth);
 	mtk_mdio_cleanup(eth);

drivers/net/ethernet/mediatek/mtk_eth_soc.h +3 -77

index c2e0fd773cc2..a72748d80bba 100644
@@ -510,65 +510,6 @@
 #define ETHSYS_DMA_AG_MAP_QDMA	BIT(1)
 #define ETHSYS_DMA_AG_MAP_PPE	BIT(2)
 
-/* SGMII subsystem config registers */
-/* BMCR (low 16) BMSR (high 16) */
-#define SGMSYS_PCS_CONTROL_1	0x0
-#define SGMII_BMCR		GENMASK(15, 0)
-#define SGMII_BMSR		GENMASK(31, 16)
-#define SGMII_AN_RESTART	BIT(9)
-#define SGMII_ISOLATE		BIT(10)
-#define SGMII_AN_ENABLE		BIT(12)
-#define SGMII_LINK_STATYS	BIT(18)
-#define SGMII_AN_ABILITY	BIT(19)
-#define SGMII_AN_COMPLETE	BIT(21)
-#define SGMII_PCS_FAULT		BIT(23)
-#define SGMII_AN_EXPANSION_CLR	BIT(30)
-
-#define SGMSYS_PCS_ADVERTISE	0x8
-#define SGMII_ADVERTISE		GENMASK(15, 0)
-#define SGMII_LPA		GENMASK(31, 16)
-
-/* Register to programmable link timer, the unit in 2 * 8ns */
-#define SGMSYS_PCS_LINK_TIMER	0x18
-#define SGMII_LINK_TIMER_MASK	GENMASK(19, 0)
-#define SGMII_LINK_TIMER_DEFAULT	(0x186a0 & SGMII_LINK_TIMER_MASK)
-
-/* Register to control remote fault */
-#define SGMSYS_SGMII_MODE		0x20
-#define SGMII_IF_MODE_SGMII		BIT(0)
-#define SGMII_SPEED_DUPLEX_AN		BIT(1)
-#define SGMII_SPEED_MASK		GENMASK(3, 2)
-#define SGMII_SPEED_10			FIELD_PREP(SGMII_SPEED_MASK, 0)
-#define SGMII_SPEED_100			FIELD_PREP(SGMII_SPEED_MASK, 1)
-#define SGMII_SPEED_1000		FIELD_PREP(SGMII_SPEED_MASK, 2)
-#define SGMII_DUPLEX_HALF		BIT(4)
-#define SGMII_IF_MODE_BIT5		BIT(5)
-#define SGMII_REMOTE_FAULT_DIS		BIT(8)
-#define SGMII_CODE_SYNC_SET_VAL		BIT(9)
-#define SGMII_CODE_SYNC_SET_EN		BIT(10)
-#define SGMII_SEND_AN_ERROR_EN		BIT(11)
-#define SGMII_IF_MODE_MASK		GENMASK(5, 1)
-
-/* Register to reset SGMII design */
-#define SGMII_RESERVED_0	0x34
-#define SGMII_SW_RESET		BIT(0)
-
-/* Register to set SGMII speed, ANA RG_ Control Signals III*/
-#define SGMSYS_ANA_RG_CS3	0x2028
-#define RG_PHY_SPEED_MASK	(BIT(2) | BIT(3))
-#define RG_PHY_SPEED_1_25G	0x0
-#define RG_PHY_SPEED_3_125G	BIT(2)
-
-/* Register to power up QPHY */
-#define SGMSYS_QPHY_PWR_STATE_CTRL 0xe8
-#define	SGMII_PHYA_PWD		BIT(4)
-
-/* Register to QPHY wrapper control */
-#define SGMSYS_QPHY_WRAP_CTRL	0xec
-#define SGMII_PN_SWAP_MASK	GENMASK(1, 0)
-#define SGMII_PN_SWAP_TX_RX	(BIT(0) | BIT(1))
-#define MTK_SGMII_FLAG_PN_SWAP	BIT(0)
-
 /* Infrasys subsystem config registers */
 #define INFRA_MISC2            0x70c
 #define CO_QPHY_SEL            BIT(0)

@@ -1103,29 +1044,13 @@ struct mtk_soc_data {
 /* currently no SoC has more than 2 macs */
 #define MTK_MAX_DEVS			2
 
-/* struct mtk_pcs -    This structure holds each sgmii regmap and associated
- *                     data
- * @regmap:            The register map pointing at the range used to setup
- *                     SGMII modes
- * @ana_rgc3:          The offset refers to register ANA_RGC3 related to regmap
- * @interface:         Currently configured interface mode
- * @pcs:               Phylink PCS structure
- * @flags:             Flags indicating hardware properties
- */
-struct mtk_pcs {
-	struct regmap	*regmap;
-	u32             ana_rgc3;
-	phy_interface_t	interface;
-	struct phylink_pcs pcs;
-	u32		flags;
-};
-
 /* struct mtk_sgmii -  This is the structure holding sgmii regmap and its
  *                     characteristics
  * @pcs                Array of individual PCS structures
  */
 struct mtk_sgmii {
-	struct mtk_pcs	pcs[MTK_MAX_DEVS];
+	struct phylink_pcs *pcs[MTK_MAX_DEVS];
+	struct device *dev;
 };
 
 /* struct mtk_eth -	This is the main datasructure for holding the state

@@ -1353,6 +1278,7 @@ u32 mtk_r32(struct mtk_eth *eth, unsigned reg);
 struct phylink_pcs *mtk_sgmii_select_pcs(struct mtk_sgmii *ss, int id);
 int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *np,
 		   u32 ana_rgc3);
+void mtk_sgmii_destroy(struct mtk_sgmii *ss);
 
 int mtk_gmac_sgmii_path_setup(struct mtk_eth *eth, int mac_id);
 int mtk_gmac_gephy_path_setup(struct mtk_eth *eth, int mac_id);

drivers/net/ethernet/mediatek/mtk_sgmii.c +1 -164

index 9c58006d1c33..d4b428e23cfc 100644
@@ -10,199 +10,35 @@
 #include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/phylink.h>
+#include <linux/pcs/pcs-mtk-lynxi.h>
 #include <linux/regmap.h>
 
 #include "mtk_eth_soc.h"
 
-static struct mtk_pcs *pcs_to_mtk_pcs(struct phylink_pcs *pcs)
-{
-	return container_of(pcs, struct mtk_pcs, pcs);
-}
-
-static void mtk_pcs_get_state(struct phylink_pcs *pcs,
-			      struct phylink_link_state *state)
-{
-	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
-	unsigned int bm, adv;
-
-	/* Read the BMSR and LPA */
-	regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &bm);
-	regmap_read(mpcs->regmap, SGMSYS_PCS_ADVERTISE, &adv);
-
-	phylink_mii_c22_pcs_decode_state(state, FIELD_GET(SGMII_BMSR, bm),
-					 FIELD_GET(SGMII_LPA, adv));
-}
-
-static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
-			  phy_interface_t interface,
-			  const unsigned long *advertising,
-			  bool permit_pause_to_mac)
-{
-	bool mode_changed = false, changed, use_an;
-	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
-	unsigned int rgc3, sgm_mode, bmcr;
-	int advertise, link_timer;
-
-	advertise = phylink_mii_c22_pcs_encode_advertisement(interface,
-							     advertising);
-	if (advertise < 0)
-		return advertise;
-
-	/* Clearing IF_MODE_BIT0 switches the PCS to BASE-X mode, and
-	 * we assume that fixes it's speed at bitrate = line rate (in
-	 * other words, 1000Mbps or 2500Mbps).
-	 */
-	if (interface == PHY_INTERFACE_MODE_SGMII) {
-		sgm_mode = SGMII_IF_MODE_SGMII;
-		if (phylink_autoneg_inband(mode)) {
-			sgm_mode |= SGMII_REMOTE_FAULT_DIS |
-				    SGMII_SPEED_DUPLEX_AN;
-			use_an = true;
-		} else {
-			use_an = false;
-		}
-	} else if (phylink_autoneg_inband(mode)) {
-		/* 1000base-X or 2500base-X autoneg */
-		sgm_mode = SGMII_REMOTE_FAULT_DIS;
-		use_an = linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
-					   advertising);
-	} else {
-		/* 1000base-X or 2500base-X without autoneg */
-		sgm_mode = 0;
-		use_an = false;
-	}
-
-	if (use_an) {
-		bmcr = SGMII_AN_ENABLE;
-	} else {
-		bmcr = 0;
-	}
-
-	if (mpcs->interface != interface) {
-		/* PHYA power down */
-		regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
-				   SGMII_PHYA_PWD, SGMII_PHYA_PWD);
-
-		/* Reset SGMII PCS state */
-		regmap_update_bits(mpcs->regmap, SGMII_RESERVED_0,
-				   SGMII_SW_RESET, SGMII_SW_RESET);
-
-		if (mpcs->flags & MTK_SGMII_FLAG_PN_SWAP)
-			regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_WRAP_CTRL,
-					   SGMII_PN_SWAP_MASK,
-					   SGMII_PN_SWAP_TX_RX);
-
-		if (interface == PHY_INTERFACE_MODE_2500BASEX)
-			rgc3 = RG_PHY_SPEED_3_125G;
-		else
-			rgc3 = 0;
-
-		/* Configure the underlying interface speed */
-		regmap_update_bits(mpcs->regmap, mpcs->ana_rgc3,
-				   RG_PHY_SPEED_3_125G, rgc3);
-
-		/* Setup the link timer and QPHY power up inside SGMIISYS */
-		link_timer = phylink_get_link_timer_ns(interface);
-		if (link_timer < 0)
-			return link_timer;
-
-		regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, link_timer / 2 / 8);
-
-		mpcs->interface = interface;
-		mode_changed = true;
-	}
-
-	/* Update the advertisement, noting whether it has changed */
-	regmap_update_bits_check(mpcs->regmap, SGMSYS_PCS_ADVERTISE,
-				 SGMII_ADVERTISE, advertise, &changed);
-
-	/* Update the sgmsys mode register */
-	regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
-			   SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN |
-			   SGMII_IF_MODE_SGMII, sgm_mode);
-
-	/* Update the BMCR */
-	regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
-			   SGMII_AN_ENABLE, bmcr);
-
-	/* Release PHYA power down state
-	 * Only removing bit SGMII_PHYA_PWD isn't enough.
-	 * There are cases when the SGMII_PHYA_PWD register contains 0x9 which
-	 * prevents SGMII from working. The SGMII still shows link but no traffic
-	 * can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was
-	 * taken from a good working state of the SGMII interface.
-	 * Unknown how much the QPHY needs but it is racy without a sleep.
-	 * Tested on mt7622 & mt7986.
-	 */
-	usleep_range(50, 100);
-	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
-
-	return changed || mode_changed;
-}
-
-static void mtk_pcs_restart_an(struct phylink_pcs *pcs)
-{
-	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
-
-	regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
-			   SGMII_AN_RESTART, SGMII_AN_RESTART);
-}
-
-static void mtk_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
-			    phy_interface_t interface, int speed, int duplex)
-{
-	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
-	unsigned int sgm_mode;
-
-	if (!phylink_autoneg_inband(mode)) {
-		/* Force the speed and duplex setting */
-		if (speed == SPEED_10)
-			sgm_mode = SGMII_SPEED_10;
-		else if (speed == SPEED_100)
-			sgm_mode = SGMII_SPEED_100;
-		else
-			sgm_mode = SGMII_SPEED_1000;
-
-		if (duplex != DUPLEX_FULL)
-			sgm_mode |= SGMII_DUPLEX_HALF;
-
-		regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
-				   SGMII_DUPLEX_HALF | SGMII_SPEED_MASK,
-				   sgm_mode);
-	}
-}
-
-static const struct phylink_pcs_ops mtk_pcs_ops = {
-	.pcs_get_state = mtk_pcs_get_state,
-	.pcs_config = mtk_pcs_config,
-	.pcs_an_restart = mtk_pcs_restart_an,
-	.pcs_link_up = mtk_pcs_link_up,
-};

drivers/net/ethernet/mediatek/mtk_sgmii.c +19 -13

 		np = of_parse_phandle(r, "mediatek,sgmiisys", i);
 		if (!np)
 			break;
 
-		ss->pcs[i].ana_rgc3 = ana_rgc3;
-		ss->pcs[i].regmap = syscon_node_to_regmap(np);
-
-		ss->pcs[i].flags = 0;
+		regmap = syscon_node_to_regmap(np);
+		flags = 0;
 		if (of_property_read_bool(np, "mediatek,pn_swap"))
-			ss->pcs[i].flags |= MTK_SGMII_FLAG_PN_SWAP;
+			flags |= MTK_SGMII_FLAG_PN_SWAP;
 
 		of_node_put(np);
-		if (IS_ERR(ss->pcs[i].regmap))
-			return PTR_ERR(ss->pcs[i].regmap);
 
-		ss->pcs[i].pcs.ops = &mtk_pcs_ops;
-		ss->pcs[i].pcs.poll = true;
-		ss->pcs[i].interface = PHY_INTERFACE_MODE_NA;
+		if (IS_ERR(regmap))
+			return PTR_ERR(regmap);
+
+		ss->pcs[i] = mtk_pcs_lynxi_create(ss->dev, regmap, ana_rgc3,
+						  flags);
 	}
 
 	return 0;

@@ -210,8 +46,16 @@ int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)
 
 struct phylink_pcs *mtk_sgmii_select_pcs(struct mtk_sgmii *ss, int id)
 {
-	if (!ss->pcs[id].regmap)
-		return NULL;
+	return ss->pcs[id];
+}
+
+void mtk_sgmii_destroy(struct mtk_sgmii *ss)
+{
+	int i;
+
+	if (!ss)
+		return;
 
-	return &ss->pcs[id].pcs;
+	for (i = 0; i < MTK_MAX_DEVS; i++)
+		mtk_pcs_lynxi_destroy(ss->pcs[i]);
 }

Signature · 1 line

2.39.1

drivers/net/dsa/mt7530.c mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port, · line 2615

Quoted 5 lines · Daniel Golle

drivers/net/dsa/mt7530.c +1 -0

@@ -2728,11 +2612,11 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,
 
 	switch (interface) {
 	case PHY_INTERFACE_MODE_TRGMII:
+		return &priv->pcs[port].pcs;

drivers/net/dsa/mt7530.c mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port, · line 2619

Quoted 6 lines

drivers/net/dsa/mt7530.c +1 -2

 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_1000BASEX:
 	case PHY_INTERFACE_MODE_2500BASEX:
-		return &priv->pcs[port].pcs;
-
+		return priv->ports[port].sgmii_pcs;

My only concern here is that we also use the PCS when in TRGMII mode in this driver, but the mtk pcs code from mtk_eth_soc doesn't handle TRGMII (and getting the link timer will fail for this mode, causing the pcs_config() method to fail.)

Thus, this driver will stop working in TRGMII mode.

drivers/net/dsa/mt7530.c mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port, · line 2619

Quoted 11 lines · Daniel Golle

drivers/net/dsa/mt7530.c +2 -2

@@ -2728,11 +2612,11 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,
 
 	switch (interface) {
 	case PHY_INTERFACE_MODE_TRGMII:
+		return &priv->pcs[port].pcs;
 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_1000BASEX:
 	case PHY_INTERFACE_MODE_2500BASEX:
-		return &priv->pcs[port].pcs;
-
+		return priv->ports[port].sgmii_pcs;

My only concern here is that we also use the PCS when in TRGMII mode in this driver, but the mtk pcs code from mtk_eth_soc doesn't handle TRGMII (and getting the link timer will fail for this mode, causing the pcs_config() method to fail.)

Thus, this driver will stop working in TRGMII mode.