diff mbox series

[3/9] dt-bindings: crypto: Add DT bindings documentation for sun8i-ce Crypto Engine

Message ID 20190906184551.17858-4-clabbe.montjoie@gmail.com (mailing list archive)
State New, archived
Headers show
Series crypto: add sun8i-ce driver for Allwinner crypto engine | expand

Commit Message

Corentin Labbe Sept. 6, 2019, 6:45 p.m. UTC
This patch adds documentation for Device-Tree bindings for the
Crypto Engine cryptographic accelerator driver.

Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
---
 .../bindings/crypto/allwinner,sun8i-ce.yaml   | 84 +++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml

Comments

Maxime Ripard Sept. 7, 2019, 4:01 a.m. UTC | #1
On Fri, Sep 06, 2019 at 08:45:45PM +0200, Corentin Labbe wrote:
> This patch adds documentation for Device-Tree bindings for the
> Crypto Engine cryptographic accelerator driver.
>
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> ---
>  .../bindings/crypto/allwinner,sun8i-ce.yaml   | 84 +++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
>
> diff --git a/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
> new file mode 100644
> index 000000000000..bd8ccedd6059
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml

So, usually we're using the first compatible supported here as the
name.

> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/crypto/allwinner,sun8i-ce.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allwinner Crypto Engine driver
> +
> +maintainers:
> +  - Corentin Labbe <clabbe@baylibre.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: allwinner,sun8i-h3-crypto
> +      - const: allwinner,sun8i-r40-crypto
> +      - const: allwinner,sun50i-a64-crypto
> +      - const: allwinner,sun50i-h5-crypto
> +      - const: allwinner,sun50i-h6-crypto

An enum would be better here, it provides a more obvious error
message.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +if:
> +  properties:
> +    compatible:
> +      contains:
> +        const: allwinner,sun50i-h6-crypto
> +then:
> +  clocks:
> +    items:
> +      - description: Bus clock
> +      - description: Module clock
> +      - description: MBus clock
> +
> +  clock-names:
> +    items:
> +      - const: ahb
> +      - const: mod
> +      - const: mbus

It looks like there's a reset line on the H6 as well for that
controller (register 0x68c of the CCU, "CE_BGR_REG").

> +else:
> +  clocks:
> +    items:
> +      - description: Bus clock
> +      - description: Module clock
> +
> +  clock-names:
> +    items:
> +      - const: ahb
> +      - const: mod
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: ahb

This prevents the usage of the additionalProperties property, which
you should really use.

What you can do instead is moving the clocks and clock-names
description under properties, with a minItems of 2 and a maxItems of
3. Then you can restrict the length of that property to either 2 or 3
depending on the case here.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Corentin Labbe Sept. 11, 2019, 6:31 p.m. UTC | #2
On Sat, Sep 07, 2019 at 07:01:16AM +0300, Maxime Ripard wrote:
> On Fri, Sep 06, 2019 at 08:45:45PM +0200, Corentin Labbe wrote:
> > This patch adds documentation for Device-Tree bindings for the
> > Crypto Engine cryptographic accelerator driver.
> >
> > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> > ---
> >  .../bindings/crypto/allwinner,sun8i-ce.yaml   | 84 +++++++++++++++++++
> >  1 file changed, 84 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
[...]
> > +else:
> > +  clocks:
> > +    items:
> > +      - description: Bus clock
> > +      - description: Module clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: ahb
> > +      - const: mod
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  reset-names:
> > +    const: ahb
> 
> This prevents the usage of the additionalProperties property, which
> you should really use.
> 
> What you can do instead is moving the clocks and clock-names
> description under properties, with a minItems of 2 and a maxItems of
> 3. Then you can restrict the length of that property to either 2 or 3
> depending on the case here.
> 

Hello

I fail to do this.
I do the following (keeped only clock stuff)
properties:

  clocks:
    items:
      - description: Bus clock
      - description: Module clock
      - description: MBus clock

  clock-names:
    items:
      - const: ahb
      - const: mod
      - const: mbus

if:
  properties:
    compatible:
      items:
        const: allwinner,sun50i-h6-crypto
then:
  properties:
      clocks:
        minItems: 3
        maxItems: 3
      clock-names:
        minItems: 3
        maxItems: 3
else:
  properties:
      clocks:
        minItems: 2
        maxItems: 2
      clock-names:
        minItems: 2
        maxItems: 2

With this, the dtb_check keep complain that a64 have two short clocks.

Regards
Maxime Ripard Sept. 12, 2019, 9:37 a.m. UTC | #3
Hi Corentin,

