From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759417AbbLBR1E (ORCPT ); Wed, 2 Dec 2015 12:27:04 -0500 Received: from mail-wm0-f51.google.com ([74.125.82.51]:33834 "EHLO mail-wm0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759370AbbLBR0t (ORCPT ); Wed, 2 Dec 2015 12:26:49 -0500 MIME-Version: 1.0 In-Reply-To: References: <1436922022-17564-1-git-send-email-moritz.fischer@ettus.com> <1436922022-17564-3-git-send-email-moritz.fischer@ettus.com> Date: Wed, 2 Dec 2015 09:26:48 -0800 Message-ID: Subject: Re: [PATCHv7 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox. From: Moritz Fischer To: Jassi Brar Cc: Linux Kernel Mailing List , Rob Herring , Pawel Moll , Mark Rutland , "ijc+devicetree@hellion.org.uk" , Kumar Gala , Michal Simek , =?UTF-8?Q?S=C3=B6ren_Brinkmann?= , Andrew Morton , Greg Kroah-Hartman , mchehab@osg.samsung.com, Arnd Bergmann , joe@perches.com, Jingoo Han , Devicetree List , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jassi, thanks for your feedback. On Mon, Aug 10, 2015 at 1:05 AM, Jassi Brar wrote: > On Wed, Jul 15, 2015 at 6:30 AM, Moritz Fischer > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: moritz.fischer@ettus.com (Moritz Fischer) Date: Wed, 2 Dec 2015 09:26:48 -0800 Subject: [PATCHv7 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox. In-Reply-To: References: <1436922022-17564-1-git-send-email-moritz.fischer@ettus.com> <1436922022-17564-3-git-send-email-moritz.fischer@ettus.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Jassi, thanks for your feedback. On Mon, Aug 10, 2015 at 1:05 AM, Jassi Brar wrote: > On Wed, Jul 15, 2015 at 6:30 AM, Moritz Fischer > 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