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

On Sat, Dec 21, 2024 at 12:45 AM Tudor Ambarus [off-list ref] wrote:

Hi, Jassi,

Thanks for the review!

On 12/21/24 2:19 AM, Jassi Brar wrote:

quoted

On Thu, Dec 19, 2024 at 4:51 AM Tudor Ambarus [off-list ref] wrote:

quoted

Hi, Krzysztof, Jassi,

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

quoted

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

cut

quoted

+
+  '#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