RE: [PATCH v3 01/19] dt-bindings: crypto: add Rambus CryptoManager Hub

[Ousherovitch, Alex]

On 10/08/2026 01:16, Krzysztof Kozlowski wrote:
> > Co-developed-by: Saravanakrishnan Krishnamoorthy <skrishnamoorthy@xxxxxxxxxx>
> > Signed-off-by: Saravanakrishnan Krishnamoorthy <skrishnamoorthy@xxxxxxxxxx>
>
> Completely messed order of tags.
>
> > Signed-off-by: Alex Ousherovitch <aousherovitch@xxxxxxxxxx>
>
> How Alex could sign after you?
>
> > Reviewed-by: Joel Wittenauer <Joel.Wittenauer@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Thi Nguyen <thin@xxxxxxxxxx>
>
> Your SoB is the last.
>
> Are you sure these people reviewed THIS code instead of blanket-review
> for everything?
>
> Did they find any issues in the schema which you fixed?

Right - Alex authored; Saravanakrishnan co-developed and submitted. We will
fix the trailer order across the series to:

  Signed-off-by: Alex Ousherovitch <aousherovitch@xxxxxxxxxx>
  Co-developed-by: Saravanakrishnan Krishnamoorthy <skrishnamoorthy@xxxxxxxxxx>
  Signed-off-by: Saravanakrishnan Krishnamoorthy <skrishnamoorthy@xxxxxxxxxx>

We will also drop the internal Reviewed-by tags; reviewers can add them
on-list if they choose.

> > +      "<vendor>,<soc>-cmh" compatible first with "rambus,cmh-v1030" as a
> > +      fallback; that extended form will be added when such a platform is
> > +      upstreamed.
>
> Considering how variable the binding looks like, this needs real SoC
> compatibles.

Agreed - we will not keep a generic-only binding. The SoC-specific
"<vendor>,<soc>-cmh" compatible (with "rambus,cmh-v1030" as fallback) will be
added together with the corresponding in-tree DTS user once the integrating
SoC is upstreamable. We will keep refining the series on-list in the meantime
so it is ready to land with that user.

> > +      side-channel-protected cores) and "rt" (the real-time tick clock
> > +      for the internal timer).  Absent on integrations where a separate
> > +      management/power controller owns the clocks and Linux has no clock
> > +      handle.
>
> Don't repeat constraints in free form text.
>
> List the items instead with description. minItems stays.

Will do - we will drop the count-restating and "Absent on..." prose and
describe the clocks per item, keeping minItems: 1. The clocks are not a
strictly positional list (core-div2 is present only on SCA-core
configurations, while rt may be present without it), so the valid name
combinations stay enumerated in clock-names (oneOf).

> > +      One node per hardware mailbox owned by this host.  The mailboxes a
> > +      host owns are assigned by the management host at integration time
>
> So they are SoC-deducible.
>
> > +      and cannot be probed, so they are enumerated here.
>
> Where are mbox-cells?

Correct - the mailbox set is fixed at integration and lives in the SoC .dtsi;
We will reword this as a hardware description and drop the "cannot be probed"
framing. This is not a Linux mailbox provider (it exposes no channels to other
consumers), so there are no #mbox-cells. To avoid that connotation, we will
rename the "mailbox@" child nodes to "queue@".

> > +          Completion/error interrupt for this mailbox.  Optional; when no
>
> Don't repeat constraints in free form text.
>
> The schema tells if something is optional or not.
>
> > +          mailbox has an interrupt the driver falls back to polling.
>
> Driver is usually irrelevant here and that internal review which
> supposedly happen for this code should tell you this.

Agreed - we will cut this to "Completion/error interrupt for this mailbox." and
drop the optionality restatement and the driver-behavior description.

> > +          log2 of the number of VCQ ring slots the driver provisions for
>
> Driver stuff does not belong to DT.
>
> > +          6 (64 slots).
>
> Missing default. Anyway, I do not understand why standard number you
> express as log. slots-num is a number. Number is 1, 2 or 4 etc. Not
> logarithm of something. Do you say: I have log-8 dollars in my wallet?

Understood - we will express these as plain numbers with defaults instead of
log2:

  rambus,num-slots:         enum [2, 4, 8, ..., 32768], default 64
  rambus,slot-stride-bytes: enum [128, 256, 512, 1024], default 512

The driver converts to the register's log2 encoding internally. We will also
drop the "driver provisions" / "imposed by the hardware" wording and describe
them as the per-board host-memory ring geometry. The same applies to
rambus,strides-log2.

On your follow-up:

> Also filename should match compatible.

Agreed - we will rename the binding to rambus,cmh-v1030.yaml to match the
compatible.

Thanks for the review.

Regards,
Alex