dt-bindings: pinctrl: brcm: Ensure all child node properties are documented

RFC In review Rob Herring 2 participants 3 messages Aug 16, 2022 – Aug 25, 2022

Subsystems (3)

Device tree bindings GPIO Pin control

lore

Share & tools

lore

Rob Herring

author maintainer

2022-08-16 18:39 UTC #0 lore raw

RFC 1/1 dt-bindings: pinctrl: brcm: Ensure all child node properties are documented

The Broadcom pinctrl bindings are incomplete for child nodes as they are missing 'unevaluatedProperties: false' to prevent unknown properties. Fixing this reveals many warnings including having grandchild nodes in some cases.

Signed-off-by Rob Herring <[email protected]>

---
This recursive schema requires a fix not yet committed in dtschema.

I'm looking for feedback on whether group->pins or group->groups is the right fix here. There's more warnings with this change in the gpio-sysctl bindings.

---
 .../pinctrl/brcm,bcm6318-pinctrl.yaml         |  5 ++++
 .../pinctrl/brcm,bcm63268-pinctrl.yaml        | 23 +++++++++++--------
 .../pinctrl/brcm,bcm6328-pinctrl.yaml         |  5 ++++
 .../pinctrl/brcm,bcm6358-pinctrl.yaml         |  1 +
 .../pinctrl/brcm,bcm6362-pinctrl.yaml         |  7 +++++-
 .../pinctrl/brcm,bcm6368-pinctrl.yaml         |  7 +++++-
 6 files changed, 37 insertions(+), 11 deletions(-)

Documentation/devicetree/bindings/pinctrl/brcm,bcm6318-pinctrl.yaml +5 -0

index ab019a1998e8..1792d07e497d 100644
@@ -24,6 +24,7 @@ patternProperties:
   '-pins$':
     type: object
     $ref: pinmux-node.yaml#
+    additionalProperties: false
 
     properties:
       function:

@@ -37,6 +38,10 @@ patternProperties:
         enum: [ gpio0, gpio1, gpio2, gpio3, gpio4, gpio5, gpio6, gpio7,
                 gpio8, gpio9, gpio10, gpio11, gpio12, gpio13, gpio40 ]
 
+    patternProperties:
+      '-pins$':
+        $ref: '#/patternProperties/-pins$'
+
 allOf:
   - $ref: "pinctrl.yaml#"

Documentation/devicetree/bindings/pinctrl/brcm,bcm63268-pinctrl.yaml +14 -9

index 8c9d4668c8c4..7aa1b6738ebf 100644
@@ -24,6 +24,7 @@ patternProperties:
   '-pins$':
     type: object
     $ref: pinmux-node.yaml#
+    unevaluatedProperties: false
 
     properties:
       function:

@@ -41,6 +42,10 @@ patternProperties:
                 vdsl_phy_override_1_grp, vdsl_phy_override_2_grp,
                 vdsl_phy_override_3_grp, dsl_gpio8, dsl_gpio9 ]
 
+    patternProperties:
+      '-pins$':
+        $ref: '#/patternProperties/-pins$'
+
 allOf:
   - $ref: "pinctrl.yaml#"

