[PATCH v4 3/3] riscv: dts: thead: Add mailbox node
Michal Wilczynski
m.wilczynski at samsung.com
Tue Oct 15 11:44:00 PDT 2024
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 at samsung.com>
>> ---
>> 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 at 0 {
>> interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
>> };
>> };
>> +
>> + mbox_910t: mailbox at 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 at lists.infradead.org
>> 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
>
More information about the linux-riscv
mailing list