Message-ID: <671bd796.df0a0220.216be3.8504@mx.google.com>
Date: Fri, 25 Oct 2024 19:38:26 +0200
From: Christian Marangi <ansuelsmth@...il.com>
To: Conor Dooley <conor@...nel.org>
Cc: Herbert Xu <herbert@...dor.apana.org.au>,
	"David S. Miller" <davem@...emloft.net>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Antoine Tenart <atenart@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
	Waiman Long <longman@...hat.com>, Boqun Feng <boqun.feng@...il.com>,
	Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>, linux-crypto@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	llvm@...ts.linux.dev, upstream@...oha.com
Subject: Re: [PATCH v4 2/3] dt-bindings: crypto: Add Inside Secure SafeXcel
 EIP-93 crypto engine

On Fri, Oct 25, 2024 at 05:44:39PM +0100, Conor Dooley wrote:
> On Fri, Oct 25, 2024 at 11:47:23AM +0200, Christian Marangi wrote:
> > Add bindings for the Inside Secure SafeXcel EIP-93 crypto engine.
> > 
> > The IP is present on Airoha SoC and on various Mediatek devices and
> > other SoC under different names like mtk-eip93 or PKTE.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> > ---
> > Changes v4:
> > - Out of RFC
> 
> I left comments on v3, that I do not see addressed here.
>

I missed them sorry, I was confused with the other reply about RFC not
asking for comments. Let me copy the comments here so we can continue
here.

> > Changes v3:
> > - Add SoC compatible with generic one
> > Changes v2:
> > - Change to better compatible
> > - Add description for EIP93 models
> > 
> >  .../crypto/inside-secure,safexcel-eip93.yaml  | 63 +++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/crypto/inside-secure,safexcel-eip93.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/crypto/inside-secure,safexcel-eip93.yaml b/Documentation/devicetree/bindings/crypto/inside-secure,safexcel-eip93.yaml
> > new file mode 100644
> > index 000000000000..13341710ee31
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/crypto/inside-secure,safexcel-eip93.yaml
> > @@ -0,0 +1,63 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/crypto/inside-secure,safexcel-eip93.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Inside Secure SafeXcel EIP-93 cryptographic engine
> > +
> > +maintainers:
> > +  - Christian Marangi <ansuelsmth@...il.com>
> > +
> > +description: |
> > +  The Inside Secure SafeXcel EIP-93 is a cryptographic engine IP block
> > +  integrated in varios devices with very different and generic name from
> > +  PKTE to simply vendor+EIP93. The real IP under the hood is actually
> > +  developed by Inside Secure and given to license to vendors.
> > +
> > +  The IP block is sold with different model based on what feature are
> > +  needed and are identified with the final letter. Each letter correspond
> > +  to a specific set of feature and multiple letter reflect the sum of the
> > +  feature set.
> > +
> > +  EIP-93 models:
> > +    - EIP-93i: (basic) DES/Triple DES, AES, PRNG, IPsec ESP, SRTP, SHA1
> > +    - EIP-93ie: i + SHA224/256, AES-192/256
> > +    - EIP-93is: i + SSL/DTLS/DTLS, MD5, ARC4
> > +    - EIP-93ies: i + e + s
> > +    - EIP-93iw: i + AES-XCB-MAC, AES-CCM
> >
> This implies that you should have a non-trivial set of fallbacks, with
> the "i" model as the base for that. eg:
> 
> "ie", "i"
> "is", "i"
> "iw", "i"
> "ies", "ie, "is", "i" (I dunno which would be a better order here)
>

These info are what I found around since informations about models are very
scarce. The driver itself makes use of a bitmap in the IP to detect the
supported stuff so the meaning of this is really to comunicate the set
of feature mounted on the system.

Any hint on how to describe this better? I assume you refer to some kind
of yaml logic structure to put in the compatible?

> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: airoha,crypto-eip93
> > +      - enum:
> > +          - inside-secure,safexcel-eip93i
> > +          - inside-secure,safexcel-eip93ie
> > +          - inside-secure,safexcel-eip93is
> > +          - inside-secure,safexcel-eip93ies
> > +          - inside-secure,safexcel-eip93iw
> > +
>
> I don't really get what's going on here. Why is the first compatible the
> generic one? That seems suspect to me, as I doubt the crypto block on a
> particular SoC varies? I'd expect to see some soc-specific compatibles
> with a fallback to the inside-secure IP version that it integrates.
>

This was already discussed and hoped this solution was accepted (I
didn't get any reply in the other revision, so I'm probably wrong)

Everything started with:
- airoha,mtk-eip93

Was wrong as the compatible wasn't clear on what mtk was and if the IP
was from airoha (it's not, it's licensed to...)

Then only the inside-secure ones, following how it's done for the newer
inside-secure eip197.

Krzysztof then suggested that, since it's licensed but OEM can make
modification, it should be sensible to put a compatible of the SoC where
the thing is mounted at the front of the other compatible. Eip197 should
have received the same treatement but for some reason it didn't.

So here in v3/v4 with this proposed solution.

First compatible is SoC name, useful if the Vendor made modification to
the IP. Then the generic model that describe the set of feature
supported.

I checked the register of 3 different device where EIP93 is implemented
and they ALL match them. ONLY additional register are added for debug
purpose and never conflicting bits are introduced.

Hence why IMHO it's OK to use the combo of Vendor + second compatible
for the generic implementation.

>From both comments I'm not really sure what do you mean about fallback,
anyway hope it's clear now.

> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    crypto@...04000 {
> > +      compatible = "airoha,crypto-eip93", "inside-secure,safexcel-eip93ies";
> > +      reg = <0x1fb70000 0x1000>;
> > +
> > +      interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
> > +    };
> > -- 
> > 2.45.2
> > 



-- 
	Ansuel

Powered by blists - more mailing lists