[PATCH 0/6] mailbox: arm_mhu: add support for subchannels

[PATCH 0/6] mailbox: arm_mhu: add support for subchannels

[Sudeep Holla]

Hi Jassi,

This series adds subchannel support to ARM MHU mailbox controller
driver. Since SCPI never used second slot, we were able to use the
existing driver as is. However, that's changing soon and the new
SCMI protocol under development needs subchannel support. If you
recall when you initially added this driver, I was pushing for some
of these changes like threaded irq. This patch series adds support
for the subchannels on ARM MHU controllers.

Regards,
Sudeep

Sudeep Holla (6):
  mailbox: arm_mhu: reorder header inclusion and drop unneeded ones
  Documentation: devicetree: add bindings to support ARM MHU subchannels
  mailbox: arm_mhu: migrate to threaded irq handler
  mailbox: arm_mhu: re-factor data structure to add subchannel support
  mailbox: arm_mhu: add full support for sub-channels
  mailbox: arm_mhu: add name support to record mbox-name

 .../devicetree/bindings/mailbox/arm-mhu.txt        |  44 ++-
 drivers/mailbox/arm_mhu.c                          | 381 ++++++++++++++++++---
 2 files changed, 376 insertions(+), 49 deletions(-)

-- 
2.7.4

[PATCH 1/6] mailbox: arm_mhu: reorder header inclusion and drop unneeded ones

[Sudeep Holla]

This patch just re-orders some of the headers includes and also drop
the ones that are unnecessary.

Cc: Alexey Klimov <alexey.klimov@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/arm_mhu.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index 99befa76e37c..be0f293a9457 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -13,16 +13,13 @@
  * GNU General Public License for more details.
  */
 
-#include <linux/interrupt.h>
-#include <linux/spinlock.h>
-#include <linux/mutex.h>
-#include <linux/delay.h>
-#include <linux/slab.h>
+#include <linux/amba/bus.h>
+#include <linux/device.h>
 #include <linux/err.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
-#include <linux/module.h>
-#include <linux/amba/bus.h>
 #include <linux/mailbox_controller.h>
+#include <linux/module.h>
 
 #define INTR_STAT_OFS	0x0
 #define INTR_SET_OFS	0x8
-- 
2.7.4

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

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

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

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

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

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

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@arm.com> 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. 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!

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@arm.com> 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. 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

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

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

> 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

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

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

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


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

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

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

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

[PATCH 3/6] mailbox: arm_mhu: migrate to threaded irq handler

[Sudeep Holla]

In preparation to introduce support for subchannels which require the
interrupt handlers to be threaded, this patch moves the existing
interrupt handler to threaded handler.

Also it moves out the registering and freeing of the handlers from
the mailbox startup and shutdown methods. This also is required to
support sub-channels.

Cc: Alexey Klimov <alexey.klimov@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/arm_mhu.c | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index be0f293a9457..ebe17c097f43 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -84,27 +84,15 @@ static int mhu_startup(struct mbox_chan *chan)
 {
 	struct mhu_link *mlink = chan->con_priv;
 	u32 val;
-	int ret;
 
 	val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
 	writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);
 
-	ret = request_irq(mlink->irq, mhu_rx_interrupt,
-			  IRQF_SHARED, "mhu_link", chan);
-	if (ret) {
-		dev_err(chan->mbox->dev,
-			"Unable to acquire IRQ %d\n", mlink->irq);
-		return ret;
-	}
-
 	return 0;
 }
 
 static void mhu_shutdown(struct mbox_chan *chan)
 {
-	struct mhu_link *mlink = chan->con_priv;
-
-	free_irq(mlink->irq, chan);
 }
 
 static const struct mbox_chan_ops mhu_ops = {
@@ -132,13 +120,6 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		return PTR_ERR(mhu->base);
 	}
 
-	for (i = 0; i < MHU_CHANS; i++) {
-		mhu->chan[i].con_priv = &mhu->mlink[i];
-		mhu->mlink[i].irq = adev->irq[i];
-		mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
-		mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
-	}
-
 	mhu->mbox.dev = dev;
 	mhu->mbox.chans = &mhu->chan[0];
 	mhu->mbox.num_chans = MHU_CHANS;
