Re: [PATCH 5/7] dt-bindings: arm: tegra: pmc: Restructure pad configuration node schema

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 10846 bytes --]

On Mon, Jul 10, 2023 at 03:57:53PM +0200, Thierry Reding wrote:
> On Fri, Jul 07, 2023 at 04:35:03PM -0600, Rob Herring wrote:
> > On Fri, Jul 07, 2023 at 03:17:09PM +0200, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > The pad configuration node schema in its current form can accidentally
> > > match other properties as well. Restructure the schema to better match
> > > how the device trees are using these.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > >  .../arm/tegra/nvidia,tegra20-pmc.yaml         | 181 ++++++++++++------
> > >  1 file changed, 120 insertions(+), 61 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.yaml b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.yaml
> > > index 82070d47ac7c..271aa8f80a65 100644
> > > --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.yaml
> > > +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.yaml
> > > @@ -245,69 +245,82 @@ properties:
> > >            - resets
> > >            - '#power-domain-cells'
> > >  
> > > -patternProperties:
> > > -  "^[a-f0-9]+-[a-f0-9]+$":
> > > +  pinmux:
> > >      type: object
> > > -    description:
> > > -      This is a Pad configuration node. On Tegra SOCs a pad is a set of
> > > -      pins which are configured as a group. The pin grouping is a fixed
> > > -      attribute of the hardware. The PMC can be used to set pad power state
> > > -      and signaling voltage. A pad can be either in active or power down mode.
> > > -      The support for power state and signaling voltage configuration varies
> > > -      depending on the pad in question. 3.3V and 1.8V signaling voltages
> > > -      are supported on pins where software controllable signaling voltage
> > > -      switching is available.
> > > -
> > > -      The pad configuration state nodes are placed under the pmc node and they
> > > -      are referred to by the pinctrl client properties. For more information
> > > -      see Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt.
> > > -      The pad name should be used as the value of the pins property in pin
> > > -      configuration nodes.
> > > -
> > > -      The following pads are present on Tegra124 and Tegra132
> > > -      audio, bb, cam, comp, csia, csb, cse, dsi, dsib, dsic, dsid, hdmi, hsic,
> > > -      hv, lvds, mipi-bias, nand, pex-bias, pex-clk1, pex-clk2, pex-cntrl,
> > > -      sdmmc1, sdmmc3, sdmmc4, sys_ddc, uart, usb0, usb1, usb2, usb_bias.
> > > -
> > > -      The following pads are present on Tegra210
> > > -      audio, audio-hv, cam, csia, csib, csic, csid, csie, csif, dbg,
> > > -      debug-nonao, dmic, dp, dsi, dsib, dsic, dsid, emmc, emmc2, gpio, hdmi,
> > > -      hsic, lvds, mipi-bias, pex-bias, pex-clk1, pex-clk2, pex-cntrl, sdmmc1,
> > > -      sdmmc3, spi, spi-hv, uart, usb0, usb1, usb2, usb3, usb-bias.
> > > -
> > >      properties:
> > > -      pins:
> > > -        $ref: /schemas/types.yaml#/definitions/string
> > > -        description: Must contain name of the pad(s) to be configured.
> > > -
> > > -      low-power-enable:
> > > -        $ref: /schemas/types.yaml#/definitions/flag
> > > -        description: Configure the pad into power down mode.
> > > -
> > > -      low-power-disable:
> > > -        $ref: /schemas/types.yaml#/definitions/flag
> > > -        description: Configure the pad into active mode.
> > > -
> > > -      power-source:
> > > -        $ref: /schemas/types.yaml#/definitions/uint32
> > > -        description:
> > > -          Must contain either TEGRA_IO_PAD_VOLTAGE_1V8 or
> > > -          TEGRA_IO_PAD_VOLTAGE_3V3 to select between signaling voltages.
> > > -          The values are defined in
> > > -          include/dt-bindings/pinctrl/pinctrl-tegra-io-pad.h.
> > > -          Power state can be configured on all Tegra124 and Tegra132
> > > -          pads. None of the Tegra124 or Tegra132 pads support signaling
> > > -          voltage switching.
> > > -          All of the listed Tegra210 pads except pex-cntrl support power
> > > -          state configuration. Signaling voltage switching is supported
> > > -          on below Tegra210 pads.
> > > -          audio, audio-hv, cam, dbg, dmic, gpio, pex-cntrl, sdmmc1,
> > > -          sdmmc3, spi, spi-hv, and uart.
> > > -
> > > -    required:
> > > -      - pins
> > > -
> > > -    additionalProperties: false
> > > +      status: true
> > 
> > If you need this, that's a bug in dtschema.
> 
> Looks like I don't need it here...
> 
> > 
> > > +
> > > +    additionalProperties:
> > > +      type: object
> > > +      description: |
> > > +        This is a pad configuration node. On Tegra SoCs a pad is a set of pins
> > > +        which are configured as a group. The pin grouping is a fixed attribute
> > > +        of the hardware. The PMC can be used to set pad power state and
> > > +        signaling voltage. A pad can be either in active or power down mode.
> > > +        The support for power state and signaling voltage configuration varies
> > > +        depending on the pad in question. 3.3V and 1.8V signaling voltages are
> > > +        supported on pins where software controllable signaling voltage
> > > +        switching is available.
> > > +
> > > +        The pad configuration state nodes are placed under the pmc node and
> > > +        they are referred to by the pinctrl client properties. For more
> > > +        information see:
> > > +
> > > +          Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > > +
> > > +        The pad name should be used as the value of the pins property in pin
> > > +        configuration nodes.
> > > +
> > > +        The following pads are present on Tegra124 and Tegra132:
> > > +
> > > +          audio, bb, cam, comp, csia, csb, cse, dsi, dsib, dsic, dsid, hdmi,
> > > +          hsic, hv, lvds, mipi-bias, nand, pex-bias, pex-clk1, pex-clk2,
> > > +          pex-cntrl, sdmmc1, sdmmc3, sdmmc4, sys_ddc, uart, usb0, usb1, usb2,
> > > +          usb_bias
> > > +
> > > +        The following pads are present on Tegra210:
> > > +
> > > +          audio, audio-hv, cam, csia, csib, csic, csid, csie, csif, dbg,
> > > +          debug-nonao, dmic, dp, dsi, dsib, dsic, dsid, emmc, emmc2, gpio,
> > > +          hdmi, hsic, lvds, mipi-bias, pex-bias, pex-clk1, pex-clk2, pex-cntrl,
> > > +          sdmmc1, sdmmc3, spi, spi-hv, uart, usb0, usb1, usb2, usb3, usb-bias
> > > +      additionalProperties: false
> > > +      properties:
> > > +        pins:
> > > +          $ref: /schemas/types.yaml#/definitions/string-array
> > > +          description: Must contain name of the pad(s) to be configured.
> > > +
> > > +        low-power-enable:
> > > +          $ref: /schemas/types.yaml#/definitions/flag
> > > +          description: Configure the pad into power down mode.
> > > +
> > > +        low-power-disable:
> > > +          $ref: /schemas/types.yaml#/definitions/flag
> > > +          description: Configure the pad into active mode.
> > > +
> > > +        power-source:
> > > +          $ref: /schemas/types.yaml#/definitions/uint32
> > > +          description: |
> > > +            Must contain either TEGRA_IO_PAD_VOLTAGE_1V8 or
> > > +            TEGRA_IO_PAD_VOLTAGE_3V3 to select between signaling voltages. The
> > > +            values are defined in:
> > > +
> > > +              include/dt-bindings/pinctrl/pinctrl-tegra-io-pad.h
> > > +
> > > +            Power state can be configured on all Tegra124 and Tegra132 pads.
> > > +            None of the Tegra124 or Tegra132 pads support signaling voltage
> > > +            switching. All of the listed Tegra210 pads except pex-cntrl support
> > > +            power state configuration. Signaling voltage switching is supported
> > > +            on the following Tegra210 pads:
> > > +
> > > +              audio, audio-hv, cam, dbg, dmic, gpio, pex-cntrl, sdmmc1, sdmmc3,
> > > +              spi, spi-hv, uart
> > > +
> > > +        phandle:
> > > +          $ref: /schemas/types.yaml#/definitions/uint32
> > 
> > ditto
> 
> ... and not this either. Might be some leftovers from testing with
> earlier versions of dtschema. I've dropped them now.

