diff mbox

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

Message ID 1454690044-2560-2-git-send-email-nm@ti.com
State New
Headers show

Commit Message

Nishanth Menon Feb. 5, 2016, 4:34 p.m. UTC
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

-- 
2.7.0

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

Comments

Rob Herring (Arm) Feb. 8, 2016, 7:37 p.m. UTC | #1
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 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nishanth Menon Feb. 8, 2016, 8:31 p.m. UTC | #2
On Mon, Feb 8, 2016 at 1:37 PM, Rob Herring <robh@kernel.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@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?


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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jassi Brar Feb. 9, 2016, 4:14 a.m. UTC | #3
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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nishanth Menon Feb. 9, 2016, 12:31 p.m. UTC | #4
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 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jassi Brar Feb. 9, 2016, 3:35 p.m. UTC | #5
On Tue, Feb 9, 2016 at 8:24 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> 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:

>>


>>>> +

>>> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nishanth Menon Feb. 9, 2016, 3:43 p.m. UTC | #6
On Tue, Feb 9, 2016 at 8:54 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> 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>;

> };

>


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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nishanth Menon Feb. 9, 2016, 6:10 p.m. UTC | #7
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"
- 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nishanth Menon Feb. 10, 2016, 8:51 p.m. UTC | #8
On Wed, Feb 10, 2016 at 2:13 PM, Suman Anna <s-anna@ti.com> 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@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.



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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jassi Brar Feb. 11, 2016, 4:23 a.m. UTC | #9
On Tue, Feb 9, 2016 at 11:40 PM, Nishanth Menon <nm@ti.com> 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"

> - 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nishanth Menon Feb. 11, 2016, 5:03 a.m. UTC | #10
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jassi Brar Feb. 26, 2016, 11:59 a.m. UTC | #11
On Thu, Feb 11, 2016 at 10:33 AM, Nishanth Menon <nm@ti.com> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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