[PATCH net-next v5 0/3] riscv: sophgo: Add ethernet support for SG2044

[PATCH net-next v5 0/3] riscv: sophgo: Add ethernet support for SG2044

[Inochi Amaoto]

The ethernet controller of SG2044 is Synopsys DesignWare IP with
custom clock. Add glue layer for it.

Changed from v4:
- https://lore.kernel.org/netdev/20250209013054.816580-1-inochiama@gmail.com/
1. apply Romain's tag
2. patch 3: use device variable to replace &pdev->dev.
3. patch 3: remove unused include.
4. patch 3: make error message more useful.

Changed from v3:
- https://lore.kernel.org/netdev/20241223005843.483805-1-inochiama@gmail.com/
1. rebase for 6.14.rc1
2. remove the dependency requirement as it was already merged
   into master.

Changed from RFC:
- https://lore.kernel.org/netdev/20241101014327.513732-1-inochiama@gmail.com/
1. patch 1: apply Krzysztof' tag

Changed from v2:
- https://lore.kernel.org/netdev/20241025011000.244350-1-inochiama@gmail.com/
1. patch 1: merge the first and the second bindings patch to show the all
            compatible change.
2. patch 2: use of_device_compatible_match helper function to perform check.
2. patch 3: remove unused include and sort the left.
3. patch 3: fix wrong variable usage in sophgo_dwmac_fix_mac_speed
4. patch 3: drop unused variable in the patch.

Changed from v1:
- https://lore.kernel.org/netdev/20241021103617.653386-1-inochiama@gmail.com/
1. patch 2: remove sophgo,syscon as this mac delay is resolved.
2. patch 2: apply all the properties unconditionally.
3. patch 4: remove sophgo,syscon code as this mac delay is resolved.
4. patch 4: use the helper function to compute rgmii clock.
5. patch 4: use remove instead of remove_new for the platform driver.

Inochi Amaoto (3):
  dt-bindings: net: Add support for Sophgo SG2044 dwmac
  net: stmmac: platform: Add snps,dwmac-5.30a IP compatible string
  net: stmmac: Add glue layer for Sophgo SG2044 SoC

 .../devicetree/bindings/net/snps,dwmac.yaml   |   4 +
 .../bindings/net/sophgo,sg2044-dwmac.yaml     | 124 ++++++++++++++++++
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |  11 ++
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-sophgo.c    | 107 +++++++++++++++
 .../ethernet/stmicro/stmmac/stmmac_platform.c |  17 ++-
 6 files changed, 259 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/sophgo,sg2044-dwmac.yaml
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c

--
2.48.1

[PATCH net-next v5 1/3] dt-bindings: net: Add support for Sophgo SG2044 dwmac

[Inochi Amaoto]

The GMAC IP on SG2044 is almost a standard Synopsys DesignWare
MAC (version 5.30a) with some extra clock.

Add necessary compatible string for this device.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/net/snps,dwmac.yaml   |   4 +
 .../bindings/net/sophgo,sg2044-dwmac.yaml     | 124 ++++++++++++++++++
 2 files changed, 128 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/sophgo,sg2044-dwmac.yaml

diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index 91e75eb3f329..02ab6a9aded2 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -32,6 +32,7 @@ select:
           - snps,dwmac-4.20a
           - snps,dwmac-5.10a
           - snps,dwmac-5.20
+          - snps,dwmac-5.30a
           - snps,dwxgmac
           - snps,dwxgmac-2.10
 
@@ -98,8 +99,10 @@ properties:
         - snps,dwmac-4.20a
         - snps,dwmac-5.10a
         - snps,dwmac-5.20
+        - snps,dwmac-5.30a
         - snps,dwxgmac
         - snps,dwxgmac-2.10
+        - sophgo,sg2044-dwmac
         - starfive,jh7100-dwmac
         - starfive,jh7110-dwmac
         - thead,th1520-gmac
@@ -631,6 +634,7 @@ allOf:
                 - snps,dwmac-4.20a
                 - snps,dwmac-5.10a
                 - snps,dwmac-5.20
+                - snps,dwmac-5.30a
                 - snps,dwxgmac
                 - snps,dwxgmac-2.10
                 - st,spear600-gmac
