Re: [PATCH v4 2/3] dt-bindings: mailbox: Add thead,th1520-mailbox bindings

[Michal Wilczynski]

On 10/16/24 01:14, Samuel Holland wrote:
> Hi Michal,
> 
> On 2024-10-14 7:33 AM, Michal Wilczynski wrote:
>> Add bindings for the mailbox controller. This work is based on the vendor
>> kernel. [1]
>>
>> Link: https://protect2.fireeye.com/v1/url?k=deccc9a8-815707cc-decd42e7-000babda0201-ff79d4aaff73f36c&q=1&e=7028c1ef-1c3d-4e17-b7ed-76d362b80caf&u=https%3A%2F%2Fgithub.com%2Frevyos%2Fthead-kernel.git [1]
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
>> ---
>>  .../bindings/mailbox/thead,th1520-mbox.yaml   | 80 +++++++++++++++++++
>>  MAINTAINERS                                   |  1 +
>>  2 files changed, 81 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
>> new file mode 100644
>> index 000000000000..12507c426691
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
>> @@ -0,0 +1,80 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: https://protect2.fireeye.com/v1/url?k=7fcda92a-2056674e-7fcc2265-000babda0201-c5d541bdd4cc5f35&q=1&e=7028c1ef-1c3d-4e17-b7ed-76d362b80caf&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fmailbox%2Fthead%2Cth1520-mbox.yaml%23
>> +$schema: https://protect2.fireeye.com/v1/url?k=068c7881-5917b6e5-068df3ce-000babda0201-b045bd7290e64f0e&q=1&e=7028c1ef-1c3d-4e17-b7ed-76d362b80caf&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>> +
>> +title: T-head TH1520 Mailbox Controller
>> +
>> +description:
>> +  The T-head mailbox controller enables communication and coordination between
>> +  cores within the SoC by passing messages (e.g., data, status, and control)
>> +  through mailbox channels. It also allows one core to signal another processor
>> +  using interrupts via the Interrupt Controller Unit (ICU).
>> +
>> +maintainers:
>> +  - Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
>> +
>> +properties:
>> +  compatible:
>> +    const: thead,th1520-mbox
>> +
>> +  reg:
>> +    items:
>> +      - description: Mailbox local base address
>> +      - description: Remote ICU 0 base address
>> +      - description: Remote ICU 1 base address
>> +      - description: Remote ICU 2 base address
> 
>>From the SoC documentation, it looks like these are four different instances of
> the same IP block, each containing 4 unidirectional mailbox channels and an
> interrupt output. So these should be four separate nodes, no? The C910 would
> receive on channels 1-3 of instance 0, and send on channel 0 of instances 1-3.
> 

Hi, and thank you for your review.

I wanted to take some time to familiarize myself with the device tree
documentation and review best practices for mailbox drivers before
responding.

Upon reviewing the Linux device tree documentation, I couldn’t find any
specific rule that mandates having one device node per IP block in the
hardware. The T-Head manual is more focused on the hardware from a
programmer’s perspective rather than describing how exaclty the ICU's
are implemented in the HW. The device tree documentation provides
guidelines for how hardware blocks should be represented, but it doesn't
impose a strict requirement for separate device nodes per hardware
block, especially when it comes to devices like mailbox controllers.

Technically, your suggestion to create separate nodes for each ICU
instance is possible. However, doing so would require additional
complexity in the driver, as it would need to handle each node
individually. For instance, the driver would need to request interrupts
only in the case of mailbox910_t_0, while handling other device nodes
like mailbox910_t_1, mailbox910_t_2, and mailbox910_t_3 separately.

In my opinion, this approach would add unnecessary complexity to the
driver code. The current approach — using a single device node for the
mailbox controller and utilizing channels to steer messages seems to
align better with existing best practices for mailbox drivers. Many
mailbox drivers create a single mailbox controller and leverage multiple
channels for different communication paths, which simplifies both the
device tree and the driver implementation.

I hope this explanation clarifies my perspective, and I’d like to
propose keeping the current design as it stands.

>> +
>> +  reg-names:
>> +    items:
>> +      - const: local
>> +      - const: remote-icu0
>> +      - const: remote-icu1
>> +      - const: remote-icu2
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  '#mbox-cells':
>> +    const: 2
>> +    description: |
>> +      Specifies the number of cells needed to encode the mailbox specifier.
>> +      The mailbox specifier consists of two cells:
>> +        - Destination CPU ID.
>> +        - Type, which can be one of the following:
>> +            - 0:
>> +                - TX & RX channels share the same channel.
>> +                - Equipped with 7 info registers to facilitate data sharing.
>> +                - Supports IRQ for signaling.
>> +            - 1:
>> +                - TX & RX operate as doorbell channels.
>> +                - Does not have dedicated info registers.
>> +                - Lacks ACK support.
> 
> It appears that these types are not describing hardware, but the protocol used
> by the Linux driver to glue two unidirectional hardware channels together to
> make a virtual bidirectional channel. This is really the responsibility of the
> mailbox client to know what protocol it needs, not the devicetree.
> 

The protocols in question are actually handled by the firmware running
on the other cores, like the E902. I couldn’t find the firmware source
code, as it’s distributed as binary blobs. For example, the firmware
binary [1] gets loaded by U-Boot at specific addresses.

For the current case — communicating with the E902 core — the Type ‘0’
protocol is all we need. So, I don’t see any issue in removing the
second cell, since we’re only using one protocol here. 

[1] -
https://git.beagleboard.org/beaglev-ahead/xuantie-ubuntu/-/blob/48bc51286483257f153ba58813bb601267d0f087/bins/light_aon_fpga.bin

Thanks,
Michał

> Regards,
> Samuel
> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - interrupts
>> +  - '#mbox-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +
>> +    soc {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +      mailbox@ffffc38000 {
>> +        compatible = "thead,th1520-mbox";
>> +        reg = <0xff 0xffc38000 0x0 0x4000>,
>> +              <0xff 0xffc44000 0x0 0x1000>,
>> +              <0xff 0xffc4c000 0x0 0x1000>,
>> +              <0xff 0xffc54000 0x0 0x1000>;
>> +        reg-names = "local", "remote-icu0", "remote-icu1", "remote-icu2";
>> +        interrupts = <28>;
>> +        #mbox-cells = <2>;
>> +      };
>> +    };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0655c6ba5435..7401c7cb6533 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19951,6 +19951,7 @@ L:	linux-riscv@xxxxxxxxxxxxxxxxxxx
>>  S:	Maintained
>>  T:	git https://protect2.fireeye.com/v1/url?k=9b23b6b0-c4b878d4-9b223dff-000babda0201-68366f7c65442b8f&q=1&e=7028c1ef-1c3d-4e17-b7ed-76d362b80caf&u=https%3A%2F%2Fgithub.com%2Fpdp7%2Flinux.git
>>  F:	Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
>> +F:	Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
>>  F:	arch/riscv/boot/dts/thead/
>>  F:	drivers/clk/thead/clk-th1520-ap.c
>>  F:	drivers/mailbox/mailbox-th1520.c
> 
>