[RFC PATCH net-next v3 0/8] Add support for 10G Ethernet SerDes on MT7988

[RFC PATCH net-next v3 0/8] Add support for 10G Ethernet SerDes on MT7988

[Daniel Golle]

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

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

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

Note that this series depends on series "dt-bindings: clock: mediatek:
add MT7988 clock IDs"[1] as well as "dt-bindings: watchdog:
mediatek,mtk-wdt: add MT7988 watchdog and toprgu"[2] being merged
before.

[1]: https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=809031
[2]: https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=802588

Changes since RFC v2:
 - use clk_bulk_* when ever feasible
 - rework Ethernet <-> PCS driver link, use device_link
 
Changes since RFC v1:
 - drop patch inhibiting SGMII AN in 2500Base-X mode
 - make pcs-mtk-lynxi a proper platform driver
 - ... hence allowing to remove all the wrappers from the usxgmii driver
 - attach PEXTP to MAC instead of to USXGMII PCS

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

 .../devicetree/bindings/net/mediatek,net.yaml | 180 +++++--
 .../bindings/net/pcs/mediatek,usxgmii.yaml    |  60 +++
 .../bindings/phy/mediatek,xfi-pextp.yaml      |  80 +++
 MAINTAINERS                                   |   3 +
 drivers/net/ethernet/mediatek/mtk_eth_path.c  | 122 ++++-
 drivers/net/ethernet/mediatek/mtk_eth_soc.c   | 291 +++++++++--
 drivers/net/ethernet/mediatek/mtk_eth_soc.h   | 107 +++-
 drivers/net/pcs/Kconfig                       |  11 +
 drivers/net/pcs/Makefile                      |   1 +
 drivers/net/pcs/pcs-mtk-lynxi.c               | 226 ++++++++-
 drivers/net/pcs/pcs-mtk-usxgmii.c             | 456 ++++++++++++++++++
 drivers/phy/mediatek/Kconfig                  |  11 +
 drivers/phy/mediatek/Makefile                 |   1 +
 drivers/phy/mediatek/phy-mtk-pextp.c          | 361 ++++++++++++++
 include/linux/pcs/pcs-mtk-lynxi.h             |  11 +
 include/linux/pcs/pcs-mtk-usxgmii.h           |  27 ++
 16 files changed, 1859 insertions(+), 89 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml
 create mode 100644 Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml
 create mode 100644 drivers/net/pcs/pcs-mtk-usxgmii.c
 create mode 100644 drivers/phy/mediatek/phy-mtk-pextp.c
 create mode 100644 include/linux/pcs/pcs-mtk-usxgmii.h

-- 
2.43.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH net-next v3 1/8] dt-bindings: phy: mediatek,xfi-pextp: add new bindings

[Daniel Golle]

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

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

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 .../bindings/phy/mediatek,xfi-pextp.yaml      | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

On Tue, 12 Dec 2023 03:46:26 +0000, Daniel Golle wrote:
> 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 <daniel@makrotopia.org>
> ---
>  .../bindings/phy/mediatek,xfi-pextp.yaml      | 80 +++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml
> 

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

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

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.example.dts:18:18: fatal error: dt-bindings/clock/mediatek,mt7988-clk.h: No such file or directory
   18 |         #include <dt-bindings/clock/mediatek,mt7988-clk.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1424: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

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

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

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

pip3 install dtschema --upgrade

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 443 bytes --]

On Tue, Dec 12, 2023 at 03:46:26AM +0000, Daniel Golle wrote:

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Daniel Golle]

On Tue, Dec 12, 2023 at 04:21:38PM +0000, Conor Dooley wrote:
> On Tue, Dec 12, 2023 at 03:46:26AM +0000, Daniel Golle wrote:
> 
> > +  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.


[1]: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/a500d94cd47e279015ce22947e1ce396a7516598%5E%21/#F0

[2]: https://patchwork.kernel.org/comment/25592845/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1311 bytes --]

On Tue, Dec 12, 2023 at 04:42:45PM +0000, Daniel Golle wrote:
> On Tue, Dec 12, 2023 at 04:21:38PM +0000, Conor Dooley wrote:
> > On Tue, Dec 12, 2023 at 03:46:26AM +0000, Daniel Golle wrote:
> > 
> > > +  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.

Please put it in the commit message so that when the next version shows
up, Krzysztof doesn't show up and question the property for the third
time.

Also, on another note, this series is aimed at net-next but half the
series is fixed for incorrect bindings. Why not net?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andrew Lunn]

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

     Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Daniel Golle]

On Wed, Dec 13, 2023 at 02:20:45PM +0100, Andrew Lunn wrote:
> > 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?

As far as I know we can't infer the channel number by any of the
registers default values.

We could infer it from the base address, and that would kinda defy the
purpose of having Device Tree to begin with in my feeling at least,
but it would be possible, of course.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH net-next v3 2/8] phy: add driver for MediaTek pextp 10GE SerDes PHY

[Daniel Golle]

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

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

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

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

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

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 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

diff --git a/MAINTAINERS b/MAINTAINERS
index 98f7dd0499f17..b2772cfe2a704 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13510,6 +13510,7 @@ L:	netdev@vger.kernel.org
 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 <qii.wang@mediatek.com>
diff --git a/drivers/phy/mediatek/Kconfig b/drivers/phy/mediatek/Kconfig
index 3125ecb5d119f..a7749a6d96541 100644
--- a/drivers/phy/mediatek/Kconfig
+++ b/drivers/phy/mediatek/Kconfig
@@ -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
diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile
index c9a50395533eb..ca60c7b9b02ac 100644
--- a/drivers/phy/mediatek/Makefile
+++ b/drivers/phy/mediatek/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PHY_MTK_PCIE)		+= phy-mtk-pcie.o
 obj-$(CONFIG_PHY_MTK_TPHY)		+= phy-mtk-tphy.o
 obj-$(CONFIG_PHY_MTK_UFS)		+= phy-mtk-ufs.o
 obj-$(CONFIG_PHY_MTK_XSPHY)		+= phy-mtk-xsphy.o
+obj-$(CONFIG_PHY_MTK_PEXTP)		+= phy-mtk-pextp.o
 
 phy-mtk-hdmi-drv-y			:= phy-mtk-hdmi.o
 phy-mtk-hdmi-drv-y			+= phy-mtk-hdmi-mt2701.o
diff --git a/drivers/phy/mediatek/phy-mtk-pextp.c b/drivers/phy/mediatek/phy-mtk-pextp.c
new file mode 100644
index 0000000000000..3ec48cf129105
--- /dev/null
+++ b/drivers/phy/mediatek/phy-mtk-pextp.c
@@ -0,0 +1,361 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* MediaTek 10GE SerDes PHY driver
+ *
+ * Copyright (c) 2023 Daniel Golle <daniel@makrotopia.org>
+ * based on mtk_usxgmii.c found in MediaTek's SDK released under GPL-2.0
+ * Copyright (c) 2022 MediaTek Inc.
+ * Author: Henry Yen <henry.yen@mediatek.com>
+ */
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/netdevice.h>
+#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);
+	}
+
+	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 <daniel@makrotopia.org>");
+MODULE_LICENSE("GPL");
-- 
2.43.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH net-next v3 2/8] phy: add driver for MediaTek pextp 10GE SerDes PHY

[AngeloGioacchino Del Regno]

Il 12/12/23 04:46, Daniel Golle ha scritto:
> 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 <daniel@makrotopia.org>
> ---

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

> +
> +	/* 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 <daniel@makrotopia.org>");
> +MODULE_LICENSE("GPL");



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH net-next v3 2/8] phy: add driver for MediaTek pextp 10GE SerDes PHY

[Chunfeng Yun (云春峰)]

On Tue, 2023-12-12 at 11:41 +0100, AngeloGioacchino Del Regno wrote:
> Il 12/12/23 04:46, Daniel Golle ha scritto:
> > 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 <daniel@makrotopia.org>
> > ---
> 
> ..snip..
> 
> Can anyone from MediaTek **please** define those registers and
> fields?
I discussed with my software coworker who also don't have register map
doc, need our hardware designer help to provide the names of these
register and fields.

Thanks
> 
> 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
> 
> > +
> > +	/* 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 <daniel@makrotopia.org>");
> > +MODULE_LICENSE("GPL");
> 
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH net-next v3 2/8] phy: add driver for MediaTek pextp 10GE SerDes PHY

[Vinod Koul]

On 12-12-23, 03:46, Daniel Golle wrote:
> Add driver for MediaTek's pextp 10 Gigabit/s Ethernet SerDes PHY which
> can be found in the MT7988 SoC.
> 
> The PHY can operates only in PHY_MODE_ETHERNET, the submode is one of
> PHY_INTERFACE_MODE_* corresponding to the supported modes:
> 
>  * USXGMII
>  * 10GBase-R
>  * 5GBase-R
>  * 2500Base-X
>  * 1000Base-X
>  * Cisco SGMII (MAC side)
> 
> In order to work-around a performance issue present on the first of
> two PEXTP present in MT7988 special tuning is applied which can be
> selected by adding the mediatek,usxgmii-performance-errata property to
> the device tree node.
> 
> There is no documentation what-so-ever for the pextp registers and
> this driver is based on a GPL licensed implementation found in
> MediaTek's SDK.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  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
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 98f7dd0499f17..b2772cfe2a704 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13510,6 +13510,7 @@ L:	netdev@vger.kernel.org
>  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 <qii.wang@mediatek.com>
> diff --git a/drivers/phy/mediatek/Kconfig b/drivers/phy/mediatek/Kconfig
> index 3125ecb5d119f..a7749a6d96541 100644
> --- a/drivers/phy/mediatek/Kconfig
> +++ b/drivers/phy/mediatek/Kconfig
> @@ -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
> diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile
> index c9a50395533eb..ca60c7b9b02ac 100644
> --- a/drivers/phy/mediatek/Makefile
> +++ b/drivers/phy/mediatek/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_PHY_MTK_PCIE)		+= phy-mtk-pcie.o
>  obj-$(CONFIG_PHY_MTK_TPHY)		+= phy-mtk-tphy.o
>  obj-$(CONFIG_PHY_MTK_UFS)		+= phy-mtk-ufs.o
>  obj-$(CONFIG_PHY_MTK_XSPHY)		+= phy-mtk-xsphy.o
> +obj-$(CONFIG_PHY_MTK_PEXTP)		+= phy-mtk-pextp.o

alphabetically sorted please

>  
>  phy-mtk-hdmi-drv-y			:= phy-mtk-hdmi.o
>  phy-mtk-hdmi-drv-y			+= phy-mtk-hdmi-mt2701.o
> diff --git a/drivers/phy/mediatek/phy-mtk-pextp.c b/drivers/phy/mediatek/phy-mtk-pextp.c
> new file mode 100644
> index 0000000000000..3ec48cf129105
> --- /dev/null
> +++ b/drivers/phy/mediatek/phy-mtk-pextp.c
> @@ -0,0 +1,361 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* MediaTek 10GE SerDes PHY driver
> + *
> + * Copyright (c) 2023 Daniel Golle <daniel@makrotopia.org>
> + * based on mtk_usxgmii.c found in MediaTek's SDK released under GPL-2.0
> + * Copyright (c) 2022 MediaTek Inc.
> + * Author: Henry Yen <henry.yen@mediatek.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/netdevice.h>

why do you need this header?

> +#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);
> +	}
> +
> +	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);

ouch, why mixed case, make it small everwhere please

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

why is this driver in netdev series, it should not be dependent on rest
and can be send separately to phy ss

> +
> +MODULE_DESCRIPTION("MediaTek pextp SerDes PHY driver");
> +MODULE_AUTHOR("Daniel Golle <daniel@makrotopia.org>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.43.0

-- 
~Vinod

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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 <daniel@makrotopia.org>
---
 drivers/net/pcs/pcs-mtk-lynxi.c   | 226 ++++++++++++++++++++++++++++--
 include/linux/pcs/pcs-mtk-lynxi.h |  11 ++
 2 files changed, 226 insertions(+), 11 deletions(-)

diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c
index 8501dd365279b..e06dd7db66144 100644
--- a/drivers/net/pcs/pcs-mtk-lynxi.c
+++ b/drivers/net/pcs/pcs-mtk-lynxi.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2018-2019 MediaTek Inc.
-/* A library for MediaTek SGMII circuit
+/* A library and platform driver for the MediaTek LynxI SGMII circuit
  *
  * Author: Sean Wang <sean.wang@mediatek.com>
  * Author: Alexander Couzens <lynxis@fe80.eu>
@@ -8,11 +8,17 @@
  *
  */
 
+#include <linux/clk.h>
 #include <linux/mdio.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/pcs/pcs-mtk-lynxi.h>
 #include <linux/phylink.h>
+#include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/reset.h>
 
 /* SGMII subsystem config registers */
 /* BMCR (low 16) BMSR (high 16) */
@@ -65,6 +71,8 @@
 #define SGMII_PN_SWAP_MASK		GENMASK(1, 0)
 #define SGMII_PN_SWAP_TX_RX		(BIT(0) | BIT(1))
 
+#define MTK_NETSYS_V3_AMA_RGC3		0x128
+
 /* struct mtk_pcs_lynxi -  This structure holds each sgmii regmap andassociated
  *                         data
  * @regmap:                The register map pointing at the range used to setup
@@ -74,15 +82,29 @@
  * @interface:             Currently configured interface mode
  * @pcs:                   Phylink PCS structure
  * @flags:                 Flags indicating hardware properties
+ * @rstc:                  Reset controller
+ * @sgmii_sel:             SGMII Register Clock
+ * @sgmii_rx:              SGMII RX Clock
+ * @sgmii_tx:              SGMII TX Clock
+ * @node:                  List node
  */
 struct mtk_pcs_lynxi {
 	struct regmap		*regmap;
+	struct device		*dev;
 	u32			ana_rgc3;
 	phy_interface_t		interface;
 	struct			phylink_pcs pcs;
 	u32			flags;
+	struct reset_control	*rstc;
+	struct clk		*sgmii_sel;
+	struct clk		*sgmii_rx;
+	struct clk		*sgmii_tx;
+	struct list_head	node;
 };
 
+static LIST_HEAD(mtk_pcs_lynxi_instances);
+static DEFINE_MUTEX(instance_mutex);
+
 static struct mtk_pcs_lynxi *pcs_to_mtk_pcs_lynxi(struct phylink_pcs *pcs)
 {
 	return container_of(pcs, struct mtk_pcs_lynxi, pcs);
@@ -102,6 +124,17 @@ static void mtk_pcs_lynxi_get_state(struct phylink_pcs *pcs,
 					 FIELD_GET(SGMII_LPA, adv));
 }
 
