Re: [PATCH v4 3/3] riscv: dts: thead: Add mailbox node

[Michal Wilczynski]

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
>