[PATCH v5 0/3] mailbox: add Samsung Exynos driver

[PATCH v5 0/3] mailbox: add Samsung Exynos driver

[Tudor Ambarus]

The samsung exynos mailbox controller has 16 flag bits for hardware
interrupt generation and a shared register for passing mailbox messages.
When the controller is used by the ACPM protocol the shared register is
ignored and the mailbox controller acts as a doorbell. The controller
just raises the interrupt to APM after the ACPM protocol has written
the message to SRAM.

Changes in v5:
- fix dt-bindings by using the correct compatible name in the example
- drop redundand "bindings" from the dt-bindings patch subject
- rebase on top of v6.13-rc3
- Link to v4: https://lore.kernel.org/r/20241212-acpm-v4-upstream-mbox-v4-0-02f8de92cfaf@linaro.org

Changes in v4:
- rename bindings file to be based on compatible: google,gs101-acpm-mbox
- specify doorbell or data mode via '#mbox-cells' dt property. Update
  driver and introduce exynos_mbox_of_xlate() to parse the mode.
- s/samsung/Samsung/, s/exynos/Exynos/
- use writel instead of writel_relaxed
- remove stray of_match_ptr()
- Link to v3: https://lore.kernel.org/linux-arm-kernel/20241205174137.190545-1-tudor.ambarus@linaro.org/

Changes in v3:
- decouple the mailbox controller driver from the ACPM protocol driver
- address Krzysztof's review comments

v2:
https://lore.kernel.org/linux-arm-kernel/20241017163649.3007062-1-tudor.ambarus@linaro.org/

v1:
https://lore.kernel.org/linux-arm-kernel/20241004165301.1979527-1-tudor.ambarus@linaro.org/

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
Tudor Ambarus (3):
      dt-bindings: mailbox: add google,gs101-mbox
      mailbox: add Samsung Exynos driver
      MAINTAINERS: add entry for Samsung Exynos mailbox driver

 .../bindings/mailbox/google,gs101-mbox.yaml        |  79 +++++++++
 MAINTAINERS                                        |  10 ++
 drivers/mailbox/Kconfig                            |  11 ++
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/exynos-mailbox.c                   | 184 +++++++++++++++++++++
 include/dt-bindings/mailbox/google,gs101.h         |  14 ++
 6 files changed, 300 insertions(+)
---
base-commit: 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8
change-id: 20241212-acpm-v4-upstream-mbox-948714004b05

Best regards,
-- 
Tudor Ambarus <tudor.ambarus@linaro.org>

[PATCH v5 1/3] dt-bindings: mailbox: add google,gs101-mbox

[Tudor Ambarus]

Add bindings for the Samsung Exynos Mailbox Controller.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 .../bindings/mailbox/google,gs101-mbox.yaml        | 79 ++++++++++++++++++++++
 include/dt-bindings/mailbox/google,gs101.h         | 14 ++++
 2 files changed, 93 insertions(+)

diff --git a/Documentation/devicetree/bindings/mailbox/google,gs101-mbox.yaml b/Documentation/devicetree/bindings/mailbox/google,gs101-mbox.yaml
new file mode 100644
index 000000000000..bc7288923795
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/google,gs101-mbox.yaml
@@ -0,0 +1,79 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2024 Linaro Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/google,gs101-mbox.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung Exynos Mailbox Controller
+
+maintainers:
+  - Tudor Ambarus <tudor.ambarus@linaro.org>
+
+description: |
+  The samsung exynos mailbox controller has 16 flag bits for hardware interrupt
+  generation and a shared register for passing mailbox messages. When the
+  controller is used by the ACPM protocol the shared register is ignored and
+  the mailbox controller acts as a doorbell. The controller just raises the
+  interrupt to the firmware after the ACPM protocol has written the message to
+  SRAM.
+
+properties:
+  compatible:
+    const: google,gs101-mbox
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: pclk
+
+  interrupts:
+    description: IRQ line for the RX mailbox.
+    maxItems: 1
+
+  '#mbox-cells':
+    description: |
+      <&phandle type channel>
+      phandle : label name of controller.
+      type    : channel type, doorbell or data-transfer.
+      channel : channel number.
+
+      Here is how a client can reference them:
+      mboxes = <&ap2apm_mailbox DOORBELL 2>;
+      mboxes = <&ap2apm_mailbox DATA 3>;
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - '#mbox-cells'
+
+additionalProperties: false
+
+examples:
+  # Doorbell mode.
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/google,gs101.h>
+
+    soc {
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        ap2apm_mailbox: mailbox@17610000 {
+            compatible = "google,gs101-mbox";
+            reg = <0x17610000 0x1000>;
+            clocks = <&cmu_apm CLK_GOUT_APM_MAILBOX_APM_AP_PCLK>;
+            clock-names = "pclk";
+            interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH 0>;
+            #mbox-cells = <2>;
+        };
+    };
diff --git a/include/dt-bindings/mailbox/google,gs101.h b/include/dt-bindings/mailbox/google,gs101.h
new file mode 100644
index 000000000000..7ff4fe669f9e
--- /dev/null
+++ b/include/dt-bindings/mailbox/google,gs101.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright 2024 Linaro Ltd.
+ *
+ * This header provides constants for the defined mailbox channel types.
+ */
+
+#ifndef _DT_BINDINGS_MAILBOX_GOOGLE_GS101_H
+#define _DT_BINDINGS_MAILBOX_GOOGLE_GS101_H
+
+#define DOORBELL	0
+#define DATA		1
+
+#endif /* _DT_BINDINGS_MAILBOX_GOOGLE_GS101_H */

