On Sun, Jan 5, 2020 at 9:57 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Rob Herring (2019-12-20 15:10:40)
> > On Tue, Dec 17, 2019 at 09:45:02AM -0800, Doug Anderson wrote:
> > > On Mon, Dec 16, 2019 at 4:54 PM Stephen Boyd <[email protected]> wrote:
> > > > 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 <[email protected]>
> > >
> > > Does Andrey agree to be the maintainer here?
>
> I Cced Andrey in hopes of eliciting a response.
Yes, I finally can confirm I agree to be the maintainer.
>
> > >
> > >
> > > 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?
>
> I'm not sure what to do here.
>
> > >
> > >
> > > > +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.
>
> Ok.
>
> > >
> > >
> > > > +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.
>
> The old binding doc had it as required and the spi framework seems to
> bail out if this property isn't specified (see of_spi_parse_dt() for
> more details).
>
> >
> > >
> > >
> > > > + - 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.
>
> Ok. Will add an interrupt parent like
>
> interrupt-parent = <&gpio_controller>;
>
--
Andrey