2022-08-16 18:46:24

by Rob Herring (Arm)

[permalink] [raw]
Subject: [RFC] 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(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6318-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6318-pinctrl.yaml
index ab019a1998e8..1792d07e497d 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6318-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6318-pinctrl.yaml
@@ -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#"

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm63268-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm63268-pinctrl.yaml
index 8c9d4668c8c4..7aa1b6738ebf 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm63268-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm63268-pinctrl.yaml
@@ -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";
};
};
diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
index a8e22ec02215..e42071abf282 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
@@ -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#"

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6358-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6358-pinctrl.yaml
index 35867355a47a..20dbb1a3c19b 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6358-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6358-pinctrl.yaml
@@ -24,6 +24,7 @@ patternProperties:
'-pins$':
type: object
$ref: pinmux-node.yaml#
+ unevaluatedProperties: false

properties:
function:
diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6362-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6362-pinctrl.yaml
index b584d4b27223..ac0e775383b9 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6362-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6362-pinctrl.yaml
@@ -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";
};
};
diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6368-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6368-pinctrl.yaml
index 229323d9237d..91f0ec45c871 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6368-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6368-pinctrl.yaml
@@ -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";
};
};
--
2.34.1


2022-08-25 15:54:00

by Jonas Gorski

[permalink] [raw]
Subject: Re: [RFC] dt-bindings: pinctrl: brcm: Ensure all child node properties are documented

On Tue, 16 Aug 2022 at 20:39, Rob Herring <[email protected]> wrote:
>
> 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.

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

2022-08-25 19:25:50

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [RFC] dt-bindings: pinctrl: brcm: Ensure all child node properties are documented

On Thu, Aug 25, 2022 at 10:23 AM Jonas Gorski <[email protected]> wrote:
>
> On Tue, 16 Aug 2022 at 20:39, Rob Herring <[email protected]> wrote:
> >
> > 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.
>
> 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.

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

Rob