On 04/10/17 15:17, Arnd Bergmann wrote:

On Wed, Oct 4, 2017 at 3:53 PM, Sudeep Holla [off-list ref] wrote:

quoted

On 04/10/17 13:35, Arnd Bergmann wrote:

quoted

On Wed, Oct 4, 2017 at 1:07 PM, Sudeep Holla [off-list ref] wrote:

quoted

quoted

quoted

Yes it depends on #clock-cells property. That's the main reason for
adding #clock-cells

I'm still unclear on this. Do you mean we look for a subnode with
reg=<0x14> and then assume it's a clock node and require the
 #clock-cells to be there,

Yes that's how it's used. Presence of subnode with reg=0x14 indicates
clock protocol and #clock-cells to indicate that it's clock provider
expecting 1 parameter from consumer which is the clock identifier.

or do we look through the sub-nodes to

quoted

find one with the #clock-cells property and then look up the 'reg'
property to find out which protocol number to use?

Not this way. Do you see any issues ?

We normally don't assume that a particular unit address implies
a specific function. Conventionally that would be done by matching
the "compatible" property instead.

What you do clearly works, but it's surprising to the reader.

quoted

quoted

I think the problem is the way we use the mailbox API in Linux, which
is completely abstract at the moment: it could be a pure doorbell, a
single-register for a data, some structured memory, or a
variable-length message. The assumption today is that the mailbox
user and the mailbox driver agree on the interpretation of that
void pointer.

Unfortunately true.

quoted

This breaks down here, as you require the message to be a
variable-length message in a fixed physical location, but assume that
the mailbox serves only as a doorbell.

Yes.

quoted

The solution might be to extend the mailbox API slightly, to
have explicit support for variable-length messages, and implement
support for that in either mailbox drivers or as an abstraction
on top of doorbell-type mailboxes.

I got the concept. But are you also suggesting that in bindings it shmem
should be associated with mailbox controller rather than client ?

There are probably several ways of doing this better, we should see
what the best is we can come up with.

I think generally speaking we need a way for a mailbox user to
know what it should use as the mailbox data here, so it is
able to talk to different incompatible mailbox providers.

One idea I had is to use a nested mailbox driver, that turns
a doorbell or single-register styled mailbox into a variable-length
mailbox by adding a memory region, like

    mailbox at 1233000 {
        compatible = "vendor-hardware-specifc-id";
        interrupts = <34>;
        reg = <0x1233000 0x100>;
        #mbox-cells = <1>;
    };

    mailbox {
           compatible = "shmem-mailbox";
           mboxes = <&/mailbox@1233000  25>;
           #mbox-cells = <1>;
           shmem = <&cpu_scp_lpri &cpu_scp_hpri>;
    };

This would create one mailbox that only takes a register argument,
and another one that can take longer messages based on the first.
In your driver, you then refer to the second one and pass the
variable-length data into that directly.

1. IIUC it was intentional not to include shmem as part of mailbox
   controller binding and was pushed to client drivers as it's generally
   not part of mailbox IP block. I am not sure if there are any other
   specific reasons for that, but I may be missing some facts.

2. I am not sure if we need nested driver/bindings (at-least to begin
   with). On a platform I don't think both/all modes will be used.
   I had  proposal for adding doorbell for ARM MHU based on extended
   bindings, but it was rejected[1]. But I really preferred that over
   the shim layer I had to add in v3.

--
Regards,
Sudeep

[1] https://patchwork.kernel.org/patch/9745683/