Hi Krzysztof,

From: Krzysztof Kozlowski, Sent: Thursday, September 22, 2022 4:37 PM

On 21/09/2022 10:47, Yoshihiro Shimoda wrote:

quoted

Document Renesas Etherent Switch for R-Car S4-8 (r8a779f0).

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 .../bindings/net/renesas,etherswitch.yaml     | 286 ++++++++++++++++++
 1 file changed, 286 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/renesas,etherswitch.yaml

diff --git a/Documentation/devicetree/bindings/net/renesas,etherswitch.yaml

b/Documentation/devicetree/bindings/net/renesas,etherswitch.yaml

quoted

new file mode 100644
index 000000000000..988d14f5c54e

--- /dev/null
+++ b/Documentation/devicetree/bindings/net/renesas,etherswitch.yaml

Isn't dsa directory for this?

As Andrew mentioned, this is not a DSA driver.

quoted

@@ -0,0 +1,286 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/renesas,etherswitch.yaml#

Filename: renesas,r8a779f0-ether-switch.yaml

I'll rename this file.

quoted

+$schema:

<snip>

quoted

+
+title: Renesas Ethernet Switch
+
+maintainers:
+  - Yoshihiro Shimoda [off-list ref]
+
+properties:
+  compatible:
+    const: renesas,r8a779f0-ether-switch
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: base
+      - const: secure_base
+
+  interrupts:
+    maxItems: 47
+
+  interrupt-names:
+    items:
+      - const: mfwd_error
+      - const: race_error
+      - const: coma_error
+      - const: gwca0_error
+      - const: gwca1_error
+      - const: etha0_error
+      - const: etha1_error
+      - const: etha2_error
+      - const: gptp0_status
+      - const: gptp1_status
+      - const: mfwd_status
+      - const: race_status
+      - const: coma_status
+      - const: gwca0_status
+      - const: gwca1_status
+      - const: etha0_status
+      - const: etha1_status
+      - const: etha2_status
+      - const: rmac0_status
+      - const: rmac1_status
+      - const: rmac2_status
+      - const: gwca0_rxtx0
+      - const: gwca0_rxtx1
+      - const: gwca0_rxtx2
+      - const: gwca0_rxtx3
+      - const: gwca0_rxtx4
+      - const: gwca0_rxtx5
+      - const: gwca0_rxtx6
+      - const: gwca0_rxtx7
+      - const: gwca1_rxtx0
+      - const: gwca1_rxtx1
+      - const: gwca1_rxtx2
+      - const: gwca1_rxtx3
+      - const: gwca1_rxtx4
+      - const: gwca1_rxtx5
+      - const: gwca1_rxtx6
+      - const: gwca1_rxtx7
+      - const: gwca0_rxts0
+      - const: gwca0_rxts1
+      - const: gwca1_rxts0
+      - const: gwca1_rxts1
+      - const: rmac0_mdio
+      - const: rmac1_mdio
+      - const: rmac2_mdio
+      - const: rmac0_phy
+      - const: rmac1_phy
+      - const: rmac2_phy
+
+  clocks:
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: fck
+      - const: tsn
+
+  resets:
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: rswitch2
+      - const: tsn
+
+  iommus:
+    maxItems: 16
+
+  power-domains:
+    maxItems: 1
+
+  ethernet-ports:
+    type: object
+
+    properties:
+      '#address-cells':
+        description: Port number of ETHA (TSNA).
+        const: 1

Blank line

I'll add a blank line here.

quoted

+      '#size-cells':
+        const: 0
+
+    additionalProperties: false

Don't put it between properties. For nested object usually this is
before properties:

I'll drop it.

quoted

+
+    patternProperties:
+      "^port@[0-9a-f]+$":
+        type: object
+

Skip blank line.

I got it.

quoted

+        $ref: "/schemas/net/ethernet-controller.yaml#"

No need for quotes.

I'll drop the quotes.

quoted

+        unevaluatedProperties: false
+
+        properties:
+          reg:
+            description:
+              Port number of ETHA (TSNA).
+
+          phy-handle:
+            description:
+              Phandle of an Ethernet PHY.

Why do you need to mention this property? Isn't it coming from
ethernet-controller.yaml?

Indeed. I'll drop the description.

quoted

+
+          phy-mode:
+            description:
+              This specifies the interface used by the Ethernet PHY.
+            enum:
+              - mii
+              - sgmii
+              - usxgmii
+
+          phys:
+            maxItems: 1
+            description:
+              Phandle of an Ethernet SERDES.

This is getting confusing. You have now:
- phy-handle
- phy
- phy-device
- phys
in one schema... although lan966x serdes seems to do the same. :/

Yes... I found the following documents have "phy" and "phy-handle" by using
git grep -l -w "phys" `git grep -l phy-handle Documentation/devicetree/bindings/`:
Documentation/devicetree/bindings/net/cdns,macb.yaml
Documentation/devicetree/bindings/net/cpsw.txt
Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml
Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml
Documentation/devicetree/bindings/net/ti,cpsw-switch.yaml
Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
Documentation/devicetree/bindings/phy/phy-bindings.txt

And I'm interesting that the phy-bindings.txt said the following:
-----
phys : the phandle for the PHY device (used by the PHY subsystem; not to be
       confused with the Ethernet specific 'phy' and 'phy-handle' properties,
       see Documentation/devicetree/bindings/net/ethernet.txt for these)
-----

quoted

+
+          mdio:
+            $ref: "/schemas/net/mdio.yaml#"

No need for quotes.

I got it.

Are you sure this is property of each port? I don't
know the net/ethernet bindings that good, so I need to ask sometimes
basic questions. Other bindings seem to do it differently a bit.

Yes, each port has mdio bus.

quoted

+            unevaluatedProperties: false
+
+        required:
+          - phy-handle
+          - phy-mode
+          - phys
+          - mdio
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clock-names
+  - resets
+  - power-domains
+  - ethernet-ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/r8a779f0-cpg-mssr.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/power/r8a779f0-sysc.h>
+
+    ethernet@e6880000 {
+            compatible = "renesas,r8a779f0-ether-switch";

Wrong indentation. Use 4 spaces.

I'll fix it.

Best regards,
Yoshihiro Shimoda

quoted

+            reg = <0xe6880000 0x20000>, <0xe68c0000 0x20000>;
+            reg-names = "base", "secure_base";
+            interrupts = <GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>,

Best regards,
Krzysztof