[PATCH v5 0/8] Allwinner sun6i message box support

[PATCH v5 0/8] Allwinner sun6i message box support

[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 on the A64, H5,
and H6 SoCs (including specifically this arm_scpi patch on A64 and H6),
and Ondrej Jirman has tested the driver on the A83T (using a similar
patch to arm_scpi).

The change to make the arm_scpi compatible with unidirectional mailbox
controllers is attached to the end of this patch series. While it would
be nice to get this merged too, I don't consider it a prerequisite to
getting the driver merged. And even without the driver, the clock change
(patch #1) can go in at any time.

Thanks,
Samuel

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 (8):
  clk: sunxi-ng: Mark msgbox clocks as critical
  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
  firmware: arm_scpi: Support unidirectional mailbox channels

 .../mailbox/allwinner,sun6i-a31-msgbox.yaml   |  78 ++++
 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/clk/sunxi-ng/ccu-sun50i-a64.c         |   3 +-
 drivers/clk/sunxi-ng/ccu-sun50i-h6.c          |   3 +-
 drivers/clk/sunxi-ng/ccu-sun8i-a23.c          |   3 +-
 drivers/clk/sunxi-ng/ccu-sun8i-a33.c          |   3 +-
 drivers/clk/sunxi-ng/ccu-sun8i-a83t.c         |   3 +-
 drivers/clk/sunxi-ng/ccu-sun8i-h3.c           |   3 +-
 drivers/clk/sunxi-ng/ccu-sun9i-a80.c          |   3 +-
 drivers/firmware/arm_scpi.c                   |  58 ++-
 drivers/mailbox/Kconfig                       |   9 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/sun6i-msgbox.c                | 332 ++++++++++++++++++
 16 files changed, 520 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
 create mode 100644 drivers/mailbox/sun6i-msgbox.c

-- 
2.23.0

[PATCH v5 1/8] clk: sunxi-ng: Mark msgbox clocks as critical

[Samuel Holland]

The msgbox clock is critical because the hardware it controls is shared
between Linux and system firmware. The message box may be used by the
EL3 secure monitor's PSCI implementation. On 64-bit sunxi SoCs, this is
provided by ARM TF-A; 32-bit SoCs use a different implementation. The
secure monitor uses the message box to forward requests to power
management firmware running on a separate CPU.

It is not enough for the secure monitor to enable the clock each time
Linux performs a SMC into EL3, as both the firmware and Linux can run
concurrently on separate CPUs. So it is never safe for Linux to turn
this clock off, and it should be marked as critical.

At this time, such power management firmware only exists for the A64 and
H5 SoCs.  However, it makes sense to take care of all CCU drivers now
for consistency, and to ease the transition in the future once firmware
is ported to the other SoCs.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 3 ++-
 drivers/clk/sunxi-ng/ccu-sun50i-h6.c  | 3 ++-
 drivers/clk/sunxi-ng/ccu-sun8i-a23.c  | 3 ++-
 drivers/clk/sunxi-ng/ccu-sun8i-a33.c  | 3 ++-
 drivers/clk/sunxi-ng/ccu-sun8i-a83t.c | 3 ++-
 drivers/clk/sunxi-ng/ccu-sun8i-h3.c   | 3 ++-
 drivers/clk/sunxi-ng/ccu-sun9i-a80.c  | 3 ++-
 7 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
index 49bd7a4c015c..045121b50da3 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -342,8 +342,9 @@ static SUNXI_CCU_GATE(bus_de_clk,	"bus-de",	"ahb1",
 		      0x064, BIT(12), 0);
 static SUNXI_CCU_GATE(bus_gpu_clk,	"bus-gpu",	"ahb1",
 		      0x064, BIT(20), 0);
+/* Used for communication between firmware components at runtime */
 static SUNXI_CCU_GATE(bus_msgbox_clk,	"bus-msgbox",	"ahb1",
-		      0x064, BIT(21), 0);
+		      0x064, BIT(21), CLK_IS_CRITICAL);
 static SUNXI_CCU_GATE(bus_spinlock_clk,	"bus-spinlock",	"ahb1",
 		      0x064, BIT(22), 0);
 
diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6.c
index f2497d0a4683..91da52ca3c75 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-h6.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6.c
@@ -349,8 +349,9 @@ static SUNXI_CCU_GATE(bus_vp9_clk, "bus-vp9", "psi-ahb1-ahb2",
 static SUNXI_CCU_GATE(bus_dma_clk, "bus-dma", "psi-ahb1-ahb2",
 		      0x70c, BIT(0), 0);
 
+/* Used for communication between firmware components at runtime */
 static SUNXI_CCU_GATE(bus_msgbox_clk, "bus-msgbox", "psi-ahb1-ahb2",
-		      0x71c, BIT(0), 0);
+		      0x71c, BIT(0), CLK_IS_CRITICAL);
 
 static SUNXI_CCU_GATE(bus_spinlock_clk, "bus-spinlock", "psi-ahb1-ahb2",
 		      0x72c, BIT(0), 0);
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-a23.c b/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
index 103aa504f6c8..5a28583f57e2 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
@@ -255,8 +255,9 @@ static SUNXI_CCU_GATE(bus_de_fe_clk,	"bus-de-fe",	"ahb1",
 		      0x064, BIT(14), 0);
 static SUNXI_CCU_GATE(bus_gpu_clk,	"bus-gpu",	"ahb1",
 		      0x064, BIT(20), 0);
+/* Used for communication between firmware components at runtime */
 static SUNXI_CCU_GATE(bus_msgbox_clk,	"bus-msgbox",	"ahb1",
-		      0x064, BIT(21), 0);
+		      0x064, BIT(21), CLK_IS_CRITICAL);
 static SUNXI_CCU_GATE(bus_spinlock_clk,	"bus-spinlock",	"ahb1",
 		      0x064, BIT(22), 0);
 static SUNXI_CCU_GATE(bus_drc_clk,	"bus-drc",	"ahb1",
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-a33.c b/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
index 91838cd11037..50cf3726ef30 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
@@ -267,8 +267,9 @@ static SUNXI_CCU_GATE(bus_de_fe_clk,	"bus-de-fe",	"ahb1",
 		      0x064, BIT(14), 0);
 static SUNXI_CCU_GATE(bus_gpu_clk,	"bus-gpu",	"ahb1",
 		      0x064, BIT(20), 0);
+/* Used for communication between firmware components at runtime */
 static SUNXI_CCU_GATE(bus_msgbox_clk,	"bus-msgbox",	"ahb1",
-		      0x064, BIT(21), 0);
+		      0x064, BIT(21), CLK_IS_CRITICAL);
 static SUNXI_CCU_GATE(bus_spinlock_clk,	"bus-spinlock",	"ahb1",
 		      0x064, BIT(22), 0);
 static SUNXI_CCU_GATE(bus_drc_clk,	"bus-drc",	"ahb1",
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-a83t.c b/drivers/clk/sunxi-ng/ccu-sun8i-a83t.c
index 2b434521c5cc..4ab3a76f4ffa 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-a83t.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-a83t.c
@@ -339,8 +339,9 @@ static SUNXI_CCU_GATE(bus_de_clk,	"bus-de",	"ahb1",
 		      0x064, BIT(12), 0);
 static SUNXI_CCU_GATE(bus_gpu_clk,	"bus-gpu",	"ahb1",
 		      0x064, BIT(20), 0);
+/* Used for communication between firmware components at runtime */
 static SUNXI_CCU_GATE(bus_msgbox_clk,	"bus-msgbox",	"ahb1",
-		      0x064, BIT(21), 0);
+		      0x064, BIT(21), CLK_IS_CRITICAL);
 static SUNXI_CCU_GATE(bus_spinlock_clk,	"bus-spinlock",	"ahb1",
 		      0x064, BIT(22), 0);
 
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
index 6b636362379e..7429d3fe8fb7 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
@@ -273,8 +273,9 @@ static SUNXI_CCU_GATE(bus_de_clk,	"bus-de",	"ahb1",
 		      0x064, BIT(12), 0);
 static SUNXI_CCU_GATE(bus_gpu_clk,	"bus-gpu",	"ahb1",
 		      0x064, BIT(20), 0);
