linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/6] Allwinner sun6i message box support
@ 2020-01-13  5:18 Samuel Holland
  2020-01-13  5:18 ` [PATCH v6 1/6] dt-bindings: mailbox: Add a sun6i message box binding Samuel Holland
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Samuel Holland @ 2020-01-13  5:18 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai, Jassi Brar, Rob Herring,
	Mark Rutland, Philipp Zabel, Ondrej Jirman, Vasily Khoruzhick
  Cc: devicetree, linux-sunxi, linux-kernel, linux-arm-kernel, Samuel Holland

This series adds support for the "hardware message box" in sun8i, sun9i,
and sun50i SoCs, used for communication with the ARISC management
processor (the platform's equivalent of the ARM SCP). The end goal is to
use the arm_scpi driver as a client, communicating with firmware running
on the ARISC CPU.

I have tested this driver with various firmware programs and mailbox
clients on the A64, H5, and H6 SoCs (including specifically the arm_scpi
driver), and Ondrej Jirman has tested the driver on the A83T (using a
similar patch to arm_scpi).

The clock changes are dropped in favor of:
https://lore.kernel.org/linux-clk/20191230193127.8803-1-samuel@sholland.org/

Hopefully I've learned my lesson that adding more tangentially-related
patches doesn't increase the likelihood of getting things merged. This
patch just includes the driver and the device tree changes.

Thanks,
Samuel

Changes from v5:
  - Rebased on tag sunxi-dt-for-5.5-2
  - Dropped unnecessary/unrelated patches
  - Addressed Maxime's dt-binding comments
  - Used devm_reset_control_get_exclusive

Changes from v4:
  - Rebased on sunxi-next
  - Dropped AR100 clock patch, as it was controversial and unnecessary
  - Renamed sunxi-msgbox to sun6i-msgbox and sun6i-a31-msgbox
  - Added comments about not asserting the reset line
  - Dropped A80 DTS changes as they were untested
  - Added Ondrej's Tested-by for A83T
  - Dropped the demo; replaced with a real arm_scpi fix

Changes from v3:
  - Rebased on sunxi-next
  - Added Rob's Reviewed-by for patch 3
  - Fixed a crash when receiving a message on a disabled channel
  - Cleaned up some comments/formatting in the driver
  - Fixed #mbox-cells in sunxi-h3-h5.dtsi (patch 7)
  - Removed the irqchip example (no longer relevant to the fw design)
  - Added a demo/example client that uses the driver and a toy firmware

Changes from v2:
  - Merge patches 1-3
  - Add a comment in the code explaining the CLK_IS_CRITICAL usage
  - Add a patch to mark the AR100 clocks as critical
  - Use YAML for the device tree binding
  - Include a not-for-merge example usage of the mailbox

Changes from v1:
  - Marked message box clocks as critical instead of hacks in the driver
  - 8 unidirectional channels instead of 4 bidirectional pairs
  - Use per-SoC compatible strings and an A31 fallback compatible
  - Dropped the mailbox framework patch
  - Include DT patches for SoCs that document the message box

Samuel Holland (6):
  dt-bindings: mailbox: Add a sun6i message box binding
  mailbox: sun6i-msgbox: Add a new mailbox driver
  ARM: dts: sunxi: a83t: Add msgbox node
  ARM: dts: sunxi: h3/h5: Add msgbox node
  arm64: dts: allwinner: a64: Add msgbox node
  arm64: dts: allwinner: h6: Add msgbox node

 .../mailbox/allwinner,sun6i-a31-msgbox.yaml   |  80 +++++
 arch/arm/boot/dts/sun8i-a83t.dtsi             |  10 +
 arch/arm/boot/dts/sunxi-h3-h5.dtsi            |  10 +
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |  10 +
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  |  10 +
 drivers/mailbox/Kconfig                       |   9 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/sun6i-msgbox.c                | 332 ++++++++++++++++++
 8 files changed, 463 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
 create mode 100644 drivers/mailbox/sun6i-msgbox.c

-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v6 1/6] dt-bindings: mailbox: Add a sun6i message box binding
  2020-01-13  5:18 [PATCH v6 0/6] Allwinner sun6i message box support Samuel Holland
@ 2020-01-13  5:18 ` Samuel Holland
  2020-01-13  9:30   ` Maxime Ripard
  2020-01-13 22:38   ` Rob Herring
  2020-01-13  5:18 ` [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver Samuel Holland
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 16+ messages in thread
From: Samuel Holland @ 2020-01-13  5:18 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai, Jassi Brar, Rob Herring,
	Mark Rutland, Philipp Zabel, Ondrej Jirman, Vasily Khoruzhick
  Cc: devicetree, linux-sunxi, linux-kernel, linux-arm-kernel, Samuel Holland

This mailbox hardware is present in Allwinner sun6i, sun8i, sun9i, and
sun50i SoCs. Add a device tree binding for it.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 .../mailbox/allwinner,sun6i-a31-msgbox.yaml   | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml

diff --git a/Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml b/Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
new file mode 100644
index 000000000000..75d5d97305e1
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/allwinner,sun6i-a31-msgbox.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner sunxi Message Box
+
+maintainers:
+  - Samuel Holland <samuel@sholland.org>
+
+description: |
+  The hardware message box on sun6i, sun8i, sun9i, and sun50i SoCs is a
+  two-user mailbox controller containing 8 unidirectional FIFOs. An interrupt
+  is raised for received messages, but software must poll to know when a
+  transmitted message has been acknowledged by the remote user. Each FIFO can
+  hold four 32-bit messages; when a FIFO is full, clients must wait before
+  attempting more transmissions.
+
+  Refer to ./mailbox.txt for generic information about mailbox device-tree
+  bindings.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - allwinner,sun8i-a83t-msgbox
+              - allwinner,sun8i-h3-msgbox
+              - allwinner,sun9i-a80-msgbox
+              - allwinner,sun50i-a64-msgbox
+              - allwinner,sun50i-h6-msgbox
+          - const: allwinner,sun6i-a31-msgbox
+      - const: allwinner,sun6i-a31-msgbox
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: bus clock
+
+  resets:
+    maxItems: 1
+    description: bus reset
+
+  interrupts:
+    maxItems: 1
+
+  '#mbox-cells':
+    const: 1
+    description: first cell is the channel number (0-7)
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - resets
+  - interrupts
+  - '#mbox-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/sun8i-h3-ccu.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/reset/sun8i-h3-ccu.h>
+
+    msgbox: mailbox@1c17000 {
+            compatible = "allwinner,sun8i-h3-msgbox",
+                         "allwinner,sun6i-a31-msgbox";
+            reg = <0x01c17000 0x1000>;
+            clocks = <&ccu CLK_BUS_MSGBOX>;
+            resets = <&ccu RST_BUS_MSGBOX>;
+            interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
+            #mbox-cells = <1>;
+    };
+
+...
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver
  2020-01-13  5:18 [PATCH v6 0/6] Allwinner sun6i message box support Samuel Holland
  2020-01-13  5:18 ` [PATCH v6 1/6] dt-bindings: mailbox: Add a sun6i message box binding Samuel Holland
