[PATCH v3 0/4] Move Loongson1 MAC arch-code to the driver dir

[PATCH v3 0/4] Move Loongson1 MAC arch-code to the driver dir

[Keguang Zhang]

In order to convert Loongson1 MAC platform devices to the devicetree
nodes, Loongson1 MAC arch-code should be moved to the driver dir.
Add dt-binding document and update MAINTAINERS file accordingly. 
    
In other words, this patchset is a preparation for converting
Loongson1 platform devices to devicetree.

Changelog
V2 -> V3: Split the DT-schema file into loongson,ls1b-gmac.yaml
          and loongson,ls1c-emac.yaml (suggested by Serge Semin)
          Change the compatibles to loongson,ls1b-gmac and loongson,ls1c-emac
          Rename loongson,dwmac-syscon to loongson,ls1-syscon
          Amend the title
          Add description
          Change compatibles back to loongson,ls1b-syscon
          and loongson,ls1c-syscon
          Determine the device ID by physical
          base address(suggested by Serge Semin)
          Use regmap instead of regmap fields
          Use syscon_regmap_lookup_by_phandle()
          Some minor fixes
          Update the entries of MAINTAINERS
V1 -> V2: Leave the Ethernet platform data for now
          Make the syscon compatibles more specific
          Fix "clock-names" and "interrupt-names" property
          Rename the syscon property to "loongson,dwmac-syscon"
          Drop "phy-handle" and "phy-mode" requirement
          Revert adding loongson,ls1b-dwmac/loongson,ls1c-dwmac
          to snps,dwmac.yaml
          Fix the build errors due to CONFIG_OF being unset
          Change struct reg_field definitions to const
          Rename the syscon property to "loongson,dwmac-syscon"
          Add MII PHY mode for LS1C
          Improve the commit message

Keguang Zhang (4):
  dt-bindings: mfd: syscon: Add compatibles for Loongson-1 syscon
  dt-bindings: net: Add Loongson-1 Ethernet Controller
  net: stmmac: Add glue layer for Loongson-1 SoC
  MAINTAINERS: Update MIPS/LOONGSON1 entry

 .../devicetree/bindings/mfd/syscon.yaml       |   2 +
 .../bindings/net/loongson,ls1b-gmac.yaml      | 115 +++++++++
 .../bindings/net/loongson,ls1c-emac.yaml      | 114 +++++++++
 MAINTAINERS                                   |   3 +
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |  11 +
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-loongson1.c | 240 ++++++++++++++++++
 7 files changed, 486 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
 create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c


base-commit: 2b3bd393093b04d4882152398019cbb96b0440ff
-- 
2.39.2

[PATCH v3 1/4] dt-bindings: mfd: syscon: Add compatibles for Loongson-1 syscon

[Keguang Zhang]

Add Loongson LS1B and LS1C compatibles for system controller.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
V2 -> V3: Change compatibles back to loongson,ls1b-syscon
          and loongson,ls1c-syscon
V1 -> V2: Make the syscon compatibles more specific

 Documentation/devicetree/bindings/mfd/syscon.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
index 8103154bbb52..c77d7b155a4c 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.yaml
+++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
@@ -49,6 +49,8 @@ properties:
               - hisilicon,peri-subctrl
               - hpe,gxp-sysreg
               - intel,lgm-syscon
+              - loongson,ls1b-syscon
+              - loongson,ls1c-syscon
               - marvell,armada-3700-usb2-host-misc
               - mediatek,mt8135-pctl-a-syscfg
               - mediatek,mt8135-pctl-b-syscfg
-- 
2.39.2

Re: [PATCH v3 1/4] dt-bindings: mfd: syscon: Add compatibles for Loongson-1 syscon

[Krzysztof Kozlowski]

On 24/08/2023 14:50, Keguang Zhang wrote:
> Add Loongson LS1B and LS1C compatibles for system controller.
> 
> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>

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

Best regards,
Krzysztof

[PATCH v3 2/4] dt-bindings: net: Add Loongson-1 Ethernet Controller

[Keguang Zhang]