+static void mtk_sgmii_reset(struct mtk_pcs_lynxi *mpcs)
+{
+	if (!mpcs->rstc)
+		return;
+
+	reset_control_assert(mpcs->rstc);
+	udelay(100);
+	reset_control_deassert(mpcs->rstc);
+	mdelay(1);
+}
+
 static int mtk_pcs_lynxi_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 				phy_interface_t interface,
 				const unsigned long *advertising,
@@ -147,6 +180,7 @@ static int mtk_pcs_lynxi_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 				SGMII_PHYA_PWD);
 
 		/* Reset SGMII PCS state */
+		mtk_sgmii_reset(mpcs);
 		regmap_set_bits(mpcs->regmap, SGMSYS_RESERVED_0,
 				SGMII_SW_RESET);
 
@@ -233,10 +267,29 @@ static void mtk_pcs_lynxi_link_up(struct phylink_pcs *pcs,
 	}
 }
 
+static int mtk_pcs_lynxi_enable(struct phylink_pcs *pcs)
+{
+	struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
+
+	if (mpcs->sgmii_tx && mpcs->sgmii_rx) {
+		clk_prepare_enable(mpcs->sgmii_rx);
+		clk_prepare_enable(mpcs->sgmii_tx);
+	}
+
+	return 0;
+}
+
 static void mtk_pcs_lynxi_disable(struct phylink_pcs *pcs)
 {
 	struct mtk_pcs_lynxi *mpcs = pcs_to_mtk_pcs_lynxi(pcs);
 
+	regmap_set_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
+
+	if (mpcs->sgmii_tx && mpcs->sgmii_rx) {
+		clk_disable_unprepare(mpcs->sgmii_tx);
+		clk_disable_unprepare(mpcs->sgmii_rx);
+	}
+
 	mpcs->interface = PHY_INTERFACE_MODE_NA;
 }
 
@@ -246,11 +299,12 @@ static const struct phylink_pcs_ops mtk_pcs_lynxi_ops = {
 	.pcs_an_restart = mtk_pcs_lynxi_restart_an,
 	.pcs_link_up = mtk_pcs_lynxi_link_up,
 	.pcs_disable = mtk_pcs_lynxi_disable,
+	.pcs_enable = mtk_pcs_lynxi_enable,
 };
 
-struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
-					 struct regmap *regmap, u32 ana_rgc3,
-					 u32 flags)
+static struct phylink_pcs *mtk_pcs_lynxi_init(struct device *dev, struct regmap *regmap,
+					      u32 ana_rgc3, u32 flags,
+					      struct mtk_pcs_lynxi *prealloc)
 {
 	struct mtk_pcs_lynxi *mpcs;
 	u32 id, ver;
@@ -258,29 +312,33 @@ struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
 
 	ret = regmap_read(regmap, SGMSYS_PCS_DEVICE_ID, &id);
 	if (ret < 0)
-		return NULL;
+		return ERR_PTR(ret);
 
 	if (id != SGMII_LYNXI_DEV_ID) {
 		dev_err(dev, "unknown PCS device id %08x\n", id);
-		return NULL;
+		return ERR_PTR(-ENODEV);
 	}
 
 	ret = regmap_read(regmap, SGMSYS_PCS_SCRATCH, &ver);
 	if (ret < 0)
-		return NULL;
+		return ERR_PTR(ret);
 
 	ver = FIELD_GET(SGMII_DEV_VERSION, ver);
 	if (ver != 0x1) {
 		dev_err(dev, "unknown PCS device version %04x\n", ver);
-		return NULL;
+		return ERR_PTR(-ENODEV);
 	}
 
 	dev_dbg(dev, "MediaTek LynxI SGMII PCS (id 0x%08x, ver 0x%04x)\n", id,
 		ver);
 
-	mpcs = kzalloc(sizeof(*mpcs), GFP_KERNEL);
-	if (!mpcs)
-		return NULL;
+	if (prealloc) {
+		mpcs = prealloc;
+	} else {
+		mpcs = kzalloc(sizeof(*mpcs), GFP_KERNEL);
+		if (!mpcs)
+			return ERR_PTR(-ENOMEM);
+	};
 
 	mpcs->ana_rgc3 = ana_rgc3;
 	mpcs->regmap = regmap;
@@ -291,6 +349,13 @@ struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
 	mpcs->interface = PHY_INTERFACE_MODE_NA;
 
 	return &mpcs->pcs;
+};
+
+struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
+					 struct regmap *regmap, u32 ana_rgc3,
+					 u32 flags)
+{
+	return mtk_pcs_lynxi_init(dev, regmap, ana_rgc3, flags, NULL);
 }
 EXPORT_SYMBOL(mtk_pcs_lynxi_create);
 
@@ -303,4 +368,143 @@ void mtk_pcs_lynxi_destroy(struct phylink_pcs *pcs)
 }
 EXPORT_SYMBOL(mtk_pcs_lynxi_destroy);
 
+static int mtk_pcs_lynxi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct mtk_pcs_lynxi *mpcs;
+	struct phylink_pcs *pcs;
+	struct regmap *regmap;
+	u32 flags = 0;
+
+	mpcs = devm_kzalloc(dev, sizeof(*mpcs), GFP_KERNEL);
+	if (!mpcs)
+		return -ENOMEM;
+
+	mpcs->dev = dev;
+	regmap = syscon_node_to_regmap(np->parent);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	if (of_property_read_bool(np->parent, "mediatek,pnswap"))
+		flags |= MTK_SGMII_FLAG_PN_SWAP;
+
+	mpcs->rstc = of_reset_control_get_shared(np->parent, NULL);
+	if (IS_ERR(mpcs->rstc))
+		return PTR_ERR(mpcs->rstc);
+
+	reset_control_deassert(mpcs->rstc);
+	mpcs->sgmii_sel = devm_clk_get_enabled(dev, "sgmii_sel");
+	if (IS_ERR(mpcs->sgmii_sel))
+		return PTR_ERR(mpcs->sgmii_sel);
+
+	mpcs->sgmii_rx = devm_clk_get(dev, "sgmii_rx");
+	if (IS_ERR(mpcs->sgmii_rx))
+		return PTR_ERR(mpcs->sgmii_rx);
+
+	mpcs->sgmii_tx = devm_clk_get(dev, "sgmii_tx");
+	if (IS_ERR(mpcs->sgmii_tx))
+		return PTR_ERR(mpcs->sgmii_tx);
+
+	pcs = mtk_pcs_lynxi_init(dev, regmap, (uintptr_t)of_device_get_match_data(dev),
+				 flags, mpcs);
+	if (IS_ERR(pcs))
+		return PTR_ERR(pcs);
+
+	regmap_set_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
+
+	platform_set_drvdata(pdev, mpcs);
+
+	mutex_lock(&instance_mutex);
+	list_add_tail(&mpcs->node, &mtk_pcs_lynxi_instances);
+	mutex_unlock(&instance_mutex);
+
+	return 0;
+}
+
+static int mtk_pcs_lynxi_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mtk_pcs_lynxi *cur, *tmp;
+
+	mutex_lock(&instance_mutex);
+	list_for_each_entry_safe(cur, tmp, &mtk_pcs_lynxi_instances, node)
+		if (cur->dev == dev) {
+			list_del(&cur->node);
+			kfree(cur);
+			break;
+		}
+	mutex_unlock(&instance_mutex);
+
+	return 0;
+}
+
+static const struct of_device_id mtk_pcs_lynxi_of_match[] = {
+	{ .compatible = "mediatek,mt7988-sgmii", .data = (void *)MTK_NETSYS_V3_AMA_RGC3 },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mtk_pcs_lynxi_of_match);
+
+struct phylink_pcs *mtk_pcs_lynxi_get(struct device *dev, struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct mtk_pcs_lynxi *mpcs;
+
+	if (!np)
+		return NULL;
+
+	if (!of_device_is_available(np))
+		return ERR_PTR(-ENODEV);
+
+	if (!of_match_node(mtk_pcs_lynxi_of_match, np))
+		return ERR_PTR(-EINVAL);
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev || !platform_get_drvdata(pdev)) {
+		if (pdev)
+			put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	mpcs = platform_get_drvdata(pdev);
+	device_link_add(dev, mpcs->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
+
+	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;
+
+	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);
+
+	if (WARN_ON(!mpcs))
+		return;
+
+	put_device(mpcs->dev);
+}
+EXPORT_SYMBOL(mtk_pcs_lynxi_put);
+
+static struct platform_driver mtk_pcs_lynxi_driver = {
+	.driver = {
+		.name = "mtk-pcs-lynxi",
+		.of_match_table = mtk_pcs_lynxi_of_match,
+	},
+	.probe = mtk_pcs_lynxi_probe,
+	.remove = mtk_pcs_lynxi_remove,
+};
+module_platform_driver(mtk_pcs_lynxi_driver);
+
+MODULE_AUTHOR("Daniel Golle <daniel@makrotopia.org>");
+MODULE_DESCRIPTION("MediaTek LynxI HSGMII PCS");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/pcs/pcs-mtk-lynxi.h b/include/linux/pcs/pcs-mtk-lynxi.h
index be3b4ab32f4a7..2d44e951229c7 100644
--- a/include/linux/pcs/pcs-mtk-lynxi.h
+++ b/include/linux/pcs/pcs-mtk-lynxi.h
@@ -10,4 +10,15 @@ struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
 					 struct regmap *regmap,
 					 u32 ana_rgc3, u32 flags);
 void mtk_pcs_lynxi_destroy(struct phylink_pcs *pcs);
+
+#if IS_ENABLED(CONFIG_PCS_MTK_LYNXI)
+struct phylink_pcs *mtk_pcs_lynxi_get(struct device *dev, struct device_node *np);
+void mtk_pcs_lynxi_put(struct phylink_pcs *pcs);
+#else
+static inline struct phylink_pcs *mtk_pcs_lynxi_get(struct device *dev, struct device_node *np)
+{
+	return NULL;
+}
+static inline void mtk_pcs_lynxi_put(struct phylink_pcs *pcs) { }
+#endif /* IS_ENABLED(CONFIG_PCS_MTK_LYNXI) */
 #endif
-- 
2.43.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

On Tue, Dec 12, 2023 at 03:47:18AM +0000, Daniel Golle wrote:
> 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 <daniel@makrotopia.org>

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.

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

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

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

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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:

On Wed, Dec 13, 2023 at 04:04:12PM +0000, Russell King (Oracle) wrote:
> On Tue, Dec 12, 2023 at 03:47:18AM +0000, Daniel Golle wrote:
> > 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 <daniel@makrotopia.org>
> 
> 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.
> 
> > +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)
> > +		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...
> 
> > +		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...
> 
> > +	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.

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.

> 
> 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:
diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
index b0dd133665986..9517c96319595 100644
--- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
+++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
@@ -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/...


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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Daniel Golle]

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:

On Wed, Feb 07, 2024 at 01:29:18AM +0000, Daniel Golle wrote:
> 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:
> 
> On Wed, Dec 13, 2023 at 04:04:12PM +0000, Russell King (Oracle) wrote:
> > On Tue, Dec 12, 2023 at 03:47:18AM +0000, Daniel Golle wrote:
> > > 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 <daniel@makrotopia.org>
> > 
> > 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.
> > 
> > > +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)
> > > +		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...
> > 
> > > +		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...
> > 
> > > +	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.
> 
> 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.
> 
> > 
> > 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:
> diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> index b0dd133665986..9517c96319595 100644
> --- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> +++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> @@ -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/...
> 

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.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Frank Wunderlich]

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:
> 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:
>
> On Wed, Feb 07, 2024 at 01:29:18AM +0000, Daniel Golle wrote:
>> 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:
>>
>> On Wed, Dec 13, 2023 at 04:04:12PM +0000, Russell King (Oracle) wrote:
>>> On Tue, Dec 12, 2023 at 03:47:18AM +0000, Daniel Golle wrote:
>>>> 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 <daniel@makrotopia.org>
>>> 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.
>>>
>>>> +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)
>>>> +		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...
>>>
>>>> +		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...
>>>
>>>> +	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.
>> 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.
>>
>>> 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:
>> diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
>> index b0dd133665986..9517c96319595 100644
>> --- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
>> +++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
>> @@ -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/...
>>
> 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.
>
>>>> +
>>>> +	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;
>>>> +
>>>> +	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.
>> 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
>>

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

[Frank Wunderlich]

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

> Gesendet: Mittwoch, 10. Juli 2024 um 13:17 Uhr
> Von: "Frank Wunderlich" <frank-w@public-files.de>
> An: "Russell King (Oracle)" <linux@armlinux.org.uk>
> Cc: "David S. Miller" <davem@davemloft.net>, "Eric Dumazet" <edumazet@google.com>, "Jakub Kicinski" <kuba@kernel.org>, "Paolo Abeni" <pabeni@redhat.com>, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Conor Dooley" <conor+dt@kernel.org>, "Chunfeng Yun" <chunfeng.yun@mediatek.com>, "Vinod Koul" <vkoul@kernel.org>, "Kishon Vijay Abraham I" <kishon@kernel.org>, "Felix Fietkau" <nbd@nbd.name>, "John Crispin" <john@phrozen.org>, "Sean Wang" <sean.wang@mediatek.com>, "Mark Lee" <Mark-MC.Lee@mediatek.com>, "Lorenzo Bianconi" <lorenzo@kernel.org>, "Matthias Brugger" <matthias.bgg@gmail.com>, "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>, "Andrew Lunn" <andrew@lunn.ch>, "Heiner Kallweit" <hkallweit1@gmail.com>, "Alexander Couzens" <lynxis@fe80.eu>, "Qingfang Deng" <dqfext@gmail.com>, "SkyLake Huang" <SkyLake.Huang@mediatek.com>, "Philipp Zabel" <p.zabel@pengutronix.de>, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-phy@lists.infradead.org, "Daniel Golle" <daniel@makrotopia.org>
> 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:
> > 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:
> >
> > On Wed, Feb 07, 2024 at 01:29:18AM +0000, Daniel Golle wrote:
> >> 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:
> >>
> >> On Wed, Dec 13, 2023 at 04:04:12PM +0000, Russell King (Oracle) wrote:
> >>> On Tue, Dec 12, 2023 at 03:47:18AM +0000, Daniel Golle wrote:
> >>>> 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 <daniel@makrotopia.org>
> >>> 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.
> >>>
> >>>> +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)
> >>>> +		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...
> >>>
> >>>> +		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...
> >>>
> >>>> +	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.
> >> 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.
> >>
> >>> 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:
> >> diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> >> index b0dd133665986..9517c96319595 100644
> >> --- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> >> +++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> >> @@ -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/...
> >>
> > 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.
> >
> >>>> +
> >>>> +	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;
> >>>> +
> >>>> +	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.
> >> 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
> >>
>

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

