| Message ID | 20241217-acpm-v4-upstream-mbox-v5-1-cd1d3951fe84@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mailbox: add Samsung Exynos driver | expand |
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
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.
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
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
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.
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.
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 */
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(+)