Add devicetree binding document for Loongson-1 Ethernet controller.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
V2 -> V3: Split the DT-schema file into loongson,ls1b-gmac.yaml
          and loongson,ls1c-emac.yaml (suggested by Serge Semin)
          Change the compatibles to loongson,ls1b-gmac and loongson,ls1c-emac
          Rename loongson,dwmac-syscon to loongson,ls1-syscon
          Amend the title
          Add description
          Add Reviewed-by tag from Krzysztof Kozlowski(Sorry! I'm not sure)
V1 -> V2: Fix "clock-names" and "interrupt-names" property
          Rename the syscon property to "loongson,dwmac-syscon"
          Drop "phy-handle" and "phy-mode" requirement
          Revert adding loongson,ls1b-dwmac/loongson,ls1c-dwmac
          to snps,dwmac.yaml

 .../bindings/net/loongson,ls1b-gmac.yaml      | 115 ++++++++++++++++++
 .../bindings/net/loongson,ls1c-emac.yaml      | 114 +++++++++++++++++
 2 files changed, 229 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
 create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml

diff --git a/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml b/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
new file mode 100644
index 000000000000..f661d5b86649
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
@@ -0,0 +1,115 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/loongson,ls1b-gmac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson-1B Gigabit Ethernet MAC Controller
+
+maintainers:
+  - Keguang Zhang <keguang.zhang@gmail.com>
+
+description:
+  Loongson-1B Gigabit Ethernet MAC Controller is based on
+  Synopsys DesignWare MAC (version 3.50a).
+
+  Main features
+  - Dual 10/100/1000Mbps GMAC controllers
+  - Full-duplex operation (IEEE 802.3x flow control automatic transmission)
+  - Half-duplex operation (CSMA/CD Protocol and back-pressure support)
+  - RX Checksum Offload
+  - TX Checksum insertion
+  - MII interface
+  - RGMII interface
+
+select:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - loongson,ls1b-gmac
+  required:
+    - compatible
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - loongson,ls1b-gmac
+      - const: snps,dwmac-3.50a
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: stmmaceth
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-names:
+    items:
+      - const: macirq
+
+  loongson,ls1-syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the syscon containing some extra configurations
+      including PHY interface mode.
+
+  phy-mode:
+    items:
+      - enum:
+          - mii
+          - rgmii-id
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+  - loongson,ls1-syscon
+
+allOf:
+  - $ref: snps,dwmac.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/loongson,ls1x-clk.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    gmac0: ethernet@1fe10000 {
+        compatible = "loongson,ls1b-gmac", "snps,dwmac-3.50a";
+        reg = <0x1fe10000 0x10000>;
+
+        clocks = <&clkc LS1X_CLKID_AHB>;
+        clock-names = "stmmaceth";
+
+        interrupt-parent = <&intc1>;
+        interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "macirq";
+
+        loongson,ls1-syscon = <&syscon>;
+
+        phy-handle = <&phy0>;
+        phy-mode = "mii";
+        snps,pbl = <1>;
+
+        mdio {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "snps,dwmac-mdio";
+
+            phy0: ethernet-phy@0 {
+                reg = <0x0>;
+            };
+        };
+    };
diff --git a/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml b/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
new file mode 100644
index 000000000000..1ffad41941bf
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
@@ -0,0 +1,114 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/loongson,ls1c-emac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson-1C Ethernet MAC Controller
+
+maintainers:
+  - Keguang Zhang <keguang.zhang@gmail.com>
+
+description:
+  Loongson-1C Ethernet MAC Controller is based on
+  Synopsys DesignWare MAC (version 3.50a).
+
+  Main features
+  - 10/100Mbps
+  - Full-duplex operation (IEEE 802.3x flow control automatic transmission)
+  - Half-duplex operation (CSMA/CD Protocol and back-pressure support)
+  - IEEE 802.1Q VLAN tag detection for reception frames
+  - MII interface
+  - RMII interface
+
+select:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - loongson,ls1c-emac
+  required:
+    - compatible
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - loongson,ls1c-emac
+      - const: snps,dwmac-3.50a
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: stmmaceth
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-names:
+    items:
+      - const: macirq
+
+  loongson,ls1-syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the syscon containing some extra configurations
+      including PHY interface mode.
+
+  phy-mode:
+    items:
+      - enum:
+          - mii
+          - rmii
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+  - loongson,ls1-syscon
+
+allOf:
+  - $ref: snps,dwmac.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/loongson,ls1x-clk.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    emac: ethernet@1fe10000 {
+        compatible = "loongson,ls1c-emac", "snps,dwmac-3.50a";
+        reg = <0x1fe10000 0x10000>;
+
+        clocks = <&clkc LS1X_CLKID_AHB>;
+        clock-names = "stmmaceth";
+
+        interrupt-parent = <&intc1>;
+        interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "macirq";
+
+        loongson,ls1-syscon = <&syscon>;
+
+        phy-handle = <&phy0>;
+        phy-mode = "mii";
+        snps,pbl = <1>;
+
+        mdio {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "snps,dwmac-mdio";
+
+            phy0: ethernet-phy@13 {
+                reg = <0x13>;
+            };
+        };
+    };
-- 
2.39.2

Re: [PATCH v3 2/4] dt-bindings: net: Add Loongson-1 Ethernet Controller

[Serge Semin]

On Thu, Aug 24, 2023 at 08:50:10PM +0800, Keguang Zhang wrote:
> Add devicetree binding document for Loongson-1 Ethernet controller.
> 
> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
> V2 -> V3: Split the DT-schema file into loongson,ls1b-gmac.yaml
>           and loongson,ls1c-emac.yaml (suggested by Serge Semin)
>           Change the compatibles to loongson,ls1b-gmac and loongson,ls1c-emac
>           Rename loongson,dwmac-syscon to loongson,ls1-syscon
>           Amend the title
>           Add description
>           Add Reviewed-by tag from Krzysztof Kozlowski(Sorry! I'm not sure)
> V1 -> V2: Fix "clock-names" and "interrupt-names" property
>           Rename the syscon property to "loongson,dwmac-syscon"
>           Drop "phy-handle" and "phy-mode" requirement
>           Revert adding loongson,ls1b-dwmac/loongson,ls1c-dwmac
>           to snps,dwmac.yaml
> 
>  .../bindings/net/loongson,ls1b-gmac.yaml      | 115 ++++++++++++++++++
>  .../bindings/net/loongson,ls1c-emac.yaml      | 114 +++++++++++++++++
>  2 files changed, 229 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
>  create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml b/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
> new file mode 100644
> index 000000000000..f661d5b86649
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
> @@ -0,0 +1,115 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/loongson,ls1b-gmac.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Loongson-1B Gigabit Ethernet MAC Controller
> +
> +maintainers:
> +  - Keguang Zhang <keguang.zhang@gmail.com>
> +

> +description:

Use "|" to keep the text formatting.
  description: |

> +  Loongson-1B Gigabit Ethernet MAC Controller is based on
> +  Synopsys DesignWare MAC (version 3.50a).
> +

> +  Main features
> +  - Dual 10/100/1000Mbps GMAC controllers
> +  - Full-duplex operation (IEEE 802.3x flow control automatic transmission)
> +  - Half-duplex operation (CSMA/CD Protocol and back-pressure support)
> +  - RX Checksum Offload
> +  - TX Checksum insertion
> +  - MII interface
> +  - RGMII interface

If only all the DW *MAC-based devices have such info in the
bindings the life would have been much easier...

> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - loongson,ls1b-gmac
> +  required:
> +    - compatible
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - loongson,ls1b-gmac
> +      - const: snps,dwmac-3.50a
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +

> +  clock-names:
> +    items:
> +      - const: stmmaceth

since there is a single clock then just:
  clock-names:
    const: stmmaceth

> +
> +  interrupts:
> +    maxItems: 1
> +

> +  interrupt-names:
> +    items:
> +      - const: macirq

ditto. just
  interrupt-names:
    const: macirq

> +
> +  loongson,ls1-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the syscon containing some extra configurations
> +      including PHY interface mode.
> +

> +  phy-mode:
> +    items:
> +      - enum:
> +          - mii
> +          - rgmii-id

it's a single string then just:
  phy-mode:
    enum: ...

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - interrupt-names
> +  - loongson,ls1-syscon
> +
> +allOf:
> +  - $ref: snps,dwmac.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/loongson,ls1x-clk.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    gmac0: ethernet@1fe10000 {
> +        compatible = "loongson,ls1b-gmac", "snps,dwmac-3.50a";
> +        reg = <0x1fe10000 0x10000>;
> +
> +        clocks = <&clkc LS1X_CLKID_AHB>;
> +        clock-names = "stmmaceth";
> +
> +        interrupt-parent = <&intc1>;
> +        interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
> +        interrupt-names = "macirq";
> +
> +        loongson,ls1-syscon = <&syscon>;
> +
> +        phy-handle = <&phy0>;
> +        phy-mode = "mii";
> +        snps,pbl = <1>;
> +
> +        mdio {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "snps,dwmac-mdio";
> +
> +            phy0: ethernet-phy@0 {
> +                reg = <0x0>;
> +            };
> +        };
> +    };
> diff --git a/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml b/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
> new file mode 100644
> index 000000000000..1ffad41941bf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
> @@ -0,0 +1,114 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/loongson,ls1c-emac.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Loongson-1C Ethernet MAC Controller
> +
> +maintainers:
> +  - Keguang Zhang <keguang.zhang@gmail.com>
> +

> +description:

the same comment about the "|" modifier.

> +  Loongson-1C Ethernet MAC Controller is based on
> +  Synopsys DesignWare MAC (version 3.50a).
> +

> +  Main features
> +  - 10/100Mbps
> +  - Full-duplex operation (IEEE 802.3x flow control automatic transmission)
> +  - Half-duplex operation (CSMA/CD Protocol and back-pressure support)
> +  - IEEE 802.1Q VLAN tag detection for reception frames
> +  - MII interface
> +  - RMII interface

Based on the plat_stmmacenet_data data defined for the LS1C MAC it
also support support Tx COE. Isn't it? What about Rx COE?

> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - loongson,ls1c-emac
> +  required:
> +    - compatible
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - loongson,ls1c-emac
> +      - const: snps,dwmac-3.50a
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +

> +  clock-names:
> +    items:
> +      - const: stmmaceth

  clock-names:
    const: stmmaceth
?

> +
> +  interrupts:
> +    maxItems: 1
> +

> +  interrupt-names:
> +    items:
> +      - const: macirq

  interrupt-names:
    const: macirq
?

> +
> +  loongson,ls1-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the syscon containing some extra configurations
> +      including PHY interface mode.
> +

> +  phy-mode:
> +    items:
> +      - enum:
> +          - mii
> +          - rmii

  phy-mode:
    enum: ...
?

-Serge(y)

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - interrupt-names
> +  - loongson,ls1-syscon
> +
> +allOf:
> +  - $ref: snps,dwmac.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/loongson,ls1x-clk.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    emac: ethernet@1fe10000 {
> +        compatible = "loongson,ls1c-emac", "snps,dwmac-3.50a";
> +        reg = <0x1fe10000 0x10000>;
> +
> +        clocks = <&clkc LS1X_CLKID_AHB>;
> +        clock-names = "stmmaceth";
> +
> +        interrupt-parent = <&intc1>;
> +        interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
> +        interrupt-names = "macirq";
> +
> +        loongson,ls1-syscon = <&syscon>;
> +
> +        phy-handle = <&phy0>;
> +        phy-mode = "mii";
> +        snps,pbl = <1>;
> +
> +        mdio {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "snps,dwmac-mdio";
> +
> +            phy0: ethernet-phy@13 {
> +                reg = <0x13>;
> +            };
> +        };
> +    };
> -- 
> 2.39.2
> 
> 

Re: [PATCH v3 2/4] dt-bindings: net: Add Loongson-1 Ethernet Controller

[Krzysztof Kozlowski]

On 26/08/2023 23:04, Serge Semin wrote:
>> +  clock-names:
>> +    items:
>> +      - const: stmmaceth
> 
>   clock-names:
>     const: stmmaceth
> ?

The existing syntax is correct. This is a string array.

> 
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
> 
>> +  interrupt-names:
>> +    items:
>> +      - const: macirq
> 
>   interrupt-names:
>     const: macirq
> ?

As well.

> 
>> +
>> +  loongson,ls1-syscon:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Phandle to the syscon containing some extra configurations
>> +      including PHY interface mode.
>> +
> 
>> +  phy-mode:
>> +    items:
>> +      - enum:
>> +          - mii
>> +          - rmii
> 
>   phy-mode:
>     enum: ...
> ?

Here indeed, this is a string, not a list, so items are wrong.



Best regards,
Krzysztof

Re: [PATCH v3 2/4] dt-bindings: net: Add Loongson-1 Ethernet Controller

[Serge Semin]

Hi Krzysztof

On Sun, Aug 27, 2023 at 09:56:06AM +0200, Krzysztof Kozlowski wrote:
> On 26/08/2023 23:04, Serge Semin wrote:
> >> +  clock-names:
> >> +    items:
> >> +      - const: stmmaceth
> > 
> >   clock-names:
> >     const: stmmaceth
> > ?
> 

> The existing syntax is correct. This is a string array.

Could you please clarify whether it's a requirement (always specify
items: property for an array) or just an acceptable option (another
one is suggested in my comment)? I am asking because:
1. In this case the "clock-names" array is supposed to have only one
item. Directly setting "const: stmmaceth" with no items: property
shall simplify it.
2. There are single-entry "clock-names" property in the DT-bindings
defined as I suggested.
3. There is a "compatible" property which is also a string array but
it can be defined as I suggested (omitting the items property).

so based on all of that using the "items:"-based constraint here seems
redundant. Am I wrong to think like that? If so in what aspect?

-Serge(y)

> 
> > 
> >> +
> >> +  interrupts:
> >> +    maxItems: 1
> >> +
> > 
> >> +  interrupt-names:
> >> +    items:
> >> +      - const: macirq
> > 
> >   interrupt-names:
> >     const: macirq
> > ?
> 
> As well.
> 
> > 
> >> +
> >> +  loongson,ls1-syscon:
> >> +    $ref: /schemas/types.yaml#/definitions/phandle
> >> +    description:
> >> +      Phandle to the syscon containing some extra configurations
> >> +      including PHY interface mode.
> >> +
> > 
> >> +  phy-mode:
> >> +    items:
> >> +      - enum:
> >> +          - mii
> >> +          - rmii
> > 
> >   phy-mode:
> >     enum: ...
> > ?
> 
> Here indeed, this is a string, not a list, so items are wrong.
> 
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 2/4] dt-bindings: net: Add Loongson-1 Ethernet Controller

[Krzysztof Kozlowski]

On 27/08/2023 23:01, Serge Semin wrote:
> Hi Krzysztof
> 
> On Sun, Aug 27, 2023 at 09:56:06AM +0200, Krzysztof Kozlowski wrote:
>> On 26/08/2023 23:04, Serge Semin wrote:
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: stmmaceth
>>>
>>>   clock-names:
>>>     const: stmmaceth
>>> ?
>>
> 
>> The existing syntax is correct. This is a string array.
> 
> Could you please clarify whether it's a requirement (always specify
> items: property for an array) or just an acceptable option (another
> one is suggested in my comment)? I am asking because:
> 1. In this case the "clock-names" array is supposed to have only one
> item. Directly setting "const: stmmaceth" with no items: property
> shall simplify it.
> 2. There are single-entry "clock-names" property in the DT-bindings
> defined as I suggested.
> 3. There is a "compatible" property which is also a string array but
> it can be defined as I suggested (omitting the items property).
> 
> so based on all of that using the "items:"-based constraint here seems
> redundant. Am I wrong to think like that? If so in what aspect?

Syntax is correct in both cases. However the single list compatible
*cannot grow*, while single list clock might, when developer notices
that the binding was incomplete. People add binding matching drivers,
not the hardware, thus having incomplete list of clocks is happening all
the time.

Best regards,
Krzysztof

Re: [PATCH v3 2/4] dt-bindings: net: Add Loongson-1 Ethernet Controller

[Serge Semin]

On Mon, Aug 28, 2023 at 09:15:17AM +0200, Krzysztof Kozlowski wrote:
> On 27/08/2023 23:01, Serge Semin wrote:
> > Hi Krzysztof
> > 
> > On Sun, Aug 27, 2023 at 09:56:06AM +0200, Krzysztof Kozlowski wrote:
> >> On 26/08/2023 23:04, Serge Semin wrote:
> >>>> +  clock-names:
> >>>> +    items:
> >>>> +      - const: stmmaceth
> >>>
> >>>   clock-names:
> >>>     const: stmmaceth
> >>> ?
> >>
> > 
> >> The existing syntax is correct. This is a string array.
> > 
> > Could you please clarify whether it's a requirement (always specify
> > items: property for an array) or just an acceptable option (another
> > one is suggested in my comment)? I am asking because:
> > 1. In this case the "clock-names" array is supposed to have only one
> > item. Directly setting "const: stmmaceth" with no items: property
> > shall simplify it.
> > 2. There are single-entry "clock-names" property in the DT-bindings
> > defined as I suggested.
> > 3. There is a "compatible" property which is also a string array but
> > it can be defined as I suggested (omitting the items property).
> > 
> > so based on all of that using the "items:"-based constraint here seems
> > redundant. Am I wrong to think like that? If so in what aspect?
> 

> Syntax is correct in both cases. However the single list compatible
> *cannot grow*, while single list clock might, when developer notices
> that the binding was incomplete. People add binding matching drivers,
> not the hardware, thus having incomplete list of clocks is happening all
> the time.

So it's just a matter of maintainability. Got it. Thanks.

-Serge(y)

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 2/4] dt-bindings: net: Add Loongson-1 Ethernet Controller

[Keguang Zhang]