@ 2020-01-13  5:18 ` Samuel Holland
  2020-01-13  9:15   ` Philipp Zabel
  2020-02-13  2:02   ` Jassi Brar
  2020-01-13  5:18 ` [PATCH v6 3/6] ARM: dts: sunxi: a83t: Add msgbox node Samuel Holland
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 16+ messages in thread
From: Samuel Holland @ 2020-01-13  5:18 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai, Jassi Brar, Rob Herring,
	Mark Rutland, Philipp Zabel, Ondrej Jirman, Vasily Khoruzhick
  Cc: devicetree, linux-sunxi, linux-kernel, linux-arm-kernel, Samuel Holland

Allwinner sun6i, sun8i, sun9i, and sun50i SoCs contain a hardware
message box used for communication between the ARM CPUs and the ARISC
management coprocessor. This mailbox contains 8 unidirectional
4-message FIFOs.

Add a driver for it, so it can be used for SCPI or other communication
protocols.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/mailbox/Kconfig        |   9 +
 drivers/mailbox/Makefile       |   2 +
 drivers/mailbox/sun6i-msgbox.c | 332 +++++++++++++++++++++++++++++++++
 3 files changed, 343 insertions(+)
 create mode 100644 drivers/mailbox/sun6i-msgbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ab4eb750bbdd..5a577a6734cf 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -227,4 +227,13 @@ config ZYNQMP_IPI_MBOX
 	  message to the IPI buffer and will access the IPI control
 	  registers to kick the other processor or enquire status.
 
+config SUN6I_MSGBOX
+	tristate "Allwinner sun6i/sun8i/sun9i/sun50i Message Box"
+	depends on ARCH_SUNXI || COMPILE_TEST
+	default ARCH_SUNXI
+	help
+	  Mailbox implementation for the hardware message box present in
+	  various Allwinner SoCs. This mailbox is used for communication
+	  between the application CPUs and the power management coprocessor.
+
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index c22fad6f696b..2e4364ef5c47 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -48,3 +48,5 @@ obj-$(CONFIG_STM32_IPCC) 	+= stm32-ipcc.o
 obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
 
 obj-$(CONFIG_ZYNQMP_IPI_MBOX)	+= zynqmp-ipi-mailbox.o