-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v5 1/3] dt-bindings: mailbox: add google,gs101-mbox

[Krzysztof Kozlowski]

On Tue, Dec 17, 2024 at 09:40:20AM +0000, Tudor Ambarus wrote:
> +description: |
> +  The samsung exynos mailbox controller has 16 flag bits for hardware interrupt

If there is going to be any new posting:

The Samsung Exynos mailbox controller, used on Google GS101 SoC, ....

> +  generation and a shared register for passing mailbox messages. When the
> +  controller is used by the ACPM protocol the shared register is ignored and
> +  the mailbox controller acts as a doorbell. The controller just raises the
> +  interrupt to the firmware after the ACPM protocol has written the message to
> +  SRAM.
> +
> +properties:
> +  compatible:
> +    const: google,gs101-mbox
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: pclk
> +
> +  interrupts:
> +    description: IRQ line for the RX mailbox.
> +    maxItems: 1
> +
> +  '#mbox-cells':
> +    description: |
> +      <&phandle type channel>
> +      phandle : label name of controller.
> +      type    : channel type, doorbell or data-transfer.
> +      channel : channel number.
> +
> +      Here is how a client can reference them:
> +      mboxes = <&ap2apm_mailbox DOORBELL 2>;
> +      mboxes = <&ap2apm_mailbox DATA 3>;

This ordering assumes there is channel "2" in doorbel and in data, so
two times "2" and of course the same for all others. If this is
configuration of one channel, so "2" is either doorbell or data, then
type should be the last.

With that assumption:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v5 1/3] dt-bindings: mailbox: add google,gs101-mbox

[Peter Griffin]

Hi,

On Wed, 18 Dec 2024 at 09:29, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Tue, Dec 17, 2024 at 09:40:20AM +0000, Tudor Ambarus wrote:
> > +description: |
> > +  The samsung exynos mailbox controller has 16 flag bits for hardware interrupt
>
> If there is going to be any new posting:
>
> The Samsung Exynos mailbox controller, used on Google GS101 SoC, ....
>
> > +  generation and a shared register for passing mailbox messages. When the
> > +  controller is used by the ACPM protocol the shared register is ignored and
> > +  the mailbox controller acts as a doorbell. The controller just raises the
> > +  interrupt to the firmware after the ACPM protocol has written the message to
> > +  SRAM.
> > +
> > +properties:
> > +  compatible:
> > +    const: google,gs101-mbox
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: pclk
> > +
> > +  interrupts:
> > +    description: IRQ line for the RX mailbox.
> > +    maxItems: 1
> > +
> > +  '#mbox-cells':
> > +    description: |
> > +      <&phandle type channel>
> > +      phandle : label name of controller.
> > +      type    : channel type, doorbell or data-transfer.
> > +      channel : channel number.
> > +
> > +      Here is how a client can reference them:
> > +      mboxes = <&ap2apm_mailbox DOORBELL 2>;
> > +      mboxes = <&ap2apm_mailbox DATA 3>;
>
> This ordering assumes there is channel "2" in doorbel and in data, so
> two times "2" and of course the same for all others. If this is
> configuration of one channel, so "2" is either doorbell or data, then
> type should be the last.

My understanding was each channel is either configured for doorbell or
data, but Tudor can confirm. With Krzysztof's feedback addressed:

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>

regards,

Peter.

Re: [PATCH v5 1/3] dt-bindings: mailbox: add google,gs101-mbox

[Tudor Ambarus]

On 12/18/24 11:23 AM, Peter Griffin wrote:
> Hi,
> 
> On Wed, 18 Dec 2024 at 09:29, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Tue, Dec 17, 2024 at 09:40:20AM +0000, Tudor Ambarus wrote:
>>> +description: |
>>> +  The samsung exynos mailbox controller has 16 flag bits for hardware interrupt
>>
>> If there is going to be any new posting:
>>
>> The Samsung Exynos mailbox controller, used on Google GS101 SoC, ....
>>
>>> +  generation and a shared register for passing mailbox messages. When the
>>> +  controller is used by the ACPM protocol the shared register is ignored and
>>> +  the mailbox controller acts as a doorbell. The controller just raises the
>>> +  interrupt to the firmware after the ACPM protocol has written the message to
>>> +  SRAM.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: google,gs101-mbox
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: pclk
>>> +
>>> +  interrupts:
>>> +    description: IRQ line for the RX mailbox.
>>> +    maxItems: 1
>>> +
>>> +  '#mbox-cells':
>>> +    description: |
>>> +      <&phandle type channel>
>>> +      phandle : label name of controller.
>>> +      type    : channel type, doorbell or data-transfer.
>>> +      channel : channel number.
>>> +
>>> +      Here is how a client can reference them:
>>> +      mboxes = <&ap2apm_mailbox DOORBELL 2>;
>>> +      mboxes = <&ap2apm_mailbox DATA 3>;
>>
>> This ordering assumes there is channel "2" in doorbel and in data, so
>> two times "2" and of course the same for all others. If this is
>> configuration of one channel, so "2" is either doorbell or data, then
>> type should be the last.

