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

On 10/08/2026 01:16, Krzysztof Kozlowski wrote:

quoted

Co-developed-by: Saravanakrishnan Krishnamoorthy <redacted>
Signed-off-by: Saravanakrishnan Krishnamoorthy <redacted>

Completely messed order of tags.

quoted

Signed-off-by: Alex Ousherovitch <redacted>

How Alex could sign after you?

quoted

Reviewed-by: Joel Wittenauer <redacted>
Reviewed-by: Thi Nguyen <redacted>

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 [off-list ref]
  Co-developed-by: Saravanakrishnan Krishnamoorthy [off-list ref]
  Signed-off-by: Saravanakrishnan Krishnamoorthy [off-list ref]

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+          log2 of the number of VCQ ring slots the driver provisions for

Driver stuff does not belong to DT.

quoted

+          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