[Russell King (Oracle)]

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.

On Fri, Oct 04, 2024 at 04:17:37PM +0200, Frank Wunderlich wrote:
> 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
> 
> > Gesendet: Mittwoch, 10. Juli 2024 um 13:17 Uhr
> > Von: "Frank Wunderlich" <frank-w@public-files.de>
> > An: "Russell King (Oracle)" <linux@armlinux.org.uk>
> > Cc: "David S. Miller" <davem@davemloft.net>, "Eric Dumazet" <edumazet@google.com>, "Jakub Kicinski" <kuba@kernel.org>, "Paolo Abeni" <pabeni@redhat.com>, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Conor Dooley" <conor+dt@kernel.org>, "Chunfeng Yun" <chunfeng.yun@mediatek.com>, "Vinod Koul" <vkoul@kernel.org>, "Kishon Vijay Abraham I" <kishon@kernel.org>, "Felix Fietkau" <nbd@nbd.name>, "John Crispin" <john@phrozen.org>, "Sean Wang" <sean.wang@mediatek.com>, "Mark Lee" <Mark-MC.Lee@mediatek.com>, "Lorenzo Bianconi" <lorenzo@kernel.org>, "Matthias Brugger" <matthias.bgg@gmail.com>, "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>, "Andrew Lunn" <andrew@lunn.ch>, "Heiner Kallweit" <hkallweit1@gmail.com>, "Alexander Couzens" <lynxis@fe80.eu>, "Qingfang Deng" <dqfext@gmail.com>, "SkyLake Huang" <SkyLake.Huang@mediatek.com>, "Philipp Zabel" <p.zabel@pengutronix.de>, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-phy@lists.infradead.org, "Daniel Golle" <daniel@makrotopia.org>
> > 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:
> > > 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:
> > >
> > > On Wed, Feb 07, 2024 at 01:29:18AM +0000, Daniel Golle wrote:
> > >> 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:
> > >>
> > >> On Wed, Dec 13, 2023 at 04:04:12PM +0000, Russell King (Oracle) wrote:
> > >>> On Tue, Dec 12, 2023 at 03:47:18AM +0000, Daniel Golle wrote:
> > >>>> 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 <daniel@makrotopia.org>
> > >>> 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.
> > >>>
> > >>>> +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)
> > >>>> +		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...
> > >>>
> > >>>> +		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...
> > >>>
> > >>>> +	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.
> > >> 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.
> > >>
> > >>> 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:
> > >> diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> > >> index b0dd133665986..9517c96319595 100644
> > >> --- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> > >> +++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> > >> @@ -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/...
> > >>
> > > 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.
> > >
> > >>>> +
> > >>>> +	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;
> > >>>> +
> > >>>> +	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.
> > >> 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
> > >>
> >
> 

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

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

[Frank Wunderlich]

Hi Russel,

> Gesendet: Freitag, 4. Oktober 2024 um 16:35
> Von: "Russell King (Oracle)" <linux@armlinux.org.uk>
> 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.

How are you? I hope you're feeling better now....
I see you were more active on the Mailinglist in last time :)

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

Have you found some time (and were you able) to look a bit into this ([1])?

How would be the right way here? Daniel posted generic infrastructure for standalone
pcs drivers [2] similar to phy to have a generic base which all drivers can use and
which can be extended if needed.

There was some discussion about how pcs should handle if the underlaying device is removed...
unlikely on SoC, but possible on external bus devices like mdio or pcie. Imho this could be
a callback in common code handled by vendor driver (query information from mac driver or for
SoC simply return fixed value).

How should the subsystems talk with each other (callbacks, shared memory, ...)?

DT maintainers want to avoid syscon compatibles which are widely used for nodes representing only
a register range that is used from different subsystems. As we are currently upstreaming mt7988
(e.g. ethernet), there are some of these "devices" which currently have no own driver and only
handled by syscon driver to get the regmap. Should we really write different drivers with additional
compatibles (duplicate code!) only to handle exchange of the regmap between subsystems? For mt7988
which can have up to 7 syscon devices it is maybe better to have substructures like phy, pcs and so on
packed into an own driver to have a better overview in devicetree. But for some functional blocks
imho it makes not much sense (when really only a regmap has to be exchanged between different devices).

Daniel needs an answer to his questions in [1] (and [2]) on how to proceed. So please help to get the
pcs-part into the right shape.

regards Frank

[1] https://patchwork.kernel.org/project/netdevbpf/patch/8aa905080bdb6760875d62cb3b2b41258837f80e.1702352117.git.daniel@makrotopia.org/
[2] https://patchwork.kernel.org/project/netdevbpf/patch/ba4e359584a6b3bc4b3470822c42186d5b0856f9.1721910728.git.daniel@makrotopia.org/

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

[Daniel Golle]

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

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 .../bindings/net/pcs/mediatek,usxgmii.yaml    | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml

diff --git a/Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml b/Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml
new file mode 100644
index 0000000000000..0cdaa3545edb0
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml
@@ -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 <daniel@makrotopia.org>
+
+description:
+  The MediaTek USXGMII PCS provides physical link control and status
+  for USXGMII, 10GBase-R and 5GBase-R links on the SerDes interfaces
+  provided by the PEXTP PHY.
+  In order to also support legacy 2500Base-X, 1000Base-X and Cisco
+  SGMII an existing mediatek,*-sgmiisys LynxI PCS is wrapped to
+  provide those interfaces modes on the same SerDes interfaces shared
+  with the USXGMII PCS.
+
+properties:
+  $nodename:
+    pattern: "^pcs@[0-9a-f]+$"
+
+  compatible:
+    const: mediatek,mt7988-usxgmiisys
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: USXGMII top-level clock
+
+  resets:
+    items:
+      - description: XFI reset
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/mediatek,mt7988-clk.h>
+    #define MT7988_TOPRGU_XFI0_GRST 12
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+        usxgmiisys0: pcs@10080000 {
+          compatible = "mediatek,mt7988-usxgmiisys";
+          reg = <0 0x10080000 0 0x1000>;
+          clocks = <&topckgen CLK_TOP_USXGMII_SBUS_0_SEL>;
+          resets = <&watchdog MT7988_TOPRGU_XFI0_GRST>;
+        };
+    };
-- 
2.43.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

On Tue, 12 Dec 2023 03:47:31 +0000, Daniel Golle wrote:
> 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 <daniel@makrotopia.org>
> ---
>  .../bindings/net/pcs/mediatek,usxgmii.yaml    | 60 +++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:


doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/510af8550385da947e2e2516629c4fbed7fc0f64.1702352117.git.daniel@makrotopia.org

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

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

pip3 install dtschema --upgrade

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2874 bytes --]

On Tue, Dec 12, 2023 at 03:47:31AM +0000, Daniel Golle wrote:
> 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 <daniel@makrotopia.org>
> ---
>  .../bindings/net/pcs/mediatek,usxgmii.yaml    | 60 +++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml b/Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml
> new file mode 100644
> index 0000000000000..0cdaa3545edb0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml
> @@ -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 <daniel@makrotopia.org>
> +
> +description:
> +  The MediaTek USXGMII PCS provides physical link control and status
> +  for USXGMII, 10GBase-R and 5GBase-R links on the SerDes interfaces
> +  provided by the PEXTP PHY.
> +  In order to also support legacy 2500Base-X, 1000Base-X and Cisco
> +  SGMII an existing mediatek,*-sgmiisys LynxI PCS is wrapped to
> +  provide those interfaces modes on the same SerDes interfaces shared
> +  with the USXGMII PCS.
> +
> +properties:
> +  $nodename:
> +    pattern: "^pcs@[0-9a-f]+$"
> +
> +  compatible:
> +    const: mediatek,mt7988-usxgmiisys
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: USXGMII top-level clock
> +
> +  resets:
> +    items:
> +      - description: XFI reset

For this two, why not just "maxItems: 1", since there are only one of
each?

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - resets
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/mediatek,mt7988-clk.h>

> +    #define MT7988_TOPRGU_XFI0_GRST 12

Why? You can just put the raw numbers here and avoid the issues with the
bot being unable to test your series.

> +    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>;
> +        };
> +    };
> -- 
> 2.43.0

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH net-next v3 5/8] net: pcs: add driver for MediaTek USXGMII PCS

[Daniel Golle]

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 <daniel@makrotopia.org>
---
 MAINTAINERS                         |   2 +
 drivers/net/pcs/Kconfig             |  11 +
 drivers/net/pcs/Makefile            |   1 +
 drivers/net/pcs/pcs-mtk-usxgmii.c   | 456 ++++++++++++++++++++++++++++
 include/linux/pcs/pcs-mtk-usxgmii.h |  27 ++
 5 files changed, 497 insertions(+)
 create mode 100644 drivers/net/pcs/pcs-mtk-usxgmii.c
 create mode 100644 include/linux/pcs/pcs-mtk-usxgmii.h

diff --git a/MAINTAINERS b/MAINTAINERS
index b2772cfe2a704..bf798f3b2c0c9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13500,7 +13500,9 @@ M:	Daniel Golle <daniel@makrotopia.org>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/pcs/pcs-mtk-lynxi.c
+F:	drivers/net/pcs/pcs-mtk-usxgmii.c
 F:	include/linux/pcs/pcs-mtk-lynxi.h
+F:	include/linux/pcs/pcs-mtk-usxgmii.h
 
 MEDIATEK ETHERNET PHY DRIVERS
 M:	Daniel Golle <daniel@makrotopia.org>
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 87cf308fc6d8b..55a6865bdaba3 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -25,6 +25,17 @@ config PCS_MTK_LYNXI
 	  This module provides helpers to phylink for managing the LynxI PCS
 	  which is part of MediaTek's SoC and Ethernet switch ICs.
 
+config PCS_MTK_USXGMII
+	tristate "MediaTek USXGMII PCS"
+	select PCS_MTK_LYNXI
+	select PHY_MTK_PEXTP
+	select PHYLINK
+	help
+	  This module provides a driver for MediaTek's USXGMII PCS supporting
+	  10GBase-R, 5GBase-R and USXGMII interface modes.
+	  1000Base-X, 2500Base-X and Cisco SGMII are supported on the same
+	  differential pairs via an embedded LynxI PHY.
+
 config PCS_RZN1_MIIC
 	tristate "Renesas RZ/N1 MII converter"
 	depends on OF && (ARCH_RZN1 || COMPILE_TEST)
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index fb1694192ae63..cc355152ca1ca 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -6,4 +6,5 @@ pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-nxp.o pcs-xpcs-wx.o
 obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
 obj-$(CONFIG_PCS_LYNX)		+= pcs-lynx.o
 obj-$(CONFIG_PCS_MTK_LYNXI)	+= pcs-mtk-lynxi.o
+obj-$(CONFIG_PCS_MTK_USXGMII)	+= pcs-mtk-usxgmii.o
 obj-$(CONFIG_PCS_RZN1_MIIC)	+= pcs-rzn1-miic.o