On Sun, Aug 27, 2023 at 5:04 AM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Thu, Aug 24, 2023 at 08:50:10PM +0800, Keguang Zhang wrote:
> > Add devicetree binding document for Loongson-1 Ethernet controller.
> >
> > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > ---
> > V2 -> V3: Split the DT-schema file into loongson,ls1b-gmac.yaml
> >           and loongson,ls1c-emac.yaml (suggested by Serge Semin)
> >           Change the compatibles to loongson,ls1b-gmac and loongson,ls1c-emac
> >           Rename loongson,dwmac-syscon to loongson,ls1-syscon
> >           Amend the title
> >           Add description
> >           Add Reviewed-by tag from Krzysztof Kozlowski(Sorry! I'm not sure)
> > V1 -> V2: Fix "clock-names" and "interrupt-names" property
> >           Rename the syscon property to "loongson,dwmac-syscon"
> >           Drop "phy-handle" and "phy-mode" requirement
> >           Revert adding loongson,ls1b-dwmac/loongson,ls1c-dwmac
> >           to snps,dwmac.yaml
> >
> >  .../bindings/net/loongson,ls1b-gmac.yaml      | 115 ++++++++++++++++++
> >  .../bindings/net/loongson,ls1c-emac.yaml      | 114 +++++++++++++++++
> >  2 files changed, 229 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
> >  create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml b/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
> > new file mode 100644
> > index 000000000000..f661d5b86649
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
> > @@ -0,0 +1,115 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/loongson,ls1b-gmac.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Loongson-1B Gigabit Ethernet MAC Controller
> > +
> > +maintainers:
> > +  - Keguang Zhang <keguang.zhang@gmail.com>
> > +
>
> > +description:
>
> Use "|" to keep the text formatting.
>   description: |
>
> > +  Loongson-1B Gigabit Ethernet MAC Controller is based on
> > +  Synopsys DesignWare MAC (version 3.50a).
> > +
>
> > +  Main features
> > +  - Dual 10/100/1000Mbps GMAC controllers
> > +  - Full-duplex operation (IEEE 802.3x flow control automatic transmission)
> > +  - Half-duplex operation (CSMA/CD Protocol and back-pressure support)
> > +  - RX Checksum Offload
> > +  - TX Checksum insertion
> > +  - MII interface
> > +  - RGMII interface
>
> If only all the DW *MAC-based devices have such info in the
> bindings the life would have been much easier...
>
> > +
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - loongson,ls1b-gmac
> > +  required:
> > +    - compatible
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - loongson,ls1b-gmac
> > +      - const: snps,dwmac-3.50a
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
>
> > +  clock-names:
> > +    items:
> > +      - const: stmmaceth
>
> since there is a single clock then just:
>   clock-names:
>     const: stmmaceth
>
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
>
> > +  interrupt-names:
> > +    items:
> > +      - const: macirq
>
> ditto. just
>   interrupt-names:
>     const: macirq
>
> > +
> > +  loongson,ls1-syscon:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Phandle to the syscon containing some extra configurations
> > +      including PHY interface mode.
> > +
>
> > +  phy-mode:
> > +    items:
> > +      - enum:
> > +          - mii
> > +          - rgmii-id
>
> it's a single string then just:
>   phy-mode:
>     enum: ...
>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - interrupt-names
> > +  - loongson,ls1-syscon
> > +
> > +allOf:
> > +  - $ref: snps,dwmac.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/loongson,ls1x-clk.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    gmac0: ethernet@1fe10000 {
> > +        compatible = "loongson,ls1b-gmac", "snps,dwmac-3.50a";
> > +        reg = <0x1fe10000 0x10000>;
> > +
> > +        clocks = <&clkc LS1X_CLKID_AHB>;
> > +        clock-names = "stmmaceth";
> > +
> > +        interrupt-parent = <&intc1>;
> > +        interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
> > +        interrupt-names = "macirq";
> > +
> > +        loongson,ls1-syscon = <&syscon>;
> > +
> > +        phy-handle = <&phy0>;
> > +        phy-mode = "mii";
> > +        snps,pbl = <1>;
> > +
> > +        mdio {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            compatible = "snps,dwmac-mdio";
> > +
> > +            phy0: ethernet-phy@0 {
> > +                reg = <0x0>;
> > +            };
> > +        };
> > +    };
> > diff --git a/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml b/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
> > new file mode 100644
> > index 000000000000..1ffad41941bf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
> > @@ -0,0 +1,114 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/loongson,ls1c-emac.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Loongson-1C Ethernet MAC Controller
> > +
> > +maintainers:
> > +  - Keguang Zhang <keguang.zhang@gmail.com>
> > +
>
> > +description:
>
> the same comment about the "|" modifier.
>
> > +  Loongson-1C Ethernet MAC Controller is based on
> > +  Synopsys DesignWare MAC (version 3.50a).
> > +
>
> > +  Main features
> > +  - 10/100Mbps
> > +  - Full-duplex operation (IEEE 802.3x flow control automatic transmission)
> > +  - Half-duplex operation (CSMA/CD Protocol and back-pressure support)
> > +  - IEEE 802.1Q VLAN tag detection for reception frames
> > +  - MII interface
> > +  - RMII interface
>
> Based on the plat_stmmacenet_data data defined for the LS1C MAC it
> also support support Tx COE. Isn't it? What about Rx COE?
>
According to the value read from HW Feature Register(offset 0x1058),
both TXCOESEL an
RXTYP1COE/RXTYP2COE are zero.
Therefore, neither TX COE nor RX COE is supported by LS1C MAC.
Current plat_stmmacenet_data is Inaccurate.

> > +
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - loongson,ls1c-emac
> > +  required:
> > +    - compatible
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - loongson,ls1c-emac
> > +      - const: snps,dwmac-3.50a
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
>
> > +  clock-names:
> > +    items:
> > +      - const: stmmaceth
>
>   clock-names:
>     const: stmmaceth
> ?
>
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
>
> > +  interrupt-names:
> > +    items:
> > +      - const: macirq
>
>   interrupt-names:
>     const: macirq
> ?
>
> > +
> > +  loongson,ls1-syscon:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Phandle to the syscon containing some extra configurations
> > +      including PHY interface mode.
> > +
>
> > +  phy-mode:
> > +    items:
> > +      - enum:
> > +          - mii
> > +          - rmii
>
>   phy-mode:
>     enum: ...
> ?
>
> -Serge(y)
>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - interrupt-names
> > +  - loongson,ls1-syscon
> > +
> > +allOf:
> > +  - $ref: snps,dwmac.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/loongson,ls1x-clk.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    emac: ethernet@1fe10000 {
> > +        compatible = "loongson,ls1c-emac", "snps,dwmac-3.50a";
> > +        reg = <0x1fe10000 0x10000>;
> > +
> > +        clocks = <&clkc LS1X_CLKID_AHB>;
> > +        clock-names = "stmmaceth";
> > +
> > +        interrupt-parent = <&intc1>;
> > +        interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
> > +        interrupt-names = "macirq";
> > +
> > +        loongson,ls1-syscon = <&syscon>;
> > +
> > +        phy-handle = <&phy0>;
> > +        phy-mode = "mii";
> > +        snps,pbl = <1>;
> > +
> > +        mdio {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            compatible = "snps,dwmac-mdio";
> > +
> > +            phy0: ethernet-phy@13 {
> > +                reg = <0x13>;
> > +            };
> > +        };
> > +    };
> > --
> > 2.39.2
> >
> >



--
Best regards,

Keguang Zhang

Re: [PATCH v3 2/4] dt-bindings: net: Add Loongson-1 Ethernet Controller

[Serge Semin]

On Mon, Aug 28, 2023 at 12:38:37PM +0800, Keguang Zhang wrote:
> On Sun, Aug 27, 2023 at 5:04 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> >
> > On Thu, Aug 24, 2023 at 08:50:10PM +0800, Keguang Zhang wrote:
> > > Add devicetree binding document for Loongson-1 Ethernet controller.
> > >
> > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > ---
> > > V2 -> V3: Split the DT-schema file into loongson,ls1b-gmac.yaml
> > >           and loongson,ls1c-emac.yaml (suggested by Serge Semin)
> > >           Change the compatibles to loongson,ls1b-gmac and loongson,ls1c-emac
> > >           Rename loongson,dwmac-syscon to loongson,ls1-syscon
> > >           Amend the title
> > >           Add description
> > >           Add Reviewed-by tag from Krzysztof Kozlowski(Sorry! I'm not sure)
> > > V1 -> V2: Fix "clock-names" and "interrupt-names" property
> > >           Rename the syscon property to "loongson,dwmac-syscon"
> > >           Drop "phy-handle" and "phy-mode" requirement
> > >           Revert adding loongson,ls1b-dwmac/loongson,ls1c-dwmac
> > >           to snps,dwmac.yaml
> > >
> > >  .../bindings/net/loongson,ls1b-gmac.yaml      | 115 ++++++++++++++++++
> > >  .../bindings/net/loongson,ls1c-emac.yaml      | 114 +++++++++++++++++
> > >  2 files changed, 229 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
> > >  create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml b/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
> > > new file mode 100644
> > > index 000000000000..f661d5b86649
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
> > > @@ -0,0 +1,115 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/net/loongson,ls1b-gmac.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Loongson-1B Gigabit Ethernet MAC Controller
> > > +
> > > +maintainers:
> > > +  - Keguang Zhang <keguang.zhang@gmail.com>
> > > +
> >
> > > +description:
> >
> > Use "|" to keep the text formatting.
> >   description: |
> >
> > > +  Loongson-1B Gigabit Ethernet MAC Controller is based on
> > > +  Synopsys DesignWare MAC (version 3.50a).
> > > +
> >
> > > +  Main features
> > > +  - Dual 10/100/1000Mbps GMAC controllers
> > > +  - Full-duplex operation (IEEE 802.3x flow control automatic transmission)
> > > +  - Half-duplex operation (CSMA/CD Protocol and back-pressure support)
> > > +  - RX Checksum Offload
> > > +  - TX Checksum insertion
> > > +  - MII interface
> > > +  - RGMII interface
> >
> > If only all the DW *MAC-based devices have such info in the
> > bindings the life would have been much easier...
> >
> > > +
> > > +select:
> > > +  properties:
> > > +    compatible:
> > > +      contains:
> > > +        enum:
> > > +          - loongson,ls1b-gmac
> > > +  required:
> > > +    - compatible
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - enum:
> > > +          - loongson,ls1b-gmac
> > > +      - const: snps,dwmac-3.50a
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> >

> > > +  clock-names:
> > > +    items:
> > > +      - const: stmmaceth
> >
> > since there is a single clock then just:
> >   clock-names:
> >     const: stmmaceth

As Krzysztof noted please ignore this comment.

> >
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> >
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: macirq
> >

> > ditto. just
> >   interrupt-names:
> >     const: macirq

ditto.

> >
> > > +
> > > +  loongson,ls1-syscon:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description:
> > > +      Phandle to the syscon containing some extra configurations
> > > +      including PHY interface mode.
> > > +
> >
> > > +  phy-mode:
> > > +    items:
> > > +      - enum:
> > > +          - mii
> > > +          - rgmii-id
> >
> > it's a single string then just:
> >   phy-mode:
> >     enum: ...
> >
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - clock-names
> > > +  - interrupts
> > > +  - interrupt-names
> > > +  - loongson,ls1-syscon
> > > +
> > > +allOf:
> > > +  - $ref: snps,dwmac.yaml#
> > > +
> > > +unevaluatedProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/clock/loongson,ls1x-clk.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +
> > > +    gmac0: ethernet@1fe10000 {
> > > +        compatible = "loongson,ls1b-gmac", "snps,dwmac-3.50a";
> > > +        reg = <0x1fe10000 0x10000>;
> > > +
> > > +        clocks = <&clkc LS1X_CLKID_AHB>;
> > > +        clock-names = "stmmaceth";
> > > +
> > > +        interrupt-parent = <&intc1>;
> > > +        interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
> > > +        interrupt-names = "macirq";
> > > +
> > > +        loongson,ls1-syscon = <&syscon>;
> > > +
> > > +        phy-handle = <&phy0>;
> > > +        phy-mode = "mii";
> > > +        snps,pbl = <1>;
> > > +
> > > +        mdio {
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +            compatible = "snps,dwmac-mdio";
> > > +
> > > +            phy0: ethernet-phy@0 {
> > > +                reg = <0x0>;
> > > +            };
> > > +        };
> > > +    };
> > > diff --git a/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml b/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
> > > new file mode 100644
> > > index 000000000000..1ffad41941bf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
> > > @@ -0,0 +1,114 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/net/loongson,ls1c-emac.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Loongson-1C Ethernet MAC Controller
> > > +
> > > +maintainers:
> > > +  - Keguang Zhang <keguang.zhang@gmail.com>
> > > +
> >
> > > +description:
> >
> > the same comment about the "|" modifier.
> >
> > > +  Loongson-1C Ethernet MAC Controller is based on
> > > +  Synopsys DesignWare MAC (version 3.50a).
> > > +
> >
> > > +  Main features
> > > +  - 10/100Mbps
> > > +  - Full-duplex operation (IEEE 802.3x flow control automatic transmission)
> > > +  - Half-duplex operation (CSMA/CD Protocol and back-pressure support)
> > > +  - IEEE 802.1Q VLAN tag detection for reception frames
> > > +  - MII interface
> > > +  - RMII interface
> >
> > Based on the plat_stmmacenet_data data defined for the LS1C MAC it
> > also support support Tx COE. Isn't it? What about Rx COE?
> >

> According to the value read from HW Feature Register(offset 0x1058),
> both TXCOESEL an
> RXTYP1COE/RXTYP2COE are zero.
> Therefore, neither TX COE nor RX COE is supported by LS1C MAC.
> Current plat_stmmacenet_data is Inaccurate.

And since STMMAC probe() procedure overrides the
plat_stmmacenet_data.tx_coe field then no problems the mistaken value
caused. I see. Thanks for clarification.

-Serge(y)

> 
> > > +
> > > +select:
> > > +  properties:
> > > +    compatible:
> > > +      contains:
> > > +        enum:
> > > +          - loongson,ls1c-emac
> > > +  required:
> > > +    - compatible
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - enum:
> > > +          - loongson,ls1c-emac
> > > +      - const: snps,dwmac-3.50a
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> >
> > > +  clock-names:
> > > +    items:
> > > +      - const: stmmaceth
> >

> >   clock-names:
> >     const: stmmaceth
> > ?

Please ignore this comment.

> >
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> >
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: macirq
> >

> >   interrupt-names:
> >     const: macirq
> > ?

ditto.

-Serge(y)

> >
> > > +
> > > +  loongson,ls1-syscon:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description:
> > > +      Phandle to the syscon containing some extra configurations
> > > +      including PHY interface mode.
> > > +
> >
> > > +  phy-mode:
> > > +    items:
> > > +      - enum:
> > > +          - mii
> > > +          - rmii
> >
> >   phy-mode:
> >     enum: ...
> > ?
> >
> > -Serge(y)
> >
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - clock-names
> > > +  - interrupts
> > > +  - interrupt-names
> > > +  - loongson,ls1-syscon
> > > +
> > > +allOf:
> > > +  - $ref: snps,dwmac.yaml#
> > > +
> > > +unevaluatedProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/clock/loongson,ls1x-clk.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +
> > > +    emac: ethernet@1fe10000 {
> > > +        compatible = "loongson,ls1c-emac", "snps,dwmac-3.50a";
> > > +        reg = <0x1fe10000 0x10000>;
> > > +
> > > +        clocks = <&clkc LS1X_CLKID_AHB>;
> > > +        clock-names = "stmmaceth";
> > > +
> > > +        interrupt-parent = <&intc1>;
> > > +        interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
> > > +        interrupt-names = "macirq";
> > > +
> > > +        loongson,ls1-syscon = <&syscon>;
> > > +
> > > +        phy-handle = <&phy0>;
> > > +        phy-mode = "mii";
> > > +        snps,pbl = <1>;
> > > +
> > > +        mdio {
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +            compatible = "snps,dwmac-mdio";
> > > +
> > > +            phy0: ethernet-phy@13 {
> > > +                reg = <0x13>;
> > > +            };
> > > +        };
> > > +    };
> > > --
> > > 2.39.2
> > >
> > >
> 
> 
> 
> --
> Best regards,
> 
> Keguang Zhang

Re: [PATCH v3 2/4] dt-bindings: net: Add Loongson-1 Ethernet Controller

[Keguang Zhang]

On Sun, Aug 27, 2023 at 5:04 AM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Thu, Aug 24, 2023 at 08:50:10PM +0800, Keguang Zhang wrote:
> > Add devicetree binding document for Loongson-1 Ethernet controller.
> >
> > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > ---
> > V2 -> V3: Split the DT-schema file into loongson,ls1b-gmac.yaml
> >           and loongson,ls1c-emac.yaml (suggested by Serge Semin)
> >           Change the compatibles to loongson,ls1b-gmac and loongson,ls1c-emac
> >           Rename loongson,dwmac-syscon to loongson,ls1-syscon
> >           Amend the title
> >           Add description
> >           Add Reviewed-by tag from Krzysztof Kozlowski(Sorry! I'm not sure)
> > V1 -> V2: Fix "clock-names" and "interrupt-names" property
> >           Rename the syscon property to "loongson,dwmac-syscon"
> >           Drop "phy-handle" and "phy-mode" requirement
> >           Revert adding loongson,ls1b-dwmac/loongson,ls1c-dwmac
> >           to snps,dwmac.yaml
> >
> >  .../bindings/net/loongson,ls1b-gmac.yaml      | 115 ++++++++++++++++++
> >  .../bindings/net/loongson,ls1c-emac.yaml      | 114 +++++++++++++++++
> >  2 files changed, 229 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
> >  create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml b/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
> > new file mode 100644
> > index 000000000000..f661d5b86649
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/loongson,ls1b-gmac.yaml
> > @@ -0,0 +1,115 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/loongson,ls1b-gmac.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Loongson-1B Gigabit Ethernet MAC Controller
> > +
> > +maintainers:
> > +  - Keguang Zhang <keguang.zhang@gmail.com>
> > +
>
> > +description:
>
> Use "|" to keep the text formatting.
>   description: |
>
Will do.

> > +  Loongson-1B Gigabit Ethernet MAC Controller is based on
> > +  Synopsys DesignWare MAC (version 3.50a).
> > +
>
> > +  Main features
> > +  - Dual 10/100/1000Mbps GMAC controllers
> > +  - Full-duplex operation (IEEE 802.3x flow control automatic transmission)
> > +  - Half-duplex operation (CSMA/CD Protocol and back-pressure support)
> > +  - RX Checksum Offload
> > +  - TX Checksum insertion
> > +  - MII interface
> > +  - RGMII interface
>
> If only all the DW *MAC-based devices have such info in the
> bindings the life would have been much easier...
>
> > +
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - loongson,ls1b-gmac
> > +  required:
> > +    - compatible
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - loongson,ls1b-gmac
> > +      - const: snps,dwmac-3.50a
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
>
> > +  clock-names:
> > +    items:
> > +      - const: stmmaceth
>
> since there is a single clock then just:
>   clock-names:
>     const: stmmaceth
>
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
>
> > +  interrupt-names:
> > +    items:
> > +      - const: macirq
>
> ditto. just
>   interrupt-names:
>     const: macirq
>
> > +
> > +  loongson,ls1-syscon:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Phandle to the syscon containing some extra configurations
> > +      including PHY interface mode.
> > +
>
> > +  phy-mode:
> > +    items:
> > +      - enum:
> > +          - mii
> > +          - rgmii-id
>
> it's a single string then just:
>   phy-mode:
>     enum: ...
>
Will do.

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - interrupt-names
> > +  - loongson,ls1-syscon
> > +
> > +allOf:
> > +  - $ref: snps,dwmac.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/loongson,ls1x-clk.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    gmac0: ethernet@1fe10000 {
> > +        compatible = "loongson,ls1b-gmac", "snps,dwmac-3.50a";
> > +        reg = <0x1fe10000 0x10000>;
> > +
> > +        clocks = <&clkc LS1X_CLKID_AHB>;
> > +        clock-names = "stmmaceth";
> > +
> > +        interrupt-parent = <&intc1>;
> > +        interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
> > +        interrupt-names = "macirq";
> > +
> > +        loongson,ls1-syscon = <&syscon>;
> > +
> > +        phy-handle = <&phy0>;
> > +        phy-mode = "mii";
> > +        snps,pbl = <1>;
> > +
> > +        mdio {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            compatible = "snps,dwmac-mdio";
> > +
> > +            phy0: ethernet-phy@0 {
> > +                reg = <0x0>;
> > +            };
> > +        };
> > +    };
> > diff --git a/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml b/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
> > new file mode 100644
> > index 000000000000..1ffad41941bf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/loongson,ls1c-emac.yaml
> > @@ -0,0 +1,114 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/loongson,ls1c-emac.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Loongson-1C Ethernet MAC Controller
> > +
> > +maintainers:
> > +  - Keguang Zhang <keguang.zhang@gmail.com>
> > +
>
> > +description:
>
> the same comment about the "|" modifier.
>
Will do.

> > +  Loongson-1C Ethernet MAC Controller is based on
> > +  Synopsys DesignWare MAC (version 3.50a).
> > +
>
> > +  Main features
> > +  - 10/100Mbps
> > +  - Full-duplex operation (IEEE 802.3x flow control automatic transmission)
> > +  - Half-duplex operation (CSMA/CD Protocol and back-pressure support)
> > +  - IEEE 802.1Q VLAN tag detection for reception frames
> > +  - MII interface
> > +  - RMII interface
>
> Based on the plat_stmmacenet_data data defined for the LS1C MAC it
> also support support Tx COE. Isn't it? What about Rx COE?
>
> > +
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - loongson,ls1c-emac
> > +  required:
> > +    - compatible
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - loongson,ls1c-emac
> > +      - const: snps,dwmac-3.50a
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
>
> > +  clock-names:
> > +    items:
> > +      - const: stmmaceth
>
>   clock-names:
>     const: stmmaceth
> ?
>
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
>
> > +  interrupt-names:
> > +    items:
> > +      - const: macirq
>
>   interrupt-names:
>     const: macirq
> ?
>
> > +
> > +  loongson,ls1-syscon:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Phandle to the syscon containing some extra configurations
> > +      including PHY interface mode.
> > +
>
> > +  phy-mode:
> > +    items:
> > +      - enum:
> > +          - mii
> > +          - rmii
>
>   phy-mode:
>     enum: ...
> ?
Will do.
>
> -Serge(y)
>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - interrupt-names
> > +  - loongson,ls1-syscon
> > +
> > +allOf:
> > +  - $ref: snps,dwmac.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/loongson,ls1x-clk.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    emac: ethernet@1fe10000 {
> > +        compatible = "loongson,ls1c-emac", "snps,dwmac-3.50a";
> > +        reg = <0x1fe10000 0x10000>;
> > +
> > +        clocks = <&clkc LS1X_CLKID_AHB>;
> > +        clock-names = "stmmaceth";
> > +
> > +        interrupt-parent = <&intc1>;
> > +        interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
> > +        interrupt-names = "macirq";
> > +
> > +        loongson,ls1-syscon = <&syscon>;
> > +
> > +        phy-handle = <&phy0>;
> > +        phy-mode = "mii";
> > +        snps,pbl = <1>;
> > +
> > +        mdio {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            compatible = "snps,dwmac-mdio";
> > +
> > +            phy0: ethernet-phy@13 {
> > +                reg = <0x13>;
> > +            };
> > +        };
> > +    };
> > --
> > 2.39.2
> >
> >



-- 
Best regards,

Keguang Zhang

[PATCH v3 3/4] net: stmmac: Add glue layer for Loongson-1 SoC

[Keguang Zhang]

This glue driver is created based on the arch-code
implemented earlier with the platform-specific settings.

Use syscon for SYSCON register access.

Partially based on the previous work by Serge Semin.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
V2 -> V3: Determine the device ID by physical
          base address(suggested by Serge Semin)
          Use regmap instead of regmap fields
          Use syscon_regmap_lookup_by_phandle()
          Some minor fixes
V1 -> V2: Fix the build errors due to CONFIG_OF being unset
          Change struct reg_field definitions to const
          Rename the syscon property to "loongson,dwmac-syscon"
          Add MII PHY mode for LS1C

 drivers/net/ethernet/stmicro/stmmac/Kconfig   |  11 +
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-loongson1.c | 240 ++++++++++++++++++
 3 files changed, 252 insertions(+)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 06c6871f8788..a2b9e289aa36 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -239,6 +239,17 @@ config DWMAC_INTEL_PLAT
 	  the stmmac device driver. This driver is used for the Intel Keem Bay
 	  SoC.
 
+config DWMAC_LOONGSON1
+	tristate "Loongson1 GMAC support"
+	default MACH_LOONGSON32
+	depends on OF && (MACH_LOONGSON32 || COMPILE_TEST)
+	help
+	  Support for ethernet controller on Loongson1 SoC.
+
+	  This selects Loongson1 SoC glue layer support for the stmmac
+	  device driver. This driver is used for Loongson1-based boards
+	  like Loongson LS1B/LS1C.
+
 config DWMAC_TEGRA
 	tristate "NVIDIA Tegra MGBE support"
 	depends on ARCH_TEGRA || COMPILE_TEST
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index 5b57aee19267..80e598bd4255 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_DWMAC_SUNXI)	+= dwmac-sunxi.o
 obj-$(CONFIG_DWMAC_SUN8I)	+= dwmac-sun8i.o
 obj-$(CONFIG_DWMAC_DWC_QOS_ETH)	+= dwmac-dwc-qos-eth.o
 obj-$(CONFIG_DWMAC_INTEL_PLAT)	+= dwmac-intel-plat.o
+obj-$(CONFIG_DWMAC_LOONGSON1)	+= dwmac-loongson1.o
 obj-$(CONFIG_DWMAC_GENERIC)	+= dwmac-generic.o
 obj-$(CONFIG_DWMAC_IMX8)	+= dwmac-imx.o
 obj-$(CONFIG_DWMAC_TEGRA)	+= dwmac-tegra.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
new file mode 100644
index 000000000000..347d842141e4
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
@@ -0,0 +1,240 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Loongson-1 DWMAC glue layer
+ *
+ * Copyright (C) 2011-2023 Keguang Zhang <keguang.zhang@gmail.com>
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include "stmmac.h"
+#include "stmmac_platform.h"
+
+#define LS1X_GMAC0_BASE		(0xe10000)
+#define LS1X_GMAC1_BASE		(0xe20000)
+
+/* Loongson-1 SYSCON Registers */
+#define LS1X_SYSCON0		(0x0)
+#define LS1X_SYSCON1		(0x4)
+
+/* Loongson-1B SYSCON Register Bits */
+#define GMAC1_USE_UART1		BIT(4)
+#define GMAC1_USE_UART0		BIT(3)
+
+#define GMAC1_SHUT		BIT(13)
+#define GMAC0_SHUT		BIT(12)
+
+#define GMAC1_USE_TXCLK		BIT(3)
+#define GMAC0_USE_TXCLK		BIT(2)
+#define GMAC1_USE_PWM23		BIT(1)
+#define GMAC0_USE_PWM01		BIT(0)
+
+/* Loongson-1C SYSCON Register Bits */
+#define GMAC_SHUT		BIT(6)
+
+#define PHY_INTF_SELI		GENMASK(30, 28)
+#define PHY_INTF_SELI_SHIFT	28
+
+struct ls1x_dwmac_syscon {
+	int (*syscon_init)(struct plat_stmmacenet_data *plat);
+};
+
+struct ls1x_dwmac {
+	unsigned long reg_base;
+	struct device *dev;
+	struct plat_stmmacenet_data *plat_dat;
+	const struct ls1x_dwmac_syscon *syscon;
+	struct regmap *regmap;
+};
+
+static int ls1b_dwmac_syscon_init(struct plat_stmmacenet_data *plat)
+{
+	struct ls1x_dwmac *dwmac = plat->bsp_priv;
+	struct regmap *regmap = dwmac->regmap;
+
+	if ((dwmac->reg_base & LS1X_GMAC0_BASE) == LS1X_GMAC0_BASE) {
+		switch (plat->phy_interface) {
+		case PHY_INTERFACE_MODE_RGMII_ID:
+			regmap_update_bits(regmap, LS1X_SYSCON0,
+					   GMAC0_USE_TXCLK | GMAC0_USE_PWM01,
+					   0);
+			break;
+		case PHY_INTERFACE_MODE_MII:
+			regmap_update_bits(regmap, LS1X_SYSCON0,
+					   GMAC0_USE_TXCLK | GMAC0_USE_PWM01,
+					   GMAC0_USE_TXCLK | GMAC0_USE_PWM01);
+			break;
+		default:
+			dev_err(dwmac->dev, "Unsupported PHY mode %u\n",
+				plat->phy_interface);
+			return -EOPNOTSUPP;
+		}
+
+		regmap_update_bits(regmap, LS1X_SYSCON0, GMAC0_SHUT, 0);
+	}
+
+	if ((dwmac->reg_base & LS1X_GMAC1_BASE) == LS1X_GMAC1_BASE) {
+		regmap_update_bits(regmap, LS1X_SYSCON0,
+				   GMAC1_USE_UART1 | GMAC1_USE_UART0,
+				   GMAC1_USE_UART1 | GMAC1_USE_UART0);
+
+		switch (plat->phy_interface) {
+		case PHY_INTERFACE_MODE_RGMII_ID:
+			regmap_update_bits(regmap, LS1X_SYSCON1,
+					   GMAC1_USE_TXCLK | GMAC1_USE_PWM23,
+					   0);
+
+			break;
+		case PHY_INTERFACE_MODE_MII:
+			regmap_update_bits(regmap, LS1X_SYSCON1,
+					   GMAC1_USE_TXCLK | GMAC1_USE_PWM23,
+					   GMAC1_USE_TXCLK | GMAC1_USE_PWM23);
+			break;
+		default:
+			dev_err(dwmac->dev, "Unsupported PHY mode %u\n",
+				plat->phy_interface);
+			return -EOPNOTSUPP;
+		}
+
+		regmap_update_bits(regmap, LS1X_SYSCON1, GMAC1_SHUT, 0);
+	}
+
+	return 0;
+}
+
+static int ls1c_dwmac_syscon_init(struct plat_stmmacenet_data *plat)
+{
+	struct ls1x_dwmac *dwmac = plat->bsp_priv;
+	struct regmap *regmap = dwmac->regmap;
+
+	switch (plat->phy_interface) {
+	case PHY_INTERFACE_MODE_MII:
+		regmap_update_bits(regmap, LS1X_SYSCON1, PHY_INTF_SELI, 0);
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		regmap_update_bits(regmap, LS1X_SYSCON1, PHY_INTF_SELI,
+				   4 << PHY_INTF_SELI_SHIFT);
+		break;
+	default:
+		dev_err(dwmac->dev, "Unsupported PHY-mode %u\n",
+			plat->phy_interface);
+		return -EOPNOTSUPP;
+	}
+
+	regmap_update_bits(regmap, LS1X_SYSCON0, GMAC0_SHUT, 0);
+
+	return 0;
+}
+
+static const struct ls1x_dwmac_syscon ls1b_dwmac_syscon = {
+	.syscon_init = ls1b_dwmac_syscon_init,
+};
+
+static const struct ls1x_dwmac_syscon ls1c_dwmac_syscon = {
+	.syscon_init = ls1c_dwmac_syscon_init,
+};
+
+static int ls1x_dwmac_init(struct platform_device *pdev, void *priv)
+{
+	struct ls1x_dwmac *dwmac = priv;
+	struct resource *res;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Could not get IO_MEM resources\n");
+		return -EINVAL;
+	}
+	dwmac->reg_base = (unsigned long)res->start;
+
+	if (dwmac->syscon->syscon_init) {
+		ret = dwmac->syscon->syscon_init(dwmac->plat_dat);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ls1x_dwmac_probe(struct platform_device *pdev)
+{
+	struct plat_stmmacenet_data *plat_dat;
+	struct stmmac_resources stmmac_res;
+	struct regmap *regmap;
+	struct ls1x_dwmac *dwmac;
+	const struct ls1x_dwmac_syscon *syscon;
+	int ret;
+
+	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
+	if (ret)
+		return ret;
+
+	/* Probe syscon */
+	regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+						 "loongson,ls1-syscon");
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		dev_err(&pdev->dev, "Unable to map syscon: %d\n", ret);
+		return ret;
+	}
+
+	syscon = of_device_get_match_data(&pdev->dev);
+	if (!syscon) {
+		dev_err(&pdev->dev, "No of match data provided\n");
+		return -EINVAL;
+	}
+
+	dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
+	if (!dwmac)
+		return -ENOMEM;
+
+	plat_dat = stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	if (IS_ERR(plat_dat)) {
+		dev_err(&pdev->dev, "dt configuration failed\n");
+		return PTR_ERR(plat_dat);
+	}
+
+	plat_dat->bsp_priv = dwmac;
+	plat_dat->init = ls1x_dwmac_init;
+	dwmac->dev = &pdev->dev;
+	dwmac->plat_dat = plat_dat;
+	dwmac->syscon = syscon;
+	dwmac->regmap = regmap;
+
+	ret = stmmac_pltfr_probe(pdev, plat_dat, &stmmac_res);
+	if (ret)
+		goto err_remove_config_dt;
+
+	return 0;
+
+err_remove_config_dt:
+	stmmac_remove_config_dt(pdev, plat_dat);
+
+	return ret;
+}
+
+static const struct of_device_id ls1x_dwmac_match[] = {
+	{ .compatible = "loongson,ls1b-gmac", .data = &ls1b_dwmac_syscon, },
+	{ .compatible = "loongson,ls1c-emac", .data = &ls1c_dwmac_syscon, },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ls1x_dwmac_match);
+
+static struct platform_driver ls1x_dwmac_driver = {
+	.probe = ls1x_dwmac_probe,
+	.remove_new = stmmac_pltfr_remove,
+	.driver = {
+		.name = "loongson1-dwmac",
+		.of_match_table = ls1x_dwmac_match,
+	},
+};
+module_platform_driver(ls1x_dwmac_driver);
+
+MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
+MODULE_DESCRIPTION("Loongson1 DWMAC glue layer");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH v3 3/4] net: stmmac: Add glue layer for Loongson-1 SoC

