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

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

[Sudeep Holla]

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@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 .../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.
+
 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
 - 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 subchannel with the channel when
+			compatible is "arm,mhu-v2"
 - 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 subchannels
+
 	mhu: mailbox@2b1f0000 {
 		#mbox-cells = <1>;
 		compatible = "arm,mhu", "arm,primecell";
@@ -41,3 +60,22 @@ used by Linux running NS.
 		reg = <0 0x2e000000 0x4000>;
 		mboxes = <&mhu 1>; /* HP-NonSecure */
 	};
+
+2. Controller which supports subchannels
+
+	mhu: mailbox@2b1f0000 {
+		#mbox-cells = <2>;
+		compatible = "arm,mhu-v2", "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 sub-channel */
+	};
-- 
2.7.4

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

[Rob Herring]

+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-5wv7dgnIgG8@public.gmane.org>
> Cc: Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> ---
>  .../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.

> +
>  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 ?

>  - 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 subchannel with the channel when
> +			compatible is "arm,mhu-v2"
>  - 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 subchannels
> +
>  	mhu: mailbox@2b1f0000 {
>  		#mbox-cells = <1>;
>  		compatible = "arm,mhu", "arm,primecell";
> @@ -41,3 +60,22 @@ used by Linux running NS.
>  		reg = <0 0x2e000000 0x4000>;
>  		mboxes = <&mhu 1>; /* HP-NonSecure */
>  	};
> +
> +2. Controller which supports subchannels
> +
> +	mhu: mailbox@2b1f0000 {
> +		#mbox-cells = <2>;
> +		compatible = "arm,mhu-v2", "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 sub-channel */
> +	};
> -- 
> 2.7.4
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jassi Brar]

On Mon, May 8, 2017 at 9:40 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 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-5wv7dgnIgG8@public.gmane.org>
>> Cc: Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
>> ---
>>  .../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.
  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.

>> +
>>  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.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 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-5wv7dgnIgG8@public.gmane.org>
>>> Cc: Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
>>> ---
>>>  .../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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bjorn Andersson]

On Mon 08 May 10:07 PDT 2017, Sudeep Holla wrote:

> 
> 
> On 08/05/17 17:46, Jassi Brar wrote:
> > On Mon, May 8, 2017 at 9:40 PM, Rob Herring <robh@kernel.org> 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@arm.com>
> >>> Cc: Jassi Brar <jaswinder.singh@linaro.org>
> >>> Cc: Rob Herring <robh+dt@kernel.org>
> >>> Cc: devicetree@vger.kernel.org
> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >>> ---
> >>>  .../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.

Each of the (used) bits in the Qualcomm HW are routed to a interrupt
controller in the remote processors.

As the APCS IPC is one way and each incoming "channel" is exposed as a
separate physical interrupt they are directly consumed by the higher
levels as needed - hence there's no traces of interrupts in the APCS
IPC.

> 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.
> 

In the Qualcomm platform I'm looking at right now 15 of the 32 bits are
used by the local CPU, so the utilization isn't awesome but I didn't
feel the waste was worth addressing in my case.

You should be able to provide a custom of_xlate that hides the fact that
your mailbox-space is sparse - i.e. only register the mailboxes you have
but expose them with ids as expected by the clients.

Regards,
Bjorn

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

[Sudeep Holla]

On 08/05/17 18:52, Bjorn Andersson wrote:
> On Mon 08 May 10:07 PDT 2017, Sudeep Holla wrote:
> 
>>
>>
>> On 08/05/17 17:46, Jassi Brar wrote:
>>> On Mon, May 8, 2017 at 9:40 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 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-5wv7dgnIgG8@public.gmane.org>
>>>>> Cc: Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>>> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
>>>>> ---
>>>>>  .../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.
> 
> Each of the (used) bits in the Qualcomm HW are routed to a interrupt
> controller in the remote processors.
> 
> As the APCS IPC is one way and each incoming "channel" is exposed as a
> separate physical interrupt they are directly consumed by the higher
> levels as needed - hence there's no traces of interrupts in the APCS
> IPC.
> 

Indeed, I followed the full thread later and came to same understanding.

>> 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.
>>
> 
> In the Qualcomm platform I'm looking at right now 15 of the 32 bits are
> used by the local CPU, so the utilization isn't awesome but I didn't
> feel the waste was worth addressing in my case.
> 

