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

From: Sudeep Holla
Date: Fri Jun 02 2017 - 05:33:05 EST



On 02/06/17 06:45, Jassi Brar wrote:
> Hi Rob,
>
> On Wed, May 31, 2017 at 10:38 PM, Rob Herring <robh@xxxxxxxxxx> 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