[Serge Semin]

On Thu, Aug 24, 2023 at 08:50:11PM +0800, Keguang Zhang wrote:
> This glue driver is created based on the arch-code
> implemented earlier with the platform-specific settings.
> 
> Use syscon for SYSCON register access.
> 
> Partially based on the previous work by Serge Semin.
> 
> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> ---
> V2 -> V3: Determine the device ID by physical
>           base address(suggested by Serge Semin)
>           Use regmap instead of regmap fields
>           Use syscon_regmap_lookup_by_phandle()
>           Some minor fixes
> V1 -> V2: Fix the build errors due to CONFIG_OF being unset
>           Change struct reg_field definitions to const
>           Rename the syscon property to "loongson,dwmac-syscon"
>           Add MII PHY mode for LS1C
> 
>  drivers/net/ethernet/stmicro/stmmac/Kconfig   |  11 +
>  drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
>  .../ethernet/stmicro/stmmac/dwmac-loongson1.c | 240 ++++++++++++++++++
>  3 files changed, 252 insertions(+)
>  create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> index 06c6871f8788..a2b9e289aa36 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -239,6 +239,17 @@ config DWMAC_INTEL_PLAT
>  	  the stmmac device driver. This driver is used for the Intel Keem Bay
>  	  SoC.
>  
> +config DWMAC_LOONGSON1
> +	tristate "Loongson1 GMAC support"
> +	default MACH_LOONGSON32
> +	depends on OF && (MACH_LOONGSON32 || COMPILE_TEST)
> +	help
> +	  Support for ethernet controller on Loongson1 SoC.
> +
> +	  This selects Loongson1 SoC glue layer support for the stmmac
> +	  device driver. This driver is used for Loongson1-based boards
> +	  like Loongson LS1B/LS1C.
> +
>  config DWMAC_TEGRA
>  	tristate "NVIDIA Tegra MGBE support"
>  	depends on ARCH_TEGRA || COMPILE_TEST
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
> index 5b57aee19267..80e598bd4255 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_DWMAC_SUNXI)	+= dwmac-sunxi.o
>  obj-$(CONFIG_DWMAC_SUN8I)	+= dwmac-sun8i.o
>  obj-$(CONFIG_DWMAC_DWC_QOS_ETH)	+= dwmac-dwc-qos-eth.o
>  obj-$(CONFIG_DWMAC_INTEL_PLAT)	+= dwmac-intel-plat.o
> +obj-$(CONFIG_DWMAC_LOONGSON1)	+= dwmac-loongson1.o
>  obj-$(CONFIG_DWMAC_GENERIC)	+= dwmac-generic.o
>  obj-$(CONFIG_DWMAC_IMX8)	+= dwmac-imx.o
>  obj-$(CONFIG_DWMAC_TEGRA)	+= dwmac-tegra.o
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
> new file mode 100644
> index 000000000000..347d842141e4
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
> @@ -0,0 +1,240 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Loongson-1 DWMAC glue layer
> + *
> + * Copyright (C) 2011-2023 Keguang Zhang <keguang.zhang@gmail.com>
> + */
> +
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include "stmmac.h"
> +#include "stmmac_platform.h"
> +
> +#define LS1X_GMAC0_BASE		(0xe10000)