Make senses, but this controller in question was used on other platforms
too and hence I wanted to retain existing behavior intact without much
bloat of memory for channel allocation.

> You should be able to provide a custom of_xlate that hides the fact that
> your mailbox-space is sparse - i.e. only register the mailboxes you have
> but expose them with ids as expected by the clients.
> 

Spot on, that's exactly what I have done :)

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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-5wv7dgnIgG8@public.gmane.org> wrote:
>
>
> On 08/05/17 17:46, Jassi Brar wrote:
>> On Mon, May 8, 2017 at 9:40 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 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-5wv7dgnIgG8@public.gmane.org>
>>>> Cc: Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
>>>> ---
>>>>  .../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!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sudeep Holla]

On 09/05/17 03:50, Jassi Brar wrote:
> On Mon, May 8, 2017 at 10:37 PM, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
>>
>>
>> On 08/05/17 17:46, Jassi Brar wrote:
>>> On Mon, May 8, 2017 at 9:40 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 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-5wv7dgnIgG8@public.gmane.org>
>>>>> Cc: Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>>> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
>>>>> ---
>>>>>  .../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)
> 

OK, how will I get this info from the device tree ?

> 
>>>   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]
> 

How will the right client get the call from mbox_chan_received_data ?

>>>>> +
>>>>>  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
> 

Yes that's what I did for SCPI and it was all fine as long as we had
just one client. It simply can't scale with multiple client implementing
different protocols.

> 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.
> 

That's not at all the use case. I will never ever need that when used as
a doorbell bit.

> [[ BTW, today you may need only 4bits because you have only 4
> different commands. 

Let me clarify once again, they are not commands, they are just doorbell
bits and hence form a different channel as they will have associated
command set, response and the shared memory as part of their protocol.

> What if the command set grows beyond 32bits? The

Just not that use case here. They are just 32 bit doorbell and can be
used by 32 different protocol/clients. That's all. Nothing more nothing
less.

> 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. ]]
> 

FYI it will be totally different remote firmware/protocol and hence
channel even there.

E.g. on Juno SCPI protocol uses BIT(0) on one channel and a new protocol
SCMI uses say BIT(1) and BIT(2). It's not the same remote firmware protocol.

>> 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.
> 

Really ? What if it's generic protocol like SCPI or SCMI used with
different mailbox. What you are saying is to make it special with ARM
MHU ? Yuck !

>> 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.
> 

Just take example of SCPI in the mainline. Assume there's another
protocol SCMI which uses few more bits in the same channel and the
remote firmware implements both but both are totally independent and not
related/linked. Also be keep in mind that SCPI is used by other
platforms and so will be the new protocol. We simply make SCPI or SCMI
bindings aligned to ARM MHU. That's ruled out.

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jassi Brar]

On Tue, May 9, 2017 at 3:28 PM, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:

>>
>> If it is still not clear, please share your client driver. I will
>> adapt that to work with existing MHU driver & bindings.
>>
>
> Just take example of SCPI in the mainline. Assume there's another
> protocol SCMI which uses few more bits in the same channel and the
> remote firmware implements both but both are totally independent and not
> related/linked. Also be keep in mind that SCPI is used by other
> platforms and so will be the new protocol. We simply make SCPI or SCMI
> bindings aligned to ARM MHU. That's ruled out.
>
Not sure what you mean by "that's ruled out". Anyways, to be clear,
the bindings must remain same as long as the h/w doesn't change. It is
the client driver (DT node) that should be versioned for SCPI or SCMI
based on what the platform supports.

I have tried many ways to explain how to implement it and apparently
failed. So lets talk code.
You have already shared this "v2" MHU driver, now please also share
your client driver. I'll make it work with original MHU driver and
that should settle your confusion.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sudeep Holla]

On 09/05/17 11:31, Jassi Brar wrote:
> On Tue, May 9, 2017 at 3:28 PM, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> wrote:
> 
>>> 
>>> If it is still not clear, please share your client driver. I
>>> will adapt that to work with existing MHU driver & bindings.
>>> 
>> 
>> Just take example of SCPI in the mainline. Assume there's another 
>> protocol SCMI which uses few more bits in the same channel and the 
>> remote firmware implements both but both are totally independent
>> and not related/linked. Also be keep in mind that SCPI is used by
>> other platforms and so will be the new protocol. We simply make
>> SCPI or SCMI bindings aligned to ARM MHU. That's ruled out.
>> 
> Not sure what you mean by "that's ruled out".