@@ -155,6 +136,28 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		return err;
 	}
 
+	for (i = 0; i < MHU_CHANS; i++) {
+		int irq = mhu->mlink[i].irq = adev->irq[i];
+
+		if (irq <= 0) {
+			dev_dbg(dev, "No IRQ found for Channel %d\n", i);
+			continue;
+		}
+
+		mhu->chan[i].con_priv = &mhu->mlink[i];
+		mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
+		mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
+
+		err = devm_request_threaded_irq(dev, irq, NULL,
+						mhu_rx_interrupt, IRQF_ONESHOT,
+						"mhu_link", &mhu->chan[i]);
+		if (err) {
+			dev_err(dev, "Can't claim IRQ %d\n", irq);
+			mbox_controller_unregister(&mhu->mbox);
+			return err;
+		}
+	}
+
 	dev_info(dev, "ARM MHU Mailbox registered\n");
 	return 0;
 }
-- 
2.7.4

[PATCH 4/6] mailbox: arm_mhu: re-factor data structure to add subchannel support

[Sudeep Holla]

In order to support subchannels, we need a bit of reword around data
structures that are per-channel. Since the number of subchannels are
not fixed though restricted to maximum of 20, the channel assignment
and initialization is move to xlate function.

This patch also adds the platform data for the existing support of one
channel per physical channel.