ha, nice observation!

> My understanding was each channel is either configured for doorbell or
> data, but Tudor can confirm. With Krzysztof's feedback addressed:

For the ACPM interface use case, mailbox is always used as a doorbell
indeed. Regardless if the ACPM interface writes data or not to SRAM, it
will use the mailbox controller just to flip the interrupt bit without
touching the mbox controller data registers.

For the other cases where the mailbox controller is used to (also?) pass
data via its data registers, I can't tell whether the passing of data is
mandatory or not. At least not by reading the gs101 mailbox datasheet
that I have, it doesn't describe the SR registers. However, Exynos850 -
which has the same registers, says that:
```CPU cores can use these registers for sharing data```.
"Can" implies that writing to SR is optional.

So I assume a channel can work in both modes and clients use mboxes to
specify which mode to use. Shall I still switch the order?

Thanks,
ta

Re: [PATCH v5 1/3] dt-bindings: mailbox: add google,gs101-mbox

[Tudor Ambarus]

Hi, Krzysztof, Jassi,

On 12/17/24 9:40 AM, Tudor Ambarus wrote:

> diff --git a/Documentation/devicetree/bindings/mailbox/google,gs101-mbox.yaml b/Documentation/devicetree/bindings/mailbox/google,gs101-mbox.yaml

cut

> +
> +  '#mbox-cells':
> +    description: |
> +      <&phandle type channel>
> +      phandle : label name of controller.
> +      type    : channel type, doorbell or data-transfer.
> +      channel : channel number.
> +
> +      Here is how a client can reference them:
> +      mboxes = <&ap2apm_mailbox DOORBELL 2>;
> +      mboxes = <&ap2apm_mailbox DATA 3>;
> +    const: 2
> +

Revisiting this, I think that for the ACPM interface mailbox client use
case, it would be better to introduce a mbox property where I reference
just the phandle to the controller:
	mbox = <&ap2apm_mailbox>;

The ACPM interface discovers the mailbox channel IDs at runtime by
parsing SRAM. And all ACPM's channels are of type DOORBELL, thus
specifying the type and channel in DT is redundant.

It would require to extend a bit the mailbox core to provide a
mbox_request_channel_by_args() method. I already wrote a draft and
tested it.

Do you find the idea fine?

Thanks,
ta

Re: [PATCH v5 1/3] dt-bindings: mailbox: add google,gs101-mbox

[Jassi Brar]

On Thu, Dec 19, 2024 at 4:51 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Hi, Krzysztof, Jassi,
>
> On 12/17/24 9:40 AM, Tudor Ambarus wrote:
>
> > diff --git a/Documentation/devicetree/bindings/mailbox/google,gs101-mbox.yaml b/Documentation/devicetree/bindings/mailbox/google,gs101-mbox.yaml
>
> cut
>
> > +
> > +  '#mbox-cells':
> > +    description: |
> > +      <&phandle type channel>
> > +      phandle : label name of controller.
> > +      type    : channel type, doorbell or data-transfer.
> > +      channel : channel number.
> > +
> > +      Here is how a client can reference them:
> > +      mboxes = <&ap2apm_mailbox DOORBELL 2>;
> > +      mboxes = <&ap2apm_mailbox DATA 3>;
> > +    const: 2
> > +
>
> Revisiting this, I think that for the ACPM interface mailbox client use
> case, it would be better to introduce a mbox property where I reference
> just the phandle to the controller:
>         mbox = <&ap2apm_mailbox>;
>
> The ACPM interface discovers the mailbox channel IDs at runtime by
> parsing SRAM. And all ACPM's channels are of type DOORBELL, thus
> specifying the type and channel in DT is redundant.
>
> It would require to extend a bit the mailbox core to provide a
> mbox_request_channel_by_args() method. I already wrote a draft and
> tested it.
>
> Do you find the idea fine?
>
Looking at v6, I prefer this version... maybe modify it a bit.

Even if you get the channel number at runtime, the type (Data vs
Doorbell) is static and needs to be passed via DT. You may have
  mbox = <&ap2apm_mailbox DOORBELL>;
and in your custom of_xlate implementation return any available
"virtual" channel. You could use 'void *data' in
exynos_mbox_send_data() to pass the h/w channel-id, instead of the
index of the virtual channel.

Thanks.

Re: [PATCH v5 1/3] dt-bindings: mailbox: add google,gs101-mbox

[Tudor Ambarus]

Hi, Jassi,

Thanks for the review!

