On Wed, Aug 16, 2023 at 8:54 PM Serge Semin [off-list ref] wrote:

Hi Keguang

On Sat, Aug 12, 2023 at 11:11:33PM +0800, Keguang Zhang wrote:

quoted

Add devicetree binding document for Loongson-1 DWMAC glue layer.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
 .../bindings/net/loongson,ls1x-dwmac.yaml     | 98 +++++++++++++++++++
 .../devicetree/bindings/net/snps,dwmac.yaml   |  2 +
 2 files changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1x-dwmac.yaml

diff --git a/Documentation/devicetree/bindings/net/loongson,ls1x-dwmac.yaml b/Documentation/devicetree/bindings/net/loongson,ls1x-dwmac.yaml
new file mode 100644
index 000000000000..150799460599
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/loongson,ls1x-dwmac.yaml

@@ -0,0 +1,98 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/loongson,ls1x-dwmac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+

quoted

+title: Loongson-1 DWMAC glue layer

DT-schemas describe a device. It has nothing to do with the glue
driver/layer/whatever.

OK. But what about the MODULE_DESCRIPTION in dwmac-loongson1.c
MODULE_DESCRIPTION("Loongson1 DWMAC glue layer");
Should the two parts be aligned with each other?
If not, what's your suggestion then?

Also I suggest to add a brief device description in the
"description:" property and add there a brief info regarding the SoCs
the controllers can be found on, the DW (G)MAC IP-core version the
ethernet controllers are based on and if possible some data about the
synthesize parameters: SMA (MDIO-bus), Tx/Rx COE, DMA FIFOs size,
perfect and hash MAC-filters size, L3L4 frame filters availability,
PHY interfaces (MII, RMII, RGMII, etc), EEE support, IEEE 1588(-2008)
Timestamping support, PMT and Wake-up frame support, MAC Management
counters (MMC).

Note DMA FIFO sizes can be also constrained in the properties
"rx-fifo-depth" and "tx-fifo-depth"; perfect and hash MAC-filter sizes -
in "snps,perfect-filter-entries" and "snps,multicast-filter-bins".

OK. The description could be added in next version.

quoted

+
+maintainers:
+  - Keguang Zhang [off-list ref]
+
+select:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - loongson,ls1b-dwmac
+          - loongson,ls1c-dwmac
+  required:
+    - compatible
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - loongson,ls1b-dwmac
+          - loongson,ls1c-dwmac
+      - const: snps,dwmac-3.50a
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: stmmaceth
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-names:
+    const: macirq
+

quoted

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

I believe the property is supposed to have a vendor-specific name like
"loongson,ls1-syscon" or similar.

This has been fixed in v2.
Could you please review v2?
Thanks!

quoted

+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+  - phy-handle

quoted

+  - phy-mode

You may want to specify the enum-constraints with the value permitted
for the particular Loongson (G)MAC controller. Seeing ls1b and ls1c
imply different sets of the PHY-modes the constraints are better to be
defined in the allOf sub-schemas. Alternatively you can split the
DT-schema file into two: one for ls1b-dwmac, another one for
ls1c-dwmac. IMO the later option seems better.

The "phy-mode", as pointed by Krzysztof, is defined in
ethernet-controller and already required by snps,dwmac.
So I have dropped it in v2.
For allOf sub-schemas, do you mean something below?
allOf:
 - $ref: snps,dwmac.yaml#

 - if:
     properties:
       compatible:
         contains:
           const: loongson,ls1b-dwmac
   then:
     properties:
       phy-mode:
         enum:
           - mii
           - rgmii

 - if:
     properties:
       compatible:
         contains:
           const: loongson,ls1c-dwmac
   then:
     properties:
       phy-mode:
         enum:
           - mii
           - rmii

-Serge(y)

quoted

+  - 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-dwmac", "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";
+
+        phy-handle = <&phy0>;
+        phy-mode = "mii";
+
+        snps,pbl = <1>;
+        syscon = <&syscon>;
+
+        mdio {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "snps,dwmac-mdio";
+
+            phy0: ethernet-phy@0 {
+                reg = <0x0>;
+            };
+        };
+    };

diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index ddf9522a5dc2..e1a956cf171e 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml

@@ -66,6 +66,8 @@ properties:
         - ingenic,x2000-mac
         - loongson,ls2k-dwmac
         - loongson,ls7a-dwmac
+        - loongson,ls1b-dwmac
+        - loongson,ls1c-dwmac
         - qcom,qcs404-ethqos
         - qcom,sa8775p-ethqos
         - qcom,sc8280xp-ethqos
--

2.39.2

--
Best regards,

Keguang Zhang