diff mbox series

[net-next] dt-bindings: net: dsa: sja1105: convert to YAML schema

Message ID 20210531234735.1582031-1-olteanv@gmail.com
State New
Headers show
Series [net-next] dt-bindings: net: dsa: sja1105: convert to YAML schema | expand

Commit Message

Vladimir Oltean May 31, 2021, 11:47 p.m. UTC
From: Vladimir Oltean <vladimir.oltean@nxp.com>

The following issues exist with the device-specific sja1105,role-mac and
sja1105,role-phy:

(a) the "sja1105" is not a valid vendor prefix and should probably have
    been "nxp", but
(b) as per the discussion with Florian here:
    https://lore.kernel.org/netdev/20210201214515.cx6ivvme2tlquge2@skbuf/
    more phy-mode values similar to "revmii" can be added which denote
    that the port is in the role of a PHY (such as "revrmii"), making
    the sja1105,role-phy redundant. Because there are no upstream users
    (or any users at all, to my knowledge) of these properties, they
    could even be removed in a future commit as far as I am concerned.
(c) when I force-add sja1105,role-phy to a device tree for testing, the
    patternProperties matching does not work, it results in the following
    error:

ethernet-switch@2: ethernet-ports:port@1: 'sja1105,role-phy' does not match any of the regexes: 'pinctrl-[0-9]+'
        From schema: Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml

But what's even more interesting is that if I remove the
"additionalProperties: true" that dsa.yaml has, I get even more
validation errors coming from patternProperties not matching either,
from spi-controller.yaml:

ethernet-switch@2: 'compatible', 'mdio', 'reg', 'spi-cpol', 'spi-max-frequency' do not match any of the regexes: '^(ethernet-)?ports$', 'pinctrl-[0-9]+'

So... it is probably broken. Rob Herring says here:
https://lore.kernel.org/linux-spi/20210324181037.GB3320002@robh.at.kernel.org/

  I'm aware of the issue, but I don't have a solution for this situation.
  It's a problem anywhere we have a parent or bus binding defining
  properties for child nodes. For now, I'd just avoid it in the examples
  and we'll figure out how to deal with actual dts files later.

So that's what I did.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 .../bindings/net/dsa/nxp,sja1105.yaml         | 128 ++++++++++++++
 .../devicetree/bindings/net/dsa/sja1105.txt   | 156 ------------------
 2 files changed, 128 insertions(+), 156 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
 delete mode 100644 Documentation/devicetree/bindings/net/dsa/sja1105.txt

Comments

Rob Herring June 1, 2021, 1:32 p.m. UTC | #1
On Tue, 01 Jun 2021 02:47:35 +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The following issues exist with the device-specific sja1105,role-mac and
> sja1105,role-phy:
> 
> (a) the "sja1105" is not a valid vendor prefix and should probably have
>     been "nxp", but
> (b) as per the discussion with Florian here:
>     https://lore.kernel.org/netdev/20210201214515.cx6ivvme2tlquge2@skbuf/
>     more phy-mode values similar to "revmii" can be added which denote
>     that the port is in the role of a PHY (such as "revrmii"), making
>     the sja1105,role-phy redundant. Because there are no upstream users
>     (or any users at all, to my knowledge) of these properties, they
>     could even be removed in a future commit as far as I am concerned.
> (c) when I force-add sja1105,role-phy to a device tree for testing, the
>     patternProperties matching does not work, it results in the following
>     error:
> 
> ethernet-switch@2: ethernet-ports:port@1: 'sja1105,role-phy' does not match any of the regexes: 'pinctrl-[0-9]+'
>         From schema: Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> 
> But what's even more interesting is that if I remove the
> "additionalProperties: true" that dsa.yaml has, I get even more
> validation errors coming from patternProperties not matching either,
> from spi-controller.yaml:
> 
> ethernet-switch@2: 'compatible', 'mdio', 'reg', 'spi-cpol', 'spi-max-frequency' do not match any of the regexes: '^(ethernet-)?ports$', 'pinctrl-[0-9]+'
> 
> So... it is probably broken. Rob Herring says here:
> https://lore.kernel.org/linux-spi/20210324181037.GB3320002@robh.at.kernel.org/
> 
>   I'm aware of the issue, but I don't have a solution for this situation.
>   It's a problem anywhere we have a parent or bus binding defining
>   properties for child nodes. For now, I'd just avoid it in the examples
>   and we'll figure out how to deal with actual dts files later.
> 
> So that's what I did.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  .../bindings/net/dsa/nxp,sja1105.yaml         | 128 ++++++++++++++
>  .../devicetree/bindings/net/dsa/sja1105.txt   | 156 ------------------
>  2 files changed, 128 insertions(+), 156 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
>  delete mode 100644 Documentation/devicetree/bindings/net/dsa/sja1105.txt
> 

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

