Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <8d29eba045ef18c5489e122b3668afc20431f15d.1587894279.git.baolin.wang7@gmail.com>
 <6f8ac1af5a43d6fd487dc21c052624277325e6f1.1587894279.git.baolin.wang7@gmail.com>
 <CABb+yY31zAr9xfS3bWC5k=94L921CyhzA7fot3eJP1WYSvNCpw@mail.gmail.com>
In-Reply-To: <CABb+yY31zAr9xfS3bWC5k=94L921CyhzA7fot3eJP1WYSvNCpw@mail.gmail.com>
From:   Baolin Wang <baolin.wang7@gmail.com>
Date:   Tue, 28 Apr 2020 11:06:16 +0800
Message-ID: <CADBw62pk7AtahXXMqrHo4Fh64TQC_2k4VWZ2U47LGxZv41o4Fg@mail.gmail.com>
Subject: Re: [RESEND PATCH v3 2/2] mailbox: sprd: Add Spreadtrum mailbox driver
To:     Jassi Brar <jassisinghbrar@gmail.com>
Cc:     Rob Herring <robh+dt@kernel.org>, Orson Zhai <orsonzhai@gmail.com>,
        Chunyan Zhang <zhang.lyra@gmail.com>,
        Devicetree List <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Jassi,

On Tue, Apr 28, 2020 at 4:19 AM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Mon, Apr 27, 2020 at 2:20 AM Baolin Wang <baolin.wang7@gmail.com> wrote:
> >
> > From: Baolin Wang <baolin.wang@unisoc.com>
> >
> > The Spreadtrum mailbox controller supports 8 channels to communicate
> > with MCUs, and it contains 2 different parts: inbox and outbox, which
> > are used to send and receive messages by IRQ mode.
> >
> > Signed-off-by: Baolin Wang <baolin.wang@unisoc.com>
> > Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
> > ---
> > Changes from v2:
> >  - None.
> >
> > Changes from v1:
> >  - None
> > ---
> >  drivers/mailbox/Kconfig        |   8 +
> >  drivers/mailbox/Makefile       |   2 +
> >  drivers/mailbox/sprd-mailbox.c | 350 +++++++++++++++++++++++++++++++++
> >  3 files changed, 360 insertions(+)
> >  create mode 100644 drivers/mailbox/sprd-mailbox.c
> >
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > index 5a577a6734cf..e03f3fb5caed 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -236,4 +236,12 @@ config SUN6I_MSGBOX
> >           various Allwinner SoCs. This mailbox is used for communication
> >           between the application CPUs and the power management coprocessor.
> >
> > +config SPRD_MBOX
> > +       tristate "Spreadtrum Mailbox"
> > +       depends on ARCH_SPRD || COMPILE_TEST
> > +       help
> > +         Mailbox driver implementation for the Spreadtrum platform. It is used
> > +         to send message between application processors and MCU. Say Y here if
> > +         you want to build the Spreatrum mailbox controller driver.
> > +
> >  endif
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > index 2e4364ef5c47..9caf4ede6ce0 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -50,3 +50,5 @@ obj-$(CONFIG_MTK_CMDQ_MBOX)   += mtk-cmdq-mailbox.o
> >  obj-$(CONFIG_ZYNQMP_IPI_MBOX)  += zynqmp-ipi-mailbox.o
> >
> >  obj-$(CONFIG_SUN6I_MSGBOX)     += sun6i-msgbox.o
> > +
> > +obj-$(CONFIG_SPRD_MBOX)                += sprd-mailbox.o
> > diff --git a/drivers/mailbox/sprd-mailbox.c b/drivers/mailbox/sprd-mailbox.c
> > new file mode 100644
> > index 000000000000..58e5388f190b
> > --- /dev/null
> > +++ b/drivers/mailbox/sprd-mailbox.c
> > @@ -0,0 +1,350 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Spreadtrum mailbox driver
> > + *
> > + * Copyright (c) 2020 Spreadtrum Communications Inc.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/clk.h>
> > +
> > +#define SPRD_MBOX_ID           0x0
> > +#define SPRD_MBOX_MSG_LOW      0x4
> > +#define SPRD_MBOX_MSG_HIGH     0x8
> > +#define SPRD_MBOX_TRIGGER      0xc
> > +#define SPRD_MBOX_FIFO_RST     0x10
> > +#define SPRD_MBOX_FIFO_STS     0x14
> > +#define SPRD_MBOX_IRQ_STS      0x18
> > +#define SPRD_MBOX_IRQ_MSK      0x1c
> > +#define SPRD_MBOX_LOCK         0x20
> > +#define SPRD_MBOX_FIFO_DEPTH   0x24
> > +
> > +/* Bit and mask definiation for inbox's SPRD_MBOX_FIFO_STS register */
> > +#define SPRD_INBOX_FIFO_DELIVER_MASK           GENMASK(23, 16)
> > +#define SPRD_INBOX_FIFO_OVERLOW_MASK           GENMASK(15, 8)
> > +#define SPRD_INBOX_FIFO_DELIVER_SHIFT          16
> > +#define SPRD_INBOX_FIFO_BUSY_MASK              GENMASK(7, 0)
> > +
> > +/* Bit and mask definiation for SPRD_MBOX_IRQ_STS register */
> > +#define SPRD_MBOX_IRQ_CLR                      BIT(0)
> > +
> > +/* Bit and mask definiation for outbox's SPRD_MBOX_FIFO_STS register */
> > +#define SPRD_OUTBOX_FIFO_FULL                  BIT(0)
> > +#define SPRD_OUTBOX_FIFO_WR_SHIFT              16
> > +#define SPRD_OUTBOX_FIFO_RD_SHIFT              24
> > +#define SPRD_OUTBOX_FIFO_POS_MASK              GENMASK(7, 0)
> > +
> > +/* Bit and mask definiation for inbox's SPRD_MBOX_IRQ_MSK register */
> > +#define SPRD_INBOX_FIFO_BLOCK_IRQ              BIT(0)
> > +#define SPRD_INBOX_FIFO_OVERFLOW_IRQ           BIT(1)
> > +#define SPRD_INBOX_FIFO_DELIVER_IRQ            BIT(2)
> > +#define SPRD_INBOX_FIFO_IRQ_MASK               GENMASK(2, 0)
> > +
> > +/* Bit and mask definiation for outbox's SPRD_MBOX_IRQ_MSK register */
> > +#define SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ         BIT(0)
> > +#define SPRD_OUTBOX_FIFO_IRQ_MASK              GENMASK(4, 0)
> > +
> > +#define SPRD_MBOX_CHAN_MAX                     8
> > +
> > +struct sprd_mbox_chan {
> > +       u8                      id;
> > +       struct mbox_chan        *chan;
> > +};
> If 'id' is all you need, please assign that to mbox_chan.con_priv and
> discard the sprd_mbox_chan. That will be much simpler.