+/* Used for communication between firmware components at runtime */
 static SUNXI_CCU_GATE(bus_msgbox_clk,	"bus-msgbox",	"ahb1",
-		      0x064, BIT(21), 0);
+		      0x064, BIT(21), CLK_IS_CRITICAL);
 static SUNXI_CCU_GATE(bus_spinlock_clk,	"bus-spinlock",	"ahb1",
 		      0x064, BIT(22), 0);
 
diff --git a/drivers/clk/sunxi-ng/ccu-sun9i-a80.c b/drivers/clk/sunxi-ng/ccu-sun9i-a80.c
index ef29582676f6..471e841aabdf 100644
--- a/drivers/clk/sunxi-ng/ccu-sun9i-a80.c
+++ b/drivers/clk/sunxi-ng/ccu-sun9i-a80.c
@@ -748,8 +748,9 @@ static SUNXI_CCU_GATE(bus_usb_clk,	"bus-usb",	"ahb1",
 		      0x584, BIT(1), 0);
 static SUNXI_CCU_GATE(bus_gmac_clk,	"bus-gmac",	"ahb1",
 		      0x584, BIT(17), 0);
+/* Used for communication between firmware components at runtime */
 static SUNXI_CCU_GATE(bus_msgbox_clk,	"bus-msgbox",	"ahb1",
-		      0x584, BIT(21), 0);
+		      0x584, BIT(21), CLK_IS_CRITICAL);
 static SUNXI_CCU_GATE(bus_spinlock_clk,	"bus-spinlock",	"ahb1",
 		      0x584, BIT(22), 0);
 static SUNXI_CCU_GATE(bus_hstimer_clk,	"bus-hstimer",	"ahb1",
-- 
2.23.0

[PATCH v5 2/8] dt-bindings: mailbox: Add a sun6i message box binding

[Samuel Holland]

This mailbox hardware is present in Allwinner sun6i, sun8i, sun9i, and
sun50i SoCs. Add a device tree binding for it. As it has only been
tested on the A83T, A64, H3/H5, and H6 SoCs, only those compatibles are
included.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 .../mailbox/allwinner,sun6i-a31-msgbox.yaml   | 78 +++++++++++++++++++
 1 file changed, 78 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..dd746e07acfd
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
@@ -0,0 +1,78 @@
+# 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:
+    items:
+      - enum:
+          - allwinner,sun8i-a83t-msgbox
+          - allwinner,sun8i-h3-msgbox
+          - allwinner,sun50i-a64-msgbox
+          - allwinner,sun50i-h6-msgbox
+      - const: allwinner,sun6i-a31-msgbox
+
+  reg:
+    items:
+      - description: MMIO register range
+
+  clocks:
+    maxItems: 1
+    description: bus clock
+
+  resets:
+    maxItems: 1
+    description: bus reset
+
+  interrupts:
+    maxItems: 1
+    description: controller interrupt
+
+  '#mbox-cells':
+    const: 1
+
+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

[PATCH v5 3/8] mailbox: sun6i-msgbox: Add a new mailbox driver

[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..7a41e732457c
--- /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(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

[PATCH v5 4/8] ARM: dts: sunxi: a83t: Add msgbox node

[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

[PATCH v5 5/8] ARM: dts: sunxi: h3/h5: Add msgbox node

[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

[PATCH v5 6/8] arm64: dts: allwinner: a64: Add msgbox node

[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 27e48234f1c2..10edb4b59203 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -496,6 +496,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

[PATCH v5 7/8] arm64: dts: allwinner: h6: Add msgbox node

[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 8e5999cd08bb..70ee56ba2091 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -240,6 +240,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

[PATCH v5 8/8] firmware: arm_scpi: Support unidirectional mailbox channels

[Samuel Holland]

Some mailbox controllers have only unidirectional channels, so we need a
pair of them for each SCPI channel. If a mbox-names property is present,
look for "rx" and "tx" mbox channels; otherwise, the existing behavior
is preserved, and a single mbox channel is used for each SCPI channel.

Note that since the mailbox framework only supports a single phandle
with each name (mbox_request_channel_byname always returns the first
one), this new mode only supports a single SCPI channel.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/firmware/arm_scpi.c | 58 +++++++++++++++++++++++++++++--------
 1 file changed, 46 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index a80c331c3a6e..36ff9dd8d0fa 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -231,7 +231,8 @@ struct scpi_xfer {
 
 struct scpi_chan {
 	struct mbox_client cl;
-	struct mbox_chan *chan;
+	struct mbox_chan *rx_chan;
+	struct mbox_chan *tx_chan;
 	void __iomem *tx_payload;
 	void __iomem *rx_payload;
 	struct list_head rx_pending;
@@ -505,7 +506,7 @@ static int scpi_send_message(u8 idx, void *tx_buf, unsigned int tx_len,
 	msg->rx_len = rx_len;
 	reinit_completion(&msg->done);
 
-	ret = mbox_send_message(scpi_chan->chan, msg);
+	ret = mbox_send_message(scpi_chan->tx_chan, msg);
 	if (ret < 0 || !rx_buf)
 		goto out;
 
@@ -854,8 +855,13 @@ static void scpi_free_channels(void *data)
 	struct scpi_drvinfo *info = data;
 	int i;
 
-	for (i = 0; i < info->num_chans; i++)
-		mbox_free_channel(info->channels[i].chan);
+	for (i = 0; i < info->num_chans; i++) {
+		struct scpi_chan *pchan = &info->channels[i];
+
+		if (pchan->tx_chan != pchan->rx_chan)
+			mbox_free_channel(pchan->tx_chan);
+		mbox_free_channel(pchan->rx_chan);
+	}
 }
 
 static int scpi_remove(struct platform_device *pdev)
@@ -903,6 +909,7 @@ static int scpi_probe(struct platform_device *pdev)
 	struct resource res;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
+	bool use_mbox_names = false;
 
 	scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
 	if (!scpi_info)
@@ -916,6 +923,14 @@ static int scpi_probe(struct platform_device *pdev)
 		dev_err(dev, "no mboxes property in '%pOF'\n", np);
 		return -ENODEV;
 	}
+	if (of_get_property(dev->of_node, "mbox-names", NULL)) {
+		use_mbox_names = true;
+		if (count != 2) {
+			dev_err(dev, "need exactly 2 mboxes with mbox-names\n");
+			return -ENODEV;
+		}
+		count /= 2;
+	}
 
 	scpi_info->channels = devm_kcalloc(dev, count, sizeof(struct scpi_chan),
 					   GFP_KERNEL);
@@ -961,15 +976,34 @@ static int scpi_probe(struct platform_device *pdev)
 		mutex_init(&pchan->xfers_lock);
 
 		ret = scpi_alloc_xfer_list(dev, pchan);
-		if (!ret) {
-			pchan->chan = mbox_request_channel(cl, idx);
-			if (!IS_ERR(pchan->chan))
-				continue;
-			ret = PTR_ERR(pchan->chan);
-			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "failed to get channel%d err %d\n",
-					idx, ret);
+		if (ret)
+			return ret;
+
+		if (use_mbox_names) {
+			pchan->rx_chan = mbox_request_channel_byname(cl, "rx");
+			if (IS_ERR(pchan->rx_chan)) {
+				ret = PTR_ERR(pchan->rx_chan);
+				goto fail;
+			}
+			pchan->tx_chan = mbox_request_channel_byname(cl, "tx");
+			if (IS_ERR(pchan->rx_chan)) {
+				ret = PTR_ERR(pchan->tx_chan);
+				goto fail;
+			}
+		} else {
+			pchan->rx_chan = mbox_request_channel(cl, idx);
+			if (IS_ERR(pchan->rx_chan)) {
+				ret = PTR_ERR(pchan->rx_chan);
+				goto fail;
+			}
+			pchan->tx_chan = pchan->rx_chan;
 		}
+		continue;
+
+fail:
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get channel%d err %d\n",
+				idx, ret);
 		return ret;
 	}
 
-- 
2.23.0

Re: [PATCH v5 1/8] clk: sunxi-ng: Mark msgbox clocks as critical

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

On Sat, Dec 14, 2019 at 10:24:48PM -0600, Samuel Holland wrote:
> The msgbox clock is critical because the hardware it controls is shared
> between Linux and system firmware. The message box may be used by the
> EL3 secure monitor's PSCI implementation. On 64-bit sunxi SoCs, this is
> provided by ARM TF-A; 32-bit SoCs use a different implementation. The
> secure monitor uses the message box to forward requests to power
> management firmware running on a separate CPU.
>
> It is not enough for the secure monitor to enable the clock each time
> Linux performs a SMC into EL3, as both the firmware and Linux can run
> concurrently on separate CPUs. So it is never safe for Linux to turn
> this clock off, and it should be marked as critical.
>
> At this time, such power management firmware only exists for the A64 and
> H5 SoCs.  However, it makes sense to take care of all CCU drivers now
> for consistency, and to ease the transition in the future once firmware
> is ported to the other SoCs.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>

This is pretty much the same case than for the AR100 clock though,
right?

I'm still not sure about why we should enable it that clock all the
time, even if you're not using the ARISC.

Maxime

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

Re: [PATCH v5 2/8] dt-bindings: mailbox: Add a sun6i message box binding

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3732 bytes --]

Hi,

On Sat, Dec 14, 2019 at 10:24:49PM -0600, Samuel Holland wrote:
> This mailbox hardware is present in Allwinner sun6i, sun8i, sun9i, and
> sun50i SoCs. Add a device tree binding for it. As it has only been
> tested on the A83T, A64, H3/H5, and H6 SoCs, only those compatibles are
> included.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  .../mailbox/allwinner,sun6i-a31-msgbox.yaml   | 78 +++++++++++++++++++
>  1 file changed, 78 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..dd746e07acfd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
> @@ -0,0 +1,78 @@
> +# 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:
> +     items:
> +      - enum:
> +          - allwinner,sun8i-a83t-msgbox
> +          - allwinner,sun8i-h3-msgbox
> +          - allwinner,sun50i-a64-msgbox
> +          - allwinner,sun50i-h6-msgbox
> +      - const: allwinner,sun6i-a31-msgbox

This will fail for the A31, since it won't have two compatibles but
just one.

You can have something like this if you want to do it with an enum:

compatible:
  oneOf:
    - const: allwinner,sun6i-a31-msgbox
    - items:
      - enum:
        - allwinner,sun8i-a83t-msgbox
        - allwinner,sun8i-h3-msgbox
        - allwinner,sun50i-a64-msgbox
        - allwinner,sun50i-h6-msgbox
      - const: allwinner,sun6i-a31-msgbox

> +  reg:
> +    items:
> +      - description: MMIO register range

There's no need for an obvious description like this.
Just set it to maxItems: 1

> +
> +  clocks:
> +    maxItems: 1
> +    description: bus clock
> +
> +  resets:
> +    maxItems: 1
> +    description: bus reset
> +
> +  interrupts:
> +    maxItems: 1
> +    description: controller interrupt

Ditto, you can drop the description here.

> +  '#mbox-cells':
> +    const: 1

However, you should document what the argument is about?

> +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>;
> +    };

Look good otherwise, thanks!
Maxime

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

Re: [PATCH v5 2/8] dt-bindings: mailbox: Add a sun6i message box binding

[Samuel Holland]

Hi,

On 12/16/19 8:04 AM, Maxime Ripard wrote:
> Hi,
> 
> On Sat, Dec 14, 2019 at 10:24:49PM -0600, Samuel Holland wrote:
>> This mailbox hardware is present in Allwinner sun6i, sun8i, sun9i, and
>> sun50i SoCs. Add a device tree binding for it. As it has only been
>> tested on the A83T, A64, H3/H5, and H6 SoCs, only those compatibles are
>> included.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  .../mailbox/allwinner,sun6i-a31-msgbox.yaml   | 78 +++++++++++++++++++
>>  1 file changed, 78 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..dd746e07acfd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
>> @@ -0,0 +1,78 @@
>> +# 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:
>> +     items:
>> +      - enum:
>> +          - allwinner,sun8i-a83t-msgbox
>> +          - allwinner,sun8i-h3-msgbox
>> +          - allwinner,sun50i-a64-msgbox
>> +          - allwinner,sun50i-h6-msgbox
>> +      - const: allwinner,sun6i-a31-msgbox
> 
> This will fail for the A31, since it won't have two compatibles but
> just one.

You asked me earlier to only include compatibles that had been tested, so I did.
This hasn't been tested on the A31, so there's no A31-only compatible.

> You can have something like this if you want to do it with an enum:
> 
> compatible:
>   oneOf:
>     - const: allwinner,sun6i-a31-msgbox
>     - items:
>       - enum:
>         - allwinner,sun8i-a83t-msgbox
>         - allwinner,sun8i-h3-msgbox
>         - allwinner,sun50i-a64-msgbox
>         - allwinner,sun50i-h6-msgbox
>       - const: allwinner,sun6i-a31-msgbox
> 
>> +  reg:
>> +    items:
>> +      - description: MMIO register range
> 
> There's no need for an obvious description like this.
> Just set it to maxItems: 1

Will do for v6.

>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: bus clock
>> +
>> +  resets:
>> +    maxItems: 1
>> +    description: bus reset
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +    description: controller interrupt
> 
> Ditto, you can drop the description here.

Will do for v6.

>> +  '#mbox-cells':
>> +    const: 1
> 
> However, you should document what the argument is about?

Ok. "Number of cells used to encode a mailbox specifier" should work.

>> +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>;
>> +    };
> 
> Look good otherwise, thanks!
> Maxime
> 

Thanks,
Samuel

Re: [PATCH v5 8/8] firmware: arm_scpi: Support unidirectional mailbox channels

[Sudeep Holla]

On Sat, Dec 14, 2019 at 10:24:55PM -0600, Samuel Holland wrote:
> Some mailbox controllers have only unidirectional channels, so we need a
> pair of them for each SCPI channel. If a mbox-names property is present,
> look for "rx" and "tx" mbox channels; otherwise, the existing behavior
> is preserved, and a single mbox channel is used for each SCPI channel.
>

I need to look at the bindings again, but I think you must update it.

> Note that since the mailbox framework only supports a single phandle
> with each name (mbox_request_channel_byname always returns the first
> one), this new mode only supports a single SCPI channel.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/firmware/arm_scpi.c | 58 +++++++++++++++++++++++++++++--------
>  1 file changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index a80c331c3a6e..36ff9dd8d0fa 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -231,7 +231,8 @@ struct scpi_xfer {
>
>  struct scpi_chan {
>  	struct mbox_client cl;
> -	struct mbox_chan *chan;
> +	struct mbox_chan *rx_chan;
> +	struct mbox_chan *tx_chan;
>  	void __iomem *tx_payload;
>  	void __iomem *rx_payload;
>  	struct list_head rx_pending;
> @@ -505,7 +506,7 @@ static int scpi_send_message(u8 idx, void *tx_buf, unsigned int tx_len,
>  	msg->rx_len = rx_len;
>  	reinit_completion(&msg->done);
>
> -	ret = mbox_send_message(scpi_chan->chan, msg);
> +	ret = mbox_send_message(scpi_chan->tx_chan, msg);
>  	if (ret < 0 || !rx_buf)
>  		goto out;
>
> @@ -854,8 +855,13 @@ static void scpi_free_channels(void *data)
>  	struct scpi_drvinfo *info = data;
>  	int i;
>
> -	for (i = 0; i < info->num_chans; i++)
> -		mbox_free_channel(info->channels[i].chan);
> +	for (i = 0; i < info->num_chans; i++) {
> +		struct scpi_chan *pchan = &info->channels[i];
> +
> +		if (pchan->tx_chan != pchan->rx_chan)
> +			mbox_free_channel(pchan->tx_chan);
> +		mbox_free_channel(pchan->rx_chan);

I think mbox_free_channel handles !chan->cl, so just do unconditionally.

> +	}
>  }
>
>  static int scpi_remove(struct platform_device *pdev)
> @@ -903,6 +909,7 @@ static int scpi_probe(struct platform_device *pdev)
>  	struct resource res;
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
> +	bool use_mbox_names = false;
>
>  	scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
>  	if (!scpi_info)
> @@ -916,6 +923,14 @@ static int scpi_probe(struct platform_device *pdev)
>  		dev_err(dev, "no mboxes property in '%pOF'\n", np);
>  		return -ENODEV;
>  	}
> +	if (of_get_property(dev->of_node, "mbox-names", NULL)) {

So, for this platform, this is required and must fail if it is not found.
But instead your check here is optional and may end up populating 2
scpi channels instead of one. I would suggest to make it required and
fail for it based on some compatible, otherwise you are not checking
correctly.

Something like:
        if (of_match_device(blah_blah_of_match, &pdev->dev)) {
                use_mbox_names = true;
		count = 1;
	}


> +		use_mbox_names = true;
> +		if (count != 2) {
> +			dev_err(dev, "need exactly 2 mboxes with mbox-names\n");
> +			return -ENODEV;
> +		}
> +		count /= 2;
> +	}

Ah, OK then you must update the binding as it's different usage of mailbox. 

General query, not related to this patch: If you are in process of
implementing the firmware, I suggest to move to SCMI protocol than this
one if not too late. This specification is deprecated and no longer
updated while SCMI is actively developed and maintained. However it has
slightly different notion of tx and rx and the way the specification
commands which messages are synchronous and which can be async/delayed.

--
Regards,
Sudeep

Re: [PATCH v5 2/8] dt-bindings: mailbox: Add a sun6i message box binding

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3328 bytes --]

Hi,

On Mon, Dec 16, 2019 at 01:45:08PM -0600, Samuel Holland wrote:
> On 12/16/19 8:04 AM, Maxime Ripard wrote:
> > Hi,
> >
> > On Sat, Dec 14, 2019 at 10:24:49PM -0600, Samuel Holland wrote:
> >> This mailbox hardware is present in Allwinner sun6i, sun8i, sun9i, and
> >> sun50i SoCs. Add a device tree binding for it. As it has only been
> >> tested on the A83T, A64, H3/H5, and H6 SoCs, only those compatibles are
> >> included.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>  .../mailbox/allwinner,sun6i-a31-msgbox.yaml   | 78 +++++++++++++++++++
> >>  1 file changed, 78 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..dd746e07acfd
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
> >> @@ -0,0 +1,78 @@
> >> +# 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:
> >> +     items:
> >> +      - enum:
> >> +          - allwinner,sun8i-a83t-msgbox
> >> +          - allwinner,sun8i-h3-msgbox
> >> +          - allwinner,sun50i-a64-msgbox
> >> +          - allwinner,sun50i-h6-msgbox
> >> +      - const: allwinner,sun6i-a31-msgbox
> >
> > This will fail for the A31, since it won't have two compatibles but
> > just one.
>
> You asked me earlier to only include compatibles that had been tested, so I did.
> This hasn't been tested on the A31, so there's no A31-only compatible.

The binding is the description, and that description already matches
the A31 compatible, and it's completely abstract from whether we have
software to support it.

We have bindings that have no drivers in the tree for example, but are
just there to make the representation complete.

In this case, we shouldn't enable it in the A31 DTSI, but we should
document the binding properly.

> >> +  '#mbox-cells':
> >> +    const: 1
> >
> > However, you should document what the argument is about?
>
> Ok. "Number of cells used to encode a mailbox specifier" should work.

It's not really what I meant, what I meant is what is is that
specifier you're talking about here. The customers' mailbox properties
will have a phandle and a number: what is this number representing?

Maxime

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

Re: [PATCH v5 3/8] mailbox: sun6i-msgbox: Add a new mailbox driver

[Philipp Zabel]

On Sat, 2019-12-14 at 22:24 -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..7a41e732457c
> --- /dev/null
> +++ b/drivers/mailbox/sun6i-msgbox.c
> @@ -0,0 +1,332 @@
[...]
> +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(dev, NULL);

Please use devm_reset_control_get_exclusive() explicitly.

regards
Philipp