yamllint warnings/errors:
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:65:17: [warning] wrong indentation: expected 14 but found 16 (indentation)

dtschema/dtc warnings/errors:

See https://patchwork.ozlabs.org/patch/1485820

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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

pip3 install dtschema --upgrade

Please check and re-submit.
Rob Herring June 1, 2021, 9:34 p.m. UTC | #2
On Tue, Jun 01, 2021 at 02:47:35AM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The following issues exist with the device-specific sja1105,role-mac and
> sja1105,role-phy:
> 
> (a) the "sja1105" is not a valid vendor prefix and should probably have
>     been "nxp", but
> (b) as per the discussion with Florian here:
>     https://lore.kernel.org/netdev/20210201214515.cx6ivvme2tlquge2@skbuf/
>     more phy-mode values similar to "revmii" can be added which denote
>     that the port is in the role of a PHY (such as "revrmii"), making
>     the sja1105,role-phy redundant. Because there are no upstream users
>     (or any users at all, to my knowledge) of these properties, they
>     could even be removed in a future commit as far as I am concerned.
> (c) when I force-add sja1105,role-phy to a device tree for testing, the
>     patternProperties matching does not work, it results in the following
>     error:
> 
> ethernet-switch@2: ethernet-ports:port@1: 'sja1105,role-phy' does not match any of the regexes: 'pinctrl-[0-9]+'
>         From schema: Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml

I believe that would be from 'additionalProperties: false' under 
"^(ethernet-)?port@[0-9]+$" in dsa.yaml. If additional properties need 
to be allowed, then it needs to be changed to 'true'. But if the 
properties aren't really used, just removing them would be better. But 
maybe there's other DSA users with custom properties.

> 
> But what's even more interesting is that if I remove the
> "additionalProperties: true" that dsa.yaml has, I get even more
> validation errors coming from patternProperties not matching either,
> from spi-controller.yaml:

Why would you do that?

> 
> ethernet-switch@2: 'compatible', 'mdio', 'reg', 'spi-cpol', 'spi-max-frequency' do not match any of the regexes: '^(ethernet-)?ports$', 'pinctrl-[0-9]+'
> 
> So... it is probably broken. Rob Herring says here:
> https://lore.kernel.org/linux-spi/20210324181037.GB3320002@robh.at.kernel.org/
> 
>   I'm aware of the issue, but I don't have a solution for this situation.
>   It's a problem anywhere we have a parent or bus binding defining
>   properties for child nodes. For now, I'd just avoid it in the examples
>   and we'll figure out how to deal with actual dts files later.

That was mainly in reference to vendor specific SPI master properties. 

For 'spi-cpol', that generally shouldn't be needed. A given device 
generally only supports 1 mode and the driver should know that. IOW, it 
can be implied from the compatible. There's of course some exceptions.

