Re: [PATCH RFC 04/18] dt-bindings: mailbox: add the A210 AP to E902 doorbell

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: mailbox: zhihe,a210-ap-mailbox models software SCMI usage in DT
--

commit e3663120f1bca9d6b72ccfedb9289dcb341101ef
Author: Han Gao <gaohan@iscas.ac.cn>

dt-bindings: mailbox: add the A210 AP to E902 doorbell

This patch adds the Devicetree binding schema for the Zhihe A210 application
processor to E902 doorbell mailbox controller.

> diff --git a/Documentation/devicetree/bindings/mailbox/zhihe,a210-ap-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/zhihe,a210-ap-mailbox.yaml
> new file mode 100644
> index 0000000000000..4541750223eb2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/zhihe,a210-ap-mailbox.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/zhihe,a210-ap-mailbox.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Zhihe A210 application processor to E902 doorbell
> +
> +maintainers:
> +  - Han Gao <gaohan@iscas.ac.cn>
> +
> +description: |
> +  The A210 exposes a bidirectional notification channel between the
> +  application processors and E902 firmware. Payloads and transaction
> +  completion are managed by the client using shared memory. The firmware
> +  initializes the remote endpoint before the application processor binds.
> +  When firmware manages the endpoint clocks and keeps them running, omit
> +  clocks and clock-names to avoid a dependency on services using this mailbox.
> +
> +properties:
> +  compatible:
> +    const: zhihe,a210-ap-mailbox
> +  reg:
> +    minItems: 2
> +    items:
> +      - description: Application processor interrupt status and control
> +      - description: Remote E902 notification channel
> +      - description:
> +          E902 to application processor channel data, which identifies the
> +          channel that raised the shared interrupt
> +  reg-names:
> +    minItems: 2
> +    items:
> +      - const: interrupt
> +      - const: tx
> +      - const: rx
> +  interrupts:
> +    maxItems: 1
> +  clocks:
> +    items:
> +      - description: Application processor mailbox clock
> +      - description: E902 mailbox clock
> +  clock-names:
> +    items:
> +      - const: local
> +      - const: remote
> +  '#mbox-cells':
> +    const: 1
> +    description: |
> +      Channel index. Channel 0 carries requests to E902 and their completion.
> +      Channel 1 receives E902 notifications and requires the rx region.

[ ... ]

> +examples:
> +  - |
> +    mailbox@321000 {
> +        compatible = "zhihe,a210-ap-mailbox";
> +        reg = <0x321000 0x1000>, <0x311000 0x1000>, <0x320000 0x1000>;
> +        reg-names = "interrupt", "tx", "rx";

[Severity: Medium]
Does this binding model software-specific SCMI usage and driver probe
workarounds rather than the physical A210 ICU mailbox hardware?

Looking at the example and the corresponding driver in
drivers/mailbox/mailbox-a210.c alongside drivers/mailbox/mailbox-th1520.c, the
A210 uses the ICU mailbox architecture where 0x320000 (size 0x1000, the rx
INFO registers from E902) and 0x321000 (size 0x1000, the AP interrupt status,
clear, and mask registers) are adjacent 4 KiB slices of the same local AP
mailbox peripheral starting at 0x320000, while 0x311000 (size 0x1000, tx) is
the remote E902 ICU slice for the AP.

By splitting the contiguous local AP mailbox region out of address order and
placing 0x321000 first so that 0x320000 can be an optional third reg entry via
minItems: 2, the node unit-address becomes an interior offset (mailbox@321000
instead of mailbox@320000), and a physically present hardware register bank
(0x320000) is marked optional depending on whether a client uses channel 1.

In addition, the description and #mbox-cells properties define the mailbox
channels in terms of SCMI shared-memory protocol roles (channel 0 for requests
and completion, channel 1 for notifications requiring the rx region), and the
description instructs DT authors to omit clocks and clock-names when firmware
keeps the clocks running to avoid a circular dependency on services using this
mailbox.

If those clocks are controlled via SCMI over this mailbox, referencing them
from the mailbox node creates a circular dependency that cannot resolve before
probe; if they are physical SoC clock inputs, they are fixed hardware
connections that should not be conditionally omitted based on OS probe
ordering.

Should the binding instead describe the physical hardware layout (starting at
0x320000 with non-optional physical register regions) and hardware channel
endpoints without encoding SCMI client protocol behavior or clock omission
workarounds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-a210-scmi-v1-0-74d58ce85bd8@iscas.ac.cn?part=4