Re: [PATCH] dt-bindings: tpm: Convert cr50 binding to YAML

[Rob Herring]

On Tue, Dec 17, 2019 at 09:45:02AM -0800, Doug Anderson wrote:
> Hi,
> 
> On Mon, Dec 16, 2019 at 4:54 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
> >
> > This allows us to validate the dt binding to the implementation. Add the
> > interrupt property too, because that's required but nobody noticed when
> > the non-YAML binding was introduced.
> >
> > Cc: Andrey Pronin <apronin@xxxxxxxxxxxx>
> > Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> > ---
> >  .../bindings/security/tpm/google,cr50.txt     | 19 -------
> >  .../bindings/security/tpm/google,cr50.yaml    | 52 +++++++++++++++++++
> >  2 files changed, 52 insertions(+), 19 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/security/tpm/google,cr50.txt
> >  create mode 100644 Documentation/devicetree/bindings/security/tpm/google,cr50.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/security/tpm/google,cr50.txt b/Documentation/devicetree/bindings/security/tpm/google,cr50.txt
> > deleted file mode 100644
> > index cd69c2efdd37..000000000000
> > --- a/Documentation/devicetree/bindings/security/tpm/google,cr50.txt
> > +++ /dev/null
> > @@ -1,19 +0,0 @@
> > -* H1 Secure Microcontroller with Cr50 Firmware on SPI Bus.
> > -
> > -H1 Secure Microcontroller running Cr50 firmware provides several
> > -functions, including TPM-like functionality. It communicates over
> > -SPI using the FIFO protocol described in the PTP Spec, section 6.
> > -
> > -Required properties:
> > -- compatible: Should be "google,cr50".
> > -- spi-max-frequency: Maximum SPI frequency.
> > -
> > -Example:
> > -
> > -&spi0 {
> > -       tpm@0 {
> > -               compatible = "google,cr50";
> > -               reg = <0>;
> > -               spi-max-frequency = <800000>;
> > -       };
> > -};
> > diff --git a/Documentation/devicetree/bindings/security/tpm/google,cr50.yaml b/Documentation/devicetree/bindings/security/tpm/google,cr50.yaml
> > new file mode 100644
> > index 000000000000..8bfff0e757af
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/security/tpm/google,cr50.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/tpm/google,cr50.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: H1 Secure Microcontroller with Cr50 Firmware on SPI Bus
> > +
> > +description:
> > +  H1 Secure Microcontroller running Cr50 firmware provides several functions,
> > +  including TPM-like functionality. It communicates over SPI using the FIFO
> > +  protocol described in the PTP Spec, section 6.
> > +
> > +maintainers:
> > +  - Andrey Pronin <apronin@xxxxxxxxxxxx>
> 
> Does Andrey agree to be the maintainer here?
> 
> 
> I'd like to see if we can delete most of what you've written here.
> Specifically in "spi/spi-controller.yaml" you can see a really nice
> description of what SPI devices ought to look like.  Can we just
> reference that?  To do that I _think_ we actually need to break that
> description into a separate YAML file and then include it from there
> and here.  Maybe someone on the list can confirm or we can just post
> some patches for that?
> 
> 
> > +properties:
> > +  compatible:
> > +    const: google,cr50
> > +
> > +  reg:
> > +    maxItems: 1
> 
> I'm curious if you need a minItems here.  ...and if we don't somehow
> include it, should we follow 'spi-controller.yaml' and treat this like
> an int?

Really, just 'true' is sufficient as you can't say which CS number it is 
here.
> 
> 
> > +  spi-max-frequency:
> > +    maxItems: 1
> 
> This is not an array type.  Why do you need maxItems?  Should treat
> like an int?  Do we have any ranges of sane values we can put here?
> I'm sure that there is a maximum that Cr50 can talk at.
> 
> 
> > +  interrupts:
> > +    maxItems: 1
> 
> I'm curious if you need a minItems here.

No. It's implied to be the same. (The tooling adds it because that's not 
how json-schema works).

> 
> ...also: should we be trying to validate the flags at all?  AKA that
> Cr50 expects a rising edge interrupt?

You can't really because you don't know how many cells.

> 
> 
> > +required:
> > +  - compatible
> > +  - reg
> > +  - spi-max-frequency
> 
> Technically spi-max-frequency might not be required (the SPI binding
> doesn't list it as such), but I guess it was before...

Generally, we expect a device knows its max and this should only be used 
it a board has a lower value. However, sometimes there's exceptions. 

Shouldn't really be debate here unless the old binding doc was wrong.

> 
> 
> > +  - interrupts
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    spi {
> > +      #address-cells = <0x1>;
> > +      #size-cells = <0x0>;
> > +      tpm@0 {
> > +          compatible = "google,cr50";
> > +          reg = <0>;
> > +          spi-max-frequency = <800000>;
> > +          interrupts = <50 IRQ_TYPE_EDGE_RISING>;
> 
> I would tend to prefer seeing the interrupt parent in the example
> since it's pretty likely that the GPIO controller isn't the overall
> parent and likely that our interrupt is a GPIO.  I'm not sure the
> convention, though.

Example is fine, but shouldn't be in the schema.

> > +      };
> > +    };
> > +
> > +...
> 
> Is the "..." important here?  I guess this is only if you're trying to
> jam two bindings into the same file, but I could be wrong.  I guess a
> bunch of arm ones owned by Rob have it at the end (though the example
> doesn't?), so maybe it's right?

We won't ever have 2 because '$ref' lookups wouldn't work. 

It only matters that we are consistent because if/when we want to 
programatically edit files, the ruamel yaml parser writes out what it is 
told, not what it read in. That also means fixing these is pretty easy.

My current position is to have them, but it's not anything I check ATM 
and I forget it too.

Rob