Re: [PATCH v2 2/6] Documentation: devicetree: add bindings to support ARM MHU doorbells

[Sudeep Holla <sudeep.holla@arm.com>]

On 02/06/17 06:45, Jassi Brar wrote:
> Hi Rob,
> 
> On Wed, May 31, 2017 at 10:38 PM, Rob Herring <robh@kernel.org> wrote:
>> On Thu, May 25, 2017 at 02:23:44PM +0100, Sudeep Holla wrote:
>>>
>>>>>  .../devicetree/bindings/mailbox/arm-mhu.txt        | 46 ++++++++++++++++++++--
>>>>>  1 file changed, 43 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>>>>> index 4971f03f0b33..bd9a3a267caf 100644
>>>>> --- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>>>>> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>>>>> @@ -10,21 +10,42 @@ STAT register and the remote clears it after having read the data.
>>>>>  The last channel is specified to be a 'Secure' resource, hence can't be
>>>>>  used by Linux running NS.
>>>>>
>>>>> +The MHU drives the interrupt signal using a 32-bit register, with all
>>>>> +32-bits logically ORed together. It provides a set of registers to
>>>>> +enable software to set, clear and check the status of each of the bits
>>>>> +of this register independently. The use of 32 bits per interrupt line
>>>>> +enables software to provide more information about the source of the
>>>>> +interrupt. For example, each bit of the register can be associated with
>>>>> +a type of event that can contribute to raising the interrupt. Each of
>>>>> +the 32-bits can be used as "doorbell" to alert the remote processor.
>>>>> +
>>>>>  Mailbox Device Node:
>>>>>  ====================
>>>>>
>>>>>  Required properties:
>>>>>  --------------------
>>>>> -- compatible:          Shall be "arm,mhu" & "arm,primecell"
>>>>> +- compatible:          Shall be "arm,primecell" and one of the below:
>>>>> +                       "arm,mhu" - if the controller doesn't support
>>>>> +                                   doorbell model
>>>>> +                       "arm,mhu-doorbell" - if the controller supports
>>>>> +                                   doorbell model
>>>>>  - reg:                 Contains the mailbox register address range (base
>>>>>                         address and length)
>>>>> -- #mbox-cells          Shall be 1 - the index of the channel needed.
>>>>> +- #mbox-cells          Shall be 1 - the index of the channel needed when
>>>>> +                       compatible is "arm,mhu"
>>>>> +                       Shall be 2 - the index of the channel needed, and
>>>>> +                       the index of the doorbell bit with the channel when
>>>>> +                       compatible is "arm,mhu-doorbell"
>>>>>  - interrupts:          Contains the interrupt information corresponding to
>>>>> -                       each of the 3 links of MHU.
>>>>> +                       each of the 3 physical channels of MHU namely low
>>>>> +                       priority non-secure, high priority non-secure and
>>>>> +                       secure channels.
>>>>>
>>>>>  Example:
>>>>>  --------
>>>>>
>>>>> +1. Controller which doesn't support doorbells
>>>>> +
>>>>>         mhu: mailbox@2b1f0000 {
>>>>>                 #mbox-cells = <1>;
>>>>>                 compatible = "arm,mhu", "arm,primecell";
>>>>> @@ -41,3 +62,22 @@ used by Linux running NS.
>>>>>                 reg = <0 0x2e000000 0x4000>;
>>>>>                 mboxes = <&mhu 1>; /* HP-NonSecure */
>>>>>         };
>>>>> +
>>>>> +2. Controller which supports doorbells
>>>>> +
>>>>> +       mhu: mailbox@2b1f0000 {
>>>>> +               #mbox-cells = <2>;
>>>>> +               compatible = "arm,mhu-doorbell", "arm,primecell";
>>>>> +               reg = <0 0x2b1f0000 0x1000>;
>>>>> +               interrupts = <0 36 4>, /* LP-NonSecure */
>>>>> +                            <0 35 4>, /* HP-NonSecure */
>>>>> +                            <0 37 4>; /* Secure */
>>>>> +               clocks = <&clock 0 2 1>;
>>>>> +               clock-names = "apb_pclk";
>>>>> +       };
>>>>> +
>>>>> +       mhu_client: scb@2e000000 {
>>>>> +               compatible = "arm,scpi";
>>>>> +               reg = <0 0x2e000000 0x200>;
>>>>> +               mboxes = <&mhu 1 4>; /* HP-NonSecure 5th doorbell bit */
>>>>> +       };
>>>>>
>>>> Every MHU controller can by driven as "arm,mhu-doorbell" or "arm,mhu"
>>>> equally fine. So you are basically smuggling a s/w feature into DT.
>>>>
>>>
>>> I disagree, the spec clearly says each bit can be used for different
>>> event and hence we need a way to specify that in DT when used as doorbell.
>>
>> I think the point is that you should not continue to use both. The
>>
> FYKI my point (here and in other threads) is that current MHU
> driver/bindings can very well support Sudeep's usecase.
> 
>> single cell usage should be deprecated. Maybe you'll have to encode the
>> 2nd cell when not used as 0 means bit 0?
>>
>> Arguably, you don't even need a new compatible. #mbox-cells tells you
>> whether to use the old or new binding. I'm fine either way though.
>>
> In mailbox terminology, there is a channel and command(s) that we
> send/recv over it. OOB data passing is optional.
> 
> MHU driver accepts a command as a u32. Sudeep's usecase has commands
> of the form of BIT(x)  ... an instance of u32.
> 

Again read the spec, it is designed to be used as doorbell

> Instead of having his client driver pass BIT(x) like other MHU

Client is generic and doesn't understand which controller it's working
with. It's not custom protocol. I have given enough links and details to
say it's generic protocol adapted by other vendors.

> clients, he wants to modify the driver and bindings to specify 'x' via
> second cell of mboxes.  That would have made sense (and already
> implemented) if the controller could send only 1 bit at a time and
> every client/protocol used exactly 1 command - both of which are not
> true.

Yes but the controller is designed to provide single bit doorbell
capability. Please understand that instead of pushing how you think it
needs to work.

> 
> I even offered Sudeep to share his client driver and I'll adapt it for
> him, but her refuses to either see my point or share his code.
> 

You have not answered my queries, you keep telling I can help but never
read and answer what I ask. Also I have told you the driver is in
mainline(drivers/firmware/arm_scpi.c) and there will be another similar
one. And theres are used by different platforms with different mailbox
controllers.
Please post patches to make 2 different protocols like SCPI work. Please
note these protocols are used by other platforms which have different
controllers.

-- 
Regards,
Sudeep