For 'spi-max-frequency', just add 'spi-max-frequency: true' (or provide 
some constraints as to what the max is.

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
new file mode 100644
index 000000000000..312f859cc40c
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
@@ -0,0 +1,128 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/dsa/nxp,sja1105.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP SJA1105 Automotive Ethernet Switch Family Device Tree Bindings
+
+description:
+  The SJA1105 SPI interface requires a CS-to-CLK time (t2 in UM10944.pdf) of at
+  least one half of t_CLK. At an SPI frequency of 1MHz, this means a minimum
+  cs_sck_delay of 500ns. Ensuring that this SPI timing requirement is observed
+  depends on the SPI bus master driver.
+
+allOf:
+  - $ref: "dsa.yaml#"
+
+maintainers:
+  - Vladimir Oltean <vladimir.oltean@nxp.com>
+
+properties:
+  compatible:
+    enum:
+      - nxp,sja1105e
+      - nxp,sja1105t
+      - nxp,sja1105p
+      - nxp,sja1105q
+      - nxp,sja1105r
+      - nxp,sja1105s
+
+  reg:
+    maxItems: 1
+
+patternProperties:
+  "^(ethernet-)?ports$":
+    type: object
+
+    patternProperties:
+      "^(ethernet-)?port@[0-9]+$":
+        type: object
+        properties:
+
+          # By default (unless otherwise specified) a port is configured as MAC
+          # if it is driving a PHY (phy-handle is present) or as PHY if it is
+          # PHY-less (fixed-link specified, presumably because it is connected
+          # to a MAC). It is suggested to not use these bindings unless an
+          # explicit override is necessary (for example, if SJA1105 ports are at
+          # both ends of a MII/RMII PHY-less setup. In that case, one end would
+          # need to have sja1105,role-mac, and the other sja1105,role-phy).
+          sja1105,role-mac:
+            $ref: /schemas/types.yaml#/definitions/flag
+            description: |
+                Specifies whether the SJA1105 port is a clock source or sink
+                for this interface, according to the applicable standard
+                (applicable for MII and RMII, but not applicable for RGMII
+                where there are separate TX and RX clocks). In the case of
+                RGMII it affects the behavior regarding internal delays.
+                If the port is configured in the role of an RGMII MAC, it will
+                let the PHY apply internal RGMII delays according to the
+                phy-mode property, otherwise it will apply the delays itself.
+
+          sja1105,role-phy:
+            $ref: /schemas/types.yaml#/definitions/flag
+            description:
+                See sja1105,role-mac.
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-switch@1 {
+            reg = <0x1>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "nxp,sja1105t";
+
+            ethernet-ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    phy-handle = <&rgmii_phy6>;
+                    phy-mode = "rgmii-id";
+                    reg = <0>;
+                    /* Implicit "sja1105,role-mac;" */
+                };
+
+                port@1 {
+                    phy-handle = <&rgmii_phy3>;
+                    phy-mode = "rgmii-id";
+                    reg = <1>;
+                    /* Implicit "sja1105,role-mac;" */
+                };
+
+                port@2 {
+                    phy-handle = <&rgmii_phy4>;
+                    phy-mode = "rgmii-id";
+                    reg = <2>;
+                    /* Implicit "sja1105,role-mac;" */
+                };
+
+                port@3 {
+                    phy-mode = "rgmii-id";
+                    reg = <3>;
+                };
+
+                port@4 {
+                    ethernet = <&enet2>;
+                    phy-mode = "rgmii";
+                    reg = <4>;
+                    /* Implicit "sja1105,role-phy;" */
+
+                    fixed-link {
+                        speed = <1000>;
+                        full-duplex;
+                    };
+                };
+            };
+        };
+    };
diff --git a/Documentation/devicetree/bindings/net/dsa/sja1105.txt b/Documentation/devicetree/bindings/net/dsa/sja1105.txt
deleted file mode 100644
index 13fd21074d48..000000000000
--- a/Documentation/devicetree/bindings/net/dsa/sja1105.txt
+++ /dev/null
@@ -1,156 +0,0 @@ 
-NXP SJA1105 switch driver
-=========================
-
-Required properties:
-
-- compatible:
-	Must be one of:
-	- "nxp,sja1105e"
-	- "nxp,sja1105t"
-	- "nxp,sja1105p"
-	- "nxp,sja1105q"
-	- "nxp,sja1105r"
-	- "nxp,sja1105s"
-
-	Although the device ID could be detected at runtime, explicit bindings
-	are required in order to be able to statically check their validity.
-	For example, SGMII can only be specified on port 4 of R and S devices,
-	and the non-SGMII devices, while pin-compatible, are not equal in terms
-	of support for RGMII internal delays (supported on P/Q/R/S, but not on
-	E/T).
-
-Optional properties:
-
-- sja1105,role-mac:
-- sja1105,role-phy:
-	Boolean properties that can be assigned under each port node. By
-	default (unless otherwise specified) a port is configured as MAC if it
-	is driving a PHY (phy-handle is present) or as PHY if it is PHY-less
-	(fixed-link specified, presumably because it is connected to a MAC).
-	The effect of this property (in either its implicit or explicit form)
-	is:
-	- In the case of MII or RMII it specifies whether the SJA1105 port is a
-	  clock source or sink for this interface (not applicable for RGMII
-	  where there is a Tx and an Rx clock).
-	- In the case of RGMII it affects the behavior regarding internal
-	  delays:
-	  1. If sja1105,role-mac is specified, and the phy-mode property is one
-	     of "rgmii-id", "rgmii-txid" or "rgmii-rxid", then the entity
-	     designated to apply the delay/clock skew necessary for RGMII
-	     is the PHY. The SJA1105 MAC does not apply any internal delays.
-	  2. If sja1105,role-phy is specified, and the phy-mode property is one
-	     of the above, the designated entity to apply the internal delays
-	     is the SJA1105 MAC (if hardware-supported). This is only supported
-	     by the second-generation (P/Q/R/S) hardware. On a first-generation
-	     E or T device, it is an error to specify an RGMII phy-mode other
-	     than "rgmii" for a port that is in fixed-link mode. In that case,
-	     the clock skew must either be added by the MAC at the other end of
-	     the fixed-link, or by PCB serpentine traces on the board.
-	These properties are required, for example, in the case where SJA1105
-	ports are at both ends of a MII/RMII PHY-less setup. One end would need
-	to have sja1105,role-mac, while the other sja1105,role-phy.
-
-See Documentation/devicetree/bindings/net/dsa/dsa.txt for the list of standard
-DSA required and optional properties.
-
-Other observations
-------------------
-
-The SJA1105 SPI interface requires a CS-to-CLK time (t2 in UM10944) of at least
-one half of t_CLK. At an SPI frequency of 1MHz, this means a minimum
-cs_sck_delay of 500ns. Ensuring that this SPI timing requirement is observed
-depends on the SPI bus master driver.
-
-Example
--------
-
-Ethernet switch connected via SPI to the host, CPU port wired to enet2:
-
-arch/arm/boot/dts/ls1021a-tsn.dts:
-
-/* SPI controller of the LS1021 */
-&dspi0 {
-	sja1105@1 {
-		reg = <0x1>;
-		#address-cells = <1>;
-		#size-cells = <0>;
-		compatible = "nxp,sja1105t";
-		spi-max-frequency = <4000000>;
-		fsl,spi-cs-sck-delay = <1000>;
-		fsl,spi-sck-cs-delay = <1000>;
-		ports {
-			#address-cells = <1>;
-			#size-cells = <0>;
-			port@0 {
-				/* ETH5 written on chassis */
-				label = "swp5";
-				phy-handle = <&rgmii_phy6>;
-				phy-mode = "rgmii-id";
-				reg = <0>;
-				/* Implicit "sja1105,role-mac;" */
-			};
-			port@1 {
-				/* ETH2 written on chassis */
-				label = "swp2";
-				phy-handle = <&rgmii_phy3>;
-				phy-mode = "rgmii-id";
-				reg = <1>;
-				/* Implicit "sja1105,role-mac;" */
-			};
-			port@2 {
-				/* ETH3 written on chassis */
-				label = "swp3";
-				phy-handle = <&rgmii_phy4>;
-				phy-mode = "rgmii-id";
-				reg = <2>;
-				/* Implicit "sja1105,role-mac;" */
-			};
-			port@3 {
-				/* ETH4 written on chassis */
-				phy-handle = <&rgmii_phy5>;
-				label = "swp4";
-				phy-mode = "rgmii-id";
-				reg = <3>;
-				/* Implicit "sja1105,role-mac;" */
-			};
-			port@4 {
-				/* Internal port connected to eth2 */
-				ethernet = <&enet2>;
-				phy-mode = "rgmii";
-				reg = <4>;
-				/* Implicit "sja1105,role-phy;" */
-				fixed-link {
-					speed = <1000>;
-					full-duplex;
-				};
-			};
-		};
-	};
-};
-
-/* MDIO controller of the LS1021 */
-&mdio0 {
-	/* BCM5464 */
-	rgmii_phy3: ethernet-phy@3 {
-		reg = <0x3>;
-	};
-	rgmii_phy4: ethernet-phy@4 {
-		reg = <0x4>;
-	};
-	rgmii_phy5: ethernet-phy@5 {
-		reg = <0x5>;
-	};
-	rgmii_phy6: ethernet-phy@6 {
-		reg = <0x6>;
-	};
-};
-
-/* Ethernet master port of the LS1021 */
-&enet2 {
-	phy-connection-type = "rgmii";
-	status = "ok";
-	fixed-link {
-		speed = <1000>;
-		full-duplex;
-	};
-};