diff --git a/Documentation/devicetree/bindings/net/sophgo,sg2044-dwmac.yaml b/Documentation/devicetree/bindings/net/sophgo,sg2044-dwmac.yaml
new file mode 100644
index 000000000000..b7e4216ea45a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/sophgo,sg2044-dwmac.yaml
@@ -0,0 +1,124 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/sophgo,sg2044-dwmac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo SG2044 DWMAC glue layer
+
+maintainers:
+  - Inochi Amaoto <inochiama@gmail.com>
+
+select:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - sophgo,sg2044-dwmac
+  required:
+    - compatible
+
+properties:
+  compatible:
+    items:
+      - const: sophgo,sg2044-dwmac
+      - const: snps,dwmac-5.30a
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: GMAC main clock
+      - description: PTP clock
+      - description: TX clock
+
+  clock-names:
+    items:
+      - const: stmmaceth
+      - const: ptp_ref
+      - const: tx
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-names:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    const: stmmaceth
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+  - resets
+  - reset-names
+
+allOf:
+  - $ref: snps,dwmac.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    ethernet@30006000 {
+      compatible = "sophgo,sg2044-dwmac", "snps,dwmac-5.30a";
+      reg = <0x30006000 0x4000>;
+      clocks = <&clk 151>, <&clk 152>, <&clk 154>;
+      clock-names = "stmmaceth", "ptp_ref", "tx";
+      interrupt-parent = <&intc>;
+      interrupts = <296 IRQ_TYPE_LEVEL_HIGH>;
+      interrupt-names = "macirq";
+      resets = <&rst 30>;
+      reset-names = "stmmaceth";
+      snps,multicast-filter-bins = <0>;
+      snps,perfect-filter-entries = <1>;
+      snps,aal;
+      snps,tso;
+      snps,txpbl = <32>;
+      snps,rxpbl = <32>;
+      snps,mtl-rx-config = <&gmac0_mtl_rx_setup>;
+      snps,mtl-tx-config = <&gmac0_mtl_tx_setup>;
+      snps,axi-config = <&gmac0_stmmac_axi_setup>;
+      status = "disabled";
+
+      gmac0_mtl_rx_setup: rx-queues-config {
+        snps,rx-queues-to-use = <8>;
+        snps,rx-sched-wsp;
+        queue0 {};
+        queue1 {};
+        queue2 {};
+        queue3 {};
+        queue4 {};
+        queue5 {};
+        queue6 {};
+        queue7 {};
+      };
+
+      gmac0_mtl_tx_setup: tx-queues-config {
+        snps,tx-queues-to-use = <8>;
+        queue0 {};
+        queue1 {};
+        queue2 {};
+        queue3 {};
+        queue4 {};
+        queue5 {};
+        queue6 {};
+        queue7 {};
+      };
+
+      gmac0_stmmac_axi_setup: stmmac-axi-config {
+        snps,blen = <16 8 4 0 0 0 0>;
+        snps,wr_osr_lmt = <1>;
+        snps,rd_osr_lmt = <2>;
+      };
+    };
-- 
2.48.1

[PATCH net-next v5 2/3] net: stmmac: platform: Add snps,dwmac-5.30a IP compatible string

[Inochi Amaoto]

Add "snps,dwmac-5.30a" compatible string for 5.30a version that can avoid
to define some platform data in the glue layer.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>
---
 .../ethernet/stmicro/stmmac/stmmac_platform.c   | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index d0e61aa1a495..8dc3bd6946c6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -405,6 +405,17 @@ static int stmmac_of_get_mac_mode(struct device_node *np)
 	return -ENODEV;
 }
 