+
+obj-$(CONFIG_SUN6I_MSGBOX)	+= sun6i-msgbox.o
diff --git a/drivers/mailbox/sun6i-msgbox.c b/drivers/mailbox/sun6i-msgbox.c
new file mode 100644
index 000000000000..15d6fd522dc5
--- /dev/null
+++ b/drivers/mailbox/sun6i-msgbox.c
@@ -0,0 +1,332 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2017-2019 Samuel Holland <samuel@sholland.org>
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/spinlock.h>
+
+#define NUM_CHANS		8
+
+#define CTRL_REG(n)		(0x0000 + 0x4 * ((n) / 4))
+#define CTRL_RX(n)		BIT(0 + 8 * ((n) % 4))
+#define CTRL_TX(n)		BIT(4 + 8 * ((n) % 4))
+
+#define REMOTE_IRQ_EN_REG	0x0040
+#define REMOTE_IRQ_STAT_REG	0x0050
+#define LOCAL_IRQ_EN_REG	0x0060
+#define LOCAL_IRQ_STAT_REG	0x0070
+
+#define RX_IRQ(n)		BIT(0 + 2 * (n))
+#define RX_IRQ_MASK		0x5555
+#define TX_IRQ(n)		BIT(1 + 2 * (n))
+#define TX_IRQ_MASK		0xaaaa
+
+#define FIFO_STAT_REG(n)	(0x0100 + 0x4 * (n))
+#define FIFO_STAT_MASK		GENMASK(0, 0)
+
+#define MSG_STAT_REG(n)		(0x0140 + 0x4 * (n))
+#define MSG_STAT_MASK		GENMASK(2, 0)
+
+#define MSG_DATA_REG(n)		(0x0180 + 0x4 * (n))
+
+#define mbox_dbg(mbox, ...)	dev_dbg((mbox)->controller.dev, __VA_ARGS__)
+
+struct sun6i_msgbox {
+	struct mbox_controller controller;
+	struct clk *clk;
+	spinlock_t lock;
+	void __iomem *regs;
+};
+
+static bool sun6i_msgbox_last_tx_done(struct mbox_chan *chan);
+static bool sun6i_msgbox_peek_data(struct mbox_chan *chan);
+
+static inline int channel_number(struct mbox_chan *chan)
+{
+	return chan - chan->mbox->chans;
+}
+
+static inline struct sun6i_msgbox *to_sun6i_msgbox(struct mbox_chan *chan)
+{
+	return chan->con_priv;
+}
+
+static irqreturn_t sun6i_msgbox_irq(int irq, void *dev_id)
+{
+	struct sun6i_msgbox *mbox = dev_id;
+	uint32_t status;
+	int n;
+
+	/* Only examine channels that are currently enabled. */
+	status = readl(mbox->regs + LOCAL_IRQ_EN_REG) &
+		 readl(mbox->regs + LOCAL_IRQ_STAT_REG);
+
+	if (!(status & RX_IRQ_MASK))
+		return IRQ_NONE;
+
+	for (n = 0; n < NUM_CHANS; ++n) {
+		struct mbox_chan *chan = &mbox->controller.chans[n];
+
+		if (!(status & RX_IRQ(n)))
+			continue;
+
+		while (sun6i_msgbox_peek_data(chan)) {
+			uint32_t msg = readl(mbox->regs + MSG_DATA_REG(n));
+
+			mbox_dbg(mbox, "Channel %d received 0x%08x\n", n, msg);
+			mbox_chan_received_data(chan, &msg);
+		}
+
+		/* The IRQ can be cleared only once the FIFO is empty. */
+		writel(RX_IRQ(n), mbox->regs + LOCAL_IRQ_STAT_REG);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int sun6i_msgbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct sun6i_msgbox *mbox = to_sun6i_msgbox(chan);
+	int n = channel_number(chan);
+	uint32_t msg = *(uint32_t *)data;
+
+	/* Using a channel backwards gets the hardware into a bad state. */
+	if (WARN_ON_ONCE(!(readl(mbox->regs + CTRL_REG(n)) & CTRL_TX(n))))
+		return 0;
+
+	/* We cannot post a new message if the FIFO is full. */
+	if (readl(mbox->regs + FIFO_STAT_REG(n)) & FIFO_STAT_MASK) {
+		mbox_dbg(mbox, "Channel %d busy sending 0x%08x\n", n, msg);
+		return -EBUSY;
+	}
+
+	writel(msg, mbox->regs + MSG_DATA_REG(n));
+	mbox_dbg(mbox, "Channel %d sent 0x%08x\n", n, msg);
+
+	return 0;
+}
+
+static int sun6i_msgbox_startup(struct mbox_chan *chan)
+{
+	struct sun6i_msgbox *mbox = to_sun6i_msgbox(chan);
+	int n = channel_number(chan);
+
+	/* The coprocessor is responsible for setting channel directions. */
+	if (readl(mbox->regs + CTRL_REG(n)) & CTRL_RX(n)) {
+		/* Flush the receive FIFO. */
+		while (sun6i_msgbox_peek_data(chan))
+			readl(mbox->regs + MSG_DATA_REG(n));
+		writel(RX_IRQ(n), mbox->regs + LOCAL_IRQ_STAT_REG);
+
+		/* Enable the receive IRQ. */
+		spin_lock(&mbox->lock);
+		writel(readl(mbox->regs + LOCAL_IRQ_EN_REG) | RX_IRQ(n),
+		       mbox->regs + LOCAL_IRQ_EN_REG);
+		spin_unlock(&mbox->lock);
+	}
+
+	mbox_dbg(mbox, "Channel %d startup complete\n", n);
+
+	return 0;
+}
+
+static void sun6i_msgbox_shutdown(struct mbox_chan *chan)
+{
+	struct sun6i_msgbox *mbox = to_sun6i_msgbox(chan);
+	int n = channel_number(chan);
+
+	if (readl(mbox->regs + CTRL_REG(n)) & CTRL_RX(n)) {
+		/* Disable the receive IRQ. */
+		spin_lock(&mbox->lock);
+		writel(readl(mbox->regs + LOCAL_IRQ_EN_REG) & ~RX_IRQ(n),
+		       mbox->regs + LOCAL_IRQ_EN_REG);
+		spin_unlock(&mbox->lock);
+
+		/* Attempt to flush the FIFO until the IRQ is cleared. */
+		do {
+			while (sun6i_msgbox_peek_data(chan))
+				readl(mbox->regs + MSG_DATA_REG(n));
+			writel(RX_IRQ(n), mbox->regs + LOCAL_IRQ_STAT_REG);
+		} while (readl(mbox->regs + LOCAL_IRQ_STAT_REG) & RX_IRQ(n));
+	}
+
+	mbox_dbg(mbox, "Channel %d shutdown complete\n", n);
+}
+
+static bool sun6i_msgbox_last_tx_done(struct mbox_chan *chan)
+{
+	struct sun6i_msgbox *mbox = to_sun6i_msgbox(chan);
+	int n = channel_number(chan);
+
+	/*
+	 * The hardware allows snooping on the remote user's IRQ statuses.
+	 * We consider a message to be acknowledged only once the receive IRQ
+	 * for that channel is cleared. Since the receive IRQ for a channel
+	 * cannot be cleared until the FIFO for that channel is empty, this
+	 * ensures that the message has actually been read. It also gives the
+	 * recipient an opportunity to perform minimal processing before
+	 * acknowledging the message.
+	 */
+	return !(readl(mbox->regs + REMOTE_IRQ_STAT_REG) & RX_IRQ(n));
+}
+
+static bool sun6i_msgbox_peek_data(struct mbox_chan *chan)
+{
+	struct sun6i_msgbox *mbox = to_sun6i_msgbox(chan);
+	int n = channel_number(chan);
+
+	return readl(mbox->regs + MSG_STAT_REG(n)) & MSG_STAT_MASK;
+}
+
+static const struct mbox_chan_ops sun6i_msgbox_chan_ops = {
+	.send_data    = sun6i_msgbox_send_data,
+	.startup      = sun6i_msgbox_startup,
+	.shutdown     = sun6i_msgbox_shutdown,
+	.last_tx_done = sun6i_msgbox_last_tx_done,
+	.peek_data    = sun6i_msgbox_peek_data,
+};
+
+static int sun6i_msgbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mbox_chan *chans;
+	struct reset_control *reset;
+	struct resource *res;
+	struct sun6i_msgbox *mbox;
+	int i, ret;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	chans = devm_kcalloc(dev, NUM_CHANS, sizeof(*chans), GFP_KERNEL);
+	if (!chans)
+		return -ENOMEM;
+
+	for (i = 0; i < NUM_CHANS; ++i)
+		chans[i].con_priv = mbox;
+
+	mbox->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(mbox->clk)) {
+		ret = PTR_ERR(mbox->clk);
+		dev_err(dev, "Failed to get clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(mbox->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock: %d\n", ret);
+		return ret;
+	}
+
+	reset = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(reset)) {
+		ret = PTR_ERR(reset);
+		dev_err(dev, "Failed to get reset control: %d\n", ret);
+		goto err_disable_unprepare;
+	}
+
+	/*
+	 * NOTE: We rely on platform firmware to preconfigure the channel
+	 * directions, and we share this hardware block with other firmware
+	 * that runs concurrently with Linux (e.g. a trusted monitor).
+	 *
+	 * Therefore, we do *not* assert the reset line if probing fails or
+	 * when removing the device.
+	 */
+	ret = reset_control_deassert(reset);
+	if (ret) {
+		dev_err(dev, "Failed to deassert reset: %d\n", ret);
+		goto err_disable_unprepare;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		ret = -ENODEV;
+		goto err_disable_unprepare;
+	}
+
+	mbox->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(mbox->regs)) {
+		ret = PTR_ERR(mbox->regs);
+		dev_err(dev, "Failed to map MMIO resource: %d\n", ret);
+		goto err_disable_unprepare;
+	}
+
+	/* Disable all IRQs for this end of the msgbox. */
+	writel(0, mbox->regs + LOCAL_IRQ_EN_REG);
+
+	ret = devm_request_irq(dev, irq_of_parse_and_map(dev->of_node, 0),
+			       sun6i_msgbox_irq, 0, dev_name(dev), mbox);
+	if (ret) {
+		dev_err(dev, "Failed to register IRQ handler: %d\n", ret);
+		goto err_disable_unprepare;
+	}
+
+	mbox->controller.dev           = dev;
+	mbox->controller.ops           = &sun6i_msgbox_chan_ops;
+	mbox->controller.chans         = chans;
+	mbox->controller.num_chans     = NUM_CHANS;
+	mbox->controller.txdone_irq    = false;
+	mbox->controller.txdone_poll   = true;
+	mbox->controller.txpoll_period = 5;
+
+	spin_lock_init(&mbox->lock);
+	platform_set_drvdata(pdev, mbox);
+
+	ret = mbox_controller_register(&mbox->controller);
+	if (ret) {
+		dev_err(dev, "Failed to register controller: %d\n", ret);
+		goto err_disable_unprepare;
+	}
+
+	return 0;
+
+err_disable_unprepare:
+	clk_disable_unprepare(mbox->clk);
+
+	return ret;
+}
+
+static int sun6i_msgbox_remove(struct platform_device *pdev)
+{
+	struct sun6i_msgbox *mbox = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&mbox->controller);
+	/* See the comment in sun6i_msgbox_probe about the reset line. */
+	clk_disable_unprepare(mbox->clk);
+
+	return 0;
+}
+
+static const struct of_device_id sun6i_msgbox_of_match[] = {
+	{ .compatible = "allwinner,sun6i-a31-msgbox", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sun6i_msgbox_of_match);
+
+static struct platform_driver sun6i_msgbox_driver = {
+	.driver = {
+		.name = "sun6i-msgbox",
+		.of_match_table = sun6i_msgbox_of_match,
+	},
+	.probe  = sun6i_msgbox_probe,
+	.remove = sun6i_msgbox_remove,
+};
+module_platform_driver(sun6i_msgbox_driver);
+
+MODULE_AUTHOR("Samuel Holland <samuel@sholland.org>");
+MODULE_DESCRIPTION("Allwinner sun6i/sun8i/sun9i/sun50i Message Box");
+MODULE_LICENSE("GPL v2");
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v6 3/6] ARM: dts: sunxi: a83t: Add msgbox node
  2020-01-13  5:18 [PATCH v6 0/6] Allwinner sun6i message box support Samuel Holland
  2020-01-13  5:18 ` [PATCH v6 1/6] dt-bindings: mailbox: Add a sun6i message box binding Samuel Holland
  2020-01-13  5:18 ` [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver Samuel Holland
@ 2020-01-13  5:18 ` Samuel Holland
  2020-01-13  5:18 ` [PATCH v6 4/6] ARM: dts: sunxi: h3/h5: " Samuel Holland
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Samuel Holland @ 2020-01-13  5:18 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai, Jassi Brar, Rob Herring,
	Mark Rutland, Philipp Zabel, Ondrej Jirman, Vasily Khoruzhick
  Cc: devicetree, linux-sunxi, linux-kernel, linux-arm-kernel, Samuel Holland

The A83T SoC contains a message box that can be used to send messages
and interrupts back and forth between the ARM application CPUs and the
ARISC coprocessor. Add a device tree node for it.

Tested-by: Ondrej Jirman <megous@megous.com>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 arch/arm/boot/dts/sun8i-a83t.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
index 53c38deb8a08..464b57d03dc0 100644
--- a/arch/arm/boot/dts/sun8i-a83t.dtsi
+++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
@@ -592,6 +592,16 @@
 			clock-names = "bus", "mod";
 		};
 
