Re: [PATCH 1/3] dt-bindings: crypto: Add Keem Bay OCS HCU bindings

[Daniele Alessandrelli <daniele.alessandrelli@linux.intel.com>]

Hi Rob,

Thanks for reviewing the patch.

On Mon, 2020-10-26 at 08:17 -0500, Rob Herring wrote:
> On Fri, Oct 16, 2020 at 06:27:57PM +0100, Daniele Alessandrelli
> wrote:
> > From: Declan Murphy <declan.murphy@intel.com>
> > 
> > Add device-tree bindings for the Intel Keem Bay Offload Crypto
> > Subsystem
> > (OCS) Hashing Control Unit (HCU) crypto driver.
> > 
> > Signed-off-by: Declan Murphy <declan.murphy@intel.com>
> > Signed-off-by: Daniele Alessandrelli <
> > daniele.alessandrelli@intel.com>
> > Acked-by: Mark Gross <mgross@linux.intel.com>
> > ---
> >  .../crypto/intel,keembay-ocs-hcu.yaml         | 52
> > +++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/crypto/intel,keembay-ocs-hcu.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/crypto/intel,keembay-ocs-
> > hcu.yaml b/Documentation/devicetree/bindings/crypto/intel,keembay-
> > ocs-hcu.yaml
> > new file mode 100644
> > index 000000000000..dd4b82ee872b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/crypto/intel,keembay-ocs-
> > hcu.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > http://devicetree.org/schemas/crypto/intel,keembay-ocs-hcu.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Intel Keem Bay OCS HCU Device Tree Bindings
> > +
> > +maintainers:
> > +  - Declan Murphy <declan.murphy@intel.com>
> > +  - Daniele Alessandrelli <deniele.alessandrelli@intel.com>
> 
> typo:                          ^?

How embarrassing :/

> 
> > +
> > +description: |
> 
> Can drop '|' if there's no formatting to preserve.

Thanks, I'll fix that.

> 
> > +  The Intel Keem Bay Offload and Crypto Subsystem (OCS) Hash
> > Control Unit (HCU)
> > +  crypto driver enables use of the hardware accelerated hashing
> > module embedded
> > +  in the Intel Movidius SoC code name Keem Bay, via the kernel
> > crypto API.
> 
> Don't put Linux details in bindings. Describe the h/w, not a driver.

I'll fix that.

> 
> > +
> > +properties:
> > +  compatible:
> > +    const: intel,keembay-ocs-hcu
> > +
> > +  reg:
> > +    items:
> > +      - description: The OCS HCU base register address
> > +
> > +  interrupts:
> > +    items:
> > +      - description: OCS HCU interrupt
> > +
> > +  clocks:
> > +    items:
> > +      - description: OCS clock
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    hcu@3000b000 {
> 
> crypto@...

I'll change it to crypto@3000b000, but I wonder: we are going to
upstream other Keem Bay crypto drivers (an AES driver and an ECC one,
in addition to this hash one), so, would it make sense to use something
like crypto-hash@ (and then in future crypto-aes@ and crypto-ecc@)? Or
should I use crypto@<reg> (with different reg vals) for all of them?

> 
> > +      compatible = "intel,keembay-ocs-hcu";
> > +      reg = <0x3000b000 0x1000>;
> > +      interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
> > +      clocks = <&scmi_clk 94>;
> > +    };
> > +
> > +...
> > -- 
> > 2.26.2
> >