> +#define LS1X_GMAC1_BASE		(0xe20000)

If LS1C doesn't have the second GMAC then what about changing the
macros name to LS1B_GMAC1_BASE?

> +
> +/* Loongson-1 SYSCON Registers */
> +#define LS1X_SYSCON0		(0x0)
> +#define LS1X_SYSCON1		(0x4)
> +
> +/* Loongson-1B SYSCON Register Bits */

> +#define GMAC1_USE_UART1		BIT(4)
> +#define GMAC1_USE_UART0		BIT(3)
> +
> +#define GMAC1_SHUT		BIT(13)
> +#define GMAC0_SHUT		BIT(12)
> +
> +#define GMAC1_USE_TXCLK		BIT(3)
> +#define GMAC0_USE_TXCLK		BIT(2)
> +#define GMAC1_USE_PWM23		BIT(1)
> +#define GMAC0_USE_PWM01		BIT(0)
> +
> +/* Loongson-1C SYSCON Register Bits */
> +#define GMAC_SHUT		BIT(6)
> +
> +#define PHY_INTF_SELI		GENMASK(30, 28)

IMO having the SoC-specific prefixes (LS1B_ and LS1C_) in these names
would make the driver a bit more readable. But it's up to you to
decide.

> +#define PHY_INTF_SELI_SHIFT	28

