Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Jassi Brar]

On 15 May 2014 19:57, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Thursday 15 May 2014 11:41:00 Jassi Brar wrote:
>> Introduce common framework for client/protocol drivers and
>> controller drivers of Inter-Processor-Communication (IPC).
>>
>> Client driver developers should have a look at
>>  include/linux/mailbox_client.h to understand the part of
>> the API exposed to client drivers.
>> Similarly controller driver developers should have a look
>> at include/linux/mailbox_controller.h
>>
>> Signed-off-by: Jassi Brar <jaswinder.singh@xxxxxxxxxx>
>
> I hadn't followed all the previous versions, but this looks very nice!
>
> I only have comments about tiny details.
>
>> new file mode 100644
>> index 0000000..0eb2fb0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt
>> @@ -0,0 +1,30 @@
>> +* Generic Mailbox Controller and client driver bindings
>> +
>> +Generic binding to provide a way for Mailbox controller drivers to assign appropriate mailbox channel to client drivers.
>> +
>> +* MAILBOX Controller
>
> Just formatting: wrap the lines after 70 characters, and don't use
> capital letters for 'mailbox'.
>
OK, will fix. I usually do care about 80 char limits, but here
checkpatch didn't catch it either.

>> +
>> +enum mbox_result {
>> +     MBOX_OK = 0,
>> +     MBOX_ERR,
>> +};
>
> How about using a standard error number?
>
OK

>> +/**
>> + * struct mbox_client - User of a mailbox
>> + * @dev:             The client device
>> + * @chan_name:               The "controller:channel" this client wants
>> + * @rx_callback:     Atomic callback to provide client the data received
>> + * @tx_done:         Atomic callback to tell client of data transmission
>> + * @tx_block:                If the mbox_send_message should block until data is
>> + *                   transmitted.
>> + * @tx_tout:         Max block period in ms before TX is assumed failure
>> + * @knows_txdone:    if the client could run the TX state machine. Usually
>> + *                   if the client receives some ACK packet for transmission.
>> + *                   Unused if the controller already has TX_Done/RTR IRQ.
>> + */
>> +struct mbox_client {
>> +     struct device *dev;
>> +     const char *chan_name;
>> +     void (*rx_callback)(struct mbox_client *cl, void *mssg);
>> +     void (*tx_done)(struct mbox_client *cl, void *mssg, enum mbox_result r);
>> +     bool tx_block;
>> +     unsigned long tx_tout;
>> +     bool knows_txdone;
>> +};
>> +
>> +struct mbox_chan *mbox_request_channel(struct mbox_client *cl);
>
> Is it possible to make the argument 'const'?
>
Yes, will do.

> Maybe document how you expect an mbox_client to be allocated:
> - static const definition in driver
> - dynamically allocated and embedded in some client specific struct
> - on the stack and discarded after mbox_request_channel()
>
OK. The struct needs to be preserved until free_channel returns. So
clients may allocate on stack for quick single xfer or dynamically
allocated for 'server' type clients.

>> +/**
>> + * struct mbox_controller - Controller of a class of communication chans
>> + * @dev:             Device backing this controller
>> + * @controller_name: Literal name of the controller.
>> + * @ops:             Operators that work on each communication chan
>> + * @chans:           Null terminated array of chans.
>> + * @txdone_irq:              Indicates if the controller can report to API when
>> + *                   the last transmitted data was read by the remote.
>> + *                   Eg, if it has some TX ACK irq.
>> + * @txdone_poll:     If the controller can read but not report the TX
>> + *                   done. Ex, some register shows the TX status but
>> + *                   no interrupt rises. Ignored if 'txdone_irq' is set.
>> + * @txpoll_period:   If 'txdone_poll' is in effect, the API polls for
>> + *                   last TX's status after these many millisecs
>> + */
>> +struct mbox_controller {
>> +     struct device *dev;
>> +     struct mbox_chan_ops *ops;
>> +     struct mbox_chan *chans;
>> +     int num_chans;
>> +     bool txdone_irq;
>> +     bool txdone_poll;
>> +     unsigned txpoll_period;
>> +     struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
>> +                                     const struct of_phandle_args *sp);
>> +     /*
>> +      * If the controller supports only TXDONE_BY_POLL,
>> +      * this timer polls all the links for txdone.
>> +      */
>> +     struct timer_list poll;
>> +     unsigned period;
>> +     /* Hook to add to the global controller list */
>> +     struct list_head node;
>> +} __aligned(32);
>
> What is the __aligned(32) for?
>
Attempt to align access to mailbox?

I am still open to opinion about whether the mailbox ownership should
be exclusive or shared among clients. Need to handle async messages
from remote is one reason one might want more than one client to own a
channel. Allowing for RX via notifiers might be one option but that
breaks semantics of 'ownership' of a mailbox channel.
 Also, some platform might need to communicate with remote master
during very early boot like for initializing system timers and clocks.
The API isn't working then.

Thanks
Jassi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/