On 10/14/24 16:57, Emil Renner Berthing wrote: > Michal Wilczynski wrote: >> Add mailbox device tree node. This work is based on the vendor kernel [1]. >> >> Link: https://protect2.fireeye.com/v1/url?k=0bc95f25-545267d8-0bc8d46a-000babff317b-85a52eab21db9d22&q=1&e=63a49acd-e343-43d2-a57d-b4f6fcd23b61&u=https%3A%2F%2Fgithub.com%2Frevyos%2Fthead-kernel.git [1] >> >> Signed-off-by: Michal Wilczynski <m.wilczynski@xxxxxxxxxxx> >> --- >> arch/riscv/boot/dts/thead/th1520.dtsi | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi >> index 6992060e6a54..435f0ab0174d 100644 >> --- a/arch/riscv/boot/dts/thead/th1520.dtsi >> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi >> @@ -555,5 +555,17 @@ portf: gpio-controller@0 { >> interrupts = <55 IRQ_TYPE_LEVEL_HIGH>; >> }; >> }; >> + >> + mbox_910t: mailbox@ffffc38000 { > > Hi Michal, > > Thanks for your patch! Please sort this by address similar to the other nodes. Thank you for your review. Will do. > >> + compatible = "thead,th1520-mbox"; >> + reg = <0xff 0xffc38000 0x0 0x4000>, > > The documentation[1] calls this area MBOX0_T, but it says it's 24kB long. > > [1]: https://protect2.fireeye.com/v1/url?k=182b68d6-47b0502b-182ae399-000babff317b-d2b05f97b85a09ff&q=1&e=63a49acd-e343-43d2-a57d-b4f6fcd23b61&u=https%3A%2F%2Fgit.beagleboard.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.pdf > >> + <0xff 0xffc44000 0x0 0x1000>, > > According to the documentation this is inside the 24kB MBOX1_T area. > >> + <0xff 0xffc4c000 0x0 0x1000>, > > This is callod MBOX2_T, but is 8kB long. > >> + <0xff 0xffc54000 0x0 0x1000>; > > This is callod MBOX3_T, but is 8kB long. > >> + reg-names = "local", "remote-icu0", "remote-icu1", "remote-icu2"; > > Maybe these should match the MBOXn_T names in the documentation? Indeed, those are excellent points. I wondered about this today, trying to understand why the mapping was done this way. For the MBOX0_T mapping, the mailbox driver needs to map the M0_* registers, including the M0_Cn registers, where other cores write their messages. This setup requires a total of 16KB, with an additional 8KB that remains unused. Regarding MBOX1_T, MBOX2_T, and MBOX3_T, only one set of registers is necessary - specifically, Mn_C0 since the kernel always sends messages from the 910t core with CPU_IDX=0. The MBOX1_T mapping is particularly confusing, as the relevant registers, M1_C0*, start with an offset of 0x4000 relative to the beginning of the mapping. For MBOX2_T and MBOX3_T, the necessary register sets, M2_C0 and M3_C0, each occupy 4KB of address space, leaving extra 4kB unused. I assume the hardware designers found these mappings more straightforward to implement this way. I’m fairly confident that these numbers are accurate, as I have tested them and confirmed they work. > >> + interrupt-parent = <&plic>; >> + interrupts = <28 IRQ_TYPE_LEVEL_HIGH>; > > You should probably also add the clocks here: > > clocks = <&clk CLK_MBOX0>, .., <&clk CLK_MBOX3>; > > ..and claim them in the driver. Otherwise the clock framework will consider > them unused and turn them off without the clk_ignore_unesed kernel command > line. Thanks for the suggestion! I’ll re-test with the clocks added. I had clk_ignore_unused enabled permanently in my U-Boot configuration, so I might have overlooked this detail. > > /Emil > >> + #mbox-cells = <2>; >> + }; >> }; >> }; >> -- >> 2.34.1 >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@xxxxxxxxxxxxxxxxxxx >> https://protect2.fireeye.com/v1/url?k=cb1a1bba-94812347-cb1b90f5-000babff317b-3fe071fc8037390e&q=1&e=63a49acd-e343-43d2-a57d-b4f6fcd23b61&u=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-riscv >
