| Message ID | 1406055374-29275-1-git-send-email-jaswinder.singh@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, 23 Jul 2014, 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@linaro.org> > --- > MAINTAINERS | 8 + > drivers/mailbox/Makefile | 4 + > drivers/mailbox/mailbox.c | 467 +++++++++++++++++++++++++++++++++++++ > include/linux/mailbox_client.h | 45 ++++ > include/linux/mailbox_controller.h | 131 +++++++++++ > 5 files changed, 655 insertions(+) > create mode 100644 drivers/mailbox/mailbox.c > create mode 100644 include/linux/mailbox_client.h > create mode 100644 include/linux/mailbox_controller.h After a _quick_ look through: > diff --git a/MAINTAINERS b/MAINTAINERS > index 61a8f48..9f225d2 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5672,6 +5672,14 @@ S: Maintained > F: drivers/net/macvlan.c > F: include/linux/if_macvlan.h > > +MAILBOX API > +M: Jassi Brar <jassisinghbrar@gmail.com> Gmail? > +L: linux-kernel@vger.kernel.org > +S: Maintained > +F: drivers/mailbox/ > +F: include/linux/mailbox_client.h > +F: include/linux/mailbox_controller.h > + [...] > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > new file mode 100644 > index 0000000..99c0d23 > --- /dev/null > +++ b/drivers/mailbox/mailbox.c > @@ -0,0 +1,467 @@ > +/* > + * Mailbox: Common code for Mailbox controllers and users > + * > + * Copyright (C) 2013-2014 Linaro Ltd. > + * Author: Jassi Brar <jassisinghbrar@gmail.com> (C) Linaro, but you've supplied your Gmail? > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/interrupt.h> > +#include <linux/spinlock.h> > +#include <linux/mutex.h> > +#include <linux/delay.h> > +#include <linux/slab.h> > +#include <linux/err.h> > +#include <linux/module.h> > +#include <linux/device.h> > +#include <linux/mailbox_client.h> > +#include <linux/mailbox_controller.h> > + > +#define TXDONE_BY_IRQ (1 << 0) /* controller has remote RTR irq */ > +#define TXDONE_BY_POLL (1 << 1) /* controller can read status of last TX */ > +#define TXDONE_BY_ACK (1 << 2) /* S/W ACK recevied by Client ticks the TX */ BIT(0), BIT(1), BIT(2). > +static LIST_HEAD(mbox_cons); > +static DEFINE_MUTEX(con_mutex); > + > +static int add_to_rbuf(struct mbox_chan *chan, void *mssg) > +{ > + int idx; > + unsigned long flags; > + > + spin_lock_irqsave(&chan->lock, flags); > + > + /* See if there is any space left */ > + if (chan->msg_count == MBOX_TX_QUEUE_LEN) { > + spin_unlock_irqrestore(&chan->lock, flags); > + return -ENOMEM; Doesn't -ENOMEM signify out-of-system-memory, rather than a buffer has been filled? I'm thinking ENOBUFS would be more appropriate. > + } > + > + idx = chan->msg_free; > + chan->msg_data[idx] = mssg; > + chan->msg_count++; > + > + if (idx == MBOX_TX_QUEUE_LEN - 1) > + chan->msg_free = 0; > + else > + chan->msg_free++; > + > + spin_unlock_irqrestore(&chan->lock, flags); > + > + return idx; > +} > + > +static void msg_submit(struct mbox_chan *chan) > +{ > + unsigned count, idx; > + unsigned long flags; > + void *data; > + int err; > + > + spin_lock_irqsave(&chan->lock, flags); > + > + if (!chan->msg_count || chan->active_req) { > + spin_unlock_irqrestore(&chan->lock, flags); > + return; > + } You could reduce by a line here if you use a goto, to jump down to the other spin_unlock*(). > + count = chan->msg_count; > + idx = chan->msg_free; > + if (idx >= count) > + idx -= count; > + else > + idx += MBOX_TX_QUEUE_LEN - count; > + > + data = chan->msg_data[idx]; > + > + /* Try to submit a message to the MBOX controller */ > + err = chan->mbox->ops->send_data(chan, data); > + if (!err) { > + chan->active_req = data; > + chan->msg_count--; > + } How do we let the caller know if an error occured? > + spin_unlock_irqrestore(&chan->lock, flags); > +} [...] > +static void poll_txdone(unsigned long data) > +{ > + struct mbox_controller *mbox = (struct mbox_controller *)data; > + bool txdone, resched = false; > + int i; > + > + for (i = 0; i < mbox->num_chans; i++) { > + struct mbox_chan *chan = &mbox->chans[i]; > + > + if (chan->active_req && chan->cl) { > + resched = true; > + txdone = chan->mbox->ops->last_tx_done(chan); You should probably add a comment somewhere that this operation is compulsory. Same for all of the other mandatory call-backs. > + if (txdone) > + tx_tick(chan, 0); > + } > + } > + > + if (resched) > + mod_timer(&mbox->poll, jiffies + > + msecs_to_jiffies(mbox->period)); > +} [...] > +/** > + * mbox_send_message - For client to submit a message to be > + * sent to the remote. > + * @chan: Mailbox channel assigned to this client. > + * @mssg: Client specific message typecasted. > + * > + * For client to submit data to the controller destined for a remote > + * processor. If the client had set 'tx_block', the call will return > + * either when the remote receives the data or when 'tx_tout' millisecs > + * run out. > + * In non-blocking mode, the requests are buffered by the API and a > + * non-negative token is returned for each queued request. If the request > + * is not queued, a negative token is returned. Upon failure or successful > + * TX, the API calls 'tx_done' from atomic context, from which the client > + * could submit yet another request. > + * The pointer to message should be preserved until it is sent > + * over the chan, i.e, tx_done() is made. > + * This function could be called from atomic context as it simply > + * queues the data and returns a token against the request. > + * > + * Return: Non-negative integer for successful submission (non-blocking mode) > + * or transmission over chan (blocking mode). > + * Negative value denotes failure. > + */ > +int mbox_send_message(struct mbox_chan *chan, void *mssg) > +{ > + int t; > + > + if (!chan || !chan->cl) > + return -EINVAL; > + > + t = add_to_rbuf(chan, mssg); > + if (t < 0) { > + pr_err("Try increasing MBOX_TX_QUEUE_LEN\n"); Really? > + return t; > + } > + > + msg_submit(chan); > + > + reinit_completion(&chan->tx_complete); > + > + if (chan->txdone_method == TXDONE_BY_POLL) > + poll_txdone((unsigned long)chan->mbox); > + > + if (chan->cl->tx_block && chan->active_req) { > + unsigned long wait; > + int ret; > + > + if (!chan->cl->tx_tout) /* wait for ever */ s/for ever/forever > + wait = msecs_to_jiffies(3600000); What will this do? Will it block the system forever? > + else > + wait = msecs_to_jiffies(chan->cl->tx_tout); > + > + ret = wait_for_completion_timeout(&chan->tx_complete, wait); > + if (ret == 0) { > + t = -EIO; > + tx_tick(chan, -EIO); > + } > + } > + > + return t; > +} > +EXPORT_SYMBOL_GPL(mbox_send_message); > + > +/** > + * mbox_request_channel - Request a mailbox channel. > + * @cl: Identity of the client requesting the channel. > + * @index: Index of mailbox specifier in 'mboxes' property. > + * > + * The Client specifies its requirements and capabilities while asking for > + * a mailbox channel. It can't be called from atomic context. > + * The channel is exclusively allocated and can't be used by another > + * client before the owner calls mbox_free_channel. > + * After assignment, any packet received on this channel will be > + * handed over to the client via the 'rx_callback'. > + * The framework holds reference to the client, so the mbox_client > + * structure shouldn't be modified until the mbox_free_channel returns. > + * > + * Return: Pointer to the channel assigned to the client if successful. > + * ERR_PTR for request failure. > + */ > +struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index) > +{ > + struct device *dev = cl->dev; > + struct mbox_controller *mbox; > + struct of_phandle_args spec; > + struct mbox_chan *chan; > + unsigned long flags; > + int ret; > + > + if (!dev || !dev->of_node) { > + pr_debug("%s: No owner device node\n", __func__); Are you sure you want all of this debug prints in the final version? > + return ERR_PTR(-ENODEV); > + } > + > + mutex_lock(&con_mutex); > + > + if (of_parse_phandle_with_args(dev->of_node, "mboxes", > + "#mbox-cells", index, &spec)) { > + pr_debug("%s: can't parse \"mboxes\" property\n", __func__); How you have a 'struct device' to play with, can't you use dev_dbg() and friends? > + mutex_unlock(&con_mutex); > + return ERR_PTR(-ENODEV); > + } > + > + chan = NULL; Do this during declaration. > + list_for_each_entry(mbox, &mbox_cons, node) > + if (mbox->dev->of_node == spec.np) { > + chan = mbox->of_xlate(mbox, &spec); > + break; > + } > + > + of_node_put(spec.np); > + > + if (!chan || chan->cl || !try_module_get(mbox->dev->driver->owner)) { > + pr_debug("%s: mailbox not free\n", __func__); > + mutex_unlock(&con_mutex); > + return ERR_PTR(-EBUSY); > + } > + > + spin_lock_irqsave(&chan->lock, flags); > + chan->msg_free = 0; > + chan->msg_count = 0; > + chan->active_req = NULL; > + chan->cl = cl; > + init_completion(&chan->tx_complete); > + > + if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) > + chan->txdone_method |= TXDONE_BY_ACK; > + > + spin_unlock_irqrestore(&chan->lock, flags); > + > + ret = chan->mbox->ops->startup(chan); > + if (ret) { > + pr_err("Unable to startup the chan (%d)\n", ret); > + mbox_free_channel(chan); > + chan = ERR_PTR(ret); > + } > + > + mutex_unlock(&con_mutex); > + return chan; > +} > +EXPORT_SYMBOL_GPL(mbox_request_channel); > + > +/** > + * mbox_free_channel - The client relinquishes control of a mailbox > + * channel by this call. > + * @chan: The mailbox channel to be freed. > + */ > +void mbox_free_channel(struct mbox_chan *chan) > +{ > + unsigned long flags; > + > + if (!chan || !chan->cl) > + return; > + > + chan->mbox->ops->shutdown(chan); > + > + /* The queued TX requests are simply aborted, no callbacks are made */ > + spin_lock_irqsave(&chan->lock, flags); > + chan->cl = NULL; > + chan->active_req = NULL; > + if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)) > + chan->txdone_method = TXDONE_BY_POLL; Unless you're leaving it there for clarity, you can drop the "TXDONE_BY_POLL |" from if(). > + module_put(chan->mbox->dev->driver->owner); > + spin_unlock_irqrestore(&chan->lock, flags); > +} > +EXPORT_SYMBOL_GPL(mbox_free_channel); > + > +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]; > +} > + > +/** > + * mbox_controller_register - Register the mailbox controller > + * @mbox: Pointer to the mailbox controller. > + * > + * The controller driver registers its communication channels > + */ > +int mbox_controller_register(struct mbox_controller *mbox) > +{ > + int i, txdone; > + > + /* Sanity check */ > + if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans) > + return -EINVAL; > + > + if (mbox->txdone_irq) > + txdone = TXDONE_BY_IRQ; > + else if (mbox->txdone_poll) > + txdone = TXDONE_BY_POLL; > + else /* It has to be ACK then */ > + txdone = TXDONE_BY_ACK; > + > + if (txdone == TXDONE_BY_POLL) { > + mbox->poll.function = &poll_txdone; > + mbox->poll.data = (unsigned long)mbox; > + init_timer(&mbox->poll); > + } Can't you put this in the statement above? > + for (i = 0; i < mbox->num_chans; i++) { > + struct mbox_chan *chan = &mbox->chans[i]; > + > + chan->cl = NULL; > + chan->mbox = mbox; > + chan->txdone_method = txdone; > + spin_lock_init(&chan->lock); > + } > + > + if (!mbox->of_xlate) > + mbox->of_xlate = of_mbox_index_xlate; > + > + mutex_lock(&con_mutex); > + list_add_tail(&mbox->node, &mbox_cons); > + mutex_unlock(&con_mutex); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(mbox_controller_register); > + > +/** > + * mbox_controller_unregister - Unregister the mailbox controller > + * @mbox: Pointer to the mailbox controller. > + */ > +void mbox_controller_unregister(struct mbox_controller *mbox) > +{ > + int i; > + > + if (!mbox) > + return; > + > + mutex_lock(&con_mutex); > + > + list_del(&mbox->node); > + > + for (i = 0; i < mbox->num_chans; i++) > + mbox_free_channel(&mbox->chans[i]); > + > + if (mbox->txdone_poll) > + del_timer_sync(&mbox->poll); > + > + mutex_unlock(&con_mutex); > +} > +EXPORT_SYMBOL_GPL(mbox_controller_unregister); > diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h > new file mode 100644 > index 0000000..0f8cba6 > --- /dev/null > +++ b/include/linux/mailbox_client.h > @@ -0,0 +1,45 @@ > +/* > + * Copyright (C) 2013-2014 Linaro Ltd. > + * Author: Jassi Brar <jassisinghbrar@gmail.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#ifndef __MAILBOX_CLIENT_H > +#define __MAILBOX_CLIENT_H > + > +#include <linux/of.h> device.h > +struct mbox_chan; > + > +/** > + * struct mbox_client - User of a mailbox > + * @dev: The client device > + * @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 > + */ > +struct mbox_client { > + struct device *dev; > + bool tx_block; > + unsigned long tx_tout; > + bool knows_txdone; > + > + void (*rx_callback)(struct mbox_client *cl, void *mssg); > + void (*tx_done)(struct mbox_client *cl, void *mssg, int r); > +}; > + > +struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index); > +int mbox_send_message(struct mbox_chan *chan, void *mssg); > +void mbox_client_txdone(struct mbox_chan *chan, int r); /* atomic */ > +bool mbox_client_peek_data(struct mbox_chan *chan); /* atomic */ > +void mbox_free_channel(struct mbox_chan *chan); /* may sleep */ > + > +#endif /* __MAILBOX_CLIENT_H */ > diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h > new file mode 100644 > index 0000000..976c4ed > --- /dev/null > +++ b/include/linux/mailbox_controller.h > @@ -0,0 +1,131 @@ > +/* > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#ifndef __MAILBOX_CONTROLLER_H > +#define __MAILBOX_CONTROLLER_H > + > +#include <linux/of.h> completion.h device.h timer.h Etc. [...]
On 23 July 2014 14:24, Lee Jones <lee.jones@linaro.org> wrote: > On Wed, 23 Jul 2014, 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@linaro.org> >> --- >> MAINTAINERS | 8 + >> drivers/mailbox/Makefile | 4 + >> drivers/mailbox/mailbox.c | 467 +++++++++++++++++++++++++++++++++++++ >> include/linux/mailbox_client.h | 45 ++++ >> include/linux/mailbox_controller.h | 131 +++++++++++ >> 5 files changed, 655 insertions(+) >> create mode 100644 drivers/mailbox/mailbox.c >> create mode 100644 include/linux/mailbox_client.h >> create mode 100644 include/linux/mailbox_controller.h > > After a _quick_ look through: > >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 61a8f48..9f225d2 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -5672,6 +5672,14 @@ S: Maintained >> F: drivers/net/macvlan.c >> F: include/linux/if_macvlan.h >> >> +MAILBOX API >> +M: Jassi Brar <jassisinghbrar@gmail.com> > > Gmail? > Yup, as much as I am grateful to Linaro for giving me the opportunity to work on this... the unfortunate fact remains that every, but the personal, email-id eventually starts bouncing mails :) Please note the copyright is owned by Linaro as it should be. >> +L: linux-kernel@vger.kernel.org >> +S: Maintained >> +F: drivers/mailbox/ >> +F: include/linux/mailbox_client.h >> +F: include/linux/mailbox_controller.h >> + > > [...] > >> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c >> new file mode 100644 >> index 0000000..99c0d23 >> --- /dev/null >> +++ b/drivers/mailbox/mailbox.c >> @@ -0,0 +1,467 @@ >> +/* >> + * Mailbox: Common code for Mailbox controllers and users >> + * >> + * Copyright (C) 2013-2014 Linaro Ltd. >> + * Author: Jassi Brar <jassisinghbrar@gmail.com> > > (C) Linaro, but you've supplied your Gmail? > I don't understand how that's a problem. Is it? I always try to post from my Linaro id still. >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include <linux/interrupt.h> >> +#include <linux/spinlock.h> >> +#include <linux/mutex.h> >> +#include <linux/delay.h> >> +#include <linux/slab.h> >> +#include <linux/err.h> >> +#include <linux/module.h> >> +#include <linux/device.h> >> +#include <linux/mailbox_client.h> >> +#include <linux/mailbox_controller.h> >> + >> +#define TXDONE_BY_IRQ (1 << 0) /* controller has remote RTR irq */ >> +#define TXDONE_BY_POLL (1 << 1) /* controller can read status of last TX */ >> +#define TXDONE_BY_ACK (1 << 2) /* S/W ACK recevied by Client ticks the TX */ > > BIT(0), BIT(1), BIT(2). > Yeah that would look cooler. >> +static LIST_HEAD(mbox_cons); >> +static DEFINE_MUTEX(con_mutex); >> + >> +static int add_to_rbuf(struct mbox_chan *chan, void *mssg) >> +{ >> + int idx; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&chan->lock, flags); >> + >> + /* See if there is any space left */ >> + if (chan->msg_count == MBOX_TX_QUEUE_LEN) { >> + spin_unlock_irqrestore(&chan->lock, flags); >> + return -ENOMEM; > > Doesn't -ENOMEM signify out-of-system-memory, rather than a buffer has > been filled? I'm thinking ENOBUFS would be more appropriate. > OK :) >> + } >> + >> + idx = chan->msg_free; >> + chan->msg_data[idx] = mssg; >> + chan->msg_count++; >> + >> + if (idx == MBOX_TX_QUEUE_LEN - 1) >> + chan->msg_free = 0; >> + else >> + chan->msg_free++; >> + >> + spin_unlock_irqrestore(&chan->lock, flags); >> + >> + return idx; >> +} >> + >> +static void msg_submit(struct mbox_chan *chan) >> +{ >> + unsigned count, idx; >> + unsigned long flags; >> + void *data; >> + int err; >> + >> + spin_lock_irqsave(&chan->lock, flags); >> + >> + if (!chan->msg_count || chan->active_req) { >> + spin_unlock_irqrestore(&chan->lock, flags); >> + return; >> + } > > You could reduce by a line here if you use a goto, to jump down to the > other spin_unlock*(). > Thanks >> + count = chan->msg_count; >> + idx = chan->msg_free; >> + if (idx >= count) >> + idx -= count; >> + else >> + idx += MBOX_TX_QUEUE_LEN - count; >> + >> + data = chan->msg_data[idx]; >> + >> + /* Try to submit a message to the MBOX controller */ >> + err = chan->mbox->ops->send_data(chan, data); >> + if (!err) { >> + chan->active_req = data; >> + chan->msg_count--; >> + } > > How do we let the caller know if an error occured? > Messages are only expected to be queued, not sent from the submit call. So we eventually let the user know something bad happened via timeout. >> + spin_unlock_irqrestore(&chan->lock, flags); >> +} > > [...] > >> +static void poll_txdone(unsigned long data) >> +{ >> + struct mbox_controller *mbox = (struct mbox_controller *)data; >> + bool txdone, resched = false; >> + int i; >> + >> + for (i = 0; i < mbox->num_chans; i++) { >> + struct mbox_chan *chan = &mbox->chans[i]; >> + >> + if (chan->active_req && chan->cl) { >> + resched = true; >> + txdone = chan->mbox->ops->last_tx_done(chan); > > You should probably add a comment somewhere that this operation is > compulsory. Same for all of the other mandatory call-backs. > I thought I rather marked some calls as optional. >> + if (txdone) >> + tx_tick(chan, 0); >> + } >> + } >> + >> + if (resched) >> + mod_timer(&mbox->poll, jiffies + >> + msecs_to_jiffies(mbox->period)); >> +} > > [...] > >> +/** >> + * mbox_send_message - For client to submit a message to be >> + * sent to the remote. >> + * @chan: Mailbox channel assigned to this client. >> + * @mssg: Client specific message typecasted. >> + * >> + * For client to submit data to the controller destined for a remote >> + * processor. If the client had set 'tx_block', the call will return >> + * either when the remote receives the data or when 'tx_tout' millisecs >> + * run out. >> + * In non-blocking mode, the requests are buffered by the API and a >> + * non-negative token is returned for each queued request. If the request >> + * is not queued, a negative token is returned. Upon failure or successful >> + * TX, the API calls 'tx_done' from atomic context, from which the client >> + * could submit yet another request. >> + * The pointer to message should be preserved until it is sent >> + * over the chan, i.e, tx_done() is made. >> + * This function could be called from atomic context as it simply >> + * queues the data and returns a token against the request. >> + * >> + * Return: Non-negative integer for successful submission (non-blocking mode) >> + * or transmission over chan (blocking mode). >> + * Negative value denotes failure. >> + */ >> +int mbox_send_message(struct mbox_chan *chan, void *mssg) >> +{ >> + int t; >> + >> + if (!chan || !chan->cl) >> + return -EINVAL; >> + >> + t = add_to_rbuf(chan, mssg); >> + if (t < 0) { >> + pr_err("Try increasing MBOX_TX_QUEUE_LEN\n"); > > Really? > Yup. There's a rich bikeshedding history of more than a year in the archives, over this rather simple aspect. >> + return t; >> + } >> + >> + msg_submit(chan); >> + >> + reinit_completion(&chan->tx_complete); >> + >> + if (chan->txdone_method == TXDONE_BY_POLL) >> + poll_txdone((unsigned long)chan->mbox); >> + >> + if (chan->cl->tx_block && chan->active_req) { >> + unsigned long wait; >> + int ret; >> + >> + if (!chan->cl->tx_tout) /* wait for ever */ > > s/for ever/forever > thanks >> + wait = msecs_to_jiffies(3600000); > > What will this do? Will it block the system forever? > Only if the user wanted the message xfer to be blocking but didn't care to specify the timeout. >> + else >> + wait = msecs_to_jiffies(chan->cl->tx_tout); >> + >> + ret = wait_for_completion_timeout(&chan->tx_complete, wait); >> + if (ret == 0) { >> + t = -EIO; >> + tx_tick(chan, -EIO); >> + } >> + } >> + >> + return t; >> +} >> +EXPORT_SYMBOL_GPL(mbox_send_message); >> + >> +/** >> + * mbox_request_channel - Request a mailbox channel. >> + * @cl: Identity of the client requesting the channel. >> + * @index: Index of mailbox specifier in 'mboxes' property. >> + * >> + * The Client specifies its requirements and capabilities while asking for >> + * a mailbox channel. It can't be called from atomic context. >> + * The channel is exclusively allocated and can't be used by another >> + * client before the owner calls mbox_free_channel. >> + * After assignment, any packet received on this channel will be >> + * handed over to the client via the 'rx_callback'. >> + * The framework holds reference to the client, so the mbox_client >> + * structure shouldn't be modified until the mbox_free_channel returns. >> + * >> + * Return: Pointer to the channel assigned to the client if successful. >> + * ERR_PTR for request failure. >> + */ >> +struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index) >> +{ >> + struct device *dev = cl->dev; >> + struct mbox_controller *mbox; >> + struct of_phandle_args spec; >> + struct mbox_chan *chan; >> + unsigned long flags; >> + int ret; >> + >> + if (!dev || !dev->of_node) { >> + pr_debug("%s: No owner device node\n", __func__); > > Are you sure you want all of this debug prints in the final version? > pr_debug is as good as absent unless DEBUG is defined >> + return ERR_PTR(-ENODEV); >> + } >> + >> + mutex_lock(&con_mutex); >> + >> + if (of_parse_phandle_with_args(dev->of_node, "mboxes", >> + "#mbox-cells", index, &spec)) { >> + pr_debug("%s: can't parse \"mboxes\" property\n", __func__); > > How you have a 'struct device' to play with, can't you use dev_dbg() > and friends? > At some places we don't/can't, so same here. >> + mutex_unlock(&con_mutex); >> + return ERR_PTR(-ENODEV); >> + } >> + >> + chan = NULL; > > Do this during declaration. > >> + list_for_each_entry(mbox, &mbox_cons, node) >> + if (mbox->dev->of_node == spec.np) { >> + chan = mbox->of_xlate(mbox, &spec); >> + break; >> + } >> + >> + of_node_put(spec.np); >> + >> + if (!chan || chan->cl || !try_module_get(mbox->dev->driver->owner)) { >> + pr_debug("%s: mailbox not free\n", __func__); >> + mutex_unlock(&con_mutex); >> + return ERR_PTR(-EBUSY); >> + } >> + >> + spin_lock_irqsave(&chan->lock, flags); >> + chan->msg_free = 0; >> + chan->msg_count = 0; >> + chan->active_req = NULL; >> + chan->cl = cl; >> + init_completion(&chan->tx_complete); >> + >> + if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) >> + chan->txdone_method |= TXDONE_BY_ACK; >> + >> + spin_unlock_irqrestore(&chan->lock, flags); >> + >> + ret = chan->mbox->ops->startup(chan); >> + if (ret) { >> + pr_err("Unable to startup the chan (%d)\n", ret); >> + mbox_free_channel(chan); >> + chan = ERR_PTR(ret); >> + } >> + >> + mutex_unlock(&con_mutex); >> + return chan; >> +} >> +EXPORT_SYMBOL_GPL(mbox_request_channel); >> + >> +/** >> + * mbox_free_channel - The client relinquishes control of a mailbox >> + * channel by this call. >> + * @chan: The mailbox channel to be freed. >> + */ >> +void mbox_free_channel(struct mbox_chan *chan) >> +{ >> + unsigned long flags; >> + >> + if (!chan || !chan->cl) >> + return; >> + >> + chan->mbox->ops->shutdown(chan); >> + >> + /* The queued TX requests are simply aborted, no callbacks are made */ >> + spin_lock_irqsave(&chan->lock, flags); >> + chan->cl = NULL; >> + chan->active_req = NULL; >> + if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)) >> + chan->txdone_method = TXDONE_BY_POLL; > > Unless you're leaving it there for clarity, you can drop the > "TXDONE_BY_POLL |" from if(). > We need to check for both. >> + module_put(chan->mbox->dev->driver->owner); >> + spin_unlock_irqrestore(&chan->lock, flags); >> +} >> +EXPORT_SYMBOL_GPL(mbox_free_channel); >> + >> +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]; >> +} >> + >> +/** >> + * mbox_controller_register - Register the mailbox controller >> + * @mbox: Pointer to the mailbox controller. >> + * >> + * The controller driver registers its communication channels >> + */ >> +int mbox_controller_register(struct mbox_controller *mbox) >> +{ >> + int i, txdone; >> + >> + /* Sanity check */ >> + if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans) >> + return -EINVAL; >> + >> + if (mbox->txdone_irq) >> + txdone = TXDONE_BY_IRQ; >> + else if (mbox->txdone_poll) >> + txdone = TXDONE_BY_POLL; >> + else /* It has to be ACK then */ >> + txdone = TXDONE_BY_ACK; >> + >> + if (txdone == TXDONE_BY_POLL) { >> + mbox->poll.function = &poll_txdone; >> + mbox->poll.data = (unsigned long)mbox; >> + init_timer(&mbox->poll); >> + } > > Can't you put this in the statement above? > This looked cleaner. >> + for (i = 0; i < mbox->num_chans; i++) { >> + struct mbox_chan *chan = &mbox->chans[i]; >> + >> + chan->cl = NULL; >> + chan->mbox = mbox; >> + chan->txdone_method = txdone; >> + spin_lock_init(&chan->lock); >> + } >> + >> + if (!mbox->of_xlate) >> + mbox->of_xlate = of_mbox_index_xlate; >> + >> + mutex_lock(&con_mutex); >> + list_add_tail(&mbox->node, &mbox_cons); >> + mutex_unlock(&con_mutex); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(mbox_controller_register); >> + >> +/** >> + * mbox_controller_unregister - Unregister the mailbox controller >> + * @mbox: Pointer to the mailbox controller. >> + */ >> +void mbox_controller_unregister(struct mbox_controller *mbox) >> +{ >> + int i; >> + >> + if (!mbox) >> + return; >> + >> + mutex_lock(&con_mutex); >> + >> + list_del(&mbox->node); >> + >> + for (i = 0; i < mbox->num_chans; i++) >> + mbox_free_channel(&mbox->chans[i]); >> + >> + if (mbox->txdone_poll) >> + del_timer_sync(&mbox->poll); >> + >> + mutex_unlock(&con_mutex); >> +} >> +EXPORT_SYMBOL_GPL(mbox_controller_unregister); >> diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h >> new file mode 100644 >> index 0000000..0f8cba6 >> --- /dev/null >> +++ b/include/linux/mailbox_client.h >> @@ -0,0 +1,45 @@ >> +/* >> + * Copyright (C) 2013-2014 Linaro Ltd. >> + * Author: Jassi Brar <jassisinghbrar@gmail.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#ifndef __MAILBOX_CLIENT_H >> +#define __MAILBOX_CLIENT_H >> + >> +#include <linux/of.h> > > device.h > >> +struct mbox_chan; >> + >> +/** >> + * struct mbox_client - User of a mailbox >> + * @dev: The client device >> + * @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 >> + */ >> +struct mbox_client { >> + struct device *dev; >> + bool tx_block; >> + unsigned long tx_tout; >> + bool knows_txdone; >> + >> + void (*rx_callback)(struct mbox_client *cl, void *mssg); >> + void (*tx_done)(struct mbox_client *cl, void *mssg, int r); >> +}; >> + >> +struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index); >> +int mbox_send_message(struct mbox_chan *chan, void *mssg); >> +void mbox_client_txdone(struct mbox_chan *chan, int r); /* atomic */ >> +bool mbox_client_peek_data(struct mbox_chan *chan); /* atomic */ >> +void mbox_free_channel(struct mbox_chan *chan); /* may sleep */ >> + >> +#endif /* __MAILBOX_CLIENT_H */ >> diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h >> new file mode 100644 >> index 0000000..976c4ed >> --- /dev/null >> +++ b/include/linux/mailbox_controller.h >> @@ -0,0 +1,131 @@ >> +/* >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#ifndef __MAILBOX_CONTROLLER_H >> +#define __MAILBOX_CONTROLLER_H >> + >> +#include <linux/of.h> > > completion.h > device.h > timer.h > > Etc. > And what else? :) Cheers, -jassi
On Wed, 23 Jul 2014, Jassi Brar wrote: > On 23 July 2014 14:24, Lee Jones <lee.jones@linaro.org> wrote: > > On Wed, 23 Jul 2014, 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@linaro.org> > >> --- > >> MAINTAINERS | 8 + > >> drivers/mailbox/Makefile | 4 + > >> drivers/mailbox/mailbox.c | 467 +++++++++++++++++++++++++++++++++++++ > >> include/linux/mailbox_client.h | 45 ++++ > >> include/linux/mailbox_controller.h | 131 +++++++++++ > >> 5 files changed, 655 insertions(+) > >> create mode 100644 drivers/mailbox/mailbox.c > >> create mode 100644 include/linux/mailbox_client.h > >> create mode 100644 include/linux/mailbox_controller.h [...] > >> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > >> new file mode 100644 > >> index 0000000..99c0d23 > >> --- /dev/null > >> +++ b/drivers/mailbox/mailbox.c > >> @@ -0,0 +1,467 @@ > >> +/* > >> + * Mailbox: Common code for Mailbox controllers and users > >> + * > >> + * Copyright (C) 2013-2014 Linaro Ltd. > >> + * Author: Jassi Brar <jassisinghbrar@gmail.com> > > > > (C) Linaro, but you've supplied your Gmail? > > > I don't understand how that's a problem. Is it? > I always try to post from my Linaro id still. I don't know to be honest. I always use my Linaro address when using Linaro's time (and sometimes when I'm using my own), and plan to s/linaro.org/gmail.com when/if I move on. Not sure if there's even a policy on this, just thought I'd mention it. [...] > >> + if (!dev || !dev->of_node) { > >> + pr_debug("%s: No owner device node\n", __func__); > > > > Are you sure you want all of this debug prints in the final version? > > > pr_debug is as good as absent unless DEBUG is defined I'm more concerned with how the code looks than the system log, but if think it will be useful to someone, by all means keep it in. [...] > >> + if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)) > >> + chan->txdone_method = TXDONE_BY_POLL; > > > > Unless you're leaving it there for clarity, you can drop the > > "TXDONE_BY_POLL |" from if(). > > > We need to check for both. What I'm trying to get at is; if it's already TXDONE_BY_POLL, there is no need to set it to TXDONE_BY_POLL. [...] > >> +++ b/include/linux/mailbox_controller.h > >> @@ -0,0 +1,131 @@ > >> +/* > >> + * This program is free software; you can redistribute it and/or modify > >> + * it under the terms of the GNU General Public License version 2 as > >> + * published by the Free Software Foundation. > >> + */ > >> + > >> +#ifndef __MAILBOX_CONTROLLER_H > >> +#define __MAILBOX_CONTROLLER_H > >> + > >> +#include <linux/of.h> > > > > completion.h > > device.h > > timer.h > > > > Etc. > > > And what else? :) I'm not going to do all the work for you Jassi. ;) (Actually, that's as far as I got. It was more a hint for you to look to see _if_ there are any others to add, not to say that there were.)
On 23 July 2014 20:56, Lee Jones <lee.jones@linaro.org> wrote: > On Wed, 23 Jul 2014, Jassi Brar wrote: >> >> + if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)) >> >> + chan->txdone_method = TXDONE_BY_POLL; >> > >> > Unless you're leaving it there for clarity, you can drop the >> > "TXDONE_BY_POLL |" from if(). >> > >> We need to check for both. > > What I'm trying to get at is; if it's already TXDONE_BY_POLL, there is no > need to set it to TXDONE_BY_POLL. > In mbox_request_channel() we added the ACK flag, if POLL was set and now we need to revert that in mbox_free_channel(). Thanks Jassi
On 22/07/14 19:56, 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@linaro.org> > --- > MAINTAINERS | 8 + > drivers/mailbox/Makefile | 4 + > drivers/mailbox/mailbox.c | 467 +++++++++++++++++++++++++++++++++++++ > include/linux/mailbox_client.h | 45 ++++ > include/linux/mailbox_controller.h | 131 +++++++++++ > 5 files changed, 655 insertions(+) > create mode 100644 drivers/mailbox/mailbox.c > create mode 100644 include/linux/mailbox_client.h > create mode 100644 include/linux/mailbox_controller.h > [...] > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > new file mode 100644 > index 0000000..99c0d23 > --- /dev/null > +++ b/drivers/mailbox/mailbox.c > @@ -0,0 +1,467 @@ > +/* > + * Mailbox: Common code for Mailbox controllers and users > + * > + * Copyright (C) 2013-2014 Linaro Ltd. > + * Author: Jassi Brar <jassisinghbrar@gmail.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/interrupt.h> > +#include <linux/spinlock.h> > +#include <linux/mutex.h> > +#include <linux/delay.h> > +#include <linux/slab.h> > +#include <linux/err.h> > +#include <linux/module.h> > +#include <linux/device.h> > +#include <linux/mailbox_client.h> > +#include <linux/mailbox_controller.h> > + > +#define TXDONE_BY_IRQ (1 << 0) /* controller has remote RTR irq */ > +#define TXDONE_BY_POLL (1 << 1) /* controller can read status of last TX */ > +#define TXDONE_BY_ACK (1 << 2) /* S/W ACK recevied by Client ticks the TX */ > + > +static LIST_HEAD(mbox_cons); > +static DEFINE_MUTEX(con_mutex); > + > +static int add_to_rbuf(struct mbox_chan *chan, void *mssg) > +{ > + int idx; > + unsigned long flags; > + > + spin_lock_irqsave(&chan->lock, flags); > + > + /* See if there is any space left */ > + if (chan->msg_count == MBOX_TX_QUEUE_LEN) { > + spin_unlock_irqrestore(&chan->lock, flags); > + return -ENOMEM; > + } > + > + idx = chan->msg_free; > + chan->msg_data[idx] = mssg; > + chan->msg_count++; > + > + if (idx == MBOX_TX_QUEUE_LEN - 1) > + chan->msg_free = 0; > + else > + chan->msg_free++; > + > + spin_unlock_irqrestore(&chan->lock, flags); > + > + return idx; > +} > + > +static void msg_submit(struct mbox_chan *chan) > +{ > + unsigned count, idx; > + unsigned long flags; > + void *data; > + int err; > + > + spin_lock_irqsave(&chan->lock, flags); > + > + if (!chan->msg_count || chan->active_req) { > + spin_unlock_irqrestore(&chan->lock, flags); > + return; > + } > + > + count = chan->msg_count; > + idx = chan->msg_free; > + if (idx >= count) > + idx -= count; > + else > + idx += MBOX_TX_QUEUE_LEN - count; > + > + data = chan->msg_data[idx]; > + > + /* Try to submit a message to the MBOX controller */ > + err = chan->mbox->ops->send_data(chan, data); Probably this is discussed already, but again I missed to find it in archives, so asking here. If the protocol has some payload associated with the message and controller expects it to be in place before calling send_data, we essentially end up not using this queue at all by waiting in the protocol layer(may be in it's own queue) Instead can we have some kind of chan->cl->prepare_data callback so that client can prepare payload ? This in turn avoids to implement it's *own Tx queue* and *reusing the mailbox queue*. Or am I missing something ? > + if (!err) { > + chan->active_req = data; > + chan->msg_count--; > + } > + > + spin_unlock_irqrestore(&chan->lock, flags); > +} > + > +static void tx_tick(struct mbox_chan *chan, int r) > +{ > + unsigned long flags; > + void *mssg; > + > + spin_lock_irqsave(&chan->lock, flags); > + mssg = chan->active_req; > + chan->active_req = NULL; > + spin_unlock_irqrestore(&chan->lock, flags); > + > + /* Submit next message */ > + msg_submit(chan); > + > + /* Notify the client */ > + if (mssg && chan->cl->tx_done) > + chan->cl->tx_done(chan->cl, mssg, r); > + > + if (chan->cl->tx_block) > + complete(&chan->tx_complete); > +} > + > +static void poll_txdone(unsigned long data) > +{ > + struct mbox_controller *mbox = (struct mbox_controller *)data; > + bool txdone, resched = false; > + int i; > + > + for (i = 0; i < mbox->num_chans; i++) { > + struct mbox_chan *chan = &mbox->chans[i]; > + > + if (chan->active_req && chan->cl) { > + resched = true; > + txdone = chan->mbox->ops->last_tx_done(chan); > + if (txdone) > + tx_tick(chan, 0); What if all the active channels finished Tx(i.e. txdone = 1), we still have resched = true and add a timer, not really harmful though. But IMO you can move it to else part instead ? Regards, Sudeep
On 31 July 2014 23:02, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 22/07/14 19:56, 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@linaro.org> >> --- >> MAINTAINERS | 8 + >> drivers/mailbox/Makefile | 4 + >> drivers/mailbox/mailbox.c | 467 >> +++++++++++++++++++++++++++++++++++++ >> include/linux/mailbox_client.h | 45 ++++ >> include/linux/mailbox_controller.h | 131 +++++++++++ >> 5 files changed, 655 insertions(+) >> create mode 100644 drivers/mailbox/mailbox.c >> create mode 100644 include/linux/mailbox_client.h >> create mode 100644 include/linux/mailbox_controller.h >> > > [...] > > >> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c >> new file mode 100644 >> index 0000000..99c0d23 >> --- /dev/null >> +++ b/drivers/mailbox/mailbox.c >> @@ -0,0 +1,467 @@ >> +/* >> + * Mailbox: Common code for Mailbox controllers and users >> + * >> + * Copyright (C) 2013-2014 Linaro Ltd. >> + * Author: Jassi Brar <jassisinghbrar@gmail.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include <linux/interrupt.h> >> +#include <linux/spinlock.h> >> +#include <linux/mutex.h> >> +#include <linux/delay.h> >> +#include <linux/slab.h> >> +#include <linux/err.h> >> +#include <linux/module.h> >> +#include <linux/device.h> >> +#include <linux/mailbox_client.h> >> +#include <linux/mailbox_controller.h> >> + >> +#define TXDONE_BY_IRQ (1 << 0) /* controller has remote RTR irq */ >> +#define TXDONE_BY_POLL (1 << 1) /* controller can read status of last TX >> */ >> +#define TXDONE_BY_ACK (1 << 2) /* S/W ACK recevied by Client ticks the >> TX */ >> + >> +static LIST_HEAD(mbox_cons); >> +static DEFINE_MUTEX(con_mutex); >> + >> +static int add_to_rbuf(struct mbox_chan *chan, void *mssg) >> +{ >> + int idx; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&chan->lock, flags); >> + >> + /* See if there is any space left */ >> + if (chan->msg_count == MBOX_TX_QUEUE_LEN) { >> + spin_unlock_irqrestore(&chan->lock, flags); >> + return -ENOMEM; >> + } >> + >> + idx = chan->msg_free; >> + chan->msg_data[idx] = mssg; >> + chan->msg_count++; >> + >> + if (idx == MBOX_TX_QUEUE_LEN - 1) >> + chan->msg_free = 0; >> + else >> + chan->msg_free++; >> + >> + spin_unlock_irqrestore(&chan->lock, flags); >> + >> + return idx; >> +} >> + >> +static void msg_submit(struct mbox_chan *chan) >> +{ >> + unsigned count, idx; >> + unsigned long flags; >> + void *data; >> + int err; >> + >> + spin_lock_irqsave(&chan->lock, flags); >> + >> + if (!chan->msg_count || chan->active_req) { >> + spin_unlock_irqrestore(&chan->lock, flags); >> + return; >> + } >> + >> + count = chan->msg_count; >> + idx = chan->msg_free; >> + if (idx >= count) >> + idx -= count; >> + else >> + idx += MBOX_TX_QUEUE_LEN - count; >> + >> + data = chan->msg_data[idx]; >> + >> + /* Try to submit a message to the MBOX controller */ >> + err = chan->mbox->ops->send_data(chan, data); > > > Probably this is discussed already, but again I missed to find it > in archives, so asking here. If the protocol has some payload associated > with the message and controller expects it to be in place before calling > send_data, we essentially end up not using this queue at all by waiting > in the protocol layer(may be in it's own queue) > Why essentially? The 'data' is a void * i.o.w the packet format is a completely internal understanding between a controller and a client. The 'data' can point to a copy of payload that the controller driver sets up in the shared payload region as the first thing in send_data() callback. OR, like my 'server' type platform, where the access is serialized to the shared payload region. > >> + if (!err) { >> + chan->active_req = data; >> + chan->msg_count--; >> + } >> + >> + spin_unlock_irqrestore(&chan->lock, flags); >> +} >> + >> +static void tx_tick(struct mbox_chan *chan, int r) >> +{ >> + unsigned long flags; >> + void *mssg; >> + >> + spin_lock_irqsave(&chan->lock, flags); >> + mssg = chan->active_req; >> + chan->active_req = NULL; >> + spin_unlock_irqrestore(&chan->lock, flags); >> + >> + /* Submit next message */ >> + msg_submit(chan); >> + >> + /* Notify the client */ >> + if (mssg && chan->cl->tx_done) >> + chan->cl->tx_done(chan->cl, mssg, r); >> + >> + if (chan->cl->tx_block) >> + complete(&chan->tx_complete); >> +} >> + >> +static void poll_txdone(unsigned long data) >> +{ >> + struct mbox_controller *mbox = (struct mbox_controller *)data; >> + bool txdone, resched = false; >> + int i; >> + >> + for (i = 0; i < mbox->num_chans; i++) { >> + struct mbox_chan *chan = &mbox->chans[i]; >> + >> + if (chan->active_req && chan->cl) { >> + resched = true; >> + txdone = chan->mbox->ops->last_tx_done(chan); >> + if (txdone) >> + tx_tick(chan, 0); > > > What if all the active channels finished Tx(i.e. txdone = 1), we still have > resched = true and add a timer, not really harmful though. But IMO you can > move it to else part instead ? > Doing that will be wrong. For txdone, another message may be submitted on the channel and we definitely need to poll again. -Jassi
On Thu, 31 Jul 2014, Jassi Brar wrote: > On 23 July 2014 20:56, Lee Jones <lee.jones@linaro.org> wrote: > > On Wed, 23 Jul 2014, Jassi Brar wrote: > > >> >> + if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)) > >> >> + chan->txdone_method = TXDONE_BY_POLL; > >> > > >> > Unless you're leaving it there for clarity, you can drop the > >> > "TXDONE_BY_POLL |" from if(). > >> > > >> We need to check for both. > > > > What I'm trying to get at is; if it's already TXDONE_BY_POLL, there is no > > need to set it to TXDONE_BY_POLL. > > > In mbox_request_channel() we added the ACK flag, if POLL was set and > now we need to revert that in mbox_free_channel(). Okay, I see what you're doing. Thanks for the clarification.
On 31/07/14 18:58, Jassi Brar wrote: > On 31 July 2014 23:02, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> >> On 22/07/14 19:56, 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@linaro.org> >>> --- >>> MAINTAINERS | 8 + >>> drivers/mailbox/Makefile | 4 + >>> drivers/mailbox/mailbox.c | 467 >>> +++++++++++++++++++++++++++++++++++++ >>> include/linux/mailbox_client.h | 45 ++++ >>> include/linux/mailbox_controller.h | 131 +++++++++++ >>> 5 files changed, 655 insertions(+) >>> create mode 100644 drivers/mailbox/mailbox.c >>> create mode 100644 include/linux/mailbox_client.h >>> create mode 100644 include/linux/mailbox_controller.h >>> >> >> [...] >> >> >>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c >>> new file mode 100644 >>> index 0000000..99c0d23 >>> --- /dev/null >>> +++ b/drivers/mailbox/mailbox.c >>> @@ -0,0 +1,467 @@ >>> +/* >>> + * Mailbox: Common code for Mailbox controllers and users >>> + * >>> + * Copyright (C) 2013-2014 Linaro Ltd. >>> + * Author: Jassi Brar <jassisinghbrar@gmail.com> >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >>> + */ >>> + >>> +#include <linux/interrupt.h> >>> +#include <linux/spinlock.h> >>> +#include <linux/mutex.h> >>> +#include <linux/delay.h> >>> +#include <linux/slab.h> >>> +#include <linux/err.h> >>> +#include <linux/module.h> >>> +#include <linux/device.h> >>> +#include <linux/mailbox_client.h> >>> +#include <linux/mailbox_controller.h> >>> + >>> +#define TXDONE_BY_IRQ (1 << 0) /* controller has remote RTR irq */ >>> +#define TXDONE_BY_POLL (1 << 1) /* controller can read status of last TX >>> */ >>> +#define TXDONE_BY_ACK (1 << 2) /* S/W ACK recevied by Client ticks the >>> TX */ >>> + >>> +static LIST_HEAD(mbox_cons); >>> +static DEFINE_MUTEX(con_mutex); >>> + >>> +static int add_to_rbuf(struct mbox_chan *chan, void *mssg) >>> +{ >>> + int idx; >>> + unsigned long flags; >>> + >>> + spin_lock_irqsave(&chan->lock, flags); >>> + >>> + /* See if there is any space left */ >>> + if (chan->msg_count == MBOX_TX_QUEUE_LEN) { >>> + spin_unlock_irqrestore(&chan->lock, flags); >>> + return -ENOMEM; >>> + } >>> + >>> + idx = chan->msg_free; >>> + chan->msg_data[idx] = mssg; >>> + chan->msg_count++; >>> + >>> + if (idx == MBOX_TX_QUEUE_LEN - 1) >>> + chan->msg_free = 0; >>> + else >>> + chan->msg_free++; >>> + >>> + spin_unlock_irqrestore(&chan->lock, flags); >>> + >>> + return idx; >>> +} >>> + >>> +static void msg_submit(struct mbox_chan *chan) >>> +{ >>> + unsigned count, idx; >>> + unsigned long flags; >>> + void *data; >>> + int err; >>> + >>> + spin_lock_irqsave(&chan->lock, flags); >>> + >>> + if (!chan->msg_count || chan->active_req) { >>> + spin_unlock_irqrestore(&chan->lock, flags); >>> + return; >>> + } >>> + >>> + count = chan->msg_count; >>> + idx = chan->msg_free; >>> + if (idx >= count) >>> + idx -= count; >>> + else >>> + idx += MBOX_TX_QUEUE_LEN - count; >>> + >>> + data = chan->msg_data[idx]; >>> + >>> + /* Try to submit a message to the MBOX controller */ >>> + err = chan->mbox->ops->send_data(chan, data); >> >> >> Probably this is discussed already, but again I missed to find it >> in archives, so asking here. If the protocol has some payload associated >> with the message and controller expects it to be in place before calling >> send_data, we essentially end up not using this queue at all by waiting >> in the protocol layer(may be in it's own queue) >> > Why essentially? > The 'data' is a void * i.o.w the packet format is a completely > internal understanding between a controller and a client. The 'data' > can point to a copy of payload that the controller driver sets up in > the shared payload region as the first thing in send_data() callback. > OR, like my 'server' type platform, where the access is serialized to > the shared payload region. > Yes that's exactly what I have now. But based on your driver I thought that the shared memory should not be associated with the mailbox controller and should be completely handled in the protocol/client. Also to handle async packet from remote, controller has to pass shared memory pointer always to the client to read the data as the buffer won't to ready in such case. So I thought it's good idea to handle shared memory completely in client/protocol. Let me know your thoughts. >> >>> + if (!err) { >>> + chan->active_req = data; >>> + chan->msg_count--; >>> + } >>> + >>> + spin_unlock_irqrestore(&chan->lock, flags); >>> +} >>> + >>> +static void tx_tick(struct mbox_chan *chan, int r) >>> +{ >>> + unsigned long flags; >>> + void *mssg; >>> + >>> + spin_lock_irqsave(&chan->lock, flags); >>> + mssg = chan->active_req; >>> + chan->active_req = NULL; >>> + spin_unlock_irqrestore(&chan->lock, flags); >>> + >>> + /* Submit next message */ >>> + msg_submit(chan); >>> + >>> + /* Notify the client */ >>> + if (mssg && chan->cl->tx_done) >>> + chan->cl->tx_done(chan->cl, mssg, r); >>> + >>> + if (chan->cl->tx_block) >>> + complete(&chan->tx_complete); >>> +} >>> + >>> +static void poll_txdone(unsigned long data) >>> +{ >>> + struct mbox_controller *mbox = (struct mbox_controller *)data; >>> + bool txdone, resched = false; >>> + int i; >>> + >>> + for (i = 0; i < mbox->num_chans; i++) { >>> + struct mbox_chan *chan = &mbox->chans[i]; >>> + >>> + if (chan->active_req && chan->cl) { >>> + resched = true; >>> + txdone = chan->mbox->ops->last_tx_done(chan); >>> + if (txdone) >>> + tx_tick(chan, 0); >> >> >> What if all the active channels finished Tx(i.e. txdone = 1), we still have >> resched = true and add a timer, not really harmful though. But IMO you can >> move it to else part instead ? >> > Doing that will be wrong. For txdone, another message may be submitted > on the channel and we definitely need to poll again. Ah OK, thanks for clarification. Regards, Sudeep
On 1 August 2014 15:09, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 31/07/14 18:58, Jassi Brar wrote: >>> >>> Probably this is discussed already, but again I missed to find it >>> in archives, so asking here. If the protocol has some payload associated >>> with the message and controller expects it to be in place before calling >>> send_data, we essentially end up not using this queue at all by waiting >>> in the protocol layer(may be in it's own queue) >>> >> Why essentially? >> The 'data' is a void * i.o.w the packet format is a completely >> internal understanding between a controller and a client. The 'data' >> can point to a copy of payload that the controller driver sets up in >> the shared payload region as the first thing in send_data() callback. >> OR, like my 'server' type platform, where the access is serialized to >> the shared payload region. >> > > Yes that's exactly what I have now. But based on your driver I thought > that the shared memory should not be associated with the mailbox controller > and should be completely handled in the protocol/client. > > Also to handle async packet from remote, controller has to pass shared > memory pointer always to the client to read the data as the buffer won't > to ready in such case. So I thought it's good idea to handle shared memory > completely in client/protocol. > Again ur platform has the freedom. The async client should have a free packet to copy the data as soon as it arrives. My platform divides the SHM into two parts - one for each direction. -jassi
On 01/08/14 13:28, Jassi Brar wrote: > On 1 August 2014 15:09, Sudeep Holla <sudeep.holla@arm.com> wrote: >> On 31/07/14 18:58, Jassi Brar wrote: > >>>> >>>> Probably this is discussed already, but again I missed to find it >>>> in archives, so asking here. If the protocol has some payload associated >>>> with the message and controller expects it to be in place before calling >>>> send_data, we essentially end up not using this queue at all by waiting >>>> in the protocol layer(may be in it's own queue) >>>> >>> Why essentially? >>> The 'data' is a void * i.o.w the packet format is a completely >>> internal understanding between a controller and a client. The 'data' >>> can point to a copy of payload that the controller driver sets up in >>> the shared payload region as the first thing in send_data() callback. >>> OR, like my 'server' type platform, where the access is serialized to >>> the shared payload region. >>> >> >> Yes that's exactly what I have now. But based on your driver I thought >> that the shared memory should not be associated with the mailbox controller >> and should be completely handled in the protocol/client. >> >> Also to handle async packet from remote, controller has to pass shared >> memory pointer always to the client to read the data as the buffer won't >> to ready in such case. So I thought it's good idea to handle shared memory >> completely in client/protocol. >> > Again ur platform has the freedom. The async client should have a free > packet to copy the data as soon as it arrives. > My platform divides the SHM into two parts - one for each direction. > I understand that, but I wanted to know what's the general recommendation for mailbox controller drivers. Do they need to be associated with the shared memory or is it left the protocol ? The main reason I am asking is that the MHU driver you had didn't deal with any shared memory. That's fine and if we want to make that common/generic we need to know if we are going to handle shared memory in there or leave it to client/protocol implementations. Regards. Sudeep
On 1 August 2014 18:08, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 01/08/14 13:28, Jassi Brar wrote: >> >>>>> Probably this is discussed already, but again I missed to find it >>>>> in archives, so asking here. If the protocol has some payload >>>>> associated >>>>> with the message and controller expects it to be in place before >>>>> calling >>>>> send_data, we essentially end up not using this queue at all by waiting >>>>> in the protocol layer(may be in it's own queue) >>>>> >>>> Why essentially? >>>> The 'data' is a void * i.o.w the packet format is a completely >>>> internal understanding between a controller and a client. The 'data' >>>> can point to a copy of payload that the controller driver sets up in >>>> the shared payload region as the first thing in send_data() callback. >>>> OR, like my 'server' type platform, where the access is serialized to >>>> the shared payload region. >>>> >>> >>> Yes that's exactly what I have now. But based on your driver I thought >>> that the shared memory should not be associated with the mailbox >>> controller >>> and should be completely handled in the protocol/client. >>> >>> Also to handle async packet from remote, controller has to pass shared >>> memory pointer always to the client to read the data as the buffer won't >>> to ready in such case. So I thought it's good idea to handle shared >>> memory >>> completely in client/protocol. >>> >> Again ur platform has the freedom. The async client should have a free >> packet to copy the data as soon as it arrives. >> My platform divides the SHM into two parts - one for each direction. >> > > I understand that, but I wanted to know what's the general > recommendation for mailbox controller drivers. Do they need to be > associated with the shared memory or is it left the protocol ? > The framework already provides for either way ('data' may point to signal/code as well as payload info). I would leave the decision to the platforms that share the controller driver. Otherwise by 'recommending' a scheme we may end up controller drivers implement code that nobody ever uses. Personally, I would write a controller driver that manages only the resources within the controller and leave SHM for clients to manage. However, if a controller can't convey enough info on its own (like a simple irq raise) and no meaningful IPC can be had without SHM, then the driver may assume SHM to always be used. > The main reason I am asking is that the MHU driver you had didn't > deal with any shared memory. That's fine and if we want to make that > common/generic we need to know if we are going to handle shared > memory in there or leave it to client/protocol implementations. > Well, that driver is for MHU, which has nothing to do with SHM which may or may not be present on a platform. Anyways, could we please discuss the MHU driver in the patchset that introduces it? This API already supports both, and this patchset has had enough delays for more than a year now. Thanks Jassi
On 01/08/14 17:38, Jassi Brar wrote: > On 1 August 2014 18:08, Sudeep Holla <sudeep.holla@arm.com> wrote: >> On 01/08/14 13:28, Jassi Brar wrote: >>> >>>>>> Probably this is discussed already, but again I missed to find it >>>>>> in archives, so asking here. If the protocol has some payload >>>>>> associated >>>>>> with the message and controller expects it to be in place before >>>>>> calling >>>>>> send_data, we essentially end up not using this queue at all by waiting >>>>>> in the protocol layer(may be in it's own queue) >>>>>> >>>>> Why essentially? >>>>> The 'data' is a void * i.o.w the packet format is a completely >>>>> internal understanding between a controller and a client. The 'data' >>>>> can point to a copy of payload that the controller driver sets up in >>>>> the shared payload region as the first thing in send_data() callback. >>>>> OR, like my 'server' type platform, where the access is serialized to >>>>> the shared payload region. >>>>> >>>> >>>> Yes that's exactly what I have now. But based on your driver I thought >>>> that the shared memory should not be associated with the mailbox >>>> controller >>>> and should be completely handled in the protocol/client. >>>> >>>> Also to handle async packet from remote, controller has to pass shared >>>> memory pointer always to the client to read the data as the buffer won't >>>> to ready in such case. So I thought it's good idea to handle shared >>>> memory >>>> completely in client/protocol. >>>> >>> Again ur platform has the freedom. The async client should have a free >>> packet to copy the data as soon as it arrives. >>> My platform divides the SHM into two parts - one for each direction. >>> >> >> I understand that, but I wanted to know what's the general >> recommendation for mailbox controller drivers. Do they need to be >> associated with the shared memory or is it left the protocol ? >> > The framework already provides for either way ('data' may point to > signal/code as well as payload info). > I would leave the decision to the platforms that share the controller > driver. Otherwise by 'recommending' a scheme we may end up controller > drivers implement code that nobody ever uses. > Personally, I would write a controller driver that manages only the > resources within the controller and leave SHM for clients to manage. > However, if a controller can't convey enough info on its own (like a > simple irq raise) and no meaningful IPC can be had without SHM, then > the driver may assume SHM to always be used. > Thanks for the confirmation, this is exactly what had in mind too when I first raised question on prepare_data. >> The main reason I am asking is that the MHU driver you had didn't >> deal with any shared memory. That's fine and if we want to make that >> common/generic we need to know if we are going to handle shared >> memory in there or leave it to client/protocol implementations. >> > Well, that driver is for MHU, which has nothing to do with SHM which > may or may not be present on a platform. Again agreed. > Anyways, could we please discuss the MHU driver in the patchset that > introduces it? This API already supports both, and this patchset has > had enough delays for more than a year now. > Sorry I have no intention to delay this getting merged(I too need it ASAP) as long as the APIs can be extended if needed in future(which I think should be). Now coming back to my original question: If the protocol/client manage the SHM and assume there's some payload associated with a message and controller expects it to be in place before calling send_data, then how can you achieve this without blocking for one command to complete before copying the payload for the next. If you wait in the protocol to update payload before queueing the request, you end-up with no use of Tx queue in mailbox.c Now as you said we can do this in controller's send_data(also agreed and that's what I too thought of) or in mailbox just before calling send_data(doesn't matter where). But doesn't it make sense to have standard API like mbox_chan_prepare_data so that we can do that in standard way? Regards, Sudeep
On 1 August 2014 23:03, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 01/08/14 17:38, Jassi Brar wrote: >>>> >>>>>>> Probably this is discussed already, but again I missed to find it >>>>>>> in archives, so asking here. If the protocol has some payload >>>>>>> associated >>>>>>> with the message and controller expects it to be in place before >>>>>>> calling >>>>>>> send_data, we essentially end up not using this queue at all by >>>>>>> waiting >>>>>>> in the protocol layer(may be in it's own queue) >>>>>>> >>>>>> Why essentially? >>>>>> The 'data' is a void * i.o.w the packet format is a completely >>>>>> internal understanding between a controller and a client. The 'data' >>>>>> can point to a copy of payload that the controller driver sets up in >>>>>> the shared payload region as the first thing in send_data() callback. >>>>>> OR, like my 'server' type platform, where the access is serialized to >>>>>> the shared payload region. >>>>>> >>>>> >>>>> Yes that's exactly what I have now. But based on your driver I thought >>>>> that the shared memory should not be associated with the mailbox >>>>> controller >>>>> and should be completely handled in the protocol/client. >>>>> >>>>> Also to handle async packet from remote, controller has to pass shared >>>>> memory pointer always to the client to read the data as the buffer >>>>> won't >>>>> to ready in such case. So I thought it's good idea to handle shared >>>>> memory >>>>> completely in client/protocol. >>>>> >>>> Again ur platform has the freedom. The async client should have a free >>>> packet to copy the data as soon as it arrives. >>>> My platform divides the SHM into two parts - one for each direction. >>>> >>> >>> I understand that, but I wanted to know what's the general >>> recommendation for mailbox controller drivers. Do they need to be >>> associated with the shared memory or is it left the protocol ? >>> >> The framework already provides for either way ('data' may point to >> signal/code as well as payload info). >> I would leave the decision to the platforms that share the controller >> driver. Otherwise by 'recommending' a scheme we may end up controller >> drivers implement code that nobody ever uses. >> Personally, I would write a controller driver that manages only the >> resources within the controller and leave SHM for clients to manage. >> However, if a controller can't convey enough info on its own (like a >> simple irq raise) and no meaningful IPC can be had without SHM, then >> the driver may assume SHM to always be used. >> > > Thanks for the confirmation, this is exactly what had in mind too when > I first raised question on prepare_data. > I know we think the same, I just need to confirm occasionally :) > >>> The main reason I am asking is that the MHU driver you had didn't >>> deal with any shared memory. That's fine and if we want to make that >>> common/generic we need to know if we are going to handle shared >>> memory in there or leave it to client/protocol implementations. >>> >> Well, that driver is for MHU, which has nothing to do with SHM which >> may or may not be present on a platform. > > > Again agreed. > > >> Anyways, could we please discuss the MHU driver in the patchset that >> introduces it? This API already supports both, and this patchset has >> had enough delays for more than a year now. >> > > Sorry I have no intention to delay this getting merged(I too need it > ASAP) as long as the APIs can be extended if needed in future(which I > think should be). > > Now coming back to my original question: If the protocol/client manage > the SHM and assume there's some payload associated with a message and > controller expects it to be in place before calling send_data, then > how can you achieve this without blocking for one command to complete > before copying the payload for the next. If you wait in the protocol > to update payload before queueing the request, you end-up with no use of > Tx queue in mailbox.c > 1) When you say Client/Protocol manages the SHM, then they should be responsible for laying out the SHM with payloads. Not the controller driver. (I hope you don't mean the prepare_data for the client, because that would mean even after submitting a packet for transmission the client has to respond to yet another call before the packet is actually put on the bus.) 2) Considering ur example anyway ... Mailbox api queues "void *data" which could point to a payload structure on client's temporary buffer (like stack). When the packet's turn comes up, the controller driver may copy the payload data as the first thing in send_data() and then continue to trigger it as usual. Where does the client have to block? > Now as you said we can do this in controller's send_data(also agreed > and that's what I too thought of) or in mailbox just before calling > send_data(doesn't matter where). > No, I never said doing it in mailbox api. Which I think will be redundant. > But doesn't it make sense to have > standard API like mbox_chan_prepare_data so that we can do that in > standard way? > As explained above, I don't think we need any 'prepare_data' callback. Cheers, Jassi
Hi Jassi, Sorry for late response. On 02/08/14 04:58, Jassi Brar wrote: > On 1 August 2014 23:03, Sudeep Holla <sudeep.holla@arm.com> wrote: [...] >> >> Sorry I have no intention to delay this getting merged(I too need it >> ASAP) as long as the APIs can be extended if needed in future(which I >> think should be). >> >> Now coming back to my original question: If the protocol/client manage >> the SHM and assume there's some payload associated with a message and >> controller expects it to be in place before calling send_data, then >> how can you achieve this without blocking for one command to complete >> before copying the payload for the next. If you wait in the protocol >> to update payload before queueing the request, you end-up with no use of >> Tx queue in mailbox.c >> > 1) When you say Client/Protocol manages the SHM, then they should be > responsible for laying out the SHM with payloads. Not the controller > driver. (I hope you don't mean the prepare_data for the client, > because that would mean even after submitting a packet for > transmission the client has to respond to yet another call before the > packet is actually put on the bus.) > Agreed even I don't think that's good idea, but .... > 2) Considering ur example anyway ... Mailbox api queues "void *data" > which could point to a payload structure on client's temporary buffer > (like stack). When the packet's turn comes up, the controller driver > may copy the payload data as the first thing in send_data() and then > continue to trigger it as usual. Where does the client have to block? > That wouldn't help as controller has no idea where is your shared memory, so it can't do anything with copied data. Anyway, it's better to take up this when we hit this scenario. Regards, Sudeep
diff --git a/MAINTAINERS b/MAINTAINERS index 61a8f48..9f225d2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5672,6 +5672,14 @@ S: Maintained F: drivers/net/macvlan.c F: include/linux/if_macvlan.h +MAILBOX API +M: Jassi Brar <jassisinghbrar@gmail.com> +L: linux-kernel@vger.kernel.org +S: Maintained +F: drivers/mailbox/ +F: include/linux/mailbox_client.h +F: include/linux/mailbox_controller.h + MAN-PAGES: MANUAL PAGES FOR LINUX -- Sections 2, 3, 4, 5, and 7 M: Michael Kerrisk <mtk.manpages@gmail.com> W: http://www.kernel.org/doc/man-pages diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index e0facb3..2fa343a 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -1,3 +1,7 @@ +# Generic MAILBOX API + +obj-$(CONFIG_MAILBOX) += mailbox.o + obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o obj-$(CONFIG_OMAP_MBOX) += omap-mailbox.o diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c new file mode 100644 index 0000000..99c0d23 --- /dev/null +++ b/drivers/mailbox/mailbox.c @@ -0,0 +1,467 @@ +/* + * Mailbox: Common code for Mailbox controllers and users + * + * Copyright (C) 2013-2014 Linaro Ltd. + * Author: Jassi Brar <jassisinghbrar@gmail.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/interrupt.h> +#include <linux/spinlock.h> +#include <linux/mutex.h> +#include <linux/delay.h> +#include <linux/slab.h> +#include <linux/err.h> +#include <linux/module.h> +#include <linux/device.h> +#include <linux/mailbox_client.h> +#include <linux/mailbox_controller.h> + +#define TXDONE_BY_IRQ (1 << 0) /* controller has remote RTR irq */ +#define TXDONE_BY_POLL (1 << 1) /* controller can read status of last TX */ +#define TXDONE_BY_ACK (1 << 2) /* S/W ACK recevied by Client ticks the TX */ + +static LIST_HEAD(mbox_cons); +static DEFINE_MUTEX(con_mutex); + +static int add_to_rbuf(struct mbox_chan *chan, void *mssg) +{ + int idx; + unsigned long flags; + + spin_lock_irqsave(&chan->lock, flags); + + /* See if there is any space left */ + if (chan->msg_count == MBOX_TX_QUEUE_LEN) { + spin_unlock_irqrestore(&chan->lock, flags); + return -ENOMEM; + } + + idx = chan->msg_free; + chan->msg_data[idx] = mssg; + chan->msg_count++; + + if (idx == MBOX_TX_QUEUE_LEN - 1) + chan->msg_free = 0; + else + chan->msg_free++; + + spin_unlock_irqrestore(&chan->lock, flags); + + return idx; +} + +static void msg_submit(struct mbox_chan *chan) +{ + unsigned count, idx; + unsigned long flags; + void *data; + int err; + + spin_lock_irqsave(&chan->lock, flags); + + if (!chan->msg_count || chan->active_req) { + spin_unlock_irqrestore(&chan->lock, flags); + return; + } + + count = chan->msg_count; + idx = chan->msg_free; + if (idx >= count) + idx -= count; + else + idx += MBOX_TX_QUEUE_LEN - count; + + data = chan->msg_data[idx]; + + /* Try to submit a message to the MBOX controller */ + err = chan->mbox->ops->send_data(chan, data); + if (!err) { + chan->active_req = data; + chan->msg_count--; + } + + spin_unlock_irqrestore(&chan->lock, flags); +} + +static void tx_tick(struct mbox_chan *chan, int r) +{ + unsigned long flags; + void *mssg; + + spin_lock_irqsave(&chan->lock, flags); + mssg = chan->active_req; + chan->active_req = NULL; + spin_unlock_irqrestore(&chan->lock, flags); + + /* Submit next message */ + msg_submit(chan); + + /* Notify the client */ + if (mssg && chan->cl->tx_done) + chan->cl->tx_done(chan->cl, mssg, r); + + if (chan->cl->tx_block) + complete(&chan->tx_complete); +} + +static void poll_txdone(unsigned long data) +{ + struct mbox_controller *mbox = (struct mbox_controller *)data; + bool txdone, resched = false; + int i; + + for (i = 0; i < mbox->num_chans; i++) { + struct mbox_chan *chan = &mbox->chans[i]; + + if (chan->active_req && chan->cl) { + resched = true; + txdone = chan->mbox->ops->last_tx_done(chan); + if (txdone) + tx_tick(chan, 0); + } + } + + if (resched) + mod_timer(&mbox->poll, jiffies + + msecs_to_jiffies(mbox->period)); +} + +/** + * mbox_chan_received_data - A way for controller driver to push data + * received from remote to the upper layer. + * @chan: Pointer to the mailbox channel on which RX happened. + * @mssg: Client specific message typecasted as void * + * + * After startup and before shutdown any data received on the chan + * is passed on to the API via atomic mbox_chan_received_data(). + * The controller should ACK the RX only after this call returns. + */ +void mbox_chan_received_data(struct mbox_chan *chan, void *mssg) +{ + /* No buffering the received data */ + if (chan->cl->rx_callback) + chan->cl->rx_callback(chan->cl, mssg); +} +EXPORT_SYMBOL_GPL(mbox_chan_received_data); + +/** + * mbox_chan_txdone - A way for controller driver to notify the + * framework that the last TX has completed. + * @chan: Pointer to the mailbox chan on which TX happened. + * @r: Status of last TX - OK or ERROR + * + * The controller that has IRQ for TX ACK calls this atomic API + * to tick the TX state machine. It works only if txdone_irq + * is set by the controller. + */ +void mbox_chan_txdone(struct mbox_chan *chan, int r) +{ + if (unlikely(!(chan->txdone_method & TXDONE_BY_IRQ))) { + pr_err("Controller can't run the TX ticker\n"); + return; + } + + tx_tick(chan, r); +} +EXPORT_SYMBOL_GPL(mbox_chan_txdone); + +/** + * mbox_client_txdone - The way for a client to run the TX state machine. + * @chan: Mailbox channel assigned to this client. + * @r: Success status of last transmission. + * + * The client/protocol had received some 'ACK' packet and it notifies + * the API that the last packet was sent successfully. This only works + * if the controller can't sense TX-Done. + */ +void mbox_client_txdone(struct mbox_chan *chan, int r) +{ + if (unlikely(!(chan->txdone_method & TXDONE_BY_ACK))) { + pr_err("Client can't run the TX ticker\n"); + return; + } + + tx_tick(chan, r); +} +EXPORT_SYMBOL_GPL(mbox_client_txdone); + +/** + * mbox_client_peek_data - A way for client driver to pull data + * received from remote by the controller. + * @chan: Mailbox channel assigned to this client. + * + * A poke to controller driver for any received data. + * The data is actually passed onto client via the + * mbox_chan_received_data() + * The call can be made from atomic context, so the controller's + * implementation of peek_data() must not sleep. + * + * Return: True, if controller has, and is going to push after this, + * some data. + * False, if controller doesn't have any data to be read. + */ +bool mbox_client_peek_data(struct mbox_chan *chan) +{ + if (chan->mbox->ops->peek_data) + return chan->mbox->ops->peek_data(chan); + + return false; +} +EXPORT_SYMBOL_GPL(mbox_client_peek_data); + +/** + * mbox_send_message - For client to submit a message to be + * sent to the remote. + * @chan: Mailbox channel assigned to this client. + * @mssg: Client specific message typecasted. + * + * For client to submit data to the controller destined for a remote + * processor. If the client had set 'tx_block', the call will return + * either when the remote receives the data or when 'tx_tout' millisecs + * run out. + * In non-blocking mode, the requests are buffered by the API and a + * non-negative token is returned for each queued request. If the request + * is not queued, a negative token is returned. Upon failure or successful + * TX, the API calls 'tx_done' from atomic context, from which the client + * could submit yet another request. + * The pointer to message should be preserved until it is sent + * over the chan, i.e, tx_done() is made. + * This function could be called from atomic context as it simply + * queues the data and returns a token against the request. + * + * Return: Non-negative integer for successful submission (non-blocking mode) + * or transmission over chan (blocking mode). + * Negative value denotes failure. + */ +int mbox_send_message(struct mbox_chan *chan, void *mssg) +{ + int t; + + if (!chan || !chan->cl) + return -EINVAL; + + t = add_to_rbuf(chan, mssg); + if (t < 0) { + pr_err("Try increasing MBOX_TX_QUEUE_LEN\n"); + return t; + } + + msg_submit(chan); + + reinit_completion(&chan->tx_complete); + + if (chan->txdone_method == TXDONE_BY_POLL) + poll_txdone((unsigned long)chan->mbox); + + if (chan->cl->tx_block && chan->active_req) { + unsigned long wait; + int ret; + + if (!chan->cl->tx_tout) /* wait for ever */ + wait = msecs_to_jiffies(3600000); + else + wait = msecs_to_jiffies(chan->cl->tx_tout); + + ret = wait_for_completion_timeout(&chan->tx_complete, wait); + if (ret == 0) { + t = -EIO; + tx_tick(chan, -EIO); + } + } + + return t; +} +EXPORT_SYMBOL_GPL(mbox_send_message); + +/** + * mbox_request_channel - Request a mailbox channel. + * @cl: Identity of the client requesting the channel. + * @index: Index of mailbox specifier in 'mboxes' property. + * + * The Client specifies its requirements and capabilities while asking for + * a mailbox channel. It can't be called from atomic context. + * The channel is exclusively allocated and can't be used by another + * client before the owner calls mbox_free_channel. + * After assignment, any packet received on this channel will be + * handed over to the client via the 'rx_callback'. + * The framework holds reference to the client, so the mbox_client + * structure shouldn't be modified until the mbox_free_channel returns. + * + * Return: Pointer to the channel assigned to the client if successful. + * ERR_PTR for request failure. + */ +struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index) +{ + struct device *dev = cl->dev; + struct mbox_controller *mbox; + struct of_phandle_args spec; + struct mbox_chan *chan; + unsigned long flags; + int ret; + + if (!dev || !dev->of_node) { + pr_debug("%s: No owner device node\n", __func__); + return ERR_PTR(-ENODEV); + } + + mutex_lock(&con_mutex); + + if (of_parse_phandle_with_args(dev->of_node, "mboxes", + "#mbox-cells", index, &spec)) { + pr_debug("%s: can't parse \"mboxes\" property\n", __func__); + mutex_unlock(&con_mutex); + return ERR_PTR(-ENODEV); + } + + chan = NULL; + list_for_each_entry(mbox, &mbox_cons, node) + if (mbox->dev->of_node == spec.np) { + chan = mbox->of_xlate(mbox, &spec); + break; + } + + of_node_put(spec.np); + + if (!chan || chan->cl || !try_module_get(mbox->dev->driver->owner)) { + pr_debug("%s: mailbox not free\n", __func__); + mutex_unlock(&con_mutex); + return ERR_PTR(-EBUSY); + } + + spin_lock_irqsave(&chan->lock, flags); + chan->msg_free = 0; + chan->msg_count = 0; + chan->active_req = NULL; + chan->cl = cl; + init_completion(&chan->tx_complete); + + if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) + chan->txdone_method |= TXDONE_BY_ACK; + + spin_unlock_irqrestore(&chan->lock, flags); + + ret = chan->mbox->ops->startup(chan); + if (ret) { + pr_err("Unable to startup the chan (%d)\n", ret); + mbox_free_channel(chan); + chan = ERR_PTR(ret); + } + + mutex_unlock(&con_mutex); + return chan; +} +EXPORT_SYMBOL_GPL(mbox_request_channel); + +/** + * mbox_free_channel - The client relinquishes control of a mailbox + * channel by this call. + * @chan: The mailbox channel to be freed. + */ +void mbox_free_channel(struct mbox_chan *chan) +{ + unsigned long flags; + + if (!chan || !chan->cl) + return; + + chan->mbox->ops->shutdown(chan); + + /* The queued TX requests are simply aborted, no callbacks are made */ + spin_lock_irqsave(&chan->lock, flags); + chan->cl = NULL; + chan->active_req = NULL; + if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)) + chan->txdone_method = TXDONE_BY_POLL; + + module_put(chan->mbox->dev->driver->owner); + spin_unlock_irqrestore(&chan->lock, flags); +} +EXPORT_SYMBOL_GPL(mbox_free_channel); + +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]; +} + +/** + * mbox_controller_register - Register the mailbox controller + * @mbox: Pointer to the mailbox controller. + * + * The controller driver registers its communication channels + */ +int mbox_controller_register(struct mbox_controller *mbox) +{ + int i, txdone; + + /* Sanity check */ + if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans) + return -EINVAL; + + if (mbox->txdone_irq) + txdone = TXDONE_BY_IRQ; + else if (mbox->txdone_poll) + txdone = TXDONE_BY_POLL; + else /* It has to be ACK then */ + txdone = TXDONE_BY_ACK; + + if (txdone == TXDONE_BY_POLL) { + mbox->poll.function = &poll_txdone; + mbox->poll.data = (unsigned long)mbox; + init_timer(&mbox->poll); + } + + for (i = 0; i < mbox->num_chans; i++) { + struct mbox_chan *chan = &mbox->chans[i]; + + chan->cl = NULL; + chan->mbox = mbox; + chan->txdone_method = txdone; + spin_lock_init(&chan->lock); + } + + if (!mbox->of_xlate) + mbox->of_xlate = of_mbox_index_xlate; + + mutex_lock(&con_mutex); + list_add_tail(&mbox->node, &mbox_cons); + mutex_unlock(&con_mutex); + + return 0; +} +EXPORT_SYMBOL_GPL(mbox_controller_register); + +/** + * mbox_controller_unregister - Unregister the mailbox controller + * @mbox: Pointer to the mailbox controller. + */ +void mbox_controller_unregister(struct mbox_controller *mbox) +{ + int i; + + if (!mbox) + return; + + mutex_lock(&con_mutex); + + list_del(&mbox->node); + + for (i = 0; i < mbox->num_chans; i++) + mbox_free_channel(&mbox->chans[i]); + + if (mbox->txdone_poll) + del_timer_sync(&mbox->poll); + + mutex_unlock(&con_mutex); +} +EXPORT_SYMBOL_GPL(mbox_controller_unregister); diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h new file mode 100644 index 0000000..0f8cba6 --- /dev/null +++ b/include/linux/mailbox_client.h @@ -0,0 +1,45 @@ +/* + * Copyright (C) 2013-2014 Linaro Ltd. + * Author: Jassi Brar <jassisinghbrar@gmail.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __MAILBOX_CLIENT_H +#define __MAILBOX_CLIENT_H + +#include <linux/of.h> + +struct mbox_chan; + +/** + * struct mbox_client - User of a mailbox + * @dev: The client device + * @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 + */ +struct mbox_client { + struct device *dev; + bool tx_block; + unsigned long tx_tout; + bool knows_txdone; + + void (*rx_callback)(struct mbox_client *cl, void *mssg); + void (*tx_done)(struct mbox_client *cl, void *mssg, int r); +}; + +struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index); +int mbox_send_message(struct mbox_chan *chan, void *mssg); +void mbox_client_txdone(struct mbox_chan *chan, int r); /* atomic */ +bool mbox_client_peek_data(struct mbox_chan *chan); /* atomic */ +void mbox_free_channel(struct mbox_chan *chan); /* may sleep */ + +#endif /* __MAILBOX_CLIENT_H */ diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h new file mode 100644 index 0000000..976c4ed --- /dev/null +++ b/include/linux/mailbox_controller.h @@ -0,0 +1,131 @@ +/* + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __MAILBOX_CONTROLLER_H +#define __MAILBOX_CONTROLLER_H + +#include <linux/of.h> + +struct mbox_chan; + +/** + * struct mbox_chan_ops - methods to control mailbox channels + * @send_data: The API asks the MBOX controller driver, in atomic + * context try to transmit a message on the bus. Returns 0 if + * data is accepted for transmission, -EBUSY while rejecting + * if the remote hasn't yet read the last data sent. Actual + * transmission of data is reported by the controller via + * mbox_chan_txdone (if it has some TX ACK irq). It must not + * sleep. + * @startup: Called when a client requests the chan. The controller + * could ask clients for additional parameters of communication + * to be provided via client's chan_data. This call may + * block. After this call the Controller must forward any + * data received on the chan by calling mbox_chan_received_data. + * The controller may do stuff that need to sleep. + * @shutdown: Called when a client relinquishes control of a chan. + * This call may block too. The controller must not forward + * any received data anymore. + * The controller may do stuff that need to sleep. + * @last_tx_done: If the controller sets 'txdone_poll', the API calls + * this to poll status of last TX. The controller must + * give priority to IRQ method over polling and never + * set both txdone_poll and txdone_irq. Only in polling + * mode 'send_data' is expected to return -EBUSY. + * The controller may do stuff that need to sleep/block. + * Used only if txdone_poll:=true && txdone_irq:=false + * @peek_data: Atomic check for any received data. Return true if controller + * has some data to push to the client. False otherwise. + */ +struct mbox_chan_ops { + int (*send_data)(struct mbox_chan *chan, void *data); + int (*startup)(struct mbox_chan *chan); + void (*shutdown)(struct mbox_chan *chan); + bool (*last_tx_done)(struct mbox_chan *chan); + bool (*peek_data)(struct mbox_chan *chan); +}; + +/** + * struct mbox_controller - Controller of a class of communication channels + * @dev: Device backing this controller + * @ops: Operators that work on each communication chan + * @chans: Array of channels + * @num_chans: Number of channels in the 'chans' array. + * @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 + * @of_xlate: Controller driver specific mapping of channel via DT + * @poll: API private. Used to poll for TXDONE on all channels. + * @period: API private. Polling period. + * @node: API private. To hook into list of controllers. + */ +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); + /* Internal to API */ + struct timer_list poll; + unsigned period; + struct list_head node; +}; + +/* + * The length of circular buffer for queuing messages from a client. + * 'msg_count' tracks the number of buffered messages while 'msg_free' + * is the index where the next message would be buffered. + * We shouldn't need it too big because every transfer is interrupt + * triggered and if we have lots of data to transfer, the interrupt + * latencies are going to be the bottleneck, not the buffer length. + * Besides, mbox_send_message could be called from atomic context and + * the client could also queue another message from the notifier 'tx_done' + * of the last transfer done. + * REVISIT: If too many platforms see the "Try increasing MBOX_TX_QUEUE_LEN" + * print, it needs to be taken from config option or somesuch. + */ +#define MBOX_TX_QUEUE_LEN 20 + +/** + * struct mbox_chan - s/w representation of a communication chan + * @mbox: Pointer to the parent/provider of this channel + * @txdone_method: Way to detect TXDone chosen by the API + * @cl: Pointer to the current owner of this channel + * @tx_complete: Transmission completion + * @active_req: Currently active request hook + * @msg_count: No. of mssg currently queued + * @msg_free: Index of next available mssg slot + * @msg_data: Hook for data packet + * @lock: Serialise access to the channel + * @con_priv: Hook for controller driver to attach private data + */ +struct mbox_chan { + struct mbox_controller *mbox; + unsigned txdone_method; + struct mbox_client *cl; + struct completion tx_complete; + void *active_req; + unsigned msg_count, msg_free; + void *msg_data[MBOX_TX_QUEUE_LEN]; + spinlock_t lock; /* Serialise access to the channel */ + void *con_priv; +}; + +int mbox_controller_register(struct mbox_controller *mbox); /* can sleep */ +void mbox_controller_unregister(struct mbox_controller *mbox); /* can sleep */ +void mbox_chan_received_data(struct mbox_chan *chan, void *data); /* atomic */ +void mbox_chan_txdone(struct mbox_chan *chan, int r); /* atomic */ + +#endif /* __MAILBOX_CONTROLLER_H */
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@linaro.org> --- MAINTAINERS | 8 + drivers/mailbox/Makefile | 4 + drivers/mailbox/mailbox.c | 467 +++++++++++++++++++++++++++++++++++++ include/linux/mailbox_client.h | 45 ++++ include/linux/mailbox_controller.h | 131 +++++++++++ 5 files changed, 655 insertions(+) create mode 100644 drivers/mailbox/mailbox.c create mode 100644 include/linux/mailbox_client.h create mode 100644 include/linux/mailbox_controller.h