On Wed, Sep 11, 2019 at 08:31:58PM +0200, Corentin Labbe wrote:
> On Sat, Sep 07, 2019 at 07:01:16AM +0300, Maxime Ripard wrote:
> > On Fri, Sep 06, 2019 at 08:45:45PM +0200, Corentin Labbe wrote:
> > > This patch adds documentation for Device-Tree bindings for the
> > > Crypto Engine cryptographic accelerator driver.
> > >
> > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> > > ---
> > >  .../bindings/crypto/allwinner,sun8i-ce.yaml   | 84 +++++++++++++++++++
> > >  1 file changed, 84 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
> [...]
> > > +else:
> > > +  clocks:
> > > +    items:
> > > +      - description: Bus clock
> > > +      - description: Module clock
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: ahb
> > > +      - const: mod
> > > +
> > > +  resets:
> > > +    maxItems: 1
> > > +
> > > +  reset-names:
> > > +    const: ahb
> > 
> > This prevents the usage of the additionalProperties property, which
> > you should really use.
> > 
> > What you can do instead is moving the clocks and clock-names
> > description under properties, with a minItems of 2 and a maxItems of
> > 3. Then you can restrict the length of that property to either 2 or 3
> > depending on the case here.
> > 
> 
> Hello
> 
> I fail to do this.
> I do the following (keeped only clock stuff)
> properties:
> 
>   clocks:
>     items:
>       - description: Bus clock
>       - description: Module clock
>       - description: MBus clock

Add minItems: 2  and maxItems: 3 at the same level than items

> 
>   clock-names:
>     items:
>       - const: ahb
>       - const: mod
>       - const: mbus

And here as well

Something I missed earlier though was that we've tried to unify as
much as possible the ahb / apb / axi clocks around the bus name, it
would be great if you could do it.

> 
> if:
>   properties:
>     compatible:
>       items:
>         const: allwinner,sun50i-h6-crypto
> then:
>   properties:
>       clocks:
>         minItems: 3
>         maxItems: 3
>       clock-names:
>         minItems: 3
>         maxItems: 3

You don't need to duplicate the min and maxItems here

Maxime
Chen-Yu Tsai Sept. 12, 2019, 8:26 p.m. UTC | #4
On Thu, Sep 12, 2019 at 10:37 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi Corentin,
>
> On Wed, Sep 11, 2019 at 08:31:58PM +0200, Corentin Labbe wrote:
> > On Sat, Sep 07, 2019 at 07:01:16AM +0300, Maxime Ripard wrote:
> > > On Fri, Sep 06, 2019 at 08:45:45PM +0200, Corentin Labbe wrote:
> > > > This patch adds documentation for Device-Tree bindings for the
> > > > Crypto Engine cryptographic accelerator driver.
> > > >
> > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> > > > ---
> > > >  .../bindings/crypto/allwinner,sun8i-ce.yaml   | 84 +++++++++++++++++++
> > > >  1 file changed, 84 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
> > [...]
> > > > +else:
> > > > +  clocks:
> > > > +    items:
> > > > +      - description: Bus clock
> > > > +      - description: Module clock
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: ahb
> > > > +      - const: mod
> > > > +
> > > > +  resets:
> > > > +    maxItems: 1
> > > > +
> > > > +  reset-names:
> > > > +    const: ahb
> > >
> > > This prevents the usage of the additionalProperties property, which
> > > you should really use.
> > >
> > > What you can do instead is moving the clocks and clock-names
> > > description under properties, with a minItems of 2 and a maxItems of
> > > 3. Then you can restrict the length of that property to either 2 or 3
> > > depending on the case here.
> > >
> >
> > Hello
> >
> > I fail to do this.
> > I do the following (keeped only clock stuff)
> > properties:
> >
> >   clocks:
> >     items:
> >       - description: Bus clock
> >       - description: Module clock
> >       - description: MBus clock
>
> Add minItems: 2  and maxItems: 3 at the same level than items
>
> >
> >   clock-names:
> >     items:
> >       - const: ahb
> >       - const: mod
> >       - const: mbus
>
> And here as well
>
> Something I missed earlier though was that we've tried to unify as
> much as possible the ahb / apb / axi clocks around the bus name, it
> would be great if you could do it.

I think we also want to standardize "mbus" as "dram"?

ChenYu

