Re: [PATCH v4 01/19] dt-bindings: crypto: add Rambus CryptoManager Hub

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3957 bytes --]

On Thu, Aug 27, 2026 at 01:39:35AM +0000, Ousherovitch, Alex wrote:
> On Wed, Aug 26, 2026 at 06:02:16PM +0100, Conor Dooley wrote:
> > On Tue, Aug 25, 2026 at 03:15:20PM -0700, Alex Ousherovitch wrote:
> > > +      command ABI).  A specific SoC integration should list its own
> > > +      "<vendor>,<soc>-cmh" compatible first with "rambus,cmh-v1030" as a
> > > +      fallback; that extended form will be added when such a platform is
> > > +      upstreamed.
> >
> > This commentary should just not be here.
> > Instead, do what's been done in sifive,clint.yaml for the clint2.
> 
> Will do -- I'll drop the prose and model the compatible on sifive,clint2
> so the schema itself requires an SoC-specific compatible ahead of the IP
> fallback:
> 
>     compatible:
>       items:
>         - {}
>         - const: rambus,cmh-v1030
> 
> Since that makes the two-item form the only valid one, I'll also move the
> binding example (and our test DTs) to it, using a placeholder SoC-specific
> compatible under the now-registered rambus prefix, e.g.
> 
>     compatible = "rambus,<soc>-cmh", "rambus,cmh-v1030";
> 
> and swap in the real "<vendor>,<soc>-cmh" string once the first integrating
> platform is upstreamed.

With the correct syntax, which I think the sifive example I copied from
doesn't have, but
Documentation/devicetree/bindings/crypto/inside-secure,safexcel-eip93.yaml
does, fake compatibles won't be permitted (and we would probably object
to seeing them too). What we want is the binding to force people to use
a soc-specific one to pass validation. Unfortunately this does mean you
can't have an example because it won't pass the validation, which is
annoying!

> 
> > > +      rambus,cores:
> > > +        $ref: /schemas/types.yaml#/definitions/uint32-array
> > > +        items:
> > > +          enum: [0x02, 0x03, 0x04, 0x05, 0x08, 0x09, 0x0a, 0x18]
> > [...]
> >
> > You could just make this a string array, and then you'd not need this
> > mapping of unintelligible hex values. Of course then your driver has to
> > do it, but I'd rather see understandable (and portable, since a new
> > revision could change the IDs) values in DT.
> 
> Agreed, that's clearly better.  The next revision makes rambus,cores a
> string array and maps the names to core IDs in the driver, so the
> numbering can change across IP revisions without touching the DT:
> 
>     rambus,cores:
>       $ref: /schemas/types.yaml#/definitions/string-array
>       items:
>         enum: [hc, aes, sm4, sm3, hcq, qse, pke, ccp]
> 
> (keeping the affinity semantics in the description -- a core appears on at
> most one queue, unlisted cores are load-balanced -- just without the hex
> table.)
> 
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - "#address-cells"
> > > +  - "#size-cells"
> >
> > Surely clocks are also mandatory?
> 
> They can't be mandatory for this binding -- there may be no clock provider
> for the node to reference:
> 
> - CMH is shared between hosts.  The SIC presents up to four host
>    interfaces; a designated management host owns the privileged
>    configuration (clock, reset and power) while the other hosts are
>    granted only a subset of mailboxes.  A node describing a non-management
>    host therefore has no clock/reset provider to point at -- those lines
>    are driven by the management host or an SoC power controller.
> 
> - Even where a provider exists, only the main "core" clock is always
>    fitted; "core-div2" is present only on builds that include the
>    side-channel-protected slow cores, and "rt" is the real-time tick,
>    which is why clock-names is a oneOf of subsets rather than a fixed
>    list.
> 
> So I'd keep clocks (and reset-gpios) optional.  I can add a one-line,
> hardware-framed note to the description if you'd like that rationale on
> record.

Please do, yeah.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]