@@ -122,46 +127,46 @@ examples:
 
       pinctrl_nand: nand-pins {
         function = "nand";
-        group = "nand_grp";
+        pins = "nand_grp";
       };
 
       pinctrl_gpio35_alt: gpio35_alt-pins {
         function = "gpio35_alt";
-        pin = "gpio35";
+        pins = "gpio35";
       };
 
       pinctrl_dectpd: dectpd-pins {
         function = "dectpd";
-        group = "dectpd_grp";
+        pins = "dectpd_grp";
       };
 
       pinctrl_vdsl_phy_override_0: vdsl_phy_override_0-pins {
         function = "vdsl_phy_override_0";
-        group = "vdsl_phy_override_0_grp";
+        pins = "vdsl_phy_override_0_grp";
       };
 
       pinctrl_vdsl_phy_override_1: vdsl_phy_override_1-pins {
         function = "vdsl_phy_override_1";
-        group = "vdsl_phy_override_1_grp";
+        pins = "vdsl_phy_override_1_grp";
       };
 
       pinctrl_vdsl_phy_override_2: vdsl_phy_override_2-pins {
         function = "vdsl_phy_override_2";
-        group = "vdsl_phy_override_2_grp";
+        pins = "vdsl_phy_override_2_grp";
       };
 
       pinctrl_vdsl_phy_override_3: vdsl_phy_override_3-pins {
         function = "vdsl_phy_override_3";
-        group = "vdsl_phy_override_3_grp";
+        pins = "vdsl_phy_override_3_grp";
       };
 
       pinctrl_dsl_gpio8: dsl_gpio8-pins {
         function = "dsl_gpio8";
-        group = "dsl_gpio8";
+        pins = "dsl_gpio8";
       };
 
       pinctrl_dsl_gpio9: dsl_gpio9-pins {
         function = "dsl_gpio9";
-        group = "dsl_gpio9";
+        pins = "dsl_gpio9";
       };
     };

Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml +5 -0

index a8e22ec02215..e42071abf282 100644
@@ -24,6 +24,7 @@ patternProperties:
   '-pins$':
     type: object
     $ref: pinmux-node.yaml#
+    unevaluatedProperties: false
 
     properties:
       function:

@@ -36,6 +37,10 @@ patternProperties:
                 gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1,
                 usb_port1 ]
 
+    patternProperties:
+      '-pins$':
+        $ref: '#/patternProperties/-pins$'
+
 allOf:
   - $ref: "pinctrl.yaml#"

Documentation/devicetree/bindings/pinctrl/brcm,bcm6358-pinctrl.yaml +1 -0

index 35867355a47a..20dbb1a3c19b 100644
@@ -24,6 +24,7 @@ patternProperties:
   '-pins$':
     type: object
     $ref: pinmux-node.yaml#
+    unevaluatedProperties: false
 
     properties:
       function:

Documentation/devicetree/bindings/pinctrl/brcm,bcm6362-pinctrl.yaml +6 -1

index b584d4b27223..ac0e775383b9 100644
@@ -24,6 +24,7 @@ patternProperties:
   '-pins$':
     type: object
     $ref: pinmux-node.yaml#
+    unevaluatedProperties: false
 
     properties:
       function:

@@ -41,6 +42,10 @@ patternProperties:
                 gpio15, gpio16, gpio17, gpio18, gpio19, gpio20, gpio21,
                 gpio22, gpio23, gpio24, gpio25, gpio26, gpio27, nand_grp ]
 
+    patternProperties:
+      '-pins$':
+        $ref: '#/patternProperties/-pins$'
+
 allOf:
   - $ref: "pinctrl.yaml#"

@@ -204,6 +209,6 @@ examples:
 
       pinctrl_nand: nand-pins {
         function = "nand";
-        group = "nand_grp";
+        pins = "nand_grp";
       };
     };

Documentation/devicetree/bindings/pinctrl/brcm,bcm6368-pinctrl.yaml +6 -1

index 229323d9237d..91f0ec45c871 100644
@@ -24,6 +24,7 @@ patternProperties:
   '-pins$':
     type: object
     $ref: pinmux-node.yaml#
+    unevaluatedProperties: false
 
     properties:
       function:

@@ -42,6 +43,10 @@ patternProperties:
                 gpio24, gpio25, gpio26, gpio27, gpio28, gpio29, gpio30,
                 gpio31, uart1_grp ]
 
+    patternProperties:
+      '-pins$':
+        $ref: '#/patternProperties/-pins$'
+
 allOf:
   - $ref: "pinctrl.yaml#"

@@ -215,6 +220,6 @@ examples:
 
       pinctrl_uart1: uart1-pins {
         function = "uart1";
-        group = "uart1_grp";
+        pins = "uart1_grp";
       };
     };