> >
> > if:
> >   properties:
> >     compatible:
> >       items:
> >         const: allwinner,sun50i-h6-crypto
> > then:
> >   properties:
> >       clocks:
> >         minItems: 3
> >         maxItems: 3
> >       clock-names:
> >         minItems: 3
> >         maxItems: 3
>
> You don't need to duplicate the min and maxItems here
>
> Maxime
Maxime Ripard Sept. 12, 2019, 8:33 p.m. UTC | #5
On Thu, Sep 12, 2019 at 09:26:27PM +0100, Chen-Yu Tsai wrote:
> > >
> > >   clock-names:
> > >     items:
> > >       - const: ahb
> > >       - const: mod
> > >       - const: mbus
> >
> > And here as well
> >
> > Something I missed earlier though was that we've tried to unify as
> > much as possible the ahb / apb / axi clocks around the bus name, it
> > would be great if you could do it.
> 
> I think we also want to standardize "mbus" as "dram"?

Do we? The only user so far seems to be sun9i-de, while mbus has more
users. I don't really care though, both mbus and dram are pretty
generic to me. What makes you prefer dram over mbus?

Maxime
Chen-Yu Tsai Sept. 12, 2019, 8:37 p.m. UTC | #6
On Thu, Sep 12, 2019 at 9:33 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Thu, Sep 12, 2019 at 09:26:27PM +0100, Chen-Yu Tsai wrote:
> > > >
> > > >   clock-names:
> > > >     items:
> > > >       - const: ahb
> > > >       - const: mod
> > > >       - const: mbus
> > >
> > > And here as well
> > >
> > > Something I missed earlier though was that we've tried to unify as
> > > much as possible the ahb / apb / axi clocks around the bus name, it
> > > would be great if you could do it.
> >
> > I think we also want to standardize "mbus" as "dram"?
>
> Do we? The only user so far seems to be sun9i-de, while mbus has more
> users. I don't really care though, both mbus and dram are pretty
> generic to me. What makes you prefer dram over mbus?

Argh... it's actually "ram" we use the most. Both "dram" and "mbus"
have only one instance each.

ChenYu
Maxime Ripard Sept. 13, 2019, 12:11 p.m. UTC | #7
On Thu, Sep 12, 2019 at 09:37:17PM +0100, Chen-Yu Tsai wrote:
> On Thu, Sep 12, 2019 at 9:33 PM Maxime Ripard <mripard@kernel.org> wrote:
> > On Thu, Sep 12, 2019 at 09:26:27PM +0100, Chen-Yu Tsai wrote:
> > > > >
> > > > >   clock-names:
> > > > >     items:
> > > > >       - const: ahb
> > > > >       - const: mod
> > > > >       - const: mbus
> > > >
> > > > And here as well
> > > >
> > > > Something I missed earlier though was that we've tried to unify as
> > > > much as possible the ahb / apb / axi clocks around the bus name, it
> > > > would be great if you could do it.
> > >
> > > I think we also want to standardize "mbus" as "dram"?
> >
> > Do we? The only user so far seems to be sun9i-de, while mbus has more
> > users. I don't really care though, both mbus and dram are pretty
> > generic to me. What makes you prefer dram over mbus?
> 
> Argh... it's actually "ram" we use the most. Both "dram" and "mbus"
> have only one instance each.

Let's use ram then :)

Maxime
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
new file mode 100644
index 000000000000..bd8ccedd6059
--- /dev/null
+++ b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
@@ -0,0 +1,84 @@ 
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/crypto/allwinner,sun8i-ce.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner Crypto Engine driver
+
+maintainers:
+  - Corentin Labbe <clabbe@baylibre.com>
+
+properties:
+  compatible:
+    oneOf:
+      - const: allwinner,sun8i-h3-crypto
+      - const: allwinner,sun8i-r40-crypto
+      - const: allwinner,sun50i-a64-crypto
+      - const: allwinner,sun50i-h5-crypto
+      - const: allwinner,sun50i-h6-crypto
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+if:
+  properties:
+    compatible:
+      contains:
+        const: allwinner,sun50i-h6-crypto
+then:
+  clocks:
+    items:
+      - description: Bus clock
+      - description: Module clock
+      - description: MBus clock
+
+  clock-names:
+    items:
+      - const: ahb
+      - const: mod
+      - const: mbus
+else:
+  clocks:
+    items:
+      - description: Bus clock
+      - description: Module clock
+
+  clock-names:
+    items:
+      - const: ahb
+      - const: mod
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    const: ahb
+
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/sun50i-a64-ccu.h>
+    #include <dt-bindings/reset/sun50i-a64-ccu.h>
+
+    crypto: crypto@1c15000 {
+      compatible = "allwinner,sun8i-h3-crypto";
+      reg = <0x01c15000 0x1000>;
+      interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
+      clocks = <&ccu CLK_BUS_CE>, <&ccu CLK_CE>;
+      clock-names = "ahb", "mod";
+      resets = <&ccu RST_BUS_CE>;
+      reset-names = "ahb";
+    };
+