+		msgbox: mailbox@1c17000 {
+			compatible = "allwinner,sun8i-a83t-msgbox",
+				     "allwinner,sun6i-a31-msgbox";
+			reg = <0x01c17000 0x1000>;
+			clocks = <&ccu CLK_BUS_MSGBOX>;
+			resets = <&ccu RST_BUS_MSGBOX>;
+			interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
+			#mbox-cells = <1>;
+		};
+
 		usb_otg: usb@1c19000 {
 			compatible = "allwinner,sun8i-a83t-musb",
 				     "allwinner,sun8i-a33-musb";
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v6 4/6] ARM: dts: sunxi: h3/h5: Add msgbox node
  2020-01-13  5:18 [PATCH v6 0/6] Allwinner sun6i message box support Samuel Holland
                   ` (2 preceding siblings ...)
  2020-01-13  5:18 ` [PATCH v6 3/6] ARM: dts: sunxi: a83t: Add msgbox node Samuel Holland
@ 2020-01-13  5:18 ` Samuel Holland
  2020-01-13  5:18 ` [PATCH v6 5/6] arm64: dts: allwinner: a64: " Samuel Holland
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Samuel Holland @ 2020-01-13  5:18 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai, Jassi Brar, Rob Herring,
	Mark Rutland, Philipp Zabel, Ondrej Jirman, Vasily Khoruzhick
  Cc: devicetree, linux-sunxi, linux-kernel, linux-arm-kernel, Samuel Holland

The H3 and H5 SoCs contain a message box that can be used to send
messages and interrupts back and forth between the ARM application CPUs
and the ARISC coprocessor. Add a device tree node for it.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 arch/arm/boot/dts/sunxi-h3-h5.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
index 0afea59486c2..6c5b283962a1 100644
--- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
+++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
@@ -233,6 +233,16 @@
 			reg = <0x1c14000 0x400>;
 		};
 
+		msgbox: mailbox@1c17000 {
+			compatible = "allwinner,sun8i-h3-msgbox",
+				     "allwinner,sun6i-a31-msgbox";
+			reg = <0x01c17000 0x1000>;
+			clocks = <&ccu CLK_BUS_MSGBOX>;
+			resets = <&ccu RST_BUS_MSGBOX>;
+			interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
+			#mbox-cells = <1>;
+		};
+
 		usb_otg: usb@1c19000 {
 			compatible = "allwinner,sun8i-h3-musb";
 			reg = <0x01c19000 0x400>;
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v6 5/6] arm64: dts: allwinner: a64: Add msgbox node
  2020-01-13  5:18 [PATCH v6 0/6] Allwinner sun6i message box support Samuel Holland
                   ` (3 preceding siblings ...)
  2020-01-13  5:18 ` [PATCH v6 4/6] ARM: dts: sunxi: h3/h5: " Samuel Holland
