1. Patchew
  2. linux
  3. [PATCH v4 0/3] Introduce support for T-head TH1520 Mailbox
  4. View patch

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

Michal Wilczynski posted 3 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH v4 3/3] riscv: dts: thead: Add mailbox node
Posted by Michal Wilczynski 1 month, 1 week ago 
Add mailbox device tree node. This work is based on the vendor kernel [1].

Link: https://github.com/revyos/thead-kernel.git [1]

Signed-off-by: Michal Wilczynski <m.wilczynski@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@0 {
 				interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
 			};
 		};
+
+		mbox_910t: mailbox@ffffc38000 {
+		       compatible = "thead,th1520-mbox";
+		       reg = <0xff 0xffc38000 0x0 0x4000>,
+			     <0xff 0xffc44000 0x0 0x1000>,
+			     <0xff 0xffc4c000 0x0 0x1000>,
+			     <0xff 0xffc54000 0x0 0x1000>;
+		       reg-names = "local", "remote-icu0", "remote-icu1", "remote-icu2";
+		       interrupt-parent = <&plic>;
+		       interrupts = <28 IRQ_TYPE_LEVEL_HIGH>;
+		       #mbox-cells = <2>;
+		};
 	};
 };
-- 
2.34.1
Re: [PATCH v4 3/3] riscv: dts: thead: Add mailbox node
Posted by Emil Renner Berthing 1 month, 1 week ago 
Michal Wilczynski wrote:
> Add mailbox device tree node. This work is based on the vendor kernel [1].
>
> Link: https://github.com/revyos/thead-kernel.git [1]
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@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@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.

> +		       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://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.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?

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

/Emil

> +		       #mbox-cells = <2>;
> +		};
>  	};
>  };
> --
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH v4 3/3] riscv: dts: thead: Add mailbox node
Posted by Michal Wilczynski 1 month, 1 week ago 

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@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@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@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
>
Re: [PATCH v4 3/3] riscv: dts: thead: Add mailbox node
Posted by Emil Renner Berthing 1 month, 1 week ago 
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@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@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.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.