[Quoting the above in full for reference]

Scratch that. This isn't triggered by the example, but I do see a
warning when trying to validate DTS files that have pad configuration
nodes that are referenced by phandle. You can see this in various board
DTS files derived from tegra210.dtsi.

One slight improvement to this might be to do:

	phandle: true

instead. However, given that this is completely standard, maybe that's
not what we want.

Any idea what might be causing this to be required? We do have
additionalProperties: false, but I thought the likes of phandle, status,
etc. were exempt from that.

> > > +      required:
> > > +        - pins
> > >  
> > >  required:
> > >    - compatible
> > > @@ -316,6 +329,52 @@ required:
> > >    - clocks
> > >    - '#clock-cells'
> > >  
> > > +allOf:
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: nvidia,tegra124-pmc
> > > +    then:
> > > +      properties:
> > > +        pinmux:
> > > +          properties:
> > > +            status: true
> 
> However I do get complaints if I drop this one:
> 
> 	Documentation/devicetree/bindings/soc/tegra/nvidia,tegra20-pmc.yaml: allOf:0:then:properties:pinmux: 'anyOf' conditional failed, one must be fixed:
> 		'type' is a required property
> 		'properties' is a required property
> 		'patternProperties' is a required property
> 		hint: 'additionalProperties' depends on 'properties' or 'patternProperties'
> 		from schema $id: http://devicetree.org/meta-schemas/core.yaml#
> 	Documentation/devicetree/bindings/soc/tegra/nvidia,tegra20-pmc.yaml: allOf:1:then:properties:pinmux: 'anyOf' conditional failed, one must be fixed:
> 		'type' is a required property
> 		'properties' is a required property
> 		'patternProperties' is a required property
> 		hint: 'additionalProperties' depends on 'properties' or 'patternProperties'
> 		from schema $id: http://devicetree.org/meta-schemas/core.yaml#
> 
> Would you consider that a bug in dtschema as well that I should try to
> fix or is that expected?

I guess this is a special case in a way because typically we would use
patternProperties for this. However, there really aren't any rules as to
what's valid here as a name or not. We could of course make something up
but this is description that most closely reflects reality.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]