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

[Sudeep Holla]

On 08/05/17 17:46, Jassi Brar wrote:
> On Mon, May 8, 2017 at 9:40 PM, Rob Herring <robh@xxxxxxxxxx> wrote:
>> +Bjorn
>>
>> On Tue, May 02, 2017 at 02:55:49PM +0100, Sudeep Holla wrote:
>>> The ARM MHU has mechanism to assert interrupt signals to facilitate
>>> inter-processor message based communication. It drives the signal using
>>> a 32-bit register, with all 32-bits logically ORed together. It also
>>> enables software to set, clear and check the status of each of the bits
>>> of this register independently. Each bit of the register can be
>>> associated with a type of event that can contribute to raising the
>>> interrupt thereby allowing it to be used as independent subchannels.
>>>
>>> Since the first version of this binding can't support sub-channels,
>>> this patch extends the existing binding to support them.
>>>
>>> Cc: Alexey Klimov <alexey.klimov@xxxxxxx>
>>> Cc: Jassi Brar <jaswinder.singh@xxxxxxxxxx>
>>> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
>>> Cc: devicetree@xxxxxxxxxxxxxxx
>>> Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
>>> ---
>>>  .../devicetree/bindings/mailbox/arm-mhu.txt        | 44 ++++++++++++++++++++--
>>>  1 file changed, 41 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>>> index 4971f03f0b33..86a66f7918e2 100644
>>> --- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>>> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>>> @@ -10,21 +10,40 @@ 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.
>>
>> Sounds like a doorbell? (i.e. a single bit mailbox). Bjorn is doing
>> something similar for QCom h/w. I guess the difference here is you have
>> 32 sources and 1 output. It seems to me these should be described
>> similarly.
>>
> Yes, QCom controller triggers different interrupt for each bit of a
> 32bits register i.e, each signal is associated with 1bit information.
> Whereas MHU signals 32bits at a time to the target cpu.

Agreed. I had a look at Qcom driver, not entirely clear if each bit as
interrupt as I don't see any interrupt support there. Also, it just adds
all the 32 channels which I am trying to avoid as at-most 4-5 will be
used while we end up creating 64 channels.

>   Both these cases are already supported by mailbox framework, so
> Bjorn has implemented QCom's 'doorbell' driver over mailbox api. And
> we can do without this "arm,mhu-v2" driver. I believe Sudeep already
> knows well how to use the MHU driver as such to get what his client
> drivers need.
> 

As I mentioned above one reason for adding the complexity is avoiding
creation of 32*2 channels. Secondly we still need a way to distinguish
between the 2 use-cases(existing and new one). Any thoughts ?

>>> +
>>>  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
>>> +                                 subchannels
>>> +                     "arm,mhu-v2" - if the controller supports subchannels
>>
>> How do I know if I have v2? This correlates to an IP version or
>> IP configuration or ?
>>
> This is purely a software concept - virtual channel. There are only 3
> physical channels and that are managed by existing version of driver &
> bindings. This is another reason I am against this patchset.
> 

I understand your concern. Please suggest alternative if we need to use
each bit in the single set register as a different doorbell ? We need
this from DT as we need to specify each bit as a channel for different
client. Let me know how would you like me to proceed to deal with such
a scenario. The specification clearly states each bit can be used as a
doorbell.

-- 
Regards,
Sudeep