@ 2020-01-13  5:18 ` Samuel Holland
  2020-01-13  5:18 ` [PATCH v6 6/6] arm64: dts: allwinner: h6: " Samuel Holland
  2020-02-13  1:43 ` [PATCH v6 0/6] Allwinner sun6i message box support Samuel Holland
  6 siblings, 0 replies; 16+ messages in thread
From: Samuel Holland @ 2020-01-13  5:18 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai, Jassi Brar, Rob Herring,
	Mark Rutland, Philipp Zabel, Ondrej Jirman, Vasily Khoruzhick
  Cc: devicetree, linux-sunxi, linux-kernel, linux-arm-kernel, Samuel Holland

The A64 SoC contains a message box that can be used to send messages and
interrupts back and forth between the ARM application CPUs and the ARISC
coprocessor. Add a device tree node for it.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 0287d8458675..690f0a7d9cfa 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -487,6 +487,16 @@
 			resets = <&ccu RST_BUS_CE>;
 		};
 
+		msgbox: mailbox@1c17000 {
+			compatible = "allwinner,sun50i-a64-msgbox",
+				     "allwinner,sun6i-a31-msgbox";
+			reg = <0x01c17000 0x1000>;
+			clocks = <&ccu CLK_BUS_MSGBOX>;
+			resets = <&ccu RST_BUS_MSGBOX>;
+			interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
+			#mbox-cells = <1>;
+		};
+
 		usb_otg: usb@1c19000 {
 			compatible = "allwinner,sun8i-a33-musb";
 			reg = <0x01c19000 0x0400>;
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v6 6/6] arm64: dts: allwinner: h6: Add msgbox node
  2020-01-13  5:18 [PATCH v6 0/6] Allwinner sun6i message box support Samuel Holland
                   ` (4 preceding siblings ...)
  2020-01-13  5:18 ` [PATCH v6 5/6] arm64: dts: allwinner: a64: " Samuel Holland
@ 2020-01-13  5:18 ` Samuel Holland
  2020-02-13  1:43 ` [PATCH v6 0/6] Allwinner sun6i message box support Samuel Holland
  6 siblings, 0 replies; 16+ messages in thread
From: Samuel Holland @ 2020-01-13  5:18 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai, Jassi Brar, Rob Herring,
	Mark Rutland, Philipp Zabel, Ondrej Jirman, Vasily Khoruzhick
  Cc: devicetree, linux-sunxi, linux-kernel, linux-arm-kernel, Samuel Holland

The H6 SoC contains a message box that can be used to send messages and
interrupts back and forth between the ARM application CPUs and the ARISC
coprocessor. Add a device tree node for it.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 29824081b43b..8dc6ba71a901 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -230,6 +230,16 @@
 			#dma-cells = <1>;
 		};
 
+		msgbox: mailbox@3003000 {
+			compatible = "allwinner,sun50i-h6-msgbox",
+				     "allwinner,sun6i-a31-msgbox";
+			reg = <0x03003000 0x1000>;
+			clocks = <&ccu CLK_BUS_MSGBOX>;
+			resets = <&ccu RST_BUS_MSGBOX>;
+			interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
+			#mbox-cells = <1>;
+		};
+
 		sid: efuse@3006000 {
 			compatible = "allwinner,sun50i-h6-sid";
 			reg = <0x03006000 0x400>;
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver
  2020-01-13  5:18 ` [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver Samuel Holland
@ 2020-01-13  9:15   ` Philipp Zabel
  2020-02-13  2:02   ` Jassi Brar
  1 sibling, 0 replies; 16+ messages in thread
From: Philipp Zabel @ 2020-01-13  9:15 UTC (permalink / raw)
  To: Samuel Holland, Maxime Ripard, Chen-Yu Tsai, Jassi Brar,
	Rob Herring, Mark Rutland, Ondrej Jirman, Vasily Khoruzhick
  Cc: devicetree, linux-sunxi, linux-kernel, linux-arm-kernel

On Sun, 2020-01-12 at 23:18 -0600, Samuel Holland wrote:
> Allwinner sun6i, sun8i, sun9i, and sun50i SoCs contain a hardware
> message box used for communication between the ARM CPUs and the ARISC
> management coprocessor. This mailbox contains 8 unidirectional
> 4-message FIFOs.
> 
> Add a driver for it, so it can be used for SCPI or other communication
> protocols.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/mailbox/Kconfig        |   9 +
>  drivers/mailbox/Makefile       |   2 +
>  drivers/mailbox/sun6i-msgbox.c | 332 +++++++++++++++++++++++++++++++++
>  3 files changed, 343 insertions(+)
>  create mode 100644 drivers/mailbox/sun6i-msgbox.c
> 
[...]
> diff --git a/drivers/mailbox/sun6i-msgbox.c b/drivers/mailbox/sun6i-msgbox.c
> new file mode 100644
> index 000000000000..15d6fd522dc5
> --- /dev/null
> +++ b/drivers/mailbox/sun6i-msgbox.c
> @@ -0,0 +1,332 @@
[...]
> +	reset = devm_reset_control_get_exclusive(dev, NULL);
> +	if (IS_ERR(reset)) {
> +		ret = PTR_ERR(reset);
> +		dev_err(dev, "Failed to get reset control: %d\n", ret);
> +		goto err_disable_unprepare;
> +	}
> +
> +	/*
> +	 * NOTE: We rely on platform firmware to preconfigure the channel
> +	 * directions, and we share this hardware block with other firmware
> +	 * that runs concurrently with Linux (e.g. a trusted monitor).
> +	 *
> +	 * Therefore, we do *not* assert the reset line if probing fails or
> +	 * when removing the device.
> +	 */
> +	ret = reset_control_deassert(reset);
> +	if (ret) {
> +		dev_err(dev, "Failed to deassert reset: %d\n", ret);
> +		goto err_disable_unprepare;
> +	}

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 1/6] dt-bindings: mailbox: Add a sun6i message box binding
  2020-01-13  5:18 ` [PATCH v6 1/6] dt-bindings: mailbox: Add a sun6i message box binding Samuel Holland
@ 2020-01-13  9:30   ` Maxime Ripard
  2020-01-13 22:38   ` Rob Herring
  1 sibling, 0 replies; 16+ messages in thread
From: Maxime Ripard @ 2020-01-13  9:30 UTC (permalink / raw)
  To: Samuel Holland
  Cc: Mark Rutland, Ondrej Jirman, devicetree, linux-sunxi, Jassi Brar,
	linux-kernel, Chen-Yu Tsai, Rob Herring, Philipp Zabel,
	linux-arm-kernel


[-- Attachment #1.1: Type: text/plain, Size: 304 bytes --]

On Sun, Jan 12, 2020 at 11:18:47PM -0600, Samuel Holland wrote:
> This mailbox hardware is present in Allwinner sun6i, sun8i, sun9i, and
> sun50i SoCs. Add a device tree binding for it.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 1/6] dt-bindings: mailbox: Add a sun6i message box binding
  2020-01-13  5:18 ` [PATCH v6 1/6] dt-bindings: mailbox: Add a sun6i message box binding Samuel Holland
  2020-01-13  9:30   ` Maxime Ripard
@ 2020-01-13 22:38   ` Rob Herring
  1 sibling, 0 replies; 16+ messages in thread