On 12/21/24 2:19 AM, Jassi Brar wrote:
> On Thu, Dec 19, 2024 at 4:51 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Hi, Krzysztof, Jassi,
>>
>> On 12/17/24 9:40 AM, Tudor Ambarus wrote:
>>
>>> diff --git a/Documentation/devicetree/bindings/mailbox/google,gs101-mbox.yaml b/Documentation/devicetree/bindings/mailbox/google,gs101-mbox.yaml
>>
>> cut
>>
>>> +
>>> +  '#mbox-cells':
>>> +    description: |
>>> +      <&phandle type channel>
>>> +      phandle : label name of controller.
>>> +      type    : channel type, doorbell or data-transfer.
>>> +      channel : channel number.
>>> +
>>> +      Here is how a client can reference them:
>>> +      mboxes = <&ap2apm_mailbox DOORBELL 2>;
>>> +      mboxes = <&ap2apm_mailbox DATA 3>;
>>> +    const: 2
>>> +
>>
>> Revisiting this, I think that for the ACPM interface mailbox client use
>> case, it would be better to introduce a mbox property where I reference
>> just the phandle to the controller:
>>         mbox = <&ap2apm_mailbox>;
>>
>> The ACPM interface discovers the mailbox channel IDs at runtime by
>> parsing SRAM. And all ACPM's channels are of type DOORBELL, thus
>> specifying the type and channel in DT is redundant.
>>
>> It would require to extend a bit the mailbox core to provide a
>> mbox_request_channel_by_args() method. I already wrote a draft and
>> tested it.
>>
>> Do you find the idea fine?
>>
> Looking at v6, I prefer this version... maybe modify it a bit.

Just to summarize for the readers, in the end I chose for the
controllers to allow #mbox-cells = <0>; and for the clients to still use
the mboxes property, but just to reference the phandle to the controller:
	mboxes = <&ap2apm_mailbox>;
Then I updated the mailbox core to allow clients to request channels by
passing some args containing channel identifiers to the controllers,
that the controllers xlate() using their own method.
> 
> Even if you get the channel number at runtime, the type (Data vs
> Doorbell) is static and needs to be passed via DT. You may have
>   mbox = <&ap2apm_mailbox DOORBELL>;

The ACPM interface uses mailbox always in DOORBELL mode. If it has some
data to send, it will always send it via SRAM. Do I still need to
specify the channel type in DT?

For all the other mailbox clients than ACPM, that will reference the
same mailbox controller (same compatible if you want), I'm thinking that
we'll update the #mbox-cells to have an maxItems of 2, where they'll be
able to pass the channel ID and type via DT.

ACPM has its own dedicated mailbox controller ap2apm_mailbox, thus it uses:
	#mbox-cells = <0>;
channels IDs are from SRAM and type is always DOORBELL.

All the other mailbox controllers using the same compatible will use
	#mbox-cells = <2>;

> and in your custom of_xlate implementation return any available
> "virtual" channel. You could use 'void *data' in

Would you please extend this idea a little bit? What is a virtual channel?

In the ACPM interface mailbox client I request all the channels that are
advertised in SRAM, each mailbox channel has a 1 to 1 relation with a
h/w channel ID.

Cheers,
ta

> exynos_mbox_send_data() to pass the h/w channel-id, instead of the
> index of the virtual channel.
> 
> Thanks.

Re: [PATCH v5 1/3] dt-bindings: mailbox: add google,gs101-mbox

[Jassi Brar]

On Sat, Dec 21, 2024 at 12:45 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Hi, Jassi,
>
> Thanks for the review!
>
> On 12/21/24 2:19 AM, Jassi Brar wrote:
> > On Thu, Dec 19, 2024 at 4:51 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >> Hi, Krzysztof, Jassi,
> >>
> >> On 12/17/24 9:40 AM, Tudor Ambarus wrote:
> >>
> >>> diff --git a/Documentation/devicetree/bindings/mailbox/google,gs101-mbox.yaml b/Documentation/devicetree/bindings/mailbox/google,gs101-mbox.yaml
> >>
> >> cut
> >>
> >>> +
> >>> +  '#mbox-cells':
> >>> +    description: |
> >>> +      <&phandle type channel>
> >>> +      phandle : label name of controller.
> >>> +      type    : channel type, doorbell or data-transfer.
> >>> +      channel : channel number.
> >>> +
> >>> +      Here is how a client can reference them:
> >>> +      mboxes = <&ap2apm_mailbox DOORBELL 2>;
> >>> +      mboxes = <&ap2apm_mailbox DATA 3>;
> >>> +    const: 2
> >>> +
> >>
> >> Revisiting this, I think that for the ACPM interface mailbox client use
> >> case, it would be better to introduce a mbox property where I reference
> >> just the phandle to the controller:
> >>         mbox = <&ap2apm_mailbox>;
> >>
> >> The ACPM interface discovers the mailbox channel IDs at runtime by
> >> parsing SRAM. And all ACPM's channels are of type DOORBELL, thus
> >> specifying the type and channel in DT is redundant.
> >>
> >> It would require to extend a bit the mailbox core to provide a
> >> mbox_request_channel_by_args() method. I already wrote a draft and
> >> tested it.
> >>
> >> Do you find the idea fine?
> >>
> > Looking at v6, I prefer this version... maybe modify it a bit.
>
> Just to summarize for the readers, in the end I chose for the
> controllers to allow #mbox-cells = <0>; and for the clients to still use
> the mboxes property, but just to reference the phandle to the controller:
>         mboxes = <&ap2apm_mailbox>;
>
This was already supported, see drivers/mailbox/bcm2835-mailbox.c for example.

> Then I updated the mailbox core to allow clients to request channels by
> passing some args containing channel identifiers to the controllers,
> that the controllers xlate() using their own method.
>
This is unnecessary.
If you don't pass the doorbell number from DT, each channel populated
by the driver is just a s/w construct or a 'virtual' channel. Make use
of 'void *data'  in send_data() to specify the doorbell.

