linux-kernel - Re: [PATCH v3 02/22] dt-bindings: arm: add support for ARM System Control and Management Interface(SCMI) protocol

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 4 Oct 2017 15:47:55 +0100
From:   Sudeep Holla <sudeep.holla@....com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Sudeep Holla <sudeep.holla@....com>,
        ALKML <linux-arm-kernel@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        DTML <devicetree@...r.kernel.org>,
        Roy Franz <roy.franz@...ium.com>,
        Harb Abdulhamid <harba@...eaurora.org>,
        Nishanth Menon <nm@...com>, Loc Ho <lho@....com>,
        Alexey Klimov <alexey.klimov@....com>,
        Ryan Harkin <Ryan.Harkin@....com>,
        Jassi Brar <jassisinghbrar@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v3 02/22] dt-bindings: arm: add support for ARM System
 Control and Management Interface(SCMI) protocol



On 04/10/17 15:17, Arnd Bergmann wrote:
> On Wed, Oct 4, 2017 at 3:53 PM, Sudeep Holla <sudeep.holla@....com> wrote:
>> On 04/10/17 13:35, Arnd Bergmann wrote:
>>> On Wed, Oct 4, 2017 at 1:07 PM, Sudeep Holla <sudeep.holla@....com> wrote:
> 
>>>>
>>>> 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
>>> 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.
> 
> 
>>> 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.
>>
>>> 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.
>>
>>> 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@...3000 {
>         compatible = "vendor-hardware-specifc-id";
>         interrupts = <34>;
>         reg = <0x1233000 0x100>;
>         #mbox-cells = <1>;
>     };
> 
>     mailbox {
>            compatible = "shmem-mailbox";
>            mboxes = <&/mailbox@...3000  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/