| Message ID | 20241219-mbox_request_channel_by_args-v1-0-617a6910f842@linaro.org |
|---|---|
| Headers | show |
| Series | mailbox: add support for clients to request channels by arguments | expand |
On Thu, Dec 19, 2024 at 01:07:46PM +0000, Tudor Ambarus wrote: > There are mailbox clients that can discover the mailbox channel ID at > run-time. For such cases passing the channel identifier via DT is > redundant. Add support for referencing controllers solely by node. I don't really get your implementation, why not just allow #mbox-cells = 0? That's what's done for things like fixed frequency clocks that only have a single output. Cheers, Conor. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > Documentation/devicetree/bindings/mailbox/mailbox.txt | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mailbox/mailbox.txt b/Documentation/devicetree/bindings/mailbox/mailbox.txt > index af8ecee2ac68..0c4295a62f61 100644 > --- a/Documentation/devicetree/bindings/mailbox/mailbox.txt > +++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt > @@ -5,9 +5,10 @@ assign appropriate mailbox channel to client drivers. > > * Mailbox Controller > > -Required property: > -- #mbox-cells: Must be at least 1. Number of cells in a mailbox > - specifier. > +Optional property: > +- #mbox-cells: Must be at least 1. Number of cells in a mailbox specifier. > + The property becomes mandatory for the cases where the clients > + reference the controller via the mboxes property. > > Example: > mailbox: mailbox { > @@ -19,7 +20,11 @@ Example: > * Mailbox Client > > Required property: > +Clients must reference the mailbox controller either via the mboxes or mbox > +properties. > - mboxes: List of phandle and mailbox channel specifiers. > +- mbox: phandle pointing to the controller. Used by clients that can discover > + the channel identifiers at runtime. > > Optional property: > - mbox-names: List of identifier strings for each mailbox channel. > @@ -29,7 +34,13 @@ Optional property: > communication between the mailbox client and the remote. > > > -Example: > +Example using mbox: > + power-management { > + ... > + mbox = <&mailbox>; > + }; > + > +Example using mboxes: > pwr_cntrl: power { > ... > mbox-names = "pwr-ctrl", "rpc"; > > -- > 2.47.1.613.gc27f4b7a9f-goog >
Hi, Conor, On 12/19/24 2:11 PM, Conor Dooley wrote: >> There are mailbox clients that can discover the mailbox channel ID at >> run-time. For such cases passing the channel identifier via DT is >> redundant. Add support for referencing controllers solely by node. > I don't really get your implementation, why not just allow #mbox-cells = 0? > That's what's done for things like fixed frequency clocks that only have > a single output. Ah, indeed! instead of: of_parse_phandle(dev->of_node, "mbox", 0); I can do a: of_parse_phandle_with_args(dev->of_node, "mboxes", "#mbox-cells", 0, &of_args) where #mbox-cells = 0; Or ... can I pass NULL for cells_name and make the #mbox-cells property optional and still keeping its requirement of being at least 1? Thanks! ta
On Thu, Dec 19, 2024 at 03:42:11PM +0000, Tudor Ambarus wrote: > Hi, Conor, > > On 12/19/24 2:11 PM, Conor Dooley wrote: > >> There are mailbox clients that can discover the mailbox channel ID at > >> run-time. For such cases passing the channel identifier via DT is > >> redundant. Add support for referencing controllers solely by node. > > I don't really get your implementation, why not just allow #mbox-cells = 0? > > That's what's done for things like fixed frequency clocks that only have > > a single output. > > Ah, indeed! > > instead of: > of_parse_phandle(dev->of_node, "mbox", 0); > I can do a: > of_parse_phandle_with_args(dev->of_node, "mboxes", > "#mbox-cells", 0, &of_args) > where #mbox-cells = 0; > > Or ... can I pass NULL for cells_name and make the #mbox-cells property > optional and still keeping its requirement of being at least 1? I think the mbox-cells = 0 approach is preferred, that property is what marks it as a mailbox controller after all. Perhaps Rob or Krzysztof can comment?
On 12/19/24 6:58 PM, Conor Dooley wrote: > On Thu, Dec 19, 2024 at 03:42:11PM +0000, Tudor Ambarus wrote: >> Hi, Conor, >> >> On 12/19/24 2:11 PM, Conor Dooley wrote: >>>> There are mailbox clients that can discover the mailbox channel ID at >>>> run-time. For such cases passing the channel identifier via DT is >>>> redundant. Add support for referencing controllers solely by node. >>> I don't really get your implementation, why not just allow #mbox-cells = 0? >>> That's what's done for things like fixed frequency clocks that only have >>> a single output. >> >> Ah, indeed! >> >> instead of: >> of_parse_phandle(dev->of_node, "mbox", 0); >> I can do a: >> of_parse_phandle_with_args(dev->of_node, "mboxes", >> "#mbox-cells", 0, &of_args) >> where #mbox-cells = 0; >> >> Or ... can I pass NULL for cells_name and make the #mbox-cells property >> optional and still keeping its requirement of being at least 1? > > I think the mbox-cells = 0 approach is preferred, that property is what > marks it as a mailbox controller after all. Perhaps Rob or Krzysztof can > comment? I think using mbox-cells = 0 is better indeed. In my proposal I considered the list to always have a single phandle, thus a reference to a single mailbox controller, whereas it may be possible that clients to reference multiple mailbox controllers. If so, #mbox-cells needs to be defined in all the controllers, for consistency reasons, similar to what happens with fixed clocks, as you already mentioned. Thus I'll change the method to: struct mbox_chan *mbox_request_channel_by_args(struct mbox_client *cl, int index, const struct mbox_xlate_args *spec); and use of_parse_phandle_with_args() in it. Thanks, Conor! ta
On Fri, Dec 20, 2024 at 07:51:32AM +0000, Tudor Ambarus wrote: > > > On 12/19/24 6:58 PM, Conor Dooley wrote: > > On Thu, Dec 19, 2024 at 03:42:11PM +0000, Tudor Ambarus wrote: > >> Hi, Conor, > >> > >> On 12/19/24 2:11 PM, Conor Dooley wrote: > >>>> There are mailbox clients that can discover the mailbox channel ID at > >>>> run-time. For such cases passing the channel identifier via DT is > >>>> redundant. Add support for referencing controllers solely by node. > >>> I don't really get your implementation, why not just allow #mbox-cells = 0? > >>> That's what's done for things like fixed frequency clocks that only have > >>> a single output. > >> > >> Ah, indeed! > >> > >> instead of: > >> of_parse_phandle(dev->of_node, "mbox", 0); > >> I can do a: > >> of_parse_phandle_with_args(dev->of_node, "mboxes", > >> "#mbox-cells", 0, &of_args) > >> where #mbox-cells = 0; > >> > >> Or ... can I pass NULL for cells_name and make the #mbox-cells property > >> optional and still keeping its requirement of being at least 1? > > > > I think the mbox-cells = 0 approach is preferred, that property is what > > marks it as a mailbox controller after all. Perhaps Rob or Krzysztof can > > comment? > > I think using mbox-cells = 0 is better indeed. In my proposal I > considered the list to always have a single phandle, thus a reference to > a single mailbox controller, whereas it may be possible that clients to > reference multiple mailbox controllers. If so, #mbox-cells needs to be > defined in all the controllers, for consistency reasons, similar to what > happens with fixed clocks, as you already mentioned. Oh right, I had totally forgotten about that, that's a solid argument for the mbox-cells = 0 approach for sure.
There are clients that can discover channel identifiers at runtime by parsing a shared memory for example, as in the ACPM interface's case. For such cases passing the channel identifiers via DT is redundant. Supply a new framework API: mbox_request_channel_by_args(). It works by supplying the usual client pointer as the first argument and a pointer to a ``const struct mbox_xlate_args`` as a second. The newly introduced struct is modeled after ``struct of_phandle_args``. The API will search the client's node for a ``mbox`` phandle, identify the controller's device node, and then call that controller's xlate() method that will return a pointer to a mbox_chan or a ERR_PTR. The binding between the channel and the client is done in the typical way. This allows clients to reference the controller node as following: firmware { acpm_ipc: power-management { compatible = "google,gs101-acpm-ipc"; - mboxes = <&ap2apm_mailbox 0 0 - &ap2apm_mailbox 0 1 - &ap2apm_mailbox 0 2 - &ap2apm_mailbox 0 3 - &ap2apm_mailbox 0 4 - &ap2apm_mailbox 0 5 - &ap2apm_mailbox 0 6 - &ap2apm_mailbox 0 7 - &ap2apm_mailbox 0 8 - &ap2apm_mailbox 0 9 - &ap2apm_mailbox 0 10 - &ap2apm_mailbox 0 11 - &ap2apm_mailbox 0 12 - &ap2apm_mailbox 0 13 - &ap2apm_mailbox 0 14>; + mbox = <&ap2apm_mailbox>; shmem = <&apm_sram>; }; }; Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> --- Tudor Ambarus (2): dt-bindings: mailbox: add support for referencing controllers solely by node mailbox: add support for clients to request channels by arguments .../devicetree/bindings/mailbox/mailbox.txt | 19 ++++++-- drivers/mailbox/mailbox.c | 57 ++++++++++++++++++++++ include/linux/mailbox.h | 17 +++++++ include/linux/mailbox_client.h | 3 ++ include/linux/mailbox_controller.h | 4 ++ 5 files changed, 96 insertions(+), 4 deletions(-) --- base-commit: 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8 change-id: 20241219-mbox_request_channel_by_args-7115089ed492 Best regards,