diff --git a/drivers/net/pcs/pcs-mtk-usxgmii.c b/drivers/net/pcs/pcs-mtk-usxgmii.c
new file mode 100644
index 0000000000000..190bc51739a40
--- /dev/null
+++ b/drivers/net/pcs/pcs-mtk-usxgmii.c
@@ -0,0 +1,456 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2023 MediaTek Inc.
+ * Author: Henry Yen <henry.yen@mediatek.com>
+ *         Daniel Golle <daniel@makrotopia.org>
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mdio.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/reset.h>
+#include <linux/pcs/pcs-mtk-usxgmii.h>
+#include <linux/platform_device.h>
+
+/* USXGMII subsystem config registers */
+/* Register to control speed */
+#define RG_PHY_TOP_SPEED_CTRL1			0x80c
+#define USXGMII_RATE_UPDATE_MODE		BIT(31)
+#define USXGMII_MAC_CK_GATED			BIT(29)
+#define USXGMII_IF_FORCE_EN			BIT(28)
+#define USXGMII_RATE_ADAPT_MODE			GENMASK(10, 8)
+#define USXGMII_RATE_ADAPT_MODE_X1		0
+#define USXGMII_RATE_ADAPT_MODE_X2		1
+#define USXGMII_RATE_ADAPT_MODE_X4		2
+#define USXGMII_RATE_ADAPT_MODE_X10		3
+#define USXGMII_RATE_ADAPT_MODE_X100		4
+#define USXGMII_RATE_ADAPT_MODE_X5		5
+#define USXGMII_RATE_ADAPT_MODE_X50		6
+#define USXGMII_XFI_RX_MODE			GENMASK(6, 4)
+#define USXGMII_XFI_TX_MODE			GENMASK(2, 0)
+#define USXGMII_XFI_MODE_10G			0
+#define USXGMII_XFI_MODE_5G			1
+#define USXGMII_XFI_MODE_2P5G			3
+
+/* Register to control PCS AN */
+#define RG_PCS_AN_CTRL0				0x810
+#define USXGMII_AN_RESTART			BIT(31)
+#define USXGMII_AN_SYNC_CNT			GENMASK(30, 11)
+#define USXGMII_AN_ENABLE			BIT(0)
+
+#define RG_PCS_AN_CTRL2				0x818
+#define USXGMII_LINK_TIMER_IDLE_DETECT		GENMASK(29, 20)
+#define USXGMII_LINK_TIMER_COMP_ACK_DETECT	GENMASK(19, 10)
+#define USXGMII_LINK_TIMER_AN_RESTART		GENMASK(9, 0)
+
+/* Register to read PCS AN status */
+#define RG_PCS_AN_STS0				0x81c
+#define USXGMII_LPA				GENMASK(15, 0)
+#define USXGMII_LPA_LATCH			BIT(31)
+
+/* Register to read PCS link status */
+#define RG_PCS_RX_STATUS0			0x904
+#define RG_PCS_RX_STATUS_UPDATE			BIT(16)
+#define RG_PCS_RX_LINK_STATUS			BIT(2)
+
+/* struct mtk_usxgmii_pcs - This structure holds each usxgmii PCS
+ * @pcs:		Phylink PCS structure
+ * @dev:		Pointer to device structure
+ * @base:		IO memory to access PCS hardware
+ * @clk:		Pointer to USXGMII clk
+ * @reset:		Pointer to USXGMII reset control
+ * @interface:		Currently selected interface mode
+ * @neg_mode:		Currently used phylink neg_mode
+ * @node:		List node
+ */
+struct mtk_usxgmii_pcs {
+	struct phylink_pcs		pcs;
+	struct device			*dev;
+	void __iomem			*base;
+	struct clk			*clk;
+	struct reset_control		*reset;
+	phy_interface_t			interface;
+	unsigned int			neg_mode;
+	struct list_head		node;
+};
+
+static LIST_HEAD(mtk_usxgmii_pcs_instances);
+static DEFINE_MUTEX(instance_mutex);
+
+static u32 mtk_r32(struct mtk_usxgmii_pcs *mpcs, unsigned int reg)
+{
+	return ioread32(mpcs->base + reg);
+}
+
+static void mtk_m32(struct mtk_usxgmii_pcs *mpcs, unsigned int reg, u32 mask, u32 set)
+{
+	u32 val;
+
+	val = ioread32(mpcs->base + reg);
+	val &= ~mask;
+	val |= set;
+	iowrite32(val, mpcs->base + reg);
+}
+
+static struct mtk_usxgmii_pcs *pcs_to_mtk_usxgmii_pcs(struct phylink_pcs *pcs)
+{
+	return container_of(pcs, struct mtk_usxgmii_pcs, pcs);
+}
+
+static void mtk_usxgmii_reset(struct mtk_usxgmii_pcs *mpcs)
+{
+	reset_control_assert(mpcs->reset);
+	udelay(100);
+	reset_control_deassert(mpcs->reset);
+
+	mdelay(10);
+}
+
+static int mtk_usxgmii_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
+				  phy_interface_t interface,
+				  const unsigned long *advertising,
+				  bool permit_pause_to_mac)
+{
+	struct mtk_usxgmii_pcs *mpcs = pcs_to_mtk_usxgmii_pcs(pcs);
+	unsigned int an_ctrl = 0, link_timer = 0, xfi_mode = 0, adapt_mode = 0;
+	bool mode_changed = false;
+
+	if (interface == PHY_INTERFACE_MODE_USXGMII) {
+		an_ctrl = FIELD_PREP(USXGMII_AN_SYNC_CNT, 0x1FF) | USXGMII_AN_ENABLE;
+		link_timer = FIELD_PREP(USXGMII_LINK_TIMER_IDLE_DETECT, 0x7B) |
+			     FIELD_PREP(USXGMII_LINK_TIMER_COMP_ACK_DETECT, 0x7B) |
+			     FIELD_PREP(USXGMII_LINK_TIMER_AN_RESTART, 0x7B);
+		xfi_mode = FIELD_PREP(USXGMII_XFI_RX_MODE, USXGMII_XFI_MODE_10G) |
+			   FIELD_PREP(USXGMII_XFI_TX_MODE, USXGMII_XFI_MODE_10G);
+	} else if (interface == PHY_INTERFACE_MODE_10GBASER) {
+		an_ctrl = FIELD_PREP(USXGMII_AN_SYNC_CNT, 0x1FF);
+		link_timer = FIELD_PREP(USXGMII_LINK_TIMER_IDLE_DETECT, 0x7B) |
+			     FIELD_PREP(USXGMII_LINK_TIMER_COMP_ACK_DETECT, 0x7B) |
+			     FIELD_PREP(USXGMII_LINK_TIMER_AN_RESTART, 0x7B);
+		xfi_mode = FIELD_PREP(USXGMII_XFI_RX_MODE, USXGMII_XFI_MODE_10G) |
+			   FIELD_PREP(USXGMII_XFI_TX_MODE, USXGMII_XFI_MODE_10G);
+		adapt_mode = USXGMII_RATE_UPDATE_MODE;
+	} else if (interface == PHY_INTERFACE_MODE_5GBASER) {
+		an_ctrl = FIELD_PREP(USXGMII_AN_SYNC_CNT, 0xFF);
+		link_timer = FIELD_PREP(USXGMII_LINK_TIMER_IDLE_DETECT, 0x3D) |
+			     FIELD_PREP(USXGMII_LINK_TIMER_COMP_ACK_DETECT, 0x3D) |
+			     FIELD_PREP(USXGMII_LINK_TIMER_AN_RESTART, 0x3D);
+		xfi_mode = FIELD_PREP(USXGMII_XFI_RX_MODE, USXGMII_XFI_MODE_5G) |
+			   FIELD_PREP(USXGMII_XFI_TX_MODE, USXGMII_XFI_MODE_5G);
+		adapt_mode = USXGMII_RATE_UPDATE_MODE;
+	} else {
+		return -EINVAL;
+	}
+
+	adapt_mode |= FIELD_PREP(USXGMII_RATE_ADAPT_MODE, USXGMII_RATE_ADAPT_MODE_X1);
+
+	if (mpcs->interface != interface) {
+		mpcs->interface = interface;
+		mode_changed = true;
+	}
+
+	mtk_usxgmii_reset(mpcs);
+
+	/* Setup USXGMII AN ctrl */
+	mtk_m32(mpcs, RG_PCS_AN_CTRL0,
+		USXGMII_AN_SYNC_CNT | USXGMII_AN_ENABLE,
+		an_ctrl);
+
+	mtk_m32(mpcs, RG_PCS_AN_CTRL2,
+		USXGMII_LINK_TIMER_IDLE_DETECT |
+		USXGMII_LINK_TIMER_COMP_ACK_DETECT |
+		USXGMII_LINK_TIMER_AN_RESTART,
+		link_timer);
+
+	mpcs->neg_mode = neg_mode;
+
+	/* Gated MAC CK */
+	mtk_m32(mpcs, RG_PHY_TOP_SPEED_CTRL1,
+		USXGMII_MAC_CK_GATED, USXGMII_MAC_CK_GATED);
+
+	/* Enable interface force mode */
+	mtk_m32(mpcs, RG_PHY_TOP_SPEED_CTRL1,
+		USXGMII_IF_FORCE_EN, USXGMII_IF_FORCE_EN);
+
+	/* Setup USXGMII adapt mode */
+	mtk_m32(mpcs, RG_PHY_TOP_SPEED_CTRL1,
+		USXGMII_RATE_UPDATE_MODE | USXGMII_RATE_ADAPT_MODE,
+		adapt_mode);
+
+	/* Setup USXGMII speed */
+	mtk_m32(mpcs, RG_PHY_TOP_SPEED_CTRL1,
+		USXGMII_XFI_RX_MODE | USXGMII_XFI_TX_MODE,
+		xfi_mode);
+
+	usleep_range(1, 10);
+
+	/* Un-gated MAC CK */
+	mtk_m32(mpcs, RG_PHY_TOP_SPEED_CTRL1, USXGMII_MAC_CK_GATED, 0);
+
+	usleep_range(1, 10);
+
+	/* Disable interface force mode for the AN mode */
+	if (an_ctrl & USXGMII_AN_ENABLE)
+		mtk_m32(mpcs, RG_PHY_TOP_SPEED_CTRL1, USXGMII_IF_FORCE_EN, 0);
+
+	return mode_changed;
+}
+
+static void mtk_usxgmii_pcs_get_fixed_speed(struct mtk_usxgmii_pcs *mpcs,
+					    struct phylink_link_state *state)
+{
+	u32 val = mtk_r32(mpcs, RG_PHY_TOP_SPEED_CTRL1);
+	int speed;
+
+	/* Calculate speed from interface speed and rate adapt mode */
+	switch (FIELD_GET(USXGMII_XFI_RX_MODE, val)) {
+	case USXGMII_XFI_MODE_10G:
+		speed = 10000;
+		break;
+	case USXGMII_XFI_MODE_5G:
+		speed = 5000;
+		break;
+	case USXGMII_XFI_MODE_2P5G:
+		speed = 2500;
+		break;
+	default:
+		state->speed = SPEED_UNKNOWN;
+		return;
+	}
+
+	switch (FIELD_GET(USXGMII_RATE_ADAPT_MODE, val)) {
+	case USXGMII_RATE_ADAPT_MODE_X100:
+		speed /= 100;
+		break;
+	case USXGMII_RATE_ADAPT_MODE_X50:
+		speed /= 50;
+		break;
+	case USXGMII_RATE_ADAPT_MODE_X10:
+		speed /= 10;
+		break;
+	case USXGMII_RATE_ADAPT_MODE_X5:
+		speed /= 5;
+		break;
+	case USXGMII_RATE_ADAPT_MODE_X4:
+		speed /= 4;
+		break;
+	case USXGMII_RATE_ADAPT_MODE_X2:
+		speed /= 2;
+		break;
+	case USXGMII_RATE_ADAPT_MODE_X1:
+		break;
+	default:
+		state->speed = SPEED_UNKNOWN;
+		return;
+	}
+
+	state->speed = speed;
+	state->duplex = DUPLEX_FULL;
+}
+
+static void mtk_usxgmii_pcs_get_an_state(struct mtk_usxgmii_pcs *mpcs,
+					 struct phylink_link_state *state)
+{
+	u16 lpa;
+
+	/* Refresh LPA by toggling LPA_LATCH */
+	mtk_m32(mpcs, RG_PCS_AN_STS0, USXGMII_LPA_LATCH, USXGMII_LPA_LATCH);
+	ndelay(1020);
+	mtk_m32(mpcs, RG_PCS_AN_STS0, USXGMII_LPA_LATCH, 0);
+	ndelay(1020);
+	lpa = FIELD_GET(USXGMII_LPA, mtk_r32(mpcs, RG_PCS_AN_STS0));
+
+	phylink_decode_usxgmii_word(state, lpa);
+}
+
+static void mtk_usxgmii_pcs_get_state(struct phylink_pcs *pcs,
+				      struct phylink_link_state *state)
+{
+	struct mtk_usxgmii_pcs *mpcs = pcs_to_mtk_usxgmii_pcs(pcs);
+
+	/* Refresh USXGMII link status by toggling RG_PCS_AN_STATUS_UPDATE */
+	mtk_m32(mpcs, RG_PCS_RX_STATUS0, RG_PCS_RX_STATUS_UPDATE,
+		RG_PCS_RX_STATUS_UPDATE);
+	ndelay(1020);
+	mtk_m32(mpcs, RG_PCS_RX_STATUS0, RG_PCS_RX_STATUS_UPDATE, 0);
+	ndelay(1020);
+
+	/* Read USXGMII link status */
+	state->link = FIELD_GET(RG_PCS_RX_LINK_STATUS,
+				mtk_r32(mpcs, RG_PCS_RX_STATUS0));
+
+	/* Continuously repeat re-configuration sequence until link comes up */
+	if (!state->link) {
+		mtk_usxgmii_pcs_config(pcs, mpcs->neg_mode,
+				       state->interface, NULL, false);
+		return;
+	}
+
+	if (FIELD_GET(USXGMII_AN_ENABLE, mtk_r32(mpcs, RG_PCS_AN_CTRL0)))
+		mtk_usxgmii_pcs_get_an_state(mpcs, state);
+	else
+		mtk_usxgmii_pcs_get_fixed_speed(mpcs, state);
+}
+
+static void mtk_usxgmii_pcs_restart_an(struct phylink_pcs *pcs)
+{
+	struct mtk_usxgmii_pcs *mpcs = pcs_to_mtk_usxgmii_pcs(pcs);
+
+	mtk_m32(mpcs, RG_PCS_AN_CTRL0, USXGMII_AN_RESTART, USXGMII_AN_RESTART);
+}
+
+static void mtk_usxgmii_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
+				    phy_interface_t interface,
+				    int speed, int duplex)
+{
+	/* Reconfiguring USXGMII to ensure the quality of the RX signal
+	 * after the line side link up.
+	 */
+	mtk_usxgmii_pcs_config(pcs, neg_mode, interface, NULL, false);
+}
+
+static void mtk_usxgmii_pcs_disable(struct phylink_pcs *pcs)
+{
+	struct mtk_usxgmii_pcs *mpcs = pcs_to_mtk_usxgmii_pcs(pcs);
+
+	mpcs->interface = PHY_INTERFACE_MODE_NA;
+	mpcs->neg_mode = -1;
+}
+
+static const struct phylink_pcs_ops mtk_usxgmii_pcs_ops = {
+	.pcs_config = mtk_usxgmii_pcs_config,
+	.pcs_get_state = mtk_usxgmii_pcs_get_state,
+	.pcs_an_restart = mtk_usxgmii_pcs_restart_an,
+	.pcs_link_up = mtk_usxgmii_pcs_link_up,
+	.pcs_disable = mtk_usxgmii_pcs_disable,
+};
+
+static int mtk_usxgmii_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mtk_usxgmii_pcs *mpcs;
+
+	mpcs = devm_kzalloc(dev, sizeof(*mpcs), GFP_KERNEL);
+	if (!mpcs)
+		return -ENOMEM;
+
+	mpcs->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mpcs->base))
+		return PTR_ERR(mpcs->base);
+
+	mpcs->dev = dev;
+	mpcs->pcs.ops = &mtk_usxgmii_pcs_ops;
+	mpcs->pcs.poll = true;
+	mpcs->pcs.neg_mode = true;
+	mpcs->interface = PHY_INTERFACE_MODE_NA;
+	mpcs->neg_mode = -1;
+
+	mpcs->clk = devm_clk_get_enabled(mpcs->dev, NULL);
+	if (IS_ERR(mpcs->clk))
+		return PTR_ERR(mpcs->clk);
+
+	mpcs->reset = devm_reset_control_get_shared(dev, NULL);
+	if (IS_ERR(mpcs->reset))
+		return PTR_ERR(mpcs->reset);
+
+	reset_control_deassert(mpcs->reset);
+
+	platform_set_drvdata(pdev, mpcs);
+
+	mutex_lock(&instance_mutex);
+	list_add_tail(&mpcs->node, &mtk_usxgmii_pcs_instances);
+	mutex_unlock(&instance_mutex);
+
+	return 0;
+}
+
+static int mtk_usxgmii_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mtk_usxgmii_pcs *cur, *tmp;
+
+	mutex_lock(&instance_mutex);
+	list_for_each_entry_safe(cur, tmp, &mtk_usxgmii_pcs_instances, node)
+		if (cur->dev == dev) {
+			list_del(&cur->node);
+			break;
+		}
+	mutex_unlock(&instance_mutex);
+
+	return 0;
+}
+
+static const struct of_device_id mtk_usxgmii_of_mtable[] = {
+	{ .compatible = "mediatek,mt7988-usxgmiisys" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mtk_usxgmii_of_mtable);
+
+struct phylink_pcs *mtk_usxgmii_pcs_get(struct device *dev, struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct mtk_usxgmii_pcs *mpcs;
+
+	if (!np)
+		return NULL;
+
+	if (!of_device_is_available(np))
+		return ERR_PTR(-ENODEV);
+
+	if (!of_match_node(mtk_usxgmii_of_mtable, np))
+		return ERR_PTR(-EINVAL);
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev || !platform_get_drvdata(pdev)) {
+		if (pdev)
+			put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	mpcs = platform_get_drvdata(pdev);
+	device_link_add(dev, mpcs->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
+
+	return &mpcs->pcs;
+}
+EXPORT_SYMBOL(mtk_usxgmii_pcs_get);
+
+void mtk_usxgmii_pcs_put(struct phylink_pcs *pcs)
+{
+	struct mtk_usxgmii_pcs *cur, *mpcs = NULL;
+
+	if (!pcs)
+		return;
+
+	mutex_lock(&instance_mutex);
+	list_for_each_entry(cur, &mtk_usxgmii_pcs_instances, node)
+		if (pcs == &cur->pcs) {
+			mpcs = cur;
+			break;
+		}
+	mutex_unlock(&instance_mutex);
+
+	if (WARN_ON(!mpcs))
+		return;
+
+	put_device(mpcs->dev);
+}
+EXPORT_SYMBOL(mtk_usxgmii_pcs_put);
+
+static struct platform_driver mtk_usxgmii_driver = {
+	.driver = {
+		.name			= "mtk_usxgmii",
+		.suppress_bind_attrs	= true,
+		.of_match_table		= mtk_usxgmii_of_mtable,
+	},
+	.probe = mtk_usxgmii_probe,
+	.remove = mtk_usxgmii_remove,
+};
+module_platform_driver(mtk_usxgmii_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MediaTek USXGMII PCS driver");
+MODULE_AUTHOR("Daniel Golle <daniel@makrotopia.org>");
diff --git a/include/linux/pcs/pcs-mtk-usxgmii.h b/include/linux/pcs/pcs-mtk-usxgmii.h
new file mode 100644
index 0000000000000..ef936d9c5f116
--- /dev/null
+++ b/include/linux/pcs/pcs-mtk-usxgmii.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_PCS_MTK_USXGMII_H
+#define __LINUX_PCS_MTK_USXGMII_H
+
+#include <linux/phylink.h>
+
+/**
+ * mtk_usxgmii_select_pcs() - Get MediaTek PCS instance
+ * @np:		Pointer to device node indentifying a MediaTek USXGMII PCS
+ * @mode:	Ethernet PHY interface mode
+ *
+ * Return PCS identified by a device node and the PHY interface mode in use
+ *
+ * Return:	Pointer to phylink PCS instance of NULL
+ */
+#if IS_ENABLED(CONFIG_PCS_MTK_USXGMII)
+struct phylink_pcs *mtk_usxgmii_pcs_get(struct device *dev, struct device_node *np);
+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 */
-- 
2.43.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Simon Horman]

On Tue, Dec 12, 2023 at 03:47:47AM +0000, Daniel Golle wrote:
> 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 <daniel@makrotopia.org>

Hi Daniel,

some minor feedback from my side.

...

> diff --git a/drivers/net/pcs/pcs-mtk-usxgmii.c b/drivers/net/pcs/pcs-mtk-usxgmii.c

...

> +static int mtk_usxgmii_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> +				  phy_interface_t interface,
> +				  const unsigned long *advertising,
> +				  bool permit_pause_to_mac)
> +{
> +	struct mtk_usxgmii_pcs *mpcs = pcs_to_mtk_usxgmii_pcs(pcs);
> +	unsigned int an_ctrl = 0, link_timer = 0, xfi_mode = 0, adapt_mode = 0;
> +	bool mode_changed = false;

nit: please consider arranging local variables in networking code
     in reverse xmas tree order - longest line to shortest.

     This may be useful:
     https://github.com/ecree-solarflare/xmastree

...
> diff --git a/include/linux/pcs/pcs-mtk-usxgmii.h b/include/linux/pcs/pcs-mtk-usxgmii.h
> new file mode 100644
> index 0000000000000..ef936d9c5f116
> --- /dev/null
> +++ b/include/linux/pcs/pcs-mtk-usxgmii.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_PCS_MTK_USXGMII_H
> +#define __LINUX_PCS_MTK_USXGMII_H
> +
> +#include <linux/phylink.h>
> +
> +/**
> + * mtk_usxgmii_select_pcs() - Get MediaTek PCS instance
> + * @np:		Pointer to device node indentifying a MediaTek USXGMII PCS

nit: identifying

     ./scripts/checkpatch.py --codespell is your friend here.

> + * @mode:	Ethernet PHY interface mode
> + *
> + * Return PCS identified by a device node and the PHY interface mode in use
> + *
> + * Return:	Pointer to phylink PCS instance of NULL
> + */
> +#if IS_ENABLED(CONFIG_PCS_MTK_USXGMII)
> +struct phylink_pcs *mtk_usxgmii_pcs_get(struct device *dev, struct device_node *np);

nit: The kernel doc above does not match the signature
     of mtk_usxgmii_pcs_get().

     ./scripts/kernel-doc -none is helpful here.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Daniel Golle]

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

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

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

Fixes: c94a9aabec36 ("dt-bindings: net: mediatek,net: add mt7988-eth binding")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 .../devicetree/bindings/net/mediatek,net.yaml | 32 ++++---------------
 1 file changed, 7 insertions(+), 25 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml
index e74502a0afe86..030d106bc7d3f 100644
--- a/Documentation/devicetree/bindings/net/mediatek,net.yaml
+++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml
@@ -337,32 +337,23 @@ allOf:
           minItems: 4
 
         clocks:
-          minItems: 34
-          maxItems: 34
+          minItems: 24
+          maxItems: 24
 
         clock-names:
           items:
-            - const: crypto
+            - const: xgp1
+            - const: xgp2
+            - const: xgp3
             - const: fe
             - const: gp2
             - const: gp1
             - const: gp3
+            - const: esw
+            - const: crypto
             - const: ethwarp_wocpu2
             - const: ethwarp_wocpu1
             - const: ethwarp_wocpu0
-            - const: esw
-            - const: netsys0
-            - const: netsys1
-            - const: sgmii_tx250m
-            - const: sgmii_rx250m
-            - const: sgmii2_tx250m
-            - const: sgmii2_rx250m
-            - const: top_usxgmii0_sel
-            - const: top_usxgmii1_sel
-            - const: top_sgm0_sel
-            - const: top_sgm1_sel
-            - const: top_xfi_phy0_xtal_sel
-            - const: top_xfi_phy1_xtal_sel
             - const: top_eth_gmii_sel
             - const: top_eth_refck_50m_sel
             - const: top_eth_sys_200m_sel
@@ -375,15 +366,6 @@ allOf:
             - const: top_netsys_sync_250m_sel
             - const: top_netsys_ppefb_250m_sel
             - const: top_netsys_warp_sel
-            - const: wocpu1
-            - const: wocpu0
-            - const: xgp1
-            - const: xgp2
-            - const: xgp3
-
-        mediatek,sgmiisys:
-          minItems: 2
-          maxItems: 2
 
 patternProperties:
   "^mac@[0-1]$":
-- 
2.43.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

On Tue, 12 Dec 2023 03:48:10 +0000, Daniel Golle wrote:
> 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 <daniel@makrotopia.org>
> ---
>  .../devicetree/bindings/net/mediatek,net.yaml | 32 ++++---------------
>  1 file changed, 7 insertions(+), 25 deletions(-)
> 

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

yamllint warnings/errors:

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

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/5859da6629b8b6c100eca4062dd193105bf829ba.1702352117.git.daniel@makrotopia.org

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

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

pip3 install dtschema --upgrade

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 894 bytes --]