Use FIELD_PREP():
#define PHY_INTF_MII			FIELD_PREP(PHY_INTF_SELI, 0)
#define PHY_INTF_RMII			FIELD_PREP(PHY_INTF_SELI, 4)

> +
> +struct ls1x_dwmac_syscon {
> +	int (*syscon_init)(struct plat_stmmacenet_data *plat);
> +};

This struct is redundant. See further for details.

> +
> +struct ls1x_dwmac {

> +	unsigned long reg_base;

this field

> +	struct device *dev;

> +	struct plat_stmmacenet_data *plat_dat;
> +	const struct ls1x_dwmac_syscon *syscon;

and these fields are also redundant. See further for details.

> +	struct regmap *regmap;
> +};
> +
> +static int ls1b_dwmac_syscon_init(struct plat_stmmacenet_data *plat)
> +{
> +	struct ls1x_dwmac *dwmac = plat->bsp_priv;
> +	struct regmap *regmap = dwmac->regmap;
> +

> +	if ((dwmac->reg_base & LS1X_GMAC0_BASE) == LS1X_GMAC0_BASE) {

Is it necessary to bitwise-and-ing? What if LS1X_GMAC0_BASE would be
just a full physical base address? Then you'll be able to just compare
the base addresses.

> +		switch (plat->phy_interface) {
> +		case PHY_INTERFACE_MODE_RGMII_ID:
> +			regmap_update_bits(regmap, LS1X_SYSCON0,
> +					   GMAC0_USE_TXCLK | GMAC0_USE_PWM01,
> +					   0);
> +			break;
> +		case PHY_INTERFACE_MODE_MII:
> +			regmap_update_bits(regmap, LS1X_SYSCON0,
> +					   GMAC0_USE_TXCLK | GMAC0_USE_PWM01,
> +					   GMAC0_USE_TXCLK | GMAC0_USE_PWM01);
> +			break;
> +		default:
> +			dev_err(dwmac->dev, "Unsupported PHY mode %u\n",
> +				plat->phy_interface);
> +			return -EOPNOTSUPP;
> +		}
> +
> +		regmap_update_bits(regmap, LS1X_SYSCON0, GMAC0_SHUT, 0);

> +	}
> +
> +	if ((dwmac->reg_base & LS1X_GMAC1_BASE) == LS1X_GMAC1_BASE) {

else if?

> +		regmap_update_bits(regmap, LS1X_SYSCON0,
> +				   GMAC1_USE_UART1 | GMAC1_USE_UART0,
> +				   GMAC1_USE_UART1 | GMAC1_USE_UART0);
> +
> +		switch (plat->phy_interface) {
> +		case PHY_INTERFACE_MODE_RGMII_ID:
> +			regmap_update_bits(regmap, LS1X_SYSCON1,
> +					   GMAC1_USE_TXCLK | GMAC1_USE_PWM23,
> +					   0);
> +
> +			break;
> +		case PHY_INTERFACE_MODE_MII:
> +			regmap_update_bits(regmap, LS1X_SYSCON1,
> +					   GMAC1_USE_TXCLK | GMAC1_USE_PWM23,
> +					   GMAC1_USE_TXCLK | GMAC1_USE_PWM23);
> +			break;
> +		default:
> +			dev_err(dwmac->dev, "Unsupported PHY mode %u\n",
> +				plat->phy_interface);
> +			return -EOPNOTSUPP;
> +		}
> +
> +		regmap_update_bits(regmap, LS1X_SYSCON1, GMAC1_SHUT, 0);

> +	}

else {
	dev_err(...)
	return -EINVAL;
}
?

* unless you have some more DW GMACs on the SoC which don't require the
syscon setups.

> +
> +	return 0;
> +}
> +
> +static int ls1c_dwmac_syscon_init(struct plat_stmmacenet_data *plat)
> +{
> +	struct ls1x_dwmac *dwmac = plat->bsp_priv;
> +	struct regmap *regmap = dwmac->regmap;
> +
> +	switch (plat->phy_interface) {
> +	case PHY_INTERFACE_MODE_MII:

> +		regmap_update_bits(regmap, LS1X_SYSCON1, PHY_INTF_SELI, 0);


		regmap_update_bits(regmap, LS1X_SYSCON1, PHY_INTF_SELI,
				   PHY_INTF_MII);

> +		break;
> +	case PHY_INTERFACE_MODE_RMII:

> +		regmap_update_bits(regmap, LS1X_SYSCON1, PHY_INTF_SELI,
> +				   4 << PHY_INTF_SELI_SHIFT);

		regmap_update_bits(regmap, LS1X_SYSCON1, PHY_INTF_SELI,
				   PHY_INTF_RMII);

> +		break;
> +	default:
> +		dev_err(dwmac->dev, "Unsupported PHY-mode %u\n",
> +			plat->phy_interface);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	regmap_update_bits(regmap, LS1X_SYSCON0, GMAC0_SHUT, 0);
> +
> +	return 0;
> +}
> +

> +static const struct ls1x_dwmac_syscon ls1b_dwmac_syscon = {
> +	.syscon_init = ls1b_dwmac_syscon_init,
> +};
> +
> +static const struct ls1x_dwmac_syscon ls1c_dwmac_syscon = {
> +	.syscon_init = ls1c_dwmac_syscon_init,
> +};

This can be dropped. See the next comment for details.

> +
> +static int ls1x_dwmac_init(struct platform_device *pdev, void *priv)
> +{
> +	struct ls1x_dwmac *dwmac = priv;
> +	struct resource *res;
> +	int ret;
> +

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Could not get IO_MEM resources\n");
> +		return -EINVAL;
> +	}
> +	dwmac->reg_base = (unsigned long)res->start;

What about moving this to ls1b_dwmac_syscon_init() seeing it's used
for LS1B only? Thus you won't need to have the reg_base defined in the
private data and can get rid from the ls1x_dwmac_init() method setting
the ls1b_dwmac_syscon_init() and ls1c_dwmac_syscon_init() pointers
directly to the device match data.

> +
> +	if (dwmac->syscon->syscon_init) {
> +		ret = dwmac->syscon->syscon_init(dwmac->plat_dat);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ls1x_dwmac_probe(struct platform_device *pdev)
> +{
> +	struct plat_stmmacenet_data *plat_dat;
> +	struct stmmac_resources stmmac_res;
> +	struct regmap *regmap;
> +	struct ls1x_dwmac *dwmac;

> +	const struct ls1x_dwmac_syscon *syscon;

This can be replaced with just
int (*init)(struct platform_device *pdev, void *priv);

> +	int ret;
> +
> +	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
> +	if (ret)
> +		return ret;
> +
> +	/* Probe syscon */
> +	regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +						 "loongson,ls1-syscon");
> +	if (IS_ERR(regmap)) {

> +		ret = PTR_ERR(regmap);
> +		dev_err(&pdev->dev, "Unable to map syscon: %d\n", ret);
> +		return ret;

return dev_err_probe(&pdev->dev, PTR_ERR(regmap), "Unable to find syscon\n");

> +	}
> +

> +	syscon = of_device_get_match_data(&pdev->dev);

init = of_device_get_match_data(&pdev->dev);

> +	if (!syscon) {
> +		dev_err(&pdev->dev, "No of match data provided\n");
> +		return -EINVAL;
> +	}
> +
> +	dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
> +	if (!dwmac)
> +		return -ENOMEM;
> +
> +	plat_dat = stmmac_probe_config_dt(pdev, stmmac_res.mac);
> +	if (IS_ERR(plat_dat)) {
> +		dev_err(&pdev->dev, "dt configuration failed\n");
> +		return PTR_ERR(plat_dat);
> +	}
> +
> +	plat_dat->bsp_priv = dwmac;

> +	plat_dat->init = ls1x_dwmac_init;

plat_dat->init = init;

> +	dwmac->dev = &pdev->dev;
> +	dwmac->plat_dat = plat_dat;
> +	dwmac->syscon = syscon;
> +	dwmac->regmap = regmap;
> +
> +	ret = stmmac_pltfr_probe(pdev, plat_dat, &stmmac_res);
> +	if (ret)
> +		goto err_remove_config_dt;
> +
> +	return 0;
> +
> +err_remove_config_dt:
> +	stmmac_remove_config_dt(pdev, plat_dat);
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id ls1x_dwmac_match[] = {

> +	{ .compatible = "loongson,ls1b-gmac", .data = &ls1b_dwmac_syscon, },
> +	{ .compatible = "loongson,ls1c-emac", .data = &ls1c_dwmac_syscon, },

{ .compatible = "loongson,ls1b-gmac", .data = &ls1b_dwmac_syscon_init, },
{ .compatible = "loongson,ls1c-emac", .data = &ls1c_dwmac_syscon_init, },

Thus you can simplify the driver by:
1. dropping ls1x_dwmac_syscon definition and its instances.
2. dropping three redundant fields from the ls1x_dwmac structure.
3. dropping the ls1x_dwmac_init() method.
Sounds like worth it.)

Note if no further driver update is planned then you can drop the
ls1x_dwmac->dev field too. Otherwise you can keep it seeing some of
the plat_stmmacenet_data data callbacks do have any device instance
passed.

-Serge(y)

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ls1x_dwmac_match);
> +
> +static struct platform_driver ls1x_dwmac_driver = {
> +	.probe = ls1x_dwmac_probe,
> +	.remove_new = stmmac_pltfr_remove,
> +	.driver = {
> +		.name = "loongson1-dwmac",
> +		.of_match_table = ls1x_dwmac_match,
> +	},
> +};
> +module_platform_driver(ls1x_dwmac_driver);
> +
> +MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
> +MODULE_DESCRIPTION("Loongson1 DWMAC glue layer");
> +MODULE_LICENSE("GPL");
> -- 
> 2.39.2
> 
> 

Re: [PATCH v3 3/4] net: stmmac: Add glue layer for Loongson-1 SoC

[Keguang Zhang]

On Sun, Aug 27, 2023 at 4:40 AM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Thu, Aug 24, 2023 at 08:50:11PM +0800, Keguang Zhang wrote:
> > This glue driver is created based on the arch-code
> > implemented earlier with the platform-specific settings.
> >
> > Use syscon for SYSCON register access.
> >
> > Partially based on the previous work by Serge Semin.
> >
> > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > ---
> > V2 -> V3: Determine the device ID by physical
> >           base address(suggested by Serge Semin)
> >           Use regmap instead of regmap fields
> >           Use syscon_regmap_lookup_by_phandle()
> >           Some minor fixes
> > V1 -> V2: Fix the build errors due to CONFIG_OF being unset
> >           Change struct reg_field definitions to const
> >           Rename the syscon property to "loongson,dwmac-syscon"
> >           Add MII PHY mode for LS1C
> >
> >  drivers/net/ethernet/stmicro/stmmac/Kconfig   |  11 +
> >  drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
> >  .../ethernet/stmicro/stmmac/dwmac-loongson1.c | 240 ++++++++++++++++++
> >  3 files changed, 252 insertions(+)
> >  create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > index 06c6871f8788..a2b9e289aa36 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > @@ -239,6 +239,17 @@ config DWMAC_INTEL_PLAT
> >         the stmmac device driver. This driver is used for the Intel Keem Bay
> >         SoC.
> >
> > +config DWMAC_LOONGSON1
> > +     tristate "Loongson1 GMAC support"
> > +     default MACH_LOONGSON32
> > +     depends on OF && (MACH_LOONGSON32 || COMPILE_TEST)
> > +     help
> > +       Support for ethernet controller on Loongson1 SoC.
> > +
> > +       This selects Loongson1 SoC glue layer support for the stmmac
> > +       device driver. This driver is used for Loongson1-based boards
> > +       like Loongson LS1B/LS1C.
> > +
> >  config DWMAC_TEGRA
> >       tristate "NVIDIA Tegra MGBE support"
> >       depends on ARCH_TEGRA || COMPILE_TEST
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
> > index 5b57aee19267..80e598bd4255 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> > +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> > @@ -29,6 +29,7 @@ obj-$(CONFIG_DWMAC_SUNXI)   += dwmac-sunxi.o
> >  obj-$(CONFIG_DWMAC_SUN8I)    += dwmac-sun8i.o
> >  obj-$(CONFIG_DWMAC_DWC_QOS_ETH)      += dwmac-dwc-qos-eth.o
> >  obj-$(CONFIG_DWMAC_INTEL_PLAT)       += dwmac-intel-plat.o
> > +obj-$(CONFIG_DWMAC_LOONGSON1)        += dwmac-loongson1.o
> >  obj-$(CONFIG_DWMAC_GENERIC)  += dwmac-generic.o
> >  obj-$(CONFIG_DWMAC_IMX8)     += dwmac-imx.o
> >  obj-$(CONFIG_DWMAC_TEGRA)    += dwmac-tegra.o
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
> > new file mode 100644
> > index 000000000000..347d842141e4
> > --- /dev/null
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
> > @@ -0,0 +1,240 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Loongson-1 DWMAC glue layer
> > + *
> > + * Copyright (C) 2011-2023 Keguang Zhang <keguang.zhang@gmail.com>
> > + */
> > +
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +#include "stmmac.h"
> > +#include "stmmac_platform.h"
> > +
> > +#define LS1X_GMAC0_BASE              (0xe10000)
>
> > +#define LS1X_GMAC1_BASE              (0xe20000)
>
> If LS1C doesn't have the second GMAC then what about changing the
> macros name to LS1B_GMAC1_BASE?
>
> > +
> > +/* Loongson-1 SYSCON Registers */
> > +#define LS1X_SYSCON0         (0x0)
> > +#define LS1X_SYSCON1         (0x4)
> > +
> > +/* Loongson-1B SYSCON Register Bits */
>
> > +#define GMAC1_USE_UART1              BIT(4)
> > +#define GMAC1_USE_UART0              BIT(3)
> > +
> > +#define GMAC1_SHUT           BIT(13)
> > +#define GMAC0_SHUT           BIT(12)
> > +
> > +#define GMAC1_USE_TXCLK              BIT(3)
> > +#define GMAC0_USE_TXCLK              BIT(2)
> > +#define GMAC1_USE_PWM23              BIT(1)
> > +#define GMAC0_USE_PWM01              BIT(0)
> > +
> > +/* Loongson-1C SYSCON Register Bits */
> > +#define GMAC_SHUT            BIT(6)
> > +
> > +#define PHY_INTF_SELI                GENMASK(30, 28)
>
> IMO having the SoC-specific prefixes (LS1B_ and LS1C_) in these names
> would make the driver a bit more readable. But it's up to you to
> decide.
>
> > +#define PHY_INTF_SELI_SHIFT  28
>
> Use FIELD_PREP():
> #define PHY_INTF_MII                    FIELD_PREP(PHY_INTF_SELI, 0)
> #define PHY_INTF_RMII                   FIELD_PREP(PHY_INTF_SELI, 4)
>
> > +
> > +struct ls1x_dwmac_syscon {
> > +     int (*syscon_init)(struct plat_stmmacenet_data *plat);
> > +};
>
> This struct is redundant. See further for details.
>
> > +
> > +struct ls1x_dwmac {
>
> > +     unsigned long reg_base;
>
> this field
>
> > +     struct device *dev;
>
> > +     struct plat_stmmacenet_data *plat_dat;
> > +     const struct ls1x_dwmac_syscon *syscon;
>
> and these fields are also redundant. See further for details.
>
> > +     struct regmap *regmap;
> > +};
> > +
> > +static int ls1b_dwmac_syscon_init(struct plat_stmmacenet_data *plat)
> > +{
> > +     struct ls1x_dwmac *dwmac = plat->bsp_priv;
> > +     struct regmap *regmap = dwmac->regmap;
> > +
>
> > +     if ((dwmac->reg_base & LS1X_GMAC0_BASE) == LS1X_GMAC0_BASE) {
>
> Is it necessary to bitwise-and-ing? What if LS1X_GMAC0_BASE would be
> just a full physical base address? Then you'll be able to just compare
> the base addresses.
>
> > +             switch (plat->phy_interface) {
> > +             case PHY_INTERFACE_MODE_RGMII_ID:
> > +                     regmap_update_bits(regmap, LS1X_SYSCON0,
> > +                                        GMAC0_USE_TXCLK | GMAC0_USE_PWM01,
> > +                                        0);
> > +                     break;
> > +             case PHY_INTERFACE_MODE_MII:
> > +                     regmap_update_bits(regmap, LS1X_SYSCON0,
> > +                                        GMAC0_USE_TXCLK | GMAC0_USE_PWM01,
> > +                                        GMAC0_USE_TXCLK | GMAC0_USE_PWM01);
> > +                     break;
> > +             default:
> > +                     dev_err(dwmac->dev, "Unsupported PHY mode %u\n",
> > +                             plat->phy_interface);
> > +                     return -EOPNOTSUPP;
> > +             }
> > +
> > +             regmap_update_bits(regmap, LS1X_SYSCON0, GMAC0_SHUT, 0);
>
> > +     }
> > +
> > +     if ((dwmac->reg_base & LS1X_GMAC1_BASE) == LS1X_GMAC1_BASE) {
>
> else if?
>
> > +             regmap_update_bits(regmap, LS1X_SYSCON0,
> > +                                GMAC1_USE_UART1 | GMAC1_USE_UART0,
> > +                                GMAC1_USE_UART1 | GMAC1_USE_UART0);
> > +
> > +             switch (plat->phy_interface) {
> > +             case PHY_INTERFACE_MODE_RGMII_ID:
> > +                     regmap_update_bits(regmap, LS1X_SYSCON1,
> > +                                        GMAC1_USE_TXCLK | GMAC1_USE_PWM23,
> > +                                        0);
> > +
> > +                     break;
> > +             case PHY_INTERFACE_MODE_MII:
> > +                     regmap_update_bits(regmap, LS1X_SYSCON1,
> > +                                        GMAC1_USE_TXCLK | GMAC1_USE_PWM23,
> > +                                        GMAC1_USE_TXCLK | GMAC1_USE_PWM23);
> > +                     break;
> > +             default:
> > +                     dev_err(dwmac->dev, "Unsupported PHY mode %u\n",
> > +                             plat->phy_interface);
> > +                     return -EOPNOTSUPP;
> > +             }
> > +
> > +             regmap_update_bits(regmap, LS1X_SYSCON1, GMAC1_SHUT, 0);
>
> > +     }
>
> else {
>         dev_err(...)
>         return -EINVAL;
> }
> ?
>
> * unless you have some more DW GMACs on the SoC which don't require the
> syscon setups.
>
> > +
> > +     return 0;
> > +}
> > +
> > +static int ls1c_dwmac_syscon_init(struct plat_stmmacenet_data *plat)
> > +{
> > +     struct ls1x_dwmac *dwmac = plat->bsp_priv;
> > +     struct regmap *regmap = dwmac->regmap;
> > +
> > +     switch (plat->phy_interface) {
> > +     case PHY_INTERFACE_MODE_MII:
>
> > +             regmap_update_bits(regmap, LS1X_SYSCON1, PHY_INTF_SELI, 0);
>
>
>                 regmap_update_bits(regmap, LS1X_SYSCON1, PHY_INTF_SELI,
>                                    PHY_INTF_MII);
>
> > +             break;
> > +     case PHY_INTERFACE_MODE_RMII:
>
> > +             regmap_update_bits(regmap, LS1X_SYSCON1, PHY_INTF_SELI,
> > +                                4 << PHY_INTF_SELI_SHIFT);
>
>                 regmap_update_bits(regmap, LS1X_SYSCON1, PHY_INTF_SELI,
>                                    PHY_INTF_RMII);
>
> > +             break;
> > +     default:
> > +             dev_err(dwmac->dev, "Unsupported PHY-mode %u\n",
> > +                     plat->phy_interface);
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     regmap_update_bits(regmap, LS1X_SYSCON0, GMAC0_SHUT, 0);
> > +
> > +     return 0;
> > +}
> > +
>
> > +static const struct ls1x_dwmac_syscon ls1b_dwmac_syscon = {
> > +     .syscon_init = ls1b_dwmac_syscon_init,
> > +};
> > +
> > +static const struct ls1x_dwmac_syscon ls1c_dwmac_syscon = {
> > +     .syscon_init = ls1c_dwmac_syscon_init,
> > +};
>
> This can be dropped. See the next comment for details.
>
> > +
> > +static int ls1x_dwmac_init(struct platform_device *pdev, void *priv)
> > +{
> > +     struct ls1x_dwmac *dwmac = priv;
> > +     struct resource *res;
> > +     int ret;
> > +
>
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     if (!res) {
> > +             dev_err(&pdev->dev, "Could not get IO_MEM resources\n");
> > +             return -EINVAL;
> > +     }
> > +     dwmac->reg_base = (unsigned long)res->start;
>
> What about moving this to ls1b_dwmac_syscon_init() seeing it's used
> for LS1B only? Thus you won't need to have the reg_base defined in the
> private data and can get rid from the ls1x_dwmac_init() method setting
> the ls1b_dwmac_syscon_init() and ls1c_dwmac_syscon_init() pointers
> directly to the device match data.
>
> > +
> > +     if (dwmac->syscon->syscon_init) {
> > +             ret = dwmac->syscon->syscon_init(dwmac->plat_dat);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int ls1x_dwmac_probe(struct platform_device *pdev)
> > +{
> > +     struct plat_stmmacenet_data *plat_dat;
> > +     struct stmmac_resources stmmac_res;
> > +     struct regmap *regmap;
> > +     struct ls1x_dwmac *dwmac;
>
> > +     const struct ls1x_dwmac_syscon *syscon;
>
> This can be replaced with just
> int (*init)(struct platform_device *pdev, void *priv);
>
> > +     int ret;
> > +
> > +     ret = stmmac_get_platform_resources(pdev, &stmmac_res);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Probe syscon */
> > +     regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> > +                                              "loongson,ls1-syscon");
> > +     if (IS_ERR(regmap)) {
>
> > +             ret = PTR_ERR(regmap);
> > +             dev_err(&pdev->dev, "Unable to map syscon: %d\n", ret);
> > +             return ret;
>
> return dev_err_probe(&pdev->dev, PTR_ERR(regmap), "Unable to find syscon\n");
>
> > +     }
> > +
>
> > +     syscon = of_device_get_match_data(&pdev->dev);
>
> init = of_device_get_match_data(&pdev->dev);
>
> > +     if (!syscon) {
> > +             dev_err(&pdev->dev, "No of match data provided\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
> > +     if (!dwmac)
> > +             return -ENOMEM;
> > +
> > +     plat_dat = stmmac_probe_config_dt(pdev, stmmac_res.mac);
> > +     if (IS_ERR(plat_dat)) {
> > +             dev_err(&pdev->dev, "dt configuration failed\n");
> > +             return PTR_ERR(plat_dat);
> > +     }
> > +
> > +     plat_dat->bsp_priv = dwmac;
>
> > +     plat_dat->init = ls1x_dwmac_init;
>
> plat_dat->init = init;
>
> > +     dwmac->dev = &pdev->dev;
> > +     dwmac->plat_dat = plat_dat;
> > +     dwmac->syscon = syscon;
> > +     dwmac->regmap = regmap;
> > +
> > +     ret = stmmac_pltfr_probe(pdev, plat_dat, &stmmac_res);
> > +     if (ret)
> > +             goto err_remove_config_dt;
> > +
> > +     return 0;
> > +
> > +err_remove_config_dt:
> > +     stmmac_remove_config_dt(pdev, plat_dat);
> > +
> > +     return ret;
> > +}
> > +
> > +static const struct of_device_id ls1x_dwmac_match[] = {
>
> > +     { .compatible = "loongson,ls1b-gmac", .data = &ls1b_dwmac_syscon, },
> > +     { .compatible = "loongson,ls1c-emac", .data = &ls1c_dwmac_syscon, },
>
> { .compatible = "loongson,ls1b-gmac", .data = &ls1b_dwmac_syscon_init, },
> { .compatible = "loongson,ls1c-emac", .data = &ls1c_dwmac_syscon_init, },
>
> Thus you can simplify the driver by:
> 1. dropping ls1x_dwmac_syscon definition and its instances.
> 2. dropping three redundant fields from the ls1x_dwmac structure.
> 3. dropping the ls1x_dwmac_init() method.
> Sounds like worth it.)
>
> Note if no further driver update is planned then you can drop the
> ls1x_dwmac->dev field too. Otherwise you can keep it seeing some of
> the plat_stmmacenet_data data callbacks do have any device instance
> passed.
>
All done in v4.
Thanks!

> -Serge(y)
>
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ls1x_dwmac_match);
> > +
> > +static struct platform_driver ls1x_dwmac_driver = {
> > +     .probe = ls1x_dwmac_probe,
> > +     .remove_new = stmmac_pltfr_remove,
> > +     .driver = {
> > +             .name = "loongson1-dwmac",
> > +             .of_match_table = ls1x_dwmac_match,
> > +     },
> > +};
> > +module_platform_driver(ls1x_dwmac_driver);
> > +
> > +MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
> > +MODULE_DESCRIPTION("Loongson1 DWMAC glue layer");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.39.2
> >
> >



-- 
Best regards,

Keguang Zhang

[PATCH v3 4/4] MAINTAINERS: Update MIPS/LOONGSON1 entry

[Keguang Zhang]

Add two new F: entries for Loongson1 Ethernet driver
and dt-binding document.
Add a new F: entry for the rest Loongson-1 dt-binding documents.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
V2 -> V3: Update the entries and the commit message
V1 -> V2: Improve the commit message

 MAINTAINERS | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 250c43c675cb..f462f3d19e4a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14341,9 +14341,12 @@ MIPS/LOONGSON1 ARCHITECTURE
 M:	Keguang Zhang <keguang.zhang@gmail.com>
 L:	linux-mips@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/*/loongson,ls1x-*.yaml
+F:	Documentation/devicetree/bindings/net/loongson,ls1[bc]-*.yaml
 F:	arch/mips/include/asm/mach-loongson32/
 F:	arch/mips/loongson32/
 F:	drivers/*/*loongson1*
+F:	drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c
 
 MIPS/LOONGSON2EF ARCHITECTURE
 M:	Jiaxun Yang <jiaxun.yang@flygoat.com>
-- 
2.39.2

Re: [PATCH v3 4/4] MAINTAINERS: Update MIPS/LOONGSON1 entry

[Krzysztof Kozlowski]

On 24/08/2023 14:50, Keguang Zhang wrote:
> Add two new F: entries for Loongson1 Ethernet driver
> and dt-binding document.
> Add a new F: entry for the rest Loongson-1 dt-binding documents.
> 
> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> ---
> V2 -> V3: Update the entries and the commit message
> V1 -> V2: Improve the commit message
> 
>  MAINTAINERS | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 250c43c675cb..f462f3d19e4a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14341,9 +14341,12 @@ MIPS/LOONGSON1 ARCHITECTURE
>  M:	Keguang Zhang <keguang.zhang@gmail.com>
>  L:	linux-mips@vger.kernel.org
>  S:	Maintained
> +F:	Documentation/devicetree/bindings/*/loongson,ls1x-*.yaml
> +F:	Documentation/devicetree/bindings/net/loongson,ls1[bc]-*.yaml

This should be just one pattern */loongson,* or even just N: loongson,
if you want to cover any future versions as well (not only ls1).

Best regards,
Krzysztof

Re: [PATCH v3 4/4] MAINTAINERS: Update MIPS/LOONGSON1 entry

[Keguang Zhang]

On Fri, Aug 25, 2023 at 2:47 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 24/08/2023 14:50, Keguang Zhang wrote:
> > Add two new F: entries for Loongson1 Ethernet driver
> > and dt-binding document.
> > Add a new F: entry for the rest Loongson-1 dt-binding documents.
> >
> > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > ---
> > V2 -> V3: Update the entries and the commit message
> > V1 -> V2: Improve the commit message
> >
> >  MAINTAINERS | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 250c43c675cb..f462f3d19e4a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -14341,9 +14341,12 @@ MIPS/LOONGSON1 ARCHITECTURE
> >  M:   Keguang Zhang <keguang.zhang@gmail.com>
> >  L:   linux-mips@vger.kernel.org
> >  S:   Maintained
> > +F:   Documentation/devicetree/bindings/*/loongson,ls1x-*.yaml
> > +F:   Documentation/devicetree/bindings/net/loongson,ls1[bc]-*.yaml
>
> This should be just one pattern */loongson,* or even just N: loongson,
> if you want to cover any future versions as well (not only ls1).
>
Got it.
Will change to "net/loongson,ls1*.yaml" in next version.
Thanks!

> Best regards,
> Krzysztof
>


-- 
Best regards,

Keguang Zhang