Yes, will do in next version.

> > +
> > +struct sprd_mbox_priv {
> > +       struct mbox_controller  mbox;
> > +       struct device           *dev;
> > +       void __iomem            *inbox_base;
> > +       void __iomem            *outbox_base;
> > +       struct clk              *clk;
> > +       u32                     outbox_fifo_depth;
> > +
> > +       struct sprd_mbox_chan   mchan[SPRD_MBOX_CHAN_MAX];
> > +       struct mbox_chan        chan[SPRD_MBOX_CHAN_MAX];
> > +};
> > +
> .........
>
> > +
> > +static irqreturn_t sprd_mbox_inbox_isr(int irq, void *data)
> > +{
> > +       struct sprd_mbox_priv *priv = data;
> > +       struct sprd_mbox_chan *mchan;
> > +       u32 fifo_sts, send_sts, id;
> > +
> > +       fifo_sts = readl(priv->inbox_base + SPRD_MBOX_FIFO_STS);
> > +
> > +       /* Get the inbox data delivery status */
> > +       send_sts = (fifo_sts & SPRD_INBOX_FIFO_DELIVER_MASK) >>
> > +               SPRD_INBOX_FIFO_DELIVER_SHIFT;
> > +       if (!send_sts) {
> > +               dev_warn_ratelimited(priv->dev, "spurious inbox interrupt\n");
> > +               return IRQ_NONE;
> > +       }
> > +
> > +       while (send_sts) {
> > +               id = __ffs(send_sts);
> > +               send_sts &= (send_sts - 1);
> > +
> > +               mchan = &priv->mchan[id];
> > +               mbox_chan_txdone(mchan->chan, 0);
> > +       }
> > +
> > +       /* Clear FIFO delivery and overflow status */
> > +       writel(fifo_sts &
> > +              (SPRD_INBOX_FIFO_DELIVER_MASK | SPRD_INBOX_FIFO_OVERLOW_MASK),
> > +              priv->inbox_base + SPRD_MBOX_FIFO_RST);
> > +
> > +       /* Clear irq status */
> > +       writel(SPRD_MBOX_IRQ_CLR, priv->inbox_base + SPRD_MBOX_IRQ_STS);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static int sprd_mbox_send_data(struct mbox_chan *chan, void *msg)
> > +{
> > +       struct sprd_mbox_priv *priv = to_sprd_mbox_priv(chan->mbox);
> > +       struct sprd_mbox_chan *mchan = chan->con_priv;
> > +       u32 *data = msg, busy;
> > +
> > +       /*
> > +        * Check if current channel is busy or not, and we can not send data
> > +        * if current channel is busy.
> > +        */
> > +       busy = readl(priv->inbox_base + SPRD_MBOX_FIFO_STS) &
> > +               SPRD_INBOX_FIFO_BUSY_MASK;
> > +       if (busy & BIT(mchan->id)) {
> > +               dev_err(priv->dev, "Channel %d is busy\n", mchan->id);
> > +               return -EBUSY;
> > +       }
> Maybe check this before  mbox_chan_txdone(mchan->chan, 0) and avoid
> failing in send_data.

This is used to check if the target core have fetched the message from
the FIFO or not, if not, we can not send any more message, otherwise
the message will be overflowed.

Thanks for your comments.

-- 
Baolin Wang