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

On 06/08/2026 21:55, Saravanakrishnan Krishnamoorthy wrote:
> From: Alex Ousherovitch <[email protected]>
> 
> Add device tree binding schema for the Rambus CryptoManager Hub (CMH)
> hardware crypto accelerator.  The binding describes the parent
> SoC-level node with its SIC register region and one mailbox@N child
> node per mailbox the host owns, each carrying a reg (mailbox instance
> index), an optional interrupt, VCQ ring geometry (rambus,slots-log2 /
> rambus,strides-log2) and a rambus,cores affinity list.  Which crypto cores
> are present is discovered from the SIC CORE_ENABLE register at probe,
> not described in the device tree.
> 
> Register the 'rambus' vendor prefix for Rambus Inc.
> 
> Co-developed-by: Saravanakrishnan Krishnamoorthy <[email protected]>
> Signed-off-by: Saravanakrishnan Krishnamoorthy <[email protected]>

Completely messed order of tags.

> Signed-off-by: Alex Ousherovitch <[email protected]>

How Alex could sign after you?

> Reviewed-by: Joel Wittenauer <[email protected]>
> Reviewed-by: Thi Nguyen <[email protected]>

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?


> ---
>  .../bindings/crypto/rambus,cmh.yaml           | 207 ++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
>  2 files changed, 209 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/crypto/rambus,cmh.yaml
> 
> diff --git a/Documentation/devicetree/bindings/crypto/rambus,cmh.yaml b/Documentation/devicetree/bindings/crypto/rambus,cmh.yaml
> new file mode 100644
> index 000000000000..2709e20fb76f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/crypto/rambus,cmh.yaml
> @@ -0,0 +1,207 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/crypto/rambus,cmh.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rambus CryptoManager Hub (CMH) Hardware Crypto Accelerator
> +
> +maintainers:
> +  - Alex Ousherovitch <[email protected]>
> +  - Saravanakrishnan Krishnamoorthy <[email protected]>
> +  - Joel Wittenauer <[email protected]>
> +
> +description: |
> +  The Rambus CryptoManager Hub (CMH) is a hardware cryptographic accelerator
> +  accessed via a mailbox-based VCQ (Virtual Command Queue) interface.  The
> +  host writes VCQ command sequences into per-mailbox DMA queue buffers and
> +  rings a doorbell; the CMH eSW processes them and signals completion via
> +  interrupt.
> +
> +  The management host statically partitions the hardware mailboxes across
> +  the SoC's host interfaces at integration time; the set of mailboxes a
> +  given host owns is therefore fixed and not runtime-discoverable (a
> +  mailbox locked to a host reads as unavailable in the SIC availability
> +  register).  Each owned mailbox is described by a child node.  Which
> +  crypto cores are present, by contrast, is a fixed silicon-build property
> +  the driver reads from the hardware at probe (the SIC CORE_ENABLE
> +  register), so cores are not described in the device tree.
> +
> +  CMH identifies the host that owns a mailbox by a hardware HOST ID
> +  presented on the bus with every access, and permits only that HOST ID
> +  to access a mailbox while it is locked.  An integration must therefore
> +  present a single, stable HOST ID for all accesses to a given mailbox,
> +  independent of which CPU issues them.  On SMP hosts whose interconnect
> +  encodes the issuing CPU in the HOST ID, the integration must provide a
> +  single consistent HOST ID for the set of CPUs that share a mailbox; the
> +  driver neither selects nor can override the HOST ID presented by the
> +  hardware.
> +
> +  Supported algorithm families: SHA-2, SHA-3, SM3, AES, SM4,
> +  ChaCha20-Poly1305, RSA, ECDSA, EdDSA, ECDH, SM2, ML-KEM, ML-DSA,
> +  SLH-DSA, LMS, XMSS, DRBG.
> +
> +properties:
> +  compatible:
> +    const: rambus,cmh-v1030
> +    description:
> +      Identifies the CryptoManager Hub v1.030 IP revision (register map and
> +      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.

Considering how variable the binding looks like, this needs real SoC
compatibles.

> +> +  reg:
> +    maxItems: 1
> +    description:
> +      SIC (System Interface Controller) MMIO region.  The registers of
> +      mailbox instance N are at offset N * 0x1000 within this region.
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 3
> +    description:
> +      Functional clocks driving the CryptoManager Hub.  The block gates its
> +      clocks internally (integrated clock-gating cell); the host does not
> +      gate them, so these describe the input pins only.  The names, in
> +      order, are "core" (the main functional clock), "core-div2" (a
> +      half-rate clock present only on configurations with
> +      side-channel-protected cores) and "rt" (the real-time tick clock

Don't repeat constraints in free form text.

> +      for the internal timer).  Absent on integrations where a separate
> +      management/power controller owns the clocks and Linux has no clock
> +      handle.

List the items instead with description. minItems stays.

> +
> +  clock-names:
> +    minItems: 1
> +    items:
> +      - const: core
> +      - const: core-div2
> +      - const: rt
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      Optional host-driven reset for the CryptoManager Hub.  The hub has
> +      two external, active-low reset inputs -- a power-on reset and a hard
> +      reset; where a board routes one of them to a host-controlled GPIO,
> +      that line is described here.  Present only on integrations where the
> +      Linux host is the management host; where a separate management
> +      controller owns reset (the usual case) it is absent and Linux does
> +      not drive reset.
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^mailbox@[0-9a-f]+$":
> +    type: object
> +    description:
> +      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?

> +    properties:
> +      reg:
> +        maxItems: 1
> +        description:
> +          0-based mailbox instance index.  The instance's registers are
> +          at reg * 0x1000 within the SIC region.
> +
> +      interrupts:
> +        maxItems: 1
> +        description:
> +          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.

> +
> +      rambus,slots-log2:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        minimum: 1
> +        maximum: 15
> +        description:
> +          log2 of the number of VCQ ring slots the driver provisions for

Driver stuff does not belong to DT.

> +          this mailbox in host DMA memory.  This is a per-board,
> +          per-mailbox host-memory layout choice programmed into the
> +          mailbox QUEUE/SLOTS registers, not a silicon constant --
> +          boards built around the same SoC (hence the same compatible)
> +          may provision different ring geometry, so it is described per
> +          mailbox here rather than derived from the compatible.  The
> +          upper bound is imposed by the hardware.  Optional; defaults to
> +          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?

> +
> +      rambus,strides-log2:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        minimum: 7
> +        maximum: 10
> +        description:
> +          log2 of the per-slot stride in bytes for this mailbox's VCQ
> +          ring.  Like rambus,slots-log2 this is a per-board layout
> +          choice, not derived from the compatible.  The range is imposed
> +          by the hardware.  Optional; defaults to 9 (512 bytes per slot).
> +


Best regards,
Krzysztof