[PATCH 0/2] mailbox: Introduce Texas Instrument's message manager driver

[PATCH 0/2] mailbox: Introduce Texas Instrument's message manager driver

[Nishanth Menon]

Hi,

The following series provides a base driver for the support of TI's
message manager hardware block which facilitates communication
between multiple compute engines(or processors) with a central system
controller (called PMMC in K2G SoC). Keystone family of TI processors
incorporate this solution starting with K2G[1].

The hardware specification is available here:
http://www.ti.com/lit/ug/spruhy8/spruhy8.pdf Chapter 8.1 (Message
Manager).

Starting with K2G SoC, TI Keystone architecture is moving towards an
architecture similar to ARM Juno platform with a centralized system
controller where all system control takes place - all power management
and system control functions are now centralized in this system
controller. A protocol called TI-SCI (TI System Control Interface)
is built on top of this mailbox driver providing for communication
protocol (similar to SCPI drivers/firmware/arm_scpi.c) Follow on
patches (once the message manager is accepted) will enable the same.
However, an 4.1 kernel based vendor kernel implementation is available
at [2].

The following series is functional in TI vendor kernel, however, was
tested on K2G EVM using the following test patch:
http://pastebin.ubuntu.com/14872857/
Boot log: http://pastebin.ubuntu.com/14872879/

baseline: v4.5-rc1

This could potentially be a targetted for v4.6-rc1 kernel.

Nishanth Menon (2):
  Documentation: dt: mailbox: Add TI Message Manager
  mailbox: Introduce TI message manager driver

 .../bindings/mailbox/ti,message-manager.txt        |  72 +++
 drivers/mailbox/Kconfig                            |  11 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/ti-msgmgr.c                        | 603 +++++++++++++++++++++
 include/linux/ti-msgmgr.h                          |  35 ++
 5 files changed, 723 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
 create mode 100644 drivers/mailbox/ti-msgmgr.c
 create mode 100644 include/linux/ti-msgmgr.h

[1] K2G support
https://patchwork.kernel.org/patch/8196481/
https://patchwork.kernel.org/patch/8196491/
https://patchwork.kernel.org/patch/8196471/

[2] TISCI support(for reference): (mailbox client driver)
http://git.ti.com/ti-linux-kernel/ti-linux-kernel/blobs/ti-lsk-linux-4.1.y/Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
http://git.ti.com/ti-linux-kernel/ti-linux-kernel/blobs/ti-lsk-linux-4.1.y/drivers/firmware/ti_sci.c
http://git.ti.com/ti-linux-kernel/ti-linux-kernel/blobs/ti-lsk-linux-4.1.y/drivers/firmware/ti_sci.h
http://git.ti.com/ti-linux-kernel/ti-linux-kernel/blobs/ti-lsk-linux-4.1.y/include/linux/ti_sci_protocol.h
The above protocol is used for clock, generic powerdomain, reset etc..

-- 
2.7.0

[PATCH 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Nishanth Menon]

Message Manager is a hardware block used to communicate with various
processor systems within certain Texas Instrument's Keystone
generation SoCs.

This hardware engine is used to transfer messages from various compute
entities(or processors) within the SoC. It is designed to be self
contained without needing software initialization for operation.

Signed-off-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
---
 .../bindings/mailbox/ti,message-manager.txt        | 72 ++++++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/ti,message-manager.txt

diff --git a/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt b/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
new file mode 100644
index 000000000000..f3d73b0b3c66
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
@@ -0,0 +1,72 @@
+Texas Instruments' Message Manager Driver
+========================================
+
+The Texas Instruments' Message Manager is a mailbox controller that has
+configurable queues selectable at SoC(System on Chip) integration. The Message
+manager is broken up into queues in different address regions that are called
+"proxies" - each instance is unidirectional and is instantiated at SoC
+integration level to indicate receive or transmit path.
+
+Message Manager Device Node:
+===========================
+
+Required properties:
+--------------------
+- compatible:		Shall be:
+				"ti,k2g-message-manager"
+				"ti,message-manager"
+- reg-names 		queue_proxy_region - Map the queue Proxy region
+			queue_state_debug_region - Map the queue state debug
+			region.
+- reg:			Contains the register map per reg-names
+- #mbox-cells		Shall be 1
+
+Child Nodes:
+============
+A child node is used for representing the actual queue device that is
+used for the communication between the host processor and a remote processor.
+Each child node should have a unique node name across all the different
+message manager device nodes.
+
+Required Properties:
+--------------------
+- ti,queue-id:		Indicates the queue number this node represents
+- ti,proxy-id:		Proxy ID representing the processor in the SoC.
+
+Optional Properties:
+--------------------
+- interrupt-names:	'rx' - indicates a receive interrupt (mandatory ONLY if
+			this is a receive queue)
+- interrupts:		Contains the interrupt information corresponding to
+			interrupt-names property.
+
+Example:
+--------
+
+	msgmgr: msgmgr@02a00000 {
+		compatible = "ti,k2g-message-manager", "ti,message-manager";
+		#mbox-cells = <1>;
+		reg-names = "queue_proxy_region", "queue_state_debug_region";
+		reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
+
+		msgmgr_proxy_pmmc_tx_prio0: pmmc_tx_prio0 {
+			ti,queue-id = <0>;
+			ti,proxy-id = <0>;
+		};
+
+		msgmgr_proxy_pmmc_rx: pmmc_rx {
+			ti,queue-id = <5>;
+			ti,proxy-id = <2>;
+			interrupt-names = "rx";
+			interrupts = <GIC_SPI 324 IRQ_TYPE_EDGE_RISING>;
+		};
+	};
+
+...
+	pmmc {
+		...
+		mbox-names = "tx", "rx";
+		mboxes = <&msgmgr &msgmgr_proxy_pmmc_tx>
+			 <&msgmgr &msgmgr_proxy_pmmc_rx>;
+		...
+	};
-- 
2.7.0

--
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 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Rob Herring]

On Fri, Feb 05, 2016 at 10:34:03AM -0600, Nishanth Menon wrote:
> Message Manager is a hardware block used to communicate with various
> processor systems within certain Texas Instrument's Keystone
> generation SoCs.
> 
> This hardware engine is used to transfer messages from various compute
> entities(or processors) within the SoC. It is designed to be self
> contained without needing software initialization for operation.
> 
> Signed-off-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
> ---
>  .../bindings/mailbox/ti,message-manager.txt        | 72 ++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt b/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
> new file mode 100644
> index 000000000000..f3d73b0b3c66
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
> @@ -0,0 +1,72 @@
> +Texas Instruments' Message Manager Driver
> +========================================
> +
> +The Texas Instruments' Message Manager is a mailbox controller that has
> +configurable queues selectable at SoC(System on Chip) integration. The Message
> +manager is broken up into queues in different address regions that are called
> +"proxies" - each instance is unidirectional and is instantiated at SoC
> +integration level to indicate receive or transmit path.
> +
> +Message Manager Device Node:
> +===========================
> +
> +Required properties:
> +--------------------
> +- compatible:		Shall be:
> +				"ti,k2g-message-manager"
> +				"ti,message-manager"

Given that queues are configurable at integration time, does a generic 
property really make sense here?

> +- reg-names 		queue_proxy_region - Map the queue Proxy region
> +			queue_state_debug_region - Map the queue state debug
> +			region.
> +- reg:			Contains the register map per reg-names
> +- #mbox-cells		Shall be 1

And the value contained is what?

> +
> +Child Nodes:
> +============
> +A child node is used for representing the actual queue device that is
> +used for the communication between the host processor and a remote processor.
> +Each child node should have a unique node name across all the different
> +message manager device nodes.
> +
> +Required Properties:
> +--------------------
> +- ti,queue-id:		Indicates the queue number this node represents
> +- ti,proxy-id:		Proxy ID representing the processor in the SoC.

What determines these values?

> +
> +Optional Properties:
> +--------------------
> +- interrupt-names:	'rx' - indicates a receive interrupt (mandatory ONLY if
> +			this is a receive queue)

Kind of pointless if there is only 1.

> +- interrupts:		Contains the interrupt information corresponding to
> +			interrupt-names property.
> +
> +Example:
> +--------
> +
> +	msgmgr: msgmgr@02a00000 {
> +		compatible = "ti,k2g-message-manager", "ti,message-manager";
> +		#mbox-cells = <1>;
> +		reg-names = "queue_proxy_region", "queue_state_debug_region";
> +		reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
> +
> +		msgmgr_proxy_pmmc_tx_prio0: pmmc_tx_prio0 {
> +			ti,queue-id = <0>;
> +			ti,proxy-id = <0>;
> +		};
> +
> +		msgmgr_proxy_pmmc_rx: pmmc_rx {
> +			ti,queue-id = <5>;
> +			ti,proxy-id = <2>;
> +			interrupt-names = "rx";
> +			interrupts = <GIC_SPI 324 IRQ_TYPE_EDGE_RISING>;
> +		};
> +	};
> +
> +...
> +	pmmc {
> +		...
> +		mbox-names = "tx", "rx";
> +		mboxes = <&msgmgr &msgmgr_proxy_pmmc_tx>
> +			 <&msgmgr &msgmgr_proxy_pmmc_rx>;

While I guess this is valid DT, it is a bit strange having the cell 
values be phandles. Why not just make the queue and proxy ids be the 
cell values? The interrupts could be moved to the parent and the child 
nodes eliminated altogether.

The alternative would be just drop msgmgr phandle and point to the child 
nodes. I prefer getting rid of the child nodes though.

Rob
--
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 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Suman Anna]

Hi Rob,