1. The mailbox client bindings should be independent of this ARM MHU
   mailbox bindings
2. All we need in client is a mailbox to point at and not any meta data
   That's what I meant by ruled-out as both client and MHU can be used
   independent of each other and *should not* be linked.

> Anyways, to be clear, the bindings must remain same as long as the
> h/w doesn't change.

While I would generally agree with that, but since the existing binding
didn't consider the possibility of using it as sub-channels, I disagree
with your opinion now.

> It is the client driver (DT node) that should be versioned for SCPI
> or SCMI based on what the platform supports.

Why ? All that client care is about doorbell.

> I have tried many ways to explain how to implement it and apparently 
> failed. So lets talk code.

OK

> You have already shared this "v2" MHU driver, now please also share 
> your client driver. I'll make it work with original MHU driver and 
> that should settle your confusion.

It should first work with SCPI in the mainline. Then we will add another
similar protocol soon. So I think you have all you need in the mainline.
Today we have hack in the SCPI driver to pass bit 0 set in data. But
that's broken as we may want different slot on some other platform.
Basically SCPI is designed with the use of doorbell and it should not
have any details on how to write that into a particular register as
along as we just choose the right channel.

On digging more about different mailbox controllers, I found
mailbox-sti.c has exactly similar logic as what I have done in this series.

Also don't mix implementation with the binding. I need a simple answer
in this binding. How do I represent specific bits if each bit is
implemented as a doorbell ? That's all. First let's agree on that when
we use this mailbox independently and please *don't mix* with any
client here. It's simple, this controller has 2-3 sets of 32 doorbell
bits. And I am aiming to come up with the binding for that as your
initial bindings didn't consider that.

-- 
Regards,
Sudeep

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jassi Brar]

On Tue, May 9, 2017 at 4:23 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 09/05/17 11:31, Jassi Brar wrote:
>> On Tue, May 9, 2017 at 3:28 PM, Sudeep Holla <sudeep.holla@arm.com>
>> wrote:
>>
>>>>
>>>> If it is still not clear, please share your client driver. I
>>>> will adapt that to work with existing MHU driver & bindings.
>>>>
>>>
>>> Just take example of SCPI in the mainline. Assume there's another
>>> protocol SCMI which uses few more bits in the same channel and the
>>> remote firmware implements both but both are totally independent
>>> and not related/linked. Also be keep in mind that SCPI is used by
>>> other platforms and so will be the new protocol. We simply make
>>> SCPI or SCMI bindings aligned to ARM MHU. That's ruled out.
>>>
>> Not sure what you mean by "that's ruled out".
>
> 1. The mailbox client bindings should be independent of this ARM MHU
>    mailbox bindings
> 2. All we need in client is a mailbox to point at and not any meta data
>    That's what I meant by ruled-out as both client and MHU can be used
>    independent of each other and *should not* be linked.
>
I am shocked at this coming from you.

You design SCMI based upon MHU assumption of single bit "doorbell" and
then you say a client should be independent of the underlying
controller? Do you intend SCMI to work only over MHU?

 What if some controller does not support the simple "doorbell" and
expects detailed info? For example, apart from SCMI, the remote also
supports platform specific functions like thermal, watchdog, wakeup
etc. The SCMI's would just be a subset of the full command set.
You/SCMI can not dictate what numerical value the platform assigns to
SCMI commands... all that is required is the remote firmware should
uniquely identify which command is it and implement what the SCMI
protocol expects.

Have a look at  mbox_send_message(struct mbox_chan *chan, void *mssg)
The 'mssg' is the pointer to _controller-specified_ structure. The
client driver (SCMI) _must_ know what the underlying controller
expects and pass that info. For example of 'mssg', look at "struct
brcm_message" in include/linux/mailbox/brcm-message.h   The client
driver must use that platform specific knowledge to send a message
i.e, you can not make a mailbox client driver 100% provider agnostic.

You need to divide SCMI into two parts - one that manages the SCMI
protocol and the other platform specific glue that talks to the
mailbox controller over the mailbox api.