Cheers.
Jassi

Re: [PATCH v5 1/3] dt-bindings: mailbox: add google,gs101-mbox

[Tudor Ambarus]

Hi, Jassi,

On 1/3/25 3:39 AM, Jassi Brar wrote:
>>> Looking at v6, I prefer this version... maybe modify it a bit.
>>
>> Just to summarize for the readers, in the end I chose for the
>> controllers to allow #mbox-cells = <0>; and for the clients to still use
>> the mboxes property, but just to reference the phandle to the controller:
>>         mboxes = <&ap2apm_mailbox>;
>>
> This was already supported, see drivers/mailbox/bcm2835-mailbox.c for example.

Thanks for the pointer. I was referring to the bindings patch:
https://lore.kernel.org/linux-arm-kernel/20241220-acpm-v4-upstream-mbox-v6-1-a6942806e52a@linaro.org/
> 
>> Then I updated the mailbox core to allow clients to request channels by
>> passing some args containing channel identifiers to the controllers,
>> that the controllers xlate() using their own method.
>>
> This is unnecessary.
> If you don't pass the doorbell number from DT, each channel populated
> by the driver is just a s/w construct or a 'virtual' channel. Make use
> of 'void *data'  in send_data() to specify the doorbell.
> 

I think this introduces concurrency problems if the channel identifiers
passed by 'void *data' don't match the virtual channel used for sending
the messages. Do we want to allow this?

Also, if we use 'void *data' to pass channel identifiers, the channel
checks will have to be made at send_data() time. Thus if passing wrong
channel type for example, the mailbox client will eventually get a
-ENOBUFS and a "Try increasing MBOX_TX_QUEUE_LEN" message, which I find
misleading.

Thanks,
ta

Re: [PATCH v5 1/3] dt-bindings: mailbox: add google,gs101-mbox

[Tudor Ambarus]

Hi, Jassi,

On 1/3/25 9:57 AM, Tudor Ambarus wrote:
>>> Then I updated the mailbox core to allow clients to request channels by
>>> passing some args containing channel identifiers to the controllers,
>>> that the controllers xlate() using their own method.
>>>
>> This is unnecessary.
>> If you don't pass the doorbell number from DT, each channel populated
>> by the driver is just a s/w construct or a 'virtual' channel. Make use
>> of 'void *data'  in send_data() to specify the doorbell.
>>
> I think this introduces concurrency problems if the channel identifiers
> passed by 'void *data' don't match the virtual channel used for sending
> the messages. Do we want to allow this?
> 
> Also, if we use 'void *data' to pass channel identifiers, the channel
> checks will have to be made at send_data() time. Thus if passing wrong
> channel type for example, the mailbox client will eventually get a
> -ENOBUFS and a "Try increasing MBOX_TX_QUEUE_LEN" message, which I find
> misleading.

Shall I still use 'void *data' to pass channel identifiers through
send_data()? I'd like to respin everything.

Thanks!
ta

Re: [PATCH v5 1/3] dt-bindings: mailbox: add google,gs101-mbox

[Jassi Brar]

On Wed, Jan 8, 2025 at 3:38 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Hi, Jassi,
>
> On 1/3/25 9:57 AM, Tudor Ambarus wrote:
> >>> Then I updated the mailbox core to allow clients to request channels by
> >>> passing some args containing channel identifiers to the controllers,
> >>> that the controllers xlate() using their own method.
> >>>
> >> This is unnecessary.
> >> If you don't pass the doorbell number from DT, each channel populated
> >> by the driver is just a s/w construct or a 'virtual' channel. Make use
> >> of 'void *data'  in send_data() to specify the doorbell.
> >>
> > I think this introduces concurrency problems if the channel identifiers
> > passed by 'void *data' don't match the virtual channel used for sending
> > the messages. Do we want to allow this?
> >
> > Also, if we use 'void *data' to pass channel identifiers, the channel
> > checks will have to be made at send_data() time. Thus if passing wrong
> > channel type for example, the mailbox client will eventually get a
> > -ENOBUFS and a "Try increasing MBOX_TX_QUEUE_LEN" message, which I find
> > misleading.
>
> Shall I still use 'void *data' to pass channel identifiers through
> send_data()? I'd like to respin everything.
>
Yes, please do.

thanks

Re: [PATCH v5 1/3] dt-bindings: mailbox: add google,gs101-mbox

[Tudor Ambarus]

On 1/12/25 4:59 PM, Jassi Brar wrote:

>>>>> Then I updated the mailbox core to allow clients to request channels by
>>>>> passing some args containing channel identifiers to the controllers,
>>>>> that the controllers xlate() using their own method.
>>>>>
>>>> This is unnecessary.
>>>> If you don't pass the doorbell number from DT, each channel populated
>>>> by the driver is just a s/w construct or a 'virtual' channel. Make use
>>>> of 'void *data'  in send_data() to specify the doorbell.
>>>>
>>> I think this introduces concurrency problems if the channel identifiers
>>> passed by 'void *data' don't match the virtual channel used for sending
>>> the messages. Do we want to allow this?
>>>
>>> Also, if we use 'void *data' to pass channel identifiers, the channel
>>> checks will have to be made at send_data() time. Thus if passing wrong
>>> channel type for example, the mailbox client will eventually get a
>>> -ENOBUFS and a "Try increasing MBOX_TX_QUEUE_LEN" message, which I find
>>> misleading.
>>
>> Shall I still use 'void *data' to pass channel identifiers through
>> send_data()? I'd like to respin everything.
>>
> Yes, please do.
> 

