[PATCH v4 0/3] Add support for Broadcom iProc mailbox controller

[PATCH v4 0/3] Add support for Broadcom iProc mailbox controller

[Jonathan Richardson]

This patch set contains mailbox support for Broadcom iProc based SoC's. The
mailbox controller handles all communication with a Cortex-M0 MCU processor that
provides support for power, clock, and reset management. The mailbox controller
for Cygnus is enabled in DT.

Changes from v3:
 - DT changes merged into one commit.

Changes from v2:
 - Fixed binding to remove leading 0 (Rob's comment). Also added more detail as
   suggested by Jassi.
 - Added changelog to dt binding and enable dts commits.
 - Changed year in copyright in bcm_iproc_mailbox.h.
 - Changes to bcm-cygnus.dtsi were prematurely merged into the mainline after
   Rob acked the DT changes but Jassi suggested changes to the mailbox driver.
   The result was that the mailbox controller node and changes to the crmu gpio
   driver to enable interrupt handling were incorrectly added to the dtsi. A
   commit is included in this patchset to revert those changes. Enabling
   interrupt handling in the the crmu gpio driver will be introduced when the
   interrupt controller driver is added to provide mailbox interrupt support.

Jonathan Richardson (3):
  dt-bindings: Document Broadcom iProc mailbox controller driver
  mailbox: Add iProc mailbox controller driver
  ARM: dts: Enable Broadcom iProc mailbox controller

 .../bindings/mailbox/brcm,iproc-mailbox.txt        |  18 ++
 arch/arm/boot/dts/bcm-cygnus.dtsi                  |   8 +-
 drivers/mailbox/Kconfig                            |  10 ++
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/bcm-iproc-mailbox.c                | 199 +++++++++++++++++++++
 include/linux/mailbox/bcm_iproc_mailbox.h          |  32 ++++
 6 files changed, 262 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt
 create mode 100644 drivers/mailbox/bcm-iproc-mailbox.c
 create mode 100644 include/linux/mailbox/bcm_iproc_mailbox.h

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 0/3] Add support for Broadcom iProc mailbox controller

[Jonathan Richardson]

This patch set contains mailbox support for Broadcom iProc based SoC's. The
mailbox controller handles all communication with a Cortex-M0 MCU processor that
provides support for power, clock, and reset management. The mailbox controller
for Cygnus is enabled in DT.

Changes from v3:
 - DT changes merged into one commit.

Changes from v2:
 - Fixed binding to remove leading 0 (Rob's comment). Also added more detail as
   suggested by Jassi.
 - Added changelog to dt binding and enable dts commits.
 - Changed year in copyright in bcm_iproc_mailbox.h.
 - Changes to bcm-cygnus.dtsi were prematurely merged into the mainline after
   Rob acked the DT changes but Jassi suggested changes to the mailbox driver.
   The result was that the mailbox controller node and changes to the crmu gpio
   driver to enable interrupt handling were incorrectly added to the dtsi. A
   commit is included in this patchset to revert those changes. Enabling
   interrupt handling in the the crmu gpio driver will be introduced when the
   interrupt controller driver is added to provide mailbox interrupt support.

Jonathan Richardson (3):
  dt-bindings: Document Broadcom iProc mailbox controller driver
  mailbox: Add iProc mailbox controller driver
  ARM: dts: Enable Broadcom iProc mailbox controller

 .../bindings/mailbox/brcm,iproc-mailbox.txt        |  18 ++
 arch/arm/boot/dts/bcm-cygnus.dtsi                  |   8 +-
 drivers/mailbox/Kconfig                            |  10 ++
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/bcm-iproc-mailbox.c                | 199 +++++++++++++++++++++
 include/linux/mailbox/bcm_iproc_mailbox.h          |  32 ++++
 6 files changed, 262 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt
 create mode 100644 drivers/mailbox/bcm-iproc-mailbox.c
 create mode 100644 include/linux/mailbox/bcm_iproc_mailbox.h

-- 
1.9.1

[PATCH v4 1/3] dt-bindings: Document Broadcom iProc mailbox controller driver

[Jonathan Richardson]

Add binding document for brcm,iproc-mailbox.

Reviewed-by: Jonathan Richardson <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Tested-by: Jonathan Richardson <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Vikram Prakash <vikram.prakash-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Ray Jui <ray.jui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Shreesha Rajashekar <shreesha.rajashekar-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jonathan Richardson <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 .../devicetree/bindings/mailbox/brcm,iproc-mailbox.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt b/Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt
new file mode 100644
index 0000000..a832feb
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt
@@ -0,0 +1,18 @@
+* Broadcom's iProc Mailbox Controller
+
+Broadcom iProc architected SoC's have an always on Cortex-M0 MCU processor that
+handles support for power, clock, and reset management. The iProc mailbox
+controller handles all communication with this processor.
+
+Required properties:
+- compatible: Must be "brcm,iproc-mailbox"
+- reg: Defines the base address of the mailbox controller.
+- #mbox-cells: Must be 1.
+
+Example:
+
+	mailbox: mailbox@3024024 {
+		compatible = "brcm,iproc-mailbox";
+		reg = <0x03024024 0x8>;
+		#mbox-cells = <1>;
+	};
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/3] dt-bindings: Document Broadcom iProc mailbox controller driver

[Jonathan Richardson]

Add binding document for brcm,iproc-mailbox.

Reviewed-by: Jonathan Richardson <jonathan.richardson@broadcom.com>
Tested-by: Jonathan Richardson <jonathan.richardson@broadcom.com>
Reviewed-by: Vikram Prakash <vikram.prakash@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Shreesha Rajashekar <shreesha.rajashekar@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathan.richardson@broadcom.com>
---
 .../devicetree/bindings/mailbox/brcm,iproc-mailbox.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt b/Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt
new file mode 100644
index 0000000..a832feb
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt
@@ -0,0 +1,18 @@
+* Broadcom's iProc Mailbox Controller
+
+Broadcom iProc architected SoC's have an always on Cortex-M0 MCU processor that
+handles support for power, clock, and reset management. The iProc mailbox
+controller handles all communication with this processor.
+
+Required properties:
+- compatible: Must be "brcm,iproc-mailbox"
+- reg: Defines the base address of the mailbox controller.
+- #mbox-cells: Must be 1.
+
+Example:
+
+	mailbox: mailbox at 3024024 {
+		compatible = "brcm,iproc-mailbox";
+		reg = <0x03024024 0x8>;
+		#mbox-cells = <1>;
+	};
-- 
1.9.1

[PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

The Broadcom iProc mailbox controller handles all communication with a
Cortex-M0 MCU processor that provides support for power, clock, and
reset management.

Tested-by: Jonathan Richardson <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Jonathan Richardson <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Vikram Prakash <vikram.prakash-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Shreesha Rajashekar <shreesha.rajashekar-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Ray Jui <ray.jui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jonathan Richardson <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 drivers/mailbox/Kconfig                   |  10 ++
 drivers/mailbox/Makefile                  |   2 +
 drivers/mailbox/bcm-iproc-mailbox.c       | 199 ++++++++++++++++++++++++++++++
 include/linux/mailbox/bcm_iproc_mailbox.h |  32 +++++
 4 files changed, 243 insertions(+)
 create mode 100644 drivers/mailbox/bcm-iproc-mailbox.c
 create mode 100644 include/linux/mailbox/bcm_iproc_mailbox.h

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ceff415..b8828be 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -152,4 +152,14 @@ config BCM_PDC_MBOX
 	  Mailbox implementation for the Broadcom PDC ring manager,
 	  which provides access to various offload engines on Broadcom
 	  SoCs. Say Y here if you want to use the Broadcom PDC.
+
+config BCM_IPROC_MBOX
+	bool "Broadcom iProc Mailbox"
+	depends on ARCH_BCM_IPROC || COMPILE_TEST
+	default ARCH_BCM_IPROC
+	help
+	  Broadcom iProc architected SoC's have an always on Cortex-M0 MCU processor
+	  that handles support for power, clock, and reset management. The iProc
+	  mailbox controller handles all communication with this processor.
+
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 7dde4f6..374f852 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -31,3 +31,5 @@ obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
 obj-$(CONFIG_BCM_PDC_MBOX)	+= bcm-pdc-mailbox.o
 
 obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
+
+obj-$(CONFIG_BCM_IPROC_MBOX)	+= bcm-iproc-mailbox.o
diff --git a/drivers/mailbox/bcm-iproc-mailbox.c b/drivers/mailbox/bcm-iproc-mailbox.c
new file mode 100644
index 0000000..36ecad5
--- /dev/null
+++ b/drivers/mailbox/bcm-iproc-mailbox.c
@@ -0,0 +1,199 @@
+/*
+ * Copyright (C) 2017 Broadcom.
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox/bcm_iproc_mailbox.h>
+#include <linux/delay.h>
+
+#define IPROC_CRMU_MAILBOX0_OFFSET       0x0
+#define IPROC_CRMU_MAILBOX1_OFFSET       0x4
+
+#define M0_IPC_CMD_DONE_MASK             0x80000000
+#define M0_IPC_CMD_REPLY_MASK            0x3fff0000
+#define M0_IPC_CMD_REPLY_SHIFT           16
+
+/* Max time the M0 will take to respond to a message. */
+#define MAX_M0_TIMEOUT_MS                2
+
+struct iproc_mbox {
+	struct device         *dev;
+	void __iomem          *base;
+	spinlock_t            lock;
+	struct mbox_controller controller;
+	u32                   num_chans;
+};
+
+static const struct of_device_id iproc_mbox_of_match[] = {
+	{ .compatible = "brcm,iproc-mailbox" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, iproc_mbox_of_match);
+
+/*
+ * Sends a message to M0. The mailbox framework prevents multiple accesses to
+ * the same channel but there is only one h/w "channel". This driver allows
+ * multiple clients to create channels to the controller but must serialize
+ * access to the mailbox registers used to communicate with the M0.
+ */
+static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
+{
+	struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
+	struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
+		unsigned long flags;
+	int err = 0;
+	const int poll_period_us = 5;
+	const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
+
+	if (!msg)
+		return -EINVAL;
+
+	spin_lock_irqsave(&mbox->lock, flags);
+
+	dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
+		msg->cmd, msg->param, msg->wait_ack);
+
+	writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
+	writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
+
+	if (msg->wait_ack) {
+		int retries;
+
+		err = msg->reply_code = -ETIMEDOUT;
+		for (retries = 0; retries < max_retries; retries++) {
+			u32 val = readl(
+				mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
+			if (val & M0_IPC_CMD_DONE_MASK) {
+				/*
+				 * M0 replied - save reply code and
+				 * clear error.
+				 */
+				msg->reply_code = (val &
+					M0_IPC_CMD_REPLY_MASK) >>
+					M0_IPC_CMD_REPLY_SHIFT;
+				err = 0;
+				break;
+			}
+			udelay(poll_period_us);
+		}
+	}
+
+	spin_unlock_irqrestore(&mbox->lock, flags);
+
+	return err;
+}
+
+static int iproc_mbox_startup(struct mbox_chan *chan)
+{
+	/* Do nothing. */
+	return 0;
+}
+
+static void iproc_mbox_shutdown(struct mbox_chan *chan)
+{
+	/* Do nothing. */
+}
+
+static struct mbox_chan_ops iproc_mbox_ops = {
+	.send_data    = iproc_mbox_send_data_m0,
+	.startup      = iproc_mbox_startup,
+	.shutdown     = iproc_mbox_shutdown,
+};
+
+static int iproc_mbox_probe(struct platform_device *pdev)
+{
+	int err;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct iproc_mbox *iproc_mbox;
+	struct device_node *node;
+	const char *mbox_prop_name = "mboxes";
+	struct mbox_chan *chans;
+
+	dev_info(&pdev->dev, "Initializing iproc mailbox controller\n");
+
+	iproc_mbox = devm_kzalloc(dev, sizeof(*iproc_mbox), GFP_KERNEL);
+	if (!iproc_mbox)
+		return -ENOMEM;
+
+	iproc_mbox->dev = dev;
+	spin_lock_init(&iproc_mbox->lock);
+
+	platform_set_drvdata(pdev, iproc_mbox);
+
+	/* Count number of "mboxes" properties to determine # channels. */
+	for_each_of_allnodes(node) {
+		struct property *prop = of_find_property(
+			node, mbox_prop_name, NULL);
+		if (prop) {
+			struct device_node *mbox_phandle = of_parse_phandle(
+				node, mbox_prop_name, 0);
+			if (mbox_phandle == dev->of_node)
+				iproc_mbox->num_chans++;
+		}
+	}
+
+	if (iproc_mbox->num_chans == 0) {
+		dev_err(dev, "No mailbox clients configured\n");
+		return -ENODEV;
+	}
+
+	chans = devm_kzalloc(&pdev->dev,
+		sizeof(*chans) * iproc_mbox->num_chans, GFP_KERNEL);
+	if (!chans)
+		return -ENOMEM;
+
+	/* Initialize mailbox controller. */
+	iproc_mbox->controller.dev = iproc_mbox->dev;
+	iproc_mbox->controller.num_chans = iproc_mbox->num_chans;
+	iproc_mbox->controller.chans = chans;
+	iproc_mbox->controller.ops = &iproc_mbox_ops;
+	iproc_mbox->controller.txdone_irq = false;
+	iproc_mbox->controller.txdone_poll = false;
+	err = mbox_controller_register(&iproc_mbox->controller);
+	if (err) {
+		dev_err(&pdev->dev, "Register mailbox failed\n");
+		return err;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	iproc_mbox->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(iproc_mbox->base)) {
+		dev_err(&pdev->dev, "unable to map I/O memory\n");
+		return PTR_ERR(iproc_mbox->base);
+	}
+
+	return 0;
+}
+
+static struct platform_driver iproc_mbox_driver = {
+	.driver = {
+		.name = "brcm,iproc-mailbox",
+		.of_match_table = iproc_mbox_of_match,
+	},
+	.probe = iproc_mbox_probe,
+};
+
+static int __init iproc_mbox_init(void)
+{
+	return platform_driver_register(&iproc_mbox_driver);
+}
+arch_initcall(iproc_mbox_init);
+
+MODULE_AUTHOR("Broadcom");
+MODULE_DESCRIPTION("Broadcom iProc Mailbox Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mailbox/bcm_iproc_mailbox.h b/include/linux/mailbox/bcm_iproc_mailbox.h
new file mode 100644
index 0000000..6d0b07c
--- /dev/null
+++ b/include/linux/mailbox/bcm_iproc_mailbox.h
@@ -0,0 +1,32 @@
+/*
+ * Copyright (C) 2017 Broadcom.
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#ifndef _BCM_IPROC_MAILBOX_H_
+#define _BCM_IPROC_MAILBOX_H_
+
+/*
+ * A message to send to the M0 processor.
+ * @cmd Command to send.
+ * @param Parameter corresponding to command.
+ * @wait_ack true if mbox_send_message() should wait for a reply from the M0,
+ *   false if the M0 doesn't reply. This depends on the message being sent.
+ * @reply_code The response code from the M0 for the command sent (wait_ack was
+ *   set to true).
+ */
+struct iproc_mbox_msg {
+	u32       cmd;
+	u32       param;
+	bool      wait_ack;
+	u32       reply_code;
+};
+
+#endif
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

The Broadcom iProc mailbox controller handles all communication with a
Cortex-M0 MCU processor that provides support for power, clock, and
reset management.

Tested-by: Jonathan Richardson <jonathan.richardson@broadcom.com>
Reviewed-by: Jonathan Richardson <jonathan.richardson@broadcom.com>
Reviewed-by: Vikram Prakash <vikram.prakash@broadcom.com>
Reviewed-by: Shreesha Rajashekar <shreesha.rajashekar@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathan.richardson@broadcom.com>
---
 drivers/mailbox/Kconfig                   |  10 ++
 drivers/mailbox/Makefile                  |   2 +
 drivers/mailbox/bcm-iproc-mailbox.c       | 199 ++++++++++++++++++++++++++++++
 include/linux/mailbox/bcm_iproc_mailbox.h |  32 +++++
 4 files changed, 243 insertions(+)
 create mode 100644 drivers/mailbox/bcm-iproc-mailbox.c
 create mode 100644 include/linux/mailbox/bcm_iproc_mailbox.h

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ceff415..b8828be 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -152,4 +152,14 @@ config BCM_PDC_MBOX
 	  Mailbox implementation for the Broadcom PDC ring manager,
 	  which provides access to various offload engines on Broadcom
 	  SoCs. Say Y here if you want to use the Broadcom PDC.
+
+config BCM_IPROC_MBOX
+	bool "Broadcom iProc Mailbox"
+	depends on ARCH_BCM_IPROC || COMPILE_TEST
+	default ARCH_BCM_IPROC
+	help
+	  Broadcom iProc architected SoC's have an always on Cortex-M0 MCU processor
+	  that handles support for power, clock, and reset management. The iProc
+	  mailbox controller handles all communication with this processor.
+
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 7dde4f6..374f852 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -31,3 +31,5 @@ obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
 obj-$(CONFIG_BCM_PDC_MBOX)	+= bcm-pdc-mailbox.o
 
 obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
+
+obj-$(CONFIG_BCM_IPROC_MBOX)	+= bcm-iproc-mailbox.o
diff --git a/drivers/mailbox/bcm-iproc-mailbox.c b/drivers/mailbox/bcm-iproc-mailbox.c
new file mode 100644
index 0000000..36ecad5
--- /dev/null
+++ b/drivers/mailbox/bcm-iproc-mailbox.c
@@ -0,0 +1,199 @@
+/*
+ * Copyright (C) 2017 Broadcom.
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox/bcm_iproc_mailbox.h>
+#include <linux/delay.h>
+
+#define IPROC_CRMU_MAILBOX0_OFFSET       0x0
+#define IPROC_CRMU_MAILBOX1_OFFSET       0x4
+
+#define M0_IPC_CMD_DONE_MASK             0x80000000
+#define M0_IPC_CMD_REPLY_MASK            0x3fff0000
+#define M0_IPC_CMD_REPLY_SHIFT           16
+
+/* Max time the M0 will take to respond to a message. */
+#define MAX_M0_TIMEOUT_MS                2
+
+struct iproc_mbox {
+	struct device         *dev;
+	void __iomem          *base;
+	spinlock_t            lock;
+	struct mbox_controller controller;
+	u32                   num_chans;
+};
+
+static const struct of_device_id iproc_mbox_of_match[] = {
+	{ .compatible = "brcm,iproc-mailbox" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, iproc_mbox_of_match);
+
+/*
+ * Sends a message to M0. The mailbox framework prevents multiple accesses to
+ * the same channel but there is only one h/w "channel". This driver allows
+ * multiple clients to create channels to the controller but must serialize
+ * access to the mailbox registers used to communicate with the M0.
+ */
+static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
+{
+	struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
+	struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
+		unsigned long flags;
+	int err = 0;
+	const int poll_period_us = 5;
+	const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
+
+	if (!msg)
+		return -EINVAL;
+
+	spin_lock_irqsave(&mbox->lock, flags);
+
+	dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
+		msg->cmd, msg->param, msg->wait_ack);
+
+	writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
+	writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
+
+	if (msg->wait_ack) {
+		int retries;
+
+		err = msg->reply_code = -ETIMEDOUT;
+		for (retries = 0; retries < max_retries; retries++) {
+			u32 val = readl(
+				mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
+			if (val & M0_IPC_CMD_DONE_MASK) {
+				/*
+				 * M0 replied - save reply code and
+				 * clear error.
+				 */
+				msg->reply_code = (val &
+					M0_IPC_CMD_REPLY_MASK) >>
+					M0_IPC_CMD_REPLY_SHIFT;
+				err = 0;
+				break;
+			}
+			udelay(poll_period_us);
+		}
+	}
+
+	spin_unlock_irqrestore(&mbox->lock, flags);
+
+	return err;
+}
+
+static int iproc_mbox_startup(struct mbox_chan *chan)
+{
+	/* Do nothing. */
+	return 0;
+}
+
+static void iproc_mbox_shutdown(struct mbox_chan *chan)
+{
+	/* Do nothing. */
+}
+
+static struct mbox_chan_ops iproc_mbox_ops = {
+	.send_data    = iproc_mbox_send_data_m0,
+	.startup      = iproc_mbox_startup,
+	.shutdown     = iproc_mbox_shutdown,
+};
+
+static int iproc_mbox_probe(struct platform_device *pdev)
+{
+	int err;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct iproc_mbox *iproc_mbox;
+	struct device_node *node;
+	const char *mbox_prop_name = "mboxes";
+	struct mbox_chan *chans;
+
+	dev_info(&pdev->dev, "Initializing iproc mailbox controller\n");
+
+	iproc_mbox = devm_kzalloc(dev, sizeof(*iproc_mbox), GFP_KERNEL);
+	if (!iproc_mbox)
+		return -ENOMEM;
+
+	iproc_mbox->dev = dev;
+	spin_lock_init(&iproc_mbox->lock);
+
+	platform_set_drvdata(pdev, iproc_mbox);
+
+	/* Count number of "mboxes" properties to determine # channels. */
+	for_each_of_allnodes(node) {
+		struct property *prop = of_find_property(
+			node, mbox_prop_name, NULL);
+		if (prop) {
+			struct device_node *mbox_phandle = of_parse_phandle(
+				node, mbox_prop_name, 0);
+			if (mbox_phandle == dev->of_node)
+				iproc_mbox->num_chans++;
+		}
+	}
+
+	if (iproc_mbox->num_chans == 0) {
+		dev_err(dev, "No mailbox clients configured\n");
+		return -ENODEV;
+	}
+
+	chans = devm_kzalloc(&pdev->dev,
+		sizeof(*chans) * iproc_mbox->num_chans, GFP_KERNEL);
+	if (!chans)
+		return -ENOMEM;
+
+	/* Initialize mailbox controller. */
+	iproc_mbox->controller.dev = iproc_mbox->dev;
+	iproc_mbox->controller.num_chans = iproc_mbox->num_chans;
+	iproc_mbox->controller.chans = chans;
+	iproc_mbox->controller.ops = &iproc_mbox_ops;
+	iproc_mbox->controller.txdone_irq = false;
+	iproc_mbox->controller.txdone_poll = false;
+	err = mbox_controller_register(&iproc_mbox->controller);
+	if (err) {
+		dev_err(&pdev->dev, "Register mailbox failed\n");
+		return err;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	iproc_mbox->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(iproc_mbox->base)) {
+		dev_err(&pdev->dev, "unable to map I/O memory\n");
+		return PTR_ERR(iproc_mbox->base);
+	}
+
+	return 0;
+}
+
+static struct platform_driver iproc_mbox_driver = {
+	.driver = {
+		.name = "brcm,iproc-mailbox",
+		.of_match_table = iproc_mbox_of_match,
+	},
+	.probe = iproc_mbox_probe,
+};
+
+static int __init iproc_mbox_init(void)
+{
+	return platform_driver_register(&iproc_mbox_driver);
+}
+arch_initcall(iproc_mbox_init);
+
+MODULE_AUTHOR("Broadcom");
+MODULE_DESCRIPTION("Broadcom iProc Mailbox Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mailbox/bcm_iproc_mailbox.h b/include/linux/mailbox/bcm_iproc_mailbox.h
new file mode 100644
index 0000000..6d0b07c
--- /dev/null
+++ b/include/linux/mailbox/bcm_iproc_mailbox.h
@@ -0,0 +1,32 @@
+/*
+ * Copyright (C) 2017 Broadcom.
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#ifndef _BCM_IPROC_MAILBOX_H_
+#define _BCM_IPROC_MAILBOX_H_
+
+/*
+ * A message to send to the M0 processor.
+ * @cmd Command to send.
+ * @param Parameter corresponding to command.
+ * @wait_ack true if mbox_send_message() should wait for a reply from the M0,
+ *   false if the M0 doesn't reply. This depends on the message being sent.
+ * @reply_code The response code from the M0 for the command sent (wait_ack was
+ *   set to true).
+ */
+struct iproc_mbox_msg {
+	u32       cmd;
+	u32       param;
+	bool      wait_ack;
+	u32       reply_code;
+};
+
+#endif
-- 
1.9.1

[PATCH v4 3/3] ARM: dts: Enable Broadcom iProc mailbox controller

[Jonathan Richardson]

Add node for iProc mailbox controller and enable. Proposed changes to
add interrupt support to the mailbox controller and gpio crmu driver
were prematurely merged into the mainline. Those changes are reverted in
this commit.

Signed-off-by: Jonathan Richardson <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 arch/arm/boot/dts/bcm-cygnus.dtsi | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
index 8833a4c..606e11a 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -123,10 +123,7 @@
 
 		mailbox: mailbox@03024024 {
 			compatible = "brcm,iproc-mailbox";
-			reg = <0x03024024 0x40>;
-			interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
-			#interrupt-cells = <1>;
-			interrupt-controller;
+			reg = <0x03024024 0x8>;
 			#mbox-cells = <1>;
 		};
 
@@ -137,9 +134,6 @@
 			ngpios = <6>;
 			#gpio-cells = <2>;
 			gpio-controller;
-			interrupt-controller;
-			interrupt-parent = <&mailbox>;
-			interrupts = <0>;
 		};
 
 		i2c0: i2c@18008000 {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 3/3] ARM: dts: Enable Broadcom iProc mailbox controller

[Jonathan Richardson]

Add node for iProc mailbox controller and enable. Proposed changes to
add interrupt support to the mailbox controller and gpio crmu driver
were prematurely merged into the mainline. Those changes are reverted in
this commit.

Signed-off-by: Jonathan Richardson <jonathan.richardson@broadcom.com>
---
 arch/arm/boot/dts/bcm-cygnus.dtsi | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
index 8833a4c..606e11a 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -123,10 +123,7 @@
 
 		mailbox: mailbox at 03024024 {
 			compatible = "brcm,iproc-mailbox";
-			reg = <0x03024024 0x40>;
-			interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
-			#interrupt-cells = <1>;
-			interrupt-controller;
+			reg = <0x03024024 0x8>;
 			#mbox-cells = <1>;
 		};
 
@@ -137,9 +134,6 @@
 			ngpios = <6>;
 			#gpio-cells = <2>;
 			gpio-controller;
-			interrupt-controller;
-			interrupt-parent = <&mailbox>;
-			interrupts = <0>;
 		};
 
 		i2c0: i2c at 18008000 {
-- 
1.9.1

Re: [PATCH v4 1/3] dt-bindings: Document Broadcom iProc mailbox controller driver

[Rob Herring]

On Thu, Jan 26, 2017 at 12:38:00PM -0800, Jonathan Richardson wrote:
> Add binding document for brcm,iproc-mailbox.
> 
> Reviewed-by: Jonathan Richardson <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Tested-by: Jonathan Richardson <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Vikram Prakash <vikram.prakash-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Ray Jui <ray.jui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Shreesha Rajashekar <shreesha.rajashekar-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Jonathan Richardson <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> ---
>  .../devicetree/bindings/mailbox/brcm,iproc-mailbox.txt | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/3] dt-bindings: Document Broadcom iProc mailbox controller driver

[Rob Herring]

On Thu, Jan 26, 2017 at 12:38:00PM -0800, Jonathan Richardson wrote:
> Add binding document for brcm,iproc-mailbox.
> 
> Reviewed-by: Jonathan Richardson <jonathan.richardson@broadcom.com>
> Tested-by: Jonathan Richardson <jonathan.richardson@broadcom.com>
> Reviewed-by: Vikram Prakash <vikram.prakash@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Shreesha Rajashekar <shreesha.rajashekar@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> Signed-off-by: Jonathan Richardson <jonathan.richardson@broadcom.com>
> ---
>  .../devicetree/bindings/mailbox/brcm,iproc-mailbox.txt | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 0/3] Add support for Broadcom iProc mailbox controller

[Jonathan Richardson]

Pinging maintainer. Jassi, have you had a chance to look at this yet? Thanks.

On 17-01-26 12:37 PM, Jonathan Richardson wrote:
> This patch set contains mailbox support for Broadcom iProc based SoC's. The
> mailbox controller handles all communication with a Cortex-M0 MCU processor that
> provides support for power, clock, and reset management. The mailbox controller
> for Cygnus is enabled in DT.
>
> Changes from v3:
>  - DT changes merged into one commit.
>
> Changes from v2:
>  - Fixed binding to remove leading 0 (Rob's comment). Also added more detail as
>    suggested by Jassi.
>  - Added changelog to dt binding and enable dts commits.
>  - Changed year in copyright in bcm_iproc_mailbox.h.
>  - Changes to bcm-cygnus.dtsi were prematurely merged into the mainline after
>    Rob acked the DT changes but Jassi suggested changes to the mailbox driver.
>    The result was that the mailbox controller node and changes to the crmu gpio
>    driver to enable interrupt handling were incorrectly added to the dtsi. A
>    commit is included in this patchset to revert those changes. Enabling
>    interrupt handling in the the crmu gpio driver will be introduced when the
>    interrupt controller driver is added to provide mailbox interrupt support.
>
> Jonathan Richardson (3):
>   dt-bindings: Document Broadcom iProc mailbox controller driver
>   mailbox: Add iProc mailbox controller driver
>   ARM: dts: Enable Broadcom iProc mailbox controller
>
>  .../bindings/mailbox/brcm,iproc-mailbox.txt        |  18 ++
>  arch/arm/boot/dts/bcm-cygnus.dtsi                  |   8 +-
>  drivers/mailbox/Kconfig                            |  10 ++
>  drivers/mailbox/Makefile                           |   2 +
>  drivers/mailbox/bcm-iproc-mailbox.c                | 199 +++++++++++++++++++++
>  include/linux/mailbox/bcm_iproc_mailbox.h          |  32 ++++
>  6 files changed, 262 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt
>  create mode 100644 drivers/mailbox/bcm-iproc-mailbox.c
>  create mode 100644 include/linux/mailbox/bcm_iproc_mailbox.h
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 0/3] Add support for Broadcom iProc mailbox controller

[Jonathan Richardson]

Pinging maintainer. Jassi, have you had a chance to look at this yet? Thanks.

On 17-01-26 12:37 PM, Jonathan Richardson wrote:
> This patch set contains mailbox support for Broadcom iProc based SoC's. The
> mailbox controller handles all communication with a Cortex-M0 MCU processor that
> provides support for power, clock, and reset management. The mailbox controller
> for Cygnus is enabled in DT.
>
> Changes from v3:
>  - DT changes merged into one commit.
>
> Changes from v2:
>  - Fixed binding to remove leading 0 (Rob's comment). Also added more detail as
>    suggested by Jassi.
>  - Added changelog to dt binding and enable dts commits.
>  - Changed year in copyright in bcm_iproc_mailbox.h.
>  - Changes to bcm-cygnus.dtsi were prematurely merged into the mainline after
>    Rob acked the DT changes but Jassi suggested changes to the mailbox driver.
>    The result was that the mailbox controller node and changes to the crmu gpio
>    driver to enable interrupt handling were incorrectly added to the dtsi. A
>    commit is included in this patchset to revert those changes. Enabling
>    interrupt handling in the the crmu gpio driver will be introduced when the
>    interrupt controller driver is added to provide mailbox interrupt support.
>
> Jonathan Richardson (3):
>   dt-bindings: Document Broadcom iProc mailbox controller driver
>   mailbox: Add iProc mailbox controller driver
>   ARM: dts: Enable Broadcom iProc mailbox controller
>
>  .../bindings/mailbox/brcm,iproc-mailbox.txt        |  18 ++
>  arch/arm/boot/dts/bcm-cygnus.dtsi                  |   8 +-
>  drivers/mailbox/Kconfig                            |  10 ++
>  drivers/mailbox/Makefile                           |   2 +
>  drivers/mailbox/bcm-iproc-mailbox.c                | 199 +++++++++++++++++++++
>  include/linux/mailbox/bcm_iproc_mailbox.h          |  32 ++++
>  6 files changed, 262 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt
>  create mode 100644 drivers/mailbox/bcm-iproc-mailbox.c
>  create mode 100644 include/linux/mailbox/bcm_iproc_mailbox.h
>

Re: [PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jassi Brar]

On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
<jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:

> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
> +{
> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
> +               unsigned long flags;
> +       int err = 0;
> +       const int poll_period_us = 5;
> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
> +
> +       if (!msg)
> +               return -EINVAL;
> +
> +       spin_lock_irqsave(&mbox->lock, flags);
> +
> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
> +               msg->cmd, msg->param, msg->wait_ack);
> +
prints should be outside the spinlocks.

> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
> +
> +       if (msg->wait_ack) {
> +               int retries;
> +
move poll_period_us and max_retries in here or just define' them

> +               err = msg->reply_code = -ETIMEDOUT;
> +               for (retries = 0; retries < max_retries; retries++) {
> +                       u32 val = readl(
> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
> +                       if (val & M0_IPC_CMD_DONE_MASK) {
> +                               /*
> +                                * M0 replied - save reply code and
> +                                * clear error.
> +                                */
> +                               msg->reply_code = (val &
> +                                       M0_IPC_CMD_REPLY_MASK) >>
> +                                       M0_IPC_CMD_REPLY_SHIFT;
> +                               err = 0;
> +                               break;
> +                       }
> +                       udelay(poll_period_us);
>
potentially 2ms inside spin_lock_irqsave. Alternative is to implement
a simple 'peek_data' and call it for requests with 'wait_ack'
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jassi Brar]

On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
<jonathan.richardson@broadcom.com> wrote:

> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
> +{
> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
> +               unsigned long flags;
> +       int err = 0;
> +       const int poll_period_us = 5;
> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
> +
> +       if (!msg)
> +               return -EINVAL;
> +
> +       spin_lock_irqsave(&mbox->lock, flags);
> +
> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
> +               msg->cmd, msg->param, msg->wait_ack);
> +
prints should be outside the spinlocks.

> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
> +
> +       if (msg->wait_ack) {
> +               int retries;
> +
move poll_period_us and max_retries in here or just define' them

> +               err = msg->reply_code = -ETIMEDOUT;
> +               for (retries = 0; retries < max_retries; retries++) {
> +                       u32 val = readl(
> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
> +                       if (val & M0_IPC_CMD_DONE_MASK) {
> +                               /*
> +                                * M0 replied - save reply code and
> +                                * clear error.
> +                                */
> +                               msg->reply_code = (val &
> +                                       M0_IPC_CMD_REPLY_MASK) >>
> +                                       M0_IPC_CMD_REPLY_SHIFT;
> +                               err = 0;
> +                               break;
> +                       }
> +                       udelay(poll_period_us);
>
potentially 2ms inside spin_lock_irqsave. Alternative is to implement
a simple 'peek_data' and call it for requests with 'wait_ack'

Re: [PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

On 17-02-16 10:20 PM, Jassi Brar wrote:
> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
> <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>
>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>> +{
>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>> +               unsigned long flags;
>> +       int err = 0;
>> +       const int poll_period_us = 5;
>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>> +
>> +       if (!msg)
>> +               return -EINVAL;
>> +
>> +       spin_lock_irqsave(&mbox->lock, flags);
>> +
>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>> +               msg->cmd, msg->param, msg->wait_ack);
>> +
> prints should be outside the spinlocks.
Will fix.
>
>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>> +
>> +       if (msg->wait_ack) {
>> +               int retries;
>> +
> move poll_period_us and max_retries in here or just define' them
Will fix.
>
>> +               err = msg->reply_code = -ETIMEDOUT;
>> +               for (retries = 0; retries < max_retries; retries++) {
>> +                       u32 val = readl(
>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>> +                               /*
>> +                                * M0 replied - save reply code and
>> +                                * clear error.
>> +                                */
>> +                               msg->reply_code = (val &
>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>> +                               err = 0;
>> +                               break;
>> +                       }
>> +                       udelay(poll_period_us);
>>
> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
> a simple 'peek_data' and call it for requests with 'wait_ack'
The M0 responds in very few microseconds. The 2ms is an arbitrary value that came from the clients tx_tout being specified in ms. I can reduce the timeout to something smaller. No need to peek for data.

Thanks,
Jon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

On 17-02-16 10:20 PM, Jassi Brar wrote:
> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
> <jonathan.richardson@broadcom.com> wrote:
>
>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>> +{
>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>> +               unsigned long flags;
>> +       int err = 0;
>> +       const int poll_period_us = 5;
>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>> +
>> +       if (!msg)
>> +               return -EINVAL;
>> +
>> +       spin_lock_irqsave(&mbox->lock, flags);
>> +
>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>> +               msg->cmd, msg->param, msg->wait_ack);
>> +
> prints should be outside the spinlocks.
Will fix.
>
>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>> +
>> +       if (msg->wait_ack) {
>> +               int retries;
>> +
> move poll_period_us and max_retries in here or just define' them
Will fix.
>
>> +               err = msg->reply_code = -ETIMEDOUT;
>> +               for (retries = 0; retries < max_retries; retries++) {
>> +                       u32 val = readl(
>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>> +                               /*
>> +                                * M0 replied - save reply code and
>> +                                * clear error.
>> +                                */
>> +                               msg->reply_code = (val &
>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>> +                               err = 0;
>> +                               break;
>> +                       }
>> +                       udelay(poll_period_us);
>>
> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
> a simple 'peek_data' and call it for requests with 'wait_ack'
The M0 responds in very few microseconds. The 2ms is an arbitrary value that came from the clients tx_tout being specified in ms. I can reduce the timeout to something smaller. No need to peek for data.

Thanks,
Jon

Re: [PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

On 17-02-16 10:20 PM, Jassi Brar wrote:
> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
> <jonathan.richardson@broadcom.com> wrote:
>
>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>> +{
>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>> +               unsigned long flags;
>> +       int err = 0;
>> +       const int poll_period_us = 5;
>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>> +
>> +       if (!msg)
>> +               return -EINVAL;
>> +
>> +       spin_lock_irqsave(&mbox->lock, flags);
>> +
>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>> +               msg->cmd, msg->param, msg->wait_ack);
>> +
> prints should be outside the spinlocks.
>
>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>> +
>> +       if (msg->wait_ack) {
>> +               int retries;
>> +
> move poll_period_us and max_retries in here or just define' them
>
>> +               err = msg->reply_code = -ETIMEDOUT;
>> +               for (retries = 0; retries < max_retries; retries++) {
>> +                       u32 val = readl(
>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>> +                               /*
>> +                                * M0 replied - save reply code and
>> +                                * clear error.
>> +                                */
>> +                               msg->reply_code = (val &
>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>> +                               err = 0;
>> +                               break;
>> +                       }
>> +                       udelay(poll_period_us);
>>
> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
> a simple 'peek_data' and call it for requests with 'wait_ack'
Hi Jassi. The M0 response is normally 25-30 us. We have one message that takes 280us but we can get rid of it if necessary. Regarding your suggestion of peek_data. I don't see how it can be used without the spinlock to serialize multiple clients/channels over a single mailbox channel. peek_data is going to allow the client to poll for data. But the spinlock is there to prevent other clients from accessing the mailbox channel until any pending transaction is complete. last_tx_done looks like an option but even that will not prevent another client from clobbering a pending transaction. A shared channel among all clients with a blocking model would probably work, but not pretty. Let me know what you advise. Thanks.

[PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

On 17-02-16 10:20 PM, Jassi Brar wrote:
> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
> <jonathan.richardson@broadcom.com> wrote:
>
>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>> +{
>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>> +               unsigned long flags;
>> +       int err = 0;
>> +       const int poll_period_us = 5;
>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>> +
>> +       if (!msg)
>> +               return -EINVAL;
>> +
>> +       spin_lock_irqsave(&mbox->lock, flags);
>> +
>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>> +               msg->cmd, msg->param, msg->wait_ack);
>> +
> prints should be outside the spinlocks.
>
>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>> +
>> +       if (msg->wait_ack) {
>> +               int retries;
>> +
> move poll_period_us and max_retries in here or just define' them
>
>> +               err = msg->reply_code = -ETIMEDOUT;
>> +               for (retries = 0; retries < max_retries; retries++) {
>> +                       u32 val = readl(
>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>> +                               /*
>> +                                * M0 replied - save reply code and
>> +                                * clear error.
>> +                                */
>> +                               msg->reply_code = (val &
>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>> +                               err = 0;
>> +                               break;
>> +                       }
>> +                       udelay(poll_period_us);
>>
> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
> a simple 'peek_data' and call it for requests with 'wait_ack'
Hi Jassi. The M0 response is normally 25-30 us. We have one message that takes 280us but we can get rid of it if necessary. Regarding your suggestion of peek_data. I don't see how it can be used without the spinlock to serialize multiple clients/channels over a single mailbox channel. peek_data is going to allow the client to poll for data. But the spinlock is there to prevent other clients from accessing the mailbox channel until any pending transaction is complete. last_tx_done looks like an option but even that will not prevent another client from clobbering a pending transaction. A shared channel among all clients with a blocking model would probably work, but not pretty. Let me know what you advise. Thanks.

Re: [PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jassi Brar]

On Fri, Feb 24, 2017 at 12:29 AM, Jonathan Richardson
<jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>
>
> On 17-02-16 10:20 PM, Jassi Brar wrote:
>> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
>> <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>
>>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>>> +{
>>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>>> +               unsigned long flags;
>>> +       int err = 0;
>>> +       const int poll_period_us = 5;
>>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>>> +
>>> +       if (!msg)
>>> +               return -EINVAL;
>>> +
>>> +       spin_lock_irqsave(&mbox->lock, flags);
>>> +
>>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>>> +               msg->cmd, msg->param, msg->wait_ack);
>>> +
>> prints should be outside the spinlocks.
>>
>>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>>> +
>>> +       if (msg->wait_ack) {
>>> +               int retries;
>>> +
>> move poll_period_us and max_retries in here or just define' them
>>
>>> +               err = msg->reply_code = -ETIMEDOUT;
>>> +               for (retries = 0; retries < max_retries; retries++) {
>>> +                       u32 val = readl(
>>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>>> +                               /*
>>> +                                * M0 replied - save reply code and
>>> +                                * clear error.
>>> +                                */
>>> +                               msg->reply_code = (val &
>>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>>> +                               err = 0;
>>> +                               break;
>>> +                       }
>>> +                       udelay(poll_period_us);
>>>
>> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
>> a simple 'peek_data' and call it for requests with 'wait_ack'
> Hi Jassi. The M0 response is normally 25-30 us.
>
You hardcode the behaviour of your protocol in the controller driver.
What if your next platform/protocol has commands that the remote/M0
takes upto 10ms to respond (because they are not critical/trivial)?

If you don't have some h/w indicator (like irq or bit-flag) for
tx-done and data-rx , you have to use ack-by-client and peek method.
Commands that don't get a response will be immediately followed by
mbox_client_txdone(), while others would need to call peek_data() to
poll for data-rx. Please note, if you implement the tx_prepare
callback, you will know when to start peeking for incoming data.

> We have one message that takes 280us but we can get rid of it if necessary.
>
No. Please don't kill some needed feature just to make the driver simpler.

> Regarding your suggestion of peek_data. I don't see how it can be used
> without the spinlock to serialize multiple clients/channels over a single
> mailbox channel. peek_data is going to allow the client to poll for data.
> But the spinlock is there to prevent other clients from accessing the
> mailbox channel until any pending transaction is complete. last_tx_done
> looks like an option but even that will not prevent another client from
> clobbering a pending transaction. A shared channel among all clients
> with a blocking model would probably work, but not pretty.
>
Mailbox api provides exclusive access to its clients, just like dma-engine.
Please have a look at how other platforms do it.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jassi Brar]

On Fri, Feb 24, 2017 at 12:29 AM, Jonathan Richardson
<jonathan.richardson@broadcom.com> wrote:
>
>
> On 17-02-16 10:20 PM, Jassi Brar wrote:
>> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
>> <jonathan.richardson@broadcom.com> wrote:
>>
>>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>>> +{
>>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>>> +               unsigned long flags;
>>> +       int err = 0;
>>> +       const int poll_period_us = 5;
>>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>>> +
>>> +       if (!msg)
>>> +               return -EINVAL;
>>> +
>>> +       spin_lock_irqsave(&mbox->lock, flags);
>>> +
>>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>>> +               msg->cmd, msg->param, msg->wait_ack);
>>> +
>> prints should be outside the spinlocks.
>>
>>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>>> +
>>> +       if (msg->wait_ack) {
>>> +               int retries;
>>> +
>> move poll_period_us and max_retries in here or just define' them
>>
>>> +               err = msg->reply_code = -ETIMEDOUT;
>>> +               for (retries = 0; retries < max_retries; retries++) {
>>> +                       u32 val = readl(
>>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>>> +                               /*
>>> +                                * M0 replied - save reply code and
>>> +                                * clear error.
>>> +                                */
>>> +                               msg->reply_code = (val &
>>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>>> +                               err = 0;
>>> +                               break;
>>> +                       }
>>> +                       udelay(poll_period_us);
>>>
>> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
>> a simple 'peek_data' and call it for requests with 'wait_ack'
> Hi Jassi. The M0 response is normally 25-30 us.
>
You hardcode the behaviour of your protocol in the controller driver.
What if your next platform/protocol has commands that the remote/M0
takes upto 10ms to respond (because they are not critical/trivial)?

If you don't have some h/w indicator (like irq or bit-flag) for
tx-done and data-rx , you have to use ack-by-client and peek method.
Commands that don't get a response will be immediately followed by
mbox_client_txdone(), while others would need to call peek_data() to
poll for data-rx. Please note, if you implement the tx_prepare
callback, you will know when to start peeking for incoming data.

> We have one message that takes 280us but we can get rid of it if necessary.
>
No. Please don't kill some needed feature just to make the driver simpler.

> Regarding your suggestion of peek_data. I don't see how it can be used
> without the spinlock to serialize multiple clients/channels over a single
> mailbox channel. peek_data is going to allow the client to poll for data.
> But the spinlock is there to prevent other clients from accessing the
> mailbox channel until any pending transaction is complete. last_tx_done
> looks like an option but even that will not prevent another client from
> clobbering a pending transaction. A shared channel among all clients
> with a blocking model would probably work, but not pretty.
>
Mailbox api provides exclusive access to its clients, just like dma-engine.
Please have a look at how other platforms do it.

Thanks.

Re: [PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

On 17-02-23 09:00 PM, Jassi Brar wrote:
> On Fri, Feb 24, 2017 at 12:29 AM, Jonathan Richardson
> <jonathan.richardson@broadcom.com> wrote:
>>
>> On 17-02-16 10:20 PM, Jassi Brar wrote:
>>> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
>>> <jonathan.richardson@broadcom.com> wrote:
>>>
>>>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>>>> +{
>>>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>>>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>>>> +               unsigned long flags;
>>>> +       int err = 0;
>>>> +       const int poll_period_us = 5;
>>>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>>>> +
>>>> +       if (!msg)
>>>> +               return -EINVAL;
>>>> +
>>>> +       spin_lock_irqsave(&mbox->lock, flags);
>>>> +
>>>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>>>> +               msg->cmd, msg->param, msg->wait_ack);
>>>> +
>>> prints should be outside the spinlocks.
>>>
>>>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>>>> +
>>>> +       if (msg->wait_ack) {
>>>> +               int retries;
>>>> +
>>> move poll_period_us and max_retries in here or just define' them
>>>
>>>> +               err = msg->reply_code = -ETIMEDOUT;
>>>> +               for (retries = 0; retries < max_retries; retries++) {
>>>> +                       u32 val = readl(
>>>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>>>> +                               /*
>>>> +                                * M0 replied - save reply code and
>>>> +                                * clear error.
>>>> +                                */
>>>> +                               msg->reply_code = (val &
>>>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>>>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>>>> +                               err = 0;
>>>> +                               break;
>>>> +                       }
>>>> +                       udelay(poll_period_us);
>>>>
>>> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
>>> a simple 'peek_data' and call it for requests with 'wait_ack'
>> Hi Jassi. The M0 response is normally 25-30 us.
>>
> You hardcode the behaviour of your protocol in the controller driver.
> What if your next platform/protocol has commands that the remote/M0
> takes upto 10ms to respond (because they are not critical/trivial)?
>
> If you don't have some h/w indicator (like irq or bit-flag) for
> tx-done and data-rx , you have to use ack-by-client and peek method.
There isn't any functionality not in the driver. We write the message to two registers and poll another to know when it's complete. Nothing can touch the registers again until the operation is complete. We can't even pass data back to the controller. We pass pointers to the M0 and it can write to the memory. The only data we sent back to the client is that status code from the polled register. When complete, data will already be written into the clients memory from the M0.
> Commands that don't get a response will be immediately followed by
> mbox_client_txdone(), while others would need to call peek_data() to
> poll for data-rx. Please note, if you implement the tx_prepare
> callback, you will know when to start peeking for incoming data.
The driver will need to block access to the registers if we remove the spinlock. If a client sends a message that doesn't get a response and another client has already sent a message that does which is pending, it still needs to be blocked.

The framework queues messages on a per channel basis. We have several clients for various drivers each with their own mbox channel. send_data in the controller could return EBUSY if the channel is being used by another channel (ie- transaction pending). If channel A sends a message, then client B sends one before A is complete, the controller's send_data must return an error. The message remains in the queue until tx_tick is called to re-submit it again. Client A polls the controller until complete then calls mbox_client_txdone. Client B can poll the controller but first needs a way of submitting the queued message again (via tx_tick->msg_submit). Using the ACK model I see no way client B can know when to start polling (ie- when client A's message was completed) or even kick off the msg_su
 bmit again. The submitting of messages from the queue to controller's send_data relies on knowing when sending a prior message on the channel was complete. It doesn't know when another
channel's message has been sent. The only way I can see this working is using one channel shared among clients. We don't want to add any additional queuing of messages in the controller when the framework already does it (per channel). Hope this makes sense. Thanks.

>> We have one message that takes 280us but we can get rid of it if necessary.
>>
> No. Please don't kill some needed feature just to make the driver simpler.
>
>> Regarding your suggestion of peek_data. I don't see how it can be used
>> without the spinlock to serialize multiple clients/channels over a single
>> mailbox channel. peek_data is going to allow the client to poll for data.
>> But the spinlock is there to prevent other clients from accessing the
>> mailbox channel until any pending transaction is complete. last_tx_done
>> looks like an option but even that will not prevent another client from
>> clobbering a pending transaction. A shared channel among all clients
>> with a blocking model would probably work, but not pretty.
>>
> Mailbox api provides exclusive access to its clients, just like dma-engine.
> Please have a look at how other platforms do it.

> Thanks.

[PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

On 17-02-23 09:00 PM, Jassi Brar wrote:
> On Fri, Feb 24, 2017 at 12:29 AM, Jonathan Richardson
> <jonathan.richardson@broadcom.com> wrote:
>>
>> On 17-02-16 10:20 PM, Jassi Brar wrote:
>>> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
>>> <jonathan.richardson@broadcom.com> wrote:
>>>
>>>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>>>> +{
>>>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>>>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>>>> +               unsigned long flags;
>>>> +       int err = 0;
>>>> +       const int poll_period_us = 5;
>>>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>>>> +
>>>> +       if (!msg)
>>>> +               return -EINVAL;
>>>> +
>>>> +       spin_lock_irqsave(&mbox->lock, flags);
>>>> +
>>>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>>>> +               msg->cmd, msg->param, msg->wait_ack);
>>>> +
>>> prints should be outside the spinlocks.
>>>
>>>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>>>> +
>>>> +       if (msg->wait_ack) {
>>>> +               int retries;
>>>> +
>>> move poll_period_us and max_retries in here or just define' them
>>>
>>>> +               err = msg->reply_code = -ETIMEDOUT;
>>>> +               for (retries = 0; retries < max_retries; retries++) {
>>>> +                       u32 val = readl(
>>>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>>>> +                               /*
>>>> +                                * M0 replied - save reply code and
>>>> +                                * clear error.
>>>> +                                */
>>>> +                               msg->reply_code = (val &
>>>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>>>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>>>> +                               err = 0;
>>>> +                               break;
>>>> +                       }
>>>> +                       udelay(poll_period_us);
>>>>
>>> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
>>> a simple 'peek_data' and call it for requests with 'wait_ack'
>> Hi Jassi. The M0 response is normally 25-30 us.
>>
> You hardcode the behaviour of your protocol in the controller driver.
> What if your next platform/protocol has commands that the remote/M0
> takes upto 10ms to respond (because they are not critical/trivial)?
>
> If you don't have some h/w indicator (like irq or bit-flag) for
> tx-done and data-rx , you have to use ack-by-client and peek method.
There isn't any functionality not in the driver. We write the message to two registers and poll another to know when it's complete. Nothing can touch the registers again until the operation is complete. We can't even pass data back to the controller. We pass pointers to the M0 and it can write to the memory. The only data we sent back to the client is that status code from the polled register. When complete, data will already be written into the clients memory from the M0.
> Commands that don't get a response will be immediately followed by
> mbox_client_txdone(), while others would need to call peek_data() to
> poll for data-rx. Please note, if you implement the tx_prepare
> callback, you will know when to start peeking for incoming data.
The driver will need to block access to the registers if we remove the spinlock. If a client sends a message that doesn't get a response and another client has already sent a message that does which is pending, it still needs to be blocked.

The framework queues messages on a per channel basis. We have several clients for various drivers each with their own mbox channel. send_data in the controller could return EBUSY if the channel is being used by another channel (ie- transaction pending). If channel A sends a message, then client B sends one before A is complete, the controller's send_data must return an error. The message remains in the queue until tx_tick is called to re-submit it again. Client A polls the controller until complete then calls mbox_client_txdone. Client B can poll the controller but first needs a way of submitting the queued message again (via tx_tick->msg_submit). Using the ACK model I see no way client B can know when to start polling (ie- when client A's message was completed) or even kick off the msg_submit again. The submitting of messages from the queue to controller's send_data relies on knowing when sending a prior message on the channel was complete. It doesn't know when another
channel's message has been sent. The only way I can see this working is using one channel shared among clients. We don't want to add any additional queuing of messages in the controller when the framework already does it (per channel). Hope this makes sense. Thanks.

>> We have one message that takes 280us but we can get rid of it if necessary.
>>
> No. Please don't kill some needed feature just to make the driver simpler.
>
>> Regarding your suggestion of peek_data. I don't see how it can be used
>> without the spinlock to serialize multiple clients/channels over a single
>> mailbox channel. peek_data is going to allow the client to poll for data.
>> But the spinlock is there to prevent other clients from accessing the
>> mailbox channel until any pending transaction is complete. last_tx_done
>> looks like an option but even that will not prevent another client from
>> clobbering a pending transaction. A shared channel among all clients
>> with a blocking model would probably work, but not pretty.
>>
> Mailbox api provides exclusive access to its clients, just like dma-engine.
> Please have a look at how other platforms do it.

> Thanks.

Re: [PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

On 17-03-02 01:03 PM, Jonathan Richardson wrote:
>
> On 17-02-23 09:00 PM, Jassi Brar wrote:
>> On Fri, Feb 24, 2017 at 12:29 AM, Jonathan Richardson
>> <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>> On 17-02-16 10:20 PM, Jassi Brar wrote:
>>>> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
>>>> <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>>
>>>>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>>>>> +{
>>>>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>>>>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>>>>> +               unsigned long flags;
>>>>> +       int err = 0;
>>>>> +       const int poll_period_us = 5;
>>>>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>>>>> +
>>>>> +       if (!msg)
>>>>> +               return -EINVAL;
>>>>> +
>>>>> +       spin_lock_irqsave(&mbox->lock, flags);
>>>>> +
>>>>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>>>>> +               msg->cmd, msg->param, msg->wait_ack);
>>>>> +
>>>> prints should be outside the spinlocks.
>>>>
>>>>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>>>>> +
>>>>> +       if (msg->wait_ack) {
>>>>> +               int retries;
>>>>> +
>>>> move poll_period_us and max_retries in here or just define' them
>>>>
>>>>> +               err = msg->reply_code = -ETIMEDOUT;
>>>>> +               for (retries = 0; retries < max_retries; retries++) {
>>>>> +                       u32 val = readl(
>>>>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>>>>> +                               /*
>>>>> +                                * M0 replied - save reply code and
>>>>> +                                * clear error.
>>>>> +                                */
>>>>> +                               msg->reply_code = (val &
>>>>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>>>>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>>>>> +                               err = 0;
>>>>> +                               break;
>>>>> +                       }
>>>>> +                       udelay(poll_period_us);
>>>>>
>>>> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
>>>> a simple 'peek_data' and call it for requests with 'wait_ack'
>>> Hi Jassi. The M0 response is normally 25-30 us.
>>>
>> You hardcode the behaviour of your protocol in the controller driver.
>> What if your next platform/protocol has commands that the remote/M0
>> takes upto 10ms to respond (because they are not critical/trivial)?
>>
>> If you don't have some h/w indicator (like irq or bit-flag) for
>> tx-done and data-rx , you have to use ack-by-client and peek method.
> There isn't any functionality not in the driver. We write the message to two registers and poll another to know when it's complete. Nothing can touch the registers again until the operation is complete. We can't even pass data back to the controller. We pass pointers to the M0 and it can write to the memory. The only data we sent back to the client is that status code from the polled register. When complete, data will already be written into the clients memory from the M0.
>> Commands that don't get a response will be immediately followed by
>> mbox_client_txdone(), while others would need to call peek_data() to
>> poll for data-rx. Please note, if you implement the tx_prepare
>> callback, you will know when to start peeking for incoming data.
> The driver will need to block access to the registers if we remove the spinlock. If a client sends a message that doesn't get a response and another client has already sent a message that does which is pending, it still needs to be blocked.
>
> The framework queues messages on a per channel basis. We have several clients for various drivers each with their own mbox channel. send_data in the controller could return EBUSY if the channel is being used by another channel (ie- transaction pending). If channel A sends a message, then client B sends one before A is complete, the controller's send_data must return an error. The message remains in the queue until tx_tick is called to re-submit it again. Client A polls the controller until complete then calls mbox_client_txdone. Client B can poll the controller but first needs a way of submitting the queued message again (via tx_tick->msg_submit). Using the ACK model I see no way client B can know when to start polling (ie- when client A's message was completed) or even kick off the msg_
 submit again. The submitting of messages from the queue to controller's send_data relies on knowing when sending a prior message on the channel was complete. It doesn't know when another
> channel's message has been sent. The only way I can see this working is using one channel shared among clients. We don't want to add any additional queuing of messages in the controller when the framework already does it (per channel). Hope this makes sense. Thanks.
Jassi, any further comment on this? We can leave the driver as is or agree on a way to remove the spinlock. Either works for me.

To give you an idea when we're using the mailbox driver: to reset, to unmask an aon gpio interrupt from the crmu interrupt handler (that was removed from the mailbox driver), to set aon gpio's as a wake source from the pinctrl gpio driver, and to read a persistent clock from the M0 on suspend.

Please let me know so we can proceed.
>>> We have one message that takes 280us but we can get rid of it if necessary.
>>>
>> No. Please don't kill some needed feature just to make the driver simpler.
>>
>>> Regarding your suggestion of peek_data. I don't see how it can be used
>>> without the spinlock to serialize multiple clients/channels over a single
>>> mailbox channel. peek_data is going to allow the client to poll for data.
>>> But the spinlock is there to prevent other clients from accessing the
>>> mailbox channel until any pending transaction is complete. last_tx_done
>>> looks like an option but even that will not prevent another client from
>>> clobbering a pending transaction. A shared channel among all clients
>>> with a blocking model would probably work, but not pretty.
>>>
>> Mailbox api provides exclusive access to its clients, just like dma-engine.
>> Please have a look at how other platforms do it.
>> Thanks.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

On 17-03-02 01:03 PM, Jonathan Richardson wrote:
>
> On 17-02-23 09:00 PM, Jassi Brar wrote:
>> On Fri, Feb 24, 2017 at 12:29 AM, Jonathan Richardson
>> <jonathan.richardson@broadcom.com> wrote:
>>> On 17-02-16 10:20 PM, Jassi Brar wrote:
>>>> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
>>>> <jonathan.richardson@broadcom.com> wrote:
>>>>
>>>>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>>>>> +{
>>>>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>>>>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>>>>> +               unsigned long flags;
>>>>> +       int err = 0;
>>>>> +       const int poll_period_us = 5;
>>>>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>>>>> +
>>>>> +       if (!msg)
>>>>> +               return -EINVAL;
>>>>> +
>>>>> +       spin_lock_irqsave(&mbox->lock, flags);
>>>>> +
>>>>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>>>>> +               msg->cmd, msg->param, msg->wait_ack);
>>>>> +
>>>> prints should be outside the spinlocks.
>>>>
>>>>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>>>>> +
>>>>> +       if (msg->wait_ack) {
>>>>> +               int retries;
>>>>> +
>>>> move poll_period_us and max_retries in here or just define' them
>>>>
>>>>> +               err = msg->reply_code = -ETIMEDOUT;
>>>>> +               for (retries = 0; retries < max_retries; retries++) {
>>>>> +                       u32 val = readl(
>>>>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>>>>> +                               /*
>>>>> +                                * M0 replied - save reply code and
>>>>> +                                * clear error.
>>>>> +                                */
>>>>> +                               msg->reply_code = (val &
>>>>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>>>>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>>>>> +                               err = 0;
>>>>> +                               break;
>>>>> +                       }
>>>>> +                       udelay(poll_period_us);
>>>>>
>>>> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
>>>> a simple 'peek_data' and call it for requests with 'wait_ack'
>>> Hi Jassi. The M0 response is normally 25-30 us.
>>>
>> You hardcode the behaviour of your protocol in the controller driver.
>> What if your next platform/protocol has commands that the remote/M0
>> takes upto 10ms to respond (because they are not critical/trivial)?
>>
>> If you don't have some h/w indicator (like irq or bit-flag) for
>> tx-done and data-rx , you have to use ack-by-client and peek method.
> There isn't any functionality not in the driver. We write the message to two registers and poll another to know when it's complete. Nothing can touch the registers again until the operation is complete. We can't even pass data back to the controller. We pass pointers to the M0 and it can write to the memory. The only data we sent back to the client is that status code from the polled register. When complete, data will already be written into the clients memory from the M0.
>> Commands that don't get a response will be immediately followed by
>> mbox_client_txdone(), while others would need to call peek_data() to
>> poll for data-rx. Please note, if you implement the tx_prepare
>> callback, you will know when to start peeking for incoming data.
> The driver will need to block access to the registers if we remove the spinlock. If a client sends a message that doesn't get a response and another client has already sent a message that does which is pending, it still needs to be blocked.
>
> The framework queues messages on a per channel basis. We have several clients for various drivers each with their own mbox channel. send_data in the controller could return EBUSY if the channel is being used by another channel (ie- transaction pending). If channel A sends a message, then client B sends one before A is complete, the controller's send_data must return an error. The message remains in the queue until tx_tick is called to re-submit it again. Client A polls the controller until complete then calls mbox_client_txdone. Client B can poll the controller but first needs a way of submitting the queued message again (via tx_tick->msg_submit). Using the ACK model I see no way client B can know when to start polling (ie- when client A's message was completed) or even kick off the msg_submit again. The submitting of messages from the queue to controller's send_data relies on knowing when sending a prior message on the channel was complete. It doesn't know when another
> channel's message has been sent. The only way I can see this working is using one channel shared among clients. We don't want to add any additional queuing of messages in the controller when the framework already does it (per channel). Hope this makes sense. Thanks.
Jassi, any further comment on this? We can leave the driver as is or agree on a way to remove the spinlock. Either works for me.

To give you an idea when we're using the mailbox driver: to reset, to unmask an aon gpio interrupt from the crmu interrupt handler (that was removed from the mailbox driver), to set aon gpio's as a wake source from the pinctrl gpio driver, and to read a persistent clock from the M0 on suspend.

Please let me know so we can proceed.
>>> We have one message that takes 280us but we can get rid of it if necessary.
>>>
>> No. Please don't kill some needed feature just to make the driver simpler.
>>
>>> Regarding your suggestion of peek_data. I don't see how it can be used
>>> without the spinlock to serialize multiple clients/channels over a single
>>> mailbox channel. peek_data is going to allow the client to poll for data.
>>> But the spinlock is there to prevent other clients from accessing the
>>> mailbox channel until any pending transaction is complete. last_tx_done
>>> looks like an option but even that will not prevent another client from
>>> clobbering a pending transaction. A shared channel among all clients
>>> with a blocking model would probably work, but not pretty.
>>>
>> Mailbox api provides exclusive access to its clients, just like dma-engine.
>> Please have a look at how other platforms do it.
>> Thanks.

Re: [PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jassi Brar]

On Fri, Mar 3, 2017 at 2:33 AM, Jonathan Richardson
<jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>
>
> On 17-02-23 09:00 PM, Jassi Brar wrote:
>> On Fri, Feb 24, 2017 at 12:29 AM, Jonathan Richardson
>> <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>
>>> On 17-02-16 10:20 PM, Jassi Brar wrote:
>>>> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
>>>> <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>>
>>>>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>>>>> +{
>>>>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>>>>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>>>>> +               unsigned long flags;
>>>>> +       int err = 0;
>>>>> +       const int poll_period_us = 5;
>>>>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>>>>> +
>>>>> +       if (!msg)
>>>>> +               return -EINVAL;
>>>>> +
>>>>> +       spin_lock_irqsave(&mbox->lock, flags);
>>>>> +
>>>>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>>>>> +               msg->cmd, msg->param, msg->wait_ack);
>>>>> +
>>>> prints should be outside the spinlocks.
>>>>
>>>>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>>>>> +
>>>>> +       if (msg->wait_ack) {
>>>>> +               int retries;
>>>>> +
>>>> move poll_period_us and max_retries in here or just define' them
>>>>
>>>>> +               err = msg->reply_code = -ETIMEDOUT;
>>>>> +               for (retries = 0; retries < max_retries; retries++) {
>>>>> +                       u32 val = readl(
>>>>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>>>>> +                               /*
>>>>> +                                * M0 replied - save reply code and
>>>>> +                                * clear error.
>>>>> +                                */
>>>>> +                               msg->reply_code = (val &
>>>>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>>>>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>>>>> +                               err = 0;
>>>>> +                               break;
>>>>> +                       }
>>>>> +                       udelay(poll_period_us);
>>>>>
>>>> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
>>>> a simple 'peek_data' and call it for requests with 'wait_ack'
>>> Hi Jassi. The M0 response is normally 25-30 us.
>>>
>> You hardcode the behaviour of your protocol in the controller driver.
>> What if your next platform/protocol has commands that the remote/M0
>> takes upto 10ms to respond (because they are not critical/trivial)?
>>
You overlooked this concern?

>> If you don't have some h/w indicator (like irq or bit-flag) for
>> tx-done and data-rx , you have to use ack-by-client and peek method.
> There isn't any functionality not in the driver. We write the message to two registers and poll another to know when it's complete. Nothing can touch the registers again until the operation is complete. We can't even pass data back to the controller. We pass pointers to the M0 and it can write to the memory. The only data we sent back to the client is that status code from the polled register. When complete, data will already be written into the clients memory from the M0.
>> Commands that don't get a response will be immediately followed by
>> mbox_client_txdone(), while others would need to call peek_data() to
>> poll for data-rx. Please note, if you implement the tx_prepare
>> callback, you will know when to start peeking for incoming data.
> The driver will need to block access to the registers if we remove the spinlock.
> If a client sends a message that doesn't get a response and another client has
> already sent a message that does which is pending, it still needs to be blocked.
>
> The framework queues messages on a per channel basis. We have several clients for
> various drivers each with their own mbox channel. send_data in the controller could
> return EBUSY if the channel is being used by another channel (ie- transaction pending).
> If channel A sends a message, then client B sends one before A is complete, the
> controller's send_data must return an error.
>
Many platforms have clients sharing channels.
The right way to do is have a 'server/owner' client that accepts
requests from various clients and serially queue them to mailbox. That
way you can keep the controller driver free from quirks (like max wait
time of 30us) of your present platform.

thanks
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jassi Brar]

On Fri, Mar 3, 2017 at 2:33 AM, Jonathan Richardson
<jonathan.richardson@broadcom.com> wrote:
>
>
> On 17-02-23 09:00 PM, Jassi Brar wrote:
>> On Fri, Feb 24, 2017 at 12:29 AM, Jonathan Richardson
>> <jonathan.richardson@broadcom.com> wrote:
>>>
>>> On 17-02-16 10:20 PM, Jassi Brar wrote:
>>>> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
>>>> <jonathan.richardson@broadcom.com> wrote:
>>>>
>>>>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>>>>> +{
>>>>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>>>>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>>>>> +               unsigned long flags;
>>>>> +       int err = 0;
>>>>> +       const int poll_period_us = 5;
>>>>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>>>>> +
>>>>> +       if (!msg)
>>>>> +               return -EINVAL;
>>>>> +
>>>>> +       spin_lock_irqsave(&mbox->lock, flags);
>>>>> +
>>>>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>>>>> +               msg->cmd, msg->param, msg->wait_ack);
>>>>> +
>>>> prints should be outside the spinlocks.
>>>>
>>>>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>>>>> +
>>>>> +       if (msg->wait_ack) {
>>>>> +               int retries;
>>>>> +
>>>> move poll_period_us and max_retries in here or just define' them
>>>>
>>>>> +               err = msg->reply_code = -ETIMEDOUT;
>>>>> +               for (retries = 0; retries < max_retries; retries++) {
>>>>> +                       u32 val = readl(
>>>>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>>>>> +                               /*
>>>>> +                                * M0 replied - save reply code and
>>>>> +                                * clear error.
>>>>> +                                */
>>>>> +                               msg->reply_code = (val &
>>>>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>>>>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>>>>> +                               err = 0;
>>>>> +                               break;
>>>>> +                       }
>>>>> +                       udelay(poll_period_us);
>>>>>
>>>> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
>>>> a simple 'peek_data' and call it for requests with 'wait_ack'
>>> Hi Jassi. The M0 response is normally 25-30 us.
>>>
>> You hardcode the behaviour of your protocol in the controller driver.
>> What if your next platform/protocol has commands that the remote/M0
>> takes upto 10ms to respond (because they are not critical/trivial)?
>>
You overlooked this concern?

>> If you don't have some h/w indicator (like irq or bit-flag) for
>> tx-done and data-rx , you have to use ack-by-client and peek method.
> There isn't any functionality not in the driver. We write the message to two registers and poll another to know when it's complete. Nothing can touch the registers again until the operation is complete. We can't even pass data back to the controller. We pass pointers to the M0 and it can write to the memory. The only data we sent back to the client is that status code from the polled register. When complete, data will already be written into the clients memory from the M0.
>> Commands that don't get a response will be immediately followed by
>> mbox_client_txdone(), while others would need to call peek_data() to
>> poll for data-rx. Please note, if you implement the tx_prepare
>> callback, you will know when to start peeking for incoming data.
> The driver will need to block access to the registers if we remove the spinlock.
> If a client sends a message that doesn't get a response and another client has
> already sent a message that does which is pending, it still needs to be blocked.
>
> The framework queues messages on a per channel basis. We have several clients for
> various drivers each with their own mbox channel. send_data in the controller could
> return EBUSY if the channel is being used by another channel (ie- transaction pending).
> If channel A sends a message, then client B sends one before A is complete, the
> controller's send_data must return an error.
>
Many platforms have clients sharing channels.
The right way to do is have a 'server/owner' client that accepts
requests from various clients and serially queue them to mailbox. That
way you can keep the controller driver free from quirks (like max wait
time of 30us) of your present platform.

thanks

Re: [PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

On 17-03-15 10:30 AM, Jassi Brar wrote:
> On Fri, Mar 3, 2017 at 2:33 AM, Jonathan Richardson
> <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>
>> On 17-02-23 09:00 PM, Jassi Brar wrote:
>>> On Fri, Feb 24, 2017 at 12:29 AM, Jonathan Richardson
>>> <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>> On 17-02-16 10:20 PM, Jassi Brar wrote:
>>>>> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
>>>>> <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>>>
>>>>>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>>>>>> +{
>>>>>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>>>>>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>>>>>> +               unsigned long flags;
>>>>>> +       int err = 0;
>>>>>> +       const int poll_period_us = 5;
>>>>>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>>>>>> +
>>>>>> +       if (!msg)
>>>>>> +               return -EINVAL;
>>>>>> +
>>>>>> +       spin_lock_irqsave(&mbox->lock, flags);
>>>>>> +
>>>>>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>>>>>> +               msg->cmd, msg->param, msg->wait_ack);
>>>>>> +
>>>>> prints should be outside the spinlocks.
>>>>>
>>>>>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>>>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>>>>>> +
>>>>>> +       if (msg->wait_ack) {
>>>>>> +               int retries;
>>>>>> +
>>>>> move poll_period_us and max_retries in here or just define' them
>>>>>
>>>>>> +               err = msg->reply_code = -ETIMEDOUT;
>>>>>> +               for (retries = 0; retries < max_retries; retries++) {
>>>>>> +                       u32 val = readl(
>>>>>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>>>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>>>>>> +                               /*
>>>>>> +                                * M0 replied - save reply code and
>>>>>> +                                * clear error.
>>>>>> +                                */
>>>>>> +                               msg->reply_code = (val &
>>>>>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>>>>>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>>>>>> +                               err = 0;
>>>>>> +                               break;
>>>>>> +                       }
>>>>>> +                       udelay(poll_period_us);
>>>>>>
>>>>> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
>>>>> a simple 'peek_data' and call it for requests with 'wait_ack'
>>>> Hi Jassi. The M0 response is normally 25-30 us.
>>>>
>>> You hardcode the behaviour of your protocol in the controller driver.
>>> What if your next platform/protocol has commands that the remote/M0
>>> takes upto 10ms to respond (because they are not critical/trivial)?
>>>
> You overlooked this concern?
>
>>> If you don't have some h/w indicator (like irq or bit-flag) for
>>> tx-done and data-rx , you have to use ack-by-client and peek method.
>> There isn't any functionality not in the driver. We write the message to two registers and poll another to know when it's complete. Nothing can touch the registers again until the operation is complete. We can't even pass data back to the controller. We pass pointers to the M0 and it can write to the memory. The only data we sent back to the client is that status code from the polled register. When complete, data will already be written into the clients memory from the M0.
>>> Commands that don't get a response will be immediately followed by
>>> mbox_client_txdone(), while others would need to call peek_data() to
>>> poll for data-rx. Please note, if you implement the tx_prepare
>>> callback, you will know when to start peeking for incoming data.
>> The driver will need to block access to the registers if we remove the spinlock.
>> If a client sends a message that doesn't get a response and another client has
>> already sent a message that does which is pending, it still needs to be blocked.
>>
>> The framework queues messages on a per channel basis. We have several clients for
>> various drivers each with their own mbox channel. send_data in the controller could
>> return EBUSY if the channel is being used by another channel (ie- transaction pending).
>> If channel A sends a message, then client B sends one before A is complete, the
>> controller's send_data must return an error.
>>
> Many platforms have clients sharing channels.
Hi Jassi. Could you point me to one? I've looked at all of them and don't see any of them sharing channels.
> The right way to do is have a 'server/owner' client that accepts
> requests from various clients and serially queue them to mailbox. That
> way you can keep the controller driver free from quirks (like max wait
> time of 30us) of your present platform.
Do you mean 1 mailbox client with one mailbox channel that all the mbox client drivers share? I thought this would work when I suggested it previously but the client callbacks are necessary in all txdone modes. Client drivers that send the messages need the callbacks and this is only possible with multiple mbox clients. And a channel can only have 1 mbox client. Clients in multiple drivers need the callbacks to either know when to start polling, or be notified when the transaction is complete. It would be nice if multiple clients could use the same channel.

Thanks.
> thanks

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

On 17-03-15 10:30 AM, Jassi Brar wrote:
> On Fri, Mar 3, 2017 at 2:33 AM, Jonathan Richardson
> <jonathan.richardson@broadcom.com> wrote:
>>
>> On 17-02-23 09:00 PM, Jassi Brar wrote:
>>> On Fri, Feb 24, 2017 at 12:29 AM, Jonathan Richardson
>>> <jonathan.richardson@broadcom.com> wrote:
>>>> On 17-02-16 10:20 PM, Jassi Brar wrote:
>>>>> On Fri, Jan 27, 2017 at 2:08 AM, Jonathan Richardson
>>>>> <jonathan.richardson@broadcom.com> wrote:
>>>>>
>>>>>> +static int iproc_mbox_send_data_m0(struct mbox_chan *chan, void *data)
>>>>>> +{
>>>>>> +       struct iproc_mbox *mbox = dev_get_drvdata(chan->mbox->dev);
>>>>>> +       struct iproc_mbox_msg *msg = (struct iproc_mbox_msg *)data;
>>>>>> +               unsigned long flags;
>>>>>> +       int err = 0;
>>>>>> +       const int poll_period_us = 5;
>>>>>> +       const int max_retries = (MAX_M0_TIMEOUT_MS * 1000) / poll_period_us;
>>>>>> +
>>>>>> +       if (!msg)
>>>>>> +               return -EINVAL;
>>>>>> +
>>>>>> +       spin_lock_irqsave(&mbox->lock, flags);
>>>>>> +
>>>>>> +       dev_dbg(mbox->dev, "Send msg to M0: cmd=0x%x, param=0x%x, wait_ack=%d\n",
>>>>>> +               msg->cmd, msg->param, msg->wait_ack);
>>>>>> +
>>>>> prints should be outside the spinlocks.
>>>>>
>>>>>> +       writel(msg->cmd, mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>>>> +       writel(msg->param, mbox->base + IPROC_CRMU_MAILBOX1_OFFSET);
>>>>>> +
>>>>>> +       if (msg->wait_ack) {
>>>>>> +               int retries;
>>>>>> +
>>>>> move poll_period_us and max_retries in here or just define' them
>>>>>
>>>>>> +               err = msg->reply_code = -ETIMEDOUT;
>>>>>> +               for (retries = 0; retries < max_retries; retries++) {
>>>>>> +                       u32 val = readl(
>>>>>> +                               mbox->base + IPROC_CRMU_MAILBOX0_OFFSET);
>>>>>> +                       if (val & M0_IPC_CMD_DONE_MASK) {
>>>>>> +                               /*
>>>>>> +                                * M0 replied - save reply code and
>>>>>> +                                * clear error.
>>>>>> +                                */
>>>>>> +                               msg->reply_code = (val &
>>>>>> +                                       M0_IPC_CMD_REPLY_MASK) >>
>>>>>> +                                       M0_IPC_CMD_REPLY_SHIFT;
>>>>>> +                               err = 0;
>>>>>> +                               break;
>>>>>> +                       }
>>>>>> +                       udelay(poll_period_us);
>>>>>>
>>>>> potentially 2ms inside spin_lock_irqsave. Alternative is to implement
>>>>> a simple 'peek_data' and call it for requests with 'wait_ack'
>>>> Hi Jassi. The M0 response is normally 25-30 us.
>>>>
>>> You hardcode the behaviour of your protocol in the controller driver.
>>> What if your next platform/protocol has commands that the remote/M0
>>> takes upto 10ms to respond (because they are not critical/trivial)?
>>>
> You overlooked this concern?
>
>>> If you don't have some h/w indicator (like irq or bit-flag) for
>>> tx-done and data-rx , you have to use ack-by-client and peek method.
>> There isn't any functionality not in the driver. We write the message to two registers and poll another to know when it's complete. Nothing can touch the registers again until the operation is complete. We can't even pass data back to the controller. We pass pointers to the M0 and it can write to the memory. The only data we sent back to the client is that status code from the polled register. When complete, data will already be written into the clients memory from the M0.
>>> Commands that don't get a response will be immediately followed by
>>> mbox_client_txdone(), while others would need to call peek_data() to
>>> poll for data-rx. Please note, if you implement the tx_prepare
>>> callback, you will know when to start peeking for incoming data.
>> The driver will need to block access to the registers if we remove the spinlock.
>> If a client sends a message that doesn't get a response and another client has
>> already sent a message that does which is pending, it still needs to be blocked.
>>
>> The framework queues messages on a per channel basis. We have several clients for
>> various drivers each with their own mbox channel. send_data in the controller could
>> return EBUSY if the channel is being used by another channel (ie- transaction pending).
>> If channel A sends a message, then client B sends one before A is complete, the
>> controller's send_data must return an error.
>>
> Many platforms have clients sharing channels.
Hi Jassi. Could you point me to one? I've looked at all of them and don't see any of them sharing channels.
> The right way to do is have a 'server/owner' client that accepts
> requests from various clients and serially queue them to mailbox. That
> way you can keep the controller driver free from quirks (like max wait
> time of 30us) of your present platform.
Do you mean 1 mailbox client with one mailbox channel that all the mbox client drivers share? I thought this would work when I suggested it previously but the client callbacks are necessary in all txdone modes. Client drivers that send the messages need the callbacks and this is only possible with multiple mbox clients. And a channel can only have 1 mbox client. Clients in multiple drivers need the callbacks to either know when to start polling, or be notified when the transaction is complete. It would be nice if multiple clients could use the same channel.

Thanks.
> thanks

Re: [PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jassi Brar]

On Tue, Mar 28, 2017 at 11:00 PM, Jonathan Richardson
<jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> On 17-03-15 10:30 AM, Jassi Brar wrote:

>> The right way to do is have a 'server/owner' client that accepts
>> requests from various clients and serially queue them to mailbox. That
>> way you can keep the controller driver free from quirks (like max wait
>> time of 30us) of your present platform.
>
> Do you mean 1 mailbox client with one mailbox channel that all the mbox client drivers share?
> I thought this would work when I suggested it previously but the client callbacks are necessary
> in all txdone modes. Client drivers that send the messages need the callbacks
>
That's a legit requirement.

> and this is only possible with multiple mbox clients.
>
That is incorrect.
It is trivial to support callbacks to end clients from your common
code... just ask for callback along with the message to be submitted
to mailbox api.

> And a channel can only have 1 mbox client. Clients in multiple drivers need the callbacks to either know when to start polling, or be notified when the transaction is complete. It would be nice if multiple clients could use the same channel.
>
We had to choose from shared vs exclusive access to channels... latter
was chosen because there are ways to still support former.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jassi Brar]

On Tue, Mar 28, 2017 at 11:00 PM, Jonathan Richardson
<jonathan.richardson@broadcom.com> wrote:
> On 17-03-15 10:30 AM, Jassi Brar wrote:

>> The right way to do is have a 'server/owner' client that accepts
>> requests from various clients and serially queue them to mailbox. That
>> way you can keep the controller driver free from quirks (like max wait
>> time of 30us) of your present platform.
>
> Do you mean 1 mailbox client with one mailbox channel that all the mbox client drivers share?
> I thought this would work when I suggested it previously but the client callbacks are necessary
> in all txdone modes. Client drivers that send the messages need the callbacks
>
That's a legit requirement.

> and this is only possible with multiple mbox clients.
>
That is incorrect.
It is trivial to support callbacks to end clients from your common
code... just ask for callback along with the message to be submitted
to mailbox api.

> And a channel can only have 1 mbox client. Clients in multiple drivers need the callbacks to either know when to start polling, or be notified when the transaction is complete. It would be nice if multiple clients could use the same channel.
>
We had to choose from shared vs exclusive access to channels... latter
was chosen because there are ways to still support former.

Re: [PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

On 17-03-28 09:36 PM, Jassi Brar wrote:
> On Tue, Mar 28, 2017 at 11:00 PM, Jonathan Richardson
> <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>> On 17-03-15 10:30 AM, Jassi Brar wrote:
>>> The right way to do is have a 'server/owner' client that accepts
>>> requests from various clients and serially queue them to mailbox. That
>>> way you can keep the controller driver free from quirks (like max wait
>>> time of 30us) of your present platform.
>> Do you mean 1 mailbox client with one mailbox channel that all the mbox client drivers share?
>> I thought this would work when I suggested it previously but the client callbacks are necessary
>> in all txdone modes. Client drivers that send the messages need the callbacks
>>
> That's a legit requirement.
>
>> and this is only possible with multiple mbox clients.
>>
> That is incorrect.
> It is trivial to support callbacks to end clients from your common
> code... just ask for callback along with the message to be submitted
> to mailbox api.

Ok, if we can provide our own callbacks in the message instead of the client then that's perfectly fine.

>
>> And a channel can only have 1 mbox client. Clients in multiple drivers need the callbacks to either know when to start polling, or be notified when the transaction is complete. It would be nice if multiple clients could use the same channel.
>>
> We had to choose from shared vs exclusive access to channels... latter
> was chosen because there are ways to still support former.

This makes more sense now. Thanks for clarifying.

Jon


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/3] mailbox: Add iProc mailbox controller driver

[Jonathan Richardson]

On 17-03-28 09:36 PM, Jassi Brar wrote:
> On Tue, Mar 28, 2017 at 11:00 PM, Jonathan Richardson
> <jonathan.richardson@broadcom.com> wrote:
>> On 17-03-15 10:30 AM, Jassi Brar wrote:
>>> The right way to do is have a 'server/owner' client that accepts
>>> requests from various clients and serially queue them to mailbox. That
>>> way you can keep the controller driver free from quirks (like max wait
>>> time of 30us) of your present platform.
>> Do you mean 1 mailbox client with one mailbox channel that all the mbox client drivers share?
>> I thought this would work when I suggested it previously but the client callbacks are necessary
>> in all txdone modes. Client drivers that send the messages need the callbacks
>>
> That's a legit requirement.
>
>> and this is only possible with multiple mbox clients.
>>
> That is incorrect.
> It is trivial to support callbacks to end clients from your common
> code... just ask for callback along with the message to be submitted
> to mailbox api.

Ok, if we can provide our own callbacks in the message instead of the client then that's perfectly fine.

>
>> And a channel can only have 1 mbox client. Clients in multiple drivers need the callbacks to either know when to start polling, or be notified when the transaction is complete. It would be nice if multiple clients could use the same channel.
>>
> We had to choose from shared vs exclusive access to channels... latter
> was chosen because there are ways to still support former.

This makes more sense now. Thanks for clarifying.

Jon

Re: [PATCH v4 0/3] Add support for Broadcom iProc mailbox controller

[Florian Fainelli]

On 02/16/2017 12:08 PM, Jonathan Richardson wrote:
> Pinging maintainer. Jassi, have you had a chance to look at this yet? Thanks.

Jassi did provide some feedback, should we expect a v5 at some point? Thanks

> 
> On 17-01-26 12:37 PM, Jonathan Richardson wrote:
>> This patch set contains mailbox support for Broadcom iProc based SoC's. The
>> mailbox controller handles all communication with a Cortex-M0 MCU processor that
>> provides support for power, clock, and reset management. The mailbox controller
>> for Cygnus is enabled in DT.
>>
>> Changes from v3:
>>  - DT changes merged into one commit.
>>
>> Changes from v2:
>>  - Fixed binding to remove leading 0 (Rob's comment). Also added more detail as
>>    suggested by Jassi.
>>  - Added changelog to dt binding and enable dts commits.
>>  - Changed year in copyright in bcm_iproc_mailbox.h.
>>  - Changes to bcm-cygnus.dtsi were prematurely merged into the mainline after
>>    Rob acked the DT changes but Jassi suggested changes to the mailbox driver.
>>    The result was that the mailbox controller node and changes to the crmu gpio
>>    driver to enable interrupt handling were incorrectly added to the dtsi. A
>>    commit is included in this patchset to revert those changes. Enabling
>>    interrupt handling in the the crmu gpio driver will be introduced when the
>>    interrupt controller driver is added to provide mailbox interrupt support.
>>
>> Jonathan Richardson (3):
>>   dt-bindings: Document Broadcom iProc mailbox controller driver
>>   mailbox: Add iProc mailbox controller driver
>>   ARM: dts: Enable Broadcom iProc mailbox controller
>>
>>  .../bindings/mailbox/brcm,iproc-mailbox.txt        |  18 ++
>>  arch/arm/boot/dts/bcm-cygnus.dtsi                  |   8 +-
>>  drivers/mailbox/Kconfig                            |  10 ++
>>  drivers/mailbox/Makefile                           |   2 +
>>  drivers/mailbox/bcm-iproc-mailbox.c                | 199 +++++++++++++++++++++
>>  include/linux/mailbox/bcm_iproc_mailbox.h          |  32 ++++
>>  6 files changed, 262 insertions(+), 7 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt
>>  create mode 100644 drivers/mailbox/bcm-iproc-mailbox.c
>>  create mode 100644 include/linux/mailbox/bcm_iproc_mailbox.h
>>
> 

-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 0/3] Add support for Broadcom iProc mailbox controller

[Florian Fainelli]

On 02/16/2017 12:08 PM, Jonathan Richardson wrote:
> Pinging maintainer. Jassi, have you had a chance to look at this yet? Thanks.

Jassi did provide some feedback, should we expect a v5 at some point? Thanks

> 
> On 17-01-26 12:37 PM, Jonathan Richardson wrote:
>> This patch set contains mailbox support for Broadcom iProc based SoC's. The
>> mailbox controller handles all communication with a Cortex-M0 MCU processor that
>> provides support for power, clock, and reset management. The mailbox controller
>> for Cygnus is enabled in DT.
>>
>> Changes from v3:
>>  - DT changes merged into one commit.
>>
>> Changes from v2:
>>  - Fixed binding to remove leading 0 (Rob's comment). Also added more detail as
>>    suggested by Jassi.
>>  - Added changelog to dt binding and enable dts commits.
>>  - Changed year in copyright in bcm_iproc_mailbox.h.
>>  - Changes to bcm-cygnus.dtsi were prematurely merged into the mainline after
>>    Rob acked the DT changes but Jassi suggested changes to the mailbox driver.
>>    The result was that the mailbox controller node and changes to the crmu gpio
>>    driver to enable interrupt handling were incorrectly added to the dtsi. A
>>    commit is included in this patchset to revert those changes. Enabling
>>    interrupt handling in the the crmu gpio driver will be introduced when the
>>    interrupt controller driver is added to provide mailbox interrupt support.
>>
>> Jonathan Richardson (3):
>>   dt-bindings: Document Broadcom iProc mailbox controller driver
>>   mailbox: Add iProc mailbox controller driver
>>   ARM: dts: Enable Broadcom iProc mailbox controller
>>
>>  .../bindings/mailbox/brcm,iproc-mailbox.txt        |  18 ++
>>  arch/arm/boot/dts/bcm-cygnus.dtsi                  |   8 +-
>>  drivers/mailbox/Kconfig                            |  10 ++
>>  drivers/mailbox/Makefile                           |   2 +
>>  drivers/mailbox/bcm-iproc-mailbox.c                | 199 +++++++++++++++++++++
>>  include/linux/mailbox/bcm_iproc_mailbox.h          |  32 ++++
>>  6 files changed, 262 insertions(+), 7 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,iproc-mailbox.txt
>>  create mode 100644 drivers/mailbox/bcm-iproc-mailbox.c
>>  create mode 100644 include/linux/mailbox/bcm_iproc_mailbox.h
>>
> 

-- 
Florian