diff mbox

[v5,2/7] mailbox: arm_mhu: add driver for ARM MHU controller

Message ID 1422955763-6630-1-git-send-email-Vincent.Yang@tw.fujitsu.com
State New
Headers show

Commit Message

Vincent Yang Feb. 3, 2015, 9:29 a.m. UTC 
From: Jassi Brar <jaswinder.singh@linaro.org>

Add driver for the ARM Primecell Message-Handling-Unit(MHU) controller.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
Signed-off-by: Andy Green <andy.green@linaro.org>
Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
Signed-off-by: Tetsuya Nuriya <nuriya.tetsuya@jp.fujitsu.com>
---
 .../devicetree/bindings/mailbox/arm-mhu.txt        |  35 ++++
 drivers/mailbox/Kconfig                            |   7 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/arm_mhu.c                          | 203 +++++++++++++++++++++
 4 files changed, 247 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/arm-mhu.txt
 create mode 100644 drivers/mailbox/arm_mhu.c

Comments

Jassi Brar Feb. 3, 2015, 2:39 p.m. UTC | #1 
On 3 February 2015 at 18:02, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Feb 03, 2015 at 05:29:23PM +0800, Vincent Yang wrote:
>> +static int mhu_send_data(struct mbox_chan *chan, void *data)
>> +{
>> +     struct mhu_link *mlink = chan->con_priv;
>> +
>> +     pr_debug("%s:%d\n", __func__, __LINE__);
>> +     if (!mhu_last_tx_done(chan)) {
>> +             dev_err(chan->mbox->dev, "Last TX(%d) pending!\n", mlink->irq);
>> +             return -EBUSY;
>> +     }
>> +
>> +     writel_relaxed((u32)data, mlink->tx_reg + INTR_SET_OFS);
>
> Doesn't that cause a GCC warning?
>
I don't see any, but I'll drop the cast.

>> +static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
>> +{
>> +     int i, err;
>> +     struct arm_mhu *mhu;
>> +     struct device *dev = &adev->dev;
>> +     int mhu_reg[3] = {0x0, 0x20, 0x200};
>> +
>> +     err = amba_request_regions(adev, NULL);
>> +     if (err)
>> +             return err;
>> +
>> +     /* Allocate memory for device */
>> +     mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
>> +     if (!mhu)
>> +             return -ENOMEM;
>> +
>> +     mhu->base = devm_ioremap(dev, adev->res.start,
>> +                              resource_size(&adev->res));
>> +     if (!mhu->base) {
>> +             dev_err(dev, "ioremap failed\n");
>> +             return -ENXIO;
>
>         mhu->base = devm_ioremap_resource(dev, &adev->res);
>         if (IS_ERR(mhu->base)) {
>                 dev_err(dev, "ioremap failed\n");
>                 return PTR_ERR(mhu->base);
>
OK

>> +
>> +     err = mbox_controller_register(&mhu->mbox);
>> +     if (err) {
>> +             dev_err(dev, "Failed to register mailboxes %d\n", err);
>> +             return err;
>> +     }
>> +
>> +     dev_err(dev, "ARM MHU Mailbox registered\n");
>
> Why is this an error?
>
ouch! this sneaked in from debugging code. Will make it info.

Thanks.
Jassi Brar Feb. 4, 2015, 3:27 a.m. UTC | #2 
On 3 February 2015 at 20:55, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 03 February 2015 14:46:11 Russell King - ARM Linux wrote:
>> On Tue, Feb 03, 2015 at 08:09:34PM +0530, Jassi Brar wrote:
>> > On 3 February 2015 at 18:02, Russell King - ARM Linux
>> > <linux@arm.linux.org.uk> wrote:
>> > > On Tue, Feb 03, 2015 at 05:29:23PM +0800, Vincent Yang wrote:
>> > >> +static int mhu_send_data(struct mbox_chan *chan, void *data)
>> > >> +{
>> > >> +     struct mhu_link *mlink = chan->con_priv;
>> > >> +
>> > >> +     pr_debug("%s:%d\n", __func__, __LINE__);
>> > >> +     if (!mhu_last_tx_done(chan)) {
>> > >> +             dev_err(chan->mbox->dev, "Last TX(%d) pending!\n", mlink->irq);
>> > >> +             return -EBUSY;
>> > >> +     }
>> > >> +
>> > >> +     writel_relaxed((u32)data, mlink->tx_reg + INTR_SET_OFS);
>> > >
>> > > Doesn't that cause a GCC warning?
>> > >
>> > I don't see any, but I'll drop the cast.
>>
>> A cast is probably needed.  You're right that GCC no longer warns about
>> this - I'm pretty sure it used to complain about casting pointers to
>> ints, and we used to need to cast to "unsigned long" first.
>
> It definitely warns about converting an u64 to pointer or back,
> and it warns on 64-bit machines about this conversion.
>
> On a related note, I wonder why the driver actually converts a pointer to
> int. I believe we have discussed this in the past but don't remember the
> outcome.
We never discussed this (MHU) driver. It was the generic mailbox API
where, IIRC, I did take into account all your comments. The API does
do what you had asked.

> I had expected to see here something like:
>
> static int mhu_send_data(struct mbox_chan *chan, void *data)
> {
>         struct mhu_link *mlink = chan->con_priv;
>         u32 *arg = data;
>
>         writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS);
> }
>
> i.e. dereferencing the pointer instead of using the actual value.
>
OK, just curious how is this (dereferencing to the u32 variable on
stack of the client driver) better?

Thanks.
Jassi Brar Feb. 4, 2015, 2:34 p.m. UTC | #3 
On 4 February 2015 at 15:59, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 04 February 2015 08:57:43 Jassi Brar wrote:
>> On 3 February 2015 at 20:55, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Tuesday 03 February 2015 14:46:11 Russell King - ARM Linux wrote:
>> >> On Tue, Feb 03, 2015 at 08:09:34PM +0530, Jassi Brar wrote:
>
>> > I had expected to see here something like:
>> >
>> > static int mhu_send_data(struct mbox_chan *chan, void *data)
>> > {
>> >         struct mhu_link *mlink = chan->con_priv;
>> >         u32 *arg = data;
>> >
>> >         writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS);
>> > }
>> >
>> > i.e. dereferencing the pointer instead of using the actual value.
>> >
>> OK, just curious how is this (dereferencing to the u32 variable on
>> stack of the client driver) better?
>
> The API as I understand is defined to use the pointer to point to
> a chunk of data of fixed size, with the size being known to both
> the client driver and the mailbox driver. This is the reason for
> having a pointer in the first place.
>
Yes, we are on the same page. I just have a slightly more liberal view
about the usage of 'void* data'.

> Using the bits of the pointer as the message instead of pointing
> to the message feels like an abuse of the API.
>
I can see your POV.
Now consider a client, like mine, that sends a u32 value as the data.
But unlike me, the client uses the mailbox api in 'async' mode i.e,
register a callback function, submit a 32bit message and move on. It
is perfectly doable, but doesn't kalloc'ing a u32 for each submission,
seem overkill?  Lets say what the client and controller drivers do in
their bedroom is none of the API's business.

> Maybe it would have been better to pass the size explictly as
> a third argument in the API to make that clear.
>
Actually that did come up for consideration. But since the structure
of 'data' packet is already known to the controller driver, having to
also tell the size of that structure seems redundant.

Thanks.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
new file mode 100644
index 0000000..986a205
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
@@ -0,0 +1,35 @@ 
+ARM MHU Mailbox Driver
+======================
+
+The ARM's Message-Handling-Unit (MHU) is a mailbox controller that has
+3 independent channels/links to communicate with remote processor(s).
+ MHU links are hardwired on a platform. A link raises interrupt for any
+received data. However, there is no specified way of knowing if the sent
+data has been read by the remote. This driver assumes the sender polls
+STAT register and the remote clears it after having read the data.
+
+Mailbox Device Node:
+====================
+
+Required properties:
+--------------------
+- compatible:		Shall be "arm,mhu" & "arm,primecell"
+- reg:			Contains the mailbox register address range (base
+			address and length)
+- #mbox-cells		Shall be 1
+- interrupts:		Contains the interrupt information corresponding to
+			each of the 3 links of MHU.
+
+Example:
+--------
+
+	mhu: mailbox@2b1f0000 {
+		#mbox-cells = <1>;
+		compatible = "arm,mhu", "arm,primecell";
+		reg = <0 0x2b1f0000 0x1000>;
+		interrupts = <0 36 4>,
+			     <0 35 4>,
+			     <0 37 4>;
+		clocks = <&clock 0 2 1>;
+		clock-names = "apb_pclk";
+	};
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index c04fed9..9238440 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -6,6 +6,13 @@  menuconfig MAILBOX
 	  signals. Say Y if your platform supports hardware mailboxes.
 
 if MAILBOX
+
+config ARM_MHU
+	tristate "ARM MHU Mailbox"
+	depends on ARM
+	help
+	  Say Y here if you want to build the ARM MHU controller driver
+
 config PL320_MBOX
 	bool "ARM PL320 Mailbox"
 	depends on ARM_AMBA
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index dd412c2..c83791d 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -2,6 +2,8 @@ 
 
 obj-$(CONFIG_MAILBOX)		+= mailbox.o
 
+obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
+
 obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
 
 obj-$(CONFIG_OMAP2PLUS_MBOX)	+= omap-mailbox.o
diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
new file mode 100644
index 0000000..5768936
--- /dev/null
+++ b/drivers/mailbox/arm_mhu.c
@@ -0,0 +1,203 @@ 
+/*
+ * Copyright (C) 2013-2015 Fujitsu Semiconductor Ltd.
+ * Copyright (C) 2015 Linaro Ltd.
+ * Author: Jassi Brar <jaswinder.singh@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/interrupt.h>
+#include <linux/spinlock.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/amba/bus.h>
+#include <linux/mailbox_controller.h>
+
+#define INTR_STAT_OFS	0x0
+#define INTR_SET_OFS	0x8
+#define INTR_CLR_OFS	0x10
+
+struct mhu_link {
+	unsigned irq;
+	void __iomem *tx_reg;
+	void __iomem *rx_reg;
+};
+
+struct arm_mhu {
+	void __iomem *base;
+	struct mhu_link mlink[3];
+	struct mbox_chan chan[3];
+	struct mbox_controller mbox;
+};
+
+static irqreturn_t mhu_rx_interrupt(int irq, void *p)
+{
+	struct mbox_chan *chan = p;
+	struct mhu_link *mlink = chan->con_priv;
+	u32 val;
+
+	pr_debug("%s:%d\n", __func__, __LINE__);
+	val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS);
+	if (!val)
+		return IRQ_NONE;
+
+	mbox_chan_received_data(chan, (void *)val);
+
+	writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS);
+
+	return IRQ_HANDLED;
+}
+
+static bool mhu_last_tx_done(struct mbox_chan *chan)
+{
+	struct mhu_link *mlink = chan->con_priv;
+	u32 val;
+
+	pr_debug("%s:%d\n", __func__, __LINE__);
+	val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
+
+	return (val == 0);
+}
+
+static int mhu_send_data(struct mbox_chan *chan, void *data)
+{
+	struct mhu_link *mlink = chan->con_priv;
+
+	pr_debug("%s:%d\n", __func__, __LINE__);
+	if (!mhu_last_tx_done(chan)) {
+		dev_err(chan->mbox->dev, "Last TX(%d) pending!\n", mlink->irq);
+		return -EBUSY;
+	}
+
+	writel_relaxed((u32)data, mlink->tx_reg + INTR_SET_OFS);
+
+	return 0;
+}
+
+static int mhu_startup(struct mbox_chan *chan)
+{
+	struct mhu_link *mlink = chan->con_priv;
+	u32 val;
+	int ret;
+
+	pr_debug("%s:%d\n", __func__, __LINE__);
+	val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
+	writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);
+
+	ret = request_irq(mlink->irq, mhu_rx_interrupt, 0, "mhu_link", chan);
+	if (ret) {
+		dev_err(chan->mbox->dev,
+			"Unable to aquire IRQ %d\n", mlink->irq);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void mhu_shutdown(struct mbox_chan *chan)
+{
+	struct mhu_link *mlink = chan->con_priv;
+
+	pr_debug("%s:%d\n", __func__, __LINE__);
+	free_irq(mlink->irq, chan);
+}
+
+static struct mbox_chan_ops mhu_ops = {
+	.send_data = mhu_send_data,
+	.startup = mhu_startup,
+	.shutdown = mhu_shutdown,
+	.last_tx_done = mhu_last_tx_done,
+};
+
+static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
+{
+	int i, err;
+	struct arm_mhu *mhu;
+	struct device *dev = &adev->dev;
+	int mhu_reg[3] = {0x0, 0x20, 0x200};
+
+	err = amba_request_regions(adev, NULL);
+	if (err)
+		return err;
+
+	/* Allocate memory for device */
+	mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
+	if (!mhu)
+		return -ENOMEM;
+
+	mhu->base = devm_ioremap(dev, adev->res.start,
+				 resource_size(&adev->res));
+	if (!mhu->base) {
+		dev_err(dev, "ioremap failed\n");
+		return -ENXIO;
+	}
+
+	for (i = 0; i < 3; i++) {
+		mhu->chan[i].con_priv = &mhu->mlink[i];
+		mhu->mlink[i].irq = adev->irq[i];
+		mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
+		mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + 0x100;
+	}
+
+	mhu->mbox.dev = dev;
+	mhu->mbox.chans = &mhu->chan[0];
+	mhu->mbox.num_chans = 3;
+	mhu->mbox.ops = &mhu_ops;
+	mhu->mbox.txdone_irq = false;
+	mhu->mbox.txdone_poll = true;
+	mhu->mbox.txpoll_period = 10;
+
+	amba_set_drvdata(adev, mhu);
+
+	err = mbox_controller_register(&mhu->mbox);
+	if (err) {
+		dev_err(dev, "Failed to register mailboxes %d\n", err);
+		return err;
+	}
+
+	dev_err(dev, "ARM MHU Mailbox registered\n");
+	return 0;
+}
+
+static int mhu_remove(struct amba_device *adev)
+{
+	struct arm_mhu *mhu = amba_get_drvdata(adev);
+
+	mbox_controller_unregister(&mhu->mbox);
+
+	return 0;
+}
+
+static struct amba_id mhu_ids[] = {
+	{
+		.id	= 0x1bb098,
+		.mask	= 0xffffff,
+	},
+	{ 0, 0 },
+};
+MODULE_DEVICE_TABLE(amba, mhu_ids);
+
+static struct amba_driver arm_mhu_driver = {
+	.drv = {
+		.name	= "mhu",
+	},
+	.id_table	= mhu_ids,
+	.probe		= mhu_probe,
+	.remove		= mhu_remove,
+};
+module_amba_driver(arm_mhu_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("ARM MHU Driver");
+MODULE_AUTHOR("Jassi Brar <jassisinghbrar@gmail.com>");