On Tue, Dec 12, 2023 at 03:48:10AM +0000, Daniel Golle wrote:
> 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 <daniel@makrotopia.org>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Daniel Golle]

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

Fixes: c94a9aabec36 ("dt-bindings: net: mediatek,net: add mt7988-eth binding")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 .../devicetree/bindings/net/mediatek,net.yaml | 148 +++++++++++++++++-
 1 file changed, 146 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml
index 030d106bc7d3f..ca0667c51c1c2 100644
--- a/Documentation/devicetree/bindings/net/mediatek,net.yaml
+++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml
@@ -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";
+          };
+        };
+      };
+    };
-- 
2.43.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

On Tue, 12 Dec 2023 03:51:01 +0000, Daniel Golle wrote:
> 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 <daniel@makrotopia.org>
> ---
>  .../devicetree/bindings/net/mediatek,net.yaml | 148 +++++++++++++++++-
>  1 file changed, 146 insertions(+), 2 deletions(-)
> 

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

yamllint warnings/errors:

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

doc reference errors (make refcheckdocs):

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

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

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

pip3 install dtschema --upgrade

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 8235 bytes --]

On Tue, Dec 12, 2023 at 03:51:01AM +0000, Daniel Golle wrote:
> Complete support for MT7988 which comes with 3 MACs, SRAM for DMA
> descriptors and uses a dedicated PCS for the SerDes units.

The commit message here seems a bit incomplete, mostly a lack of an
explanation for why the model was initially incorrect.

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> 
> Fixes: c94a9aabec36 ("dt-bindings: net: mediatek,net: add mt7988-eth binding")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  .../devicetree/bindings/net/mediatek,net.yaml | 148 +++++++++++++++++-
>  1 file changed, 146 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/mediatek,net.yaml b/Documentation/devicetree/bindings/net/mediatek,net.yaml
> index 030d106bc7d3f..ca0667c51c1c2 100644
> --- a/Documentation/devicetree/bindings/net/mediatek,net.yaml
> +++ b/Documentation/devicetree/bindings/net/mediatek,net.yaml
> @@ -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";
> +          };
> +        };
> +      };
> +    };
> -- 
> 2.43.0

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

On 12/12/2023 04:51, Daniel Golle wrote:
> 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 <daniel@makrotopia.org>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH net-next v3 8/8] net: ethernet: mtk_eth_soc: add paths and SerDes modes for MT7988

[Daniel Golle]

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

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

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

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_path.c | 122 +++++++-
 drivers/net/ethernet/mediatek/mtk_eth_soc.c  | 291 +++++++++++++++++--
 drivers/net/ethernet/mediatek/mtk_eth_soc.h  | 107 ++++++-
 3 files changed, 469 insertions(+), 51 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_path.c b/drivers/net/ethernet/mediatek/mtk_eth_path.c
index 7c27a19c4d8f4..3f4f4cfe6a233 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_path.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_path.c
@@ -31,10 +31,20 @@ static const char *mtk_eth_path_name(u64 path)
 		return "gmac2_rgmii";
 	case MTK_ETH_PATH_GMAC2_SGMII:
 		return "gmac2_sgmii";
+	case MTK_ETH_PATH_GMAC2_2P5GPHY:
+		return "gmac2_2p5gphy";
 	case MTK_ETH_PATH_GMAC2_GEPHY:
 		return "gmac2_gephy";
+	case MTK_ETH_PATH_GMAC3_SGMII:
+		return "gmac3_sgmii";
 	case MTK_ETH_PATH_GDM1_ESW:
 		return "gdm1_esw";
+	case MTK_ETH_PATH_GMAC1_USXGMII:
+		return "gmac1_usxgmii";
+	case MTK_ETH_PATH_GMAC2_USXGMII:
+		return "gmac2_usxgmii";
+	case MTK_ETH_PATH_GMAC3_USXGMII:
+		return "gmac3_usxgmii";
 	default:
 		return "unknown path";
 	}
@@ -127,6 +137,27 @@ static int set_mux_u3_gmac2_to_qphy(struct mtk_eth *eth, u64 path)
 	return 0;
 }
 
+static int set_mux_gmac2_to_2p5gphy(struct mtk_eth *eth, u64 path)
+{
+	int ret;
+
+	if (path == MTK_ETH_PATH_GMAC2_2P5GPHY) {
+		ret = regmap_clear_bits(eth->ethsys, ETHSYS_SYSCFG0, SYSCFG0_SGMII_GMAC2_V2);
+		if (ret)
+			return ret;
+
+		/* Setup mux to 2p5g PHY */
+		ret = regmap_clear_bits(eth->infra, TOP_MISC_NETSYS_PCS_MUX, MUX_G2_USXGMII_SEL);
+		if (ret)
+			return ret;
+
+		dev_dbg(eth->dev, "path %s in %s updated\n",
+			mtk_eth_path_name(path), __func__);
+	}
+
+	return 0;
+}
+
 static int set_mux_gmac1_gmac2_to_sgmii_rgmii(struct mtk_eth *eth, u64 path)
 {
 	unsigned int val = 0;
@@ -165,7 +196,48 @@ static int set_mux_gmac1_gmac2_to_sgmii_rgmii(struct mtk_eth *eth, u64 path)
 	return 0;
 }
 
-static int set_mux_gmac12_to_gephy_sgmii(struct mtk_eth *eth, u64 path)
+static int set_mux_gmac123_to_usxgmii(struct mtk_eth *eth, u64 path)
+{
+	unsigned int val = 0;
+	bool updated = true;
+	int mac_id = 0;
+
+	/* Disable SYSCFG1 SGMII */
+	regmap_read(eth->ethsys, ETHSYS_SYSCFG0, &val);
+
+	switch (path) {
+	case MTK_ETH_PATH_GMAC1_USXGMII:
+		val &= ~(u32)SYSCFG0_SGMII_GMAC1_V2;
+		mac_id = MTK_GMAC1_ID;
+		break;
+	case MTK_ETH_PATH_GMAC2_USXGMII:
+		val &= ~(u32)SYSCFG0_SGMII_GMAC2_V2;
+		mac_id = MTK_GMAC2_ID;
+		break;
+	case MTK_ETH_PATH_GMAC3_USXGMII:
+		val &= ~(u32)SYSCFG0_SGMII_GMAC3_V2;
+		mac_id = MTK_GMAC3_ID;
+		break;
+	default:
+		updated = false;
+	};
+
+	if (updated) {
+		regmap_update_bits(eth->ethsys, ETHSYS_SYSCFG0,
+				   SYSCFG0_SGMII_MASK, val);
+
+		if (mac_id == MTK_GMAC2_ID)
+			regmap_set_bits(eth->infra, TOP_MISC_NETSYS_PCS_MUX,
+					MUX_G2_USXGMII_SEL);
+	}
+
+	dev_dbg(eth->dev, "path %s in %s updated = %d\n",
+		mtk_eth_path_name(path), __func__, updated);
+
+	return 0;
+}
+
+static int set_mux_gmac123_to_gephy_sgmii(struct mtk_eth *eth, u64 path)
 {
 	unsigned int val = 0;
 	bool updated = true;
@@ -182,6 +254,9 @@ static int set_mux_gmac12_to_gephy_sgmii(struct mtk_eth *eth, u64 path)
 	case MTK_ETH_PATH_GMAC2_SGMII:
 		val |= SYSCFG0_SGMII_GMAC2_V2;
 		break;
+	case MTK_ETH_PATH_GMAC3_SGMII:
+		val |= SYSCFG0_SGMII_GMAC3_V2;
+		break;
 	default:
 		updated = false;
 	}