Cc: Alexey Klimov <alexey.klimov@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/arm_mhu.c | 208 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 186 insertions(+), 22 deletions(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index ebe17c097f43..4e0f690b97fd 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -20,6 +20,8 @@
 #include <linux/io.h>
 #include <linux/mailbox_controller.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 #define INTR_STAT_OFS	0x0
 #define INTR_SET_OFS	0x8
@@ -30,7 +32,8 @@
 #define MHU_SEC_OFFSET	0x200
 #define TX_REG_OFFSET	0x100
 
-#define MHU_CHANS	3
+#define MHU_NUM_PCHANS	3	/* Secure, Non-Secure High and Low Priority */
+#define MHU_CHAN_MAX	20	/* Max channels to save on unused RAM */
 
 struct mhu_link {
 	unsigned irq;
@@ -40,53 +43,175 @@ struct mhu_link {
 
 struct arm_mhu {
 	void __iomem *base;
-	struct mhu_link mlink[MHU_CHANS];
-	struct mbox_chan chan[MHU_CHANS];
+	struct mhu_link mlink[MHU_NUM_PCHANS];
 	struct mbox_controller mbox;
+	struct device *dev;
 };
 
+/**
+ * ARM MHU Mailbox platform specific configuration
+ *
+ * @num_pchans: Maximum number of physical channels
+ * @num_subchans: Maximum number of sub-channels per physical channel
+ */
+struct mhu_mbox_pdata {
+	unsigned int num_pchans;
+	unsigned int num_subchans;
+	bool support_subchans;
+};
+
+/**
+ * ARM MHU Mailbox allocated channel information
+ *
+ * @mhu: Pointer to parent mailbox device
+ * @pchan: Physical channel within which this sub-channel resides in
+ * @channel: Sub-channel number pertaining to this container
+ */
+struct mhu_channel {
+	struct arm_mhu *mhu;
+	unsigned int pchan;
+	unsigned int subchan;
+};
+
+static inline struct mbox_chan *
+mhu_mbox_to_channel(struct mbox_controller *mbox,
+		    unsigned int pchan, unsigned int subchan)
+{
+	int i;
+	struct mhu_channel *chan_info;
+
+	for (i = 0; i < mbox->num_chans; i++) {
+		chan_info = mbox->chans[i].con_priv;
+		if (chan_info && chan_info->pchan == pchan &&
+		    chan_info->subchan == subchan)
+			return &mbox->chans[i];
+	}
+
+	dev_err(mbox->dev,
+		"Channel not registered: physical channel: %d subchan: %d\n",
+		pchan, subchan);
+
+	return NULL;
+}
+
+static unsigned int mhu_mbox_irq_to_pchan_num(struct arm_mhu *mhu, int irq)
+{
+	unsigned int pchan;
+	struct mhu_mbox_pdata *pdata = dev_get_platdata(mhu->dev);
+
+	for (pchan = 0; pchan < pdata->num_pchans; pchan++)
+		if (mhu->mlink[pchan].irq == irq)
+			break;
+	return pchan;
+}
+
+static struct mbox_chan *mhu_mbox_xlate(struct mbox_controller *mbox,
+					const struct of_phandle_args *spec)
+{
+	struct arm_mhu *mhu = dev_get_drvdata(mbox->dev);
+	struct mhu_mbox_pdata *pdata = dev_get_platdata(mhu->dev);
+	struct mhu_channel *chan_info;
+	struct mbox_chan *chan = NULL;
+	unsigned int pchan = spec->args[0];
+	unsigned int subchan = pdata->support_subchans ? spec->args[1] : 0;
+	int i;
+
+	/* Bounds checking */
+	if (pchan >= pdata->num_pchans || subchan >= pdata->num_subchans) {
+		dev_err(mbox->dev,
+			"Invalid channel requested pchan: %d subchan: %d\n",
+			pchan, subchan);
+		return ERR_PTR(-EINVAL);
+	}
+
+	for (i = 0; i < mbox->num_chans; i++) {
+		chan_info = mbox->chans[i].con_priv;
+
+		/* Is requested channel free? */
+		if (chan_info &&
+		    mbox->dev == chan_info->mhu->dev &&
+		    pchan == chan_info->pchan &&
+		    subchan == chan_info->subchan) {
+			dev_err(mbox->dev, "Channel in use\n");
+			return ERR_PTR(-EBUSY);
+		}
+
+		/*
+		 * Find the first free slot, then continue checking
+		 * to see if requested channel is in use
+		 */
+		if (!chan && !chan_info)
+			chan = &mbox->chans[i];
+	}
+
+	if (!chan) {
+		dev_err(mbox->dev, "No free channels left\n");
+		return ERR_PTR(-EBUSY);
+	}
+
+	chan_info = devm_kzalloc(mbox->dev, sizeof(*chan_info), GFP_KERNEL);
+	if (!chan_info)
+		return ERR_PTR(-ENOMEM);
+
+	chan_info->mhu = mhu;
+	chan_info->pchan = pchan;
+	chan_info->subchan = subchan;
+
+	chan->con_priv = chan_info;
+
+	dev_dbg(mbox->dev, "mbox: created channel - physical: %d sub: %d\n",
+		pchan, subchan);
+
+	return chan;
+}
+
 static irqreturn_t mhu_rx_interrupt(int irq, void *p)
 {
-	struct mbox_chan *chan = p;
-	struct mhu_link *mlink = chan->con_priv;
+	struct arm_mhu *mhu = p;
+	unsigned int pchan = mhu_mbox_irq_to_pchan_num(mhu, irq);
+	struct mbox_chan *chan = mhu_mbox_to_channel(&mhu->mbox, pchan, 0);
+	void __iomem *base = mhu->mlink[pchan].rx_reg;
 	u32 val;
 
-	val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS);
+	val = readl_relaxed(base + INTR_STAT_OFS);
 	if (!val)
 		return IRQ_NONE;
 
 	mbox_chan_received_data(chan, (void *)&val);
 
-	writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS);
+	writel_relaxed(val, base + INTR_CLR_OFS);
 
 	return IRQ_HANDLED;
 }
 
 static bool mhu_last_tx_done(struct mbox_chan *chan)
 {
-	struct mhu_link *mlink = chan->con_priv;
-	u32 val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
+	struct mhu_channel *chan_info = chan->con_priv;
+	void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
+	u32 val = readl_relaxed(base + INTR_STAT_OFS);
 
 	return (val == 0);
 }
 
 static int mhu_send_data(struct mbox_chan *chan, void *data)
 {
-	struct mhu_link *mlink = chan->con_priv;
+	struct mhu_channel *chan_info = chan->con_priv;
+	void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
 	u32 *arg = data;
 
-	writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS);
+	writel_relaxed(*arg, base + INTR_SET_OFS);
 
 	return 0;
 }
 
 static int mhu_startup(struct mbox_chan *chan)
 {
-	struct mhu_link *mlink = chan->con_priv;
+	struct mhu_channel *chan_info = chan->con_priv;
+	void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
 	u32 val;
 
-	val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
-	writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);
+	val = readl_relaxed(base + INTR_STAT_OFS);
+	writel_relaxed(val, base + INTR_CLR_OFS);
 
 	return 0;
 }
