lkml.org 
[lkml]   [2014]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCHv8 2/2] mailbox: Introduce framework for mailbox
Date
On Friday 11 July 2014, Jassi Brar wrote:
> +
> + This document aims to help developers write client and controller
> +drivers for the API. But before we start, let us note that the
> +client (especially) and controller drivers are likely going to be
> +very platform specific because the remote firmware is likely to be
> +proprietary and implement non-standard protocol. So even if two
> +platforms employ, say, PL320 controller, the client drivers can't
> +be shared across them. Even the PL320 driver might need to accomodate
> +some platform specific quirks. So the API is meant mainly to avoid
> +similar copies of code written for each platform.
> + Some of the choices made during implementation are the result of this
> +peculiarity of this "common" framework.

Note that there might be the case where you have a Linux instance
on both sides communicating over a standard protocol, so while it's
certainly true that a lot of the users (in particular the existing
ones) are talking to a proprietary firmware, it's not necessarily so.

An example I can think of is using the mailbox API as a low-level
implementation detail of a PCI-PCI link connecting two identical
hosts using a standard protocol like virtio or ntb-net on top.

> + Part 2 - Client Driver (See include/linux/mailbox_client.h)
> +
> + The client might want to operate in blocking mode (synchronously
> +send a message through before returning) or non-blocking/async mode (submit
> +a message and a callback function to the API and return immediately).
> +
> +
> +static struct mbox_chan *ch_async, *ch_blk;
> +static struct mbox_client cl_async, cl_blk;
> +static struct completion c_aysnc;

Using static variables for these is probably not good as an
example: we try to write all drivers in a way that lets them
handle multiple instances of the same hardware, so a better
example may be to put the same things into a data structure
that is dynamically allocatied by the client, even if that is
a little more verbose than your current examaple.

> +/*
> + * This is the handler for data received from remote. The behaviour is purely
> + * dependent upon the protocol. This is just an example.
> + */
> +static void message_from_remote(struct mbox_client *cl, void *mssg)
> +{
> + if (cl == &cl_async) {
> + if (is_an_ack(mssg)) {
> + /* An ACK to our last sample sent */
> + return; /* Or do something else here */
> + } else { /* A new message from remote */
> + queue_req(mssg);
> + }
> + } else {
> + /* Remote f/w sends only ACK packets on this channel */
> + return;
> + }
> +}
> +
> +static void sample_sent(struct mbox_client *cl, void *mssg, int r)
> +{
> + complete(&c_aysnc);
> +}

Each of these would consequently do something like

struct my_mailbox *m = container_of(mbox_client, struct my_mailbox, client);
complete(&m->completion);


> +static struct mbox_chan *
> +of_mbox_index_xlate(struct mbox_controller *mbox,
> + const struct of_phandle_args *sp)
> +{
> + int ind = sp->args[0];
> +
> + if (ind >= mbox->num_chans)
> + return NULL;
> +
> + return &mbox->chans[ind];
> +}

Should this perhaps check that #mbox-cells is '1'?
For other values, this function can't really work.

> +/**
> + * struct mbox_client - User of a mailbox
> + * @dev: The client device
> + * @chan_name: The string token to identify a channel out of more
> + * than one specified for the client via DT
> + * @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.
> + * @rx_callback: Atomic callback to provide client the data received
> + * @tx_done: Atomic callback to tell client of data transmission
> + */

It may be worthwhile listing here which callbacks are being called under a
spinlock and which are allowed to sleep. Same for the other structures with
function pointers.

None of these comments are show-stoppers, overall I'm very happy with the
current state of the mailbox API and I think we should merge it in the next
merge window.

Arnd


\
 
 \ /
  Last update: 2014-07-12 15:01    [W:0.135 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site