Re: [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver

Jassi,

On 2/12/20 8:02 PM, Jassi Brar wrote:

On Sun, Jan 12, 2020 at 11:18 PM Samuel Holland [off-list ref] wrote:

quoted

+static int sun6i_msgbox_send_data(struct mbox_chan *chan, void *data)
+{
+       struct sun6i_msgbox *mbox = to_sun6i_msgbox(chan);
+       int n = channel_number(chan);
+       uint32_t msg = *(uint32_t *)data;
+
+       /* Using a channel backwards gets the hardware into a bad state. */
+       if (WARN_ON_ONCE(!(readl(mbox->regs + CTRL_REG(n)) & CTRL_TX(n))))
+               return 0;
+
+       /* We cannot post a new message if the FIFO is full. */
+       if (readl(mbox->regs + FIFO_STAT_REG(n)) & FIFO_STAT_MASK) {
+               mbox_dbg(mbox, "Channel %d busy sending 0x%08x\n", n, msg);
+               return -EBUSY;
+       }
+

This check should go into sun6i_msgbox_last_tx_done().
send_data() assumes all is clear to send next packet.

sun6i_msgbox_last_tx_done() already checks that the FIFO is completely empty (as
the big comment explains). So this error could only be hit in the knows_txdone
== true case, if the client pipelines multiple messages by calling
mbox_client_txdone() before the message is actually removed from the FIFO.

From the comments in mailbox_controller.h, this kind of usage looks to be
unsupported. In that case, I could remove the check entirely. Does that sound right?

.....

quoted

+
+       mbox->controller.dev           = dev;
+       mbox->controller.ops           = &sun6i_msgbox_chan_ops;
+       mbox->controller.chans         = chans;
+       mbox->controller.num_chans     = NUM_CHANS;
+       mbox->controller.txdone_irq    = false;
+       mbox->controller.txdone_poll   = true;
+       mbox->controller.txpoll_period = 5;
+

nit:  just a single space should do too.

Sorry, for some reason I thought I had replied to this patch, but
apparently not. My mistake. Do you want to revise this submission or
send another patch on top?

For just this change, it would be simpler to send a follow-up patch.

thanks

Thank you,
Samuel


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel