On 02/06/17 10:32, Sudeep Holla wrote:
>
>
> 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.
>
I have posted the SCMI patches now[1], please let me know how to get
both SCPI and SCMI working together with different doorbell bits on the
same channel.
--
Regards,
Sudeep
[1] http://marc.info/?l=devicetree&m=149849482623492&w=2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