What shall I do in driver's of_xlate method, always return
&mbox->chans[0], as bcm2835 does? All 14 doorbels will be serialized
with mobox->chans[0].lock.

I could use a list of channels in the controller and provide some
get/put channel methods, but the virtual channel ID will not match the
actual channel ID that's used for communication. I'll also need to
introduce channel ops, to put the channel that was acquired via of_xlate
from the list of available channels.

Aren't we better off with the mbox_request_channel_by_args() that I
introduced in v6? Or if you think there's better option I'll be happy to
implement it. I need an agreement on the overall idea.

Thanks,
ta

Re: [PATCH v5 1/3] dt-bindings: mailbox: add google,gs101-mbox

[Jassi Brar]

On Mon, Jan 13, 2025 at 3:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> On 1/12/25 4:59 PM, Jassi Brar wrote:
>
> >>>>> Then I updated the mailbox core to allow clients to request channels by
> >>>>> passing some args containing channel identifiers to the controllers,
> >>>>> that the controllers xlate() using their own method.
> >>>>>
> >>>> This is unnecessary.
> >>>> If you don't pass the doorbell number from DT, each channel populated
> >>>> by the driver is just a s/w construct or a 'virtual' channel. Make use
> >>>> of 'void *data'  in send_data() to specify the doorbell.
> >>>>
> >>> I think this introduces concurrency problems if the channel identifiers
> >>> passed by 'void *data' don't match the virtual channel used for sending
> >>> the messages. Do we want to allow this?
> >>>
> >>> Also, if we use 'void *data' to pass channel identifiers, the channel
> >>> checks will have to be made at send_data() time. Thus if passing wrong
> >>> channel type for example, the mailbox client will eventually get a
> >>> -ENOBUFS and a "Try increasing MBOX_TX_QUEUE_LEN" message, which I find
> >>> misleading.
> >>
> >> Shall I still use 'void *data' to pass channel identifiers through
> >> send_data()? I'd like to respin everything.
> >>
> > Yes, please do.
> >
>
> What shall I do in driver's of_xlate method, always return
> &mbox->chans[0], as bcm2835 does?
>
No, not the first. The first _free_ channel. One way to identify a
free channel is by checking mbox_chan.cl is NULL.

thnx

[PATCH v5 2/3] mailbox: add Samsung Exynos driver

[Tudor Ambarus]

The Samsung Exynos mailbox controller has 16 flag bits for hardware
interrupt generation and a shared register for passing mailbox messages.
When the controller is used by the ACPM protocol the shared register is
ignored and the mailbox controller acts as a doorbell. The controller
just raises the interrupt to APM after the ACPM protocol has written
the message to SRAM.

Add support for the Samsung Exynos mailbox controller.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/mailbox/Kconfig          |  11 +++
 drivers/mailbox/Makefile         |   2 +
 drivers/mailbox/exynos-mailbox.c | 184 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 197 insertions(+)

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 8ecba7fb999e..44b808c4d97f 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -36,6 +36,17 @@ config ARM_MHU_V3
 	  that provides different means of transports: supported extensions
 	  will be discovered and possibly managed at probe-time.
 
+config EXYNOS_MBOX
+	tristate "Exynos Mailbox"
+	depends on ARCH_EXYNOS || COMPILE_TEST
+	help
+	  Say Y here if you want to build the Samsung Exynos Mailbox controller
+	  driver. The controller has 16 flag bits for hardware interrupt
+	  generation and a shared register for passing mailbox messages.
+	  When the controller is used by the ACPM protocol the shared register
+	  is ignored and the mailbox controller acts as a doorbell that raises
+	  the interrupt to the ACPM firmware.
+
 config IMX_MBOX
 	tristate "i.MX Mailbox"
 	depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 5f4f5b0ce2cc..86192b5c7c32 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -11,6 +11,8 @@ obj-$(CONFIG_ARM_MHU_V2)	+= arm_mhuv2.o
 
 obj-$(CONFIG_ARM_MHU_V3)	+= arm_mhuv3.o
 
+obj-$(CONFIG_EXYNOS_MBOX)	+= exynos-mailbox.o
+
 obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
 
 obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)	+= armada-37xx-rwtm-mailbox.o