@@ -209,6 +284,10 @@ static const struct mtk_eth_muxc mtk_eth_muxc[] = {
 		.name = "mux_u3_gmac2_to_qphy",
 		.cap_bit = MTK_ETH_MUX_U3_GMAC2_TO_QPHY,
 		.set_path = set_mux_u3_gmac2_to_qphy,
+	}, {
+		.name = "mux_gmac2_to_2p5gphy",
+		.cap_bit = MTK_ETH_MUX_GMAC2_TO_2P5GPHY,
+		.set_path = set_mux_gmac2_to_2p5gphy,
 	}, {
 		.name = "mux_gmac1_gmac2_to_sgmii_rgmii",
 		.cap_bit = MTK_ETH_MUX_GMAC1_GMAC2_TO_SGMII_RGMII,
@@ -216,7 +295,15 @@ static const struct mtk_eth_muxc mtk_eth_muxc[] = {
 	}, {
 		.name = "mux_gmac12_to_gephy_sgmii",
 		.cap_bit = MTK_ETH_MUX_GMAC12_TO_GEPHY_SGMII,
-		.set_path = set_mux_gmac12_to_gephy_sgmii,
+		.set_path = set_mux_gmac123_to_gephy_sgmii,
+	}, {
+		.name = "mux_gmac123_to_gephy_sgmii",
+		.cap_bit = MTK_ETH_MUX_GMAC123_TO_GEPHY_SGMII,
+		.set_path = set_mux_gmac123_to_gephy_sgmii,
+	}, {
+		.name = "mux_gmac123_to_usxgmii",
+		.cap_bit = MTK_ETH_MUX_GMAC123_TO_USXGMII,
+		.set_path = set_mux_gmac123_to_usxgmii,
 	},
 };
 
@@ -249,12 +336,39 @@ static int mtk_eth_mux_setup(struct mtk_eth *eth, u64 path)
 	return err;
 }
 
+int mtk_gmac_usxgmii_path_setup(struct mtk_eth *eth, int mac_id)
+{
+	u64 path;
+
+	path = (mac_id == MTK_GMAC1_ID) ?  MTK_ETH_PATH_GMAC1_USXGMII :
+	       (mac_id == MTK_GMAC2_ID) ?  MTK_ETH_PATH_GMAC2_USXGMII :
+					   MTK_ETH_PATH_GMAC3_USXGMII;
+
+	/* Setup proper MUXes along the path */
+	return mtk_eth_mux_setup(eth, path);
+}
+
 int mtk_gmac_sgmii_path_setup(struct mtk_eth *eth, int mac_id)
 {
 	u64 path;
 
-	path = (mac_id == 0) ?  MTK_ETH_PATH_GMAC1_SGMII :
-				MTK_ETH_PATH_GMAC2_SGMII;
+	path = (mac_id == MTK_GMAC1_ID) ? MTK_ETH_PATH_GMAC1_SGMII :
+	       (mac_id == MTK_GMAC2_ID) ? MTK_ETH_PATH_GMAC2_SGMII :
+					  MTK_ETH_PATH_GMAC3_SGMII;
+
+	/* Setup proper MUXes along the path */
+	return mtk_eth_mux_setup(eth, path);
+}
+
+int mtk_gmac_2p5gphy_path_setup(struct mtk_eth *eth, int mac_id)
+{
+	u64 path = 0;
+
+	if (mac_id == MTK_GMAC2_ID)
+		path = MTK_ETH_PATH_GMAC2_2P5GPHY;
+
+	if (!path)
+		return -EINVAL;
 
 	/* Setup proper MUXes along the path */
 	return mtk_eth_mux_setup(eth, path);
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index a6e91573f8dae..da0ec44268c8e 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -22,6 +22,8 @@
 #include <linux/pinctrl/devinfo.h>
 #include <linux/phylink.h>
 #include <linux/pcs/pcs-mtk-lynxi.h>
+#include <linux/pcs/pcs-mtk-usxgmii.h>
+#include <linux/phy/phy.h>
 #include <linux/jhash.h>
 #include <linux/bitfield.h>
 #include <net/dsa.h>
@@ -260,12 +262,8 @@ static const char * const mtk_clks_source_name[] = {
 	"ethwarp_wocpu2",
 	"ethwarp_wocpu1",
 	"ethwarp_wocpu0",
-	"top_usxgmii0_sel",
-	"top_usxgmii1_sel",
 	"top_sgm0_sel",
 	"top_sgm1_sel",
-	"top_xfi_phy0_xtal_sel",
-	"top_xfi_phy1_xtal_sel",
 	"top_eth_gmii_sel",
 	"top_eth_refck_50m_sel",
 	"top_eth_sys_200m_sel",
@@ -508,6 +506,30 @@ static void mtk_setup_bridge_switch(struct mtk_eth *eth)
 		MTK_GSW_CFG);
 }
 
