Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751100AbbLCFFj (ORCPT ); Thu, 3 Dec 2015 00:05:39 -0500 Received: from mail-vk0-f42.google.com ([209.85.213.42]:35197 "EHLO mail-vk0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750779AbbLCFFh (ORCPT ); Thu, 3 Dec 2015 00:05:37 -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: Thu, 3 Dec 2015 10:35:36 +0530 Message-ID: Subject: Re: [PATCHv7 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox. From: Jassi Brar To: Moritz Fischer Cc: Jassi Brar , Mark Rutland , Devicetree List , Arnd Bergmann , Pawel Moll , "ijc+devicetree@hellion.org.uk" , Greg Kroah-Hartman , mchehab@osg.samsung.com, Linux Kernel Mailing List , Michal Simek , Jingoo Han , Rob Herring , "linux-arm-kernel@lists.infradead.org" , Kumar Gala , joe@perches.com, Andrew Morton , =?UTF-8?Q?S=C3=B6ren_Brinkmann?= Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4635 Lines: 125 On 2 December 2015 at 22:56, Moritz Fischer wrote: > 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. > 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/