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

[Doug Anderson]

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?


> +  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.

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


> +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...


> +  - 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.


> +      };
> +    };
> +
> +...

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?