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

[Emil Renner Berthing]

Michal Wilczynski wrote:
>
>
> 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.

Thanks!

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

I don't doubt these mappings work, but usually the device tree should describe
the hardware and not the driver. So unless you think the documentation is
wrong, I'd still suggest you do the bindings and device tree so they describe
the hardware, and then let the driver handle the irregular register offsets.

/Emil