Re: [PATCH v5 1/4] dt-bindings: mailbox: Document Hi6220 mailbox driver

[Leo Yan]

On Mon, Feb 01, 2016 at 09:46:39PM +0530, Jassi Brar wrote:
> On Mon, Feb 1, 2016 at 8:53 PM, Leo Yan <leo.yan@xxxxxxxxxx> wrote:
> > On Mon, Feb 01, 2016 at 08:08:28AM -0600, Rob Herring wrote:
> >> On Mon, Feb 01, 2016 at 09:34:44PM +0800, Leo Yan wrote:

[...]

> >> > +Optional Properties:
> >> > +--------------------
> >> > +- hi6220,mbox-tx-noirq: Flag to allow the client user of this mailbox driver
> >> > +                   to send messages without triggering a TX completion
> >> > +                   interrupt.
> >>
> >> I don't think this belongs in DT. This should be a flag the client
> >> driver sets when it sends messages.
> >
> > The client driver can set "tx_block = true" so use this flag indicates
> > the client thread should be blocked until data is transmitted.
> >
> Yes, but the 'tx_block' feature is provided by the core. The
> controller driver should not need to know how the client works.
> 
> > But low level mailbox driver can use two method to support "tx_block"
> > mode:
> >
> No, as I said, provider shouldn't care about consumers..
> 
> > - One method is to avoid using interrupt and mailbox framework will
> >   poll with mailbox's idle flag which is set by remote processor
> >   automatically;
> > - Another method is to use interrupt to notify data has been
> >   transmitted and interrupt handler will call completion function to
> >   wake up blocked client thread;
> >
> If it is possible to have either 'idle flag set' or irq generated (not
> both) by the remote, then you may sell the hi6220,mbox-tx-noirq
> property as a "f/w feature" ... but still not for the sake of
> tx_block.

Indeed and totally agree. MCU can support two modes for "automatic idle
flag" or IRQ generated mode, so we can take "hi6220,mbox-tx-noirq" as a
firmware's property.

> >> > +
> >> > +Child Nodes:
> >> > +============
> >> > +A child node is used for representing the actual sub-mailbox 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
> >> > +mailbox device nodes.
> >> > +
> >> > +Required properties:
> >> > +--------------------
> >> > +- hi6220,mbox-tx:  sub-mailbox descriptor property defining Tx channel
> >> > +- hi6220,mbox-rx:  sub-mailbox descriptor property defining Rx channel
> >> > +
> >> > +Sub-mailbox Descriptor Data
> >> > +---------------------------
> >> > +Each of the above hi6220,mbox-tx and hi6220,mbox-rx properties should have 3
> >> > +cells of data that represent the following:
> >> > +    Cell #1 (slot_id) - mailbox slot id used either for transmitting
> >> > +                        (hi6220,mbox-tx) or for receiving (hi6220,mbox-rx)
> >> > +    Cell #2 (dst_irq) - irq identifier index number which used by MCU.
> >> > +    Cell #3 (ack_irq) - irq identifier index number with generating a tx/rx
> >> > +                        interrupt to application processor, mailbox driver
> >> > +                        used this id to acknowledge interrupt.
> >> > +
> >> > +Example:
> >> > +--------
> >> > +
> >> > +   mailbox: mailbox@F7510000 {
> >> > +           #mbox-cells = <1>;
> >> > +           compatible = "hisilicon,hi6220-mbox";
> >> > +           reg = <0x0 0xF7510000 0x0 0x1000>, /* IPC_S */
> >> > +                 <0x0 0x06DFF800 0x0 0x0800>; /* Mailbox */
> >> > +           interrupt-parent = <&gic>;
> >> > +           interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> >> > +           mbox_stub_clock: mbox_stub_clock {
> >> > +                   hi6220,mbox-rx = <0 1 10>;
> >> > +                   hi6220,mbox-tx = <1 0 11>;
> >
> This looks like meant for the client node...
>          mbox-names = "mbox-tx", "mbox-rx";
>          mboxes = <&mailbox 1 0 11>, <&mailbox 0 1 10>;

Good suggestion. Will refine with this way.

Thanks,
Leo Yan