diff --git a/drivers/mailbox/exynos-mailbox.c b/drivers/mailbox/exynos-mailbox.c
new file mode 100644
index 000000000000..9d875806d0f9
--- /dev/null
+++ b/drivers/mailbox/exynos-mailbox.c
@@ -0,0 +1,184 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2020 Samsung Electronics Co., Ltd.
+ * Copyright 2020 Google LLC.
+ * Copyright 2024 Linaro Ltd.
+ */
+
+#include <dt-bindings/mailbox/google,gs101.h>
+#include <linux/bitops.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define EXYNOS_MBOX_MCUCTRL		0x0	/* Mailbox Control Register */
+#define EXYNOS_MBOX_INTCR0		0x24	/* Interrupt Clear Register 0 */
+#define EXYNOS_MBOX_INTMR0		0x28	/* Interrupt Mask Register 0 */
+#define EXYNOS_MBOX_INTSR0		0x2c	/* Interrupt Status Register 0 */
+#define EXYNOS_MBOX_INTMSR0		0x30	/* Interrupt Mask Status Register 0 */
+#define EXYNOS_MBOX_INTGR1		0x40	/* Interrupt Generation Register 1 */
+#define EXYNOS_MBOX_INTMR1		0x48	/* Interrupt Mask Register 1 */
+#define EXYNOS_MBOX_INTSR1		0x4c	/* Interrupt Status Register 1 */
+#define EXYNOS_MBOX_INTMSR1		0x50	/* Interrupt Mask Status Register 1 */
+
+#define EXYNOS_MBOX_INTMR0_MASK		GENMASK(15, 0)
+#define EXYNOS_MBOX_INTGR1_MASK		GENMASK(15, 0)
+
+#define EXYNOS_MBOX_CHAN_COUNT		HWEIGHT32(EXYNOS_MBOX_INTGR1_MASK)
+
+enum {
+	EXYNOS_MBOX_CELL_TYPE,
+	EXYNOS_MBOX_CELL_ID,
+	EXYNOS_MBOX_CELLS
+};
+
+#define EXYNOS_MBOX_CELL_TYPE_COUNT	2
+
+/**
+ * struct exynos_mbox - driver's private data.
+ * @regs:	mailbox registers base address.
+ * @mbox:	pointer to the mailbox controller.
+ * @dev:	pointer to the mailbox device.
+ * @pclk:	pointer to the mailbox peripheral clock.
+ */
+struct exynos_mbox {
+	void __iomem *regs;
+	struct mbox_controller *mbox;
+	struct device *dev;
+	struct clk *pclk;
+};
+
+static int exynos_mbox_chan_index(struct mbox_chan *chan)
+{
+	struct mbox_controller *mbox = chan->mbox;
+	int i;
+
+	for (i = 0; i < mbox->num_chans; i++)
+		if (chan == &mbox->chans[i])
+			return i;
+	return -EINVAL;
+}
+
+static int exynos_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct exynos_mbox *exynos_mbox = dev_get_drvdata(chan->mbox->dev);
+	int index;
+
+	index = exynos_mbox_chan_index(chan);
+	if (index < 0)
+		return index;
+
+	writel(BIT(index), exynos_mbox->regs + EXYNOS_MBOX_INTGR1);
+
+	return 0;
+}
+
+static const struct mbox_chan_ops exynos_mbox_chan_ops = {
+	.send_data = exynos_mbox_send_data,
+};
+
+static struct mbox_chan *exynos_mbox_of_xlate(struct mbox_controller *mbox,
+					      const struct of_phandle_args *sp)
+{
+	u32 type, id;
+
+	if (sp->args_count != EXYNOS_MBOX_CELLS) {
+		dev_err(mbox->dev, "Invalid argument count %d\n",
+			sp->args_count);
+		return ERR_PTR(-EINVAL);
+	}
+
+	type = sp->args[EXYNOS_MBOX_CELL_TYPE];
+	if (type >= EXYNOS_MBOX_CELL_TYPE_COUNT) {
+		dev_err(mbox->dev, "Invalid channel type %d\n", type);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (type == DATA) {
+		dev_err(mbox->dev, "DATA channel type [%d] not supported\n",
+			type);
+		return ERR_PTR(-EINVAL);
+	};
+
+	id = sp->args[EXYNOS_MBOX_CELL_ID];
+	if (id >= mbox->num_chans) {
+		dev_err(mbox->dev, "Invalid channel ID %d\n", id);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return &mbox->chans[id];
+}
+
+static const struct of_device_id exynos_mbox_match[] = {
+	{ .compatible = "google,gs101-mbox" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, exynos_mbox_match);
+
+static int exynos_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct exynos_mbox *exynos_mbox;
+	struct mbox_controller *mbox;
+	struct mbox_chan *chans;
+	int i;
+
+	exynos_mbox = devm_kzalloc(dev, sizeof(*exynos_mbox), GFP_KERNEL);
+	if (!exynos_mbox)
+		return -ENOMEM;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	chans = devm_kcalloc(dev, EXYNOS_MBOX_CHAN_COUNT, sizeof(*chans),
+			     GFP_KERNEL);
+	if (!chans)
+		return -ENOMEM;
+
+	exynos_mbox->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(exynos_mbox->regs))
+		return PTR_ERR(exynos_mbox->regs);
+
+	exynos_mbox->pclk = devm_clk_get_enabled(dev, "pclk");
+	if (IS_ERR(exynos_mbox->pclk))
+		return dev_err_probe(dev, PTR_ERR(exynos_mbox->pclk),
+				     "Failed to enable clock.\n");
+
+	mbox->num_chans = EXYNOS_MBOX_CHAN_COUNT;
+	mbox->chans = chans;
+	mbox->dev = dev;
+	mbox->ops = &exynos_mbox_chan_ops;
+	mbox->of_xlate = exynos_mbox_of_xlate;
+
+	for (i = 0; i < EXYNOS_MBOX_CHAN_COUNT; i++)
+		chans[i].mbox = mbox;
+
+	exynos_mbox->dev = dev;
+	exynos_mbox->mbox = mbox;
+
+	platform_set_drvdata(pdev, exynos_mbox);
+
+	/* Mask out all interrupts. We support just polling channels for now. */
+	writel(EXYNOS_MBOX_INTMR0_MASK, exynos_mbox->regs + EXYNOS_MBOX_INTMR0);
+
+	return devm_mbox_controller_register(dev, mbox);
+}
+
+static struct platform_driver exynos_mbox_driver = {
+	.probe	= exynos_mbox_probe,
+	.driver	= {
+		.name = "exynos-acpm-mbox",
+		.of_match_table	= exynos_mbox_match,
+	},
+};
+module_platform_driver(exynos_mbox_driver);
+
+MODULE_AUTHOR("Tudor Ambarus <tudor.ambarus@linaro.org>");
+MODULE_DESCRIPTION("Samsung Exynos mailbox driver");
+MODULE_LICENSE("GPL");

-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v5 2/3] mailbox: add Samsung Exynos driver

