Message-ID: <20241027223111.GA151367-robh@kernel.org>
Date: Sun, 27 Oct 2024 17:31:11 -0500
From: Rob Herring <robh@...nel.org>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Conor Dooley <conor@...nel.org>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	"David S. Miller" <davem@...emloft.net>,
	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 07:38:26PM +0200, Christian Marangi wrote:
> 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?

I think the list is fine as-is. If we already had support for the 'i' 
version and then added new 'iX' versions, then having the fallback makes 
sense. But since this is all new and defined at one time, I don't think 
defining all those combinations buys us anything. And an OS can still 
choose to only support the 'i' features even if the h/w supports the e, 
s, and/or w features.

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

The issue is that airoha only has 1 implementation of safexcel, not any 
of them which is what you defined. Well, at least if that's an SoC 
specific compatible which it doesn't look like either.

The issue is that you probably don't have an actual user for all the 
variants. I would just list them commented out or like this:

items:
  - false (tools might complain with this. "not: {}" may work instead)
  - enum:
      # IP versions without an SoC specific compatible defined yet
      - inside-secure,safexcel-eip93i
      - inside-secure,safexcel-eip93ie
      - inside-secure,safexcel-eip93is
      - inside-secure,safexcel-eip93ies
      - inside-secure,safexcel-eip93iw

(So remove the one you use in this list)

Rob

Powered by blists - more mailing lists