[RFC PATCH 0/4] riscv: dts: thead: th1520: Add mailbox for SBI-to-AON comm

[RFC PATCH 0/4] riscv: dts: thead: th1520: Add mailbox for SBI-to-AON comm

[Icenowy Zheng]

In order to make system suspend / CPU hotplugging to work, the BSP
OpenSBI wants to talk to the AON firmware, and will parse the
information needed for this communication from the device tree.

This patchset tries to retrofit the thead,th1520-mbox binding for the
C910R mailbox, and assign it for SBI-to-AON communication.

The binding seems to be badly designed, and the retrofitting process
looks quite dirty. Should we just abandon the current compatible and
make a new one with a more proper binding?

Icenowy Zheng (4):
  dt-bindings: mailbox: thead,th1520-mbox: retrofit for other mailboxes
  dt-bindings: firmware: thead,th1520-aon: add a mailbox name for SBI
  riscv: dts: thead: th1520: add reserved node for C910R mailbox
  riscv: dts: thead: th1520: add mailbox channel for SBI-to-AON comm

 .../bindings/firmware/thead,th1520-aon.yaml   |  7 +--
 .../bindings/mailbox/thead,th1520-mbox.yaml   | 49 ++++++++++++++-----
 arch/riscv/boot/dts/thead/th1520.dtsi         | 20 +++++++-
 3 files changed, 58 insertions(+), 18 deletions(-)

-- 
2.50.1

[RFC PATCH 1/4] dt-bindings: mailbox: thead,th1520-mbox: retrofit for other mailboxes

[Icenowy Zheng]

The current binding of thead,th1520-mbox can only apply to the C910T
mailbox (which has an ID of 0).

Because of the weird mailbox register mapping practice for world
seperation on TH1520, the binding needs some reword, in addition to add
a property for mailbox ID, to describe other mailboxes.

Update the binding, in order to make it suitable to describe other
mailboxes. The example is also updated, with an addition of mbox_c910t
label to show that the example describes this specfiic mailbox, mailbox
ID added and the register window sizes updated to the values from the
manual (previously the remote-icu0 register windows is declared to be
overly small that it would never work).

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 .../bindings/mailbox/thead,th1520-mbox.yaml   | 49 ++++++++++++++-----
 1 file changed, 36 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
index 0971fb97896ef..5a24d2e8a6a8c 100644
--- a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
+++ b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
@@ -12,6 +12,17 @@ description:
   through mailbox channels. It also allows one core to signal another processor
   using interrupts via the Interrupt Controller Unit (ICU).
 
+  The SoC is divided to two worlds, REE and TEE, although it's currently unknown
+  how to enable the seperation between worlds so the seperation does not exist
+  yet. However each mailbox is assigned to a certain world, and register windows
+  for mailboxes are assigned to different worlds too. In a certain world's
+  register windows for mailboxes, only mailboxes assigned to this world will
+  have the local ICU part mapped (in addition to the remote ICU part of the
+  other same-world mailbox), and mailboxes assigned to the other world have
+  only the coressponding remote ICU part mapped to this world. Two mailboxes
+  (C910T and E902) are assigned to the TEE world and two mailboxes (C906 and
+  C910R) are assigned to the REE world.
+
 maintainers:
   - Michal Wilczynski <m.wilczynski@samsung.com>
 
@@ -22,9 +33,9 @@ properties:
   clocks:
     items:
       - description: Clock for the local mailbox
-      - description: Clock for remote ICU 0
-      - description: Clock for remote ICU 1
-      - description: Clock for remote ICU 2
+      - description: Clock for the other mailbox in the same world
+      - description: Clock for the first mailbox in the other world
+      - description: Clock for the second mailbox in the other world
 
   clock-names:
     items:
@@ -35,10 +46,14 @@ properties:
 
   reg:
     items:
-      - description: Mailbox local base address
-      - description: Remote ICU 0 base address
-      - description: Remote ICU 1 base address
-      - description: Remote ICU 2 base address
+      - description: Base address of this specific mailbox
+      - description: Base address of the other mailbox in the same world
+      - description:
+          Base address of the register window in this world corresponding to the
+          first other-world mailbox.
+      - description:
+          Base address of the register window in this world corresponding to the
+          second other-world mailbox.
 
   reg-names:
     items:
@@ -50,10 +65,17 @@ properties:
   interrupts:
     maxItems: 1
 
+  thead,mbox-id:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      The ID of this specific mailbox that this device tree node describes. For
+      compatibility with old device trees, if missing, the ID is default to 0,
+      the C910T mailbox.
+
   '#mbox-cells':
     const: 1
     description:
-      The one and only cell describes destination CPU ID.
+      The one and only cell describes destination mailbox ID.
 
 required:
   - compatible
