[PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

On 17-02-16 10:20 PM, Jassi Brar wrote:
> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
> <jonathan.richardson at broadcom.com> wrote:
>
>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>> +{
>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>> +               unsigned long flags;
>> +       int err = 0;
>> +       const int poll_period_us = 5;
>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>> +
>> +       if (!msg)
>> +               return -EINVAL;
>> +
>> +       spin_lock_irqsave(&mbox->lock, flags);
>> +
>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>> +               msg->cmd, msg->param, msg->wait_ack);
>> +
> prints should be outside the spinlocks.
>
>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>> +
>> +       if (msg->wait_ack) {
>> +               int retries;
>> +
> move poll_period_us and max_retries in here or just define' them
>
>> +               err = msg->reply_code = -ETIMEDOUT;
>> +               for (retries = 0; retries < max_retries; retries++) {
>> +                       u32 val = readl(
>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>> +                               /*
>> +                                * M0 replied - save reply code and
>> +                                * clear error.
>> +                                */
>> +                               msg->reply_code = (val &
>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>> +                               err = 0;
>> +                               break;
>> +                       }
>> +                       udelay(poll_period_us);
>>
> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
> a simple 'peek_data' and call it for requests with 'wait_ack'
Hi Jassi. The M0 response is normally 25-30 us. We have one message that takes 280us but we can get rid of it if necessary. Regarding your suggestion of peek_data. I don't see how it can be used without the spinlock to serialize multiple clients/channels over a single mailbox channel. peek_data is going to allow the client to poll for data. But the spinlock is there to prevent other clients from accessing the mailbox channel until any pending transaction is complete. last_tx_done looks like an option but even that will not prevent another client from clobbering a pending transaction. A shared channel among all clients with a blocking model would probably work, but not pretty. Let me know what you advise. Thanks.