[RFC PATCH 0/3] mailbox: hisilicon: add mailbox driver

[RFC PATCH 0/3] mailbox: hisilicon: add mailbox driver

[Leo Yan]

This patch series is to implement Hisilicon's mailbox driver and enable
the mailbox controller on Hi6220.

The Hisilicon mailbox supports up to 32 channels. Each channel is
unidirectional with a maximum message size of 8 words. I/O is performed
using register access (there is no DMA) and the cell raises an interrupt
when messages are received.

For easily extending for Hisilicon series SoCs (SoCs may have difference
for register's definition with each other), so firstly implement common
mailbox driver; this common mailbox driver provides three mainly
functionality:

 - help register channels into framework;
 - hook low level callback functions for register's operations;
 - Enhance rx channel's message queue, which is based on the code in
   drivers/mailbox/omap-mailbox.c.

Base on this common driver, also has enabled Hi6220 mailbox controller.


Leo Yan (3):
  dt-bindings: mailbox: Document Hisilicon mailbox driver
  mailbox: Hisilicon: add mailbox driver
  arm64: dts: add Hi6220 mailbox node

 .../devicetree/bindings/mailbox/hisi-mailbox.txt   |  57 ++++
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     |  20 +-
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi          |   8 +
 drivers/mailbox/Kconfig                            |   2 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/hisilicon/Kconfig                  |  13 +
 drivers/mailbox/hisilicon/Makefile                 |   2 +
 drivers/mailbox/hisilicon/common.c                 | 282 +++++++++++++++++++
 drivers/mailbox/hisilicon/common.h                 | 114 ++++++++
 drivers/mailbox/hisilicon/hi6220-mailbox.c         | 305 +++++++++++++++++++++
 10 files changed, 802 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/hisi-mailbox.txt
 create mode 100644 drivers/mailbox/hisilicon/Kconfig
 create mode 100644 drivers/mailbox/hisilicon/Makefile
 create mode 100644 drivers/mailbox/hisilicon/common.c
 create mode 100644 drivers/mailbox/hisilicon/common.h
 create mode 100644 drivers/mailbox/hisilicon/hi6220-mailbox.c

-- 
1.9.1

[RFC PATCH 1/3] dt-bindings: mailbox: Document Hisilicon mailbox driver

[Leo Yan]

Document the new compatible for Hisilicon mailbox driver.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../devicetree/bindings/mailbox/hisi-mailbox.txt   | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/hisi-mailbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/hisi-mailbox.txt b/Documentation/devicetree/bindings/mailbox/hisi-mailbox.txt
new file mode 100644
index 0000000..898823d
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/hisi-mailbox.txt
@@ -0,0 +1,57 @@
+Hisilicon Mailbox Driver
+========================
+
+The Hisilicon mailbox supports up to 32 channels. Each channel
+is unidirectional with a maximum message size of 8 words. I/O is
+performed using register access (there is no DMA) and the cell
+raises an interrupt when messages are received.
+
+Mailbox Device Node:
+====================
+
+Required properties:
+--------------------
+- compatible:		Shall be "hisilicon,hi6220-mbox"
+- reg:			Contains the mailbox register address range (base
+			address and length); the first item is for IPC
+			registers, the second item is shared buffer for
+			slots.
+- #mbox-cells		Common mailbox binding property to identify the number
+			of cells required for the mailbox specifier. Should be 1.
+- interrupts:		Contains the interrupt information for the mailbox
+			device. The format is dependent on which interrupt
+			controller the SoCs use.
+
+Example:
+--------
+
+	mailbox: mailbox@F7510000 {
+		#mbox-cells = <1>;
+		compatible = "hisilicon,hi6220-mbox";
+		reg = <0x0 0xF7510000 0x0 0x1000>, /* IPC_S */
+		      <0x0 0x06DFF800 0x0 0x0800>; /* Mailbox */
+		interrupt-parent = <&gic>;
+		interrupts = <0 94 4>;
+	};
+
+
+Mailbox client
+===============
+
+"mboxes" and the optional "mbox-names" (please see
+Documentation/devicetree/bindings/mailbox/mailbox.txt for details). Each value
+of the mboxes property should contain a phandle to the mailbox controller
+device node and second argument is the channel index. It must be 0 (hardware
+support only one channel). The equivalent "mbox-names" property value can be
+used to give a name to the communication channel to be used by the client user.
+
+Example:
+--------
+
+	stub_clock: stub_clock {
+		compatible = "hisilicon,hi6220-stub-clk";
+		hisilicon,hi6220-clk-sram = <&sram>;
+		#clock-cells = <1>;
+		mbox-names = "mbox-tx";
+		mboxes = <&mailbox 1>;
+	};
-- 
1.9.1

[RFC PATCH 2/3] mailbox: Hisilicon: add mailbox driver

[Leo Yan]

Add Hisilicon mailbox's common driver, it registers mailbox channels
into framework; it also invokes low level callback functions for
register's related operations. Enhance rx channel's message queue,
which is based on the code in drivers/mailbox/omap-mailbox.c.

Enable Hi6220 mailbox driver as the first platform to use this
framework. Hi6220's mailbox communicates with MCU; for sending data,
it can support two methods for low level implementation: one is to
use interrupt as acknowledge, another is automatic mode which without
any acknowledge. These two methods have been supported in the driver;
for receiving data, it will depend on the interrupt to notify the
channel has incoming message.

Now mailbox driver is used to send message to MCU to control dynamic
voltage and frequency scaling for CPU, GPU and DDR.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/mailbox/Kconfig                    |   2 +
 drivers/mailbox/Makefile                   |   2 +
 drivers/mailbox/hisilicon/Kconfig          |  13 ++
 drivers/mailbox/hisilicon/Makefile         |   2 +
 drivers/mailbox/hisilicon/common.c         | 282 ++++++++++++++++++++++++++
 drivers/mailbox/hisilicon/common.h         | 114 +++++++++++
 drivers/mailbox/hisilicon/hi6220-mailbox.c | 305 +++++++++++++++++++++++++++++
 7 files changed, 720 insertions(+)
 create mode 100644 drivers/mailbox/hisilicon/Kconfig
 create mode 100644 drivers/mailbox/hisilicon/Makefile
 create mode 100644 drivers/mailbox/hisilicon/common.c
 create mode 100644 drivers/mailbox/hisilicon/common.h
 create mode 100644 drivers/mailbox/hisilicon/hi6220-mailbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index e269f08..a426901 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -70,4 +70,6 @@ config BCM2835_MBOX
 	  the services of the Videocore. Say Y here if you want to use the
 	  BCM2835 Mailbox.
 
+source "drivers/mailbox/hisilicon/Kconfig"
+
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 8e6d822..1e82dfb 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -13,3 +13,5 @@ obj-$(CONFIG_PCC)		+= pcc.o
 obj-$(CONFIG_ALTERA_MBOX)	+= mailbox-altera.o
 
 obj-$(CONFIG_BCM2835_MBOX)	+= bcm2835-mailbox.o
+
+obj-$(CONFIG_HISI_MBOX)		+= hisilicon/
diff --git a/drivers/mailbox/hisilicon/Kconfig b/drivers/mailbox/hisilicon/Kconfig
new file mode 100644
index 0000000..87548a9
--- /dev/null
+++ b/drivers/mailbox/hisilicon/Kconfig
@@ -0,0 +1,13 @@
+config HISI_MBOX
+	bool "Hisilicon's Mailbox"
+	depends on ARCH_HISI || OF
+	help
+	  Support for mailbox drivers on Hisilicon series of SoCs.
+
+config HI6220_MBOX
+	tristate "Hi6220 Mailbox Controller"
+	depends on HISI_MBOX
+	help
+	  An implementation of the hi6220 mailbox. It is used to send message
+	  between application processors and MCU. Say Y here if you want to build
+	  the Hi6220 mailbox controller driver.
diff --git a/drivers/mailbox/hisilicon/Makefile b/drivers/mailbox/hisilicon/Makefile
new file mode 100644
index 0000000..59135b0
--- /dev/null
+++ b/drivers/mailbox/hisilicon/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_HISI_MBOX) += common.o
+obj-$(CONFIG_HI6220_MBOX) += hi6220-mailbox.o
diff --git a/drivers/mailbox/hisilicon/common.c b/drivers/mailbox/hisilicon/common.c
new file mode 100644
index 0000000..c3c8e49
--- /dev/null
+++ b/drivers/mailbox/hisilicon/common.c
@@ -0,0 +1,282 @@
+/*
+ * Hisilicon mailbox common driver
+ *
+ * This is skeleton driver for Hisilicon's mailbox, it registers mailbox
+ * channels into framework; it also need invoke low level's callback
+ * functions for low level operations. RX channel's message queue is
+ * based on the code written in drivers/mailbox/omap-mailbox.c.
+
+ * Copyright (c) 2015 Hisilicon Limited.
+ * Copyright (c) 2015 Linaro Limited.
+ *
+ * Author: Leo Yan <leo.yan@linaro.org>
+ *
+ * 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.
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/slab.h>
+
+#include "common.h"
+
+#define MBOX_MSG_LEN		(32)
+#define MBOX_MSG_NUM		(16)
+#define MBOX_MSG_FIFO_SIZE	(MBOX_MSG_LEN * MBOX_MSG_NUM)
+
+static bool hisi_mbox_last_tx_done(struct mbox_chan *chan)
+{
+	struct hisi_mbox_chan *mchan = chan->con_priv;
+	struct hisi_mbox_hw *mbox_hw = mchan->mbox_hw;
+
+	/* Only set idle state for polling mode */
+	BUG_ON(mbox_hw->tx_irq_mode);
+
+	return mbox_hw->ops->tx_is_done(mchan);
+}
+
+static int hisi_mbox_send_data(struct mbox_chan *chan, void *msg)
+{
+	struct hisi_mbox_chan *mchan = chan->con_priv;
+	struct hisi_mbox_hw *mbox_hw = mchan->mbox_hw;
+
+	return mbox_hw->ops->tx(mchan, msg, MBOX_MSG_LEN);
+}
+
+static void hisi_mbox_rx_work(struct work_struct *work)
+{
+	struct hisi_mbox_queue *mq =
+			container_of(work, struct hisi_mbox_queue, work);
+	struct mbox_chan *chan = mq->chan;
+	struct hisi_mbox_chan *mchan = chan->con_priv;
+	struct hisi_mbox_hw *mbox_hw = mchan->mbox_hw;
+
+	char msg[MBOX_MSG_LEN];
+	int len;
+
+	while (kfifo_len(&mq->fifo) >= sizeof(msg)) {
+		len = kfifo_out(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
+		WARN_ON(len != sizeof(msg));
+
+		mbox_chan_received_data(chan, (void *)msg);
+		spin_lock_irq(&mq->lock);
+		if (mq->full) {
+			mq->full = false;
+			mbox_hw->ops->enable_irq(mchan);
+		}
+		spin_unlock_irq(&mq->lock);
+	}
+}
+
+static void hisi_mbox_tx_interrupt(struct mbox_chan *chan)
+{
+	struct hisi_mbox_chan *mchan = chan->con_priv;
+	struct hisi_mbox_hw *mbox_hw = mchan->mbox_hw;
+
+	mbox_hw->ops->clear_irq(mchan);
+	mbox_hw->ops->ack(mchan);
+
+	mbox_chan_txdone(chan, 0);
+}
+
+static void hisi_mbox_rx_interrupt(struct mbox_chan *chan)
+{
+	struct hisi_mbox_chan *mchan = chan->con_priv;
+	struct hisi_mbox_queue *mq = mchan->mq;
+	struct hisi_mbox_hw *mbox_hw = mchan->mbox_hw;
+	char msg[MBOX_MSG_LEN];
+	int len;
+
+	if (unlikely(kfifo_avail(&mq->fifo) < sizeof(msg))) {
+		mbox_hw->ops->disable_irq(mchan);
+		mq->full = true;
+		goto nomem;
+	}
+
+	mbox_hw->ops->rx(mchan, msg, sizeof(msg));
+
+	len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
+	WARN_ON(len != sizeof(msg));
+
+	mbox_hw->ops->ack(mchan);
+nomem:
+	schedule_work(&mq->work);
+}
+
+static irqreturn_t hisi_mbox_interrupt(int irq, void *p)
+{
+	struct hisi_mbox *mbox = p;
+	struct hisi_mbox_hw *mbox_hw = mbox->hw;
+	struct hisi_mbox_chan *mchan;
+	struct mbox_chan *chan;
+	unsigned int state;
+	unsigned int intr_bit;
+
+	state = mbox_hw->ops->get_pending_irq(mbox_hw);
+	if (!state) {
+		dev_warn(mbox_hw->dev, "%s: spurious interrupt\n",
+			 __func__);
+		return IRQ_HANDLED;
+	}
+
+	while (state) {
+		intr_bit = __ffs(state);
+		state &= (state - 1);
+
+		chan = mbox->irq_map_chan[intr_bit];
+		if (!chan) {
+			dev_warn(mbox_hw->dev, "%s: unexpected irq vector %d\n",
+				 __func__, intr_bit);
+			continue;
+		}
+
+		mchan = chan->con_priv;
+		if (mchan->dir == MBOX_TX)
+			hisi_mbox_tx_interrupt(chan);
+		else
+			hisi_mbox_rx_interrupt(chan);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static struct hisi_mbox_queue *hisi_mbox_queue_alloc(
+		struct mbox_chan *chan,
+		void (*work)(struct work_struct *))
+{
+	struct hisi_mbox_queue *mq;
+
+	if (!work)
+		return NULL;
+
+	mq = kzalloc(sizeof(struct hisi_mbox_queue), GFP_KERNEL);
+	if (!mq)
+		return NULL;
+
+	spin_lock_init(&mq->lock);
+
+	if (kfifo_alloc(&mq->fifo, MBOX_MSG_FIFO_SIZE, GFP_KERNEL))
+		goto error;
+
+	mq->chan = chan;
+	INIT_WORK(&mq->work, work);
+	return mq;
+
+error:
+	kfree(mq);
+	return NULL;
+}
+
+static void hisi_mbox_queue_free(struct hisi_mbox_queue *mq)
+{
+	kfifo_free(&mq->fifo);
+	kfree(mq);
+}
+
+static int hisi_mbox_startup(struct mbox_chan *chan)
+{
+	struct hisi_mbox_chan *mchan = chan->con_priv;
+	struct hisi_mbox_hw *mbox_hw = mchan->mbox_hw;
+	struct hisi_mbox *mbox = mchan->mbox_hw->parent;
+
+	struct hisi_mbox_queue *mq;
+	unsigned int irq = mchan->local_irq;
+
+	mq = hisi_mbox_queue_alloc(chan, hisi_mbox_rx_work);
+	if (!mq)
+		return -ENOMEM;
+	mchan->mq = mq;
+
+	mbox->irq_map_chan[irq] = (void *)chan;
+	return mbox_hw->ops->startup(mchan);
+}
+
+static void hisi_mbox_shutdown(struct mbox_chan *chan)
+{
+	struct hisi_mbox_chan *mchan = chan->con_priv;
+	struct hisi_mbox_hw *mbox_hw = mchan->mbox_hw;
+	struct hisi_mbox *mbox = mbox_hw->parent;
+	unsigned int irq = mchan->local_irq;
+
+	mbox_hw->ops->shutdown(mchan);
+
+	mbox->irq_map_chan[irq] = NULL;
+	flush_work(&mchan->mq->work);
+	hisi_mbox_queue_free(mchan->mq);
+}
+
+static struct mbox_chan_ops hisi_mbox_chan_ops = {
+	.send_data    = hisi_mbox_send_data,
+	.startup      = hisi_mbox_startup,
+	.shutdown     = hisi_mbox_shutdown,
+	.last_tx_done = hisi_mbox_last_tx_done,
+};
+
+int hisi_mbox_register(struct hisi_mbox_hw *mbox_hw)
+{
+	struct device *dev = mbox_hw->dev;
+	struct hisi_mbox *mbox;
+	int i, err;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	mbox->hw = mbox_hw;
+	mbox->chan = devm_kzalloc(dev,
+		mbox_hw->chan_num * sizeof(struct mbox_chan), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	err = devm_request_irq(dev, mbox_hw->irq, hisi_mbox_interrupt, 0,
+			dev_name(dev), mbox);
+	if (err) {
+		dev_err(dev, "Failed to register a mailbox IRQ handler: %d\n",
+			err);
+		return -ENODEV;
+	}
+
+	for (i = 0; i < mbox_hw->chan_num; i++) {
+		mbox->chan[i].con_priv = &mbox_hw->chan[i];
+		mbox->irq_map_chan[i] = NULL;
+	}
+
+	mbox->controller.dev = dev;
+	mbox->controller.chans = &mbox->chan[0];
+	mbox->controller.num_chans = mbox_hw->chan_num;
+	mbox->controller.ops = &hisi_mbox_chan_ops;
+
+	if (mbox_hw->tx_irq_mode)
+		mbox->controller.txdone_irq = true;
+	else {
+		mbox->controller.txdone_poll = true;
+		mbox->controller.txpoll_period = 5;
+	}
+
+	err = mbox_controller_register(&mbox->controller);
+	if (err) {
+		dev_err(dev, "Failed to register mailbox %d\n", err);
+		return err;
+	}
+
+	mbox_hw->parent = mbox;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(hisi_mbox_register);
+
+void hisi_mbox_unregister(struct hisi_mbox_hw *mbox_hw)
+{
+	struct hisi_mbox *mbox = mbox_hw->parent;
+
+	mbox_controller_unregister(&mbox->controller);
+}
+EXPORT_SYMBOL_GPL(hisi_mbox_unregister);
diff --git a/drivers/mailbox/hisilicon/common.h b/drivers/mailbox/hisilicon/common.h
new file mode 100644
index 0000000..c2199e2
--- /dev/null
+++ b/drivers/mailbox/hisilicon/common.h
@@ -0,0 +1,114 @@
+/*
+ * Hisilicon mailbox definition
+ *
+ * Copyright (c) 2015 Hisilicon Limited.
+ * Copyright (c) 2015 Linaro Limited.
+ *
+ * Author: Leo Yan <leo.yan@linaro.org>
+ *
+ * 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.
+ *
+ */
+
+#ifndef __HISI_MBOX_H
+#define __HISI_MBOX_H
+
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/kfifo.h>
+#include <linux/mailbox_controller.h>
+#include <linux/spinlock.h>
+
+#define MBOX_MAX_CHANS		32
+#define MBOX_RX			0x0
+#define MBOX_TX			0x1
+
+struct hisi_mbox_queue {
+	spinlock_t lock;
+	struct kfifo fifo;
+	struct work_struct work;
+	struct mbox_chan *chan;
+	bool full;
+};
+
+struct hisi_mbox_chan {
+
+	/*
+	 * Description for channel's hardware info:
+	 * - direction;
+	 * - peer core id for communication;
+	 * - local irq vector or number;
+	 * - remoted irq vector or number for peer core;
+	 */
+	unsigned int dir;
+	unsigned int peer_core;
+	unsigned int remote_irq;
+	unsigned int local_irq;
+
+	/*
+	 * Slot address is cached value derived from index
+	 * within buffer for every channel
+	 */
+	void __iomem *slot;
+
+	/* For rx's fifo operations */
+	struct hisi_mbox_queue *mq;
+
+	struct hisi_mbox_hw *mbox_hw;
+};
+
+struct hisi_mbox_ops {
+	int (*startup)(struct hisi_mbox_chan *chan);
+	void (*shutdown)(struct hisi_mbox_chan *chan);
+
+	int (*rx)(struct hisi_mbox_chan *chan, void *msg, int len);
+	int (*tx)(struct hisi_mbox_chan *chan, void *msg, int len);
+	int (*tx_is_done)(struct hisi_mbox_chan *chan);
+	int (*ack)(struct hisi_mbox_chan *chan);
+
+	u32 (*get_pending_irq)(struct hisi_mbox_hw *mbox_hw);
+
+	void (*clear_irq)(struct hisi_mbox_chan *chan);
+	void (*enable_irq)(struct hisi_mbox_chan *chan);
+	void (*disable_irq)(struct hisi_mbox_chan *chan);
+};
+
+struct hisi_mbox_hw {
+	struct device *dev;
+
+	unsigned int irq;
+
+	/* flag of enabling tx's irq mode */
+	bool tx_irq_mode;
+
+	/* region for ipc event */
+	void __iomem *ipc;
+
+	/* region for share mem */
+	void __iomem *buf;
+
+	unsigned int chan_num;
+	struct hisi_mbox_chan *chan;
+	struct hisi_mbox_ops *ops;
+	struct hisi_mbox *parent;
+};
+
+struct hisi_mbox {
+	struct hisi_mbox_hw *hw;
+
+	void *irq_map_chan[MBOX_MAX_CHANS];
+	struct mbox_chan *chan;
+	struct mbox_controller controller;
+};
+
+extern int hisi_mbox_register(struct hisi_mbox_hw *mbox_hw);
+extern void hisi_mbox_unregister(struct hisi_mbox_hw *mbox_hw);
+
+#endif /* __HISI_MBOX_H */
diff --git a/drivers/mailbox/hisilicon/hi6220-mailbox.c b/drivers/mailbox/hisilicon/hi6220-mailbox.c
new file mode 100644
index 0000000..099e21d
--- /dev/null
+++ b/drivers/mailbox/hisilicon/hi6220-mailbox.c
@@ -0,0 +1,305 @@
+/*
+ * Hisilicon's Hi6220 mailbox low level driver
+ *
+ * Copyright (c) 2015 Hisilicon Limited.
+ * Copyright (c) 2015 Linaro Limited.
+ *
+ * Author: Leo Yan <leo.yan@linaro.org>
+ *
+ * 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.
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "common.h"
+
+#define HI6220_MBOX_CHAN_NUM			2
+#define HI6220_MBOX_CHAN_SLOT_SIZE		(1 << 6)
+
+/* Status & Mode Register */
+#define HI6220_MBOX_MODE_REG			(0x0)
+
+#define HI6220_MBOX_STATUS_MASK			(0xF << 4)
+#define HI6220_MBOX_STATUS_IDLE			(0x1 << 4)
+#define HI6220_MBOX_STATUS_TX			(0x2 << 4)
+#define HI6220_MBOX_STATUS_RX			(0x4 << 4)
+#define HI6220_MBOX_STATUS_ACK			(0x8 << 4)
+#define HI6220_MBOX_ACK_CONFIG_MASK		(0x1 << 0)
+#define HI6220_MBOX_ACK_AUTOMATIC		(0x1 << 0)
+#define HI6220_MBOX_ACK_IRQ			(0x0 << 0)
+
+/* Data Registers */
+#define HI6220_MBOX_DATA_REG(i)			(0x4 + (i << 2))
+
+/* ACPU Interrupt Register */
+#define HI6220_MBOX_ACPU_INT_RAW_REG		(0x400)
+#define HI6220_MBOX_ACPU_INT_MSK_REG		(0x404)
+#define HI6220_MBOX_ACPU_INT_STAT_REG		(0x408)
+#define HI6220_MBOX_ACPU_INT_CLR_REG		(0x40c)
+#define HI6220_MBOX_ACPU_INT_ENA_REG		(0x500)
+#define HI6220_MBOX_ACPU_INT_DIS_REG		(0x504)
+
+/* MCU Interrupt Register */
+#define HI6220_MBOX_MCU_INT_RAW_REG		(0x420)
+
+/* Core Id */
+#define HI6220_CORE_ACPU			(0x0)
+#define HI6220_CORE_MCU				(0x2)
+
+static u32 hi6220_mbox_get_status(struct hisi_mbox_chan *chan)
+{
+	u32 status;
+
+	status = readl(chan->slot + HI6220_MBOX_MODE_REG);
+	return status & HI6220_MBOX_STATUS_MASK;
+}
+
+static void hi6220_mbox_set_status(struct hisi_mbox_chan *chan, u32 val)
+{
+	u32 status;
+
+	status = readl(chan->slot + HI6220_MBOX_MODE_REG);
+	status &= ~HI6220_MBOX_STATUS_MASK;
+	status |= val;
+	writel(status, chan->slot + HI6220_MBOX_MODE_REG);
+}
+
+static void hi6220_mbox_set_mode(struct hisi_mbox_chan *chan, u32 val)
+{
+	u32 mode;
+
+	mode = readl(chan->slot + HI6220_MBOX_MODE_REG);
+	mode &= ~HI6220_MBOX_ACK_CONFIG_MASK;
+	mode |= val;
+	writel(mode, chan->slot + HI6220_MBOX_MODE_REG);
+}
+
+static void hi6220_mbox_clear_irq(struct hisi_mbox_chan *chan)
+{
+	struct hisi_mbox_hw *mbox_hw = chan->mbox_hw;
+	int irq = chan->local_irq;
+
+	writel(1 << irq, mbox_hw->ipc + HI6220_MBOX_ACPU_INT_CLR_REG);
+}
+
+static void hi6220_mbox_enable_irq(struct hisi_mbox_chan *chan)
+{
+	struct hisi_mbox_hw *mbox_hw = chan->mbox_hw;
+	int irq = chan->local_irq;
+
+	writel(1 << irq, mbox_hw->ipc + HI6220_MBOX_ACPU_INT_ENA_REG);
+}
+
+static void hi6220_mbox_disable_irq(struct hisi_mbox_chan *chan)
+{
+	struct hisi_mbox_hw *mbox_hw = chan->mbox_hw;
+	int irq = chan->local_irq;
+
+	writel(1 << irq, mbox_hw->ipc + HI6220_MBOX_ACPU_INT_DIS_REG);
+}
+
+static u32 hi6220_mbox_get_pending_irq(struct hisi_mbox_hw *mbox_hw)
+{
+	return readl(mbox_hw->ipc + HI6220_MBOX_ACPU_INT_STAT_REG);
+}
+
+static int hi6220_mbox_startup(struct hisi_mbox_chan *chan)
+{
+	hi6220_mbox_enable_irq(chan);
+	return 0;
+}
+
+static void hi6220_mbox_shutdown(struct hisi_mbox_chan *chan)
+{
+	hi6220_mbox_disable_irq(chan);
+}
+
+static int hi6220_mbox_rx(struct hisi_mbox_chan *chan, void *msg, int len)
+{
+	int *buf = msg;
+	int i;
+
+	for (i = 0; i < (len >> 2); i++)
+		buf[i] = readl(chan->slot + HI6220_MBOX_DATA_REG(i));
+
+	/* clear IRQ source */
+	hi6220_mbox_clear_irq(chan);
+	hi6220_mbox_set_status(chan, HI6220_MBOX_STATUS_IDLE);
+	return len;
+}
+
+static int hi6220_mbox_tx(struct hisi_mbox_chan *chan, void *msg, int len)
+{
+	struct hisi_mbox_hw *mbox_hw = chan->mbox_hw;
+	int *buf = msg;
+	int irq = chan->remote_irq, i;
+
+	hi6220_mbox_set_status(chan, HI6220_MBOX_STATUS_TX);
+
+	if (mbox_hw->tx_irq_mode)
+		hi6220_mbox_set_mode(chan, HI6220_MBOX_ACK_IRQ);
+	else
+		hi6220_mbox_set_mode(chan, HI6220_MBOX_ACK_AUTOMATIC);
+
+	for (i = 0; i < (len >> 2); i++)
+		writel(buf[i], chan->slot + HI6220_MBOX_DATA_REG(i));
+
+	/* trigger remote request */
+	writel(1 << irq, mbox_hw->ipc + HI6220_MBOX_MCU_INT_RAW_REG);
+	return 0;
+}
+
+static int hi6220_mbox_tx_is_done(struct hisi_mbox_chan *chan)
+{
+	int status;
+
+	status = hi6220_mbox_get_status(chan);
+	return (status == HI6220_MBOX_STATUS_IDLE);
+}
+
+static int hi6220_mbox_ack(struct hisi_mbox_chan *chan)
+{
+	hi6220_mbox_set_status(chan, HI6220_MBOX_STATUS_IDLE);
+	return 0;
+}
+
+static struct hisi_mbox_ops hi6220_mbox_ops = {
+	.startup	 = hi6220_mbox_startup,
+	.shutdown	 = hi6220_mbox_shutdown,
+
+	.clear_irq	 = hi6220_mbox_clear_irq,
+	.enable_irq	 = hi6220_mbox_enable_irq,
+	.disable_irq	 = hi6220_mbox_disable_irq,
+	.get_pending_irq = hi6220_mbox_get_pending_irq,
+
+	.rx		 = hi6220_mbox_rx,
+	.tx		 = hi6220_mbox_tx,
+	.tx_is_done	 = hi6220_mbox_tx_is_done,
+	.ack		 = hi6220_mbox_ack,
+};
+
+static void hi6220_mbox_init_hw(struct hisi_mbox_hw *mbox_hw)
+{
+	struct hisi_mbox_chan init_data[HI6220_MBOX_CHAN_NUM] = {
+		{ MBOX_RX, HI6220_CORE_MCU, 1, 10 },
+		{ MBOX_TX, HI6220_CORE_MCU, 0, 11 },
+	};
+	struct hisi_mbox_chan *chan = mbox_hw->chan;
+	int i;
+
+	for (i = 0; i < HI6220_MBOX_CHAN_NUM; i++) {
+		memcpy(&chan[i], &init_data[i], sizeof(*chan));
+		chan[i].slot = mbox_hw->buf + HI6220_MBOX_CHAN_SLOT_SIZE;
+		chan[i].mbox_hw = mbox_hw;
+	}
+
+	/* mask and clear all interrupt vectors */
+	writel(0x0,  mbox_hw->ipc + HI6220_MBOX_ACPU_INT_MSK_REG);
+	writel(~0x0, mbox_hw->ipc + HI6220_MBOX_ACPU_INT_CLR_REG);
+
+	/* use interrupt for tx's ack */
+	mbox_hw->tx_irq_mode = true;
+
+	/* set low level ops */
+	mbox_hw->ops = &hi6220_mbox_ops;
+}
+
+static const struct of_device_id hi6220_mbox_of_match[] = {
+	{ .compatible = "hisilicon,hi6220-mbox", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, hi6220_mbox_of_match);
+
+static int hi6220_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct hisi_mbox_hw *mbox_hw;
+	struct resource *res;
+	int err;
+
+	mbox_hw = devm_kzalloc(dev, sizeof(*mbox_hw), GFP_KERNEL);
+	if (!mbox_hw)
+		return -ENOMEM;
+
+	mbox_hw->dev = dev;
+	mbox_hw->chan_num = HI6220_MBOX_CHAN_NUM;
+	mbox_hw->chan = devm_kzalloc(dev,
+		mbox_hw->chan_num * sizeof(struct hisi_mbox_chan), GFP_KERNEL);
+	if (!mbox_hw->chan)
+		return -ENOMEM;
+
+	mbox_hw->irq = platform_get_irq(pdev, 0);
+	if (mbox_hw->irq < 0)
+		return mbox_hw->irq;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mbox_hw->ipc = devm_ioremap_resource(dev, res);
+	if (IS_ERR(mbox_hw->ipc)) {
+		dev_err(dev, "ioremap ipc failed\n");
+		return PTR_ERR(mbox_hw->ipc);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	mbox_hw->buf = devm_ioremap_resource(dev, res);
+	if (IS_ERR(mbox_hw->buf)) {
+		dev_err(dev, "ioremap buffer failed\n");
+		return PTR_ERR(mbox_hw->buf);
+	}
+
+	hi6220_mbox_init_hw(mbox_hw);
+
+	err = hisi_mbox_register(mbox_hw);
+	if (err)
+		return err;
+
+	platform_set_drvdata(pdev, mbox_hw);
+	dev_info(dev, "Mailbox enabled\n");
+	return 0;
+}
+
+static int hi6220_mbox_remove(struct platform_device *pdev)
+{
+	struct hisi_mbox_hw *mbox_hw = platform_get_drvdata(pdev);
+
+	hisi_mbox_unregister(mbox_hw);
+	return 0;
+}
+
+static struct platform_driver hi6220_mbox_driver = {
+	.driver = {
+		.name = "hi6220-mbox",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(hi6220_mbox_of_match),
+	},
+	.probe	= hi6220_mbox_probe,
+	.remove	= hi6220_mbox_remove,
+};
+
+static int __init hi6220_mbox_init(void)
+{
+	return platform_driver_register(&hi6220_mbox_driver);
+}
+module_init(hi6220_mbox_init);
+
+static void __exit hi6220_mbox_exit(void)
+{
+	platform_driver_unregister(&hi6220_mbox_driver);
+}
+module_exit(hi6220_mbox_exit);
+
+MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
+MODULE_DESCRIPTION("Hi6220 mailbox driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [RFC PATCH 2/3] mailbox: Hisilicon: add mailbox driver

[Paul Bolle]

(This RFC was part of this mornings catch of my crude mail filter. So,
for what it's worth, what follows are a few random comments for the few
things I'm able to spot.)

On ma, 2015-08-03 at 09:13 +0800, Leo Yan wrote:
> --- /dev/null
> +++ b/drivers/mailbox/hisilicon/Kconfig

> +config HISI_MBOX
> +	bool "Hisilicon's Mailbox"
> +	depends on ARCH_HISI || OF

ARCH_HISI is available on either ARM64 or ARM. ARM64 selects OF. On ARM
ARCH_HISI depends on ARCH_MULTIV7 which depends on ARCH_MULTIPLATFORM.
That selects USE_OF which on its turn selects OF.

So, HISI_MBOX implies OF, correct?

> +	help
> +	  Support for mailbox drivers on Hisilicon series of SoCs.
> +
> +config HI6220_MBOX
> +	tristate "Hi6220 Mailbox Controller"
> +	depends on HISI_MBOX
> +	help
> +	  An implementation of the hi6220 mailbox. It is used to send message
> +	  between application processors and MCU. Say Y here if you want to build
> +	  the Hi6220 mailbox controller driver.

> --- /dev/null
> +++ b/drivers/mailbox/hisilicon/common.c

> +int hisi_mbox_register(struct hisi_mbox_hw *mbox_hw)
> +{
> +	[...]
> +
> +	mbox->chan = devm_kzalloc(dev,
> +		mbox_hw->chan_num * sizeof(struct mbox_chan), GFP_KERNEL);
> +	if (!mbox)

	if (!mbox->chan)

> +		return -ENOMEM;
> +
> +	[...]
> +}

> --- /dev/null
> +++ b/drivers/mailbox/hisilicon/hi6220-mailbox.c

> +static struct platform_driver hi6220_mbox_driver = {
> +	.driver = {
> +		[...]
> +		.of_match_table = of_match_ptr(hi6220_mbox_of_match),

of_match_ptr(x) becomes NULL when CONFIG_OF is not defined. But I think
CONFIG_OF will always be defined, see above. So of_match_ptr() isn't per
se needed.

> +	},
> +	[...]
> +};

> +static int __init hi6220_mbox_init(void)
> +{
> +	return platform_driver_register(&hi6220_mbox_driver);
> +}
> +module_init(hi6220_mbox_init);
> +
> +static void __exit hi6220_mbox_exit(void)
> +{
> +	platform_driver_unregister(&hi6220_mbox_driver);
> +}
> +module_exit(hi6220_mbox_exit);

This could be flattened into one line:
    module_platform_driver(hi6220_mbox_driver);

Thanks,


Paul Bolle

Re: [RFC PATCH 2/3] mailbox: Hisilicon: add mailbox driver

[Leo Yan]

Hi Paul,

On Tue, Aug 04, 2015 at 10:30:24AM +0200, Paul Bolle wrote:
> (This RFC was part of this mornings catch of my crude mail filter. So,
> for what it's worth, what follows are a few random comments for the few
> things I'm able to spot.)
> 
> On ma, 2015-08-03 at 09:13 +0800, Leo Yan wrote:
> > --- /dev/null
> > +++ b/drivers/mailbox/hisilicon/Kconfig
> 
> > +config HISI_MBOX
> > +	bool "Hisilicon's Mailbox"
> > +	depends on ARCH_HISI || OF
> 
> ARCH_HISI is available on either ARM64 or ARM. ARM64 selects OF. On ARM
> ARCH_HISI depends on ARCH_MULTIV7 which depends on ARCH_MULTIPLATFORM.
> That selects USE_OF which on its turn selects OF.
> 
> So, HISI_MBOX implies OF, correct?

Exactly, will simply use "depends on ARCH_HISI".

> > +	help
> > +	  Support for mailbox drivers on Hisilicon series of SoCs.
> > +
> > +config HI6220_MBOX
> > +	tristate "Hi6220 Mailbox Controller"
> > +	depends on HISI_MBOX
> > +	help
> > +	  An implementation of the hi6220 mailbox. It is used to send message
> > +	  between application processors and MCU. Say Y here if you want to build
> > +	  the Hi6220 mailbox controller driver.
> 
> > --- /dev/null
> > +++ b/drivers/mailbox/hisilicon/common.c
> 
> > +int hisi_mbox_register(struct hisi_mbox_hw *mbox_hw)
> > +{
> > +	[...]
> > +
> > +	mbox->chan = devm_kzalloc(dev,
> > +		mbox_hw->chan_num * sizeof(struct mbox_chan), GFP_KERNEL);
> > +	if (!mbox)
> 
> 	if (!mbox->chan)

Will fix.

> > +		return -ENOMEM;
> > +
> > +	[...]
> > +}
> 
> > --- /dev/null
> > +++ b/drivers/mailbox/hisilicon/hi6220-mailbox.c
> 
> > +static struct platform_driver hi6220_mbox_driver = {
> > +	.driver = {
> > +		[...]
> > +		.of_match_table = of_match_ptr(hi6220_mbox_of_match),
> 
> of_match_ptr(x) becomes NULL when CONFIG_OF is not defined. But I think
> CONFIG_OF will always be defined, see above. So of_match_ptr() isn't per
> se needed.

Will fix.

> > +	},
> > +	[...]
> > +};
> 
> > +static int __init hi6220_mbox_init(void)
> > +{
> > +	return platform_driver_register(&hi6220_mbox_driver);
> > +}
> > +module_init(hi6220_mbox_init);
> > +
> > +static void __exit hi6220_mbox_exit(void)
> > +{
> > +	platform_driver_unregister(&hi6220_mbox_driver);
> > +}
> > +module_exit(hi6220_mbox_exit);
> 
> This could be flattened into one line:
>     module_platform_driver(hi6220_mbox_driver);

Will refine. And thanks a lot for your review.

Thanks,
Leo Yan

Re: [RFC PATCH 2/3] mailbox: Hisilicon: add mailbox driver

[Leo Yan]

Hi Paul,

On Tue, Aug 04, 2015 at 04:49:30PM +0800, Leo Yan wrote:
> On Tue, Aug 04, 2015 at 10:30:24AM +0200, Paul Bolle wrote:
> > (This RFC was part of this mornings catch of my crude mail filter. So,
> > for what it's worth, what follows are a few random comments for the few
> > things I'm able to spot.)

[...]

> > > +static int __init hi6220_mbox_init(void)
> > > +{
> > > +	return platform_driver_register(&hi6220_mbox_driver);
> > > +}
> > > +module_init(hi6220_mbox_init);
> > > +
> > > +static void __exit hi6220_mbox_exit(void)
> > > +{
> > > +	platform_driver_unregister(&hi6220_mbox_driver);
> > > +}
> > > +module_exit(hi6220_mbox_exit);
> > 
> > This could be flattened into one line:
> >     module_platform_driver(hi6220_mbox_driver);

Need clarify one thing: will modify to use core_initcall() for driver's
initialization; due another clock driver depends on this mailbox driver,
so mailbox init firstly with core_initcall() and use subsys_initcall()
for clock driver.

Thanks,
Leo Yan

Re: [RFC PATCH 2/3] mailbox: Hisilicon: add mailbox driver

[Paul Bolle]

On di, 2015-08-04 at 16:49 +0800, Leo Yan wrote:
> On Tue, Aug 04, 2015 at 10:30:24AM +0200, Paul Bolle wrote:
> > On ma, 2015-08-03 at 09:13 +0800, Leo Yan wrote:
> > > --- /dev/null
> > > +++ b/drivers/mailbox/hisilicon/Kconfig
> > 
> > > +config HISI_MBOX
> > > +	bool "Hisilicon's Mailbox"
> > > +	depends on ARCH_HISI || OF
> > 
> > ARCH_HISI is available on either ARM64 or ARM. ARM64 selects OF. On ARM
> > ARCH_HISI depends on ARCH_MULTIV7 which depends on ARCH_MULTIPLATFORM.
> > That selects USE_OF which on its turn selects OF.
> > 
> > So, HISI_MBOX implies OF, correct?
> 
> Exactly, will simply use "depends on ARCH_HISI".

That change would alter the dependencies. My remark was made just to
make clear why I think CONFIG_OF will always be defined. But now I guess
you actually meant
	depends on ARCH_HISI && OF

And that would, I think, indeed be equivalent to
	depends on ARCH_HISI

Thanks,


Paul Bolle

Re: [RFC PATCH 2/3] mailbox: Hisilicon: add mailbox driver

[Leo Yan]

On Tue, Aug 04, 2015 at 11:17:39AM +0200, Paul Bolle wrote:
> On di, 2015-08-04 at 16:49 +0800, Leo Yan wrote:
> > On Tue, Aug 04, 2015 at 10:30:24AM +0200, Paul Bolle wrote:
> > > On ma, 2015-08-03 at 09:13 +0800, Leo Yan wrote:
> > > > --- /dev/null
> > > > +++ b/drivers/mailbox/hisilicon/Kconfig
> > > 
> > > > +config HISI_MBOX
> > > > +	bool "Hisilicon's Mailbox"
> > > > +	depends on ARCH_HISI || OF
> > > 
> > > ARCH_HISI is available on either ARM64 or ARM. ARM64 selects OF. On ARM
> > > ARCH_HISI depends on ARCH_MULTIV7 which depends on ARCH_MULTIPLATFORM.
> > > That selects USE_OF which on its turn selects OF.
> > > 
> > > So, HISI_MBOX implies OF, correct?
> > 
> > Exactly, will simply use "depends on ARCH_HISI".
> 
> That change would alter the dependencies. My remark was made just to
> make clear why I think CONFIG_OF will always be defined. But now I guess
> you actually meant
> 	depends on ARCH_HISI && OF

Correct, shame for my wrong logic :)

> And that would, I think, indeed be equivalent to
> 	depends on ARCH_HISI

Thanks,
Leo Yan

[RFC PATCH 3/3] arm64: dts: add Hi6220 mailbox node

[Leo Yan]

On Hi6220, below memory regions in DDR have specific purpose:

- 0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
- 0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
- 0x06df,f000 - 0x06df,ffff: For mailbox message data.

This patch reserves these memory regions and add device node for
mailbox in dts.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index e36a539..1715f9d 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -7,9 +7,6 @@
 
 /dts-v1/;
 
-/*Reserved 1MB memory for MCU*/
-/memreserve/ 0x05e00000 0x00100000;
-
 #include "hi6220.dtsi"
 
 / {
@@ -28,4 +25,21 @@
 		device_type = "memory";
 		reg = <0x0 0x0 0x0 0x40000000>;
 	};
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		mcu-buf@05e00000 {
+			no-map;
+			reg = <0x0 0x0740f000 0x0 0x00001000>,	/* MCU firmware section */
+			      <0x0 0x05e00000 0x0 0x00100000>;	/* MCU firmware buffer */
+		};
+
+		mbox-buf@06dff000 {
+			no-map;
+			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
+		};
+	};
 };
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index 3f03380..b42a9b7 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -167,5 +167,13 @@
 			clocks = <&ao_ctrl 36>, <&ao_ctrl 36>;
 			clock-names = "uartclk", "apb_pclk";
 		};
+
+		mailbox: mailbox {
+			#mbox-cells = <1>;
+			compatible = "hisilicon,hi6220-mbox";
+			reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
+			      <0x0 0x06dff800 0x0 0x0800>; /* Mailbox buffer */
+			interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
+		};
 	};
 };
-- 
1.9.1

Re: [RFC PATCH 0/3] mailbox: hisilicon: add mailbox driver

[Jassi Brar]

On Mon, Aug 3, 2015 at 6:43 AM, Leo Yan <leo.yan@linaro.org> wrote:
> This patch series is to implement Hisilicon's mailbox driver and enable
> the mailbox controller on Hi6220.
>
Cool.

> The Hisilicon mailbox supports up to 32 channels. Each channel is
> unidirectional with a maximum message size of 8 words. I/O is performed
> using register access (there is no DMA) and the cell raises an interrupt
> when messages are received.
>
FYI, none of other platforms has mailbox with DMA. 8-word deep fifo,
RX interrupt
and optional TX interrupt is pretty common. So cool still.

> For easily extending for Hisilicon series SoCs (SoCs may have difference
> for register's definition with each other), so firstly implement common
> mailbox driver; this common mailbox driver provides three mainly
> functionality:
>
>  - help register channels into framework;
>  - hook low level callback functions for register's operations;
>  - Enhance rx channel's message queue, which is based on the code in
>    drivers/mailbox/omap-mailbox.c.
>
Not cool.
 Please don't reinvent the wheel by having platform specific
implementation of the mailbox api. Which vendor doesn't plan to roll
out new SoCs, and hence variations of mailbox controllers?  The OMAP
stack predates the common api, and was actually supposed to be
converted over eventually. Please implement just the
drivers/mailbox/hi6220-mailbox.c (preferably by the name of the
mailbox controller, if any)

Thanks.

Re: [RFC PATCH 0/3] mailbox: hisilicon: add mailbox driver

[Leo Yan]

Hi Jassi,

Thanks for review.

On Wed, Aug 05, 2015 at 04:22:01PM +0530, Jassi Brar wrote:
> On Mon, Aug 3, 2015 at 6:43 AM, Leo Yan <leo.yan@linaro.org> wrote:

[...]

> > For easily extending for Hisilicon series SoCs (SoCs may have difference
> > for register's definition with each other), so firstly implement common
> > mailbox driver; this common mailbox driver provides three mainly
> > functionality:
> >
> >  - help register channels into framework;
> >  - hook low level callback functions for register's operations;
> >  - Enhance rx channel's message queue, which is based on the code in
> >    drivers/mailbox/omap-mailbox.c.
> >
> Not cool.
>  Please don't reinvent the wheel by having platform specific
> implementation of the mailbox api. Which vendor doesn't plan to roll
> out new SoCs, and hence variations of mailbox controllers?  The OMAP
> stack predates the common api, and was actually supposed to be
> converted over eventually. Please implement just the
> drivers/mailbox/hi6220-mailbox.c (preferably by the name of the
> mailbox controller, if any)

Understood. Here i have one question, the rx channel's message queue is
looks like a common mechanism and can be added into framework file
mailbox.c, then Soc driver file can _ONLY_ focus on register level's
operations. If so, the common driver in this patch also is unnecessary.

Do you suggest to use upper method to rework patches? Or just think
it's okay to implement rx channel's message queue in hi6220-mailbox.c?

Thanks,
Leo Yan

Re: [RFC PATCH 0/3] mailbox: hisilicon: add mailbox driver

[Jassi Brar]

On Thu, Aug 6, 2015 at 4:47 AM, Leo Yan <leo.yan@linaro.org> wrote:
> Hi Jassi,
>
> Thanks for review.
>
> On Wed, Aug 05, 2015 at 04:22:01PM +0530, Jassi Brar wrote:
>> On Mon, Aug 3, 2015 at 6:43 AM, Leo Yan <leo.yan@linaro.org> wrote:
>
> [...]
>
>> > For easily extending for Hisilicon series SoCs (SoCs may have difference
>> > for register's definition with each other), so firstly implement common
>> > mailbox driver; this common mailbox driver provides three mainly
>> > functionality:
>> >
>> >  - help register channels into framework;
>> >  - hook low level callback functions for register's operations;
>> >  - Enhance rx channel's message queue, which is based on the code in
>> >    drivers/mailbox/omap-mailbox.c.
>> >
>> Not cool.
>>  Please don't reinvent the wheel by having platform specific
>> implementation of the mailbox api. Which vendor doesn't plan to roll
>> out new SoCs, and hence variations of mailbox controllers?  The OMAP
>> stack predates the common api, and was actually supposed to be
>> converted over eventually. Please implement just the
>> drivers/mailbox/hi6220-mailbox.c (preferably by the name of the
>> mailbox controller, if any)
>
> Understood. Here i have one question, the rx channel's message queue is
> looks like a common mechanism and can be added into framework file
> mailbox.c, then Soc driver file can _ONLY_ focus on register level's
> operations. If so, the common driver in this patch also is unnecessary.
>
Yes, that's what I say, no 'common' driver for a platform.

> Do you suggest to use upper method to rework patches? Or just think
> it's okay to implement rx channel's message queue in hi6220-mailbox.c?
>
The code in drivers/mailbox/ should only manage the controller
(registers and interrupts). Everything else (queues, shmem etc) should
be in platform specific client driver(s).

Re: [RFC PATCH 0/3] mailbox: hisilicon: add mailbox driver

[Leo Yan]

Hi Jassi,

On Mon, Aug 10, 2015 at 12:32:05PM +0530, Jassi Brar wrote:
> On Thu, Aug 6, 2015 at 4:47 AM, Leo Yan <leo.yan@linaro.org> wrote:
> > Hi Jassi,
> >
> > Thanks for review.
> >
> > On Wed, Aug 05, 2015 at 04:22:01PM +0530, Jassi Brar wrote:
> >> On Mon, Aug 3, 2015 at 6:43 AM, Leo Yan <leo.yan@linaro.org> wrote:
> >
> > [...]
> >
> >> > For easily extending for Hisilicon series SoCs (SoCs may have difference
> >> > for register's definition with each other), so firstly implement common
> >> > mailbox driver; this common mailbox driver provides three mainly
> >> > functionality:
> >> >
> >> >  - help register channels into framework;
> >> >  - hook low level callback functions for register's operations;
> >> >  - Enhance rx channel's message queue, which is based on the code in
> >> >    drivers/mailbox/omap-mailbox.c.
> >> >
> >> Not cool.
> >>  Please don't reinvent the wheel by having platform specific
> >> implementation of the mailbox api. Which vendor doesn't plan to roll
> >> out new SoCs, and hence variations of mailbox controllers?  The OMAP
> >> stack predates the common api, and was actually supposed to be
> >> converted over eventually. Please implement just the
> >> drivers/mailbox/hi6220-mailbox.c (preferably by the name of the
> >> mailbox controller, if any)
> >
> > Understood. Here i have one question, the rx channel's message queue is
> > looks like a common mechanism and can be added into framework file
> > mailbox.c, then Soc driver file can _ONLY_ focus on register level's
> > operations. If so, the common driver in this patch also is unnecessary.
> >
> Yes, that's what I say, no 'common' driver for a platform.
> 
> > Do you suggest to use upper method to rework patches? Or just think
> > it's okay to implement rx channel's message queue in hi6220-mailbox.c?
> >
> The code in drivers/mailbox/ should only manage the controller
> (registers and interrupts). Everything else (queues, shmem etc) should
> be in platform specific client driver(s).

Thanks for suggestion, will send new version patches for reviewing.

Thanks,
Leo Yan