>> You have already shared this "v2" MHU driver, now please also share
>> your client driver. I'll make it work with original MHU driver and
>> that should settle your confusion.
>
> It should first work with SCPI in the mainline. Then we will add another
> similar protocol soon. So I think you have all you need in the mainline.
> Today we have hack in the SCPI driver to pass bit 0 set in data. But
> that's broken as we may want different slot on some other platform.
> Basically SCPI is designed with the use of doorbell and it should not
> have any details on how to write that into a particular register as
> along as we just choose the right channel.
>
> On digging more about different mailbox controllers, I found
> mailbox-sti.c has exactly similar logic as what I have done in this series.
>
> Also don't mix implementation with the binding. I need a simple answer
> in this binding. How do I represent specific bits if each bit is
> implemented as a doorbell ? That's all. First let's agree on that when
> we use this mailbox independently and please *don't mix* with any
> client here. It's simple, this controller has 2-3 sets of 32 doorbell
> bits. And I am aiming to come up with the binding for that as your
> initial bindings didn't consider that.
>
Please send in whatever changes you plan to do, and I'll modify it so
we don't have to bloat the MHU driver and add bindings for a software
feature. Until then ... Cheers!

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

[Sudeep Holla]

On 09/05/17 12:55, Jassi Brar wrote:
> On Tue, May 9, 2017 at 4:23 PM, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
>>
>>
>> On 09/05/17 11:31, Jassi Brar wrote:
>>> On Tue, May 9, 2017 at 3:28 PM, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
>>> wrote:
>>>
>>>>>
>>>>> If it is still not clear, please share your client driver. I
>>>>> will adapt that to work with existing MHU driver & bindings.
>>>>>
>>>>
>>>> Just take example of SCPI in the mainline. Assume there's another
>>>> protocol SCMI which uses few more bits in the same channel and the
>>>> remote firmware implements both but both are totally independent
>>>> and not related/linked. Also be keep in mind that SCPI is used by
>>>> other platforms and so will be the new protocol. We simply make
>>>> SCPI or SCMI bindings aligned to ARM MHU. That's ruled out.
>>>>
>>> Not sure what you mean by "that's ruled out".
>>
>> 1. The mailbox client bindings should be independent of this ARM MHU
>>    mailbox bindings
>> 2. All we need in client is a mailbox to point at and not any meta data
>>    That's what I meant by ruled-out as both client and MHU can be used
>>    independent of each other and *should not* be linked.
>>
> I am shocked at this coming from you.
> 
> You design SCMI based upon MHU assumption of single bit "doorbell" and
> then you say a client should be independent of the underlying
> controller? Do you intend SCMI to work only over MHU?
> 

No, I never said that. What I said is SCMI protocol will be on doorbell
based.

>  What if some controller does not support the simple "doorbell" and
> expects detailed info? For example, apart from SCMI, the remote also
> supports platform specific functions like thermal, watchdog, wakeup
> etc. The SCMI's would just be a subset of the full command set.
> You/SCMI can not dictate what numerical value the platform assigns to
> SCMI commands... 

What ? That's the whole point of specification. The command set is
*fixed* and can be implemented on any platform and have generic driver
for that.

> all that is required is the remote firmware should
> uniquely identify which command is it and implement what the SCMI
> protocol expects.
> 

Yes, not just uniquely but exactly as the specification.

> Have a look at  mbox_send_message(struct mbox_chan *chan, void *mssg)
> The 'mssg' is the pointer to _controller-specified_ structure. The
> client driver (SCMI) _must_ know what the underlying controller
> expects and pass that info. For example of 'mssg', look at "struct
> brcm_message" in include/linux/mailbox/brcm-message.h   The client
> driver must use that platform specific knowledge to send a message
> i.e, you can not make a mailbox client driver 100% provider agnostic.
> 

I disagree. The whole idea is to make it generic with doorbell kind of
logic. We don't pass any command in the doorbell register, everything is
part of shared memory and all standardized.

> You need to divide SCMI into two parts - one that manages the SCMI
> protocol and the other platform specific glue that talks to the
> mailbox controller over the mailbox api.
> 

Why ? That's totally unnecessary IMO.

> 
>>> You have already shared this "v2" MHU driver, now please also share
>>> your client driver. I'll make it work with original MHU driver and
>>> that should settle your confusion.
>>
>> It should first work with SCPI in the mainline. Then we will add another
>> similar protocol soon. So I think you have all you need in the mainline.
>> Today we have hack in the SCPI driver to pass bit 0 set in data. But
>> that's broken as we may want different slot on some other platform.
>> Basically SCPI is designed with the use of doorbell and it should not
>> have any details on how to write that into a particular register as
>> along as we just choose the right channel.
>>