+static bool mtk_check_gmac23_idle(struct mtk_mac *mac)
+{
+	u32 mac_fsm, gdm_fsm;
+
+	mac_fsm = mtk_r32(mac->hw, MTK_MAC_FSM(mac->id));
+
+	switch (mac->id) {
+	case MTK_GMAC2_ID:
+		gdm_fsm = mtk_r32(mac->hw, MTK_FE_GDM2_FSM);
+		break;
+	case MTK_GMAC3_ID:
+		gdm_fsm = mtk_r32(mac->hw, MTK_FE_GDM3_FSM);
+		break;
+	default:
+		return true;
+	};
+
+	if ((mac_fsm & 0xFFFF0000) == 0x01010000 &&
+	    (gdm_fsm & 0xFFFF0000) == 0x00000000)
+		return true;
+
+	return false;
+}
+
 static struct phylink_pcs *mtk_mac_select_pcs(struct phylink_config *config,
 					      phy_interface_t interface)
 {
@@ -516,6 +538,21 @@ static struct phylink_pcs *mtk_mac_select_pcs(struct phylink_config *config,
 	struct mtk_eth *eth = mac->hw;
 	unsigned int sid;
 
+	if (mtk_is_netsys_v3_or_greater(eth)) {
+		switch (interface) {
+		case PHY_INTERFACE_MODE_1000BASEX:
+		case PHY_INTERFACE_MODE_2500BASEX:
+		case PHY_INTERFACE_MODE_SGMII:
+			return mac->sgmii_pcs;
+		case PHY_INTERFACE_MODE_5GBASER:
+		case PHY_INTERFACE_MODE_10GBASER:
+		case PHY_INTERFACE_MODE_USXGMII:
+			return mac->usxgmii_pcs;
+		default:
+			return NULL;
+		}
+	}
+
 	if (interface == PHY_INTERFACE_MODE_SGMII ||
 	    phy_interface_mode_is_8023z(interface)) {
 		sid = (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_SGMII)) ?
@@ -567,7 +604,22 @@ static void mtk_mac_config(struct phylink_config *config, unsigned int mode,
 					goto init_err;
 			}
 			break;
+		case PHY_INTERFACE_MODE_USXGMII:
+		case PHY_INTERFACE_MODE_10GBASER:
+		case PHY_INTERFACE_MODE_5GBASER:
+			if (MTK_HAS_CAPS(eth->soc->caps, MTK_USXGMII)) {
+				err = mtk_gmac_usxgmii_path_setup(eth, mac->id);
+				if (err)
+					goto init_err;
+			}
+			break;
 		case PHY_INTERFACE_MODE_INTERNAL:
+			if (mac->id == MTK_GMAC2_ID &&
+			    MTK_HAS_CAPS(eth->soc->caps, MTK_2P5GPHY)) {
+				err = mtk_gmac_2p5gphy_path_setup(eth, mac->id);
+				if (err)
+					goto init_err;
+			}
 			break;
 		default:
 			goto err_phy;
@@ -614,8 +666,6 @@ static void mtk_mac_config(struct phylink_config *config, unsigned int mode,
 		val &= ~SYSCFG0_GE_MODE(SYSCFG0_GE_MASK, mac->id);
 		val |= SYSCFG0_GE_MODE(ge_mode, mac->id);
 		regmap_write(eth->ethsys, ETHSYS_SYSCFG0, val);
-
-		mac->interface = state->interface;
 	}
 
 	/* SGMII */
@@ -632,21 +682,40 @@ static void mtk_mac_config(struct phylink_config *config, unsigned int mode,
 
 		/* Save the syscfg0 value for mac_finish */
 		mac->syscfg0 = val;
-	} else if (phylink_autoneg_inband(mode)) {
+	} else if (state->interface != PHY_INTERFACE_MODE_USXGMII &&
+		   state->interface != PHY_INTERFACE_MODE_10GBASER &&
+		   state->interface != PHY_INTERFACE_MODE_5GBASER &&
+		   phylink_autoneg_inband(mode)) {
 		dev_err(eth->dev,
-			"In-band mode not supported in non SGMII mode!\n");
+			"In-band mode not supported in non-SerDes modes!\n");
 		return;
 	}
 
 	/* Setup gmac */
-	if (mtk_is_netsys_v3_or_greater(eth) &&
-	    mac->interface == PHY_INTERFACE_MODE_INTERNAL) {
-		mtk_w32(mac->hw, MTK_GDMA_XGDM_SEL, MTK_GDMA_EG_CTRL(mac->id));
-		mtk_w32(mac->hw, MAC_MCR_FORCE_LINK_DOWN, MTK_MAC_MCR(mac->id));
+	if (mtk_is_netsys_v3_or_greater(eth)) {
+		if (mtk_interface_mode_is_xgmii(state->interface)) {
+			mtk_w32(mac->hw, MTK_GDMA_XGDM_SEL, MTK_GDMA_EG_CTRL(mac->id));
+			mtk_w32(mac->hw, MAC_MCR_FORCE_LINK_DOWN, MTK_MAC_MCR(mac->id));
 
-		mtk_setup_bridge_switch(eth);
+			if (mac->id == MTK_GMAC1_ID)
+				mtk_setup_bridge_switch(eth);
+		} else {
+			mtk_w32(eth, 0, MTK_GDMA_EG_CTRL(mac->id));
+
+			/* FIXME: In current hardware design, we have to reset FE
+			 * when swtiching XGDM to GDM. Therefore, here trigger an SER
+			 * to let GDM go back to the initial state.
+			 */
+			if ((mtk_interface_mode_is_xgmii(mac->interface) ||
+			     mac->interface == PHY_INTERFACE_MODE_NA) &&
+			    !mtk_check_gmac23_idle(mac) &&
+			    !test_bit(MTK_RESETTING, &eth->state))
+				schedule_work(&eth->pending_work);
+		}
 	}
 
+	mac->interface = state->interface;
+
 	return;
 
 err_phy:
@@ -659,6 +728,18 @@ static void mtk_mac_config(struct phylink_config *config, unsigned int mode,
 		mac->id, phy_modes(state->interface), err);
 }
 
+static int mtk_mac_prepare(struct phylink_config *config, unsigned int mode,
+			   phy_interface_t interface)
+{
+	struct mtk_mac *mac = container_of(config, struct mtk_mac,
+					   phylink_config);
+
+	if (mac->pextp && mac->interface != interface)
+		phy_reset(mac->pextp);
+
+	return 0;
+}
+
 static int mtk_mac_finish(struct phylink_config *config, unsigned int mode,
 			  phy_interface_t interface)
 {
@@ -667,6 +748,10 @@ static int mtk_mac_finish(struct phylink_config *config, unsigned int mode,
 	struct mtk_eth *eth = mac->hw;
 	u32 mcr_cur, mcr_new;
 
+	/* Setup PMA/PMD */
+	if (mac->pextp)
+		phy_set_mode_ext(mac->pextp, PHY_MODE_ETHERNET, interface);
+
 	/* Enable SGMII */
 	if (interface == PHY_INTERFACE_MODE_SGMII ||
 	    phy_interface_mode_is_8023z(interface))
@@ -692,10 +777,13 @@ static void mtk_mac_link_down(struct phylink_config *config, unsigned int mode,
 {
 	struct mtk_mac *mac = container_of(config, struct mtk_mac,
 					   phylink_config);
-	u32 mcr = mtk_r32(mac->hw, MTK_MAC_MCR(mac->id));
 
-	mcr &= ~(MAC_MCR_TX_EN | MAC_MCR_RX_EN);
-	mtk_w32(mac->hw, mcr, MTK_MAC_MCR(mac->id));
+	if (!mtk_interface_mode_is_xgmii(interface)) {
+		mtk_m32(mac->hw, MAC_MCR_TX_EN | MAC_MCR_RX_EN, 0, MTK_MAC_MCR(mac->id));
+		mtk_m32(mac->hw, MTK_XGMAC_FORCE_LINK(mac->id), 0, MTK_XGMAC_STS(mac->id));
+	} else if (mac->id != MTK_GMAC1_ID) {
+		mtk_m32(mac->hw, XMAC_MCR_TRX_DISABLE, XMAC_MCR_TRX_DISABLE, MTK_XMAC_MCR(mac->id));
+	}
 }
 
 static void mtk_set_queue_speed(struct mtk_eth *eth, unsigned int idx,
@@ -767,13 +855,11 @@ static void mtk_set_queue_speed(struct mtk_eth *eth, unsigned int idx,
 	mtk_w32(eth, val, soc->reg_map->qdma.qtx_sch + ofs);
 }
 
-static void mtk_mac_link_up(struct phylink_config *config,
-			    struct phy_device *phy,
-			    unsigned int mode, phy_interface_t interface,
-			    int speed, int duplex, bool tx_pause, bool rx_pause)
+static void mtk_gdm_mac_link_up(struct mtk_mac *mac,
+				struct phy_device *phy,
+				unsigned int mode, phy_interface_t interface,
+				int speed, int duplex, bool tx_pause, bool rx_pause)
 {
-	struct mtk_mac *mac = container_of(config, struct mtk_mac,
-					   phylink_config);
 	u32 mcr;
 
 	mcr = mtk_r32(mac->hw, MTK_MAC_MCR(mac->id));
@@ -807,9 +893,63 @@ static void mtk_mac_link_up(struct phylink_config *config,
 	mtk_w32(mac->hw, mcr, MTK_MAC_MCR(mac->id));
 }
 
+static void mtk_xgdm_mac_link_up(struct mtk_mac *mac,
+				 struct phy_device *phy,
+				 unsigned int mode, phy_interface_t interface,
+				 int speed, int duplex, bool tx_pause, bool rx_pause)
+{
+	u32 mcr, force_link = 0;
+
+	if (mac->id == MTK_GMAC1_ID)
+		return;
+
+	/* Eliminate the interference(before link-up) caused by PHY noise */
+	mtk_m32(mac->hw, XMAC_LOGIC_RST, 0, MTK_XMAC_LOGIC_RST(mac->id));
+	mdelay(20);
+	mtk_m32(mac->hw, XMAC_GLB_CNTCLR, XMAC_GLB_CNTCLR, MTK_XMAC_CNT_CTRL(mac->id));
+
+	if (mac->interface == PHY_INTERFACE_MODE_INTERNAL || mac->id == MTK_GMAC3_ID)
+		force_link = MTK_XGMAC_FORCE_LINK(mac->id);
+
+	mtk_m32(mac->hw, MTK_XGMAC_FORCE_LINK(mac->id), force_link, MTK_XGMAC_STS(mac->id));
+
+	mcr = mtk_r32(mac->hw, MTK_XMAC_MCR(mac->id));
+	mcr &= ~(XMAC_MCR_FORCE_TX_FC | XMAC_MCR_FORCE_RX_FC | XMAC_MCR_TRX_DISABLE);
+	/* Configure pause modes -
+	 * phylink will avoid these for half duplex
+	 */
+	if (tx_pause)
+		mcr |= XMAC_MCR_FORCE_TX_FC;
+	if (rx_pause)
+		mcr |= XMAC_MCR_FORCE_RX_FC;
+
+	mtk_w32(mac->hw, mcr, MTK_XMAC_MCR(mac->id));
+}
+
+static void mtk_mac_link_up(struct phylink_config *config,
+			    struct phy_device *phy,
+			    unsigned int mode, phy_interface_t interface,
+			    int speed, int duplex, bool tx_pause, bool rx_pause)
+{
+	struct mtk_mac *mac = container_of(config, struct mtk_mac,
+					   phylink_config);
+
+	if (mtk_interface_mode_is_xgmii(interface))
+		mtk_xgdm_mac_link_up(mac, phy, mode, interface, speed, duplex,
+				     tx_pause, rx_pause);
+	else
+		mtk_gdm_mac_link_up(mac, phy, mode, interface, speed, duplex,
+				    tx_pause, rx_pause);
+
+	/* Repeat pextp setup to tune link */
+	if (mac->pextp)
+		phy_set_mode_ext(mac->pextp, PHY_MODE_ETHERNET, interface);
+}
+
 static const struct phylink_mac_ops mtk_phylink_ops = {
 	.mac_select_pcs = mtk_mac_select_pcs,
 	.mac_config = mtk_mac_config,
+	.mac_prepare = mtk_mac_prepare,
 	.mac_finish = mtk_mac_finish,
 	.mac_link_down = mtk_mac_link_down,
 	.mac_link_up = mtk_mac_link_up,
@@ -3359,6 +3499,9 @@ static int mtk_open(struct net_device *dev)
 	struct mtk_eth *eth = mac->hw;
 	int i, err;
 
+	if (mac->pextp)
+		phy_power_on(mac->pextp);
+
 	err = phylink_of_phy_connect(mac->phylink, mac->of_node, 0);
 	if (err) {
 		netdev_err(dev, "%s: could not attach PHY: %d\n", __func__,
@@ -3488,6 +3631,9 @@ static int mtk_stop(struct net_device *dev)
 	for (i = 0; i < ARRAY_SIZE(eth->ppe); i++)
 		mtk_ppe_stop(eth->ppe[i]);
 
+	if (mac->pextp)
+		phy_power_off(mac->pextp);
+
 	return 0;
 }
 
@@ -4485,6 +4631,7 @@ static const struct net_device_ops mtk_netdev_ops = {
 static int mtk_add_mac(struct mtk_eth *eth, struct device_node *np)
 {
 	const __be32 *_id = of_get_property(np, "reg", NULL);
+	struct device_node *pcs_np;
 	phy_interface_t phy_mode;
 	struct phylink *phylink;
 	struct mtk_mac *mac;
@@ -4521,16 +4668,41 @@ static int mtk_add_mac(struct mtk_eth *eth, struct device_node *np)
 	mac->id = id;
 	mac->hw = eth;
 	mac->of_node = np;
+	pcs_np = of_parse_phandle(mac->of_node, "pcs-handle", 0);
+	if (pcs_np) {
+		mac->sgmii_pcs = mtk_pcs_lynxi_get(eth->dev, pcs_np);
+		if (IS_ERR(mac->sgmii_pcs)) {
+			if (PTR_ERR(mac->sgmii_pcs) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+
+			dev_err(eth->dev, "cannot select SGMII PCS, error %ld\n",
+				PTR_ERR(mac->sgmii_pcs));
+			return PTR_ERR(mac->sgmii_pcs);
+		}
+	}
 
-	err = of_get_ethdev_address(mac->of_node, eth->netdev[id]);
-	if (err == -EPROBE_DEFER)
-		return err;
+	pcs_np = of_parse_phandle(mac->of_node, "pcs-handle", 1);
+	if (pcs_np) {
+		mac->usxgmii_pcs = mtk_usxgmii_pcs_get(eth->dev, pcs_np);
+		if (IS_ERR(mac->usxgmii_pcs)) {
+			if (PTR_ERR(mac->usxgmii_pcs) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
 
-	if (err) {
-		/* If the mac address is invalid, use random mac address */
-		eth_hw_addr_random(eth->netdev[id]);
-		dev_err(eth->dev, "generated random MAC address %pM\n",
-			eth->netdev[id]->dev_addr);
+			dev_err(eth->dev, "cannot select USXGMII PCS, error %ld\n",
+				PTR_ERR(mac->usxgmii_pcs));
+			return PTR_ERR(mac->usxgmii_pcs);
+		}
+	}
+
+	if (mtk_is_netsys_v3_or_greater(eth) && (mac->sgmii_pcs || mac->usxgmii_pcs)) {
+		mac->pextp = devm_of_phy_get(eth->dev, mac->of_node, NULL);
+		if (IS_ERR(mac->pextp)) {
+			if (PTR_ERR(mac->pextp) != -EPROBE_DEFER)
+				dev_err(eth->dev, "cannot get PHY, error %ld\n",
+					PTR_ERR(mac->pextp));
+
+			return PTR_ERR(mac->pextp);
+		}
 	}
 
 	memset(mac->hwlro_ip, 0, sizeof(mac->hwlro_ip));
@@ -4613,8 +4785,21 @@ static int mtk_add_mac(struct mtk_eth *eth, struct device_node *np)
 		phy_interface_zero(mac->phylink_config.supported_interfaces);
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  mac->phylink_config.supported_interfaces);
+	} else if (MTK_HAS_CAPS(mac->hw->soc->caps, MTK_USXGMII)) {
+		mac->phylink_config.mac_capabilities |= MAC_5000FD | MAC_10000FD;
+		__set_bit(PHY_INTERFACE_MODE_5GBASER,
+			  mac->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_10GBASER,
+			  mac->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_USXGMII,
+			  mac->phylink_config.supported_interfaces);
 	}
 
+	if (MTK_HAS_CAPS(mac->hw->soc->caps, MTK_2P5GPHY) &&
+	    id == MTK_GMAC2_ID)
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  mac->phylink_config.supported_interfaces);
+
 	phylink = phylink_create(&mac->phylink_config,
 				 of_fwnode_handle(mac->of_node),
 				 phy_mode, &mtk_phylink_ops);
@@ -4665,6 +4850,26 @@ static int mtk_add_mac(struct mtk_eth *eth, struct device_node *np)
 	return err;
 }
 
+static int mtk_mac_assign_address(struct mtk_eth *eth, int i, bool test_defer_only)
+{
+	int err = of_get_ethdev_address(eth->mac[i]->of_node, eth->netdev[i]);
+
+	if (err == -EPROBE_DEFER)
+		return err;
+
+	if (test_defer_only)
+		return 0;
+
+	if (err) {
+		/* If the mac address is invalid, use random mac address */
+		eth_hw_addr_random(eth->netdev[i]);
+		dev_err(eth->dev, "generated random MAC address %pM\n",
+			eth->netdev[i]);
+	}
+
+	return 0;
+}
+
 void mtk_eth_set_dma_device(struct mtk_eth *eth, struct device *dma_dev)
 {
 	struct net_device *dev, *tmp;
@@ -4808,7 +5013,8 @@ static int mtk_probe(struct platform_device *pdev)
 			regmap_write(cci, 0, 3);
 	}
 
-	if (MTK_HAS_CAPS(eth->soc->caps, MTK_SGMII)) {
+	if (MTK_HAS_CAPS(eth->soc->caps, MTK_SGMII) &&
+	    !mtk_is_netsys_v3_or_greater(eth)) {
 		err = mtk_sgmii_init(eth);
 
 		if (err)
@@ -4919,6 +5125,24 @@ static int mtk_probe(struct platform_device *pdev)
 		}
 	}
 
+	for (i = 0; i < MTK_MAX_DEVS; i++) {
+		if (!eth->netdev[i])
+			continue;
+
+		err = mtk_mac_assign_address(eth, i, true);
+		if (err)
+			goto err_deinit_hw;
+	}
+
+	for (i = 0; i < MTK_MAX_DEVS; i++) {
+		if (!eth->netdev[i])
+			continue;
+
+		err = mtk_mac_assign_address(eth, i, false);
+		if (err)
+			goto err_deinit_hw;
+	}
+
 	if (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_INT)) {
 		err = devm_request_irq(eth->dev, eth->irq[0],
 				       mtk_handle_irq, 0,
@@ -5019,6 +5243,11 @@ static void mtk_remove(struct platform_device *pdev)
 		mtk_stop(eth->netdev[i]);
 		mac = netdev_priv(eth->netdev[i]);
 		phylink_disconnect_phy(mac->phylink);
+		if (mac->sgmii_pcs)
+			mtk_pcs_lynxi_put(mac->sgmii_pcs);
+
+		if (mac->usxgmii_pcs)
+			mtk_usxgmii_pcs_put(mac->usxgmii_pcs);
 	}
 
 	mtk_wed_exit();
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 9ae3b8a71d0e6..8424ac6cb725b 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -15,6 +15,7 @@
 #include <linux/u64_stats_sync.h>
 #include <linux/refcount.h>
 #include <linux/phylink.h>
+#include <linux/reset.h>
 #include <linux/rhashtable.h>
 #include <linux/dim.h>
 #include <linux/bitfield.h>
@@ -503,6 +504,21 @@
 #define INTF_MODE_RGMII_1000    (TRGMII_MODE | TRGMII_CENTRAL_ALIGNED)
 #define INTF_MODE_RGMII_10_100  0
 
+/* XFI Mac control registers */
+#define MTK_XMAC_BASE(x)	(0x12000 + (((x) - 1) * 0x1000))
+#define MTK_XMAC_MCR(x)		(MTK_XMAC_BASE(x))
+#define XMAC_MCR_TRX_DISABLE	0xf
+#define XMAC_MCR_FORCE_TX_FC	BIT(5)
+#define XMAC_MCR_FORCE_RX_FC	BIT(4)
+
+/* XFI Mac logic reset registers */
+#define MTK_XMAC_LOGIC_RST(x)	(MTK_XMAC_BASE(x) + 0x10)
+#define XMAC_LOGIC_RST		BIT(0)
+
+/* XFI Mac count global control */
+#define MTK_XMAC_CNT_CTRL(x)	(MTK_XMAC_BASE(x) + 0x100)
+#define XMAC_GLB_CNTCLR		BIT(0)
+
 /* GPIO port control registers for GMAC 2*/
 #define GPIO_OD33_CTRL8		0x4c0
 #define GPIO_BIAS_CTRL		0xed0
@@ -528,6 +544,7 @@
 #define SYSCFG0_SGMII_GMAC2    ((3 << 8) & SYSCFG0_SGMII_MASK)
 #define SYSCFG0_SGMII_GMAC1_V2 BIT(9)
 #define SYSCFG0_SGMII_GMAC2_V2 BIT(8)
+#define SYSCFG0_SGMII_GMAC3_V2 BIT(7)
 
 
 /* ethernet subsystem clock register */
@@ -566,6 +583,11 @@
 #define GEPHY_MAC_SEL          BIT(1)
 
 /* Top misc registers */
+#define TOP_MISC_NETSYS_PCS_MUX	0x84
+#define NETSYS_PCS_MUX_MASK	GENMASK(1, 0)
+#define	MUX_G2_USXGMII_SEL	BIT(1)
+#define MUX_HSGMII1_G1_SEL	BIT(0)
+
 #define USB_PHY_SWITCH_REG	0x218
 #define QPHY_SEL_MASK		GENMASK(1, 0)
 #define SGMII_QPHY_SEL		0x2
@@ -590,6 +612,8 @@
 #define MT7628_SDM_RBCNT	(MT7628_SDM_OFFSET + 0x10c)
 #define MT7628_SDM_CS_ERR	(MT7628_SDM_OFFSET + 0x110)
 
+/* Debug Purpose Register */
+#define MTK_PSE_FQFC_CFG	0x100
 #define MTK_FE_CDM1_FSM		0x220
 #define MTK_FE_CDM2_FSM		0x224
 #define MTK_FE_CDM3_FSM		0x238
@@ -598,6 +622,11 @@
 #define MTK_FE_CDM6_FSM		0x328
 #define MTK_FE_GDM1_FSM		0x228
 #define MTK_FE_GDM2_FSM		0x22C
+#define MTK_FE_GDM3_FSM		0x23C
+#define MTK_FE_PSE_FREE		0x240
+#define MTK_FE_DROP_FQ		0x244
+#define MTK_FE_DROP_FC		0x248
+#define MTK_FE_DROP_PPE		0x24C
 
 #define MTK_MAC_FSM(x)		(0x1010C + ((x) * 0x100))
 
@@ -722,12 +751,8 @@ enum mtk_clks_map {
 	MTK_CLK_ETHWARP_WOCPU2,
 	MTK_CLK_ETHWARP_WOCPU1,
 	MTK_CLK_ETHWARP_WOCPU0,
-	MTK_CLK_TOP_USXGMII_SBUS_0_SEL,
-	MTK_CLK_TOP_USXGMII_SBUS_1_SEL,
 	MTK_CLK_TOP_SGM_0_SEL,
 	MTK_CLK_TOP_SGM_1_SEL,
-	MTK_CLK_TOP_XFI_PHY_0_XTAL_SEL,
-	MTK_CLK_TOP_XFI_PHY_1_XTAL_SEL,
 	MTK_CLK_TOP_ETH_GMII_SEL,
 	MTK_CLK_TOP_ETH_REFCK_50M_SEL,
 	MTK_CLK_TOP_ETH_SYS_200M_SEL,
@@ -798,19 +823,9 @@ enum mtk_clks_map {
 				 BIT_ULL(MTK_CLK_GP3) | BIT_ULL(MTK_CLK_XGP1) | \
 				 BIT_ULL(MTK_CLK_XGP2) | BIT_ULL(MTK_CLK_XGP3) | \
 				 BIT_ULL(MTK_CLK_CRYPTO) | \
-				 BIT_ULL(MTK_CLK_SGMII_TX_250M) | \
-				 BIT_ULL(MTK_CLK_SGMII_RX_250M) | \
-				 BIT_ULL(MTK_CLK_SGMII2_TX_250M) | \
-				 BIT_ULL(MTK_CLK_SGMII2_RX_250M) | \
 				 BIT_ULL(MTK_CLK_ETHWARP_WOCPU2) | \
 				 BIT_ULL(MTK_CLK_ETHWARP_WOCPU1) | \
 				 BIT_ULL(MTK_CLK_ETHWARP_WOCPU0) | \
-				 BIT_ULL(MTK_CLK_TOP_USXGMII_SBUS_0_SEL) | \
-				 BIT_ULL(MTK_CLK_TOP_USXGMII_SBUS_1_SEL) | \
-				 BIT_ULL(MTK_CLK_TOP_SGM_0_SEL) | \
-				 BIT_ULL(MTK_CLK_TOP_SGM_1_SEL) | \
-				 BIT_ULL(MTK_CLK_TOP_XFI_PHY_0_XTAL_SEL) | \
-				 BIT_ULL(MTK_CLK_TOP_XFI_PHY_1_XTAL_SEL) | \
 				 BIT_ULL(MTK_CLK_TOP_ETH_GMII_SEL) | \
 				 BIT_ULL(MTK_CLK_TOP_ETH_REFCK_50M_SEL) | \
 				 BIT_ULL(MTK_CLK_TOP_ETH_SYS_200M_SEL) | \
@@ -944,6 +959,8 @@ enum mkt_eth_capabilities {
 	MTK_RGMII_BIT = 0,
 	MTK_TRGMII_BIT,
 	MTK_SGMII_BIT,
+	MTK_USXGMII_BIT,
+	MTK_2P5GPHY_BIT,
 	MTK_ESW_BIT,
 	MTK_GEPHY_BIT,
 	MTK_MUX_BIT,
@@ -964,8 +981,11 @@ enum mkt_eth_capabilities {
 	MTK_ETH_MUX_GDM1_TO_GMAC1_ESW_BIT,
 	MTK_ETH_MUX_GMAC2_GMAC0_TO_GEPHY_BIT,
 	MTK_ETH_MUX_U3_GMAC2_TO_QPHY_BIT,
+	MTK_ETH_MUX_GMAC2_TO_2P5GPHY_BIT,
 	MTK_ETH_MUX_GMAC1_GMAC2_TO_SGMII_RGMII_BIT,
 	MTK_ETH_MUX_GMAC12_TO_GEPHY_SGMII_BIT,
+	MTK_ETH_MUX_GMAC123_TO_GEPHY_SGMII_BIT,
+	MTK_ETH_MUX_GMAC123_TO_USXGMII_BIT,
 
 	/* PATH BITS */
 	MTK_ETH_PATH_GMAC1_RGMII_BIT,
@@ -973,14 +993,21 @@ enum mkt_eth_capabilities {
 	MTK_ETH_PATH_GMAC1_SGMII_BIT,
 	MTK_ETH_PATH_GMAC2_RGMII_BIT,
 	MTK_ETH_PATH_GMAC2_SGMII_BIT,
+	MTK_ETH_PATH_GMAC2_2P5GPHY_BIT,
 	MTK_ETH_PATH_GMAC2_GEPHY_BIT,
+	MTK_ETH_PATH_GMAC3_SGMII_BIT,
 	MTK_ETH_PATH_GDM1_ESW_BIT,
+	MTK_ETH_PATH_GMAC1_USXGMII_BIT,
+	MTK_ETH_PATH_GMAC2_USXGMII_BIT,
+	MTK_ETH_PATH_GMAC3_USXGMII_BIT,
 };
 
 /* Supported hardware group on SoCs */
 #define MTK_RGMII		BIT_ULL(MTK_RGMII_BIT)
 #define MTK_TRGMII		BIT_ULL(MTK_TRGMII_BIT)
 #define MTK_SGMII		BIT_ULL(MTK_SGMII_BIT)
+#define MTK_USXGMII		BIT_ULL(MTK_USXGMII_BIT)
+#define MTK_2P5GPHY		BIT_ULL(MTK_2P5GPHY_BIT)
 #define MTK_ESW			BIT_ULL(MTK_ESW_BIT)
 #define MTK_GEPHY		BIT_ULL(MTK_GEPHY_BIT)
 #define MTK_MUX			BIT_ULL(MTK_MUX_BIT)
@@ -1003,10 +1030,16 @@ enum mkt_eth_capabilities {
 	BIT_ULL(MTK_ETH_MUX_GMAC2_GMAC0_TO_GEPHY_BIT)
 #define MTK_ETH_MUX_U3_GMAC2_TO_QPHY		\
 	BIT_ULL(MTK_ETH_MUX_U3_GMAC2_TO_QPHY_BIT)
+#define MTK_ETH_MUX_GMAC2_TO_2P5GPHY		\
+	BIT_ULL(MTK_ETH_MUX_GMAC2_TO_2P5GPHY_BIT)
 #define MTK_ETH_MUX_GMAC1_GMAC2_TO_SGMII_RGMII	\
 	BIT_ULL(MTK_ETH_MUX_GMAC1_GMAC2_TO_SGMII_RGMII_BIT)
 #define MTK_ETH_MUX_GMAC12_TO_GEPHY_SGMII	\
 	BIT_ULL(MTK_ETH_MUX_GMAC12_TO_GEPHY_SGMII_BIT)
+#define MTK_ETH_MUX_GMAC123_TO_GEPHY_SGMII	\
+	BIT_ULL(MTK_ETH_MUX_GMAC123_TO_GEPHY_SGMII_BIT)
+#define MTK_ETH_MUX_GMAC123_TO_USXGMII	\
+	BIT_ULL(MTK_ETH_MUX_GMAC123_TO_USXGMII_BIT)
 
 /* Supported path present on SoCs */
 #define MTK_ETH_PATH_GMAC1_RGMII	BIT_ULL(MTK_ETH_PATH_GMAC1_RGMII_BIT)
@@ -1014,8 +1047,13 @@ enum mkt_eth_capabilities {
 #define MTK_ETH_PATH_GMAC1_SGMII	BIT_ULL(MTK_ETH_PATH_GMAC1_SGMII_BIT)
 #define MTK_ETH_PATH_GMAC2_RGMII	BIT_ULL(MTK_ETH_PATH_GMAC2_RGMII_BIT)
 #define MTK_ETH_PATH_GMAC2_SGMII	BIT_ULL(MTK_ETH_PATH_GMAC2_SGMII_BIT)
+#define MTK_ETH_PATH_GMAC2_2P5GPHY	BIT_ULL(MTK_ETH_PATH_GMAC2_2P5GPHY_BIT)
 #define MTK_ETH_PATH_GMAC2_GEPHY	BIT_ULL(MTK_ETH_PATH_GMAC2_GEPHY_BIT)
+#define MTK_ETH_PATH_GMAC3_SGMII	BIT_ULL(MTK_ETH_PATH_GMAC3_SGMII_BIT)
 #define MTK_ETH_PATH_GDM1_ESW		BIT_ULL(MTK_ETH_PATH_GDM1_ESW_BIT)
+#define MTK_ETH_PATH_GMAC1_USXGMII	BIT_ULL(MTK_ETH_PATH_GMAC1_USXGMII_BIT)
+#define MTK_ETH_PATH_GMAC2_USXGMII	BIT_ULL(MTK_ETH_PATH_GMAC2_USXGMII_BIT)
+#define MTK_ETH_PATH_GMAC3_USXGMII	BIT_ULL(MTK_ETH_PATH_GMAC3_USXGMII_BIT)
 
 #define MTK_GMAC1_RGMII		(MTK_ETH_PATH_GMAC1_RGMII | MTK_RGMII)
 #define MTK_GMAC1_TRGMII	(MTK_ETH_PATH_GMAC1_TRGMII | MTK_TRGMII)
@@ -1023,7 +1061,12 @@ enum mkt_eth_capabilities {
 #define MTK_GMAC2_RGMII		(MTK_ETH_PATH_GMAC2_RGMII | MTK_RGMII)
 #define MTK_GMAC2_SGMII		(MTK_ETH_PATH_GMAC2_SGMII | MTK_SGMII)
 #define MTK_GMAC2_GEPHY		(MTK_ETH_PATH_GMAC2_GEPHY | MTK_GEPHY)
+#define MTK_GMAC2_2P5GPHY	(MTK_ETH_PATH_GMAC2_2P5GPHY | MTK_2P5GPHY)
+#define MTK_GMAC3_SGMII		(MTK_ETH_PATH_GMAC3_SGMII | MTK_SGMII)
 #define MTK_GDM1_ESW		(MTK_ETH_PATH_GDM1_ESW | MTK_ESW)
+#define MTK_GMAC1_USXGMII	(MTK_ETH_PATH_GMAC1_USXGMII | MTK_USXGMII)
+#define MTK_GMAC2_USXGMII	(MTK_ETH_PATH_GMAC2_USXGMII | MTK_USXGMII)
+#define MTK_GMAC3_USXGMII	(MTK_ETH_PATH_GMAC3_USXGMII | MTK_USXGMII)
 
 /* MUXes present on SoCs */
 /* 0: GDM1 -> GMAC1, 1: GDM1 -> ESW */
@@ -1042,10 +1085,20 @@ enum mkt_eth_capabilities {
 	(MTK_ETH_MUX_GMAC1_GMAC2_TO_SGMII_RGMII | MTK_MUX | \
 	MTK_SHARED_SGMII)
 
+/* 2: GMAC2 -> XGMII */
+#define MTK_MUX_GMAC2_TO_2P5GPHY      \
+	(MTK_ETH_MUX_GMAC2_TO_2P5GPHY | MTK_MUX | MTK_INFRA)
+
 /* 0: GMACx -> GEPHY, 1: GMACx -> SGMII where x is 1 or 2 */
 #define MTK_MUX_GMAC12_TO_GEPHY_SGMII   \
 	(MTK_ETH_MUX_GMAC12_TO_GEPHY_SGMII | MTK_MUX)
 
+#define MTK_MUX_GMAC123_TO_GEPHY_SGMII   \
+	(MTK_ETH_MUX_GMAC123_TO_GEPHY_SGMII | MTK_MUX)
+
+#define MTK_MUX_GMAC123_TO_USXGMII   \
+	(MTK_ETH_MUX_GMAC123_TO_USXGMII | MTK_MUX | MTK_INFRA)
+
 #define MTK_HAS_CAPS(caps, _x)		(((caps) & (_x)) == (_x))
 
 #define MT7621_CAPS  (MTK_GMAC1_RGMII | MTK_GMAC1_TRGMII | \
@@ -1077,8 +1130,12 @@ enum mkt_eth_capabilities {
 		      MTK_MUX_GMAC12_TO_GEPHY_SGMII | MTK_QDMA | \
 		      MTK_RSTCTRL_PPE1 | MTK_SRAM)
 
-#define MT7988_CAPS  (MTK_36BIT_DMA | MTK_GDM1_ESW | MTK_QDMA | \
-		      MTK_RSTCTRL_PPE1 | MTK_RSTCTRL_PPE2 | MTK_SRAM)
+#define MT7988_CAPS  (MTK_36BIT_DMA | MTK_GDM1_ESW | MTK_GMAC1_SGMII | \
+		      MTK_GMAC2_2P5GPHY | MTK_GMAC2_SGMII | MTK_GMAC2_USXGMII | \
+		      MTK_GMAC3_SGMII | MTK_GMAC3_USXGMII | \
+		      MTK_MUX_GMAC123_TO_GEPHY_SGMII | \
+		      MTK_MUX_GMAC123_TO_USXGMII | MTK_MUX_GMAC2_TO_2P5GPHY | \
+		      MTK_QDMA | MTK_RSTCTRL_PPE1 | MTK_RSTCTRL_PPE2 | MTK_SRAM)
 
 struct mtk_tx_dma_desc_info {
 	dma_addr_t	addr;
@@ -1315,6 +1372,9 @@ struct mtk_mac {
 	struct device_node		*of_node;
 	struct phylink			*phylink;
 	struct phylink_config		phylink_config;
+	struct phylink_pcs		*sgmii_pcs;
+	struct phylink_pcs		*usxgmii_pcs;
+	struct phy			*pextp;
 	struct mtk_eth			*hw;
 	struct mtk_hw_stats		*hw_stats;
 	__be32				hwlro_ip[MTK_MAX_LRO_IP_CNT];
@@ -1421,6 +1481,19 @@ static inline u32 mtk_get_ib2_multicast_mask(struct mtk_eth *eth)
 	return MTK_FOE_IB2_MULTICAST;
 }
 
+static inline 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;
+	}
+}
+
 /* read the hardware status register */
 void mtk_stats_update_mac(struct mtk_mac *mac);
 
@@ -1429,8 +1502,10 @@ u32 mtk_r32(struct mtk_eth *eth, unsigned reg);
 u32 mtk_m32(struct mtk_eth *eth, u32 mask, u32 set, unsigned int reg);
 
 int mtk_gmac_sgmii_path_setup(struct mtk_eth *eth, int mac_id);
+int mtk_gmac_2p5gphy_path_setup(struct mtk_eth *eth, int mac_id);
 int mtk_gmac_gephy_path_setup(struct mtk_eth *eth, int mac_id);
 int mtk_gmac_rgmii_path_setup(struct mtk_eth *eth, int mac_id);
+int mtk_gmac_usxgmii_path_setup(struct mtk_eth *eth, int mac_id);
 
 int mtk_eth_offload_init(struct mtk_eth *eth);
 int mtk_eth_setup_tc(struct net_device *dev, enum tc_setup_type type,
-- 
2.43.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel