From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752080AbdLEFNz (ORCPT ); Tue, 5 Dec 2017 00:13:55 -0500 Received: from mail-pf0-f194.google.com ([209.85.192.194]:42914 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750852AbdLEFNx (ORCPT ); Tue, 5 Dec 2017 00:13:53 -0500 X-Google-Smtp-Source: AGs4zMZeMf9X7Yt7jfPBDaomBsECH8D5s8+ejaGxZp4lvot1orS9MjpMJy7qoyPjtB5oKydjqJvJ6w== Date: Tue, 5 Dec 2017 13:13:40 +0800 From: Leo Yan To: Jassi Brar Cc: Xu YiPing , Rob Herring , Mark Rutland , Wei Xu , Catalin Marinas , Will Deacon , Devicetree List , Linux Kernel Mailing List , "linux-arm-kernel@lists.infradead.org" , Guodong Xu , Haojian Zhuang , suzhuangluan@hisilicon.com, xuezhiliang@hisilicon.com, kevin.wangtao@hisilicon.com, Zhong Kaihua Subject: Re: [PATCH v3 2/3] mailbox: Add support for Hi3660 mailbox Message-ID: <20171205051340.GD20321@leoy-linaro> References: <1510910672-1409-1-git-send-email-xuyiping@hisilicon.com> <1510910672-1409-3-git-send-email-xuyiping@hisilicon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Dec 05, 2017 at 08:58:05AM +0530, Jassi Brar wrote: > On Fri, Nov 17, 2017 at 2:54 PM, Xu YiPing wrote: > > From: Kaihua Zhong > > > > Hi3660 mailbox controller is used to send message within multiple > > processors, MCU, HIFI, etc. It supports 32 mailbox channels and every > > channel can only be used for single transferring direction. Once the > > channel is enabled, it needs to specify the destination interrupt and > > acknowledge interrupt, these two interrupt vectors are used to create > > the connection between the mailbox and interrupt controllers. > > > > The data transferring supports two modes, one is named as "automatic > > acknowledge" mode so after send message the kernel doesn't need to wait > > for acknowledge from remote and directly return; there have another mode > > is to rely on handling interrupt for acknowledge. > > > > This commit is for initial version driver, which only supports > > "automatic acknowledge" mode to support CPU clock, which is the only > > one consumer to use mailbox and has been verified. Later may enhance > > this driver for interrupt mode (e.g. for supporting HIFI). > > > > Signed-off-by: Leo Yan > > Signed-off-by: Ruyi Wang > > Signed-off-by: Kaihua Zhong > > --- > > drivers/mailbox/Kconfig | 8 + > > drivers/mailbox/Makefile | 2 + > > drivers/mailbox/hi3660-mailbox.c | 322 +++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 332 insertions(+) > > create mode 100644 drivers/mailbox/hi3660-mailbox.c > > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig > > index c5731e5..4b5d6e9 100644 > > --- a/drivers/mailbox/Kconfig > > +++ b/drivers/mailbox/Kconfig > > @@ -108,6 +108,14 @@ config TI_MESSAGE_MANAGER > > multiple processors within the SoC. Select this driver if your > > platform has support for the hardware block. > > > > +config HI3660_MBOX > > + tristate "Hi3660 Mailbox" > > + depends on ARCH_HISI && OF > > + help > > + An implementation of the hi3660 mailbox. It is used to send message > > + between application processors and other processors/MCU/DSP. Select > > + Y here if you want to use Hi3660 mailbox controller. > > + > > config HI6220_MBOX > > tristate "Hi6220 Mailbox" > > depends on ARCH_HISI > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile > > index d54e412..7d1bd51 100644 > > --- a/drivers/mailbox/Makefile > > +++ b/drivers/mailbox/Makefile > > @@ -26,6 +26,8 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o > > > > obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o > > > > +obj-$(CONFIG_HI3660_MBOX) += hi3660-mailbox.o > > + > > obj-$(CONFIG_HI6220_MBOX) += hi6220-mailbox.o > > > > obj-$(CONFIG_BCM_PDC_MBOX) += bcm-pdc-mailbox.o > > diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c > > new file mode 100644 > > index 0000000..ba80834 > > --- /dev/null > > +++ b/drivers/mailbox/hi3660-mailbox.c > > @@ -0,0 +1,322 @@ > > +/* > > + * Hisilicon's Hi3660 mailbox controller driver > > + * > > + * Copyright (c) 2017 Hisilicon Limited. > > + * Copyright (c) 2017 Linaro Limited. > > + * > > + * Author: Leo Yan > > + * > > + * This program is free software: you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation, version 2 of the License. > > + * > > + * This program is distributed in the hope that 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. > > + * > > > We now use SPDX licence Thanks for quick response, Jassi. Will change to SPDX licence fashion. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "mailbox.h" > > + > > +#define MBOX_CHAN_MAX 32 > > + > > +#define MBOX_RX (0x0) > > +#define MBOX_TX (0x1) > > + > > +#define MBOX_BASE(mbox, ch) ((mbox)->base + ((ch) * 0x40)) > > +#define MBOX_SRC_REG (0x00) > > +#define MBOX_DST_REG (0x04) > > +#define MBOX_DCLR_REG (0x08) > > +#define MBOX_DSTAT_REG (0x0c) > > +#define MBOX_MODE_REG (0x10) > > +#define MBOX_IMASK_REG (0x14) > > +#define MBOX_ICLR_REG (0x18) > > +#define MBOX_SEND_REG (0x1c) > > +#define MBOX_DATA_REG (0x20) > > + > > +#define MBOX_IPC_LOCK_REG (0xa00) > > +#define MBOX_IPC_UNLOCK (0x1acce551) > > + > > +#define MBOX_AUTOMATIC_ACK (1) > > + > Please, no brackets around numbers. Sure, will remove brackets. > > +#define MBOX_STATE_IDLE BIT(4) > > +#define MBOX_STATE_ACK BIT(7) > > + > > +#define MBOX_MSG_LEN 8 > > + > > +/** > > + * Hi3660 mailbox channel device data > > + * > > + * A channel can be used for TX or RX, it can trigger remote > > + * processor interrupt to notify remote processor and can receive > > + * interrupt if has incoming message. > > + * > > + * @dst_irq: Interrupt vector for remote processor > > + * @ack_irq: Interrupt vector for local processor > > + */ > > +struct hi3660_mbox_dev { > > > Better than dev, maybe hi3660_chan_info ? > > > + unsigned int dst_irq; > > + unsigned int ack_irq; > > +}; > > + > > +/** > > + * Hi3660 mailbox controller data > > + * > > + * Mailbox controller includes 32 channels and can allocate > > + * channel for message transferring. > > + * > > + * @dev: Device to which it is attached > > + * @base: Base address of the register mapping region > > + * @chan: Representation of channels in mailbox controller > > + * @mdev: Representation of channel device data > > + * @controller: Representation of a communication channel controller > > + */ > > +struct hi3660_mbox { > > + struct device *dev; > > + void __iomem *base; > > + struct mbox_chan chan[MBOX_CHAN_MAX]; > > + struct hi3660_mbox_dev mdev[MBOX_CHAN_MAX]; > > > Maybe mchan, instead of mdev. Will refactor structure name. > > + struct mbox_controller controller; > > +}; > > + > > +static inline struct hi3660_mbox *to_hi3660_mbox(struct mbox_controller *mbox) > > +{ > > + return container_of(mbox, struct hi3660_mbox, controller); > > +} > > + > > +static int hi3660_mbox_check_state(struct mbox_chan *chan) > > +{ > > + unsigned long ch = (unsigned long)chan->con_priv; > > + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox); > > + struct hi3660_mbox_dev *mdev = &mbox->mdev[ch]; > > + void __iomem *base = MBOX_BASE(mbox, ch); > > + unsigned long val; > > + unsigned int state, ret; > > + > > + /* Mailbox is idle so directly bail out */ > > + state = readl_relaxed(base + MBOX_MODE_REG); > > + if (state & MBOX_STATE_IDLE) > > + return 0; > > + > > + /* Wait for acknowledge from remote */ > > + ret = readx_poll_timeout_atomic(readl_relaxed, base + MBOX_MODE_REG, > > + val, (val & MBOX_STATE_ACK), 1000, 300000); > > + if (ret) { > > + dev_err(mbox->dev, "%s: timeout for receiving ack\n", __func__); > > + return ret; > > + } > > + > > + /* Ensure channel is released */ > > + writel_relaxed(0xffffffff, base + MBOX_IMASK_REG); > > + writel_relaxed(BIT(mdev->ack_irq), base + MBOX_SRC_REG); > > + > > + return 0; > > +} > > + > > +static int hi3660_mbox_unlock(struct mbox_chan *chan) > > +{ > > + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox); > > + unsigned int val, retry = 3; > > + > > + do { > > + writel_relaxed(MBOX_IPC_UNLOCK, mbox->base + MBOX_IPC_LOCK_REG); > > + > > + val = readl_relaxed(mbox->base + MBOX_IPC_LOCK_REG); > > + if (!val) > > + break; > > + > > + udelay(10); > > + } while (retry--); > > + > > + return (!val) ? 0 : -ETIMEDOUT; > > +} > > + > > +static int hi3660_mbox_acquire_channel(struct mbox_chan *chan) > > +{ > > + unsigned long ch = (unsigned long)chan->con_priv; > > + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox); > > + struct hi3660_mbox_dev *mdev = &mbox->mdev[ch]; > > + void __iomem *base = MBOX_BASE(mbox, ch); > > + unsigned int val, retry; > > + > > + for (retry = 10; retry; retry--) { > > + /* Check if channel has been acquired */ > > + if (readl_relaxed(base + MBOX_MODE_REG) & MBOX_STATE_IDLE) { > > + writel_relaxed(BIT(mdev->ack_irq), base + MBOX_SRC_REG); > > + val = readl_relaxed(base + MBOX_SRC_REG); > > + if (val & BIT(mdev->ack_irq)) > > + break; > > + } > > + } > > + > > + return retry ? 0 : -ETIMEDOUT; > > +} > > + > > +static int hi3660_mbox_send(struct mbox_chan *chan, u32 *msg) > > +{ > > + unsigned long ch = (unsigned long)chan->con_priv; > > + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox); > > + struct hi3660_mbox_dev *mdev = &mbox->mdev[ch]; > > + void __iomem *base = MBOX_BASE(mbox, ch); > > + unsigned int i; > > + > > + /* Clear mask for destination interrupt */ > > + writel_relaxed(~BIT(mdev->dst_irq), base + MBOX_IMASK_REG); > > + > > + /* Config destination for interrupt vector */ > > + writel_relaxed(BIT(mdev->dst_irq), base + MBOX_DST_REG); > > + > > + /* Automatic acknowledge mode */ > > + writel_relaxed(MBOX_AUTOMATIC_ACK, base + MBOX_MODE_REG); > > + > > + /* Fill message data */ > > + for (i = 0; i < MBOX_MSG_LEN; i++) > > + writel_relaxed(msg[i], base + MBOX_DATA_REG + i * 4); > > + > > + /* Trigger data transferring */ > > + writel_relaxed(BIT(mdev->ack_irq), base + MBOX_SEND_REG); > > + return 0; > > +} > > + > > +static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg) > > +{ > > + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox); > > + int err; > > + > > + err = hi3660_mbox_check_state(chan); > > + if (err) { > > + dev_err(mbox->dev, "checking state failed\n"); > > + return err; > > + } > > + > > + err = hi3660_mbox_unlock(chan); > > + if (err) { > > + dev_err(mbox->dev, "unlocking mailbox failed\n"); > > + return err; > > + } > > + > > + err = hi3660_mbox_acquire_channel(chan); > > + if (err) { > > + dev_err(mbox->dev, "acquiring channel failed\n"); > > + return err; > > + } > > + > Please remember .send_data() is called with irqs disabled spinlock > All these polling functions hurt. Please move the above three checks > in last_tx_done() I have tried to move these three checks in last_tx_done(), since these three functions tightly couple with mailbox state machine, so I failed to move them to last_tx_done(). Anyway, I will try to move them to last_tx_done() for next version patch. Thanks, Leo Yan