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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJM55Z913WhGUVHR8e2GhS=x56hqrAMzGmRgLLNMFEWTguxgYA@mail.gmail.com>
Date: Thu, 17 Oct 2024 04:20:34 -0700
From: Emil Renner Berthing <emil.renner.berthing@...onical.com>
To: Michal Wilczynski <m.wilczynski@...sung.com>, 
	Emil Renner Berthing <emil.renner.berthing@...onical.com>, drew@...7.com, guoren@...nel.org, 
	wefu@...hat.com, jassisinghbrar@...il.com, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, paul.walmsley@...ive.com, 
	palmer@...belt.com, aou@...s.berkeley.edu, m.szyprowski@...sung.com
Cc: linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org
Subject: Re: [PATCH v4 3/3] riscv: dts: thead: Add mailbox node

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@...sung.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@0 {
> >>  				interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
> >>  			};
> >>  		};
> >> +
> >> +		mbox_910t: mailbox@...fc38000 {
> >
> > 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