Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932743AbbBBHEc (ORCPT ); Mon, 2 Feb 2015 02:04:32 -0500 Received: from mail-ig0-f181.google.com ([209.85.213.181]:45337 "EHLO mail-ig0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932199AbbBBHEa (ORCPT ); Mon, 2 Feb 2015 02:04:30 -0500 MIME-Version: 1.0 In-Reply-To: <1419241498-3290-1-git-send-email-lftan@altera.com> References: <1419241498-3290-1-git-send-email-lftan@altera.com> Date: Mon, 2 Feb 2015 12:34:29 +0530 Message-ID: Subject: Re: [PATCH v2] mailbox: Add Altera mailbox driver From: Jassi Brar To: Ley Foon Tan Cc: Linux Kernel Mailing List , Ley Foon Tan , Suman Anna , devicetree@vger.kernel.org, Dinh Nguyen 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: 7987 Lines: 241 On Mon, Dec 22, 2014 at 3:14 PM, Ley Foon Tan wrote: ... > diff --git a/drivers/mailbox/mailbox-altera.c b/drivers/mailbox/mailbox-altera.c > new file mode 100644 > index 0000000..8019795 > --- /dev/null > +++ b/drivers/mailbox/mailbox-altera.c > @@ -0,0 +1,385 @@ > +/* > + * Copyright Altera Corporation (C) 2013-2014. All rights reserved > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program. If not, see . > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DRIVER_NAME "altera-mailbox" > + > +#define MAILBOX_CMD_REG 0x00 > +#define MAILBOX_PTR_REG 0x04 > +#define MAILBOX_STS_REG 0x08 > +#define MAILBOX_INTMASK_REG 0x0C > + > +#define INT_PENDING_MSK 0x1 > +#define INT_SPACE_MSK 0x2 > + > +#define STS_PENDING_MSK 0x1 > +#define STS_FULL_MSK 0x2 > +#define STS_FULL_OFT 0x1 > + > +#define MBOX_PENDING(status) (((status) & STS_PENDING_MSK)) > +#define MBOX_FULL(status) (((status) & STS_FULL_MSK) >> STS_FULL_OFT) > + > +enum altera_mbox_msg { > + MBOX_CMD = 0, > + MBOX_PTR, > +}; > + > +#define MBOX_POLLING_MS 1 /* polling interval 1ms */ > + The intention is too aggressive - though poll won't be before next jiffy (>1ms). And if you are not expecting a reply, it should be even bigger. Say, 50ms default and 5ms when you expect RX? > +struct altera_mbox { > + bool is_sender; /* 1-sender, 0-receiver */ > + bool intr_mode; > + int irq; > + void __iomem *mbox_base; > + struct device *dev; > + struct mbox_controller controller; > + > + /* If the controller supports only RX polling mode */ > + struct timer_list rxpoll_timer; > +}; > + > +static struct altera_mbox *mbox_chan_to_altera_mbox(struct mbox_chan *chan) > +{ > + if (!chan || !chan->con_priv) > + return NULL; > + > + return (struct altera_mbox *)chan->con_priv; > +} > + > +static inline int altera_mbox_full(struct altera_mbox *mbox) > +{ > + u32 status; > + > + status = readl(mbox->mbox_base + MAILBOX_STS_REG); > + return MBOX_FULL(status); > +} > s/readl/readl_relaxed and s/writel/writel_relaxed everywhere > + > +static inline int altera_mbox_pending(struct altera_mbox *mbox) > +{ > + u32 status; > + > + status = readl(mbox->mbox_base + MAILBOX_STS_REG); > + return MBOX_PENDING(status); > +} > + > +static void altera_mbox_rx_intmask(struct altera_mbox *mbox, bool enable) > +{ > + u32 mask; > + > + mask = readl(mbox->mbox_base + MAILBOX_INTMASK_REG); > + if (enable) > + mask |= INT_PENDING_MSK; > + else > + mask &= ~INT_PENDING_MSK; > + writel(mask, mbox->mbox_base + MAILBOX_INTMASK_REG); > +} > + > +static void altera_mbox_tx_intmask(struct altera_mbox *mbox, bool enable) > +{ > + u32 mask; > + > + mask = readl(mbox->mbox_base + MAILBOX_INTMASK_REG); > + if (enable) > + mask |= INT_SPACE_MSK; > + else > + mask &= ~INT_SPACE_MSK; > + writel(mask, mbox->mbox_base + MAILBOX_INTMASK_REG); > +} > + > +static bool altera_mbox_is_sender(struct altera_mbox *mbox) > +{ > + u32 reg; > + /* Write a magic number to PTR register and read back this register. > + * This register is read-write if it is a sender. > + */ > + #define MBOX_MAGIC 0xA5A5AA55 > + writel(MBOX_MAGIC, mbox->mbox_base + MAILBOX_PTR_REG); > + reg = readl(mbox->mbox_base + MAILBOX_PTR_REG); > + if (reg == MBOX_MAGIC) { > + /* Clear to 0 */ > + writel(0, mbox->mbox_base + MAILBOX_PTR_REG); > + return true; > + } > + return false; > +} > + > +static void altera_mbox_rx_data(struct mbox_chan *chan) > +{ > + struct altera_mbox *mbox = mbox_chan_to_altera_mbox(chan); > + u32 data[2]; > + > + if (altera_mbox_pending(mbox)) { > + data[MBOX_PTR] = readl(mbox->mbox_base + MAILBOX_PTR_REG); > + data[MBOX_CMD] = readl(mbox->mbox_base + MAILBOX_CMD_REG); > + mbox_chan_received_data(chan, (void *)data); > + } > +} > + > +static void altera_mbox_poll_rx(unsigned long data) > +{ > + struct mbox_chan *chan = (struct mbox_chan *)data; > + struct altera_mbox *mbox = mbox_chan_to_altera_mbox(chan); > + > + altera_mbox_rx_data(chan); > + > + mod_timer(&mbox->rxpoll_timer, > + jiffies + msecs_to_jiffies(MBOX_POLLING_MS)); > +} > + > +static irqreturn_t altera_mbox_tx_interrupt(int irq, void *p) > +{ > + struct mbox_chan *chan = (struct mbox_chan *)p; > + struct altera_mbox *mbox = mbox_chan_to_altera_mbox(chan); > + > + altera_mbox_tx_intmask(mbox, false); > + mbox_chan_txdone(chan, 0); > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t altera_mbox_rx_interrupt(int irq, void *p) > +{ > + struct mbox_chan *chan = (struct mbox_chan *)p; > + > + altera_mbox_rx_data(chan); > + return IRQ_HANDLED; > +} > + > +static int altera_mbox_startup_sender(struct mbox_chan *chan) > +{ > + int ret; > + struct altera_mbox *mbox = mbox_chan_to_altera_mbox(chan); > + > + if (mbox->intr_mode) { > + ret = request_irq(mbox->irq, altera_mbox_tx_interrupt, 0, > + DRIVER_NAME, chan); > + if (unlikely(ret)) { > + dev_err(mbox->dev, > + "failed to register mailbox interrupt:%d\n", > + ret); > + return ret; > + } > + } > + > + return 0; > +} > + > +static int altera_mbox_startup_receiver(struct mbox_chan *chan) > +{ > + int ret; > + struct altera_mbox *mbox = mbox_chan_to_altera_mbox(chan); > + > + if (mbox->intr_mode) { > + ret = request_irq(mbox->irq, altera_mbox_rx_interrupt, 0, > + DRIVER_NAME, chan); > + if (unlikely(ret)) { > + dev_err(mbox->dev, > + "failed to register mailbox interrupt:%d\n", > + ret); > + return ret; > Is this really fatal? Maybe continue using the timer scheme below? > + } > + altera_mbox_rx_intmask(mbox, true); > + } else { > + /* Setup polling timer */ > + setup_timer(&mbox->rxpoll_timer, altera_mbox_poll_rx, > + (unsigned long)chan); > + mod_timer(&mbox->rxpoll_timer, > + jiffies + msecs_to_jiffies(MBOX_POLLING_MS)); > + } > + > + return 0; > +} > + Just these minor comments. -Jassi -- 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/