[Krzysztof Kozlowski]

On Tue, Dec 17, 2024 at 09:40:21AM +0000, Tudor Ambarus wrote:
> The Samsung Exynos mailbox controller has 16 flag bits for hardware
> interrupt generation and a shared register for passing mailbox messages.
> When the controller is used by the ACPM protocol the shared register is
> ignored and the mailbox controller acts as a doorbell. The controller
> just raises the interrupt to APM after the ACPM protocol has written
> the message to SRAM.
> 
> Add support for the Samsung Exynos mailbox controller.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/mailbox/Kconfig          |  11 +++
>  drivers/mailbox/Makefile         |   2 +
>  drivers/mailbox/exynos-mailbox.c | 184 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 197 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v5 2/3] mailbox: add Samsung Exynos driver

[Peter Griffin]

Hi Tudor,

On Tue, 17 Dec 2024 at 09:40, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> The Samsung Exynos mailbox controller has 16 flag bits for hardware
> interrupt generation and a shared register for passing mailbox messages.
> When the controller is used by the ACPM protocol the shared register is
> ignored and the mailbox controller acts as a doorbell. The controller
> just raises the interrupt to APM after the ACPM protocol has written
> the message to SRAM.
>
> Add support for the Samsung Exynos mailbox controller.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>

regards,

Peter

[..]

Re: [PATCH v5 2/3] mailbox: add Samsung Exynos driver

[Jassi Brar]

On Tue, Dec 17, 2024 at 3:40 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
....
> +
> +static int exynos_mbox_chan_index(struct mbox_chan *chan)
> +{
> +       struct mbox_controller *mbox = chan->mbox;
> +       int i;
> +
> +       for (i = 0; i < mbox->num_chans; i++)
> +               if (chan == &mbox->chans[i])
> +                       return i;
> +       return -EINVAL;
> +}
>
Maybe simply return (chan - mbox->chans) ?

Cheers!

[PATCH v5 3/3] MAINTAINERS: add entry for Samsung Exynos mailbox driver

[Tudor Ambarus]

Add entry for the Samsung Exynos mailbox driver.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index baf0eeb9a355..6bef5fc5e4ee 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3023,6 +3023,7 @@ F:	drivers/*/*s3c24*
 F:	drivers/*/*s3c64xx*
 F:	drivers/*/*s5pv210*
 F:	drivers/clocksource/samsung_pwm_timer.c
+F:	drivers/mailbox/exynos-mailbox.c
 F:	drivers/memory/samsung/
 F:	drivers/pwm/pwm-samsung.c
 F:	drivers/soc/samsung/
@@ -20717,6 +20718,15 @@ F:	arch/arm64/boot/dts/exynos/exynos850*
 F:	drivers/clk/samsung/clk-exynos850.c
 F:	include/dt-bindings/clock/exynos850.h
 
+SAMSUNG EXYNOS MAILBOX DRIVER
+M:	Tudor Ambarus <tudor.ambarus@linaro.org>
+L:	linux-kernel@vger.kernel.org
+L:	linux-samsung-soc@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/mailbox/google,gs101-mbox.yaml
+F:	drivers/mailbox/exynos-mailbox.c
+F:	include/dt-bindings/mailbox/google,gs101.h
+
 SAMSUNG EXYNOS PSEUDO RANDOM NUMBER GENERATOR (RNG) DRIVER
 M:	Krzysztof Kozlowski <krzk@kernel.org>
 L:	linux-crypto@vger.kernel.org

-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v5 3/3] MAINTAINERS: add entry for Samsung Exynos mailbox driver

[Krzysztof Kozlowski]

On Tue, Dec 17, 2024 at 09:40:22AM +0000, Tudor Ambarus wrote:
> Add entry for the Samsung Exynos mailbox driver.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  MAINTAINERS | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v5 3/3] MAINTAINERS: add entry for Samsung Exynos mailbox driver

[Peter Griffin]

Hi Tudor,

On Tue, 17 Dec 2024 at 09:40, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Add entry for the Samsung Exynos mailbox driver.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>