@@ -72,12 +94,12 @@ examples:
     soc {
       #address-cells = <2>;
       #size-cells = <2>;
-      mailbox@ffffc38000 {
+      mbox_c910t: mailbox@ffffc38000 {
         compatible = "thead,th1520-mbox";
-        reg = <0xff 0xffc38000 0x0 0x4000>,
-              <0xff 0xffc44000 0x0 0x1000>,
-              <0xff 0xffc4c000 0x0 0x1000>,
-              <0xff 0xffc54000 0x0 0x1000>;
+        reg = <0xff 0xffc38000 0x0 0x6000>,
+              <0xff 0xffc44000 0x0 0x6000>,
+              <0xff 0xffc4c000 0x0 0x2000>,
+              <0xff 0xffc54000 0x0 0x2000>;
         reg-names = "local", "remote-icu0", "remote-icu1", "remote-icu2";
         clocks = <&clk CLK_MBOX0>, <&clk CLK_MBOX1>, <&clk CLK_MBOX2>,
                  <&clk CLK_MBOX3>;
@@ -85,5 +107,6 @@ examples:
                       "clk-remote-icu2";
         interrupts = <28>;
         #mbox-cells = <1>;
+        thead,mbox-id = <0>;
       };
     };
-- 
2.50.1

[RFC PATCH 2/4] dt-bindings: firmware: thead,th1520-aon: add a mailbox name for SBI

[Icenowy Zheng]

The SBI firmware might want to communicate to the AON firmware too.

Add a mbox-name item to allow to allocate a mailbox for SBI.

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 .../devicetree/bindings/firmware/thead,th1520-aon.yaml     | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
index 3365124c7fd47..555465f4aab4e 100644
--- a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
+++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
@@ -26,11 +26,12 @@ properties:
     const: thead,th1520-aon
 
   mboxes:
-    maxItems: 1
+    maxItems: 2
 
   mbox-names:
     items:
       - const: aon
+      - const: aon-for-sbi
 
   resets:
     maxItems: 1
@@ -54,7 +55,7 @@ examples:
   - |
     aon: aon {
         compatible = "thead,th1520-aon";
-        mboxes = <&mbox_910t 1>;
-        mbox-names = "aon";
+        mboxes = <&mbox_910t 1>, <&mbox_910r 1>;
+        mbox-names = "aon", "aon-for-sbi";
         #power-domain-cells = <1>;
     };
-- 
2.50.1

[RFC PATCH 3/4] riscv: dts: thead: th1520: add reserved node for C910R mailbox

[Icenowy Zheng]

The OpenSBI firmware might want to communicate to E902 CPU too. As we
have two mailboxes assigned to the C910 CPU, declare the other C910
mailbox as a "reserved" device for OpenSBI to use.

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 arch/riscv/boot/dts/thead/th1520.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 42724bf7e90e0..136ebe210b876 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -565,6 +565,22 @@ timer3: timer@ffefc3203c {
 			status = "disabled";
 		};
 
+		mbox_910r: mbox@ffefc53000 {
+			compatible = "thead,th1520-mbox";
+			reg = <0xff 0xefc50000 0x0 0x6000>,
+			      <0xff 0xefc48000 0x0 0x6000>,
+			      <0xff 0xefc3e000 0x0 0x2000>,
+			      <0xff 0xefc46000 0x0 0x2000>;
+			reg-names = "local", "remote-icu0", "remote-icu1", "remote-icu2";
+			clocks = <&clk CLK_MBOX3>, <&clk CLK_MBOX2>, <&clk CLK_MBOX0>,
+				 <&clk CLK_MBOX1>;
+			clock-names = "clk-local", "clk-remote-icu0", "clk-remote-icu1",
+				      "clk-remote-icu2";
+			thead,mbox-id = <3>;
+			#mbox-cells = <1>;
+			status = "reserved";
+		};
+
 		uart4: serial@fff7f08000 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0xff 0xf7f08000 0x0 0x4000>;
-- 
2.50.1

[RFC PATCH 4/4] riscv: dts: thead: th1520: add mailbox channel for SBI-to-AON comm

[Icenowy Zheng]

Add a mailbox channel declaration to the AON firmware node to describe
the mailbox channel assigned to the SBI firmware to communicate with
AON.

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 arch/riscv/boot/dts/thead/th1520.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 136ebe210b876..6e5e0223d0c16 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -233,8 +233,8 @@ stmmac_axi_config: stmmac-axi-config {
 
 	aon: aon {
 		compatible = "thead,th1520-aon";
-		mboxes = <&mbox_910t 1>;
-		mbox-names = "aon";
+		mboxes = <&mbox_910t 1>, <&mbox_910r 1>;
+		mbox-names = "aon", "aon-for-sbi";
 		resets = <&rst TH1520_RESET_ID_GPU_CLKGEN>;
 		reset-names = "gpu-clkgen";
 		#power-domain-cells = <1>;
-- 
2.50.1

Re: [RFC PATCH 1/4] dt-bindings: mailbox: thead,th1520-mbox: retrofit for other mailboxes

[Krzysztof Kozlowski]

On 14/08/2025 09:07, Icenowy Zheng wrote:
> The current binding of thead,th1520-mbox can only apply to the C910T
> mailbox (which has an ID of 0).
> 
> Because of the weird mailbox register mapping practice for world
> seperation on TH1520, the binding needs some reword, in addition to add
> a property for mailbox ID, to describe other mailboxes.
> 
> Update the binding, in order to make it suitable to describe other

But I do not see any new device being added.

> mailboxes. The example is also updated, with an addition of mbox_c910t
> label to show that the example describes this specfiic mailbox, mailbox
> ID added and the register window sizes updated to the values from the
> manual (previously the remote-icu0 register windows is declared to be
> overly small that it would never work).
> 
> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> ---
>  .../bindings/mailbox/thead,th1520-mbox.yaml   | 49 ++++++++++++++-----
>  1 file changed, 36 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
> index 0971fb97896ef..5a24d2e8a6a8c 100644
> --- a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
> +++ b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
> @@ -12,6 +12,17 @@ description:
>    through mailbox channels. It also allows one core to signal another processor
>    using interrupts via the Interrupt Controller Unit (ICU).
>  
> +  The SoC is divided to two worlds, REE and TEE, although it's currently unknown
> +  how to enable the seperation between worlds so the seperation does not exist
> +  yet. However each mailbox is assigned to a certain world, and register windows
> +  for mailboxes are assigned to different worlds too. In a certain world's
> +  register windows for mailboxes, only mailboxes assigned to this world will
> +  have the local ICU part mapped (in addition to the remote ICU part of the
> +  other same-world mailbox), and mailboxes assigned to the other world have
> +  only the coressponding remote ICU part mapped to this world. Two mailboxes
> +  (C910T and E902) are assigned to the TEE world and two mailboxes (C906 and
> +  C910R) are assigned to the REE world.
> +
>  maintainers:
>    - Michal Wilczynski <m.wilczynski@samsung.com>
>  
> @@ -22,9 +33,9 @@ properties:
>    clocks:
>      items:
>        - description: Clock for the local mailbox
> -      - description: Clock for remote ICU 0
> -      - description: Clock for remote ICU 1
> -      - description: Clock for remote ICU 2
> +      - description: Clock for the other mailbox in the same world
> +      - description: Clock for the first mailbox in the other world
> +      - description: Clock for the second mailbox in the other world
>  
>    clock-names:
>      items:
> @@ -35,10 +46,14 @@ properties:
>  
>    reg:
>      items:
> -      - description: Mailbox local base address
> -      - description: Remote ICU 0 base address
> -      - description: Remote ICU 1 base address
> -      - description: Remote ICU 2 base address
> +      - description: Base address of this specific mailbox
> +      - description: Base address of the other mailbox in the same world
> +      - description:
> +          Base address of the register window in this world corresponding to the
> +          first other-world mailbox.
> +      - description:
> +          Base address of the register window in this world corresponding to the
> +          second other-world mailbox.

This feels like ABI change.

>  
>    reg-names:
>      items:
> @@ -50,10 +65,17 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  thead,mbox-id:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      The ID of this specific mailbox that this device tree node describes. For
> +      compatibility with old device trees, if missing, the ID is default to 0,
> +      the C910T mailbox.

No, you cannot have instance IDs. It's even explicitly documented in
writing bindings.

> +
>    '#mbox-cells':
>      const: 1
>      description:
> -      The one and only cell describes destination CPU ID.
> +      The one and only cell describes destination mailbox ID.
>  
>  required:
>    - compatible
> @@ -72,12 +94,12 @@ examples:
>      soc {
>        #address-cells = <2>;
>        #size-cells = <2>;
> -      mailbox@ffffc38000 {
> +      mbox_c910t: mailbox@ffffc38000 {

No, don't add unused labels. This is not improving the binding.


Best regards,
Krzysztof

Re: [RFC PATCH 2/4] dt-bindings: firmware: thead,th1520-aon: add a mailbox name for SBI

[Krzysztof Kozlowski]

On 14/08/2025 09:07, Icenowy Zheng wrote:
> The SBI firmware might want to communicate to the AON firmware too.
> 
> Add a mbox-name item to allow to allocate a mailbox for SBI.
> 
> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> ---
>  .../devicetree/bindings/firmware/thead,th1520-aon.yaml     | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> index 3365124c7fd47..555465f4aab4e 100644
> --- a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> +++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> @@ -26,11 +26,12 @@ properties:
>      const: thead,th1520-aon
>  
>    mboxes:
> -    maxItems: 1
> +    maxItems: 2


ABI break without explanation why ("allow" is not a reason to affect
ABI) and its impact.


Best regards,
Krzysztof

Re: [RFC PATCH 2/4] dt-bindings: firmware: thead,th1520-aon: add a mailbox name for SBI

[Icenowy Zheng]

在 2025-08-14星期四的 09:18 +0200，Krzysztof Kozlowski写道：
> On 14/08/2025 09:07, Icenowy Zheng wrote:
> > The SBI firmware might want to communicate to the AON firmware too.
> > 
> > Add a mbox-name item to allow to allocate a mailbox for SBI.
> > 
> > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > ---
> >  .../devicetree/bindings/firmware/thead,th1520-aon.yaml     | 7
> > ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> > b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> > index 3365124c7fd47..555465f4aab4e 100644
> > --- a/Documentation/devicetree/bindings/firmware/thead,th1520-
> > aon.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/thead,th1520-
> > aon.yaml
> > @@ -26,11 +26,12 @@ properties:
> >      const: thead,th1520-aon
> >  
> >    mboxes:
> > -    maxItems: 1
> > +    maxItems: 2
> 
> 
> ABI break without explanation why ("allow" is not a reason to affect
> ABI) and its impact.

Is adding items an ABI break?

Or should I explicitly say "minItems: 1" here?

> 
> 
> Best regards,
> Krzysztof

Re: [RFC PATCH 1/4] dt-bindings: mailbox: thead,th1520-mbox: retrofit for other mailboxes

[Icenowy Zheng]

在 2025-08-14星期四的 09:18 +0200，Krzysztof Kozlowski写道：
> On 14/08/2025 09:07, Icenowy Zheng wrote:
> > The current binding of thead,th1520-mbox can only apply to the
> > C910T
> > mailbox (which has an ID of 0).
> > 
> > Because of the weird mailbox register mapping practice for world
> > seperation on TH1520, the binding needs some reword, in addition to
> > add
> > a property for mailbox ID, to describe other mailboxes.
> > 
> > Update the binding, in order to make it suitable to describe other
> 
> But I do not see any new device being added.

See PATCH 3/4 in this patchset.

Or do you mean I need to add new compatibles as I said below?

> 
> > mailboxes. The example is also updated, with an addition of
> > mbox_c910t
> > label to show that the example describes this specfiic mailbox,
> > mailbox
> > ID added and the register window sizes updated to the values from
> > the
> > manual (previously the remote-icu0 register windows is declared to
> > be
> > overly small that it would never work).
> > 
> > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > ---
> >  .../bindings/mailbox/thead,th1520-mbox.yaml   | 49 ++++++++++++++-
> > ----
> >  1 file changed, 36 insertions(+), 13 deletions(-)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
> > b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
> > index 0971fb97896ef..5a24d2e8a6a8c 100644
> > --- a/Documentation/devicetree/bindings/mailbox/thead,th1520-
> > mbox.yaml
> > +++ b/Documentation/devicetree/bindings/mailbox/thead,th1520-
> > mbox.yaml
> > @@ -12,6 +12,17 @@ description:
> >    through mailbox channels. It also allows one core to signal
> > another processor
> >    using interrupts via the Interrupt Controller Unit (ICU).
> >  
> > +  The SoC is divided to two worlds, REE and TEE, although it's
> > currently unknown
> > +  how to enable the seperation between worlds so the seperation
> > does not exist
> > +  yet. However each mailbox is assigned to a certain world, and
> > register windows
> > +  for mailboxes are assigned to different worlds too. In a certain
> > world's
> > +  register windows for mailboxes, only mailboxes assigned to this
> > world will
> > +  have the local ICU part mapped (in addition to the remote ICU
> > part of the
> > +  other same-world mailbox), and mailboxes assigned to the other
> > world have
> > +  only the coressponding remote ICU part mapped to this world. Two
> > mailboxes
> > +  (C910T and E902) are assigned to the TEE world and two mailboxes
> > (C906 and
> > +  C910R) are assigned to the REE world.
> > +
> >  maintainers:
> >    - Michal Wilczynski <m.wilczynski@samsung.com>
> >  
> > @@ -22,9 +33,9 @@ properties:
> >    clocks:
> >      items:
> >        - description: Clock for the local mailbox
> > -      - description: Clock for remote ICU 0
> > -      - description: Clock for remote ICU 1
> > -      - description: Clock for remote ICU 2
> > +      - description: Clock for the other mailbox in the same world
> > +      - description: Clock for the first mailbox in the other
> > world
> > +      - description: Clock for the second mailbox in the other
> > world
> >  
> >    clock-names:
> >      items:
> > @@ -35,10 +46,14 @@ properties:
> >  
> >    reg:
> >      items:
> > -      - description: Mailbox local base address
> > -      - description: Remote ICU 0 base address
> > -      - description: Remote ICU 1 base address
> > -      - description: Remote ICU 2 base address
> > +      - description: Base address of this specific mailbox
> > +      - description: Base address of the other mailbox in the same
> > world
> > +      - description:
> > +          Base address of the register window in this world
> > corresponding to the
> > +          first other-world mailbox.
> > +      - description:
> > +          Base address of the register window in this world
> > corresponding to the
> > +          second other-world mailbox.
> 
> This feels like ABI change.
> 
> >  
> >    reg-names:
> >      items:
> > @@ -50,10 +65,17 @@ properties:
> >    interrupts:
> >      maxItems: 1
> >  
> > +  thead,mbox-id:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      The ID of this specific mailbox that this device tree node
> > describes. For
> > +      compatibility with old device trees, if missing, the ID is
> > default to 0,
> > +      the C910T mailbox.
> 
> No, you cannot have instance IDs. It's even explicitly documented in
> writing bindings.

The problem is that the mailbox cannot send to itself, so the "sending
to self" slot is redefined to other meaning.

Or should I assign different compatible strings to all 4 mailboxes
because they have different registers in the "sending to self" slot,
which have different offset because of instance ID?

> 
> > +
> >    '#mbox-cells':
> >      const: 1
> >      description:
> > -      The one and only cell describes destination CPU ID.
> > +      The one and only cell describes destination mailbox ID.
> >  
> >  required:
> >    - compatible
> > @@ -72,12 +94,12 @@ examples:
> >      soc {
> >        #address-cells = <2>;
> >        #size-cells = <2>;
> > -      mailbox@ffffc38000 {
> > +      mbox_c910t: mailbox@ffffc38000 {
> 
> No, don't add unused labels. This is not improving the binding.

Sorry.

> 
> 
> Best regards,
> Krzysztof

Re: [RFC PATCH 1/4] dt-bindings: mailbox: thead,th1520-mbox: retrofit for other mailboxes

[Krzysztof Kozlowski]

On 14/08/2025 09:34, Icenowy Zheng wrote:
> 在 2025-08-14星期四的 09:18 +0200，Krzysztof Kozlowski写道：
>> On 14/08/2025 09:07, Icenowy Zheng wrote:
>>> The current binding of thead,th1520-mbox can only apply to the
>>> C910T
>>> mailbox (which has an ID of 0).
>>>
>>> Because of the weird mailbox register mapping practice for world
>>> seperation on TH1520, the binding needs some reword, in addition to
>>> add
>>> a property for mailbox ID, to describe other mailboxes.
>>>
>>> Update the binding, in order to make it suitable to describe other
>>
>> But I do not see any new device being added.
> 
> See PATCH 3/4 in this patchset.
> 
> Or do you mean I need to add new compatibles as I said below?

I don't know... you say there are some other mailboxes (I understand as
mailbox controllers) but I don't see anything being added here.


> 
>>
>>> mailboxes. The example is also updated, with an addition of
>>> mbox_c910t
>>> label to show that the example describes this specfiic mailbox,
>>> mailbox
>>> ID added and the register window sizes updated to the values from
>>> the
>>> manual (previously the remote-icu0 register windows is declared to
>>> be
>>> overly small that it would never work).
>>>
>>> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
>>> ---
>>>  .../bindings/mailbox/thead,th1520-mbox.yaml   | 49 ++++++++++++++-
>>> ----
>>>  1 file changed, 36 insertions(+), 13 deletions(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
>>> b/Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml
>>> index 0971fb97896ef..5a24d2e8a6a8c 100644
>>> --- a/Documentation/devicetree/bindings/mailbox/thead,th1520-
>>> mbox.yaml
>>> +++ b/Documentation/devicetree/bindings/mailbox/thead,th1520-
>>> mbox.yaml
>>> @@ -12,6 +12,17 @@ description:
>>>    through mailbox channels. It also allows one core to signal
>>> another processor
>>>    using interrupts via the Interrupt Controller Unit (ICU).
>>>  
>>> +  The SoC is divided to two worlds, REE and TEE, although it's
>>> currently unknown
>>> +  how to enable the seperation between worlds so the seperation
>>> does not exist
>>> +  yet. However each mailbox is assigned to a certain world, and
>>> register windows
>>> +  for mailboxes are assigned to different worlds too. In a certain
>>> world's
>>> +  register windows for mailboxes, only mailboxes assigned to this
>>> world will
>>> +  have the local ICU part mapped (in addition to the remote ICU
>>> part of the
>>> +  other same-world mailbox), and mailboxes assigned to the other
>>> world have
>>> +  only the coressponding remote ICU part mapped to this world. Two
>>> mailboxes
>>> +  (C910T and E902) are assigned to the TEE world and two mailboxes
>>> (C906 and
>>> +  C910R) are assigned to the REE world.
>>> +
>>>  maintainers:
>>>    - Michal Wilczynski <m.wilczynski@samsung.com>
>>>  
>>> @@ -22,9 +33,9 @@ properties:
>>>    clocks:
>>>      items:
>>>        - description: Clock for the local mailbox
>>> -      - description: Clock for remote ICU 0
>>> -      - description: Clock for remote ICU 1
>>> -      - description: Clock for remote ICU 2
>>> +      - description: Clock for the other mailbox in the same world
>>> +      - description: Clock for the first mailbox in the other
>>> world
>>> +      - description: Clock for the second mailbox in the other
>>> world
>>>  
>>>    clock-names:
>>>      items:
>>> @@ -35,10 +46,14 @@ properties:
>>>  
>>>    reg:
>>>      items:
>>> -      - description: Mailbox local base address
>>> -      - description: Remote ICU 0 base address
>>> -      - description: Remote ICU 1 base address
>>> -      - description: Remote ICU 2 base address
>>> +      - description: Base address of this specific mailbox
>>> +      - description: Base address of the other mailbox in the same
>>> world
>>> +      - description:
>>> +          Base address of the register window in this world
>>> corresponding to the
>>> +          first other-world mailbox.
>>> +      - description:
>>> +          Base address of the register window in this world
>>> corresponding to the
>>> +          second other-world mailbox.
>>
>> This feels like ABI change.
>>
>>>  
>>>    reg-names:
>>>      items:
>>> @@ -50,10 +65,17 @@ properties:
>>>    interrupts:
>>>      maxItems: 1
>>>  
>>> +  thead,mbox-id:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      The ID of this specific mailbox that this device tree node
>>> describes. For
>>> +      compatibility with old device trees, if missing, the ID is
>>> default to 0,
>>> +      the C910T mailbox.
>>
>> No, you cannot have instance IDs. It's even explicitly documented in
>> writing bindings.
> 
> The problem is that the mailbox cannot send to itself, so the "sending
> to self" slot is redefined to other meaning.

I don't know what is sending to self...

> 
> Or should I assign different compatible strings to all 4 mailboxes
> because they have different registers in the "sending to self" slot,
> which have different offset because of instance ID?

Commit msg is quite vague in describing hardware. It has a lot of text
but no answers to questions. What are the differences? What is the hardware?

if you have different programming model, then you have different
compatibles, usually.


Best regards,
Krzysztof

Re: [RFC PATCH 2/4] dt-bindings: firmware: thead,th1520-aon: add a mailbox name for SBI

[Krzysztof Kozlowski]

On 14/08/2025 09:30, Icenowy Zheng wrote:
> 在 2025-08-14星期四的 09:18 +0200，Krzysztof Kozlowski写道：
>> On 14/08/2025 09:07, Icenowy Zheng wrote:
>>> The SBI firmware might want to communicate to the AON firmware too.
>>>
>>> Add a mbox-name item to allow to allocate a mailbox for SBI.
>>>
>>> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
>>> ---
>>>  .../devicetree/bindings/firmware/thead,th1520-aon.yaml     | 7
>>> ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>>> b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>>> index 3365124c7fd47..555465f4aab4e 100644
>>> --- a/Documentation/devicetree/bindings/firmware/thead,th1520-
>>> aon.yaml
>>> +++ b/Documentation/devicetree/bindings/firmware/thead,th1520-
>>> aon.yaml
>>> @@ -26,11 +26,12 @@ properties:
>>>      const: thead,th1520-aon
>>>  
>>>    mboxes:
>>> -    maxItems: 1
>>> +    maxItems: 2
>>
>>
>> ABI break without explanation why ("allow" is not a reason to affect
>> ABI) and its impact.
> 
> Is adding items an ABI break?

Adding required items is ABI break. You can easily test it. Apply patch
#1 and test your DTS. Apply patch #2 and test your DTS. New warnings
appear, so that's a proof of ABI impact.


> 
> Or should I explicitly say "minItems: 1" here?

Yes, but you should clearly explain the impact. Is it working? Not
working? Are you fixing something?

Best regards,
Krzysztof

Re: [RFC PATCH 1/4] dt-bindings: mailbox: thead,th1520-mbox: retrofit for other mailboxes

[Icenowy Zheng]

在 2025-08-14星期四的 09:48 +0200，Krzysztof Kozlowski写道：
> On 14/08/2025 09:34, Icenowy Zheng wrote:
> > 在 2025-08-14星期四的 09:18 +0200，Krzysztof Kozlowski写道：
> > > On 14/08/2025 09:07, Icenowy Zheng wrote:
> > > > The current binding of thead,th1520-mbox can only apply to the
> > > > C910T
> > > > mailbox (which has an ID of 0).
> > > > 
> > > > Because of the weird mailbox register mapping practice for
> > > > world
> > > > seperation on TH1520, the binding needs some reword, in
> > > > addition to
> > > > add
> > > > a property for mailbox ID, to describe other mailboxes.
> > > > 
> > > > Update the binding, in order to make it suitable to describe
> > > > other
> > > 
> > > But I do not see any new device being added.
> > 
> > See PATCH 3/4 in this patchset.
> > 
> > Or do you mean I need to add new compatibles as I said below?
> 
> I don't know... you say there are some other mailboxes (I understand
> as
> mailbox controllers) but I don't see anything being added here.
> 
> 
> > 
> > > 
> > > > mailboxes. The example is also updated, with an addition of
> > > > mbox_c910t
> > > > label to show that the example describes this specfiic mailbox,
> > > > mailbox
> > > > ID added and the register window sizes updated to the values
> > > > from
> > > > the
> > > > manual (previously the remote-icu0 register windows is declared
> > > > to
> > > > be
> > > > overly small that it would never work).
> > > > 
> > > > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > > > ---
> > > >  .../bindings/mailbox/thead,th1520-mbox.yaml   | 49
> > > > ++++++++++++++-
> > > > ----
> > > >  1 file changed, 36 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/mailbox/thead,th1520-
> > > > mbox.yaml
> > > > b/Documentation/devicetree/bindings/mailbox/thead,th1520-
> > > > mbox.yaml
> > > > index 0971fb97896ef..5a24d2e8a6a8c 100644
> > > > --- a/Documentation/devicetree/bindings/mailbox/thead,th1520-
> > > > mbox.yaml
> > > > +++ b/Documentation/devicetree/bindings/mailbox/thead,th1520-
> > > > mbox.yaml
> > > > @@ -12,6 +12,17 @@ description:
> > > >    through mailbox channels. It also allows one core to signal
> > > > another processor
> > > >    using interrupts via the Interrupt Controller Unit (ICU).
> > > >  
> > > > +  The SoC is divided to two worlds, REE and TEE, although it's
> > > > currently unknown
> > > > +  how to enable the seperation between worlds so the
> > > > seperation
> > > > does not exist
> > > > +  yet. However each mailbox is assigned to a certain world,
> > > > and
> > > > register windows
> > > > +  for mailboxes are assigned to different worlds too. In a
> > > > certain
> > > > world's
> > > > +  register windows for mailboxes, only mailboxes assigned to
> > > > this
> > > > world will
> > > > +  have the local ICU part mapped (in addition to the remote
> > > > ICU
> > > > part of the
> > > > +  other same-world mailbox), and mailboxes assigned to the
> > > > other
> > > > world have
> > > > +  only the coressponding remote ICU part mapped to this world.
> > > > Two
> > > > mailboxes
> > > > +  (C910T and E902) are assigned to the TEE world and two
> > > > mailboxes
> > > > (C906 and
> > > > +  C910R) are assigned to the REE world.
> > > > +
> > > >  maintainers:
> > > >    - Michal Wilczynski <m.wilczynski@samsung.com>
> > > >  
> > > > @@ -22,9 +33,9 @@ properties:
> > > >    clocks:
> > > >      items:
> > > >        - description: Clock for the local mailbox
> > > > -      - description: Clock for remote ICU 0
> > > > -      - description: Clock for remote ICU 1
> > > > -      - description: Clock for remote ICU 2
> > > > +      - description: Clock for the other mailbox in the same
> > > > world
> > > > +      - description: Clock for the first mailbox in the other
> > > > world
> > > > +      - description: Clock for the second mailbox in the other
> > > > world
> > > >  
> > > >    clock-names:
> > > >      items:
> > > > @@ -35,10 +46,14 @@ properties:
> > > >  
> > > >    reg:
> > > >      items:
> > > > -      - description: Mailbox local base address
> > > > -      - description: Remote ICU 0 base address
> > > > -      - description: Remote ICU 1 base address
> > > > -      - description: Remote ICU 2 base address
> > > > +      - description: Base address of this specific mailbox
> > > > +      - description: Base address of the other mailbox in the
> > > > same
> > > > world
> > > > +      - description:
> > > > +          Base address of the register window in this world
> > > > corresponding to the
> > > > +          first other-world mailbox.
> > > > +      - description:
> > > > +          Base address of the register window in this world
> > > > corresponding to the
> > > > +          second other-world mailbox.
> > > 
> > > This feels like ABI change.
> > > 
> > > >  
> > > >    reg-names:
> > > >      items:
> > > > @@ -50,10 +65,17 @@ properties:
> > > >    interrupts:
> > > >      maxItems: 1
> > > >  
> > > > +  thead,mbox-id:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    description:
> > > > +      The ID of this specific mailbox that this device tree
> > > > node
> > > > describes. For
> > > > +      compatibility with old device trees, if missing, the ID
> > > > is
> > > > default to 0,
> > > > +      the C910T mailbox.
> > > 
> > > No, you cannot have instance IDs. It's even explicitly documented
> > > in
> > > writing bindings.
> > 
> > The problem is that the mailbox cannot send to itself, so the
> > "sending
> > to self" slot is redefined to other meaning.
> 
> I don't know what is sending to self...

The first mailbox's definition is {global_control,
receiving_from_mailbox_1, receiving_from_mailbox_2,
receiving_from_mailbox_3}, and the second one's is
{receiving_from_mailbox_0, global_control, receiving_from_mailbox_2,
receiving_from_mailbox_3}, in such a pattern.

The current compatible works for the first mailbox only because it
assumes the mailbox is the first one, thus expecting the 0x0 offset of
receiving end is the global control.

Well, by totally drop the current compatible and write a new one from
scratch, and specify all these ports in reg property (instead writing a
single local-icu register window), all these problems could be solved -
- but in this case even the mailbox consumer will be affected, because
in this case the mailbox channel cell will be correspond to register
zones instead of hardware-based mailbox IDs. (in the current binding
<&mbox 0> is forbidden because it means sending from mailbox 0 to
mailbox 0, an invalid operation; but in the new binding <&mbox 0> will
be sending to the mailbox specified with reg-name "tx-0" instead.)

> 
> > 
> > Or should I assign different compatible strings to all 4 mailboxes
> > because they have different registers in the "sending to self"
> > slot,
> > which have different offset because of instance ID?
> 
> Commit msg is quite vague in describing hardware. It has a lot of
> text
> but no answers to questions. What are the differences? What is the
> hardware?
> 
> if you have different programming model, then you have different
> compatibles, usually.
> 
> 
> Best regards,
> Krzysztof

Re: [RFC PATCH 2/4] dt-bindings: firmware: thead,th1520-aon: add a mailbox name for SBI

[Icenowy Zheng]

在 2025-08-14星期四的 09:49 +0200，Krzysztof Kozlowski写道：
> On 14/08/2025 09:30, Icenowy Zheng wrote:
> > 在 2025-08-14星期四的 09:18 +0200，Krzysztof Kozlowski写道：
> > > On 14/08/2025 09:07, Icenowy Zheng wrote:
> > > > The SBI firmware might want to communicate to the AON firmware
> > > > too.
> > > > 
> > > > Add a mbox-name item to allow to allocate a mailbox for SBI.
> > > > 
> > > > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > > > ---
> > > >  .../devicetree/bindings/firmware/thead,th1520-aon.yaml     | 7
> > > > ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/firmware/thead,th1520-
> > > > aon.yaml
> > > > b/Documentation/devicetree/bindings/firmware/thead,th1520-
> > > > aon.yaml
> > > > index 3365124c7fd47..555465f4aab4e 100644
> > > > --- a/Documentation/devicetree/bindings/firmware/thead,th1520-
> > > > aon.yaml
> > > > +++ b/Documentation/devicetree/bindings/firmware/thead,th1520-
> > > > aon.yaml
> > > > @@ -26,11 +26,12 @@ properties:
> > > >      const: thead,th1520-aon
> > > >  
> > > >    mboxes:
> > > > -    maxItems: 1
> > > > +    maxItems: 2
> > > 
> > > 
> > > ABI break without explanation why ("allow" is not a reason to
> > > affect
> > > ABI) and its impact.
> > 
> > Is adding items an ABI break?
> 
> Adding required items is ABI break. You can easily test it. Apply
> patch
> #1 and test your DTS. Apply patch #2 and test your DTS. New warnings
> appear, so that's a proof of ABI impact.

Ah sorry I don't mean that item is required.

> 
> 
> > 
> > Or should I explicitly say "minItems: 1" here?
> 
> Yes, but you should clearly explain the impact. Is it working? Not
> working? Are you fixing something?

The jsonschema draft says "Omitting this keyword has the same behavior
as a value of 0." for minItems. [1]

[1]
https://json-schema.org/draft/2020-12/json-schema-validation#section-6.4.2-3

> 
> Best regards,
> Krzysztof

Re: [RFC PATCH 2/4] dt-bindings: firmware: thead,th1520-aon: add a mailbox name for SBI

[Krzysztof Kozlowski]

On 14/08/2025 10:04, Icenowy Zheng wrote:
>>
>>>
>>> Or should I explicitly say "minItems: 1" here?
>>
>> Yes, but you should clearly explain the impact. Is it working? Not
>> working? Are you fixing something?
> 
> The jsonschema draft says "Omitting this keyword has the same behavior
> as a value of 0." for minItems. [1]

No, omitting this means it is implied by maxItems, see fixups.py:130.

Best regards,
Krzysztof