Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1282123 > unrolled thread

Re: [PATCHv7 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv7 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox. Moritz Fischer <moritz.fischer@ettus.com> - 2015-12-02 18:30 +0100
    Re: [PATCHv7 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox. Jassi Brar <jaswinder.singh@linaro.org> - 2015-12-03 06:10 +0100

#1282123 — Re: [PATCHv7 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox.

Hi Jassi,

thanks for your feedback.

On Mon, Aug 10, 2015 at 1:05 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Wed, Jul 15, 2015 at 6:30 AM, Moritz Fischer
> <moritz.fischer@ettus.com> wrote:
>
>> +
>> +static void xilinx_mbox_rx_data(struct mbox_chan *chan)
>> +{
>> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
>> +       u32 data;
>> +
>> +       if (xilinx_mbox_pending(mbox)) {
>> +               data = readl_relaxed(mbox->mbox_base + MAILBOX_REG_RDDATA);
>> +               mbox_chan_received_data(chan, (void *)data);
>>
> If RDDATA is a FIFO, then above seems wrong - you are assuming
> messages are always going to be exactly 32-bits for every protocol.
> Ideally you should empty the fifo fully, at least when RX has an
> interrupt.

From my understanding it's hard to tell how much actually is in the fifo,
you can tell if it's full for send direction, or empty for read direction.

Maybe the STI / RTI can be setup in a smart way to work with multiple message
sizes.
>
>> +
>> +static int xilinx_mbox_irq_send_data(struct mbox_chan *chan, void *data)
>> +{
>> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
>> +       u32 *udata = data;
>> +
>> +       if (xilinx_mbox_full(mbox))
>> +               return -EBUSY;
>>
> This check is redundant. last_tx_done already makes sure this is always true.

Good to know. I'll fix it.
>
>> +       /* enable interrupt before send */
>> +       xilinx_mbox_tx_intmask(mbox, true);
>> +
>> +       writel_relaxed(*udata, mbox->mbox_base + MAILBOX_REG_WRDATA);
>> +
> From status EMPTY and FULL, it seems WRDATA is a FIFO. So here also
> you should expect messages to be <= fifo depth. And not assume exactly
> 32bit messages always.

How do I determine the message size? Doesn't
drivers/mailbox/bcm2835-mailbox.c or
mailbox-altera.c make the same assumption?

>
>> +
>> +static bool xilinx_mbox_last_tx_done(struct mbox_chan *chan)
>> +{
>> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
>> +
>> +       /* return false if mailbox is full */
>> +       return !xilinx_mbox_full(mbox);
>>
> Instead of FULL, I think it should check for stricter EMPTY status ...
> mbox api submits only 1 message at a time.

The EMPTY status applies to the receive direction only, I could check
the STI status
bit though I suppose.

[...]
>> +
>> +       mbox->irq = platform_get_irq(pdev, 0);
>> +       /* if irq is present, we can use it, otherwise, poll */
>> +       if (mbox->irq > 0) {
>> +               mbox->controller.txdone_irq = true;
>> +               mbox->controller.ops = &xilinx_mbox_irq_ops;
>> +       } else {
>> +               dev_info(&pdev->dev, "IRQ not found, fallback to polling.\n");
>> +               mbox->controller.txdone_poll = true;
>> +               mbox->controller.txpoll_period = MBOX_POLLING_MS;
>> +               mbox->controller.ops = &xilinx_mbox_poll_ops;
>> +
>> +               setup_timer(&mbox->rxpoll_timer, xilinx_mbox_poll_rx,
>> +                           (unsigned long)&mbox->chan);
>>
> I believe there is indeed some platform that doesn't have RX-Interrupt?
>  If no, please remove this.
>  If yes, you may want to get some hint from platform about the size of
> messages and do mbox_chan_received_data() only upon reading those many
> bytes.

I'd be fine to drop the polling use case for the moment, on my
platform I can wire up the IRQ.
We can always add it back in in a follow up use case.

Thanks,

Moritz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1282712

On 2 December 2015 at 22:56, Moritz Fischer <moritz.fischer@ettus.com> wrote:
> Hi Jassi,
>
> thanks for your feedback.
>
> On Mon, Aug 10, 2015 at 1:05 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Wed, Jul 15, 2015 at 6:30 AM, Moritz Fischer
>> <moritz.fischer@ettus.com> wrote:
>>
>>> +
>>> +static void xilinx_mbox_rx_data(struct mbox_chan *chan)
>>> +{
>>> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
>>> +       u32 data;
>>> +
>>> +       if (xilinx_mbox_pending(mbox)) {
>>> +               data = readl_relaxed(mbox->mbox_base + MAILBOX_REG_RDDATA);
>>> +               mbox_chan_received_data(chan, (void *)data);
>>>
>> If RDDATA is a FIFO, then above seems wrong - you are assuming
>> messages are always going to be exactly 32-bits for every protocol.
>> Ideally you should empty the fifo fully, at least when RX has an
>> interrupt.
>
> From my understanding it's hard to tell how much actually is in the fifo,
> you can tell if it's full for send direction, or empty for read direction.
>
Simply read the whole FIFO and leave it to the client driver to parse
that data according to the protocol it drives.

> Maybe the STI / RTI can be setup in a smart way to work with multiple message
> sizes.
>>
>>> +
>>> +static int xilinx_mbox_irq_send_data(struct mbox_chan *chan, void *data)
>>> +{
>>> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
>>> +       u32 *udata = data;
>>> +
>>> +       if (xilinx_mbox_full(mbox))
>>> +               return -EBUSY;
>>>
>> This check is redundant. last_tx_done already makes sure this is always true.
>
> Good to know. I'll fix it.
>>
>>> +       /* enable interrupt before send */
>>> +       xilinx_mbox_tx_intmask(mbox, true);
>>> +
>>> +       writel_relaxed(*udata, mbox->mbox_base + MAILBOX_REG_WRDATA);
>>> +
>> From status EMPTY and FULL, it seems WRDATA is a FIFO. So here also
>> you should expect messages to be <= fifo depth. And not assume exactly
>> 32bit messages always.
>
> How do I determine the message size?
>
Always expect any write request is exactly the size of FIFO depth.

> Doesn't
> drivers/mailbox/bcm2835-mailbox.c or
>
Well I did point it out
http://lists.infradead.org/pipermail/linux-rpi-kernel/2015-June/001902.html
 ... but developer assumes there will _never_ be need to pass a
message bigger than 32-bits. Sadly overlooking the possibility that
some protocol might have simple, say, 64-bits wide commands and
responses that could avoid using any Shared-Memory at all.

> mailbox-altera.c make the same assumption?
>
There are 2 registers, for CMD and PRT each, that make up 1 message.
It doesn't seem like a fifo.

>>
>>> +
>>> +static bool xilinx_mbox_last_tx_done(struct mbox_chan *chan)
>>> +{
>>> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
>>> +
>>> +       /* return false if mailbox is full */
>>> +       return !xilinx_mbox_full(mbox);
>>>
>> Instead of FULL, I think it should check for stricter EMPTY status ...
>> mbox api submits only 1 message at a time.
>
> The EMPTY status applies to the receive direction only, I could check
> the STI status
> bit though I suppose.
>
I don't know the h/w but you get my idea. So whatever is in the next revision.

> [...]
>>> +
>>> +       mbox->irq = platform_get_irq(pdev, 0);
>>> +       /* if irq is present, we can use it, otherwise, poll */
>>> +       if (mbox->irq > 0) {
>>> +               mbox->controller.txdone_irq = true;
>>> +               mbox->controller.ops = &xilinx_mbox_irq_ops;
>>> +       } else {
>>> +               dev_info(&pdev->dev, "IRQ not found, fallback to polling.\n");
>>> +               mbox->controller.txdone_poll = true;
>>> +               mbox->controller.txpoll_period = MBOX_POLLING_MS;
>>> +               mbox->controller.ops = &xilinx_mbox_poll_ops;
>>> +
>>> +               setup_timer(&mbox->rxpoll_timer, xilinx_mbox_poll_rx,
>>> +                           (unsigned long)&mbox->chan);
>>>
>> I believe there is indeed some platform that doesn't have RX-Interrupt?
>>  If no, please remove this.
>>  If yes, you may want to get some hint from platform about the size of
>> messages and do mbox_chan_received_data() only upon reading those many
>> bytes.
>
> I'd be fine to drop the polling use case for the moment, on my
> platform I can wire up the IRQ.
> We can always add it back in in a follow up use case.
>

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web