From: Rob Herring @ 2020-01-13 22:38 UTC (permalink / raw)
  To: Samuel Holland
  Cc: Mark Rutland, Ondrej Jirman, devicetree, linux-sunxi, Jassi Brar,
	linux-kernel, Maxime Ripard, Chen-Yu Tsai, Philipp Zabel,
	Samuel Holland, linux-arm-kernel

On Sun, 12 Jan 2020 23:18:47 -0600, Samuel Holland wrote:
> This mailbox hardware is present in Allwinner sun6i, sun8i, sun9i, and
> sun50i SoCs. Add a device tree binding for it.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  .../mailbox/allwinner,sun6i-a31-msgbox.yaml   | 80 +++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
> 

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 0/6] Allwinner sun6i message box support
  2020-01-13  5:18 [PATCH v6 0/6] Allwinner sun6i message box support Samuel Holland
                   ` (5 preceding siblings ...)
  2020-01-13  5:18 ` [PATCH v6 6/6] arm64: dts: allwinner: h6: " Samuel Holland
@ 2020-02-13  1:43 ` Samuel Holland
  6 siblings, 0 replies; 16+ messages in thread
From: Samuel Holland @ 2020-02-13  1:43 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai, Jassi Brar, Rob Herring,
	Mark Rutland, Philipp Zabel, Ondrej Jirman, Vasily Khoruzhick
  Cc: devicetree, linux-sunxi, linux-kernel, linux-arm-kernel

Jassi,

On 1/12/20 11:18 PM, Samuel Holland wrote:
> This series adds support for the "hardware message box" in sun8i, sun9i,
> and sun50i SoCs, used for communication with the ARISC management
> processor (the platform's equivalent of the ARM SCP). The end goal is to
> use the arm_scpi driver as a client, communicating with firmware running
> on the ARISC CPU.
> 
> I have tested this driver with various firmware programs and mailbox
> clients on the A64, H5, and H6 SoCs (including specifically the arm_scpi
> driver), and Ondrej Jirman has tested the driver on the A83T (using a
> similar patch to arm_scpi).

Ping. Any comments on this? Can this be merged?

Thanks,
Samuel Holland

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver
  2020-01-13  5:18 ` [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver Samuel Holland
  2020-01-13  9:15   ` Philipp Zabel
@ 2020-02-13  2:02   ` Jassi Brar
  2020-02-13  2:18     ` Samuel Holland
  1 sibling, 1 reply; 16+ messages in thread
From: Jassi Brar @ 2020-02-13  2:02 UTC (permalink / raw)
  To: Samuel Holland
  Cc: Mark Rutland, Ondrej Jirman, Devicetree List, linux-sunxi,
	Linux Kernel Mailing List, Maxime Ripard, Chen-Yu Tsai,
	Rob Herring, Philipp Zabel, linux-arm-kernel

On Sun, Jan 12, 2020 at 11:18 PM Samuel Holland <samuel@sholland.org> wrote:
>
> +static int sun6i_msgbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +       struct sun6i_msgbox *mbox = to_sun6i_msgbox(chan);
> +       int n = channel_number(chan);
> +       uint32_t msg = *(uint32_t *)data;
> +
> +       /* Using a channel backwards gets the hardware into a bad state. */
> +       if (WARN_ON_ONCE(!(readl(mbox->regs + CTRL_REG(n)) & CTRL_TX(n))))
> +               return 0;
> +
> +       /* We cannot post a new message if the FIFO is full. */
> +       if (readl(mbox->regs + FIFO_STAT_REG(n)) & FIFO_STAT_MASK) {
> +               mbox_dbg(mbox, "Channel %d busy sending 0x%08x\n", n, msg);
> +               return -EBUSY;
> +       }
> +
This check should go into sun6i_msgbox_last_tx_done().
send_data() assumes all is clear to send next packet.

.....
> +
> +       mbox->controller.dev           = dev;
> +       mbox->controller.ops           = &sun6i_msgbox_chan_ops;
> +       mbox->controller.chans         = chans;
> +       mbox->controller.num_chans     = NUM_CHANS;
> +       mbox->controller.txdone_irq    = false;
> +       mbox->controller.txdone_poll   = true;
> +       mbox->controller.txpoll_period = 5;
> +
nit:  just a single space should do too.

Sorry, for some reason I thought I had replied to this patch, but
apparently not. My mistake. Do you want to revise this submission or
send another patch on top?

thanks

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver
  2020-02-13  2:02   ` Jassi Brar
@ 2020-02-13  2:18     ` Samuel Holland
  2020-02-15  3:48       ` Samuel Holland
  0 siblings, 1 reply; 16+ messages in thread
From: Samuel Holland @ 2020-02-13  2:18 UTC (permalink / raw)
  To: Jassi Brar
  Cc: Mark Rutland, Ondrej Jirman, Devicetree List, linux-sunxi,
	Linux Kernel Mailing List, Maxime Ripard, Chen-Yu Tsai,
	Rob Herring, Philipp Zabel, linux-arm-kernel

Jassi,

On 2/12/20 8:02 PM, Jassi Brar wrote:
> On Sun, Jan 12, 2020 at 11:18 PM Samuel Holland <samuel@sholland.org> wrote:
>>
>> +static int sun6i_msgbox_send_data(struct mbox_chan *chan, void *data)
>> +{
>> +       struct sun6i_msgbox *mbox = to_sun6i_msgbox(chan);
>> +       int n = channel_number(chan);
>> +       uint32_t msg = *(uint32_t *)data;
>> +
>> +       /* Using a channel backwards gets the hardware into a bad state. */
>> +       if (WARN_ON_ONCE(!(readl(mbox->regs + CTRL_REG(n)) & CTRL_TX(n))))
>> +               return 0;
>> +
>> +       /* We cannot post a new message if the FIFO is full. */
>> +       if (readl(mbox->regs + FIFO_STAT_REG(n)) & FIFO_STAT_MASK) {
>> +               mbox_dbg(mbox, "Channel %d busy sending 0x%08x\n", n, msg);
>> +               return -EBUSY;
>> +       }
>> +
> This check should go into sun6i_msgbox_last_tx_done().
> send_data() assumes all is clear to send next packet.

sun6i_msgbox_last_tx_done() already checks that the FIFO is completely empty (as
the big comment explains). So this error could only be hit in the knows_txdone
== true case, if the client pipelines multiple messages by calling
mbox_client_txdone() before the message is actually removed from the FIFO.

From the comments in mailbox_controller.h, this kind of usage looks to be
unsupported. In that case, I could remove the check entirely. Does that sound right?

> .....
>> +
>> +       mbox->controller.dev           = dev;
>> +       mbox->controller.ops           = &sun6i_msgbox_chan_ops;
>> +       mbox->controller.chans         = chans;
>> +       mbox->controller.num_chans     = NUM_CHANS;
>> +       mbox->controller.txdone_irq    = false;
>> +       mbox->controller.txdone_poll   = true;
>> +       mbox->controller.txpoll_period = 5;
>> +
> nit:  just a single space should do too.
> 
> Sorry, for some reason I thought I had replied to this patch, but
> apparently not. My mistake. Do you want to revise this submission or
> send another patch on top?

For just this change, it would be simpler to send a follow-up patch.

> thanks

Thank you,
Samuel


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver
  2020-02-13  2:18     ` Samuel Holland
@ 2020-02-15  3:48       ` Samuel Holland
  2020-02-15  4:47         ` Jassi Brar
  0 siblings, 1 reply; 16+ messages in thread
From: Samuel Holland @ 2020-02-15  3:48 UTC (permalink / raw)
  To: Jassi Brar
  Cc: Mark Rutland, Ondrej Jirman, Devicetree List, linux-sunxi,
	Linux Kernel Mailing List, Maxime Ripard, Chen-Yu Tsai,
	Rob Herring, Philipp Zabel, linux-arm-kernel

On 2/12/20 8:18 PM, Samuel Holland wrote:
> Jassi,
> 
> On 2/12/20 8:02 PM, Jassi Brar wrote:
>> On Sun, Jan 12, 2020 at 11:18 PM Samuel Holland <samuel@sholland.org> wrote:
>>>
>>> +static int sun6i_msgbox_send_data(struct mbox_chan *chan, void *data)
>>> +{
>>> +       struct sun6i_msgbox *mbox = to_sun6i_msgbox(chan);
>>> +       int n = channel_number(chan);
>>> +       uint32_t msg = *(uint32_t *)data;
>>> +
>>> +       /* Using a channel backwards gets the hardware into a bad state. */
>>> +       if (WARN_ON_ONCE(!(readl(mbox->regs + CTRL_REG(n)) & CTRL_TX(n))))
>>> +               return 0;
>>> +
>>> +       /* We cannot post a new message if the FIFO is full. */
>>> +       if (readl(mbox->regs + FIFO_STAT_REG(n)) & FIFO_STAT_MASK) {
>>> +               mbox_dbg(mbox, "Channel %d busy sending 0x%08x\n", n, msg);
>>> +               return -EBUSY;
>>> +       }
>>> +
>> This check should go into sun6i_msgbox_last_tx_done().
>> send_data() assumes all is clear to send next packet.
> 
> sun6i_msgbox_last_tx_done() already checks that the FIFO is completely empty (as
> the big comment explains). So this error could only be hit in the knows_txdone
> == true case, if the client pipelines multiple messages by calling
> mbox_client_txdone() before the message is actually removed from the FIFO.
> 
> From the comments in mailbox_controller.h, this kind of usage looks to be
> unsupported. In that case, I could remove the check entirely. Does that sound right?

After more thought, I would prefer to keep the check. It is fast/simple, and it
keeps the hardware from getting into an inconsistent state. Silently dropping
messages sounds like a poor quality of implementation.

send_data() is documented in mailbox_controller.h as returning EBUSY, and I see
multiple other mailbox controllers implementing the same or a similar check. If
that is not the way you intend for the API to work, then please update the
comments in mailbox_controller.h.

Thanks,
Samuel

>> .....
>>> +
>>> +       mbox->controller.dev           = dev;
>>> +       mbox->controller.ops           = &sun6i_msgbox_chan_ops;
>>> +       mbox->controller.chans         = chans;
>>> +       mbox->controller.num_chans     = NUM_CHANS;
>>> +       mbox->controller.txdone_irq    = false;
>>> +       mbox->controller.txdone_poll   = true;
>>> +       mbox->controller.txpoll_period = 5;
>>> +
>> nit:  just a single space should do too.
>>
>> Sorry, for some reason I thought I had replied to this patch, but
>> apparently not. My mistake. Do you want to revise this submission or
>> send another patch on top?
> 
> For just this change, it would be simpler to send a follow-up patch.
> 
>> thanks
> 
> Thank you,
> Samuel
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver
  2020-02-15  3:48       ` Samuel Holland
@ 2020-02-15  4:47         ` Jassi Brar
  2020-02-15  6:19           ` [PATCH] mailbox: sun6i-msgbox: Remove unneeded FIFO status check Samuel Holland
  0 siblings, 1 reply; 16+ messages in thread
From: Jassi Brar @ 2020-02-15  4:47 UTC (permalink / raw)
  To: Samuel Holland
  Cc: Mark Rutland, Ondrej Jirman, Devicetree List, linux-sunxi,
	Linux Kernel Mailing List, Maxime Ripard, Chen-Yu Tsai,
	Rob Herring, Philipp Zabel, linux-arm-kernel

On Fri, Feb 14, 2020 at 9:48 PM Samuel Holland <samuel@sholland.org> wrote:
>
> On 2/12/20 8:18 PM, Samuel Holland wrote:
> > Jassi,
> >
> > On 2/12/20 8:02 PM, Jassi Brar wrote:
> >> On Sun, Jan 12, 2020 at 11:18 PM Samuel Holland <samuel@sholland.org> wrote:
> >>>
> >>> +static int sun6i_msgbox_send_data(struct mbox_chan *chan, void *data)
> >>> +{
> >>> +       struct sun6i_msgbox *mbox = to_sun6i_msgbox(chan);
> >>> +       int n = channel_number(chan);
> >>> +       uint32_t msg = *(uint32_t *)data;
> >>> +
> >>> +       /* Using a channel backwards gets the hardware into a bad state. */
> >>> +       if (WARN_ON_ONCE(!(readl(mbox->regs + CTRL_REG(n)) & CTRL_TX(n))))
> >>> +               return 0;
> >>> +
> >>> +       /* We cannot post a new message if the FIFO is full. */
> >>> +       if (readl(mbox->regs + FIFO_STAT_REG(n)) & FIFO_STAT_MASK) {
> >>> +               mbox_dbg(mbox, "Channel %d busy sending 0x%08x\n", n, msg);
> >>> +               return -EBUSY;
> >>> +       }
> >>> +
> >> This check should go into sun6i_msgbox_last_tx_done().
> >> send_data() assumes all is clear to send next packet.
> >
> > sun6i_msgbox_last_tx_done() already checks that the FIFO is completely empty (as
> > the big comment explains). So this error could only be hit in the knows_txdone
> > == true case, if the client pipelines multiple messages by calling
> > mbox_client_txdone() before the message is actually removed from the FIFO.
> >
> > From the comments in mailbox_controller.h, this kind of usage looks to be
> > unsupported. In that case, I could remove the check entirely. Does that sound right?
>
> After more thought, I would prefer to keep the check. It is fast/simple, and it
> keeps the hardware from getting into an inconsistent state. Silently dropping
> messages sounds like a poor quality of implementation.
>
If the FIFO becomes full after calling send_data(),  then
last_tx_done() should not only check remote's irq status but also
check that the data has been consumed from the FIFO locally (hence the
check becomes redundant in send_data). Otherwise the last_tx_done is
incomplete.  And you actually end up writing more code (error handling
and resubmission instead of the api managing it all transparently)

> send_data() is documented in mailbox_controller.h as returning EBUSY,
>
error is usually returned for s/w check (like mssg too big for fifo),
not h/w events.

> and I see multiple other mailbox controllers implementing the same or a similar check.
>
That it encourages next developer to repeat, is another reason to do
it right this time. Otherwise, I can live with that check in
send_data.

> If
> that is not the way you intend for the API to work, then please update the
> comments in mailbox_controller.h.
>
Mailbox implementations follow no spec. There may be prudent need to
return from send_data, but practically I haven't come across any(?)
platform that can't do without the check in send_data.

Cheers!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] mailbox: sun6i-msgbox: Remove unneeded FIFO status check
  2020-02-15  4:47         ` Jassi Brar
@ 2020-02-15  6:19           ` Samuel Holland
  0 siblings, 0 replies; 16+ messages in thread
From: Samuel Holland @ 2020-02-15  6:19 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai, Jassi Brar, Rob Herring,
	Mark Rutland, Philipp Zabel, Ondrej Jirman, Vasily Khoruzhick
  Cc: devicetree, linux-sunxi, linux-kernel, linux-arm-kernel, Samuel Holland

A transmit FIFO can never be full, because the mailbox framework
waits until mbox->ops->last_tx_done() succeeds before sending the next
message. sun6i_msgbox_last_tx_done() ensures that the FIFO is empty.

Since the extra check here is unnecessary, remove it.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/mailbox/sun6i-msgbox.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/mailbox/sun6i-msgbox.c b/drivers/mailbox/sun6i-msgbox.c
index 15d6fd522dc5..ccecf2e5941d 100644
--- a/drivers/mailbox/sun6i-msgbox.c
+++ b/drivers/mailbox/sun6i-msgbox.c
@@ -106,12 +106,6 @@ static int sun6i_msgbox_send_data(struct mbox_chan *chan, void *data)
 	if (WARN_ON_ONCE(!(readl(mbox->regs + CTRL_REG(n)) & CTRL_TX(n))))
 		return 0;
 
-	/* We cannot post a new message if the FIFO is full. */
-	if (readl(mbox->regs + FIFO_STAT_REG(n)) & FIFO_STAT_MASK) {
-		mbox_dbg(mbox, "Channel %d busy sending 0x%08x\n", n, msg);
-		return -EBUSY;
-	}
-
 	writel(msg, mbox->regs + MSG_DATA_REG(n));
 	mbox_dbg(mbox, "Channel %d sent 0x%08x\n", n, msg);
 
-- 
2.24.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-02-15  6:20 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-13  5:18 [PATCH v6 0/6] Allwinner sun6i message box support Samuel Holland
2020-01-13  5:18 ` [PATCH v6 1/6] dt-bindings: mailbox: Add a sun6i message box binding Samuel Holland
2020-01-13  9:30   ` Maxime Ripard
2020-01-13 22:38   ` Rob Herring
2020-01-13  5:18 ` [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver Samuel Holland
2020-01-13  9:15   ` Philipp Zabel
2020-02-13  2:02   ` Jassi Brar
2020-02-13  2:18     ` Samuel Holland
2020-02-15  3:48       ` Samuel Holland
2020-02-15  4:47         ` Jassi Brar
2020-02-15  6:19           ` [PATCH] mailbox: sun6i-msgbox: Remove unneeded FIFO status check Samuel Holland
2020-01-13  5:18 ` [PATCH v6 3/6] ARM: dts: sunxi: a83t: Add msgbox node Samuel Holland
2020-01-13  5:18 ` [PATCH v6 4/6] ARM: dts: sunxi: h3/h5: " Samuel Holland
2020-01-13  5:18 ` [PATCH v6 5/6] arm64: dts: allwinner: a64: " Samuel Holland
2020-01-13  5:18 ` [PATCH v6 6/6] arm64: dts: allwinner: h6: " Samuel Holland
2020-02-13  1:43 ` [PATCH v6 0/6] Allwinner sun6i message box support Samuel Holland

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).