Check arm_scpi.c, the aim is to eliminate the need to send the data with
BIT(0) set as that's platform specific and the protocol has to be generic.

>> On digging more about different mailbox controllers, I found
>> mailbox-sti.c has exactly similar logic as what I have done in this series.
>>

Did you look at this driver ?

>> Also don't mix implementation with the binding. I need a simple answer
>> in this binding. How do I represent specific bits if each bit is
>> implemented as a doorbell ? That's all. First let's agree on that when
>> we use this mailbox independently and please *don't mix* with any
>> client here. It's simple, this controller has 2-3 sets of 32 doorbell
>> bits. And I am aiming to come up with the binding for that as your
>> initial bindings didn't consider that.
>>
> Please send in whatever changes you plan to do, and I'll modify it so
> we don't have to bloat the MHU driver and add bindings for a software
> feature. Until then ... Cheers!
> 

Changes to what ? arm_mhu.c ? This series is complete and implements
doorbell completely.

You seem to have missed to answer some of my questions above. Please do.

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jassi Brar]

On Tue, May 9, 2017 at 6:11 PM, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> wrote:
> On 09/05/17 12:55, Jassi Brar wrote:

>>>>>>
>>>>>> If it is still not clear, please share your client driver. I
>>>>>> will adapt that to work with existing MHU driver & bindings.
>>>>>>
>>>>>
>>>>> Just take example of SCPI in the mainline. Assume there's another
>>>>> protocol SCMI which uses few more bits in the same channel and the
>>>>> remote firmware implements both but both are totally independent
>>>>> and not related/linked. Also be keep in mind that SCPI is used by
>>>>> other platforms and so will be the new protocol. We simply make
>>>>> SCPI or SCMI bindings aligned to ARM MHU. That's ruled out.
>>>>>
>>>> Not sure what you mean by "that's ruled out".
>>>
>>> 1. The mailbox client bindings should be independent of this ARM MHU
>>>    mailbox bindings
>>> 2. All we need in client is a mailbox to point at and not any meta data
>>>    That's what I meant by ruled-out as both client and MHU can be used
>>>    independent of each other and *should not* be linked.
>>>
>> I am shocked at this coming from you.
>>
>> You design SCMI based upon MHU assumption of single bit "doorbell" and
>> then you say a client should be independent of the underlying
>> controller? Do you intend SCMI to work only over MHU?
>>
>
> No, I never said that. What I said is SCMI protocol will be on doorbell
> based.
>
What if a controller does not support your definition of "doorbell"?
Like PL320 from ARM and many others.


>>  What if some controller does not support the simple "doorbell" and
>> expects detailed info? For example, apart from SCMI, the remote also
>> supports platform specific functions like thermal, watchdog, wakeup
>> etc. The SCMI's would just be a subset of the full command set.
>> You/SCMI can not dictate what numerical value the platform assigns to
>> SCMI commands...
>
> What ? That's the whole point of specification. The command set is
> *fixed* and can be implemented on any platform and have generic driver
> for that.
>
The code/value for commands in SHM data packet is SCMI specific. But
what a platform assigns to THIS_IS_SCMI_DOORBELL is going to be
platform specific i.e, not always BIT(x)


>>> On digging more about different mailbox controllers, I found
>>> mailbox-sti.c has exactly similar logic as what I have done in this series.
>>>
>
> Did you look at this driver ?
>
Dude, I merged this driver upstream! I don't remember exactly about
STI controller, but it definitely is different from MHU.


>>> Also don't mix implementation with the binding. I need a simple answer
>>> in this binding. How do I represent specific bits if each bit is
>>> implemented as a doorbell ? That's all. First let's agree on that when
>>> we use this mailbox independently and please *don't mix* with any
>>> client here. It's simple, this controller has 2-3 sets of 32 doorbell
>>> bits. And I am aiming to come up with the binding for that as your
>>> initial bindings didn't consider that.
>>>
>> Please send in whatever changes you plan to do, and I'll modify it so
>> we don't have to bloat the MHU driver and add bindings for a software
>> feature. Until then ... Cheers!
>>
>
> Changes to what ? arm_mhu.c ? This series is complete and implements
> doorbell completely.
>
Send in the user/client driver that you think can not work with
existing driver/bindings.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sudeep Holla]

