| Message ID | 1454690044-2560-2-git-send-email-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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.
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
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!
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
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..
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>; [...] };
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
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
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>; > [...] > };
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.. [...]
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.
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/
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>; + ... + };
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