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

[Jassi Brar]

On Mon, May 8, 2017 at 10:37 PM, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
>
>
> 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.
>
OK, so you just need to use 4 singles bits. That is, 4 different
commands to remote.

#define SCMI_CMD_1   BIT(a)
#define SCMI_CMD_2   BIT(b)
#define SCMI_CMD_3   BIT(c)
#define SCMI_CMD_4   BIT(d)


>>   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 ?
>
I say, your usecase is an instance of the supported usecases by the
existing driver.
Just send the BIT(x) as a 32-bit value. Remote doesn't even need to
find which bit is set, it can simply switch-case the value it got
against SCMI_CMD_[1,4]

>>>> +
>>>>  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 ?
>
Your commands are encoded as a simple BIT(x) which is an instance of u32

If you want to send 2 commands together to remote, even that is just
as simple... send BIT(a) | BIT(b).
The remote will figure which bits are set and take action priority wise.

[[ BTW, today you may need only 4bits because you have only 4
different commands. What if the command set grows beyond 32bits? The
perfectly capable h/w will be rendered useless just because of s/w
design decisions. So instead of assigning BIT(x) type command codes,
please consider using full range of u32. Remote does not need it to be
BIT(x) but just a unique number.  The same "doorbell" will ring and
the remote will use switch-case to figure who is it. ]]

> We need
> this from DT as we need to specify each bit as a channel for different
> client.
>
The client DT node could carry the command code (as a u32 value) that
it is going to work with.

>The specification clearly states each bit can be used as a
> doorbell.
>
Doorbell and Mailbox are the same thing.
 Its just that we are used to pass data packets over shared-memory
that we think mailbox is different. It is perfectly possible to not
need shared-memory, if command+data can be encoded within 32bits. In
that case you would call Mailbox a 32bit Doorbell :)   For example,
PL320 has 8 32bit registers that can carry data for remote.

If it is still not clear, please share your client driver. I will
adapt that to work with existing MHU driver & bindings.

Cheers!