On 02/08/2016 01:37 PM, Rob Herring wrote:
> On Fri, Feb 05, 2016 at 10:34:03AM -0600, Nishanth Menon wrote:
>> Message Manager is a hardware block used to communicate with various
>> processor systems within certain Texas Instrument's Keystone
>> generation SoCs.
>>
>> This hardware engine is used to transfer messages from various compute
>> entities(or processors) within the SoC. It is designed to be self
>> contained without needing software initialization for operation.
>>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> ---
>>  .../bindings/mailbox/ti,message-manager.txt        | 72 ++++++++++++++++++++++
>>  1 file changed, 72 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt b/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
>> new file mode 100644
>> index 000000000000..f3d73b0b3c66
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
>> @@ -0,0 +1,72 @@
>> +Texas Instruments' Message Manager Driver
>> +========================================
>> +
>> +The Texas Instruments' Message Manager is a mailbox controller that has
>> +configurable queues selectable at SoC(System on Chip) integration. The Message
>> +manager is broken up into queues in different address regions that are called
>> +"proxies" - each instance is unidirectional and is instantiated at SoC
>> +integration level to indicate receive or transmit path.
>> +
>> +Message Manager Device Node:
>> +===========================
>> +
>> +Required properties:
>> +--------------------
>> +- compatible:		Shall be:
>> +				"ti,k2g-message-manager"
>> +				"ti,message-manager"
> 
> Given that queues are configurable at integration time, does a generic 
> property really make sense here?
> 
>> +- reg-names 		queue_proxy_region - Map the queue Proxy region
>> +			queue_state_debug_region - Map the queue state debug
>> +			region.
>> +- reg:			Contains the register map per reg-names
>> +- #mbox-cells		Shall be 1
> 
> And the value contained is what?
> 
>> +
>> +Child Nodes:
>> +============
>> +A child node is used for representing the actual queue device that is
>> +used for the communication between the host processor and a remote processor.
>> +Each child node should have a unique node name across all the different
>> +message manager device nodes.
>> +
>> +Required Properties:
>> +--------------------
>> +- ti,queue-id:		Indicates the queue number this node represents
>> +- ti,proxy-id:		Proxy ID representing the processor in the SoC.
> 
> What determines these values?
> 
>> +
>> +Optional Properties:
>> +--------------------
>> +- interrupt-names:	'rx' - indicates a receive interrupt (mandatory ONLY if
>> +			this is a receive queue)
> 
> Kind of pointless if there is only 1.
> 
>> +- interrupts:		Contains the interrupt information corresponding to
>> +			interrupt-names property.
>> +
>> +Example:
>> +--------
>> +
>> +	msgmgr: msgmgr@02a00000 {
>> +		compatible = "ti,k2g-message-manager", "ti,message-manager";
>> +		#mbox-cells = <1>;
>> +		reg-names = "queue_proxy_region", "queue_state_debug_region";
>> +		reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
>> +
>> +		msgmgr_proxy_pmmc_tx_prio0: pmmc_tx_prio0 {
>> +			ti,queue-id = <0>;
>> +			ti,proxy-id = <0>;
>> +		};
>> +
>> +		msgmgr_proxy_pmmc_rx: pmmc_rx {
>> +			ti,queue-id = <5>;
>> +			ti,proxy-id = <2>;
>> +			interrupt-names = "rx";
>> +			interrupts = <GIC_SPI 324 IRQ_TYPE_EDGE_RISING>;
>> +		};
>> +	};
>> +
>> +...
>> +	pmmc {
>> +		...
>> +		mbox-names = "tx", "rx";
>> +		mboxes = <&msgmgr &msgmgr_proxy_pmmc_tx>
>> +			 <&msgmgr &msgmgr_proxy_pmmc_rx>;
> 
> While I guess this is valid DT, it is a bit strange having the cell 
> values be phandles. Why not just make the queue and proxy ids be the 
> cell values? The interrupts could be moved to the parent and the child 
> nodes eliminated altogether.

The mailbox controller node (msgmgr) and its child nodes all are
registered with the mailbox core together and the client users request a
mailbox using the framework API that requires the controller node.
Usually the channels are given numbers and the request is done based on
indices, but that meant that we had to introduce a logical device id
just for the sake of the API. Instead, we just chose to go with the
phandles.

Doing a queue and proxy id as cell values requires the mailbox
controller node registration phase to parse through all the client nodes
with the matching controller node. The current approach was in-line with
how all the mailbox controller nodes are implemented today, except for
the usage of phandle instead of an index for the cell-value.

> 
> The alternative would be just drop msgmgr phandle and point to the child 
> nodes. I prefer getting rid of the child nodes though.

The 'mboxes' property is a standard property supported by the mailbox
framework core, and requires the phandle of the controller node along
with a cell-value to lookup a channel. So, we cannot use the child nodes
directly.

regards
Suman

Re: [PATCH 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Rob Herring]

On Mon, Feb 8, 2016 at 2:23 PM, Suman Anna <s-anna@ti.com> wrote:
> Hi Rob,
>
> On 02/08/2016 01:37 PM, Rob Herring wrote:
>> On Fri, Feb 05, 2016 at 10:34:03AM -0600, Nishanth Menon wrote:
>>> Message Manager is a hardware block used to communicate with various
>>> processor systems within certain Texas Instrument's Keystone
>>> generation SoCs.
>>>
>>> This hardware engine is used to transfer messages from various compute
>>> entities(or processors) within the SoC. It is designed to be self
>>> contained without needing software initialization for operation.
>>>
>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> ---
>>>  .../bindings/mailbox/ti,message-manager.txt        | 72 ++++++++++++++++++++++
>>>  1 file changed, 72 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt b/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
>>> new file mode 100644
>>> index 000000000000..f3d73b0b3c66
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
>>> @@ -0,0 +1,72 @@
>>> +Texas Instruments' Message Manager Driver
>>> +========================================
>>> +
>>> +The Texas Instruments' Message Manager is a mailbox controller that has
>>> +configurable queues selectable at SoC(System on Chip) integration. The Message
>>> +manager is broken up into queues in different address regions that are called
>>> +"proxies" - each instance is unidirectional and is instantiated at SoC
>>> +integration level to indicate receive or transmit path.
>>> +
>>> +Message Manager Device Node:
>>> +===========================
>>> +
>>> +Required properties:
>>> +--------------------
>>> +- compatible:               Shall be:
>>> +                            "ti,k2g-message-manager"
>>> +                            "ti,message-manager"
>>
>> Given that queues are configurable at integration time, does a generic
>> property really make sense here?
>>
>>> +- reg-names                 queue_proxy_region - Map the queue Proxy region
>>> +                    queue_state_debug_region - Map the queue state debug
>>> +                    region.
>>> +- reg:                      Contains the register map per reg-names
>>> +- #mbox-cells               Shall be 1
>>
>> And the value contained is what?
>>
>>> +
>>> +Child Nodes:
>>> +============
>>> +A child node is used for representing the actual queue device that is
>>> +used for the communication between the host processor and a remote processor.
>>> +Each child node should have a unique node name across all the different
>>> +message manager device nodes.
>>> +
>>> +Required Properties:
>>> +--------------------
>>> +- ti,queue-id:              Indicates the queue number this node represents
>>> +- ti,proxy-id:              Proxy ID representing the processor in the SoC.
>>
>> What determines these values?
>>
>>> +
>>> +Optional Properties:
>>> +--------------------
>>> +- interrupt-names:  'rx' - indicates a receive interrupt (mandatory ONLY if
>>> +                    this is a receive queue)
>>
>> Kind of pointless if there is only 1.
>>
>>> +- interrupts:               Contains the interrupt information corresponding to
>>> +                    interrupt-names property.
>>> +
>>> +Example:
>>> +--------
>>> +
>>> +    msgmgr: msgmgr@02a00000 {
>>> +            compatible = "ti,k2g-message-manager", "ti,message-manager";
>>> +            #mbox-cells = <1>;
>>> +            reg-names = "queue_proxy_region", "queue_state_debug_region";
>>> +            reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
>>> +
>>> +            msgmgr_proxy_pmmc_tx_prio0: pmmc_tx_prio0 {
>>> +                    ti,queue-id = <0>;
>>> +                    ti,proxy-id = <0>;
>>> +            };
>>> +
>>> +            msgmgr_proxy_pmmc_rx: pmmc_rx {
>>> +                    ti,queue-id = <5>;
>>> +                    ti,proxy-id = <2>;
>>> +                    interrupt-names = "rx";
>>> +                    interrupts = <GIC_SPI 324 IRQ_TYPE_EDGE_RISING>;
>>> +            };
>>> +    };
>>> +
>>> +...
>>> +    pmmc {
>>> +            ...
>>> +            mbox-names = "tx", "rx";
>>> +            mboxes = <&msgmgr &msgmgr_proxy_pmmc_tx>
>>> +                     <&msgmgr &msgmgr_proxy_pmmc_rx>;
>>
>> While I guess this is valid DT, it is a bit strange having the cell
>> values be phandles. Why not just make the queue and proxy ids be the
>> cell values? The interrupts could be moved to the parent and the child
>> nodes eliminated altogether.
>
> The mailbox controller node (msgmgr) and its child nodes all are
> registered with the mailbox core together and the client users request a
> mailbox using the framework API that requires the controller node.
> Usually the channels are given numbers and the request is done based on
> indices, but that meant that we had to introduce a logical device id
> just for the sake of the API. Instead, we just chose to go with the
> phandles.
>
> Doing a queue and proxy id as cell values requires the mailbox
> controller node registration phase to parse through all the client nodes
> with the matching controller node. The current approach was in-line with
> how all the mailbox controller nodes are implemented today, except for
> the usage of phandle instead of an index for the cell-value.
>
>>
>> The alternative would be just drop msgmgr phandle and point to the child
>> nodes. I prefer getting rid of the child nodes though.
>
> The 'mboxes' property is a standard property supported by the mailbox
> framework core, and requires the phandle of the controller node along
> with a cell-value to lookup a channel. So, we cannot use the child nodes
> directly.
>
> regards
> Suman

Re: [PATCH 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Nishanth Menon]

On Mon, Feb 8, 2016 at 1:37 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

Thank you for reviewing the binding.

> On Fri, Feb 05, 2016 at 10:34:03AM -0600, Nishanth Menon wrote:
>> Message Manager is a hardware block used to communicate with various
>> processor systems within certain Texas Instrument's Keystone
>> generation SoCs.
>>
>> This hardware engine is used to transfer messages from various compute
>> entities(or processors) within the SoC. It is designed to be self
>> contained without needing software initialization for operation.
>>
>> Signed-off-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
>> ---
>>  .../bindings/mailbox/ti,message-manager.txt        | 72 ++++++++++++++++++++++
>>  1 file changed, 72 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt b/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
>> new file mode 100644
>> index 000000000000..f3d73b0b3c66
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
>> @@ -0,0 +1,72 @@
>> +Texas Instruments' Message Manager Driver
>> +========================================
>> +
>> +The Texas Instruments' Message Manager is a mailbox controller that has
>> +configurable queues selectable at SoC(System on Chip) integration. The Message
>> +manager is broken up into queues in different address regions that are called
>> +"proxies" - each instance is unidirectional and is instantiated at SoC
>> +integration level to indicate receive or transmit path.
>> +
>> +Message Manager Device Node:
>> +===========================
>> +
>> +Required properties:
>> +--------------------
>> +- compatible:                Shall be:
>> +                             "ti,k2g-message-manager"
>> +                             "ti,message-manager"
>
> Given that queues are configurable at integration time, does a generic
> property really make sense here?

True. I can drop the generic "ti,message-manager" binding.

>> +- reg-names          queue_proxy_region - Map the queue Proxy region
>> +                     queue_state_debug_region - Map the queue state debug
>> +                     region.
>> +- reg:                       Contains the register map per reg-names
>> +- #mbox-cells                Shall be 1
>
> And the value contained is what?

phandle -> I think Suman has already explained in his response.

>> +
>> +Child Nodes:
>> +============
>> +A child node is used for representing the actual queue device that is
>> +used for the communication between the host processor and a remote processor.
>> +Each child node should have a unique node name across all the different
>> +message manager device nodes.
>> +
>> +Required Properties:
>> +--------------------
>> +- ti,queue-id:               Indicates the queue number this node represents
>> +- ti,proxy-id:               Proxy ID representing the processor in the SoC.
>
> What determines these values?

This is SoC integration dependent -> Every queue, proxy combination
has it's own memory region allocated for it.

>> +
>> +Optional Properties:
>> +--------------------
>> +- interrupt-names:   'rx' - indicates a receive interrupt (mandatory ONLY if
>> +                     this is a receive queue)
>
> Kind of pointless if there is only 1.
>

This is primarily to ensure a future compatibility where we are trying
to convince the hardware team to provide an error interrupt per queue
as well for slave usage.
it would probably be pointless at that time to deal with "legacy"
binding defintions when the next hardware ip definition takes place.

>> +- interrupts:                Contains the interrupt information corresponding to
>> +                     interrupt-names property.
>> +
>> +Example:
>> +--------
>> +
>> +     msgmgr: msgmgr@02a00000 {
>> +             compatible = "ti,k2g-message-manager", "ti,message-manager";
>> +             #mbox-cells = <1>;
>> +             reg-names = "queue_proxy_region", "queue_state_debug_region";
>> +             reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
>> +
>> +             msgmgr_proxy_pmmc_tx_prio0: pmmc_tx_prio0 {
>> +                     ti,queue-id = <0>;
>> +                     ti,proxy-id = <0>;
>> +             };
>> +
>> +             msgmgr_proxy_pmmc_rx: pmmc_rx {
>> +                     ti,queue-id = <5>;
>> +                     ti,proxy-id = <2>;
>> +                     interrupt-names = "rx";
>> +                     interrupts = <GIC_SPI 324 IRQ_TYPE_EDGE_RISING>;
>> +             };
>> +     };
>> +
>> +...
>> +     pmmc {
>> +             ...
>> +             mbox-names = "tx", "rx";
>> +             mboxes = <&msgmgr &msgmgr_proxy_pmmc_tx>
>> +                      <&msgmgr &msgmgr_proxy_pmmc_rx>;
>
> While I guess this is valid DT, it is a bit strange having the cell
> values be phandles. Why not just make the queue and proxy ids be the
> cell values? The interrupts could be moved to the parent and the child
> nodes eliminated altogether.
>
> The alternative would be just drop msgmgr phandle and point to the child
> nodes. I prefer getting rid of the child nodes though.

I see that Suman has already responded to this.

--
Regards,
Nishanth Menon
--
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 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Jassi Brar]

On Fri, Feb 5, 2016 at 10:04 PM, Nishanth Menon <nm@ti.com> wrote:
> +
> +       msgmgr: msgmgr@02a00000 {
> +               compatible = "ti,k2g-message-manager", "ti,message-manager";
> +               #mbox-cells = <1>;
> +               reg-names = "queue_proxy_region", "queue_state_debug_region";
> +               reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
> +
> +               msgmgr_proxy_pmmc_tx_prio0: pmmc_tx_prio0 {
> +                       ti,queue-id = <0>;
> +                       ti,proxy-id = <0>;
> +               };
> +
> +               msgmgr_proxy_pmmc_rx: pmmc_rx {
> +                       ti,queue-id = <5>;
> +                       ti,proxy-id = <2>;
> +                       interrupt-names = "rx";
> +                       interrupts = <GIC_SPI 324 IRQ_TYPE_EDGE_RISING>;
> +               };
> +       };
> +
I think we should get rid of consumer specifics from the provider node...

> +...
> +       pmmc {
> +               ...
> +               mbox-names = "tx", "rx";
> +               mboxes = <&msgmgr &msgmgr_proxy_pmmc_tx>
> +                        <&msgmgr &msgmgr_proxy_pmmc_rx>;
> +               ...
> +       };
>
... and have consumers like
       pmmc {
               ...
               mbox-names = "tx", "rx";
               mboxes = <&msgmgr 0 0>
                        <&msgmgr 5 2>;
       };

I leave the IRQ for you to decide how to specify - a 'dummy' or
'valid' always provided as last cell in mboxes or some other way.
(I'll review other patches in detail later)

cheers.

Re: [PATCH 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Nishanth Menon]

On 02/08/2016 10:14 PM, Jassi Brar wrote:

Thanks for the review.

> On Fri, Feb 5, 2016 at 10:04 PM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
>> +
>> +       msgmgr: msgmgr@02a00000 {
>> +               compatible = "ti,k2g-message-manager", "ti,message-manager";
>> +               #mbox-cells = <1>;
>> +               reg-names = "queue_proxy_region", "queue_state_debug_region";
>> +               reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
>> +
>> +               msgmgr_proxy_pmmc_tx_prio0: pmmc_tx_prio0 {
>> +                       ti,queue-id = <0>;
>> +                       ti,proxy-id = <0>;
>> +               };
>> +
>> +               msgmgr_proxy_pmmc_rx: pmmc_rx {
>> +                       ti,queue-id = <5>;
>> +                       ti,proxy-id = <2>;
>> +                       interrupt-names = "rx";
>> +                       interrupts = <GIC_SPI 324 IRQ_TYPE_EDGE_RISING>;
>> +               };
>> +       };
>> +
> I think we should get rid of consumer specifics from the provider node...


If I get rid of the consumer nodes, how do you propose I describe the rx
queue interrupt(s) in the msmgr dt node (Every Rx queue will have it's
own interrupt - and it cannot be reverse computed from queue ID, proxy ID)?


>> +...
>> +       pmmc {
>> +               ...
>> +               mbox-names = "tx", "rx";
>> +               mboxes = <&msgmgr &msgmgr_proxy_pmmc_tx>
>> +                        <&msgmgr &msgmgr_proxy_pmmc_rx>;
>> +               ...
>> +       };
>>
> ... and have consumers like
>        pmmc {
>                ...
>                mbox-names = "tx", "rx";
>                mboxes = <&msgmgr 0 0>
>                         <&msgmgr 5 2>;
>        };
> 
> I leave the IRQ for you to decide how to specify - a 'dummy' or
> 'valid' always provided as last cell in mboxes or some other way.
> (I'll review other patches in detail later)

What do we do with the issues that Suman pointed out in the mailbox
framework itself? Could you respond to that thread[1] as well?


[1] http://marc.info/?l=devicetree&m=145496308418123&w=2

-- 
Regards,
Nishanth Menon
--
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 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Jassi Brar]

On Tue, Feb 9, 2016 at 6:01 PM, Nishanth Menon <nm@ti.com> wrote:
> On 02/08/2016 10:14 PM, Jassi Brar wrote:
>
> Thanks for the review.
>
>> On Fri, Feb 5, 2016 at 10:04 PM, Nishanth Menon <nm@ti.com> wrote:
>>> +
>>> +       msgmgr: msgmgr@02a00000 {
>>> +               compatible = "ti,k2g-message-manager", "ti,message-manager";
>>> +               #mbox-cells = <1>;
>>> +               reg-names = "queue_proxy_region", "queue_state_debug_region";
>>> +               reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
>>> +
>>> +               msgmgr_proxy_pmmc_tx_prio0: pmmc_tx_prio0 {
>>> +                       ti,queue-id = <0>;
>>> +                       ti,proxy-id = <0>;
>>> +               };
>>> +
>>> +               msgmgr_proxy_pmmc_rx: pmmc_rx {
>>> +                       ti,queue-id = <5>;
>>> +                       ti,proxy-id = <2>;
>>> +                       interrupt-names = "rx";
>>> +                       interrupts = <GIC_SPI 32I didn't respond because I think Suman got Rob's point wrong.I didn't respond because I think Suman got Rob's point wrong.4 IRQ_TYPE_EDGE_RISING>;
>>> +               };
>>> +       };
>>> +
>> I think we should get rid of consumer specifics from the provider node...
>
>
> If I get rid of the consumer nodes, how do you propose I describe the rx
> queue interrupt(s) in the msmgr dt node (Every Rx queue will have it's
> own interrupt - and it cannot be reverse computed from queue ID, proxy ID)?
>
One option is to have controller driver construct interrupt name from
queue and proxy ids like

msgmgr: msgmgr@02a00000 {
   ....
     interrupt-names = "irq_5_2", "irq_0_0";     /* irq_<queue-id>_<proxy-id> */
     interrupts = <GIC_SPI 324 IRQ_TYPE_EDGE_RISING>,
                        <GIC_SPI 325 IRQ_TYPE_EDGE_RISING>;
}

and have the consumer specify queue and proxy ids in mboxes property like
 pmmc {
       ....
       mbox-names = "tx", "rx";
       mboxes = <&msgmgr 0 0>
                          <&msgmgr 5 2>;
};


>>> +...
>>> +       pmmc {
>>> +               ...
>>> +               mbox-names = "tx", "rx";
>>> +               mboxes = <&msgmgr &msgmgr_proxy_pmmc_tx>
>>> +                        <&msgmgr &msgmgr_proxy_pmmc_rx>;
>>> +               ...
>>> +       };
>>>
>> ... and have consumers like
>>        pmmc {
>>                ...
>>                mbox-names = "tx", "rx";
>>                mboxes = <&msgmgr 0 0>
>>                         <&msgmgr 5 2>;
>>        };
>>
>> I leave the IRQ for you to decide how to specify - a 'dummy' or
>> 'valid' always provided as last cell in mboxes or some other way.
>> (I'll review other patches in detail later)
>
> What do we do with the issues that Suman pointed out in the mailbox
> framework itself? Could you respond to that thread[1] as well?
>
Phandle of provider in consumer node is quite normal and acceptable.
I think Rob (at least I am) is talking about the second cell where you
specify phandle (&msgmgr_proxy_xxx) instead of values from those child
nodes directly.
Which is what I suggest   mboxes = <&msgmgr 0 0>,  <&msgmgr 5 2>;

Cheers!

Re: [PATCH 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Jassi Brar]

On Tue, Feb 9, 2016 at 8:24 PM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Feb 9, 2016 at 6:01 PM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
>> On 02/08/2016 10:14 PM, Jassi Brar wrote:
>>

>>>> +
>>> I think we should get rid of consumer specifics from the provider node...
>>
>>
>> If I get rid of the consumer nodes, how do you propose I describe the rx
>> queue interrupt(s) in the msmgr dt node (Every Rx queue will have it's
>> own interrupt - and it cannot be reverse computed from queue ID, proxy ID)?
>>
> One option is to have controller driver construct interrupt name from
> queue and proxy ids like
>
> msgmgr: msgmgr@02a00000 {
>    ....
>      interrupt-names = "irq_5_2", "irq_0_0";     /* irq_<queue-id>_<proxy-id> */
>      interrupts = <GIC_SPI 324 IRQ_TYPE_EDGE_RISING>,
>                         <GIC_SPI 325 IRQ_TYPE_EDGE_RISING>;
> }
>
> and have the consumer specify queue and proxy ids in mboxes property like
>  pmmc {
>        ....
>        mbox-names = "tx", "rx";
>        mboxes = <&msgmgr 0 0>
>                           <&msgmgr 5 2>;
> };
>
However if the queue+proxy+interrupt tuple is a hard property of a
channel (which it seems to me now), then probably your original scheme
of chile node phandle is just as fine.

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 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Nishanth Menon]

On Tue, Feb 9, 2016 at 8:54 AM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Feb 9, 2016 at 6:01 PM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
>> On 02/08/2016 10:14 PM, Jassi Brar wrote:
>>
>> Thanks for the review.
>>
>>> On Fri, Feb 5, 2016 at 10:04 PM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
>>>> +
>>>> +       msgmgr: msgmgr@02a00000 {
>>>> +               compatible = "ti,k2g-message-manager", "ti,message-manager";
>>>> +               #mbox-cells = <1>;
>>>> +               reg-names = "queue_proxy_region", "queue_state_debug_region";
>>>> +               reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
>>>> +
>>>> +               msgmgr_proxy_pmmc_tx_prio0: pmmc_tx_prio0 {
>>>> +                       ti,queue-id = <0>;
>>>> +                       ti,proxy-id = <0>;
>>>> +               };
>>>> +
>>>> +               msgmgr_proxy_pmmc_rx: pmmc_rx {
>>>> +                       ti,queue-id = <5>;
>>>> +                       ti,proxy-id = <2>;
>>>> +                       interrupt-names = "rx";
>>>> +                       interrupts = <GIC_SPI 32I didn't respond because I think Suman got Rob's point wrong.I didn't respond because I think Suman got Rob's point wrong.4 IRQ_TYPE_EDGE_RISING>;
>>>> +               };
>>>> +       };
>>>> +
>>> I think we should get rid of consumer specifics from the provider node...
>>
>>
>> If I get rid of the consumer nodes, how do you propose I describe the rx
>> queue interrupt(s) in the msmgr dt node (Every Rx queue will have it's
>> own interrupt - and it cannot be reverse computed from queue ID, proxy ID)?
>>
> One option is to have controller driver construct interrupt name from
> queue and proxy ids like
>
> msgmgr: msgmgr@02a00000 {
>    ....
>      interrupt-names = "irq_5_2", "irq_0_0";     /* irq_<queue-id>_<proxy-id> */
>      interrupts = <GIC_SPI 324 IRQ_TYPE_EDGE_RISING>,
>                         <GIC_SPI 325 IRQ_TYPE_EDGE_RISING>;
> }
>
> and have the consumer specify queue and proxy ids in mboxes property like
>  pmmc {
>        ....
>        mbox-names = "tx", "rx";
>        mboxes = <&msgmgr 0 0>
>                           <&msgmgr 5 2>;
> };
>

I was wondering about the same as well... the best option I can think
of at the moment is as follows:
 - out of a 62*9 (558) all combination possible child nodes, only 11
or so are valid for ARM - this is what is represented as child nodes
to msgmgr. rest of the proxies and queues are inaccessible for ARM.
-  move this "valid queue list" as compatible data in the driver.
- for each of the rx queues identified in the compatible data, I can
do of_irq_get(np, rx_queue_index) without enforcing a naming
convention requirement

If I go with the above approach, I loose ability for non queue
interrupts to be identified appropriately... I think moving valid
queue information to driver compatible data and named irq names might
be the best option for flexibility.

>
>>>> +...
>>>> +       pmmc {
>>>> +               ...
>>>> +               mbox-names = "tx", "rx";
>>>> +               mboxes = <&msgmgr &msgmgr_proxy_pmmc_tx>
>>>> +                        <&msgmgr &msgmgr_proxy_pmmc_rx>;
>>>> +               ...
>>>> +       };
>>>>
>>> ... and have consumers like
>>>        pmmc {
>>>                ...
>>>                mbox-names = "tx", "rx";
>>>                mboxes = <&msgmgr 0 0>
>>>                         <&msgmgr 5 2>;
>>>        };
>>>
>>> I leave the IRQ for you to decide how to specify - a 'dummy' or
>>> 'valid' always provided as last cell in mboxes or some other way.
>>> (I'll review other patches in detail later)
>>
>> What do we do with the issues that Suman pointed out in the mailbox
>> framework itself? Could you respond to that thread[1] as well?
>>
> Phandle of provider in consumer node is quite normal and acceptable.
> I think Rob (at least I am) is talking about the second cell where you
> specify phandle (&msgmgr_proxy_xxx) instead of values from those child
> nodes directly.
> Which is what I suggest   mboxes = <&msgmgr 0 0>,  <&msgmgr 5 2>;

Let me prototype this as part of of_xlate and see if I can pull the
qinst data back out.. obviously one negative will be that I will
register *all* valid channels as part of probe.. at least based on
initial code i wrote today morning..

-- 
---
Regards,
Nishanth Menon
--
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 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Nishanth Menon]

On 09:43-20160209, Nishanth Menon wrote:
> On Tue, Feb 9, 2016 at 8:54 AM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
[..]
> Let me prototype this as part of of_xlate and see if I can pull the
> qinst data back out.. obviously one negative will be that I will
> register *all* valid channels as part of probe.. at least based on
> initial code i wrote today morning..

OK - I believe I have it working now. How does the following look? If
this looks fine to you, then I will post a v2 including the driver
update.
Changes here:
	- dropped the generic message-manager compatible
	- dropped child nodes
	- moved the valid queue information to driver (no longer in dts)
	- rx interrupts per SoC are explicitly named list in binding(and
	  dts)

Texas Instruments' Message Manager Driver
========================================

The Texas Instruments' Message Manager is a mailbox controller that has
configurable queues selectable at SoC(System on Chip) integration. The Message
manager is broken up into queues in different address regions that are called
"proxies" - each instance is unidirectional and is instantiated at SoC
integration level to indicate receive or transmit path.

Message Manager Device Node:
===========================
Required properties:
--------------------
- compatible:		Shall be: "ti,k2g-message-manager"
- reg-names 		queue_proxy_region - Map the queue proxy region.
			queue_state_debug_region - Map the queue state debug
			region.
- reg:			Contains the register map per reg-names.
- #mbox-cells		Shall be 2. Contains the queue ID and proxy ID in that
		        order referring to the transfer path.
- interrupt-names:	Contains interrupt names matching the rx transfer path
			for a given SoC. Receive interrupts shall be of the
			format: "rx_<QID>_<PID>".
			For ti,k2g-message-manager, this shall contain:
				"rx_005_002", "rx_057_002"
- interrupts:		Contains the interrupt information corresponding to
			interrupt-names property.

Example(K2G):
------------

	msgmgr: msgmgr@02a00000 {
		compatible = "ti,k2g-message-manager";
		#mbox-cells = <2>;
		reg-names = "queue_proxy_region", "queue_state_debug_region";
		reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
		interrupt-names = "rx_005_002",
				  "rx_057_002";
		interrupts = <GIC_SPI 324 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 327 IRQ_TYPE_LEVEL_HIGH>;
	};

	pmmc: pmmc {
		[...]
		mbox-names = "rx", "tx";
		# RX queue ID is 5, proxy ID is 2
		# TX queue ID is 0, proxy ID is 0
		mboxes= <&msgmgr 5 2>,
			<&msgmgr 0 0>;
		[...]
	};
-- 
Regards,
Nishanth Menon
--
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 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Suman Anna]

Hi Nishanth,

On 02/09/2016 12:10 PM, Menon, Nishanth wrote:
> On 09:43-20160209, Nishanth Menon wrote:
>> On Tue, Feb 9, 2016 at 8:54 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> [..]
>> Let me prototype this as part of of_xlate and see if I can pull the
>> qinst data back out.. obviously one negative will be that I will
>> register *all* valid channels as part of probe.. at least based on
>> initial code i wrote today morning..
> 
> OK - I believe I have it working now. How does the following look? If
> this looks fine to you, then I will post a v2 including the driver
> update.
> Changes here:
> 	- dropped the generic message-manager compatible
> 	- dropped child nodes
> 	- moved the valid queue information to driver (no longer in dts)
> 	- rx interrupts per SoC are explicitly named list in binding(and
> 	  dts)
> 
> Texas Instruments' Message Manager Driver
> ========================================
> 
> The Texas Instruments' Message Manager is a mailbox controller that has
> configurable queues selectable at SoC(System on Chip) integration. The Message
> manager is broken up into queues in different address regions that are called
> "proxies" - each instance is unidirectional and is instantiated at SoC
> integration level to indicate receive or transmit path.
> 
> Message Manager Device Node:
> ===========================
> Required properties:
> --------------------
> - compatible:		Shall be: "ti,k2g-message-manager"
> - reg-names 		queue_proxy_region - Map the queue proxy region.
> 			queue_state_debug_region - Map the queue state debug
> 			region.
> - reg:			Contains the register map per reg-names.
> - #mbox-cells		Shall be 2. Contains the queue ID and proxy ID in that
> 		        order referring to the transfer path.
> - interrupt-names:	Contains interrupt names matching the rx transfer path
> 			for a given SoC. Receive interrupts shall be of the
> 			format: "rx_<QID>_<PID>".
> 			For ti,k2g-message-manager, this shall contain:
> 				"rx_005_002", "rx_057_002"

Are these the only two that will ever be supported for
ti,k2g-message-manager or there can be more in the future? You did
mention about 11 possible valid qid_pid values for the ARM, and the
driver match data is
registering all of those mboxes.

> - interrupts:		Contains the interrupt information corresponding to
> 			interrupt-names property.
> 
> Example(K2G):
> ------------
> 
> 	msgmgr: msgmgr@02a00000 {
> 		compatible = "ti,k2g-message-manager";
> 		#mbox-cells = <2>;
> 		reg-names = "queue_proxy_region", "queue_state_debug_region";
> 		reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
> 		interrupt-names = "rx_005_002",
> 				  "rx_057_002";
> 		interrupts = <GIC_SPI 324 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 327 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 
> 	pmmc: pmmc {
> 		[...]
> 		mbox-names = "rx", "tx";
> 		# RX queue ID is 5, proxy ID is 2
> 		# TX queue ID is 0, proxy ID is 0
> 		mboxes= <&msgmgr 5 2>,
> 			<&msgmgr 0 0>;
> 		[...]
> 	};

Yeah, this will also work. I am fine with this approach - my only
concern was that the probe doesn't have to go parsing all the nodes to
be able to register the mailbox channels with the mailbox core. Since
you are registering them using match data, that is not a concern anymore.

Anyway, will let Rob give the final ACK.

regards
Suman

Re: [PATCH 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Nishanth Menon]

On Wed, Feb 10, 2016 at 2:13 PM, Suman Anna <s-anna-l0cyMroinI0@public.gmane.org> wrote:
> Hi Nishanth,
>
> On 02/09/2016 12:10 PM, Menon, Nishanth wrote:
>> On 09:43-20160209, Nishanth Menon wrote:
>>> On Tue, Feb 9, 2016 at 8:54 AM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> [..]
>>> Let me prototype this as part of of_xlate and see if I can pull the
>>> qinst data back out.. obviously one negative will be that I will
>>> register *all* valid channels as part of probe.. at least based on
>>> initial code i wrote today morning..
>>
>> OK - I believe I have it working now. How does the following look? If
>> this looks fine to you, then I will post a v2 including the driver
>> update.
>> Changes here:
>>       - dropped the generic message-manager compatible
>>       - dropped child nodes
>>       - moved the valid queue information to driver (no longer in dts)
>>       - rx interrupts per SoC are explicitly named list in binding(and
>>         dts)
>>
>> Texas Instruments' Message Manager Driver
>> ========================================
>>
>> The Texas Instruments' Message Manager is a mailbox controller that has
>> configurable queues selectable at SoC(System on Chip) integration. The Message
>> manager is broken up into queues in different address regions that are called
>> "proxies" - each instance is unidirectional and is instantiated at SoC
>> integration level to indicate receive or transmit path.
>>
>> Message Manager Device Node:
>> ===========================
>> Required properties:
>> --------------------
>> - compatible:         Shall be: "ti,k2g-message-manager"
>> - reg-names           queue_proxy_region - Map the queue proxy region.
>>                       queue_state_debug_region - Map the queue state debug
>>                       region.
>> - reg:                        Contains the register map per reg-names.
>> - #mbox-cells         Shall be 2. Contains the queue ID and proxy ID in that
>>                       order referring to the transfer path.
>> - interrupt-names:    Contains interrupt names matching the rx transfer path
>>                       for a given SoC. Receive interrupts shall be of the
>>                       format: "rx_<QID>_<PID>".
>>                       For ti,k2g-message-manager, this shall contain:
>>                               "rx_005_002", "rx_057_002"
>
> Are these the only two that will ever be supported for
> ti,k2g-message-manager or there can be more in the future? You did
> mention about 11 possible valid qid_pid values for the ARM, and the
> driver match data is
> registering all of those mboxes.


As per the internal TRM, there *only* 2 rx interrupts to the public
world ARM on K2G. I just noticed that the public TRM conveniently
stripped out every single useful information and replaced with
TRM!!!!!*&^*&^&*%&^%&%&!!! Anyways, checked internal documentation to
verify as well. we have multiple output paths to various compute
systems, but only 2 receive paths.

Ofcourse a different SoC will have different integration, which why
the interrupt list will have to be compatible property.

>
>> - interrupts:         Contains the interrupt information corresponding to
>>                       interrupt-names property.
>>
>> Example(K2G):
>> ------------
>>
>>       msgmgr: msgmgr@02a00000 {
>>               compatible = "ti,k2g-message-manager";
>>               #mbox-cells = <2>;
>>               reg-names = "queue_proxy_region", "queue_state_debug_region";
>>               reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
>>               interrupt-names = "rx_005_002",
>>                                 "rx_057_002";
>>               interrupts = <GIC_SPI 324 IRQ_TYPE_LEVEL_HIGH>,
>>                            <GIC_SPI 327 IRQ_TYPE_LEVEL_HIGH>;
>>       };
>>
>>       pmmc: pmmc {
>>               [...]
>>               mbox-names = "rx", "tx";
>>               # RX queue ID is 5, proxy ID is 2
>>               # TX queue ID is 0, proxy ID is 0
>>               mboxes= <&msgmgr 5 2>,
>>                       <&msgmgr 0 0>;
>>               [...]
>>       };
>
> Yeah, this will also work. I am fine with this approach - my only
> concern was that the probe doesn't have to go parsing all the nodes to
> be able to register the mailbox channels with the mailbox core. Since
> you are registering them using match data, that is not a concern anymore.
>
> Anyway, will let Rob give the final ACK.


Thanks for the review. will wait for Rob and Jassi or post a new rev on monday.

---
Regards,
Nishanth Menon
--
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 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Jassi Brar]

On Tue, Feb 9, 2016 at 11:40 PM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
> On 09:43-20160209, Nishanth Menon wrote:
>> On Tue, Feb 9, 2016 at 8:54 AM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> [..]
>> Let me prototype this as part of of_xlate and see if I can pull the
>> qinst data back out.. obviously one negative will be that I will
>> register *all* valid channels as part of probe.. at least based on
>> initial code i wrote today morning..
>
> OK - I believe I have it working now. How does the following look? If
> this looks fine to you, then I will post a v2 including the driver
> update.
> Changes here:
>         - dropped the generic message-manager compatible
>         - dropped child nodes
>         - moved the valid queue information to driver (no longer in dts)
>         - rx interrupts per SoC are explicitly named list in binding(and
>           dts)
>
> Texas Instruments' Message Manager Driver
> ========================================
>
> The Texas Instruments' Message Manager is a mailbox controller that has
> configurable queues selectable at SoC(System on Chip) integration. The Message
> manager is broken up into queues in different address regions that are called
> "proxies" - each instance is unidirectional and is instantiated at SoC
> integration level to indicate receive or transmit path.
>
> Message Manager Device Node:
> ===========================
> Required properties:
> --------------------
> - compatible:           Shall be: "ti,k2g-message-manager"
> - reg-names             queue_proxy_region - Map the queue proxy region.
>                         queue_state_debug_region - Map the queue state debug
>                         region.
> - reg:                  Contains the register map per reg-names.
> - #mbox-cells           Shall be 2. Contains the queue ID and proxy ID in that
>                         order referring to the transfer path.
> - interrupt-names:      Contains interrupt names matching the rx transfer path
>                         for a given SoC. Receive interrupts shall be of the
>                         format: "rx_<QID>_<PID>".
>                         For ti,k2g-message-manager, this shall contain:
>                                 "rx_005_002", "rx_057_002"
> - interrupts:           Contains the interrupt information corresponding to
>                         interrupt-names property.
>
> Example(K2G):
> ------------
>
>         msgmgr: msgmgr@02a00000 {
>                 compatible = "ti,k2g-message-manager";
>                 #mbox-cells = <2>;
>                 reg-names = "queue_proxy_region", "queue_state_debug_region";
>                 reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
>                 interrupt-names = "rx_005_002",
>                                   "rx_057_002";
>
Looking at figure in page-1445, it seems QID is the h/w channel id,
while proxy is its programming parameter. So maybe we need to list all
the ARM irq's as a list here, matched only by the qid asked by the
consumer ... assuming no two channels could have the same qid (?).

  interrupt-names = "irq_005", "irq_037", "irq_049", "irq_057",
"perr", "ferr", "eerr";

I may be slightly off, but the idea remains to not have to encode any
consumer specific info in the provider node.

>         pmmc: pmmc {
>                 [...]
>                 mbox-names = "rx", "tx";
>                 # RX queue ID is 5, proxy ID is 2
>                 # TX queue ID is 0, proxy ID is 0
>                 mboxes= <&msgmgr 5 2>,
>                         <&msgmgr 0 0>;
>                 [...]
>         };
--
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 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Nishanth Menon]

Hi Jassi,

On 02/10/2016 10:23 PM, Jassi Brar wrote:
[...]


Thanks for taking the time and checking the TRM, I apologize that the
actual details of the hardware block which was supposed to be in
sections 8.1.3 and 8.1.4 has unfortunately been dropped since the last
time I reviewed in the spec Vs what actually went out into public
domain! I do realize the problem of doing a review without comprehensive
and accurate documentation - ugghh.. :(

But, I am trying to get our internal guys to upload the proper TRM
chapter in public domain -> hopefully we will get it done some time soon.


>>         msgmgr: msgmgr@02a00000 {
>>                 compatible = "ti,k2g-message-manager";
>>                 #mbox-cells = <2>;
>>                 reg-names = "queue_proxy_region", "queue_state_debug_region";
>>                 reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
>>                 interrupt-names = "rx_005_002",
>>                                   "rx_057_002";
>>
> Looking at figure in page-1445, it seems QID is the h/w channel id,
> while proxy is its programming parameter. So maybe we need to list all
> the ARM irq's as a list here, matched only by the qid asked by the
> consumer ... assuming no two channels could have the same qid (?).

The overall story is something like what you already figured out..
message manager has a queue engine and a ram for data buffers, and n
queues. Each of these queues have a memory map corresponding to the
processor view.. we can call that programming paramater as well.

>   interrupt-names = "irq_005", "irq_037", "irq_049", "irq_057",
> "perr", "ferr", "eerr";

proxy error (perr), free index error(ferr) and ECC error(eerr) cannot be
handled by a slave, since it involves controlling a shared register set
for a single message manager instance. in the case of K2G, the master of
the message manager is actually PMMC, and not the compute processors -
it has error handling logic to handle things there - a slave can only
report these errors without ability to even expect reliable detection
(for example PMMC reacting even before any of  these cores have come up
from low power state).

irq_37 and irq_49 go to the secure world and we have no access from ARM
"non secure" world. the "missing documentation" would have helped
clarify that :(..

> 
> I may be slightly off, but the idea remains to not have to encode any
> consumer specific info in the provider node.

I do realize the reasoning behind your suggestion here. the reasoning
for providing rx_qid_pid as the interrupt name was as follows: I was
hoping to get a future SoC to provide proxy specific error instead of a
global error which is really useless since the processor generating
error should be the guy actually be notified.. queue specific interrupts
as well.. the reason for naming interrupts with the proxy id information
was primarily to let the dtb ABI stay compatible with only additional
properties defined when the new SoC gets supported.

I can make it compatible for today's SoC, but based on what i explained,
how about just "rx_<qid>" for the interrupt names?
interrupt-names = "rx_005", "rx_057" (I kinda feel using "irq" for
interrupt-names is actually redundant information)?

*if* i manage to convince to get a new IP with proxy specific
interrupts, then "perr_qid_pid" could then be introduced for that new
compatible type..

[...]
-- 
Regards,
Nishanth Menon
--
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 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Jassi Brar]

On Thu, Feb 11, 2016 at 10:33 AM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
> Hi Jassi,
>
> On 02/10/2016 10:23 PM, Jassi Brar wrote:
> [...]
>
>
> Thanks for taking the time and checking the TRM, I apologize that the
> actual details of the hardware block which was supposed to be in
> sections 8.1.3 and 8.1.4 has unfortunately been dropped since the last
> time I reviewed in the spec Vs what actually went out into public
> domain! I do realize the problem of doing a review without comprehensive
> and accurate documentation - ugghh.. :(
>
> But, I am trying to get our internal guys to upload the proper TRM
> chapter in public domain -> hopefully we will get it done some time soon.
>
>
>>>         msgmgr: msgmgr@02a00000 {
>>>                 compatible = "ti,k2g-message-manager";
>>>                 #mbox-cells = <2>;
>>>                 reg-names = "queue_proxy_region", "queue_state_debug_region";
>>>                 reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
>>>                 interrupt-names = "rx_005_002",
>>>                                   "rx_057_002";
>>>
>> Looking at figure in page-1445, it seems QID is the h/w channel id,
>> while proxy is its programming parameter. So maybe we need to list all
>> the ARM irq's as a list here, matched only by the qid asked by the
>> consumer ... assuming no two channels could have the same qid (?).
>
> The overall story is something like what you already figured out..
> message manager has a queue engine and a ram for data buffers, and n
> queues. Each of these queues have a memory map corresponding to the
> processor view.. we can call that programming paramater as well.
>
>>   interrupt-names = "irq_005", "irq_037", "irq_049", "irq_057",
>> "perr", "ferr", "eerr";
>
> proxy error (perr), free index error(ferr) and ECC error(eerr) cannot be
> handled by a slave, since it involves controlling a shared register set
> for a single message manager instance. in the case of K2G, the master of
> the message manager is actually PMMC, and not the compute processors -
> it has error handling logic to handle things there - a slave can only
> report these errors without ability to even expect reliable detection
> (for example PMMC reacting even before any of  these cores have come up
> from low power state).
>
> irq_37 and irq_49 go to the secure world and we have no access from ARM
> "non secure" world. the "missing documentation" would have helped
> clarify that :(..
>
>>
>> I may be slightly off, but the idea remains to not have to encode any
>> consumer specific info in the provider node.
>
> I do realize the reasoning behind your suggestion here. the reasoning
> for providing rx_qid_pid as the interrupt name was as follows: I was
> hoping to get a future SoC to provide proxy specific error instead of a
> global error which is really useless since the processor generating
> error should be the guy actually be notified.. queue specific interrupts
> as well.. the reason for naming interrupts with the proxy id information
> was primarily to let the dtb ABI stay compatible with only additional
> properties defined when the new SoC gets supported.
>
> I can make it compatible for today's SoC, but based on what i explained,
> how about just "rx_<qid>" for the interrupt names?
> interrupt-names = "rx_005", "rx_057" (I kinda feel using "irq" for
> interrupt-names is actually redundant information)?
>
Feel free to name the interrupts "rx_" instead of "irq_" ... assuming
the interrupts correspond to only reception of data.

> *if* i manage to convince to get a new IP with proxy specific
> interrupts, then "perr_qid_pid" could then be introduced for that new
> compatible type..
>
Yeah, let us cross the bridge when we come to it. Let us not add any
feature/binding that has no user today.

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 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Nishanth Menon]

Jassi,

On Fri, Feb 26, 2016 at 5:59 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Thu, Feb 11, 2016 at 10:33 AM, Nishanth Menon <nm@ti.com> wrote:
[...]
>> I can make it compatible for today's SoC, but based on what i explained,
>> how about just "rx_<qid>" for the interrupt names?
>> interrupt-names = "rx_005", "rx_057" (I kinda feel using "irq" for
>> interrupt-names is actually redundant information)?
>>
> Feel free to name the interrupts "rx_" instead of "irq_" ... assuming
> the interrupts correspond to only reception of data.
>

Thanks for your reviews. I have now posted a v2: hopefully these meet the needs.

https://patchwork.kernel.org/patch/8442641/
https://patchwork.kernel.org/patch/8442671/


-- 
---
Regards,
Nishanth Menon

[PATCH 2/2] mailbox: Introduce TI message manager driver

[Nishanth Menon]

Support for TI Message Manager Module. This hardware block manages a
bunch of hardware queues meant for communication between processor
entities.

Clients sitting on top of this would manage the required protocol
for communicating with the counterpart entities.

For more details on TI Message Manager hardware block, see documentation
that is available here: http://www.ti.com/lit/ug/spruhy8/spruhy8.pdf
Chapter 8.1(Message Manager)

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 drivers/mailbox/Kconfig     |  11 +
 drivers/mailbox/Makefile    |   2 +
 drivers/mailbox/ti-msgmgr.c | 603 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/ti-msgmgr.h   |  35 +++
 4 files changed, 651 insertions(+)
 create mode 100644 drivers/mailbox/ti-msgmgr.c
 create mode 100644 include/linux/ti-msgmgr.h

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 546d05f4358a..5da85ae48a59 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -78,6 +78,17 @@ config STI_MBOX
 	  Mailbox implementation for STMicroelectonics family chips with
 	  hardware for interprocessor communication.
 
+config TI_MESSAGE_MANAGER
+	tristate "Texas Instruments Message Manager Driver"
+	depends on ARCH_KEYSTONE
+	help
+	  An implementation of Message Manager slave driver for Keystone
+	  architecture SoCs from Texas Instruments. Message Manager is a
+	  communication entity found on few of Texas Instrument's keystone
+	  architecture SoCs. These may be used for communication between
+	  multiple processors within the SoC. Select this driver if your
+	  platform has support for the hardware block.
+
 config MAILBOX_TEST
 	tristate "Mailbox Test Client"
 	depends on OF
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 92435ef11f26..a9c2899e44ff 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -17,3 +17,5 @@ obj-$(CONFIG_ALTERA_MBOX)	+= mailbox-altera.o
 obj-$(CONFIG_BCM2835_MBOX)	+= bcm2835-mailbox.o
 
 obj-$(CONFIG_STI_MBOX)		+= mailbox-sti.o
+
+obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
diff --git a/drivers/mailbox/ti-msgmgr.c b/drivers/mailbox/ti-msgmgr.c
new file mode 100644
index 000000000000..46f1b0adbedd
--- /dev/null
+++ b/drivers/mailbox/ti-msgmgr.c
@@ -0,0 +1,603 @@
+/*
+ * Texas Instruments' Message Manager Driver
+ *
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ *	Nishanth Menon
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include <linux/device.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_device.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/ti-msgmgr.h>
+
+#define Q_DATA_OFFSET(proxy, queue, reg)	\
+		     ((0x10000 * (proxy)) + (0x80 * (queue)) + ((reg) * 4))
+#define Q_STATE_OFFSET(queue)			((queue) * 0x4)
+#define Q_STATE_ENTRY_COUNT_MASK		(0xFFF000)
+
+/**
+ * struct ti_msgmgr_desc - Description of message manager integration
+ * @queue_count:	Number of Queues
+ * @max_message_size:	Message size in bytes
+ * @max_messages:	Number of messages
+ * @q_slices:		Number of queue engines
+ * @q_proxies:		Number of queue proxies per page
+ * @data_first_reg:	First data register for proxy data region
+ * @data_last_reg:	Last data register for proxy data region
+ * @tx_polled:		Do I need to use polled mechanism for tx
+ * @tx_poll_timeout_ms: Timeout in ms if polled
+ *
+ * This structure is used in of match data to describe how integration
+ * for a specific compatible SoC is done.
+ */
+struct ti_msgmgr_desc {
+	u8 queue_count;
+	u8 max_message_size;
+	u8 max_messages;
+	u8 q_slices;
+	u8 q_proxies;
+	u8 data_first_reg;
+	u8 data_last_reg;
+	bool tx_polled;
+	int tx_poll_timeout_ms;
+};
+
+/**
+ * struct ti_queue_inst - Description of a queue instance
+ * @name:	Queue Name
+ * @queue_id:	Queue Identifier as mapped on SoC
+ * @proxy_id:	Proxy Identifier as mapped on SoC
+ * @irq:	IRQ for Rx Queue
+ * @is_tx:	'true' if transmit queue, else, 'false'
+ * @queue_buff_start: First register of Data Buffer
+ * @queue_buff_end: Last (or confirmation) register of Data buffer
+ * @queue_state: Queue status register
+ * @chan:	Mailbox channel
+ * @rx_buff:	Receive buffer pointer allocated at probe, max_message_size
+ */
+struct ti_queue_inst {
+	const char *name;
+	u32 queue_id;
+	u32 proxy_id;
+	int irq;
+	bool is_tx;
+	void __iomem *queue_buff_start;
+	void __iomem *queue_buff_end;
+	void __iomem *queue_state;
+	struct mbox_chan *chan;
+	u32 *rx_buff;
+};
+
+/**
+ * struct ti_msgmgr_inst - Description of a Message Manager Instance
+ * @dev:	device pointer corresponding to the Message Manager instance
+ * @desc:	Description of the SoC integration
+ * @queue_proxy_region:	Queue proxy region where queue buffers are located
+ * @queue_state_debug_region:	Queue status register regions
+ * @num_valid_queues:	Number of valid queues defined for the processor
+ *		Note: other queues are probably reserved for other processors
+ *		in the SoC.
+ * @qinsts:	Array of valid Queue Instances for the Processor
+ * @mbox:	Mailbox Controller
+ * @chans:	Array for channels corresponding to the Queue Instances.
+ */
+struct ti_msgmgr_inst {
+	struct device *dev;
+	const struct ti_msgmgr_desc *desc;
+	void __iomem *queue_proxy_region;
+	void __iomem *queue_state_debug_region;
+	u8 num_valid_queues;
+	struct ti_queue_inst *qinsts;
+	struct mbox_controller mbox;
+	struct mbox_chan *chans;
+};
+
+/**
+ * ti_msgmgr_queue_get_num_messages() - Get the number of pending messages
+ * @qinst:	Queue instance for which we check the number of pending messages
+ *
+ * Return: number of messages pending in the queue (0 == no pending messages)
+ */
+static inline int ti_msgmgr_queue_get_num_messages(struct ti_queue_inst *qinst)
+{
+	u32 val;
+
+	/*
+	 * We cannot use relaxed operation here - update may happen
+	 * real-time.
+	 */
+	val = readl(qinst->queue_state) & Q_STATE_ENTRY_COUNT_MASK;
+	val >>= __ffs(Q_STATE_ENTRY_COUNT_MASK);
+
+	return val;
+}
+
+/**
+ * ti_msgmgr_queue_rx_interrupt() - Interrupt handler for receive Queue
+ * @irq:	Interrupt number
+ * @p:		Channel Pointer
+ *
+ * Return: -EINVAL if there is no instance
+ * IRQ_NONE if the interrupt is not ours.
+ * IRQ_HANDLED if the rx interrupt was successfully handled.
+ */
+static irqreturn_t ti_msgmgr_queue_rx_interrupt(int irq, void *p)
+{
+	struct mbox_chan *chan = p;
+	struct device *dev = chan->mbox->dev;
+	struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
+	struct ti_queue_inst *qinst = chan->con_priv;
+	const struct ti_msgmgr_desc *desc;
+	int msg_count, num_words;
+	struct ti_msgmgr_message message;
+	void __iomem *data_reg;
+	u32 *word_data;
+
+	if (WARN_ON(!inst)) {
+		dev_err(dev, "no platform drv data??\n");
+		return -EINVAL;
+	}
+
+	/* Do I have an invalid interrupt source? */
+	if (qinst->is_tx) {
+		dev_err(dev, "Cannot handle rx interrupt on tx channel %s\n",
+			qinst->name);
+		return IRQ_NONE;
+	}
+
+	/* Do I actually have messages to read? */
+	msg_count = ti_msgmgr_queue_get_num_messages(qinst);
+	if (!msg_count) {
+		dev_dbg(dev, "Spurious event - 0 pending data!\n");
+		return IRQ_NONE;
+	}
+
+	/*
+	 * I have no idea about the protocol being used to communicate with the
+	 * remote producer - 0 is valid data, so I wont make a judgement of how
+	 * many bytes I should be reading. Let the client figure this out.. I
+	 * just read the full message and pass it on..
+	 */
+	desc = inst->desc;
+	message.len = desc->max_message_size;
+	message.buf = (u8 *)qinst->rx_buff;
+
+	/*
+	 * NOTE about register access involved here:
+	 * the hardware block is implemented with 32bit access operations and no
+	 * support for data splitting.  We don't want the hardware to misbehave
+	 * with sub 32bit access - For example: if the last register read is
+	 * split into byte wise access, it can result in the queue getting
+	 * stuck or indeterminate behavior. Hence, we do not use memcpy_fromio
+	 * here.
+	 * Also note that the final register read automatically marks the
+	 * queue message as read.
+	 */
+	for (data_reg = qinst->queue_buff_start, word_data = qinst->rx_buff,
+	     num_words = (desc->max_message_size / sizeof(u32));
+	     num_words; num_words--, data_reg += sizeof(u32), word_data++)
+		*word_data = readl(data_reg);
+
+	/*
+	 * Last register read automatically clears the IRQ if only 1 message
+	 * is pending - so send the data up the stack..
+	 * NOTE: Client is expected to be as optimal as possible, since
+	 * we invoke the handler in IRQ context.
+	 */
+	mbox_chan_received_data(chan, (void *)&message);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * ti_msgmgr_queue_peek_data() - Peek to see if there are any rx messages.
+ * @chan:	Channel Pointer
+ *
+ * Return: 'true' if there is pending rx data, 'false' if there is none.
+ */
+static bool ti_msgmgr_queue_peek_data(struct mbox_chan *chan)
+{
+	struct ti_queue_inst *qinst = chan->con_priv;
+	int msg_count;
+
+	if (qinst->is_tx)
+		return false;
+
+	msg_count = ti_msgmgr_queue_get_num_messages(qinst);
+
+	return msg_count ? true : false;
+}
+
+/**
+ * ti_msgmgr_last_tx_done() - See if all the tx messages are sent
+ * @chan:	Channel pointer
+ *
+ * Return: 'true' is no pending tx data, 'false' if there are any.
+ */
+static bool ti_msgmgr_last_tx_done(struct mbox_chan *chan)
+{
+	struct ti_queue_inst *qinst = chan->con_priv;
+	int msg_count;
+
+	if (!qinst->is_tx)
+		return false;
+
+	msg_count = ti_msgmgr_queue_get_num_messages(qinst);
+
+	/* if we have any messages pending.. */
+	return msg_count ? false : true;
+}
+
+/**
+ * ti_msgmgr_send_data() - Send data
+ * @chan:	Channel Pointer
+ * @data:	ti_msgmgr_message * Message Pointer
+ *
+ * Return: 0 if all goes good, else appropriate error messages.
+ */
+static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
+{
+	struct device *dev = chan->mbox->dev;
+	struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
+	const struct ti_msgmgr_desc *desc;
+	struct ti_queue_inst *qinst = chan->con_priv;
+	int msg_count, num_words, trail_bytes;
+	struct ti_msgmgr_message *message = data;
+	void __iomem *data_reg;
+	u32 *word_data;
+
+	if (WARN_ON(!inst)) {
+		dev_err(dev, "no platform drv data??\n");
+		return -EINVAL;
+	}
+	desc = inst->desc;
+
+	if (desc->max_message_size < message->len) {
+		dev_err(dev, "Queue %s message length %d > max %d\n",
+			qinst->name, message->len, desc->max_message_size);
+		return -EINVAL;
+	}
+
+	/* Are we able to send this or not? */
+	msg_count = ti_msgmgr_queue_get_num_messages(qinst);
+	if (msg_count >= desc->max_messages) {
+		dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
+			 msg_count);
+		return -EBUSY;
+	}
+
+	/*
+	 * NOTE about register access involved here:
+	 * the hardware block is implemented with 32bit access operations and no
+	 * support for data splitting.  We don't want the hardware to misbehave
+	 * with sub 32bit access - For example: if the last register write is
+	 * split into byte wise access, it can result in the queue getting
+	 * stuck or dummy messages being transmitted or indeterminate behavior.
+	 * Hence, we do not use memcpy_toio here.
+	 */
+	for (data_reg = qinst->queue_buff_start,
+	     num_words = message->len / sizeof(u32),
+	     word_data = (u32 *)message->buf;
+	     num_words; num_words--, data_reg += sizeof(u32), word_data++)
+		writel(*word_data, data_reg);
+
+	trail_bytes = message->len % sizeof(u32);
+	if (trail_bytes) {
+		u32 data_trail = *word_data;
+
+		/* Ensure all unused data is 0 */
+		data_trail &= 0xFFFFFFFF >> (8 * (sizeof(u32) - trail_bytes));
+		writel(data_trail, data_reg);
+		data_reg++;
+	}
+	/*
+	 * 'data_reg' indicates next register to write. If we did not already
+	 * write on tx complete reg(last reg), we must do so for transmit
+	 */
+	if (data_reg <= qinst->queue_buff_end)
+		writel(0, qinst->queue_buff_end);
+
+	return 0;
+}
+
+/**
+ * ti_msgmgr_queue_startup() - Startup queue
+ * @chan:	Channel pointer
+ *
+ * Return: 0 if all goes good, else return corresponding error message
+ */
+static int ti_msgmgr_queue_startup(struct mbox_chan *chan)
+{
+	struct ti_queue_inst *qinst = chan->con_priv;
+	struct device *dev = chan->mbox->dev;
+	int ret;
+
+	if (!qinst->is_tx) {
+		/*
+		 * With the expectation that the IRQ might be shared in
+		 * future
+		 */
+		ret = request_irq(qinst->irq, ti_msgmgr_queue_rx_interrupt,
+				  IRQF_SHARED, qinst->name, chan);
+		if (ret) {
+			dev_err(dev, "Unable to get IRQ %d on %s(res=%d)\n",
+				qinst->irq, qinst->name, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * ti_msgmgr_queue_shutdown() - Shutdown the queue
+ * @chan:	Channel pointer
+ */
+static void ti_msgmgr_queue_shutdown(struct mbox_chan *chan)
+{
+	struct ti_queue_inst *qinst = chan->con_priv;
+
+	if (!qinst->is_tx)
+		free_irq(qinst->irq, chan);
+}
+
+/**
+ * ti_msgmgr_of_xlate() - Translation of phandle to queue
+ * @mbox:	Mailbox controller
+ * @p:		phandle pointer
+ *
+ * Return: Mailbox channel corresponding to the queue, else return error
+ * pointer.
+ */
+static struct mbox_chan *ti_msgmgr_of_xlate(struct mbox_controller *mbox,
+					    const struct of_phandle_args *p)
+{
+	struct ti_msgmgr_inst *inst;
+	struct device_node *np;
+	phandle phandle = p->args[0];
+	struct ti_queue_inst *qinst;
+	int i;
+	struct mbox_chan *chan = ERR_PTR(-ENOENT);
+
+	inst = container_of(mbox, struct ti_msgmgr_inst, mbox);
+	if (WARN_ON(!inst))
+		return ERR_PTR(-EINVAL);
+
+	np = of_find_node_by_phandle(phandle);
+	if (!np) {
+		dev_err(inst->dev, "Unable to find phandle %p\n", p);
+		return ERR_PTR(-ENODEV);
+	}
+
+	for (qinst = inst->qinsts, i = 0; i < inst->num_valid_queues;
+	     i++, qinst++) {
+		/* not using strncmp: What could be a valid length here?? */
+		if (!strcmp(qinst->name, np->name)) {
+			chan = qinst->chan;
+			break;
+		}
+	}
+	of_node_put(np);
+
+	return chan;
+}
+
+/* Queue operations */
+static const struct mbox_chan_ops ti_msgmgr_chan_ops = {
+	.startup = ti_msgmgr_queue_startup,
+	.shutdown = ti_msgmgr_queue_shutdown,
+	.peek_data = ti_msgmgr_queue_peek_data,
+	.last_tx_done = ti_msgmgr_last_tx_done,
+	.send_data = ti_msgmgr_send_data,
+};
+
+/* Keystone K2G SoC integration details */
+static const struct ti_msgmgr_desc k2g_desc = {
+	.queue_count = 64,
+	.max_message_size = 64,
+	.max_messages = 128,
+	.q_slices = 1,
+	.q_proxies = 1,
+	.data_first_reg = 16,
+	.data_last_reg = 31,
+	.tx_polled = false,
+};
+
+static const struct of_device_id ti_msgmgr_of_match[] = {
+	{.compatible = "ti,k2g-message-manager", .data = &k2g_desc},
+	{.compatible = "ti,message-manager", .data = &k2g_desc},
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ti_msgmgr_of_match);
+
+static int ti_msgmgr_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *of_id;
+	struct device_node *np, *child;
+	struct resource *res;
+	const struct ti_msgmgr_desc *desc;
+	struct ti_msgmgr_inst *inst;
+	struct ti_queue_inst *qinst;
+	struct mbox_controller *mbox;
+	struct mbox_chan *chans;
+	int queue_count;
+	int i;
+	int ret = -EINVAL;
+
+	if (!dev->of_node) {
+		dev_err(dev, "no OF information\n");
+		return -EINVAL;
+	}
+	np = dev->of_node;
+
+	of_id = of_match_device(ti_msgmgr_of_match, dev);
+	if (!of_id) {
+		dev_err(dev, "OF data missing\n");
+		return -EINVAL;
+	}
+	desc = of_id->data;
+
+	inst = devm_kzalloc(dev, sizeof(*inst), GFP_KERNEL);
+	if (!inst)
+		return -ENOMEM;
+
+	inst->dev = dev;
+	inst->desc = desc;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+					   "queue_proxy_region");
+	inst->queue_proxy_region = devm_ioremap_resource(dev, res);
+	if (IS_ERR(inst->queue_proxy_region))
+		return PTR_ERR(inst->queue_proxy_region);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+					   "queue_state_debug_region");
+	inst->queue_state_debug_region = devm_ioremap_resource(dev, res);
+	if (IS_ERR(inst->queue_state_debug_region))
+		return PTR_ERR(inst->queue_state_debug_region);
+
+	dev_dbg(dev, "proxy region=%p, queue_state=%p\n",
+		inst->queue_proxy_region, inst->queue_state_debug_region);
+
+	queue_count = of_get_available_child_count(np);
+	if (!queue_count) {
+		dev_err(dev, "No child nodes representing queues\n");
+		return -EINVAL;
+	}
+	if (queue_count > desc->queue_count) {
+		dev_err(dev, "Number of queues %d > max %d\n",
+			queue_count, desc->queue_count);
+		return -ERANGE;
+	}
+	inst->num_valid_queues = queue_count;
+
+	qinst = devm_kzalloc(dev, sizeof(*qinst) * queue_count, GFP_KERNEL);
+	if (!qinst)
+		return -ENOMEM;
+	inst->qinsts = qinst;
+
+	chans = devm_kzalloc(dev, sizeof(*chans) * queue_count, GFP_KERNEL);
+	if (!chans)
+		return -ENOMEM;
+	inst->chans = chans;
+
+	for (i = 0, child = NULL; i < queue_count; i++, qinst++, chans++) {
+		child = of_get_next_available_child(np, child);
+		ret = of_property_read_u32(child, "ti,queue-id",
+					   &qinst->queue_id);
+		if (ret) {
+			dev_err(dev, "Child node %s[idx=%d] has no queue-id\n",
+				child->name, i);
+			return -EINVAL;
+		}
+		if (qinst->queue_id > desc->queue_count) {
+			dev_err(dev, "Child node %s[idx=%d] queuid %d > %d\n",
+				child->name, i, qinst->queue_id,
+				desc->queue_count);
+			return -ERANGE;
+		}
+
+		ret = of_property_read_u32(child, "ti,proxy-id",
+					   &qinst->proxy_id);
+		if (ret) {
+			dev_err(dev, "Child node %s[idx=%d] has no proxy-id\n",
+				child->name, i);
+			return -EINVAL;
+		}
+
+		qinst->irq = of_irq_get_byname(child, "rx");
+		/* TX queues don't have IRQ numbers */
+		if (qinst->irq < 0) {
+			qinst->irq = -1;
+			qinst->is_tx = true;
+		}
+
+		qinst->queue_buff_start = inst->queue_proxy_region +
+		    Q_DATA_OFFSET(qinst->proxy_id, qinst->queue_id,
+				  desc->data_first_reg);
+		qinst->queue_buff_end = inst->queue_proxy_region +
+		    Q_DATA_OFFSET(qinst->proxy_id, qinst->queue_id,
+				  desc->data_last_reg);
+		qinst->queue_state = inst->queue_state_debug_region +
+		    Q_STATE_OFFSET(qinst->queue_id);
+		qinst->name = child->name;
+		qinst->chan = chans;
+
+		/* Allocate usage buffer for rx */
+		if (!qinst->is_tx) {
+			qinst->rx_buff = devm_kzalloc(dev,
+						      desc->max_message_size,
+						      GFP_KERNEL);
+			if (!qinst->rx_buff)
+				return -ENOMEM;
+		}
+
+		chans->con_priv = qinst;
+
+		dev_dbg(dev, "[%d] qidx=%d pidx=%d irq=%d q_s=%p q_e = %p\n",
+			i, qinst->queue_id, qinst->proxy_id, qinst->irq,
+			qinst->queue_buff_start, qinst->queue_buff_end);
+	}
+
+	mbox = &inst->mbox;
+	mbox->dev = dev;
+	mbox->ops = &ti_msgmgr_chan_ops;
+	mbox->chans = inst->chans;
+	mbox->num_chans = inst->num_valid_queues;
+	mbox->txdone_irq = false;
+	mbox->txdone_poll = desc->tx_polled;
+	if (desc->tx_polled)
+		mbox->txpoll_period = desc->tx_poll_timeout_ms;
+	mbox->of_xlate = ti_msgmgr_of_xlate;
+
+	platform_set_drvdata(pdev, inst);
+	ret = mbox_controller_register(mbox);
+	if (ret)
+		dev_err(dev, "Failed to register mbox_controller(%d)\n", ret);
+
+	return ret;
+}
+
+static int ti_msgmgr_remove(struct platform_device *pdev)
+{
+	struct ti_msgmgr_inst *inst;
+
+	inst = platform_get_drvdata(pdev);
+	mbox_controller_unregister(&inst->mbox);
+
+	return 0;
+}
+
+static struct platform_driver ti_msgmgr_driver = {
+	.probe = ti_msgmgr_probe,
+	.remove = ti_msgmgr_remove,
+	.driver = {
+		   .name = "ti-msgmgr",
+		   .of_match_table = of_match_ptr(ti_msgmgr_of_match),
+	},
+};
+module_platform_driver(ti_msgmgr_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("TI message manager driver");
+MODULE_AUTHOR("Nishanth Menon");
+MODULE_ALIAS("platform:ti-msgmgr");
diff --git a/include/linux/ti-msgmgr.h b/include/linux/ti-msgmgr.h
new file mode 100644
index 000000000000..2c6a01850028
--- /dev/null
+++ b/include/linux/ti-msgmgr.h
@@ -0,0 +1,35 @@
+/*
+ * Texas Instruments' Message Manager
+ *
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ *	Nishanth Menon
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef TI_MSGMGR_H
+#define TI_MSGMGR_H
+
+/**
+ * struct ti_msgmgr_message - Message Manager structure
+ * @len: Length of data in the Buffer
+ * @buf: Buffer pointer
+ *
+ * This is the structure for data used in mbox_send_message
+ * the length of data buffer used depends on the SoC integration
+ * parameters - each message may be 64, 128 bytes long depending
+ * on SoC. Client is supposed to be aware of this.
+ */
+struct ti_msgmgr_message {
+	size_t len;
+	u8 *buf;
+};
+
+#endif /* TI_MSGMGR_H */
-- 
2.7.0