@@ -102,14 +227,46 @@ static const struct mbox_chan_ops mhu_ops = {
 	.last_tx_done = mhu_last_tx_done,
 };
 
+static const struct mhu_mbox_pdata arm_mhu_pdata = {
+	.num_pchans = 3,
+	.num_subchans = 1,
+	.support_subchans = false,
+};
+
+static const struct of_device_id mhu_mbox_match[] = {
+	{ .compatible = "arm,mhu", .data = (void *)&arm_mhu_pdata },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, mhu_mbox_match);
+
 static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 {
-	int i, err;
+	int i, err, max_chans;
 	struct arm_mhu *mhu;
+	struct mbox_chan *chans;
+	struct mhu_mbox_pdata *pdata;
+	const struct of_device_id *match;
 	struct device *dev = &adev->dev;
-	int mhu_reg[MHU_CHANS] = {MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET};
+	int mhu_reg[MHU_NUM_PCHANS] = {
+		MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET,
+	};
+
+	match = of_match_device(mhu_mbox_match, dev);
+	if (!match) {
+		dev_err(dev, "No configuration found\n");
+		return -ENODEV;
+	}
+	pdata = (struct mhu_mbox_pdata *)match->data;
+
+	if (pdata->num_pchans > MHU_NUM_PCHANS) {
+		dev_err(dev, "Number of physical channel can't exceed %d\n",
+			MHU_NUM_PCHANS);
+		return -EINVAL;
+	}
+
+	max_chans = pdata->support_subchans ? MHU_CHAN_MAX : MHU_NUM_PCHANS;
 
-	/* Allocate memory for device */
 	mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
 	if (!mhu)
 		return -ENOMEM;
@@ -120,14 +277,22 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		return PTR_ERR(mhu->base);
 	}
 
+	chans = devm_kcalloc(dev, max_chans, sizeof(*chans), GFP_KERNEL);
+	if (!chans)
+		return -ENOMEM;
+
+	dev->platform_data = pdata;
+
+	mhu->dev = dev;
 	mhu->mbox.dev = dev;
-	mhu->mbox.chans = &mhu->chan[0];
-	mhu->mbox.num_chans = MHU_CHANS;
+	mhu->mbox.chans = chans;
+	mhu->mbox.num_chans = max_chans;
 	mhu->mbox.ops = &mhu_ops;
 	mhu->mbox.txdone_irq = false;
 	mhu->mbox.txdone_poll = true;
 	mhu->mbox.txpoll_period = 1;
 
+	mhu->mbox.of_xlate = mhu_mbox_xlate;
 	amba_set_drvdata(adev, mhu);
 
 	err = mbox_controller_register(&mhu->mbox);
@@ -136,7 +301,7 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		return err;
 	}
 
