| Message ID | 20241217-acpm-v4-upstream-mbox-v5-0-cd1d3951fe84@linaro.org |
|---|---|
| Headers | show |
| Series | mailbox: add Samsung Exynos driver | expand |
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.
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
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
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
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,