Signature · 1 line

2.34.1

Jonas Gorski

2022-08-25 15:23 UTC #1 lore raw

Quoted 6 lines · Rob Herring

The Broadcom pinctrl bindings are incomplete for child nodes as they are missing 'unevaluatedProperties: false' to prevent unknown properties. Fixing this reveals many warnings including having grandchild nodes in some cases.

Quoted 7 lines

Signed-off-by Rob Herring <[email protected]>
---
This recursive schema requires a fix not yet committed in dtschema.
I'm looking for feedback on whether group->pins or group->groups is the right fix here. There's more warnings with this change in the gpio-sysctl bindings.

The answer is "yes", though pins is probably the closest for most.

bcm6318 has multiple field-per-pin registers, where each pin is controlled separately, with more fields than available GPIOs, and the pins outside the GPIO range controlling other functions, like switching the second USB port between host and client mode.

bcm6328/6362/6368/63268 have two registers. The first one enables an alternative function for the first 32 GPIOs, with a 1:1 mapping of bits to GPIO. The second one enables a function for whole, arbitrary groups. These groups can overlap, and may also target pins outside the first 32 GPIOs. The actual pins in use are not documented, and can sometimes be guessed/inferred by the function names (e.g. "GPIO35"), sometimes not ("NAND", "UTOPIA").

bcm6358 has only the groups register, which also includes non GPIO related functions, like inversing the MII clocks for the integrated macs.

Not supported is bcm6348, which would be the only one where groups would definitely make more sense: there are 5 groups of 8 GPIOs, where each group can be set to a certain function (but not all functions are valid for all groups). E.g. for PCI support, you would need to set the fields to PCI for groups 0, 1 and 3 (and 2 and 5 could be set to a different function). You can ignore this though, as bcm6348 is ancient (doesn't even support ADSL2+).

Hope that helps a bit.

Best regards,
Jonas

Rob Herring

author maintainer

2022-08-25 18:53 UTC #2 lore raw

Quoted 43 lines · Jonas Gorski

Quoted 6 lines · Rob Herring
The Broadcom pinctrl bindings are incomplete for child nodes as they are missing 'unevaluatedProperties: false' to prevent unknown properties. Fixing this reveals many warnings including having grandchild nodes in some cases.
Quoted 7 lines
Signed-off-by Rob Herring <[email protected]>
---
This recursive schema requires a fix not yet committed in dtschema.
I'm looking for feedback on whether group->pins or group->groups is the right fix here. There's more warnings with this change in the gpio-sysctl bindings.
The answer is "yes", though pins is probably the closest for most.
bcm6318 has multiple field-per-pin registers, where each pin is controlled separately, with more fields than available GPIOs, and the pins outside the GPIO range controlling other functions, like switching the second USB port between host and client mode.
bcm6328/6362/6368/63268 have two registers. The first one enables an alternative function for the first 32 GPIOs, with a 1:1 mapping of bits to GPIO. The second one enables a function for whole, arbitrary groups. These groups can overlap, and may also target pins outside the first 32 GPIOs. The actual pins in use are not documented, and can sometimes be guessed/inferred by the function names (e.g. "GPIO35"), sometimes not ("NAND", "UTOPIA").
bcm6358 has only the groups register, which also includes non GPIO related functions, like inversing the MII clocks for the integrated macs.
Not supported is bcm6348, which would be the only one where groups would definitely make more sense: there are 5 groups of 8 GPIOs, where each group can be set to a certain function (but not all functions are valid for all groups). E.g. for PCI support, you would need to set the fields to PCI for groups 0, 1 and 3 (and 2 and 5 could be set to a different function). You can ignore this though, as bcm6348 is ancient (doesn't even support ADSL2+).

Quoted 1 line

Hope that helps a bit.

Only in determining that someone else needs to fix this. Please send me patches.

Rob