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