On 09/05/17 14:29, Jassi Brar wrote:
> On Tue, May 9, 2017 at 6:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On 09/05/17 12:55, Jassi Brar wrote:
> 
>>>>>>>
>>>>>>> If it is still not clear, please share your client driver. I
>>>>>>> will adapt that to work with existing MHU driver & bindings.
>>>>>>>
>>>>>>
>>>>>> Just take example of SCPI in the mainline. Assume there's another
>>>>>> protocol SCMI which uses few more bits in the same channel and the
>>>>>> remote firmware implements both but both are totally independent
>>>>>> and not related/linked. Also be keep in mind that SCPI is used by
>>>>>> other platforms and so will be the new protocol. We simply make
>>>>>> SCPI or SCMI bindings aligned to ARM MHU. That's ruled out.
>>>>>>
>>>>> Not sure what you mean by "that's ruled out".
>>>>
>>>> 1. The mailbox client bindings should be independent of this ARM MHU
>>>>    mailbox bindings
>>>> 2. All we need in client is a mailbox to point at and not any meta data
>>>>    That's what I meant by ruled-out as both client and MHU can be used
>>>>    independent of each other and *should not* be linked.
>>>>
>>> I am shocked at this coming from you.
>>>
>>> You design SCMI based upon MHU assumption of single bit "doorbell" and
>>> then you say a client should be independent of the underlying
>>> controller? Do you intend SCMI to work only over MHU?
>>>
>>
>> No, I never said that. What I said is SCMI protocol will be on doorbell
>> based.
>>
> What if a controller does not support your definition of "doorbell"?
> Like PL320 from ARM and many others.
> 

OK, why are we discussing that here ?

>>>  What if some controller does not support the simple "doorbell" and
>>> expects detailed info? For example, apart from SCMI, the remote also
>>> supports platform specific functions like thermal, watchdog, wakeup
>>> etc. The SCMI's would just be a subset of the full command set.
>>> You/SCMI can not dictate what numerical value the platform assigns to
>>> SCMI commands...
>>
>> What ? That's the whole point of specification. The command set is
>> *fixed* and can be implemented on any platform and have generic driver
>> for that.
>>
> The code/value for commands in SHM data packet is SCMI specific. But
> what a platform assigns to THIS_IS_SCMI_DOORBELL is going to be
> platform specific i.e, not always BIT(x)
> 

Platform which uses this as single bit doorbell has to just choose the
tuple(bit and the register set) as shown in the example binding

>>>> On digging more about different mailbox controllers, I found
>>>> mailbox-sti.c has exactly similar logic as what I have done in this series.
>>>>
>>
>> Did you look at this driver ?
>>
> Dude, I merged this driver upstream! I don't remember exactly about
> STI controller, but it definitely is different from MHU.
> 

Yes I can know and can see you have upstreamed the driver. I have spoken
to the ARM MHU hardware IP designers and I know what it's designed for.
And that's why I gave you example to look at STI driver
to help you understand what I am trying to say faster.

>>>> Also don't mix implementation with the binding. I need a simple answer
>>>> in this binding. How do I represent specific bits if each bit is
>>>> implemented as a doorbell ? That's all. First let's agree on that when
>>>> we use this mailbox independently and please *don't mix* with any
>>>> client here. It's simple, this controller has 2-3 sets of 32 doorbell
>>>> bits. And I am aiming to come up with the binding for that as your
>>>> initial bindings didn't consider that.
>>>>
>>> Please send in whatever changes you plan to do, and I'll modify it so
>>> we don't have to bloat the MHU driver and add bindings for a software
>>> feature. Until then ... Cheers!
>>>
>>
>> Changes to what ? arm_mhu.c ? This series is complete and implements
>> doorbell completely.
>>
> Send in the user/client driver that you think can not work with
> existing driver/bindings.
> 

Again for the 3rd time see arm_scpi.c
ARM is now generalizing it with multiple vendors under the new name ARM
SCMI. And Juno is implementing using few doorbell bits on the same
channel as SCPI.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On 08/05/17 17:10, Rob Herring 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-5wv7dgnIgG8@public.gmane.org>
>> Cc: Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
>> ---
>>  .../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.
> 

Indeed single bit doorbell, but 32 of them joined together.
OK, I will have a look at that Bjorn series.

>> +
>>  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 ?
> 

No, it's the same IP, just that initial binding was pushed not to
support the single bit doorbell functionality of the IP. Jassi was/is
against that. I have just quoted the specification above.

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html