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@xxxxxxxxx> wrote:
> On Tue, Feb 9, 2016 at 6:01 PM, Nishanth Menon <nm@xxxxxx> 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@xxxxxx> 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