-	for (i = 0; i < MHU_CHANS; i++) {
+	for (i = 0; i < pdata->num_pchans; i++) {
 		int irq = mhu->mlink[i].irq = adev->irq[i];
 
 		if (irq <= 0) {
@@ -144,13 +309,12 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 			continue;
 		}
 
-		mhu->chan[i].con_priv = &mhu->mlink[i];
 		mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
 		mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
 
 		err = devm_request_threaded_irq(dev, irq, NULL,
 						mhu_rx_interrupt, IRQF_ONESHOT,
-						"mhu_link", &mhu->chan[i]);
+						"mhu_link", mhu);
 		if (err) {
 			dev_err(dev, "Can't claim IRQ %d\n", irq);
 			mbox_controller_unregister(&mhu->mbox);
-- 
2.7.4

[PATCH 5/6] mailbox: arm_mhu: add full support for sub-channels

[Sudeep Holla]

We now have the basic infrastructure and binding to support subchannels
on ARM MHU controller.

This patch adds all the necessary mailbox operations to add support for
the sub-channels. Maximum of 32 subchannels are supported on each physical
channel, however the total number of subchannels is restricted to 20
in order to save memory. It can increased if required in future.

Cc: Alexey Klimov <alexey.klimov@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/arm_mhu.c | 127 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 123 insertions(+), 4 deletions(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index 4e0f690b97fd..0f5ab2204649 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -18,6 +18,7 @@
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/kernel.h>
 #include <linux/mailbox_controller.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -94,6 +95,14 @@ mhu_mbox_to_channel(struct mbox_controller *mbox,
 	return NULL;
 }
 
+static void mhu_mbox_clear_irq(struct mbox_chan *chan)
+{
+	struct mhu_channel *chan_info = chan->con_priv;
+	void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].rx_reg;
+
+	writel_relaxed(BIT(chan_info->subchan), base + INTR_CLR_OFS);
+}
+
 static unsigned int mhu_mbox_irq_to_pchan_num(struct arm_mhu *mhu, int irq)
 {
 	unsigned int pchan;
@@ -105,6 +114,95 @@ static unsigned int mhu_mbox_irq_to_pchan_num(struct arm_mhu *mhu, int irq)
 	return pchan;
 }
 
+static struct mbox_chan *mhu_mbox_irq_to_channel(struct arm_mhu *mhu,
+						 unsigned int pchan)
+{
+	unsigned long bits;
+	unsigned int subchan;
+	struct mbox_chan *chan = NULL;
+	struct mbox_controller *mbox = &mhu->mbox;
+	void __iomem *base = mhu->mlink[pchan].rx_reg;
+
+	bits = readl_relaxed(base + INTR_STAT_OFS);
+	if (!bits)
+		/* No IRQs fired in specified physical channel */
+		return NULL;
+
+	/* An IRQ has fired, find the associated channel */
+	for (subchan = 0; bits; subchan++) {
+		if (!test_and_clear_bit(subchan, &bits))
+			continue;
+
+		chan = mhu_mbox_to_channel(mbox, pchan, subchan);
+		if (chan)
+			break;
+	}
+
+	return chan;
+}
+
+static irqreturn_t mhu_mbox_thread_handler(int irq, void *data)
+{
+	struct mbox_chan *chan;
+	struct arm_mhu *mhu = data;
+	unsigned int pchan = mhu_mbox_irq_to_pchan_num(mhu, irq);
+
+	while (NULL != (chan = mhu_mbox_irq_to_channel(mhu, pchan))) {
+		mbox_chan_received_data(chan, NULL);
+		mhu_mbox_clear_irq(chan);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static bool mhu_subchan_last_tx_done(struct mbox_chan *chan)
+{
+	struct mhu_channel *chan_info = chan->con_priv;
+	void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
+
+	if (readl_relaxed(base + INTR_STAT_OFS) & BIT(chan_info->subchan))
+		return false;
+
+	return true;
+}
+
+static int mhu_subchan_send_data(struct mbox_chan *chan, void *data)
+{
+	struct mhu_channel *chan_info = chan->con_priv;
+	void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
+
+	/* Send event to co-processor */
+	writel_relaxed(BIT(chan_info->subchan), base + INTR_SET_OFS);
+
+	return 0;
+}
+
+static int mhu_subchan_startup(struct mbox_chan *chan)
+{
+	mhu_mbox_clear_irq(chan);
+	return 0;
+}
+
+static void mhu_subchan_shutdown(struct mbox_chan *chan)
+{
+	struct mhu_channel *chan_info = chan->con_priv;
+	struct mbox_controller *mbox = &chan_info->mhu->mbox;
+	int i;
+
+	for (i = 0; i < mbox->num_chans; i++)
+		if (chan == &mbox->chans[i])
+			break;
+
+	if (mbox->num_chans == i) {
+		dev_warn(mbox->dev, "Request to free non-existent channel\n");
+		return;
+	}
+
+	/* Reset channel */
+	mhu_mbox_clear_irq(chan);
+	chan->con_priv = NULL;
+}
+
 static struct mbox_chan *mhu_mbox_xlate(struct mbox_controller *mbox,
 					const struct of_phandle_args *spec)
 {
@@ -227,14 +325,28 @@ static const struct mbox_chan_ops mhu_ops = {
 	.last_tx_done = mhu_last_tx_done,
 };
 
+static const struct mbox_chan_ops mhu_subchan_ops = {
+	.send_data = mhu_subchan_send_data,
+	.startup = mhu_subchan_startup,
+	.shutdown = mhu_subchan_shutdown,
+	.last_tx_done = mhu_subchan_last_tx_done,
+};
+
 static const struct mhu_mbox_pdata arm_mhu_pdata = {
 	.num_pchans = 3,
 	.num_subchans = 1,
 	.support_subchans = false,
 };
 
+static const struct mhu_mbox_pdata arm_mhu_v2_pdata = {
+	.num_pchans = 2,	/* Secure can't be used */
+	.num_subchans = 32,
+	.support_subchans = true,
+};
+
 static const struct of_device_id mhu_mbox_match[] = {
 	{ .compatible = "arm,mhu", .data = (void *)&arm_mhu_pdata },
+	{ .compatible = "arm,mhu-v2", .data = (void *)&arm_mhu_v2_pdata },
 	{}
 };
 
@@ -243,6 +355,7 @@ MODULE_DEVICE_TABLE(of, mhu_mbox_match);
 static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	int i, err, max_chans;
+	irq_handler_t handler;
 	struct arm_mhu *mhu;
 	struct mbox_chan *chans;
 	struct mhu_mbox_pdata *pdata;
@@ -287,7 +400,6 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 	mhu->mbox.dev = dev;
 	mhu->mbox.chans = chans;
 	mhu->mbox.num_chans = max_chans;
-	mhu->mbox.ops = &mhu_ops;
 	mhu->mbox.txdone_irq = false;
 	mhu->mbox.txdone_poll = true;
 	mhu->mbox.txpoll_period = 1;
@@ -295,6 +407,14 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 	mhu->mbox.of_xlate = mhu_mbox_xlate;
 	amba_set_drvdata(adev, mhu);
 
+	if (pdata->support_subchans) {
+		mhu->mbox.ops = &mhu_subchan_ops;
+		handler = mhu_mbox_thread_handler;
+	} else {
+		mhu->mbox.ops = &mhu_ops;
+		handler = mhu_rx_interrupt;
+	}
+
 	err = mbox_controller_register(&mhu->mbox);
 	if (err) {
 		dev_err(dev, "Failed to register mailboxes %d\n", err);
@@ -312,9 +432,8 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
 		mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
 
-		err = devm_request_threaded_irq(dev, irq, NULL,
-						mhu_rx_interrupt, IRQF_ONESHOT,
-						"mhu_link", mhu);
+		err = devm_request_threaded_irq(dev, irq, NULL, handler,
+						IRQF_ONESHOT, "mhu_link", mhu);
 		if (err) {
 			dev_err(dev, "Can't claim IRQ %d\n", irq);
 			mbox_controller_unregister(&mhu->mbox);
-- 
2.7.4

[PATCH 6/6] mailbox: arm_mhu: add name support to record mbox-name

[Sudeep Holla]

It's sometimes useful to identify the mailbox controller with the name
as specified in the devicetree via mbox-name property especially in a
system with multiple controllers.

This patch adds support to read and record the mailbox controller name.

Cc: Alexey Klimov <alexey.klimov@arm.com>
Cc: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/arm_mhu.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index 0f5ab2204649..9aa623a3aa9a 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -47,6 +47,7 @@ struct arm_mhu {
 	struct mhu_link mlink[MHU_NUM_PCHANS];
 	struct mbox_controller mbox;
 	struct device *dev;
+	const char *name;
 };
 
 /**
@@ -257,8 +258,8 @@ static struct mbox_chan *mhu_mbox_xlate(struct mbox_controller *mbox,
 
 	chan->con_priv = chan_info;
 
-	dev_dbg(mbox->dev, "mbox: created channel - physical: %d sub: %d\n",
-		pchan, subchan);
+	dev_dbg(mbox->dev, "mbox: %s, created channel - physical: %d sub: %d\n",
+		mhu->name, pchan, subchan);
 
 	return chan;
 }
@@ -361,6 +362,7 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 	struct mhu_mbox_pdata *pdata;
 	const struct of_device_id *match;
 	struct device *dev = &adev->dev;
+	struct device_node *np = dev->of_node;
 	int mhu_reg[MHU_NUM_PCHANS] = {
 		MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET,
 	};
@@ -390,6 +392,10 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		return PTR_ERR(mhu->base);
 	}
 
+	err = of_property_read_string(np, "mbox-name", &mhu->name);
+	if (err)
+		mhu->name = np->full_name;
+
 	chans = devm_kcalloc(dev, max_chans, sizeof(*chans), GFP_KERNEL);
 	if (!chans)
 		return -ENOMEM;
@@ -441,7 +447,7 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
 		}
 	}
 
-	dev_info(dev, "ARM MHU Mailbox registered\n");
+	dev_info(dev, "%s mailbox registered\n", mhu->name);
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH 0/6] mailbox: arm_mhu: add support for subchannels

[Jassi Brar]

Hi Sudeep,

On Tue, May 2, 2017 at 7:25 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> Hi Jassi,
>
> This series adds subchannel support to ARM MHU mailbox controller
> driver. Since SCPI never used second slot, we were able to use the
> existing driver as is. However, that's changing soon and the new
> SCMI protocol under development needs subchannel support. If you
> recall when you initially added this driver, I was pushing for some
> of these changes like threaded irq. This patch series adds support
> for the subchannels on ARM MHU controllers.
>
  There are really no "sub-channels" in the ARM MHU controller. There
are exactly three channels that work on 32bit registers. The SET/CLEAR
registers are there to prevent races between local and remote
firmware, and not to emulate virtual channels operating on single
bits. Please remember all 32-bits work together to generate one
signal.

 You arrived at the "sub-channel" idea only because your protocol uses
1-bit messages. This patchset seems rather regressive - reduce from
2^32 possible signals to mere 32, by bloating the MHU driver.

 If it's difficult to see how your protocol can be implemented over
existing controller driver, please let me know.

Thanks.

Re: [PATCH 0/6] mailbox: arm_mhu: add support for subchannels

[Sudeep Holla]

On 03/05/17 04:17, Jassi Brar wrote:
> Hi Sudeep,
> 
> On Tue, May 2, 2017 at 7:25 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> Hi Jassi,
>>
>> This series adds subchannel support to ARM MHU mailbox controller
>> driver. Since SCPI never used second slot, we were able to use the
>> existing driver as is. However, that's changing soon and the new
>> SCMI protocol under development needs subchannel support. If you
>> recall when you initially added this driver, I was pushing for some
>> of these changes like threaded irq. This patch series adds support
>> for the subchannels on ARM MHU controllers.
>>
>   There are really no "sub-channels" in the ARM MHU controller. There
> are exactly three channels that work on 32bit registers. The SET/CLEAR
> registers are there to prevent races between local and remote
> firmware, and not to emulate virtual channels operating on single
> bits. Please remember all 32-bits work together to generate one
> signal.
> 

If you check 3.4.4 Message Handling Unit (MHU) of Juno TRM [1],

"..the MHU drives the signal using a 32-bit register, with all 32 bits
logically ORed together. The MHU 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 for each 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."

So yes, they generate one signal, but that doesn't mean anything. We
have even PMU interrupts tied to single SPI on some SoC. Since the
design of MHU clearly indicates that each bit can be used independently
for different event, for all practical purpose, it can be treated as
different channel.

>  You arrived at the "sub-channel" idea only because your protocol uses
> 1-bit messages. 

May be. It now uses BIT 0 for one channel and BIT 1 for another on the
same physical channel. How do you propose it support that then ? We have
multiple protocols with the same remote, so this is just used as a
doorbell bit and not carrier of any message.

> This patchset seems rather regressive - reduce from
> 2^32 possible signals to mere 32, by bloating the MHU driver.
> 

I don't quite get this. There are only 3 signals as you mentioned above.
Yes there are 2^32 possible values for the register, but how can that be
used ? I don't think that was the design intention at-least from the
above text in the specification. Also the we still continue to support
one channel per physical channel.

>  If it's difficult to see how your protocol can be implemented over
> existing controller driver, please let me know.
> 

It was a workaround just because there was no other protocol so far.
I just set bit 0 and send it as u32 to send_message.

-- 
Regards,
Sudeep

[1]
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0515f/DDI0515F_juno_arm_development_platform_soc_trm.pdf

Re: [PATCH 0/6] mailbox: arm_mhu: add support for subchannels

[Jassi Brar]

On Wed, May 3, 2017 at 2:51 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 03/05/17 04:17, Jassi Brar wrote:
>> Hi Sudeep,
>>
>> On Tue, May 2, 2017 at 7:25 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>> Hi Jassi,
>>>
>>> This series adds subchannel support to ARM MHU mailbox controller
>>> driver. Since SCPI never used second slot, we were able to use the
>>> existing driver as is. However, that's changing soon and the new
>>> SCMI protocol under development needs subchannel support. If you
>>> recall when you initially added this driver, I was pushing for some
>>> of these changes like threaded irq. This patch series adds support
>>> for the subchannels on ARM MHU controllers.
>>>
>>   There are really no "sub-channels" in the ARM MHU controller. There
>> are exactly three channels that work on 32bit registers. The SET/CLEAR
>> registers are there to prevent races between local and remote
>> firmware, and not to emulate virtual channels operating on single
>> bits. Please remember all 32-bits work together to generate one
>> signal.
>>
>
> If you check 3.4.4 Message Handling Unit (MHU) of Juno TRM [1],
>
> "..the MHU drives the signal using a 32-bit register, with all 32 bits
> logically ORed together. The MHU 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 for each 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."
>
> So yes, they generate one signal, but that doesn't mean anything.
>
That means a lot. That means a MHU signal/message is 32bits, not single bit.

> We
> have even PMU interrupts tied to single SPI on some SoC. Since the
> design of MHU clearly indicates that each bit can be used independently
> for different event, for all practical purpose, it can be treated as
> different channel.
>
Please don't mess with controller driver to support your usecase,
which is already well supported.

>>  You arrived at the "sub-channel" idea only because your protocol uses
>> 1-bit messages.
>
> May be. It now uses BIT 0 for one channel and BIT 1 for another on the
> same physical channel. How do you propose it support that then ?
>
There is no "sub-channel", but only physical channel.
You are led to believe each bit represents one channel only because
your protocol uses (1<<N) type signals.

> We have
> multiple protocols with the same remote, so this is just used as a
> doorbell bit and not carrier of any message.
>
Doorbell is a single bit message in mailbox framework :)

>> This patchset seems rather regressive - reduce from
>> 2^32 possible signals to mere 32, by bloating the MHU driver.
>>
>
> I don't quite get this. There are only 3 signals as you mentioned above.
> Yes there are 2^32 possible values for the register, but how can that be
> used ?
>
_Your_ protocol don't use more than 32 values, that doesn't mean other
protocols don't either.

Re: [PATCH 0/6] mailbox: arm_mhu: add support for subchannels

[Sudeep Holla]

On 05/05/17 12:12, Jassi Brar wrote:
> On Wed, May 3, 2017 at 2:51 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

[...]

>>
>> I don't quite get this. There are only 3 signals as you mentioned above.
>> Yes there are 2^32 possible values for the register, but how can that be
>> used ?
>>
> _Your_ protocol don't use more than 32 values, that doesn't mean other
> protocols don't either.
> 

Please read what I quoted from the spec.
"..the MHU drives the signal using a 32-bit register, with all 32 bits
logically ORed together. The MHU 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 for each 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."

It is designed exactly for the above use-case. It was designed to fit
PCC in ACPI which allows what I am doing with this series.

It's very similar to many PMIC drivers we have. Though set of PMIC
events are bundled into single register and interrupt doesn't mean they
need to be support as event. E.g. look at mc13xxx, wm831x,...etc

I am not trying to fit the changes to our use-case. I would counter
argue that you did so when you push the driver. Since it's very clear
for the spec that the individual bits can be accessed atomically, it
was designed to be used as doorbell and not as a message carrier
register. I am fine if it can be used in that way but don't disagree to
support the use-case for which it was designed.

-- 
Regards,
Sudeep