+/* Compatible string array for all gmac4 devices */
+static const char * const stmmac_gmac4_compats[] = {
+	"snps,dwmac-4.00",
+	"snps,dwmac-4.10a",
+	"snps,dwmac-4.20a",
+	"snps,dwmac-5.10a",
+	"snps,dwmac-5.20",
+	"snps,dwmac-5.30a",
+	NULL
+};
+
 /**
  * stmmac_probe_config_dt - parse device-tree driver parameters
  * @pdev: platform_device structure
@@ -538,11 +549,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 		plat->pmt = 1;
 	}
 
-	if (of_device_is_compatible(np, "snps,dwmac-4.00") ||
-	    of_device_is_compatible(np, "snps,dwmac-4.10a") ||
-	    of_device_is_compatible(np, "snps,dwmac-4.20a") ||
-	    of_device_is_compatible(np, "snps,dwmac-5.10a") ||
-	    of_device_is_compatible(np, "snps,dwmac-5.20")) {
+	if (of_device_compatible_match(np, stmmac_gmac4_compats)) {
 		plat->has_gmac4 = 1;
 		plat->has_gmac = 0;
 		plat->pmt = 1;
-- 
2.48.1

Re: [PATCH net-next v5 2/3] net: stmmac: platform: Add snps,dwmac-5.30a IP compatible string

[Andrew Lunn]

On Sun, Feb 16, 2025 at 08:39:50PM +0800, Inochi Amaoto wrote:
> Add "snps,dwmac-5.30a" compatible string for 5.30a version that can avoid
> to define some platform data in the glue layer.
> 
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>

Ideally, this would be two patches, one adding the
stmmac_gmac4_compats[] with the existing compatibles, and then a patch
adding snps,dwmac-5.30a. Logically, these are different, so two
patches.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v5 2/3] net: stmmac: platform: Add snps,dwmac-5.30a IP compatible string

[Inochi Amaoto]

On Sun, Feb 16, 2025 at 05:59:31PM +0100, Andrew Lunn wrote:
> On Sun, Feb 16, 2025 at 08:39:50PM +0800, Inochi Amaoto wrote:
> > Add "snps,dwmac-5.30a" compatible string for 5.30a version that can avoid
> > to define some platform data in the glue layer.
> > 
> > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> > Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>
> 
> Ideally, this would be two patches, one adding the
> stmmac_gmac4_compats[] with the existing compatibles, and then a patch
> adding snps,dwmac-5.30a. Logically, these are different, so two
> patches.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
>     Andrew
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
>     Andrew

I will separate them and apply the tag, thanks for reviewing.

Regards,
Inochi

[PATCH net-next v5 3/3] net: stmmac: Add glue layer for Sophgo SG2044 SoC

[Inochi Amaoto]

Adds Sophgo dwmac driver support on the Sophgo SG2044 SoC.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |  11 ++
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-sophgo.c    | 107 ++++++++++++++++++
 3 files changed, 119 insertions(+)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 4cc85a36a1ab..b6ff51e1ebce 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -181,6 +181,17 @@ config DWMAC_SOCFPGA
 	  for the stmmac device driver. This driver is used for
 	  arria5 and cyclone5 FPGA SoCs.
 
+config DWMAC_SOPHGO
+	tristate "Sophgo dwmac support"
+	depends on OF && (ARCH_SOPHGO || COMPILE_TEST)
+	default m if ARCH_SOPHGO
+	help
+	  Support for ethernet controllers on Sophgo RISC-V SoCs
+
+	  This selects the Sophgo SoC specific glue layer support
+	  for the stmmac device driver. This driver is used for the
+	  ethernet controllers on various Sophgo SoCs.
+
 config DWMAC_STARFIVE
 	tristate "StarFive dwmac support"
 	depends on OF && (ARCH_STARFIVE || COMPILE_TEST)
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index b26f0e79c2b3..594883fb4164 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_DWMAC_ROCKCHIP)	+= dwmac-rk.o
 obj-$(CONFIG_DWMAC_RZN1)	+= dwmac-rzn1.o
 obj-$(CONFIG_DWMAC_S32)		+= dwmac-s32.o
 obj-$(CONFIG_DWMAC_SOCFPGA)	+= dwmac-altr-socfpga.o
+obj-$(CONFIG_DWMAC_SOPHGO)	+= dwmac-sophgo.o
 obj-$(CONFIG_DWMAC_STARFIVE)	+= dwmac-starfive.o
 obj-$(CONFIG_DWMAC_STI)		+= dwmac-sti.o
 obj-$(CONFIG_DWMAC_STM32)	+= dwmac-stm32.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c
new file mode 100644
index 000000000000..93ea280da6bd
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Sophgo DWMAC platform driver
+ *
+ * Copyright (C) 2024 Inochi Amaoto <inochiama@gmail.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+
+#include "stmmac_platform.h"
+
+struct sophgo_dwmac {
+	struct device *dev;
+	struct clk *clk_tx;
+};
+
+static void sophgo_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
+{
+	struct sophgo_dwmac *dwmac = priv;
+	long rate;
+	int ret;
+
+	rate = rgmii_clock(speed);
+	if (rate < 0) {
+		dev_err(dwmac->dev, "invalid speed %u\n", speed);
+		return;
+	}
+
+	ret = clk_set_rate(dwmac->clk_tx, rate);
+	if (ret)
+		dev_err(dwmac->dev, "failed to set tx rate %ld: %pe\n",
+			rate, ERR_PTR(ret));
+}
+
+static int sophgo_sg2044_dwmac_init(struct platform_device *pdev,
+				    struct plat_stmmacenet_data *plat_dat,
+				    struct stmmac_resources *stmmac_res)
+{
+	struct sophgo_dwmac *dwmac;
+
+	dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
+	if (!dwmac)
+		return -ENOMEM;
+
+	dwmac->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx");
+	if (IS_ERR(dwmac->clk_tx))
+		return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx),
+				     "failed to get tx clock\n");
+
+	dwmac->dev = &pdev->dev;
+	plat_dat->bsp_priv = dwmac;
+	plat_dat->flags |= STMMAC_FLAG_SPH_DISABLE;
+	plat_dat->fix_mac_speed = sophgo_dwmac_fix_mac_speed;
+	plat_dat->multicast_filter_bins = 0;
+	plat_dat->unicast_filter_entries = 1;
+
+	return 0;
+}
+
+static int sophgo_dwmac_probe(struct platform_device *pdev)
+{
+	struct plat_stmmacenet_data *plat_dat;
+	struct stmmac_resources stmmac_res;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to get platform resources\n");
+
+	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	if (IS_ERR(plat_dat))
+		return dev_err_probe(dev, PTR_ERR(plat_dat),
+				     "failed to parse DT parameters\n");
+
+	ret = sophgo_sg2044_dwmac_init(pdev, plat_dat, &stmmac_res);
+	if (ret)
+		return ret;
+
+	return stmmac_dvr_probe(dev, plat_dat, &stmmac_res);
+}
+
+static const struct of_device_id sophgo_dwmac_match[] = {
+	{ .compatible = "sophgo,sg2044-dwmac" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sophgo_dwmac_match);
+
+static struct platform_driver sophgo_dwmac_driver = {
+	.probe  = sophgo_dwmac_probe,
+	.remove = stmmac_pltfr_remove,
+	.driver = {
+		.name = "sophgo-dwmac",
+		.pm = &stmmac_pltfr_pm_ops,
+		.of_match_table = sophgo_dwmac_match,
+	},
+};
+module_platform_driver(sophgo_dwmac_driver);
+
+MODULE_AUTHOR("Inochi Amaoto <inochiama@gmail.com>");
+MODULE_DESCRIPTION("Sophgo DWMAC platform driver");
+MODULE_LICENSE("GPL");
-- 
2.48.1

Re: [PATCH net-next v5 3/3] net: stmmac: Add glue layer for Sophgo SG2044 SoC

[Russell King (Oracle)]

On Sun, Feb 16, 2025 at 08:39:51PM +0800, Inochi Amaoto wrote:
> +static void sophgo_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
> +{
> +	struct sophgo_dwmac *dwmac = priv;
> +	long rate;
> +	int ret;
> +
> +	rate = rgmii_clock(speed);
> +	if (rate < 0) {
> +		dev_err(dwmac->dev, "invalid speed %u\n", speed);
> +		return;
> +	}
> +
> +	ret = clk_set_rate(dwmac->clk_tx, rate);
> +	if (ret)
> +		dev_err(dwmac->dev, "failed to set tx rate %ld: %pe\n",
> +			rate, ERR_PTR(ret));
> +}

There are a bunch of other platform support in stmmac that are doing
the same:

dwmac-s32.c is virtually identical
dwmac-imx.c does the same, although has some pre-conditions
dwmac-dwc-qos-eth.c is virually identical but the two steps are split
  across a bunch of register writes
dwmac-starfive.c looks the same
dwmac-rk.c also
dwmac-intel-plat.c also

So, I wonder whether either this should be a helper, or whether core
code should be doing this. Maybe something to look at as a part of
this patch submission?

We really need to stop writing the same code in different ways and
think more about how to reuse the code that's already present!

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

Re: [PATCH net-next v5 3/3] net: stmmac: Add glue layer for Sophgo SG2044 SoC

[Andrew Lunn]

On Sun, Feb 16, 2025 at 03:47:18PM +0000, Russell King (Oracle) wrote:
> On Sun, Feb 16, 2025 at 08:39:51PM +0800, Inochi Amaoto wrote:
> > +static void sophgo_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
> > +{
> > +	struct sophgo_dwmac *dwmac = priv;
> > +	long rate;
> > +	int ret;
> > +
> > +	rate = rgmii_clock(speed);
> > +	if (rate < 0) {
> > +		dev_err(dwmac->dev, "invalid speed %u\n", speed);
> > +		return;
> > +	}
> > +
> > +	ret = clk_set_rate(dwmac->clk_tx, rate);
> > +	if (ret)
> > +		dev_err(dwmac->dev, "failed to set tx rate %ld: %pe\n",
> > +			rate, ERR_PTR(ret));
> > +}
> 
> There are a bunch of other platform support in stmmac that are doing
> the same:
> 
> dwmac-s32.c is virtually identical
> dwmac-imx.c does the same, although has some pre-conditions
> dwmac-dwc-qos-eth.c is virually identical but the two steps are split
>   across a bunch of register writes
> dwmac-starfive.c looks the same
> dwmac-rk.c also
> dwmac-intel-plat.c also
> 
> So, I wonder whether either this should be a helper, or whether core
> code should be doing this. Maybe something to look at as a part of
> this patch submission?

Inochi, please could you look at the datasheet for this IP. Is the
transmit clock a part of the IP? Can we expect all devices integrating
this IP to have such a clock? That would be a good indicator the clock
handling should be moved into the core.

	Andrew

Re: [PATCH net-next v5 3/3] net: stmmac: Add glue layer for Sophgo SG2044 SoC

[Inochi Amaoto]

On Sun, Feb 16, 2025 at 06:07:05PM +0100, Andrew Lunn wrote:
> On Sun, Feb 16, 2025 at 03:47:18PM +0000, Russell King (Oracle) wrote:
> > On Sun, Feb 16, 2025 at 08:39:51PM +0800, Inochi Amaoto wrote:
> > > +static void sophgo_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
> > > +{
> > > +	struct sophgo_dwmac *dwmac = priv;
> > > +	long rate;
> > > +	int ret;
> > > +
> > > +	rate = rgmii_clock(speed);
> > > +	if (rate < 0) {
> > > +		dev_err(dwmac->dev, "invalid speed %u\n", speed);
> > > +		return;
> > > +	}
> > > +
> > > +	ret = clk_set_rate(dwmac->clk_tx, rate);
> > > +	if (ret)
> > > +		dev_err(dwmac->dev, "failed to set tx rate %ld: %pe\n",
> > > +			rate, ERR_PTR(ret));
> > > +}
> > 
> > There are a bunch of other platform support in stmmac that are doing
> > the same:
> > 
> > dwmac-s32.c is virtually identical
> > dwmac-imx.c does the same, although has some pre-conditions
> > dwmac-dwc-qos-eth.c is virually identical but the two steps are split
> >   across a bunch of register writes
> > dwmac-starfive.c looks the same
> > dwmac-rk.c also
> > dwmac-intel-plat.c also
> > 
> > So, I wonder whether either this should be a helper, or whether core
> > code should be doing this. Maybe something to look at as a part of
> > this patch submission?
> 
> Inochi, please could you look at the datasheet for this IP. Is the
> transmit clock a part of the IP? 

I checked the ip databook and found it is part of the RGMII clock.
Also, I found RGMII also contains a rx clock following this quirk.

> Can we expect all devices integrating this IP to have such a clock?
> That would be a good indicator the clock handling should be moved
> into the core.
> 

I am not sure all whether devices has this clock, but it appears in
the databook. So I think it is possible to move this in the core so
any platform with these clock can reuse it.

Regards,
Inochi

Re: [PATCH net-next v5 3/3] net: stmmac: Add glue layer for Sophgo SG2044 SoC

[Andrew Lunn]

> I am not sure all whether devices has this clock, but it appears in
> the databook. So I think it is possible to move this in the core so
> any platform with these clock can reuse it.

Great

The next problem will be, has everybody called it the same thing in
DT. Since there has been a lot of cut/paste, maybe they have, by
accident.

Thanks
	Andrew

Re: [PATCH net-next v5 3/3] net: stmmac: Add glue layer for Sophgo SG2044 SoC

[Russell King (Oracle)]

On Mon, Feb 17, 2025 at 02:25:33PM +0100, Andrew Lunn wrote:
> > I am not sure all whether devices has this clock, but it appears in
> > the databook. So I think it is possible to move this in the core so
> > any platform with these clock can reuse it.
> 
> Great
> 
> The next problem will be, has everybody called it the same thing in
> DT. Since there has been a lot of cut/paste, maybe they have, by
> accident.

Tegra186: "tx"
imx: "tx"
intel: "tx_clk"
rk: "clk_mac_speed"
s32: "tx"
starfive: "tx"
sti: "sti-ethclk"

so 50% have settled on "tx" and the rest are doing their own thing, and
that horse has already bolted.

I have some ideas on sorting this out, and I'm working on some patches
today.

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

Re: [PATCH net-next v5 3/3] net: stmmac: Add glue layer for Sophgo SG2044 SoC

[Inochi Amaoto]

On Mon, Feb 17, 2025 at 02:10:50PM +0000, Russell King (Oracle) wrote:
> On Mon, Feb 17, 2025 at 02:25:33PM +0100, Andrew Lunn wrote:
> > > I am not sure all whether devices has this clock, but it appears in
> > > the databook. So I think it is possible to move this in the core so
> > > any platform with these clock can reuse it.
> > 
> > Great
> > 
> > The next problem will be, has everybody called it the same thing in
> > DT. Since there has been a lot of cut/paste, maybe they have, by
> > accident.
> 
> Tegra186: "tx"
> imx: "tx"
> intel: "tx_clk"
> rk: "clk_mac_speed"
> s32: "tx"
> starfive: "tx"
> sti: "sti-ethclk"
> 

The dwc-qos-eth also use clock name "tx", but set the clock with
extra calibration logic.

> so 50% have settled on "tx" and the rest are doing their own thing, and
> that horse has already bolted.
> 

The "rx" clock in s32 also uses the same logic. I think the core also
needs to take it, as this rx clock is also mentioned in the databook.

> I have some ideas on sorting this out, and I'm working on some patches
> today.

Great, Could you cc me when you submit them? So I can take it and
change my series.

Regards,
Inochi

Re: [PATCH net-next v5 3/3] net: stmmac: Add glue layer for Sophgo SG2044 SoC

[Russell King (Oracle)]

On Tue, Feb 18, 2025 at 06:50:24AM +0800, Inochi Amaoto wrote:
> On Mon, Feb 17, 2025 at 02:10:50PM +0000, Russell King (Oracle) wrote:
> > On Mon, Feb 17, 2025 at 02:25:33PM +0100, Andrew Lunn wrote:
> > > > I am not sure all whether devices has this clock, but it appears in
> > > > the databook. So I think it is possible to move this in the core so
> > > > any platform with these clock can reuse it.
> > > 
> > > Great
> > > 
> > > The next problem will be, has everybody called it the same thing in
> > > DT. Since there has been a lot of cut/paste, maybe they have, by
> > > accident.
> > 
> > Tegra186: "tx"
> > imx: "tx"
> > intel: "tx_clk"
> > rk: "clk_mac_speed"
> > s32: "tx"
> > starfive: "tx"
> > sti: "sti-ethclk"
> > 
> 
> The dwc-qos-eth also use clock name "tx", but set the clock with
> extra calibration logic.

Yep, that's what I meant by "Tegra186" above.

> > so 50% have settled on "tx" and the rest are doing their own thing, and
> > that horse has already bolted.
> > 
> 
> The "rx" clock in s32 also uses the same logic. I think the core also
> needs to take it, as this rx clock is also mentioned in the databook.

The "rx" clock on s32 seems to only be set to 125MHz, and the driver
seems to be limited to RGMII.

This seems weird as the receive clock is supposed to be supplied by the
PHY, and is recovered from the media (and thus will be 2.5, 25 or
125MHz as determined by the PHY.) So, I'm not sure that the s32 "rx"
clock is really the clk_rx_i clock supplied to the DWMAC core.

Certainly on stm32mp151, it states that ETH_RX_CLK in RGMII mode will
be 2.5, 25 or 125MHz provided by the PHY, and the clock tree indicates
that ETH_RX_CLK in RGMII mode will be routed directly to the clk_rx_i
input on the DWMAC(4) core.

> > I have some ideas on sorting this out, and I'm working on some patches
> > today.
> 
> Great, Could you cc me when you submit them? So I can take it and
> change my series.

Will do - I'm almost at that point, I have three other cleanup patches
I will be sending before hand, so I'll send those out and then this
series as RFC.

The only platform drivers I've left with a call to rgmii_clock() are
sti, imx (for imx93 only as that does extra fiddling after setting the
clock and I'm not sure if there's an ordering dependency there) and
the rk platforms.

Five platforms converted, three not, and hopefully your platform can
also use the helper as well!

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

Re: [PATCH net-next v5 3/3] net: stmmac: Add glue layer for Sophgo SG2044 SoC

[Inochi Amaoto]

On Mon, Feb 17, 2025 at 11:21:28PM +0000, Russell King (Oracle) wrote:
> On Tue, Feb 18, 2025 at 06:50:24AM +0800, Inochi Amaoto wrote:
> > On Mon, Feb 17, 2025 at 02:10:50PM +0000, Russell King (Oracle) wrote:
> > > On Mon, Feb 17, 2025 at 02:25:33PM +0100, Andrew Lunn wrote:
> > > > > I am not sure all whether devices has this clock, but it appears in
> > > > > the databook. So I think it is possible to move this in the core so
> > > > > any platform with these clock can reuse it.
> > > > 
> > > > Great
> > > > 
> > > > The next problem will be, has everybody called it the same thing in
> > > > DT. Since there has been a lot of cut/paste, maybe they have, by
> > > > accident.
> > > 
> > > Tegra186: "tx"
> > > imx: "tx"
> > > intel: "tx_clk"
> > > rk: "clk_mac_speed"
> > > s32: "tx"
> > > starfive: "tx"
> > > sti: "sti-ethclk"
> > > 
> > 
> > The dwc-qos-eth also use clock name "tx", but set the clock with
> > extra calibration logic.
> 
> Yep, that's what I meant by "Tegra186" above.
> 
> > > so 50% have settled on "tx" and the rest are doing their own thing, and
> > > that horse has already bolted.
> > > 
> > 
> > The "rx" clock in s32 also uses the same logic. I think the core also
> > needs to take it, as this rx clock is also mentioned in the databook.
> 
> The "rx" clock on s32 seems to only be set to 125MHz, and the driver
> seems to be limited to RGMII.
> 
> This seems weird as the receive clock is supposed to be supplied by the
> PHY, and is recovered from the media (and thus will be 2.5, 25 or
> 125MHz as determined by the PHY.) So, I'm not sure that the s32 "rx"
> clock is really the clk_rx_i clock supplied to the DWMAC core.
> 
> Certainly on stm32mp151, it states that ETH_RX_CLK in RGMII mode will
> be 2.5, 25 or 125MHz provided by the PHY, and the clock tree indicates
> that ETH_RX_CLK in RGMII mode will be routed directly to the clk_rx_i
> input on the DWMAC(4) core.
> 

RGMII is not the problem. The databook says the RGMII clock (rx/tx)
follows this set rate logic. 

For other things, I agree with you. A fixed "rx" clock does reach the
limit of what I know. And the databook told nothing about it. As we
can not determine the rx clock of s32 and it may be set for the phy,
it will be better to not move it into the core.

> > > I have some ideas on sorting this out, and I'm working on some patches
> > > today.
> > 
> > Great, Could you cc me when you submit them? So I can take it and
> > change my series.
> 
> Will do - I'm almost at that point, I have three other cleanup patches
> I will be sending before hand, so I'll send those out and then this
> series as RFC.
> 
> The only platform drivers I've left with a call to rgmii_clock() are
> sti, imx (for imx93 only as that does extra fiddling after setting the
> clock and I'm not sure if there's an ordering dependency there) and
> the rk platforms.
> 
> Five platforms converted, three not, and hopefully your platform can
> also use the helper as well!
> 

I think my platform can fit. Thanks for your effort.

Regards,
Inochi

Re: [PATCH net-next v5 3/3] net: stmmac: Add glue layer for Sophgo SG2044 SoC

[Russell King (Oracle)]

On Tue, Feb 18, 2025 at 09:01:59AM +0800, Inochi Amaoto wrote:
> On Mon, Feb 17, 2025 at 11:21:28PM +0000, Russell King (Oracle) wrote:
> > On Tue, Feb 18, 2025 at 06:50:24AM +0800, Inochi Amaoto wrote:
> > > On Mon, Feb 17, 2025 at 02:10:50PM +0000, Russell King (Oracle) wrote:
> > > > On Mon, Feb 17, 2025 at 02:25:33PM +0100, Andrew Lunn wrote:
> > > > > > I am not sure all whether devices has this clock, but it appears in
> > > > > > the databook. So I think it is possible to move this in the core so
> > > > > > any platform with these clock can reuse it.
> > > > > 
> > > > > Great
> > > > > 
> > > > > The next problem will be, has everybody called it the same thing in
> > > > > DT. Since there has been a lot of cut/paste, maybe they have, by
> > > > > accident.
> > > > 
> > > > Tegra186: "tx"
> > > > imx: "tx"
> > > > intel: "tx_clk"
> > > > rk: "clk_mac_speed"
> > > > s32: "tx"
> > > > starfive: "tx"
> > > > sti: "sti-ethclk"
> > > > 
> > > 
> > > The dwc-qos-eth also use clock name "tx", but set the clock with
> > > extra calibration logic.
> > 
> > Yep, that's what I meant by "Tegra186" above.
> > 
> > > > so 50% have settled on "tx" and the rest are doing their own thing, and
> > > > that horse has already bolted.
> > > > 
> > > 
> > > The "rx" clock in s32 also uses the same logic. I think the core also
> > > needs to take it, as this rx clock is also mentioned in the databook.
> > 
> > The "rx" clock on s32 seems to only be set to 125MHz, and the driver
> > seems to be limited to RGMII.
> > 
> > This seems weird as the receive clock is supposed to be supplied by the
> > PHY, and is recovered from the media (and thus will be 2.5, 25 or
> > 125MHz as determined by the PHY.) So, I'm not sure that the s32 "rx"
> > clock is really the clk_rx_i clock supplied to the DWMAC core.
> > 
> > Certainly on stm32mp151, it states that ETH_RX_CLK in RGMII mode will
> > be 2.5, 25 or 125MHz provided by the PHY, and the clock tree indicates
> > that ETH_RX_CLK in RGMII mode will be routed directly to the clk_rx_i
> > input on the DWMAC(4) core.
> > 
> 
> RGMII is not the problem. The databook says the RGMII clock (rx/tx)
> follows this set rate logic. 

Sorry, I find this ambiguous. "This" doesn't tell me whether you are
referring to either what s32 does (setting the "rx" clock to 125MHz
only) or what RGMII spec says about RX_CLK (which is that it comes from
the PHY and is 2.5/25/125MHz) which stm32mp151 agrees with and feeds
the PHY's RX_CLK to the clk_rx_i inputs on the DWMAC in RGMII, GMII
and MII modes.

clk_rx_i comes through a bunch of muxes on stm32mp151. When the clock
tree is configured for RMII mode, the rate on clk_rx_i depends on the
MAC speed (10/100Mbps).

This suggests as far as the core is concerned, the clock supplied as
clk_rx_i isn't a fixed rate clock but depends on the speed just like
the transmit clock.

> For other things, I agree with you. A fixed "rx" clock does reach the
> limit of what I know. And the databook told nothing about it. As we
> can not determine the rx clock of s32 and it may be set for the phy,
> it will be better to not move it into the core.

I'm intending to leave s32's rx clock alone for this reason as it does
not match what I expect. Maybe on s32 there is a bunch of dividers
which are selected by the mac_speed_o signals from the core to divide
the 125MHz clock down to 25 or 2.5MHz for 100 and 10Mbps respectively.
As I don't know, it's safer that I leave it alone as that means the
"rx" clock used there is not clk_rx_i.

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

Re: [PATCH net-next v5 3/3] net: stmmac: Add glue layer for Sophgo SG2044 SoC

[Inochi Amaoto]

On Tue, Feb 18, 2025 at 10:40:14AM +0000, Russell King (Oracle) wrote:
> On Tue, Feb 18, 2025 at 09:01:59AM +0800, Inochi Amaoto wrote:
> > On Mon, Feb 17, 2025 at 11:21:28PM +0000, Russell King (Oracle) wrote:
> > > On Tue, Feb 18, 2025 at 06:50:24AM +0800, Inochi Amaoto wrote:
> > > > On Mon, Feb 17, 2025 at 02:10:50PM +0000, Russell King (Oracle) wrote:
> > > > > On Mon, Feb 17, 2025 at 02:25:33PM +0100, Andrew Lunn wrote:
> > > > > > > I am not sure all whether devices has this clock, but it appears in
> > > > > > > the databook. So I think it is possible to move this in the core so
> > > > > > > any platform with these clock can reuse it.
> > > > > > 
> > > > > > Great
> > > > > > 
> > > > > > The next problem will be, has everybody called it the same thing in
> > > > > > DT. Since there has been a lot of cut/paste, maybe they have, by
> > > > > > accident.
> > > > > 
> > > > > Tegra186: "tx"
> > > > > imx: "tx"
> > > > > intel: "tx_clk"
> > > > > rk: "clk_mac_speed"
> > > > > s32: "tx"
> > > > > starfive: "tx"
> > > > > sti: "sti-ethclk"
> > > > > 
> > > > 
> > > > The dwc-qos-eth also use clock name "tx", but set the clock with
> > > > extra calibration logic.
> > > 
> > > Yep, that's what I meant by "Tegra186" above.
> > > 
> > > > > so 50% have settled on "tx" and the rest are doing their own thing, and
> > > > > that horse has already bolted.
> > > > > 
> > > > 
> > > > The "rx" clock in s32 also uses the same logic. I think the core also
> > > > needs to take it, as this rx clock is also mentioned in the databook.
> > > 
> > > The "rx" clock on s32 seems to only be set to 125MHz, and the driver
> > > seems to be limited to RGMII.
> > > 
> > > This seems weird as the receive clock is supposed to be supplied by the
> > > PHY, and is recovered from the media (and thus will be 2.5, 25 or
> > > 125MHz as determined by the PHY.) So, I'm not sure that the s32 "rx"
> > > clock is really the clk_rx_i clock supplied to the DWMAC core.
> > > 
> > > Certainly on stm32mp151, it states that ETH_RX_CLK in RGMII mode will
> > > be 2.5, 25 or 125MHz provided by the PHY, and the clock tree indicates
> > > that ETH_RX_CLK in RGMII mode will be routed directly to the clk_rx_i
> > > input on the DWMAC(4) core.
> > > 
> > 
> > RGMII is not the problem. The databook says the RGMII clock (rx/tx)
> > follows this set rate logic. 
> 
> Sorry, I find this ambiguous. "This" doesn't tell me whether you are
> referring to either what s32 does (setting the "rx" clock to 125MHz
> only) or what RGMII spec says about RX_CLK (which is that it comes from
> the PHY and is 2.5/25/125MHz) which stm32mp151 agrees with and feeds
> the PHY's RX_CLK to the clk_rx_i inputs on the DWMAC in RGMII, GMII
> and MII modes.
> 

What I said follows the second, the clock is set at 2.5/25/125MHz
with speed at 10/100/1000Mbps. The only thing I can refer to is the
ip databook.

> clk_rx_i comes through a bunch of muxes on stm32mp151. When the clock
> tree is configured for RMII mode, the rate on clk_rx_i depends on the
> MAC speed (10/100Mbps).
> 

OK, I have no problem and find some descriptions related to this in
the databook.

> This suggests as far as the core is concerned, the clock supplied as
> clk_rx_i isn't a fixed rate clock but depends on the speed just like
> the transmit clock.
> 

This is what I want to say. clk_rx_i is not fixed but the
s32 uses it as a fixed one (This is the thing I felt weird).

In fact, Non-fixed clk_rx_i is why I suggested adding the rx
clock to the core at first. Since the drive may not use rx
clock as the databook says, it is good to leave it alone.

At last, it seems like that I need to improve my statement.
I am sorry for it.

> > For other things, I agree with you. A fixed "rx" clock does reach the
> > limit of what I know. And the databook told nothing about it. As we
> > can not determine the rx clock of s32 and it may be set for the phy,
> > it will be better to not move it into the core.
> 
> I'm intending to leave s32's rx clock alone for this reason as it does
> not match what I expect. Maybe on s32 there is a bunch of dividers
> which are selected by the mac_speed_o signals from the core to divide
> the 125MHz clock down to 25 or 2.5MHz for 100 and 10Mbps respectively.
> As I don't know, it's safer that I leave it alone as that means the
> "rx" clock used there is not clk_rx_i.
> 

Thanks for your explanation. This is OK for me.

Regards,
Inochi

Re: [PATCH net-next v5 3/3] net: stmmac: Add glue layer for Sophgo SG2044 SoC

[Inochi Amaoto]

On Tue, Feb 18, 2025 at 07:34:47PM +0800, Inochi Amaoto wrote:
> On Tue, Feb 18, 2025 at 10:40:14AM +0000, Russell King (Oracle) wrote:
> > On Tue, Feb 18, 2025 at 09:01:59AM +0800, Inochi Amaoto wrote:
> > > On Mon, Feb 17, 2025 at 11:21:28PM +0000, Russell King (Oracle) wrote:
> > > > On Tue, Feb 18, 2025 at 06:50:24AM +0800, Inochi Amaoto wrote:
> > > > > On Mon, Feb 17, 2025 at 02:10:50PM +0000, Russell King (Oracle) wrote:
> > > > > > On Mon, Feb 17, 2025 at 02:25:33PM +0100, Andrew Lunn wrote:
> > > > > > > > I am not sure all whether devices has this clock, but it appears in
> > > > > > > > the databook. So I think it is possible to move this in the core so
> > > > > > > > any platform with these clock can reuse it.
> > > > > > > 
> > > > > > > Great
> > > > > > > 
> > > > > > > The next problem will be, has everybody called it the same thing in
> > > > > > > DT. Since there has been a lot of cut/paste, maybe they have, by
> > > > > > > accident.
> > > > > > 
> > > > > > Tegra186: "tx"
> > > > > > imx: "tx"
> > > > > > intel: "tx_clk"
> > > > > > rk: "clk_mac_speed"
> > > > > > s32: "tx"
> > > > > > starfive: "tx"
> > > > > > sti: "sti-ethclk"
> > > > > > 
> > > > > 
> > > > > The dwc-qos-eth also use clock name "tx", but set the clock with
> > > > > extra calibration logic.
> > > > 
> > > > Yep, that's what I meant by "Tegra186" above.
> > > > 
> > > > > > so 50% have settled on "tx" and the rest are doing their own thing, and
> > > > > > that horse has already bolted.
> > > > > > 
> > > > > 
> > > > > The "rx" clock in s32 also uses the same logic. I think the core also
> > > > > needs to take it, as this rx clock is also mentioned in the databook.
> > > > 
> > > > The "rx" clock on s32 seems to only be set to 125MHz, and the driver
> > > > seems to be limited to RGMII.
> > > > 
> > > > This seems weird as the receive clock is supposed to be supplied by the
> > > > PHY, and is recovered from the media (and thus will be 2.5, 25 or
> > > > 125MHz as determined by the PHY.) So, I'm not sure that the s32 "rx"
> > > > clock is really the clk_rx_i clock supplied to the DWMAC core.
> > > > 
> > > > Certainly on stm32mp151, it states that ETH_RX_CLK in RGMII mode will
> > > > be 2.5, 25 or 125MHz provided by the PHY, and the clock tree indicates
> > > > that ETH_RX_CLK in RGMII mode will be routed directly to the clk_rx_i
> > > > input on the DWMAC(4) core.
> > > > 
> > > 
> > > RGMII is not the problem. The databook says the RGMII clock (rx/tx)
> > > follows this set rate logic. 
> > 
> > Sorry, I find this ambiguous. "This" doesn't tell me whether you are
> > referring to either what s32 does (setting the "rx" clock to 125MHz
> > only) or what RGMII spec says about RX_CLK (which is that it comes from
> > the PHY and is 2.5/25/125MHz) which stm32mp151 agrees with and feeds
> > the PHY's RX_CLK to the clk_rx_i inputs on the DWMAC in RGMII, GMII
> > and MII modes.
> > 
> 
> What I said follows the second, the clock is set at 2.5/25/125MHz
> with speed at 10/100/1000Mbps. The only thing I can refer to is the
> ip databook.
> 
> > clk_rx_i comes through a bunch of muxes on stm32mp151. When the clock
> > tree is configured for RMII mode, the rate on clk_rx_i depends on the
> > MAC speed (10/100Mbps).
> > 
> 
> OK, I have no problem and find some descriptions related to this in
> the databook.
> 
> > This suggests as far as the core is concerned, the clock supplied as
> > clk_rx_i isn't a fixed rate clock but depends on the speed just like
> > the transmit clock.
> > 
> 
> This is what I want to say. clk_rx_i is not fixed but the
> s32 uses it as a fixed one (This is the thing I felt weird).
> 

> In fact, Non-fixed clk_rx_i is why I suggested adding the rx
> clock to the core at first. Since the drive may not use rx
> clock as the databook says, it is good to leave it alone.

Please ignore my wrong point, I mistake the direction of the
rx clock. It is not provided by the mac, but the phy.

Regards,
Inochi