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

[Tudor Ambarus <tudor.ambarus@linaro.org>]

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.