| Message ID | 20180702114033.15654-4-mperttunen@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 02, 2018 at 02:40:28PM +0300, Mikko Perttunen wrote: > Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the > send_data function of the mailbox driver is expected to block until > the message has been sent. The new option is used with the Tegra > Combined UART driver to minimize unnecessary overhead when transmitting > data. > > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> > --- > drivers/mailbox/mailbox.c | 30 +++++++++++++++++++++--------- > drivers/mailbox/mailbox.h | 1 + > 2 files changed, 22 insertions(+), 9 deletions(-) > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > index 674b35f402f5..5c76b70e673c 100644 > --- a/drivers/mailbox/mailbox.c > +++ b/drivers/mailbox/mailbox.c > @@ -53,6 +53,8 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg) > return idx; > } > > +static void tx_tick(struct mbox_chan *chan, int r, bool submit_next); > + > static void msg_submit(struct mbox_chan *chan) > { > unsigned count, idx; > @@ -60,10 +62,13 @@ static void msg_submit(struct mbox_chan *chan) > void *data; > int err = -EBUSY; > > +next: > spin_lock_irqsave(&chan->lock, flags); > > - if (!chan->msg_count || chan->active_req) > - goto exit; > + if (!chan->msg_count || chan->active_req) { > + spin_unlock_irqrestore(&chan->lock, flags); > + return; > + } > > count = chan->msg_count; > idx = chan->msg_free; > @@ -82,15 +87,21 @@ static void msg_submit(struct mbox_chan *chan) > chan->active_req = data; > chan->msg_count--; > } > -exit: > + > spin_unlock_irqrestore(&chan->lock, flags); > > if (!err && (chan->txdone_method & TXDONE_BY_POLL)) > /* kick start the timer immediately to avoid delays */ > hrtimer_start(&chan->mbox->poll_hrt, 0, HRTIMER_MODE_REL); > + > + if (chan->txdone_method & TXDONE_BY_BLOCK) { > + tx_tick(chan, err, false); > + if (!err) > + goto next; > + } This bit is slightly confusing: if check for err, but the call to the tx_tick() function doesn't actually change err (it's passed by value). Now, I don't have any great ideas on how to improve this, but perhaps a simple comment would help clarify what's going on here, or maybe by adding an extra blank line between the tx_tick() call and the conditional would make it more obvious that these aren't related. Also, it looks to me like this is actually modifying msg_submit() to actually do more than just submit the next data from a message. This effectively flushes the complete message via the mailbox, right? Perhaps a slightly clearer implmentation would be to turn this into a separate function that repeatedly calls msg_submit() in a loop or some such. > } > > -static void tx_tick(struct mbox_chan *chan, int r) > +static void tx_tick(struct mbox_chan *chan, int r, bool submit_next) > { > unsigned long flags; > void *mssg; > @@ -101,7 +112,8 @@ static void tx_tick(struct mbox_chan *chan, int r) > spin_unlock_irqrestore(&chan->lock, flags); > > /* Submit next message */ > - msg_submit(chan); > + if (submit_next) > + msg_submit(chan); > > if (!mssg) > return; > @@ -127,7 +139,7 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer) > if (chan->active_req && chan->cl) { > txdone = chan->mbox->ops->last_tx_done(chan); > if (txdone) > - tx_tick(chan, 0); > + tx_tick(chan, 0, true); > else > resched = true; > } > @@ -176,7 +188,7 @@ void mbox_chan_txdone(struct mbox_chan *chan, int r) > return; > } > > - tx_tick(chan, r); > + tx_tick(chan, r, true); > } > EXPORT_SYMBOL_GPL(mbox_chan_txdone); > > @@ -196,7 +208,7 @@ void mbox_client_txdone(struct mbox_chan *chan, int r) > return; > } > > - tx_tick(chan, r); > + tx_tick(chan, r, true); > } > EXPORT_SYMBOL_GPL(mbox_client_txdone); > > @@ -275,7 +287,7 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) > ret = wait_for_completion_timeout(&chan->tx_complete, wait); > if (ret == 0) { > t = -ETIME; > - tx_tick(chan, t); > + tx_tick(chan, t, true); > } > } > > diff --git a/drivers/mailbox/mailbox.h b/drivers/mailbox/mailbox.h > index 456ba68513bb..ec68e2e28cd6 100644 > --- a/drivers/mailbox/mailbox.h > +++ b/drivers/mailbox/mailbox.h > @@ -10,5 +10,6 @@ > #define TXDONE_BY_IRQ BIT(0) /* controller has remote RTR irq */ > #define TXDONE_BY_POLL BIT(1) /* controller can read status of last TX */ > #define TXDONE_BY_ACK BIT(2) /* S/W ACK recevied by Client ticks the TX */ > +#define TXDONE_BY_BLOCK BIT(3) /* mailbox driver send_data blocks until done */ Perhaps clarify here that it blocks until all data in the message has been transmitted. As it is, this could mean just block until the current datum is done transmitting. Thierry
On Mon, Jul 02, 2018 at 02:40:28PM +0300, Mikko Perttunen wrote: > Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the > send_data function of the mailbox driver is expected to block until > the message has been sent. The new option is used with the Tegra > Combined UART driver to minimize unnecessary overhead when transmitting > data. > > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> > --- > drivers/mailbox/mailbox.c | 30 +++++++++++++++++++++--------- > drivers/mailbox/mailbox.h | 1 + > 2 files changed, 22 insertions(+), 9 deletions(-) Jassi, any comments on this patch and 4/8 and 5/8? Greg's already said that he'd be fine if I take the serial driver (patch 6/8) through the Tegra tree to resolve the dependencies. If you're okay with the three mailbox patches I can pick them up as well with your Acked-by. Otherwise, if you'd prefer to take these through the mailbox tree, let me know and I'll wait until sometime after v4.19-rc1 before I send out pull requests for the DT changes. Thanks, Thierry
On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen <mperttunen@nvidia.com> wrote: > Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the > send_data function of the mailbox driver is expected to block until > the message has been sent. The new option is used with the Tegra > Combined UART driver to minimize unnecessary overhead when transmitting > data. > 1) TXDONE_BY_BLOCK flag :- Have you tried setting the flag mbox_chan->mbox_client->tx_block ? 2) Implementing TEGRA_HSP_MBOX_TYPE_SM :- In mailbox framework, a controller is a collection of identical channels. That is, instances of the same class. So ideally, in probe you should populate a controller for each type of channel, i.e, DB, SM, SS and AS.
On 08/03/2018 03:54 PM, Jassi Brar wrote: > On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen <mperttunen@nvidia.com> wrote: >> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the >> send_data function of the mailbox driver is expected to block until >> the message has been sent. The new option is used with the Tegra >> Combined UART driver to minimize unnecessary overhead when transmitting >> data. >> > 1) TXDONE_BY_BLOCK flag :- > Have you tried setting the flag mbox_chan->mbox_client->tx_block ? No - I suppose I should have done that. I'm a bit concerned about overhead as send_data may be called thousands of times per second, so I tried to make it as close as possible to the downstream driver that just pokes the mailbox register directly. > > 2) Implementing TEGRA_HSP_MBOX_TYPE_SM :- > In mailbox framework, a controller is a collection of identical > channels. That is, instances of the same class. > So ideally, in probe you should populate a controller for each > type of channel, i.e, DB, SM, SS and AS. Hmm, yes, I guess this would be possible if I change the mailbox core to allow registering multiple controllers per device. Thanks! Mikko > -- > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On 04.08.2018 13:45, Mikko Perttunen wrote: > On 08/03/2018 03:54 PM, Jassi Brar wrote: >> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen >> <mperttunen@nvidia.com> wrote: >>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the >>> send_data function of the mailbox driver is expected to block until >>> the message has been sent. The new option is used with the Tegra >>> Combined UART driver to minimize unnecessary overhead when transmitting >>> data. >>> >> 1) TXDONE_BY_BLOCK flag :- >> Have you tried setting the flag >> mbox_chan->mbox_client->tx_block ? > > No - I suppose I should have done that. I'm a bit concerned about > overhead as send_data may be called thousands of times per second, so I > tried to make it as close as possible to the downstream driver that just > pokes the mailbox register directly. I tried using polling in the mailbox framework. Some printing is done from atomic context so it seems tx_block cannot be used - wait_for_completion_timeout understandably does not work in atomic context. I also tried without tx_block, in which case I got some horribly garbled output, but "Try increasing MBOX_TX_QUEUE_LEN" was readable there. Any opinions? Thanks, Mikko > >> >> 2) Implementing TEGRA_HSP_MBOX_TYPE_SM :- >> In mailbox framework, a controller is a collection of identical >> channels. That is, instances of the same class. >> So ideally, in probe you should populate a controller for each >> type of channel, i.e, DB, SM, SS and AS. > > Hmm, yes, I guess this would be possible if I change the mailbox core to > allow registering multiple controllers per device. > > Thanks! > Mikko > >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 8, 2018 at 5:08 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote: > > > On 04.08.2018 13:45, Mikko Perttunen wrote: >> >> On 08/03/2018 03:54 PM, Jassi Brar wrote: >>> >>> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen <mperttunen@nvidia.com> >>> wrote: >>>> >>>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the >>>> send_data function of the mailbox driver is expected to block until >>>> the message has been sent. The new option is used with the Tegra >>>> Combined UART driver to minimize unnecessary overhead when transmitting >>>> data. >>>> >>> 1) TXDONE_BY_BLOCK flag :- >>> Have you tried setting the flag mbox_chan->mbox_client->tx_block >>> ? >> >> >> No - I suppose I should have done that. I'm a bit concerned about overhead >> as send_data may be called thousands of times per second, so I tried to make >> it as close as possible to the downstream driver that just pokes the mailbox >> register directly. > > > I tried using polling in the mailbox framework. Some printing is done from > atomic context so it seems tx_block cannot be used - > wait_for_completion_timeout understandably does not work in atomic context. > I also tried without tx_block, in which case I got some horribly garbled > output, but "Try increasing MBOX_TX_QUEUE_LEN" was readable there. > > Any opinions? > The problems arise because your hardware (SM) supports TXDONE_BY_POLL, but your client drives it by TXDONE_BY_ACK because the older DB channels are so. Please populate SM channels as a separate controller than DB. The DB controller, as is, run by ACK method. The SM controller should be run by polling, i.e, set txdone_poll = true and the poll period small enough. The virtual tty client driver should be able to safely set tx_block from appropriate context.
On 08/08/2018 05:10 PM, Jassi Brar wrote: > On Wed, Aug 8, 2018 at 5:08 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote: >> >> >> On 04.08.2018 13:45, Mikko Perttunen wrote: >>> >>> On 08/03/2018 03:54 PM, Jassi Brar wrote: >>>> >>>> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen <mperttunen@nvidia.com> >>>> wrote: >>>>> >>>>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the >>>>> send_data function of the mailbox driver is expected to block until >>>>> the message has been sent. The new option is used with the Tegra >>>>> Combined UART driver to minimize unnecessary overhead when transmitting >>>>> data. >>>>> >>>> 1) TXDONE_BY_BLOCK flag :- >>>> Have you tried setting the flag mbox_chan->mbox_client->tx_block >>>> ? >>> >>> >>> No - I suppose I should have done that. I'm a bit concerned about overhead >>> as send_data may be called thousands of times per second, so I tried to make >>> it as close as possible to the downstream driver that just pokes the mailbox >>> register directly. >> >> >> I tried using polling in the mailbox framework. Some printing is done from >> atomic context so it seems tx_block cannot be used - >> wait_for_completion_timeout understandably does not work in atomic context. >> I also tried without tx_block, in which case I got some horribly garbled >> output, but "Try increasing MBOX_TX_QUEUE_LEN" was readable there. >> >> Any opinions? >> > The problems arise because your hardware (SM) supports TXDONE_BY_POLL, > but your client drives it by TXDONE_BY_ACK because the older DB > channels are so. > > Please populate SM channels as a separate controller than DB. > The DB controller, as is, run by ACK method. > The SM controller should be run by polling, i.e, set txdone_poll = > true and the poll period small enough. The virtual tty client driver > should be able to safely set tx_block from appropriate context. > Sorry, I should have clarified that I already split up the controllers. The SM controller has txdone_poll = true. I didn't adjust txpoll_period so I guess it's zero. Mikko
On Wed, Aug 8, 2018 at 8:04 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote: > > > On 08/08/2018 05:10 PM, Jassi Brar wrote: >> >> On Wed, Aug 8, 2018 at 5:08 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote: >>> >>> >>> >>> On 04.08.2018 13:45, Mikko Perttunen wrote: >>>> >>>> >>>> On 08/03/2018 03:54 PM, Jassi Brar wrote: >>>>> >>>>> >>>>> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen <mperttunen@nvidia.com> >>>>> wrote: >>>>>> >>>>>> >>>>>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the >>>>>> send_data function of the mailbox driver is expected to block until >>>>>> the message has been sent. The new option is used with the Tegra >>>>>> Combined UART driver to minimize unnecessary overhead when >>>>>> transmitting >>>>>> data. >>>>>> >>>>> 1) TXDONE_BY_BLOCK flag :- >>>>> Have you tried setting the flag >>>>> mbox_chan->mbox_client->tx_block >>>>> ? >>>> >>>> >>>> >>>> No - I suppose I should have done that. I'm a bit concerned about >>>> overhead >>>> as send_data may be called thousands of times per second, so I tried to >>>> make >>>> it as close as possible to the downstream driver that just pokes the >>>> mailbox >>>> register directly. >>> >>> >>> >>> I tried using polling in the mailbox framework. Some printing is done >>> from >>> atomic context so it seems tx_block cannot be used - >>> wait_for_completion_timeout understandably does not work in atomic >>> context. >>> I also tried without tx_block, in which case I got some horribly garbled >>> output, but "Try increasing MBOX_TX_QUEUE_LEN" was readable there. >>> >>> Any opinions? >>> >> The problems arise because your hardware (SM) supports TXDONE_BY_POLL, >> but your client drives it by TXDONE_BY_ACK because the older DB >> channels are so. >> >> Please populate SM channels as a separate controller than DB. >> The DB controller, as is, run by ACK method. >> The SM controller should be run by polling, i.e, set txdone_poll = >> true and the poll period small enough. The virtual tty client driver >> should be able to safely set tx_block from appropriate context. >> > > Sorry, I should have clarified that I already split up the controllers. The > SM controller has txdone_poll = true. I didn't adjust txpoll_period so I > guess it's zero. > Can you please share your code (controller and client) ? Maybe offline if you wish.
On 08/08/2018 05:39 PM, Jassi Brar wrote: > On Wed, Aug 8, 2018 at 8:04 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote: >> >> >> On 08/08/2018 05:10 PM, Jassi Brar wrote: >>> >>> On Wed, Aug 8, 2018 at 5:08 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote: >>>> >>>> >>>> >>>> On 04.08.2018 13:45, Mikko Perttunen wrote: >>>>> >>>>> >>>>> On 08/03/2018 03:54 PM, Jassi Brar wrote: >>>>>> >>>>>> >>>>>> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen <mperttunen@nvidia.com> >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the >>>>>>> send_data function of the mailbox driver is expected to block until >>>>>>> the message has been sent. The new option is used with the Tegra >>>>>>> Combined UART driver to minimize unnecessary overhead when >>>>>>> transmitting >>>>>>> data. >>>>>>> >>>>>> 1) TXDONE_BY_BLOCK flag :- >>>>>> Have you tried setting the flag >>>>>> mbox_chan->mbox_client->tx_block >>>>>> ? >>>>> >>>>> >>>>> >>>>> No - I suppose I should have done that. I'm a bit concerned about >>>>> overhead >>>>> as send_data may be called thousands of times per second, so I tried to >>>>> make >>>>> it as close as possible to the downstream driver that just pokes the >>>>> mailbox >>>>> register directly. >>>> >>>> >>>> >>>> I tried using polling in the mailbox framework. Some printing is done >>>> from >>>> atomic context so it seems tx_block cannot be used - >>>> wait_for_completion_timeout understandably does not work in atomic >>>> context. >>>> I also tried without tx_block, in which case I got some horribly garbled >>>> output, but "Try increasing MBOX_TX_QUEUE_LEN" was readable there. >>>> >>>> Any opinions? >>>> >>> The problems arise because your hardware (SM) supports TXDONE_BY_POLL, >>> but your client drives it by TXDONE_BY_ACK because the older DB >>> channels are so. >>> >>> Please populate SM channels as a separate controller than DB. >>> The DB controller, as is, run by ACK method. >>> The SM controller should be run by polling, i.e, set txdone_poll = >>> true and the poll period small enough. The virtual tty client driver >>> should be able to safely set tx_block from appropriate context. >>> >> >> Sorry, I should have clarified that I already split up the controllers. The >> SM controller has txdone_poll = true. I didn't adjust txpoll_period so I >> guess it's zero. >> > Can you please share your code (controller and client) ? Maybe offline > if you wish. > I'll upload a git branch tomorrow -- I'm not at the machine with the code now. Mikko
Here's my current code: https://github.com/cyndis/linux/commits/wip/t194-tcu-4 "fixup! mailbox: tegra-hsp: Add support for shared mailboxes" splits up the controller into two. "tegra-hsp: use polling" changes it to use polling. There are two lines in the top patch with comments: - at the end of tegra_hsp_mailbox_send_data, I left a "while (!tegra_hsp_mailbox_last_tx_done(chan));". Without it I wasn't able to see even a few garbled characters in the output. - as mentioned, if I enable tx_block on the client side, I get a BUG: scheduling while atomic. I assume this gets printed through the earlycon as it's printing out correctly. Thanks, Mikko On 08.08.2018 17:46, Mikko Perttunen wrote: > On 08/08/2018 05:39 PM, Jassi Brar wrote: >> On Wed, Aug 8, 2018 at 8:04 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote: >>> >>> >>> On 08/08/2018 05:10 PM, Jassi Brar wrote: >>>> >>>> On Wed, Aug 8, 2018 at 5:08 PM, Mikko Perttunen <cyndis@kapsi.fi> >>>> wrote: >>>>> >>>>> >>>>> >>>>> On 04.08.2018 13:45, Mikko Perttunen wrote: >>>>>> >>>>>> >>>>>> On 08/03/2018 03:54 PM, Jassi Brar wrote: >>>>>>> >>>>>>> >>>>>>> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen >>>>>>> <mperttunen@nvidia.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> >>>>>>>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the >>>>>>>> send_data function of the mailbox driver is expected to block until >>>>>>>> the message has been sent. The new option is used with the Tegra >>>>>>>> Combined UART driver to minimize unnecessary overhead when >>>>>>>> transmitting >>>>>>>> data. >>>>>>>> >>>>>>> 1) TXDONE_BY_BLOCK flag :- >>>>>>> Have you tried setting the flag >>>>>>> mbox_chan->mbox_client->tx_block >>>>>>> ? >>>>>> >>>>>> >>>>>> >>>>>> No - I suppose I should have done that. I'm a bit concerned about >>>>>> overhead >>>>>> as send_data may be called thousands of times per second, so I >>>>>> tried to >>>>>> make >>>>>> it as close as possible to the downstream driver that just pokes the >>>>>> mailbox >>>>>> register directly. >>>>> >>>>> >>>>> >>>>> I tried using polling in the mailbox framework. Some printing is done >>>>> from >>>>> atomic context so it seems tx_block cannot be used - >>>>> wait_for_completion_timeout understandably does not work in atomic >>>>> context. >>>>> I also tried without tx_block, in which case I got some horribly >>>>> garbled >>>>> output, but "Try increasing MBOX_TX_QUEUE_LEN" was readable there. >>>>> >>>>> Any opinions? >>>>> >>>> The problems arise because your hardware (SM) supports TXDONE_BY_POLL, >>>> but your client drives it by TXDONE_BY_ACK because the older DB >>>> channels are so. >>>> >>>> Please populate SM channels as a separate controller than DB. >>>> The DB controller, as is, run by ACK method. >>>> The SM controller should be run by polling, i.e, set txdone_poll = >>>> true and the poll period small enough. The virtual tty client driver >>>> should be able to safely set tx_block from appropriate context. >>>> >>> >>> Sorry, I should have clarified that I already split up the >>> controllers. The >>> SM controller has txdone_poll = true. I didn't adjust txpoll_period so I >>> guess it's zero. >>> >> Can you please share your code (controller and client) ? Maybe offline >> if you wish. >> > > I'll upload a git branch tomorrow -- I'm not at the machine with the > code now. > > Mikko > -- > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
[dropping everyone else while we discuss the code] Thanks. I assume that branch as all the code that there is. Let me look and get back to you. On Thu, Aug 9, 2018 at 2:19 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote: > Here's my current code: > > https://github.com/cyndis/linux/commits/wip/t194-tcu-4 > > "fixup! mailbox: tegra-hsp: Add support for shared mailboxes" splits up the > controller into two. "tegra-hsp: use polling" changes it to use polling. > > There are two lines in the top patch with comments: > > - at the end of tegra_hsp_mailbox_send_data, I left a "while > (!tegra_hsp_mailbox_last_tx_done(chan));". Without it I wasn't able to see > even a few garbled characters in the output. > > - as mentioned, if I enable tx_block on the client side, I get a BUG: > scheduling while atomic. I assume this gets printed through the earlycon as > it's printing out correctly. > > Thanks, > Mikko > > > On 08.08.2018 17:46, Mikko Perttunen wrote: >> >> On 08/08/2018 05:39 PM, Jassi Brar wrote: >>> >>> On Wed, Aug 8, 2018 at 8:04 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote: >>>> >>>> >>>> >>>> On 08/08/2018 05:10 PM, Jassi Brar wrote: >>>>> >>>>> >>>>> On Wed, Aug 8, 2018 at 5:08 PM, Mikko Perttunen <cyndis@kapsi.fi> >>>>> wrote: >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On 04.08.2018 13:45, Mikko Perttunen wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 08/03/2018 03:54 PM, Jassi Brar wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen >>>>>>>> <mperttunen@nvidia.com> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the >>>>>>>>> send_data function of the mailbox driver is expected to block until >>>>>>>>> the message has been sent. The new option is used with the Tegra >>>>>>>>> Combined UART driver to minimize unnecessary overhead when >>>>>>>>> transmitting >>>>>>>>> data. >>>>>>>>> >>>>>>>> 1) TXDONE_BY_BLOCK flag :- >>>>>>>> Have you tried setting the flag >>>>>>>> mbox_chan->mbox_client->tx_block >>>>>>>> ? >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> No - I suppose I should have done that. I'm a bit concerned about >>>>>>> overhead >>>>>>> as send_data may be called thousands of times per second, so I tried >>>>>>> to >>>>>>> make >>>>>>> it as close as possible to the downstream driver that just pokes the >>>>>>> mailbox >>>>>>> register directly. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> I tried using polling in the mailbox framework. Some printing is done >>>>>> from >>>>>> atomic context so it seems tx_block cannot be used - >>>>>> wait_for_completion_timeout understandably does not work in atomic >>>>>> context. >>>>>> I also tried without tx_block, in which case I got some horribly >>>>>> garbled >>>>>> output, but "Try increasing MBOX_TX_QUEUE_LEN" was readable there. >>>>>> >>>>>> Any opinions? >>>>>> >>>>> The problems arise because your hardware (SM) supports TXDONE_BY_POLL, >>>>> but your client drives it by TXDONE_BY_ACK because the older DB >>>>> channels are so. >>>>> >>>>> Please populate SM channels as a separate controller than DB. >>>>> The DB controller, as is, run by ACK method. >>>>> The SM controller should be run by polling, i.e, set txdone_poll = >>>>> true and the poll period small enough. The virtual tty client driver >>>>> should be able to safely set tx_block from appropriate context. >>>>> >>>> >>>> Sorry, I should have clarified that I already split up the controllers. >>>> The >>>> SM controller has txdone_poll = true. I didn't adjust txpoll_period so I >>>> guess it's zero. >>>> >>> Can you please share your code (controller and client) ? Maybe offline >>> if you wish. >>> >> >> I'll upload a git branch tomorrow -- I'm not at the machine with the code >> now. >> >> Mikko >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c index 674b35f402f5..5c76b70e673c 100644 --- a/drivers/mailbox/mailbox.c +++ b/drivers/mailbox/mailbox.c @@ -53,6 +53,8 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg) return idx; } +static void tx_tick(struct mbox_chan *chan, int r, bool submit_next); + static void msg_submit(struct mbox_chan *chan) { unsigned count, idx; @@ -60,10 +62,13 @@ static void msg_submit(struct mbox_chan *chan) void *data; int err = -EBUSY; +next: spin_lock_irqsave(&chan->lock, flags); - if (!chan->msg_count || chan->active_req) - goto exit; + if (!chan->msg_count || chan->active_req) { + spin_unlock_irqrestore(&chan->lock, flags); + return; + } count = chan->msg_count; idx = chan->msg_free; @@ -82,15 +87,21 @@ static void msg_submit(struct mbox_chan *chan) chan->active_req = data; chan->msg_count--; } -exit: + spin_unlock_irqrestore(&chan->lock, flags); if (!err && (chan->txdone_method & TXDONE_BY_POLL)) /* kick start the timer immediately to avoid delays */ hrtimer_start(&chan->mbox->poll_hrt, 0, HRTIMER_MODE_REL); + + if (chan->txdone_method & TXDONE_BY_BLOCK) { + tx_tick(chan, err, false); + if (!err) + goto next; + } } -static void tx_tick(struct mbox_chan *chan, int r) +static void tx_tick(struct mbox_chan *chan, int r, bool submit_next) { unsigned long flags; void *mssg; @@ -101,7 +112,8 @@ static void tx_tick(struct mbox_chan *chan, int r) spin_unlock_irqrestore(&chan->lock, flags); /* Submit next message */ - msg_submit(chan); + if (submit_next) + msg_submit(chan); if (!mssg) return; @@ -127,7 +139,7 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer) if (chan->active_req && chan->cl) { txdone = chan->mbox->ops->last_tx_done(chan); if (txdone) - tx_tick(chan, 0); + tx_tick(chan, 0, true); else resched = true; } @@ -176,7 +188,7 @@ void mbox_chan_txdone(struct mbox_chan *chan, int r) return; } - tx_tick(chan, r); + tx_tick(chan, r, true); } EXPORT_SYMBOL_GPL(mbox_chan_txdone); @@ -196,7 +208,7 @@ void mbox_client_txdone(struct mbox_chan *chan, int r) return; } - tx_tick(chan, r); + tx_tick(chan, r, true); } EXPORT_SYMBOL_GPL(mbox_client_txdone); @@ -275,7 +287,7 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) ret = wait_for_completion_timeout(&chan->tx_complete, wait); if (ret == 0) { t = -ETIME; - tx_tick(chan, t); + tx_tick(chan, t, true); } } diff --git a/drivers/mailbox/mailbox.h b/drivers/mailbox/mailbox.h index 456ba68513bb..ec68e2e28cd6 100644 --- a/drivers/mailbox/mailbox.h +++ b/drivers/mailbox/mailbox.h @@ -10,5 +10,6 @@ #define TXDONE_BY_IRQ BIT(0) /* controller has remote RTR irq */ #define TXDONE_BY_POLL BIT(1) /* controller can read status of last TX */ #define TXDONE_BY_ACK BIT(2) /* S/W ACK recevied by Client ticks the TX */ +#define TXDONE_BY_BLOCK BIT(3) /* mailbox driver send_data blocks until done */ #endif /* __MAILBOX_H */
Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the send_data function of the mailbox driver is expected to block until the message has been sent. The new option is used with the Tegra Combined UART driver to minimize unnecessary overhead when transmitting data. Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> --- drivers/mailbox/mailbox.c | 30 +++++++++++++++++++++--------- drivers/mailbox/mailbox.h | 1 + 2 files changed, 22 insertions(+), 9 deletions(-)