[PATCH 0/3] Allwinner sunxi message box support

[PATCH 0/3] Allwinner sunxi message box support

[Samuel Holland]

This series adds support for the "hardware message box" in recent
Allwinner sunxi 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.

This driver has been tested with the base arm_scpi driver (which sends
the SCPI_CMD_SCPI_CAPABILITIES command at probe time) and an in-progress
firmware application running on the ARISC CPU. Because no firmware for
the other end of the mailbox is complete at the moment, I have not
included changes to the SoC device trees.

The second patch in the series fixes an issue in the mailbox framework
discovered while writing this driver. It is a re-send of a patch I
originally sent by itself in January.

Samuel Holland (3):
  dt-bindings: Add a binding for the sunxi message box
  mailbox: Avoid NULL dereference in mbox_chan_received_data
  mailbox: sunxi-msgbox: Add a new mailbox driver

 .../devicetree/bindings/mailbox/sunxi-msgbox.txt   |  40 +++
 drivers/mailbox/Kconfig                            |   7 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/mailbox.c                          |   6 +-
 drivers/mailbox/sunxi-msgbox.c                     | 323 +++++++++++++++++++++
 5 files changed, 376 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
 create mode 100644 drivers/mailbox/sunxi-msgbox.c

-- 
2.13.6

[PATCH 0/3] Allwinner sunxi message box support

[Samuel Holland]

This series adds support for the "hardware message box" in recent
Allwinner sunxi 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.

This driver has been tested with the base arm_scpi driver (which sends
the SCPI_CMD_SCPI_CAPABILITIES command at probe time) and an in-progress
firmware application running on the ARISC CPU. Because no firmware for
the other end of the mailbox is complete at the moment, I have not
included changes to the SoC device trees.

The second patch in the series fixes an issue in the mailbox framework
discovered while writing this driver. It is a re-send of a patch I
originally sent by itself in January.

Samuel Holland (3):
  dt-bindings: Add a binding for the sunxi message box
  mailbox: Avoid NULL dereference in mbox_chan_received_data
  mailbox: sunxi-msgbox: Add a new mailbox driver

 .../devicetree/bindings/mailbox/sunxi-msgbox.txt   |  40 +++
 drivers/mailbox/Kconfig                            |   7 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/mailbox.c                          |   6 +-
 drivers/mailbox/sunxi-msgbox.c                     | 323 +++++++++++++++++++++
 5 files changed, 376 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
 create mode 100644 drivers/mailbox/sunxi-msgbox.c

-- 
2.13.6

[PATCH 1/3] dt-bindings: Add a binding for the sunxi message box

[Samuel Holland]

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

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

diff --git a/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
new file mode 100644
index 000000000000..3b3ed7f870a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
@@ -0,0 +1,40 @@
+Allwinner sunxi Message Box
+===========================
+
+The hardware message box on sunxi SoCs is a two-user mailbox controller
+containing 8 unidirectional FIFOs bonded into 4 bidirectional mailbox channels.
+An interrupt is raised for received messages, but software must poll to know
+when a transmitted message has been acknowledged by the remote user.
+
+Refer to ./mailbox.txt for generic information about mailbox device-tree
+bindings.
+
+Mailbox Device Node:
+====================
+
+Required properties:
+--------------------
+- compatible:		Must be "allwinner,sunxi-msgbox".
+- reg:			Contains the mailbox register address range (base
+			address and length).
+- clocks:		phandle for the clock controller and specifier.
+- clock-names:		Must be "bus".
+- resets:		phandle for the reset controller and specifier.
+- reset-names:		Must be "bus".
+- interrupts:		Contains interrupt information for the mailbox.
+- #mbox-cells		Must be 2 - the indexes of the transmit and receive
+			channels, respectively.
+
+Example:
+--------
+
+	msgbox: mailbox@1c17000 {
+		compatible = "allwinner,sunxi-msgbox";
+		reg = <0x01c17000 0x1000>;
+		clocks = <&ccu CLK_BUS_MSGBOX>;
+		clock-names = "bus";
+		resets = <&ccu RST_BUS_MSGBOX>;
+		reset-names = "bus";
+		interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
+		#mbox-cells = <2>;
+	};
-- 
2.13.6

[PATCH 1/3] dt-bindings: Add a binding for the sunxi message box

[Samuel Holland]

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

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

diff --git a/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
new file mode 100644
index 000000000000..3b3ed7f870a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
@@ -0,0 +1,40 @@
+Allwinner sunxi Message Box
+===========================
+
+The hardware message box on sunxi SoCs is a two-user mailbox controller
+containing 8 unidirectional FIFOs bonded into 4 bidirectional mailbox channels.
+An interrupt is raised for received messages, but software must poll to know
+when a transmitted message has been acknowledged by the remote user.
+
+Refer to ./mailbox.txt for generic information about mailbox device-tree
+bindings.
+
+Mailbox Device Node:
+====================
+
+Required properties:
+--------------------
+- compatible:		Must be "allwinner,sunxi-msgbox".
+- reg:			Contains the mailbox register address range (base
+			address and length).
+- clocks:		phandle for the clock controller and specifier.
+- clock-names:		Must be "bus".
+- resets:		phandle for the reset controller and specifier.
+- reset-names:		Must be "bus".
+- interrupts:		Contains interrupt information for the mailbox.
+- #mbox-cells		Must be 2 - the indexes of the transmit and receive
+			channels, respectively.
+
+Example:
+--------
+
+	msgbox: mailbox at 1c17000 {
+		compatible = "allwinner,sunxi-msgbox";
+		reg = <0x01c17000 0x1000>;
+		clocks = <&ccu CLK_BUS_MSGBOX>;
+		clock-names = "bus";
+		resets = <&ccu RST_BUS_MSGBOX>;
+		reset-names = "bus";
+		interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
+		#mbox-cells = <2>;
+	};
-- 
2.13.6

[PATCH 2/3] mailbox: Avoid NULL dereference in mbox_chan_received_data

[Samuel Holland]

If a reception IRQ is pending when a mailbox channel is shut down (for
example, if the controller uses threaded interrupts), it is possible for
mbox_chan_received_data to be called while chan->cl is NULL.

This was found while developing a mailbox controller driver for use with
SCPI. The SCPI protocol driver frees its mailbox channel during probing
if the SCP firmware does not respond within a specified timeout. In this
case, if the SCP firmware takes slightly too long to respond,
mbox_chan_received_data races with mbox_free_channel clearing chan->cl.

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

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 674b35f402f5..a0258d8672d5 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -152,9 +152,11 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
  */
 void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
 {
+	struct mbox_client *cl = READ_ONCE(chan->cl);
+
 	/* No buffering the received data */
-	if (chan->cl->rx_callback)
-		chan->cl->rx_callback(chan->cl, mssg);
+	if (cl && cl->rx_callback)
+		cl->rx_callback(cl, mssg);
 }
 EXPORT_SYMBOL_GPL(mbox_chan_received_data);
 
-- 
2.13.6

[PATCH 2/3] mailbox: Avoid NULL dereference in mbox_chan_received_data

[Samuel Holland]

If a reception IRQ is pending when a mailbox channel is shut down (for
example, if the controller uses threaded interrupts), it is possible for
mbox_chan_received_data to be called while chan->cl is NULL.

This was found while developing a mailbox controller driver for use with
SCPI. The SCPI protocol driver frees its mailbox channel during probing
if the SCP firmware does not respond within a specified timeout. In this
case, if the SCP firmware takes slightly too long to respond,
mbox_chan_received_data races with mbox_free_channel clearing chan->cl.

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

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 674b35f402f5..a0258d8672d5 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -152,9 +152,11 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
  */
 void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
 {
+	struct mbox_client *cl = READ_ONCE(chan->cl);
+
 	/* No buffering the received data */
-	if (chan->cl->rx_callback)
-		chan->cl->rx_callback(chan->cl, mssg);
+	if (cl && cl->rx_callback)
+		cl->rx_callback(cl, mssg);
 }
 EXPORT_SYMBOL_GPL(mbox_chan_received_data);
 
-- 
2.13.6

[PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Samuel Holland]

Recent Allwinner sunxi SoCs contain a "hardware message box" used for
communication between the ARM CPUs and the ARISC management processor
(the platform's equivalent of the ARM SCP). Add a driver for it, so it
can be used for SCPI or other communication protocols.

The hardware contains 8 unidirectional 4-message-deep FIFOs. To fit the
mailbox framework API, this driver combines them into 4 bidirectional
pairs of channels, and only allows one message in each channel to be
queued at a time.

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

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ba2f1525f4ee..d6951b52498f 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -171,4 +171,11 @@ config BCM_FLEXRM_MBOX
 	  Mailbox implementation of the Broadcom FlexRM ring manager,
 	  which provides access to various offload engines on Broadcom
 	  SoCs. Say Y here if you want to use the Broadcom FlexRM.
+
+config SUNXI_MSGBOX
+	bool "Allwinner sunxi Message Box"
+	depends on ARCH_SUNXI
+	help
+	  Mailbox driver for the hardware message box used on Allwinner sunxi
+	  SoCs to communicate with the SCP.
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 4896f8dcae95..37a3e8f6a0eb 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -36,3 +36,5 @@ obj-$(CONFIG_BCM_FLEXRM_MBOX)	+= bcm-flexrm-mailbox.o
 obj-$(CONFIG_QCOM_APCS_IPC)	+= qcom-apcs-ipc-mailbox.o
 
 obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
+
+obj-$(CONFIG_SUNXI_MSGBOX)	+= sunxi-msgbox.o
diff --git a/drivers/mailbox/sunxi-msgbox.c b/drivers/mailbox/sunxi-msgbox.c
new file mode 100644
index 000000000000..16ff7aacfc55
--- /dev/null
+++ b/drivers/mailbox/sunxi-msgbox.c
@@ -0,0 +1,323 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 Samuel Holland <samuel@sholland.org>
+ *
+ * This driver was based on drivers/mailbox/bcm2835-mailbox.c and
+ * drivers/mailbox/rockchip-mailbox.c.
+ */
+
+#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>
+
+/*
+ * The message box hardware provides 8 unidirectional channels. As the mailbox
+ * framework expects them to be bidirectional, create virtual channels out of
+ * pairs of opposite-direction hardware channels. The first channel in each pair
+ * is set up for AP->SCP communication, and the second channel is set up for
+ * SCP->AP transmission.
+ */
+#define NUM_CHANS		4
+
+/* These macros take a virtual channel number. */
+#define CTRL_REG(n)		(0x0000 + 0x4 * ((n) / 2))
+#define CTRL_MASK(n)		(0x1111 << 16 * ((n) % 2))
+#define CTRL_SET(n)		(0x0110 << 16 * ((n) % 2))
+
+#define IRQ_EN_REG		0x0060
+#define IRQ_STATUS_REG		0x0070
+#define RX_IRQ(n)		BIT(2 + 4 * (n))
+#define TX_IRQ(n)		BIT(1 + 4 * (n))
+
+#define REMOTE_IRQ_EN_REG	0x0040
+#define REMOTE_IRQ_STATUS_REG	0x0050
+#define REMOTE_RX_IRQ(n)	BIT(0 + 4 * (n))
+#define REMOTE_TX_IRQ(n)	BIT(3 + 4 * (n))
+
+#define RX_FIFO_STATUS_REG(n)	(0x0104 + 0x8 * (n))
+#define TX_FIFO_STATUS_REG(n)	(0x0100 + 0x8 * (n))
+#define FIFO_STATUS_MASK	BIT(0)
+
+#define RX_MSG_STATUS_REG(n)	(0x0144 + 0x8 * (n))
+#define TX_MSG_STATUS_REG(n)	(0x0140 + 0x8 * (n))
+#define MSG_STATUS_MASK		GENMASK(2, 0)
+
+#define RX_MSG_DATA_REG(n)	(0x0184 + 0x8 * (n))
+#define TX_MSG_DATA_REG(n)	(0x0180 + 0x8 * (n))
+
+struct sunxi_msgbox {
+	struct mbox_controller controller;
+	spinlock_t lock;
+	void __iomem *regs;
+};
+
+static bool sunxi_msgbox_last_tx_done(struct mbox_chan *chan);
+static bool sunxi_msgbox_peek_data(struct mbox_chan *chan);
+
+static inline int channel_number(struct mbox_chan *chan)
+{
+	return chan - chan->mbox->chans;
+}
+
+static inline struct sunxi_msgbox *channel_to_msgbox(struct mbox_chan *chan)
+{
+	return container_of(chan->mbox, struct sunxi_msgbox, controller);
+}
+
+static irqreturn_t sunxi_msgbox_irq(int irq, void *dev_id)
+{
+	struct mbox_chan *chan;
+	struct sunxi_msgbox *mbox = dev_id;
+	int n;
+	uint32_t msg, reg;
+
+	reg = readl(mbox->regs + IRQ_STATUS_REG);
+	for (n = 0; n < NUM_CHANS; ++n) {
+		if (!(reg & RX_IRQ(n)))
+			continue;
+		chan = &mbox->controller.chans[n];
+		while (sunxi_msgbox_peek_data(chan)) {
+			msg = readl(mbox->regs + RX_MSG_DATA_REG(n));
+			dev_dbg(mbox->controller.dev,
+				"Received 0x%08x on channel %d\n", msg, n);
+			mbox_chan_received_data(chan, &msg);
+		}
+		/* Clear the pending interrupt once the FIFO is empty. */
+		writel(RX_IRQ(n), mbox->regs + IRQ_STATUS_REG);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int sunxi_msgbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct sunxi_msgbox *mbox = channel_to_msgbox(chan);
+	int n = channel_number(chan);
+	uint32_t msg = *(uint32_t *)data;
+
+	if (!sunxi_msgbox_last_tx_done(chan)) {
+		dev_dbg(mbox->controller.dev,
+			"Busy sending 0x%08x on channel %d\n", msg, n);
+		return -EBUSY;
+	}
+	writel(msg, mbox->regs + TX_MSG_DATA_REG(n));
+	dev_dbg(mbox->controller.dev,
+		"Sent 0x%08x on channel %d\n", msg, n);
+
+	return 0;
+}
+
+static int sunxi_msgbox_startup(struct mbox_chan *chan)
+{
+	struct sunxi_msgbox *mbox = channel_to_msgbox(chan);
+	int n = channel_number(chan);
+	uint32_t reg;
+
+	/* Ensure FIFO directions are set properly. */
+	spin_lock(&mbox->lock);
+	reg = readl(mbox->regs + CTRL_REG(n));
+	writel((reg & ~CTRL_MASK(n)) | CTRL_SET(n), mbox->regs + CTRL_REG(n));
+	spin_unlock(&mbox->lock);
+
+	/* Clear existing messages in the receive FIFO. */
+	while (sunxi_msgbox_peek_data(chan))
+		readl(mbox->regs + RX_MSG_DATA_REG(n));
+
+	/* Clear and enable the receive interrupt. */
+	spin_lock(&mbox->lock);
+	reg = readl(mbox->regs + IRQ_STATUS_REG);
+	writel(reg | RX_IRQ(n), mbox->regs + IRQ_STATUS_REG);
+	reg = readl(mbox->regs + IRQ_EN_REG);
+	writel(reg | RX_IRQ(n), mbox->regs + IRQ_EN_REG);
+	spin_unlock(&mbox->lock);
+
+	dev_dbg(mbox->controller.dev, "Startup channel %d\n", n);
+
+	return 0;
+}
+
+static void sunxi_msgbox_shutdown(struct mbox_chan *chan)
+{
+	struct sunxi_msgbox *mbox = channel_to_msgbox(chan);
+	int n = channel_number(chan);
+	uint32_t reg;
+
+	/* Disable the receive interrupt. */
+	spin_lock(&mbox->lock);
+	reg = readl(mbox->regs + IRQ_EN_REG);
+	writel(reg & ~RX_IRQ(n), mbox->regs + IRQ_EN_REG);
+	spin_unlock(&mbox->lock);
+
+	dev_dbg(mbox->controller.dev, "Shutdown channel %d\n", n);
+}
+
+static bool sunxi_msgbox_last_tx_done(struct mbox_chan *chan)
+{
+	struct sunxi_msgbox *mbox = channel_to_msgbox(chan);
+	int n = channel_number(chan);
+
+	/*
+	 * The message box hardware allows us to snoop on the other user's IRQ
+	 * statuses. Consider a message to be acknowledged when the reception
+	 * IRQ for that channel is cleared. As the hardware only allows clearing
+	 * the IRQ for a channel when the FIFO is empty, this still ensures that
+	 * the message has actually been read. Compared to checking the number
+	 * of messages in the FIFO, it also gives the receiver an opportunity to
+	 * perform minimal message handling (such as recording extra information
+	 * from a shared memory buffer) before acknowledging a message.
+	 */
+	return !(readl(mbox->regs + REMOTE_IRQ_STATUS_REG) & REMOTE_RX_IRQ(n));
+}
+
+static bool sunxi_msgbox_peek_data(struct mbox_chan *chan)
+{
+	struct sunxi_msgbox *mbox = channel_to_msgbox(chan);
+	int n = channel_number(chan);
+
+	return (readl(mbox->regs + RX_MSG_STATUS_REG(n)) & MSG_STATUS_MASK) > 0;
+}
+
+static const struct mbox_chan_ops sunxi_msgbox_chan_ops = {
+	.send_data	= sunxi_msgbox_send_data,
+	.startup	= sunxi_msgbox_startup,
+	.shutdown	= sunxi_msgbox_shutdown,
+	.last_tx_done	= sunxi_msgbox_last_tx_done,
+	.peek_data	= sunxi_msgbox_peek_data,
+};
+
+static struct mbox_chan *sunxi_msgbox_index_xlate(struct mbox_controller *mbox,
+	const struct of_phandle_args *sp)
+{
+	if (sp->args_count != 2)
+		return NULL;
+	/* Enforce this driver's assumed physical-to-virtual channel mapping. */
+	if ((sp->args[0] % 2) || (sp->args[1] != sp->args[0] + 1) ||
+	    (sp->args[0] > (NUM_CHANS - 1) * 2))
+		return NULL;
+
+	return &mbox->chans[sp->args[0] / 2];
+}
+
+static int sunxi_msgbox_probe(struct platform_device *pdev)
+{
+	struct clk *clk;
+	struct device *dev = &pdev->dev;
+	struct mbox_chan *chans;
+	struct reset_control *rst;
+	struct resource *res;
+	struct sunxi_msgbox *mbox;
+	int 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;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	mbox->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(mbox->regs))
+		return PTR_ERR(mbox->regs);
+
+	clk = devm_clk_get(dev, "bus");
+	if (IS_ERR(clk)) {
+		dev_err(dev, "Failed to get bus clock\n");
+		return PTR_ERR(clk);
+	}
+
+	rst = devm_reset_control_get(dev, "bus");
+	if (IS_ERR(rst)) {
+		dev_err(dev, "Failed to get bus reset\n");
+		return PTR_ERR(rst);
+	}
+
+	/*
+	 * The failure path should not disable the clock or assert the reset,
+	 * because the PSCI implementation in firmware relies on this device
+	 * being functional. Claiming the clock in this driver is required to
+	 * prevent Linux from turning it off.
+	 */
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = reset_control_deassert(rst);
+	if (ret) {
+		dev_err(dev, "Failed to deassert bus reset: %d\n", ret);
+		return ret;
+	}
+
+	/* Disable all interrupts. */
+	writel(0, mbox->regs + IRQ_EN_REG);
+
+	ret = devm_request_irq(dev, irq_of_parse_and_map(dev->of_node, 0),
+			       sunxi_msgbox_irq, 0, dev_name(dev), mbox);
+	if (ret) {
+		dev_err(dev, "Failed to register IRQ handler: %d\n", ret);
+		return ret;
+	}
+
+	mbox->controller.dev = dev;
+	mbox->controller.ops = &sunxi_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;
+	mbox->controller.of_xlate = sunxi_msgbox_index_xlate;
+
+	spin_lock_init(&mbox->lock);
+	platform_set_drvdata(pdev, mbox);
+
+	ret = mbox_controller_register(&mbox->controller);
+	if (ret)
+		dev_err(dev, "Failed to register mailbox: %d\n", ret);
+
+	return ret;
+}
+
+static int sunxi_msgbox_remove(struct platform_device *pdev)
+{
+	struct sunxi_msgbox *mbox = platform_get_drvdata(pdev);
+	mbox_controller_unregister(&mbox->controller);
+
+	return 0;
+}
+
+static const struct of_device_id sunxi_msgbox_of_match[] = {
+	{ .compatible = "allwinner,sunxi-msgbox", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sunxi_msgbox_of_match);
+
+static struct platform_driver sunxi_msgbox_driver = {
+	.driver = {
+		.name = "sunxi-msgbox",
+		.of_match_table = sunxi_msgbox_of_match,
+	},
+	.probe		= sunxi_msgbox_probe,
+	.remove		= sunxi_msgbox_remove,
+};
+module_platform_driver(sunxi_msgbox_driver);
+
+MODULE_AUTHOR("Samuel Holland <samuel@sholland.org>");
+MODULE_DESCRIPTION("Allwinner sunxi Message Box");
+MODULE_LICENSE("GPL v2");
-- 
2.13.6

[PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Samuel Holland]

Recent Allwinner sunxi SoCs contain a "hardware message box" used for
communication between the ARM CPUs and the ARISC management processor
(the platform's equivalent of the ARM SCP). Add a driver for it, so it
can be used for SCPI or other communication protocols.

The hardware contains 8 unidirectional 4-message-deep FIFOs. To fit the
mailbox framework API, this driver combines them into 4 bidirectional
pairs of channels, and only allows one message in each channel to be
queued at a time.

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

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ba2f1525f4ee..d6951b52498f 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -171,4 +171,11 @@ config BCM_FLEXRM_MBOX
 	  Mailbox implementation of the Broadcom FlexRM ring manager,
 	  which provides access to various offload engines on Broadcom
 	  SoCs. Say Y here if you want to use the Broadcom FlexRM.
+
+config SUNXI_MSGBOX
+	bool "Allwinner sunxi Message Box"
+	depends on ARCH_SUNXI
+	help
+	  Mailbox driver for the hardware message box used on Allwinner sunxi
+	  SoCs to communicate with the SCP.
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 4896f8dcae95..37a3e8f6a0eb 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -36,3 +36,5 @@ obj-$(CONFIG_BCM_FLEXRM_MBOX)	+= bcm-flexrm-mailbox.o
 obj-$(CONFIG_QCOM_APCS_IPC)	+= qcom-apcs-ipc-mailbox.o
 
 obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
+
+obj-$(CONFIG_SUNXI_MSGBOX)	+= sunxi-msgbox.o
diff --git a/drivers/mailbox/sunxi-msgbox.c b/drivers/mailbox/sunxi-msgbox.c
new file mode 100644
index 000000000000..16ff7aacfc55
--- /dev/null
+++ b/drivers/mailbox/sunxi-msgbox.c
@@ -0,0 +1,323 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 Samuel Holland <samuel@sholland.org>
+ *
+ * This driver was based on drivers/mailbox/bcm2835-mailbox.c and
+ * drivers/mailbox/rockchip-mailbox.c.
+ */
+
+#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>
+
+/*
+ * The message box hardware provides 8 unidirectional channels. As the mailbox
+ * framework expects them to be bidirectional, create virtual channels out of
+ * pairs of opposite-direction hardware channels. The first channel in each pair
+ * is set up for AP->SCP communication, and the second channel is set up for
+ * SCP->AP transmission.
+ */
+#define NUM_CHANS		4
+
+/* These macros take a virtual channel number. */
+#define CTRL_REG(n)		(0x0000 + 0x4 * ((n) / 2))
+#define CTRL_MASK(n)		(0x1111 << 16 * ((n) % 2))
+#define CTRL_SET(n)		(0x0110 << 16 * ((n) % 2))
+
+#define IRQ_EN_REG		0x0060
+#define IRQ_STATUS_REG		0x0070
+#define RX_IRQ(n)		BIT(2 + 4 * (n))
+#define TX_IRQ(n)		BIT(1 + 4 * (n))
+
+#define REMOTE_IRQ_EN_REG	0x0040
+#define REMOTE_IRQ_STATUS_REG	0x0050
+#define REMOTE_RX_IRQ(n)	BIT(0 + 4 * (n))
+#define REMOTE_TX_IRQ(n)	BIT(3 + 4 * (n))
+
+#define RX_FIFO_STATUS_REG(n)	(0x0104 + 0x8 * (n))
+#define TX_FIFO_STATUS_REG(n)	(0x0100 + 0x8 * (n))
+#define FIFO_STATUS_MASK	BIT(0)
+
+#define RX_MSG_STATUS_REG(n)	(0x0144 + 0x8 * (n))
+#define TX_MSG_STATUS_REG(n)	(0x0140 + 0x8 * (n))
+#define MSG_STATUS_MASK		GENMASK(2, 0)
+
+#define RX_MSG_DATA_REG(n)	(0x0184 + 0x8 * (n))
+#define TX_MSG_DATA_REG(n)	(0x0180 + 0x8 * (n))
+
+struct sunxi_msgbox {
+	struct mbox_controller controller;
+	spinlock_t lock;
+	void __iomem *regs;
+};
+
+static bool sunxi_msgbox_last_tx_done(struct mbox_chan *chan);
+static bool sunxi_msgbox_peek_data(struct mbox_chan *chan);
+
+static inline int channel_number(struct mbox_chan *chan)
+{
+	return chan - chan->mbox->chans;
+}
+
+static inline struct sunxi_msgbox *channel_to_msgbox(struct mbox_chan *chan)
+{
+	return container_of(chan->mbox, struct sunxi_msgbox, controller);
+}
+
+static irqreturn_t sunxi_msgbox_irq(int irq, void *dev_id)
+{
+	struct mbox_chan *chan;
+	struct sunxi_msgbox *mbox = dev_id;
+	int n;
+	uint32_t msg, reg;
+
+	reg = readl(mbox->regs + IRQ_STATUS_REG);
+	for (n = 0; n < NUM_CHANS; ++n) {
+		if (!(reg & RX_IRQ(n)))
+			continue;
+		chan = &mbox->controller.chans[n];
+		while (sunxi_msgbox_peek_data(chan)) {
+			msg = readl(mbox->regs + RX_MSG_DATA_REG(n));
+			dev_dbg(mbox->controller.dev,
+				"Received 0x%08x on channel %d\n", msg, n);
+			mbox_chan_received_data(chan, &msg);
+		}
+		/* Clear the pending interrupt once the FIFO is empty. */
+		writel(RX_IRQ(n), mbox->regs + IRQ_STATUS_REG);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int sunxi_msgbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct sunxi_msgbox *mbox = channel_to_msgbox(chan);
+	int n = channel_number(chan);
+	uint32_t msg = *(uint32_t *)data;
+
+	if (!sunxi_msgbox_last_tx_done(chan)) {
+		dev_dbg(mbox->controller.dev,
+			"Busy sending 0x%08x on channel %d\n", msg, n);
+		return -EBUSY;
+	}
+	writel(msg, mbox->regs + TX_MSG_DATA_REG(n));
+	dev_dbg(mbox->controller.dev,
+		"Sent 0x%08x on channel %d\n", msg, n);
+
+	return 0;
+}
+
+static int sunxi_msgbox_startup(struct mbox_chan *chan)
+{
+	struct sunxi_msgbox *mbox = channel_to_msgbox(chan);
+	int n = channel_number(chan);
+	uint32_t reg;
+
+	/* Ensure FIFO directions are set properly. */
+	spin_lock(&mbox->lock);
+	reg = readl(mbox->regs + CTRL_REG(n));
+	writel((reg & ~CTRL_MASK(n)) | CTRL_SET(n), mbox->regs + CTRL_REG(n));
+	spin_unlock(&mbox->lock);
+
+	/* Clear existing messages in the receive FIFO. */
+	while (sunxi_msgbox_peek_data(chan))
+		readl(mbox->regs + RX_MSG_DATA_REG(n));
+
+	/* Clear and enable the receive interrupt. */
+	spin_lock(&mbox->lock);
+	reg = readl(mbox->regs + IRQ_STATUS_REG);
+	writel(reg | RX_IRQ(n), mbox->regs + IRQ_STATUS_REG);
+	reg = readl(mbox->regs + IRQ_EN_REG);
+	writel(reg | RX_IRQ(n), mbox->regs + IRQ_EN_REG);
+	spin_unlock(&mbox->lock);
+
+	dev_dbg(mbox->controller.dev, "Startup channel %d\n", n);
+
+	return 0;
+}
+
+static void sunxi_msgbox_shutdown(struct mbox_chan *chan)
+{
+	struct sunxi_msgbox *mbox = channel_to_msgbox(chan);
+	int n = channel_number(chan);
+	uint32_t reg;
+
+	/* Disable the receive interrupt. */
+	spin_lock(&mbox->lock);
+	reg = readl(mbox->regs + IRQ_EN_REG);
+	writel(reg & ~RX_IRQ(n), mbox->regs + IRQ_EN_REG);
+	spin_unlock(&mbox->lock);
+
+	dev_dbg(mbox->controller.dev, "Shutdown channel %d\n", n);
+}
+
+static bool sunxi_msgbox_last_tx_done(struct mbox_chan *chan)
+{
+	struct sunxi_msgbox *mbox = channel_to_msgbox(chan);
+	int n = channel_number(chan);
+
+	/*
+	 * The message box hardware allows us to snoop on the other user's IRQ
+	 * statuses. Consider a message to be acknowledged when the reception
+	 * IRQ for that channel is cleared. As the hardware only allows clearing
+	 * the IRQ for a channel when the FIFO is empty, this still ensures that
+	 * the message has actually been read. Compared to checking the number
+	 * of messages in the FIFO, it also gives the receiver an opportunity to
+	 * perform minimal message handling (such as recording extra information
+	 * from a shared memory buffer) before acknowledging a message.
+	 */
+	return !(readl(mbox->regs + REMOTE_IRQ_STATUS_REG) & REMOTE_RX_IRQ(n));
+}
+
+static bool sunxi_msgbox_peek_data(struct mbox_chan *chan)
+{
+	struct sunxi_msgbox *mbox = channel_to_msgbox(chan);
+	int n = channel_number(chan);
+
+	return (readl(mbox->regs + RX_MSG_STATUS_REG(n)) & MSG_STATUS_MASK) > 0;
+}
+
+static const struct mbox_chan_ops sunxi_msgbox_chan_ops = {
+	.send_data	= sunxi_msgbox_send_data,
+	.startup	= sunxi_msgbox_startup,
+	.shutdown	= sunxi_msgbox_shutdown,
+	.last_tx_done	= sunxi_msgbox_last_tx_done,
+	.peek_data	= sunxi_msgbox_peek_data,
+};
+
+static struct mbox_chan *sunxi_msgbox_index_xlate(struct mbox_controller *mbox,
+	const struct of_phandle_args *sp)
+{
+	if (sp->args_count != 2)
+		return NULL;
+	/* Enforce this driver's assumed physical-to-virtual channel mapping. */
+	if ((sp->args[0] % 2) || (sp->args[1] != sp->args[0] + 1) ||
+	    (sp->args[0] > (NUM_CHANS - 1) * 2))
+		return NULL;
+
+	return &mbox->chans[sp->args[0] / 2];
+}
+
+static int sunxi_msgbox_probe(struct platform_device *pdev)
+{
+	struct clk *clk;
+	struct device *dev = &pdev->dev;
+	struct mbox_chan *chans;
+	struct reset_control *rst;
+	struct resource *res;
+	struct sunxi_msgbox *mbox;
+	int 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;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	mbox->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(mbox->regs))
+		return PTR_ERR(mbox->regs);
+
+	clk = devm_clk_get(dev, "bus");
+	if (IS_ERR(clk)) {
+		dev_err(dev, "Failed to get bus clock\n");
+		return PTR_ERR(clk);
+	}
+
+	rst = devm_reset_control_get(dev, "bus");
+	if (IS_ERR(rst)) {
+		dev_err(dev, "Failed to get bus reset\n");
+		return PTR_ERR(rst);
+	}
+
+	/*
+	 * The failure path should not disable the clock or assert the reset,
+	 * because the PSCI implementation in firmware relies on this device
+	 * being functional. Claiming the clock in this driver is required to
+	 * prevent Linux from turning it off.
+	 */
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = reset_control_deassert(rst);
+	if (ret) {
+		dev_err(dev, "Failed to deassert bus reset: %d\n", ret);
+		return ret;
+	}
+
+	/* Disable all interrupts. */
+	writel(0, mbox->regs + IRQ_EN_REG);
+
+	ret = devm_request_irq(dev, irq_of_parse_and_map(dev->of_node, 0),
+			       sunxi_msgbox_irq, 0, dev_name(dev), mbox);
+	if (ret) {
+		dev_err(dev, "Failed to register IRQ handler: %d\n", ret);
+		return ret;
+	}
+
+	mbox->controller.dev = dev;
+	mbox->controller.ops = &sunxi_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;
+	mbox->controller.of_xlate = sunxi_msgbox_index_xlate;
+
+	spin_lock_init(&mbox->lock);
+	platform_set_drvdata(pdev, mbox);
+
+	ret = mbox_controller_register(&mbox->controller);
+	if (ret)
+		dev_err(dev, "Failed to register mailbox: %d\n", ret);
+
+	return ret;
+}
+
+static int sunxi_msgbox_remove(struct platform_device *pdev)
+{
+	struct sunxi_msgbox *mbox = platform_get_drvdata(pdev);
+	mbox_controller_unregister(&mbox->controller);
+
+	return 0;
+}
+
+static const struct of_device_id sunxi_msgbox_of_match[] = {
+	{ .compatible = "allwinner,sunxi-msgbox", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sunxi_msgbox_of_match);
+
+static struct platform_driver sunxi_msgbox_driver = {
+	.driver = {
+		.name = "sunxi-msgbox",
+		.of_match_table = sunxi_msgbox_of_match,
+	},
+	.probe		= sunxi_msgbox_probe,
+	.remove		= sunxi_msgbox_remove,
+};
+module_platform_driver(sunxi_msgbox_driver);
+
+MODULE_AUTHOR("Samuel Holland <samuel@sholland.org>");
+MODULE_DESCRIPTION("Allwinner sunxi Message Box");
+MODULE_LICENSE("GPL v2");
-- 
2.13.6

Re: [PATCH 0/3] Allwinner sunxi message box support

[Maxime Ripard]

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

Hi,

On Tue, Feb 27, 2018 at 08:27:11PM -0600, Samuel Holland wrote:
> This series adds support for the "hardware message box" in recent
> Allwinner sunxi 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.
> 
> This driver has been tested with the base arm_scpi driver (which sends
> the SCPI_CMD_SCPI_CAPABILITIES command at probe time) and an in-progress
> firmware application running on the ARISC CPU. Because no firmware for
> the other end of the mailbox is complete at the moment, I have not
> included changes to the SoC device trees.

This is not directly related to this series, but what is your end goal
with regard to SCPI? What features do you expect to have there?

I'm asking because last time we discussed this, SCPI wasn't able to
abstract all the features the PMIC provides, and thus Linux needed to
still be able to access it.

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[PATCH 0/3] Allwinner sunxi message box support

[Maxime Ripard]

Hi,

On Tue, Feb 27, 2018 at 08:27:11PM -0600, Samuel Holland wrote:
> This series adds support for the "hardware message box" in recent
> Allwinner sunxi 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.
> 
> This driver has been tested with the base arm_scpi driver (which sends
> the SCPI_CMD_SCPI_CAPABILITIES command at probe time) and an in-progress
> firmware application running on the ARISC CPU. Because no firmware for
> the other end of the mailbox is complete at the moment, I have not
> included changes to the SoC device trees.

This is not directly related to this series, but what is your end goal
with regard to SCPI? What features do you expect to have there?

I'm asking because last time we discussed this, SCPI wasn't able to
abstract all the features the PMIC provides, and thus Linux needed to
still be able to access it.

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180228/374eae6a/attachment.sig>

Re: [PATCH 1/3] dt-bindings: Add a binding for the sunxi message box

[Maxime Ripard]

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

Hi,

On Tue, Feb 27, 2018 at 08:27:12PM -0600, Samuel Holland wrote:
> This mailbox hardware is present in several Allwinner sun8i and sun50i
> SoCs. Add a device tree binding for it.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  .../devicetree/bindings/mailbox/sunxi-msgbox.txt   | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
> new file mode 100644
> index 000000000000..3b3ed7f870a0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
> @@ -0,0 +1,40 @@
> +Allwinner sunxi Message Box
> +===========================
> +
> +The hardware message box on sunxi SoCs is a two-user mailbox controller
> +containing 8 unidirectional FIFOs bonded into 4 bidirectional mailbox channels.
> +An interrupt is raised for received messages, but software must poll to know
> +when a transmitted message has been acknowledged by the remote user.
> +
> +Refer to ./mailbox.txt for generic information about mailbox device-tree
> +bindings.
> +
> +Mailbox Device Node:
> +====================
> +
> +Required properties:
> +--------------------
> +- compatible:		Must be "allwinner,sunxi-msgbox".

The IP change quite often in the Allwinner SoCs, so it would be better
to use a more specific compatible there. IIRC that IP was introduced
with the A31, so what about sun6i-a31-msgbox?

> +- reg:			Contains the mailbox register address range (base
> +			address and length).
> +- clocks:		phandle for the clock controller and specifier.
> +- clock-names:		Must be "bus".
> +- resets:		phandle for the reset controller and specifier.
> +- reset-names:		Must be "bus".
> +- interrupts:		Contains interrupt information for the mailbox.
> +- #mbox-cells		Must be 2 - the indexes of the transmit and receive
> +			channels, respectively.

That would prevent any unidirectional communication, wouldn't it?
Other mailboxes driver seem to have two mbox channels, one for each
direction, which also seem to mimic our DMA bindings (where we are in
pretty much the same situation).

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[PATCH 1/3] dt-bindings: Add a binding for the sunxi message box

[Maxime Ripard]

Hi,

On Tue, Feb 27, 2018 at 08:27:12PM -0600, Samuel Holland wrote:
> This mailbox hardware is present in several Allwinner sun8i and sun50i
> SoCs. Add a device tree binding for it.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  .../devicetree/bindings/mailbox/sunxi-msgbox.txt   | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
> new file mode 100644
> index 000000000000..3b3ed7f870a0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
> @@ -0,0 +1,40 @@
> +Allwinner sunxi Message Box
> +===========================
> +
> +The hardware message box on sunxi SoCs is a two-user mailbox controller
> +containing 8 unidirectional FIFOs bonded into 4 bidirectional mailbox channels.
> +An interrupt is raised for received messages, but software must poll to know
> +when a transmitted message has been acknowledged by the remote user.
> +
> +Refer to ./mailbox.txt for generic information about mailbox device-tree
> +bindings.
> +
> +Mailbox Device Node:
> +====================
> +
> +Required properties:
> +--------------------
> +- compatible:		Must be "allwinner,sunxi-msgbox".

The IP change quite often in the Allwinner SoCs, so it would be better
to use a more specific compatible there. IIRC that IP was introduced
with the A31, so what about sun6i-a31-msgbox?

> +- reg:			Contains the mailbox register address range (base
> +			address and length).
> +- clocks:		phandle for the clock controller and specifier.
> +- clock-names:		Must be "bus".
> +- resets:		phandle for the reset controller and specifier.
> +- reset-names:		Must be "bus".
> +- interrupts:		Contains interrupt information for the mailbox.
> +- #mbox-cells		Must be 2 - the indexes of the transmit and receive
> +			channels, respectively.

That would prevent any unidirectional communication, wouldn't it?
Other mailboxes driver seem to have two mbox channels, one for each
direction, which also seem to mimic our DMA bindings (where we are in
pretty much the same situation).

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180228/8c5b4a06/attachment.sig>

Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Maxime Ripard]

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

On Tue, Feb 27, 2018 at 08:27:14PM -0600, Samuel Holland wrote:
> +	/*
> +	 * The failure path should not disable the clock or assert the reset,
> +	 * because the PSCI implementation in firmware relies on this device
> +	 * being functional. Claiming the clock in this driver is required to
> +	 * prevent Linux from turning it off.
> +	 */
> +	ret = clk_prepare_enable(clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
> +		return ret;
> +	}

You don't need it to be always on though. You only need it to be
enabled when you access the registers (on both sides I guess?). So you
could very well enable the clock in your registers accessors in Linux,
and do the same in the ARISC firmware. That should work.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Maxime Ripard]

On Tue, Feb 27, 2018 at 08:27:14PM -0600, Samuel Holland wrote:
> +	/*
> +	 * The failure path should not disable the clock or assert the reset,
> +	 * because the PSCI implementation in firmware relies on this device
> +	 * being functional. Claiming the clock in this driver is required to
> +	 * prevent Linux from turning it off.
> +	 */
> +	ret = clk_prepare_enable(clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
> +		return ret;
> +	}

You don't need it to be always on though. You only need it to be
enabled when you access the registers (on both sides I guess?). So you
could very well enable the clock in your registers accessors in Linux,
and do the same in the ARISC firmware. That should work.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180228/707f091c/attachment.sig>

Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Jassi Brar]

On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
....

> +/*
> + * The message box hardware provides 8 unidirectional channels. As the mailbox
> + * framework expects them to be bidirectional
>
That is incorrect. Mailbox framework does not require a channel to be
TX and RX capable.

You should expose each channel as per its physical capability, let the
client configure it for direction (if its bidirectional) and acquire
separate channels for RX and TX if it needs to.

Cheers!

[PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Jassi Brar]

On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
....

> +/*
> + * The message box hardware provides 8 unidirectional channels. As the mailbox
> + * framework expects them to be bidirectional
>
That is incorrect. Mailbox framework does not require a channel to be
TX and RX capable.

You should expose each channel as per its physical capability, let the
client configure it for direction (if its bidirectional) and acquire
separate channels for RX and TX if it needs to.

Cheers!

Re: [PATCH 1/3] dt-bindings: Add a binding for the sunxi message box

[Andre Przywara]

Hi Samuel,

thank you very much for writing and posting this!

On 28/02/18 08:28, Maxime Ripard wrote:
> Hi,
> 
> On Tue, Feb 27, 2018 at 08:27:12PM -0600, Samuel Holland wrote:
>> This mailbox hardware is present in several Allwinner sun8i and sun50i
>> SoCs. Add a device tree binding for it.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  .../devicetree/bindings/mailbox/sunxi-msgbox.txt   | 40 ++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
>> new file mode 100644
>> index 000000000000..3b3ed7f870a0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
>> @@ -0,0 +1,40 @@
>> +Allwinner sunxi Message Box
>> +===========================
>> +
>> +The hardware message box on sunxi SoCs is a two-user mailbox controller
>> +containing 8 unidirectional FIFOs bonded into 4 bidirectional mailbox channels.
>> +An interrupt is raised for received messages, but software must poll to know
>> +when a transmitted message has been acknowledged by the remote user.
>> +
>> +Refer to ./mailbox.txt for generic information about mailbox device-tree
>> +bindings.
>> +
>> +Mailbox Device Node:
>> +====================
>> +
>> +Required properties:
>> +--------------------
>> +- compatible:		Must be "allwinner,sunxi-msgbox".
> 
> The IP change quite often in the Allwinner SoCs, so it would be better
> to use a more specific compatible there. IIRC that IP was introduced
> with the A31, so what about sun6i-a31-msgbox?

As much as I like generic compatible strings ;-) I agree with Maxime
here. Can you change this to something like:

- compatible: Must contain at least "allwinner,sun6i-a31-msgbox".
              Should also contain one of the more specific strings, when
	      used on other SoCs which are compatible:

And then list the strings for the SoCs which we know of, ideally have
been tested also?

The rationale behind this is that by listing those strings here we
reserve them, to cover for future extensions or bugs found in those
SoCs. So even though the driver would only match on
"allwinner,sun6i-a31-msgbox" right now, we can extend it in the future.
The actual DTs would always use:
	compatible = "allwinner,sun50i-a64-msgbox",
		     "allwinner,sun6i-a31-msgbox";
Right now the first string would not match anything, but the second
would. The driver would use the generic driver functionality.
Should we find a bug or speciality in the A64, we can add the a64 string
to the driver and connect it to the workaround, and the actual DTs would
not need to be touched.
Does this make sense?

>> +- reg:			Contains the mailbox register address range (base
>> +			address and length).
>> +- clocks:		phandle for the clock controller and specifier.
>> +- clock-names:		Must be "bus".

Maxime:
Out of curiosity: There is only one clock, and just to gate the APB
clock to the MMIO registers. So do we need clock-names? Or is this good
practise these days to allow extending the clocks later in a compatible
manner? (Thinking of the sunxi pinctrl here.)
And is "bus" the correct naming? I see "apb" being used as well, is this
legacy?

Samuel: can you briefly add the purpose of the clock?

>> +- resets:		phandle for the reset controller and specifier.
>> +- reset-names:		Must be "bus".
>> +- interrupts:		Contains interrupt information for the mailbox.
>> +- #mbox-cells		Must be 2 - the indexes of the transmit and receive
>> +			channels, respectively.
> 
> That would prevent any unidirectional communication, wouldn't it?
> Other mailboxes driver seem to have two mbox channels, one for each
> direction, which also seem to mimic our DMA bindings (where we are in
> pretty much the same situation).

I also believe that this should be "1" here, just listing the channel
ID. It would be up to the DT/firmware author to put the right channel
(RX or TX) in the consumer node.

Cheers,
Andre.

[PATCH 1/3] dt-bindings: Add a binding for the sunxi message box

[Andre Przywara]

Hi Samuel,

thank you very much for writing and posting this!

On 28/02/18 08:28, Maxime Ripard wrote:
> Hi,
> 
> On Tue, Feb 27, 2018 at 08:27:12PM -0600, Samuel Holland wrote:
>> This mailbox hardware is present in several Allwinner sun8i and sun50i
>> SoCs. Add a device tree binding for it.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  .../devicetree/bindings/mailbox/sunxi-msgbox.txt   | 40 ++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
>> new file mode 100644
>> index 000000000000..3b3ed7f870a0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
>> @@ -0,0 +1,40 @@
>> +Allwinner sunxi Message Box
>> +===========================
>> +
>> +The hardware message box on sunxi SoCs is a two-user mailbox controller
>> +containing 8 unidirectional FIFOs bonded into 4 bidirectional mailbox channels.
>> +An interrupt is raised for received messages, but software must poll to know
>> +when a transmitted message has been acknowledged by the remote user.
>> +
>> +Refer to ./mailbox.txt for generic information about mailbox device-tree
>> +bindings.
>> +
>> +Mailbox Device Node:
>> +====================
>> +
>> +Required properties:
>> +--------------------
>> +- compatible:		Must be "allwinner,sunxi-msgbox".
> 
> The IP change quite often in the Allwinner SoCs, so it would be better
> to use a more specific compatible there. IIRC that IP was introduced
> with the A31, so what about sun6i-a31-msgbox?

As much as I like generic compatible strings ;-) I agree with Maxime
here. Can you change this to something like:

- compatible: Must contain at least "allwinner,sun6i-a31-msgbox".
              Should also contain one of the more specific strings, when
	      used on other SoCs which are compatible:

And then list the strings for the SoCs which we know of, ideally have
been tested also?

The rationale behind this is that by listing those strings here we
reserve them, to cover for future extensions or bugs found in those
SoCs. So even though the driver would only match on
"allwinner,sun6i-a31-msgbox" right now, we can extend it in the future.
The actual DTs would always use:
	compatible = "allwinner,sun50i-a64-msgbox",
		     "allwinner,sun6i-a31-msgbox";
Right now the first string would not match anything, but the second
would. The driver would use the generic driver functionality.
Should we find a bug or speciality in the A64, we can add the a64 string
to the driver and connect it to the workaround, and the actual DTs would
not need to be touched.
Does this make sense?

>> +- reg:			Contains the mailbox register address range (base
>> +			address and length).
>> +- clocks:		phandle for the clock controller and specifier.
>> +- clock-names:		Must be "bus".

Maxime:
Out of curiosity: There is only one clock, and just to gate the APB
clock to the MMIO registers. So do we need clock-names? Or is this good
practise these days to allow extending the clocks later in a compatible
manner? (Thinking of the sunxi pinctrl here.)
And is "bus" the correct naming? I see "apb" being used as well, is this
legacy?

Samuel: can you briefly add the purpose of the clock?

>> +- resets:		phandle for the reset controller and specifier.
>> +- reset-names:		Must be "bus".
>> +- interrupts:		Contains interrupt information for the mailbox.
>> +- #mbox-cells		Must be 2 - the indexes of the transmit and receive
>> +			channels, respectively.
> 
> That would prevent any unidirectional communication, wouldn't it?
> Other mailboxes driver seem to have two mbox channels, one for each
> direction, which also seem to mimic our DMA bindings (where we are in
> pretty much the same situation).

I also believe that this should be "1" here, just listing the channel
ID. It would be up to the DT/firmware author to put the right channel
(RX or TX) in the consumer node.

Cheers,
Andre.

Re: [PATCH 2/3] mailbox: Avoid NULL dereference in mbox_chan_received_data

[Andre Przywara]

Hi,

On 28/02/18 02:27, Samuel Holland wrote:
> If a reception IRQ is pending when a mailbox channel is shut down (for
> example, if the controller uses threaded interrupts), it is possible for
> mbox_chan_received_data to be called while chan->cl is NULL.
> 
> This was found while developing a mailbox controller driver for use with
> SCPI. The SCPI protocol driver frees its mailbox channel during probing
> if the SCP firmware does not respond within a specified timeout. In this
> case, if the SCP firmware takes slightly too long to respond,
> mbox_chan_received_data races with mbox_free_channel clearing chan->cl.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/mailbox/mailbox.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 674b35f402f5..a0258d8672d5 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -152,9 +152,11 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
>   */
>  void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
>  {
> +	struct mbox_client *cl = READ_ONCE(chan->cl);
> +
>  	/* No buffering the received data */
> -	if (chan->cl->rx_callback)
> -		chan->cl->rx_callback(chan->cl, mssg);
> +	if (cl && cl->rx_callback)
> +		cl->rx_callback(cl, mssg);

I don't think this is the proper fix. This sounds like we should have a
lock here. If mbox_free_channel now frees or clears chan->cl between the
READ_ONCE and the second part of the "&&", we will have a
use-after-free. If it's being cleared after the comparison, we will end
up with a NULL pointer dereference again, IIUC.

Or am I missing something?

Cheers,
Andre.

>  }
>  EXPORT_SYMBOL_GPL(mbox_chan_received_data);
>  
> 

[PATCH 2/3] mailbox: Avoid NULL dereference in mbox_chan_received_data

[Andre Przywara]

Hi,

On 28/02/18 02:27, Samuel Holland wrote:
> If a reception IRQ is pending when a mailbox channel is shut down (for
> example, if the controller uses threaded interrupts), it is possible for
> mbox_chan_received_data to be called while chan->cl is NULL.
> 
> This was found while developing a mailbox controller driver for use with
> SCPI. The SCPI protocol driver frees its mailbox channel during probing
> if the SCP firmware does not respond within a specified timeout. In this
> case, if the SCP firmware takes slightly too long to respond,
> mbox_chan_received_data races with mbox_free_channel clearing chan->cl.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/mailbox/mailbox.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 674b35f402f5..a0258d8672d5 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -152,9 +152,11 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
>   */
>  void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
>  {
> +	struct mbox_client *cl = READ_ONCE(chan->cl);
> +
>  	/* No buffering the received data */
> -	if (chan->cl->rx_callback)
> -		chan->cl->rx_callback(chan->cl, mssg);
> +	if (cl && cl->rx_callback)
> +		cl->rx_callback(cl, mssg);

I don't think this is the proper fix. This sounds like we should have a
lock here. If mbox_free_channel now frees or clears chan->cl between the
READ_ONCE and the second part of the "&&", we will have a
use-after-free. If it's being cleared after the comparison, we will end
up with a NULL pointer dereference again, IIUC.

Or am I missing something?

Cheers,
Andre.

>  }
>  EXPORT_SYMBOL_GPL(mbox_chan_received_data);
>  
> 

Re: [PATCH 0/3] Allwinner sunxi message box support

[Samuel Holland]

Hi,

On 02/28/18 02:24, Maxime Ripard wrote:
> On Tue, Feb 27, 2018 at 08:27:11PM -0600, Samuel Holland wrote:
>> This series adds support for the "hardware message box" in recent
>> Allwinner sunxi 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.
>>
>> This driver has been tested with the base arm_scpi driver (which sends
>> the SCPI_CMD_SCPI_CAPABILITIES command at probe time) and an in-progress
>> firmware application running on the ARISC CPU. Because no firmware for
>> the other end of the mailbox is complete at the moment, I have not
>> included changes to the SoC device trees.
> 
> This is not directly related to this series, but what is your end goal
> with regard to SCPI? What features do you expect to have there?

Current plans are to support in SCPI:
- SMP bringup and CPU hotplug (via PSCI in ATF)
- System reset and poweroff (via PSCI in ATF)
- System standby and suspend (via PSCI in ATF)
- DVFS for ARM CPUs
- Clock control for clocks in R_PRCM (e.g. R_CIR_RX)
- Thermal sensor reading

Other planned features of the firmware:
- System wakeup from poweroff (via GPIO or interrupt to firmware)
- System power/suspend LED control (connected to GPIO or PMIC GPIO)
- Polling of thermal sensor in firmware for thermal throttling

> I'm asking because last time we discussed this, SCPI wasn't able to abstract
> all the features the PMIC provides, and thus Linux needed to still be able to
> access it.

This should only be an issue for devices with AXP PMICs. Other devices with GPIO
or simple I²C regulator control, such as H3 and H5 boards, can have all
necessary features described with standard SCPI commands. SCPI provides for 127
vendor-defined "extended" commands, which could be used to build a full AXP PMIC
driver, or simply provide commands to read/write PMIC registers.

For the moment I'm not concerned with battery powered devices. So right now the
relevant boards either a) don't have a PMIC, or b) can have everything but DVFS
voltage hard-coded in the firmware.

> Thanks!
> Maxime

Regards,
Samuel

[PATCH 0/3] Allwinner sunxi message box support

[Samuel Holland]

Hi,

On 02/28/18 02:24, Maxime Ripard wrote:
> On Tue, Feb 27, 2018 at 08:27:11PM -0600, Samuel Holland wrote:
>> This series adds support for the "hardware message box" in recent
>> Allwinner sunxi 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.
>>
>> This driver has been tested with the base arm_scpi driver (which sends
>> the SCPI_CMD_SCPI_CAPABILITIES command at probe time) and an in-progress
>> firmware application running on the ARISC CPU. Because no firmware for
>> the other end of the mailbox is complete at the moment, I have not
>> included changes to the SoC device trees.
> 
> This is not directly related to this series, but what is your end goal
> with regard to SCPI? What features do you expect to have there?

Current plans are to support in SCPI:
- SMP bringup and CPU hotplug (via PSCI in ATF)
- System reset and poweroff (via PSCI in ATF)
- System standby and suspend (via PSCI in ATF)
- DVFS for ARM CPUs
- Clock control for clocks in R_PRCM (e.g. R_CIR_RX)
- Thermal sensor reading

Other planned features of the firmware:
- System wakeup from poweroff (via GPIO or interrupt to firmware)
- System power/suspend LED control (connected to GPIO or PMIC GPIO)
- Polling of thermal sensor in firmware for thermal throttling

> I'm asking because last time we discussed this, SCPI wasn't able to abstract
> all the features the PMIC provides, and thus Linux needed to still be able to
> access it.

This should only be an issue for devices with AXP PMICs. Other devices with GPIO
or simple I?C regulator control, such as H3 and H5 boards, can have all
necessary features described with standard SCPI commands. SCPI provides for 127
vendor-defined "extended" commands, which could be used to build a full AXP PMIC
driver, or simply provide commands to read/write PMIC registers.

For the moment I'm not concerned with battery powered devices. So right now the
relevant boards either a) don't have a PMIC, or b) can have everything but DVFS
voltage hard-coded in the firmware.

> Thanks!
> Maxime

Regards,
Samuel

Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Samuel Holland]

Hi,

On 02/28/18 02:32, Maxime Ripard wrote:
> On Tue, Feb 27, 2018 at 08:27:14PM -0600, Samuel Holland wrote:
>> +	/*
>> +	 * The failure path should not disable the clock or assert the reset,
>> +	 * because the PSCI implementation in firmware relies on this device
>> +	 * being functional. Claiming the clock in this driver is required to
>> +	 * prevent Linux from turning it off.
>> +	 */
>> +	ret = clk_prepare_enable(clk);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
>> +		return ret;
>> +	}
> 
> You don't need it to be always on though. You only need it to be
> enabled when you access the registers (on both sides I guess?). So you
> could very well enable the clock in your registers accessors in Linux,
> and do the same in the ARISC firmware. That should work.

It does need to always be on because the *PSCI* implementation (ATF) also uses
SCPI concurrently with Linux (on a separate channel). Turning off the clock
anywhere in Linux could turn it off in the middle of a PSCI SCPI call on a
different CPU, causing ATF to hang in EL3 (which would be very bad).

> Maxime

Regards,
Samuel

[PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Samuel Holland]

Hi,

On 02/28/18 02:32, Maxime Ripard wrote:
> On Tue, Feb 27, 2018 at 08:27:14PM -0600, Samuel Holland wrote:
>> +	/*
>> +	 * The failure path should not disable the clock or assert the reset,
>> +	 * because the PSCI implementation in firmware relies on this device
>> +	 * being functional. Claiming the clock in this driver is required to
>> +	 * prevent Linux from turning it off.
>> +	 */
>> +	ret = clk_prepare_enable(clk);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
>> +		return ret;
>> +	}
> 
> You don't need it to be always on though. You only need it to be
> enabled when you access the registers (on both sides I guess?). So you
> could very well enable the clock in your registers accessors in Linux,
> and do the same in the ARISC firmware. That should work.

It does need to always be on because the *PSCI* implementation (ATF) also uses
SCPI concurrently with Linux (on a separate channel). Turning off the clock
anywhere in Linux could turn it off in the middle of a PSCI SCPI call on a
different CPU, causing ATF to hang in EL3 (which would be very bad).

> Maxime

Regards,
Samuel

Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Samuel Holland]

Hi,

On 02/28/18 03:16, Jassi Brar wrote:
> On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
> ....
> 
>> +/*
>> + * The message box hardware provides 8 unidirectional channels. As the mailbox
>> + * framework expects them to be bidirectional
>>
> That is incorrect. Mailbox framework does not require a channel to be
> TX and RX capable.

Sorry, it would be more accurate to say that the intended mailbox _client_
expects the channels to be bidirectional.

> You should expose each channel as per its physical capability, let the
> client configure it for direction (if its bidirectional) and acquire
> separate channels for RX and TX if it needs to.

Is there any way for the mailbox framework to inform the client that a channel
is uni/bidirectional? Or if the channel supports RX/TX specifically? I couldn't
find any.

It looks like all of the clients assume one case or the other, based on the
hardware they were initially designed to support. For example arm_scpi expects
one channel per client, while ti_sci and st_remoteproc expect two separate RX
and TX channels per client.

Since there's no API for the client to know the hardware properties, modifying
arm_scpi to support unidirectional mailbox channels would break support for all
existing bidirectional mailboxes.

To expose the physical channels individually, there would need to be an API to:
a) Determine if a mailbox channel supports unidirectional or bidirectional
   operation.
b) If a channel is unidirectional, determine which direction(s) it supports.
c) If a unidirectional channel supports both directions, configure which
   direction it should run in.

Since that API doesn't currently exist, I wrote the driver to match what the
intended client expected (as all other mailbox controller drivers currently do).

> Cheers!

Regards,
Samuel

[PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Samuel Holland]

Hi,

On 02/28/18 03:16, Jassi Brar wrote:
> On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
> ....
> 
>> +/*
>> + * The message box hardware provides 8 unidirectional channels. As the mailbox
>> + * framework expects them to be bidirectional
>>
> That is incorrect. Mailbox framework does not require a channel to be
> TX and RX capable.

Sorry, it would be more accurate to say that the intended mailbox _client_
expects the channels to be bidirectional.

> You should expose each channel as per its physical capability, let the
> client configure it for direction (if its bidirectional) and acquire
> separate channels for RX and TX if it needs to.

Is there any way for the mailbox framework to inform the client that a channel
is uni/bidirectional? Or if the channel supports RX/TX specifically? I couldn't
find any.

It looks like all of the clients assume one case or the other, based on the
hardware they were initially designed to support. For example arm_scpi expects
one channel per client, while ti_sci and st_remoteproc expect two separate RX
and TX channels per client.

Since there's no API for the client to know the hardware properties, modifying
arm_scpi to support unidirectional mailbox channels would break support for all
existing bidirectional mailboxes.

To expose the physical channels individually, there would need to be an API to:
a) Determine if a mailbox channel supports unidirectional or bidirectional
   operation.
b) If a channel is unidirectional, determine which direction(s) it supports.
c) If a unidirectional channel supports both directions, configure which
   direction it should run in.

Since that API doesn't currently exist, I wrote the driver to match what the
intended client expected (as all other mailbox controller drivers currently do).

> Cheers!

Regards,
Samuel

Re: [PATCH 1/3] dt-bindings: Add a binding for the sunxi message box

[Samuel Holland]

Hi,

On 02/28/18 02:28, Maxime Ripard wrote:
> On Tue, Feb 27, 2018 at 08:27:12PM -0600, Samuel Holland wrote:
>> This mailbox hardware is present in several Allwinner sun8i and sun50i
>> SoCs. Add a device tree binding for it.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  .../devicetree/bindings/mailbox/sunxi-msgbox.txt   | 40 ++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
>> new file mode 100644
>> index 000000000000..3b3ed7f870a0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
>> @@ -0,0 +1,40 @@
>> +Allwinner sunxi Message Box
>> +===========================
>> +
>> +The hardware message box on sunxi SoCs is a two-user mailbox controller
>> +containing 8 unidirectional FIFOs bonded into 4 bidirectional mailbox channels.
>> +An interrupt is raised for received messages, but software must poll to know
>> +when a transmitted message has been acknowledged by the remote user.
>> +
>> +Refer to ./mailbox.txt for generic information about mailbox device-tree
>> +bindings.
>> +
>> +Mailbox Device Node:
>> +====================
>> +
>> +Required properties:
>> +--------------------
>> +- compatible:		Must be "allwinner,sunxi-msgbox".
> 
> The IP change quite often in the Allwinner SoCs, so it would be better
> to use a more specific compatible there. IIRC that IP was introduced
> with the A31, so what about sun6i-a31-msgbox?

Ok, I will do this for v2, following Andre's suggestion. (I knew the AR100 was
introduced with the A31, but I couldn't find any reference to the msgbox in the
A31 manual).

>> +- reg:			Contains the mailbox register address range (base
>> +			address and length).
>> +- clocks:		phandle for the clock controller and specifier.
>> +- clock-names:		Must be "bus".
>> +- resets:		phandle for the reset controller and specifier.
>> +- reset-names:		Must be "bus".
>> +- interrupts:		Contains interrupt information for the mailbox.
>> +- #mbox-cells		Must be 2 - the indexes of the transmit and receive
>> +			channels, respectively.
> 
> That would prevent any unidirectional communication, wouldn't it?
> Other mailboxes driver seem to have two mbox channels, one for each
> direction, which also seem to mimic our DMA bindings (where we are in
> pretty much the same situation).

I've responded to Jassi's comment about the same issue on patch 3.

> Thanks!
> Maxime

Regards,
Samuel

[PATCH 1/3] dt-bindings: Add a binding for the sunxi message box

[Samuel Holland]

Hi,

On 02/28/18 02:28, Maxime Ripard wrote:
> On Tue, Feb 27, 2018 at 08:27:12PM -0600, Samuel Holland wrote:
>> This mailbox hardware is present in several Allwinner sun8i and sun50i
>> SoCs. Add a device tree binding for it.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  .../devicetree/bindings/mailbox/sunxi-msgbox.txt   | 40 ++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
>> new file mode 100644
>> index 000000000000..3b3ed7f870a0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
>> @@ -0,0 +1,40 @@
>> +Allwinner sunxi Message Box
>> +===========================
>> +
>> +The hardware message box on sunxi SoCs is a two-user mailbox controller
>> +containing 8 unidirectional FIFOs bonded into 4 bidirectional mailbox channels.
>> +An interrupt is raised for received messages, but software must poll to know
>> +when a transmitted message has been acknowledged by the remote user.
>> +
>> +Refer to ./mailbox.txt for generic information about mailbox device-tree
>> +bindings.
>> +
>> +Mailbox Device Node:
>> +====================
>> +
>> +Required properties:
>> +--------------------
>> +- compatible:		Must be "allwinner,sunxi-msgbox".
> 
> The IP change quite often in the Allwinner SoCs, so it would be better
> to use a more specific compatible there. IIRC that IP was introduced
> with the A31, so what about sun6i-a31-msgbox?

Ok, I will do this for v2, following Andre's suggestion. (I knew the AR100 was
introduced with the A31, but I couldn't find any reference to the msgbox in the
A31 manual).

>> +- reg:			Contains the mailbox register address range (base
>> +			address and length).
>> +- clocks:		phandle for the clock controller and specifier.
>> +- clock-names:		Must be "bus".
>> +- resets:		phandle for the reset controller and specifier.
>> +- reset-names:		Must be "bus".
>> +- interrupts:		Contains interrupt information for the mailbox.
>> +- #mbox-cells		Must be 2 - the indexes of the transmit and receive
>> +			channels, respectively.
> 
> That would prevent any unidirectional communication, wouldn't it?
> Other mailboxes driver seem to have two mbox channels, one for each
> direction, which also seem to mimic our DMA bindings (where we are in
> pretty much the same situation).

I've responded to Jassi's comment about the same issue on patch 3.

> Thanks!
> Maxime

Regards,
Samuel

Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Jassi Brar]

On Wed, Feb 28, 2018 at 11:21 PM, Samuel Holland <samuel@sholland.org> wrote:
> Hi,
>
> On 02/28/18 03:16, Jassi Brar wrote:
>> On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
>> ....
>>
>>> +/*
>>> + * The message box hardware provides 8 unidirectional channels. As the mailbox
>>> + * framework expects them to be bidirectional
>>>
>> That is incorrect. Mailbox framework does not require a channel to be
>> TX and RX capable.
>
> Sorry, it would be more accurate to say that the intended mailbox _client_
> expects the channels to be bidirectional.
>
Mailbox clients are very mailbox provider specific. Your client driver
is unlikely to be reuseable over another controller+remote combo.
Your client has to know already what a physical channel can do (RX, TX
or RXTX). There is no api to provide that info.

thanks

Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Jassi Brar]

On Wed, Feb 28, 2018 at 11:21 PM, Samuel Holland <samuel@sholland.org> wrote:
> Hi,
>
> On 02/28/18 03:16, Jassi Brar wrote:
>> On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
>> ....
>>
>>> +/*
>>> + * The message box hardware provides 8 unidirectional channels. As the mailbox
>>> + * framework expects them to be bidirectional
>>>
>> That is incorrect. Mailbox framework does not require a channel to be
>> TX and RX capable.
>
> Sorry, it would be more accurate to say that the intended mailbox _client_
> expects the channels to be bidirectional.
>
Mailbox clients are very mailbox provider specific. Your client driver
is unlikely to be reuseable over another controller+remote combo.
Your client has to know already what a physical channel can do (RX, TX
or RXTX). There is no api to provide that info.

thanks

[PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Jassi Brar]

On Wed, Feb 28, 2018 at 11:21 PM, Samuel Holland <samuel@sholland.org> wrote:
> Hi,
>
> On 02/28/18 03:16, Jassi Brar wrote:
>> On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
>> ....
>>
>>> +/*
>>> + * The message box hardware provides 8 unidirectional channels. As the mailbox
>>> + * framework expects them to be bidirectional
>>>
>> That is incorrect. Mailbox framework does not require a channel to be
>> TX and RX capable.
>
> Sorry, it would be more accurate to say that the intended mailbox _client_
> expects the channels to be bidirectional.
>
Mailbox clients are very mailbox provider specific. Your client driver
is unlikely to be reuseable over another controller+remote combo.
Your client has to know already what a physical channel can do (RX, TX
or RXTX). There is no api to provide that info.

thanks

Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Samuel Holland]

On 02/28/18 12:14, Jassi Brar wrote:
> On Wed, Feb 28, 2018 at 11:21 PM, Samuel Holland <samuel@sholland.org> wrote:
>> Hi,
>>
>> On 02/28/18 03:16, Jassi Brar wrote:
>>> On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
>>> ....
>>>
>>>> +/*
>>>> + * The message box hardware provides 8 unidirectional channels. As the mailbox
>>>> + * framework expects them to be bidirectional
>>>>
>>> That is incorrect. Mailbox framework does not require a channel to be
>>> TX and RX capable.
>>
>> Sorry, it would be more accurate to say that the intended mailbox _client_
>> expects the channels to be bidirectional.
>>
> Mailbox clients are very mailbox provider specific. Your client driver
> is unlikely to be reuseable over another controller+remote combo.
> Your client has to know already what a physical channel can do (RX, TX
> or RXTX). There is no api to provide that info.

There's a public specification for SCPI available[1]. I'm writing the remote
endpoint to follow that protocol specification. (There's even an explicitly
platform-agnostic update to the protocol called SCMI[2]). Ideally, I should be
able to reuse the existing Linux client drivers for that protocol. Are you
suggesting that I need to write a copy of the arm_scpi driver, completely
identical except that it requests two mailbox channels per client (sending on
one and receiving on the other) instead of one? In the >1000 line file, this is
all that I would need to change:

--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -257,7 +257,8 @@ struct scpi_xfer {

 struct scpi_chan {
 	struct mbox_client cl;
-	struct mbox_chan *chan;
+	struct mbox_chan *tx_chan;
+	struct mbox_chan *rx_chan;
 	void __iomem *tx_payload;
 	void __iomem *rx_payload;
 	struct list_head rx_pending;
@@ -541,7 +542,7 @@
 	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;

@@ -894,8 +895,10 @@
 {
 	int i;

-	for (i = 0; i < count && pchan->chan; i++, pchan++) {
-		mbox_free_channel(pchan->chan);
+	for (i = 0; i < count && pchan->tx_chan; i++, pchan++) {
+		if (pchan->rx_chan && pchan->rx_chan != pchan->tx_chan)
+			mbox_free_channel(pchan->rx_chan);
+		mbox_free_channel(pchan->tx_chan);
 		devm_kfree(dev, pchan->xfers);
 		devm_iounmap(dev, pchan->rx_payload);
 	}
@@ -968,6 +971,7 @@
 		dev_err(dev, "no mboxes property in '%pOF'\n", np);
 		return -ENODEV;
 	}
+	count /= 2;

 	scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL);
 	if (!scpi_chan)
@@ -1009,13 +1013,19 @@

 		ret = scpi_alloc_xfer_list(dev, pchan);
 		if (!ret) {
-			pchan->chan = mbox_request_channel(cl, idx);
-			if (!IS_ERR(pchan->chan))
+			pchan->tx_chan = mbox_request_channel(cl, idx * 2);
+			pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1);
+			/* This isn't quite right, but the idea stands. */
+			if (!IS_ERR(pchan->tx_chan) && !IS_ERR(pchan->rx_chan))
 				continue;
-			ret = PTR_ERR(pchan->chan);
+			ret = PTR_ERR(pchan->tx_chan);
 			if (ret != -EPROBE_DEFER)
 				dev_err(dev, "failed to get channel%d err %d\n",
-					idx, ret);
+					idx * 2, ret);
+			ret = PTR_ERR(pchan->rx_chan);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "failed to get channel%d err %d\n",
+					idx * 2 + 1, ret);
 		}
 err:
 		scpi_free_channels(dev, scpi_chan, idx);


But then there are two copies of almost exactly the same driver! If there was an
API for determining if a channel was unidirectional or bidirectional, than it
would be trivial to support both models in one driver:


--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -953,7 +955,7 @@

 static int scpi_probe(struct platform_device *pdev)
 {
-	int count, idx, ret;
+	int count, idx, mbox_idx, ret;
 	struct resource res;
 	struct scpi_chan *scpi_chan;
 	struct device *dev = &pdev->dev;
@@ -971,13 +973,12 @@
 		dev_err(dev, "no mboxes property in '%pOF'\n", np);
 		return -ENODEV;
 	}
-	count /= 2;

 	scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL);
 	if (!scpi_chan)
 		return -ENOMEM;

-	for (idx = 0; idx < count; idx++) {
+	for (idx = 0, mbox_idx = 0; mbox_idx < count; idx++) {
 		resource_size_t size;
 		struct scpi_chan *pchan = scpi_chan + idx;
 		struct mbox_client *cl = &pchan->cl;
@@ -1014,7 +1015,13 @@
 		ret = scpi_alloc_xfer_list(dev, pchan);
 		if (!ret) {
 			pchan->tx_chan = mbox_request_channel(cl, idx * 2);
-			pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1);
+			if (mbox_chan_is_bidirectional(pchan->tx_chan)) {
+				pchan->rx_chan = pchan->tx_chan;
+				mbox_idx += 1;
+			} else {
+				pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1);
+				mbox_idx += 2;
+			}
 			/* This isn't quite right, but the idea stands. */
 			if (!IS_ERR(pchan->tx_chan) && !IS_ERR(pchan->rx_chan))
 				continue;
@@ -1034,7 +1041,7 @@
 	}

 	scpi_info->channels = scpi_chan;
-	scpi_info->num_chans = count;
+	scpi_info->num_chans = idx;
 	scpi_info->commands = scpi_std_commands;

 	platform_set_drvdata(pdev, scpi_info);


Obviously this is just proof-of-concept code and would need to be cleaned up a bit.

The are platform-agnostic protocols using mailbox hardware. There's no reason
that the drivers for them need to be platform-specific. I'd really like to avoid
duplicating large amounts of code unnecessarily. I'm willing to prepare a patch
series adding this functionality to the mailbox API, if it would be accepted.

Something like:
bool mbox_chan_is_bidirectional(struct mbox_chan *chan);

Implemented in drivers like:
struct mbox_controller {
	...
	bool bidirectional_chans;
};

or:
struct mbox_chan_ops {
	...
	bool (*is_bidirectional)(struct mbox_chan *chan);
};

> thanks

Regards,
Samuel

[1]:
http://infocenter.arm.com/help/topic/com.arm.doc.dui0922g/scp_message_interface_v1_2_DUI0922G_en.pdf
[2]:
http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/DEN0056A_System_Control_and_Management_Interface.pdf

Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Samuel Holland]

On 02/28/18 12:14, Jassi Brar wrote:
> On Wed, Feb 28, 2018 at 11:21 PM, Samuel Holland <samuel@sholland.org> wrote:
>> Hi,
>>
>> On 02/28/18 03:16, Jassi Brar wrote:
>>> On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
>>> ....
>>>
>>>> +/*
>>>> + * The message box hardware provides 8 unidirectional channels. As the mailbox
>>>> + * framework expects them to be bidirectional
>>>>
>>> That is incorrect. Mailbox framework does not require a channel to be
>>> TX and RX capable.
>>
>> Sorry, it would be more accurate to say that the intended mailbox _client_
>> expects the channels to be bidirectional.
>>
> Mailbox clients are very mailbox provider specific. Your client driver
> is unlikely to be reuseable over another controller+remote combo.
> Your client has to know already what a physical channel can do (RX, TX
> or RXTX). There is no api to provide that info.

There's a public specification for SCPI available[1]. I'm writing the remote
endpoint to follow that protocol specification. (There's even an explicitly
platform-agnostic update to the protocol called SCMI[2]). Ideally, I should be
able to reuse the existing Linux client drivers for that protocol. Are you
suggesting that I need to write a copy of the arm_scpi driver, completely
identical except that it requests two mailbox channels per client (sending on
one and receiving on the other) instead of one? In the >1000 line file, this is
all that I would need to change:

--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -257,7 +257,8 @@ struct scpi_xfer {

 struct scpi_chan {
 	struct mbox_client cl;
-	struct mbox_chan *chan;
+	struct mbox_chan *tx_chan;
+	struct mbox_chan *rx_chan;
 	void __iomem *tx_payload;
 	void __iomem *rx_payload;
 	struct list_head rx_pending;
@@ -541,7 +542,7 @@
 	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;

@@ -894,8 +895,10 @@
 {
 	int i;

-	for (i = 0; i < count && pchan->chan; i++, pchan++) {
-		mbox_free_channel(pchan->chan);
+	for (i = 0; i < count && pchan->tx_chan; i++, pchan++) {
+		if (pchan->rx_chan && pchan->rx_chan != pchan->tx_chan)
+			mbox_free_channel(pchan->rx_chan);
+		mbox_free_channel(pchan->tx_chan);
 		devm_kfree(dev, pchan->xfers);
 		devm_iounmap(dev, pchan->rx_payload);
 	}
@@ -968,6 +971,7 @@
 		dev_err(dev, "no mboxes property in '%pOF'\n", np);
 		return -ENODEV;
 	}
+	count /= 2;

 	scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL);
 	if (!scpi_chan)
@@ -1009,13 +1013,19 @@

 		ret = scpi_alloc_xfer_list(dev, pchan);
 		if (!ret) {
-			pchan->chan = mbox_request_channel(cl, idx);
-			if (!IS_ERR(pchan->chan))
+			pchan->tx_chan = mbox_request_channel(cl, idx * 2);
+			pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1);
+			/* This isn't quite right, but the idea stands. */
+			if (!IS_ERR(pchan->tx_chan) && !IS_ERR(pchan->rx_chan))
 				continue;
-			ret = PTR_ERR(pchan->chan);
+			ret = PTR_ERR(pchan->tx_chan);
 			if (ret != -EPROBE_DEFER)
 				dev_err(dev, "failed to get channel%d err %d\n",
-					idx, ret);
+					idx * 2, ret);
+			ret = PTR_ERR(pchan->rx_chan);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "failed to get channel%d err %d\n",
+					idx * 2 + 1, ret);
 		}
 err:
 		scpi_free_channels(dev, scpi_chan, idx);


But then there are two copies of almost exactly the same driver! If there was an
API for determining if a channel was unidirectional or bidirectional, than it
would be trivial to support both models in one driver:


--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -953,7 +955,7 @@

 static int scpi_probe(struct platform_device *pdev)
 {
-	int count, idx, ret;
+	int count, idx, mbox_idx, ret;
 	struct resource res;
 	struct scpi_chan *scpi_chan;
 	struct device *dev = &pdev->dev;
@@ -971,13 +973,12 @@
 		dev_err(dev, "no mboxes property in '%pOF'\n", np);
 		return -ENODEV;
 	}
-	count /= 2;

 	scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL);
 	if (!scpi_chan)
 		return -ENOMEM;

-	for (idx = 0; idx < count; idx++) {
+	for (idx = 0, mbox_idx = 0; mbox_idx < count; idx++) {
 		resource_size_t size;
 		struct scpi_chan *pchan = scpi_chan + idx;
 		struct mbox_client *cl = &pchan->cl;
@@ -1014,7 +1015,13 @@
 		ret = scpi_alloc_xfer_list(dev, pchan);
 		if (!ret) {
 			pchan->tx_chan = mbox_request_channel(cl, idx * 2);
-			pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1);
+			if (mbox_chan_is_bidirectional(pchan->tx_chan)) {
+				pchan->rx_chan = pchan->tx_chan;
+				mbox_idx += 1;
+			} else {
+				pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1);
+				mbox_idx += 2;
+			}
 			/* This isn't quite right, but the idea stands. */
 			if (!IS_ERR(pchan->tx_chan) && !IS_ERR(pchan->rx_chan))
 				continue;
@@ -1034,7 +1041,7 @@
 	}

 	scpi_info->channels = scpi_chan;
-	scpi_info->num_chans = count;
+	scpi_info->num_chans = idx;
 	scpi_info->commands = scpi_std_commands;

 	platform_set_drvdata(pdev, scpi_info);


Obviously this is just proof-of-concept code and would need to be cleaned up a bit.

The are platform-agnostic protocols using mailbox hardware. There's no reason
that the drivers for them need to be platform-specific. I'd really like to avoid
duplicating large amounts of code unnecessarily. I'm willing to prepare a patch
series adding this functionality to the mailbox API, if it would be accepted.

Something like:
bool mbox_chan_is_bidirectional(struct mbox_chan *chan);

Implemented in drivers like:
struct mbox_controller {
	...
	bool bidirectional_chans;
};

or:
struct mbox_chan_ops {
	...
	bool (*is_bidirectional)(struct mbox_chan *chan);
};

> thanks

Regards,
Samuel

[1]:
http://infocenter.arm.com/help/topic/com.arm.doc.dui0922g/scp_message_interface_v1_2_DUI0922G_en.pdf
[2]:
http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/DEN0056A_System_Control_and_Management_Interface.pdf

[PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Samuel Holland]

On 02/28/18 12:14, Jassi Brar wrote:
> On Wed, Feb 28, 2018 at 11:21 PM, Samuel Holland <samuel@sholland.org> wrote:
>> Hi,
>>
>> On 02/28/18 03:16, Jassi Brar wrote:
>>> On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
>>> ....
>>>
>>>> +/*
>>>> + * The message box hardware provides 8 unidirectional channels. As the mailbox
>>>> + * framework expects them to be bidirectional
>>>>
>>> That is incorrect. Mailbox framework does not require a channel to be
>>> TX and RX capable.
>>
>> Sorry, it would be more accurate to say that the intended mailbox _client_
>> expects the channels to be bidirectional.
>>
> Mailbox clients are very mailbox provider specific. Your client driver
> is unlikely to be reuseable over another controller+remote combo.
> Your client has to know already what a physical channel can do (RX, TX
> or RXTX). There is no api to provide that info.

There's a public specification for SCPI available[1]. I'm writing the remote
endpoint to follow that protocol specification. (There's even an explicitly
platform-agnostic update to the protocol called SCMI[2]). Ideally, I should be
able to reuse the existing Linux client drivers for that protocol. Are you
suggesting that I need to write a copy of the arm_scpi driver, completely
identical except that it requests two mailbox channels per client (sending on
one and receiving on the other) instead of one? In the >1000 line file, this is
all that I would need to change:

--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -257,7 +257,8 @@ struct scpi_xfer {

 struct scpi_chan {
 	struct mbox_client cl;
-	struct mbox_chan *chan;
+	struct mbox_chan *tx_chan;
+	struct mbox_chan *rx_chan;
 	void __iomem *tx_payload;
 	void __iomem *rx_payload;
 	struct list_head rx_pending;
@@ -541,7 +542,7 @@
 	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;

@@ -894,8 +895,10 @@
 {
 	int i;

-	for (i = 0; i < count && pchan->chan; i++, pchan++) {
-		mbox_free_channel(pchan->chan);
+	for (i = 0; i < count && pchan->tx_chan; i++, pchan++) {
+		if (pchan->rx_chan && pchan->rx_chan != pchan->tx_chan)
+			mbox_free_channel(pchan->rx_chan);
+		mbox_free_channel(pchan->tx_chan);
 		devm_kfree(dev, pchan->xfers);
 		devm_iounmap(dev, pchan->rx_payload);
 	}
@@ -968,6 +971,7 @@
 		dev_err(dev, "no mboxes property in '%pOF'\n", np);
 		return -ENODEV;
 	}
+	count /= 2;

 	scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL);
 	if (!scpi_chan)
@@ -1009,13 +1013,19 @@

 		ret = scpi_alloc_xfer_list(dev, pchan);
 		if (!ret) {
-			pchan->chan = mbox_request_channel(cl, idx);
-			if (!IS_ERR(pchan->chan))
+			pchan->tx_chan = mbox_request_channel(cl, idx * 2);
+			pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1);
+			/* This isn't quite right, but the idea stands. */
+			if (!IS_ERR(pchan->tx_chan) && !IS_ERR(pchan->rx_chan))
 				continue;
-			ret = PTR_ERR(pchan->chan);
+			ret = PTR_ERR(pchan->tx_chan);
 			if (ret != -EPROBE_DEFER)
 				dev_err(dev, "failed to get channel%d err %d\n",
-					idx, ret);
+					idx * 2, ret);
+			ret = PTR_ERR(pchan->rx_chan);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "failed to get channel%d err %d\n",
+					idx * 2 + 1, ret);
 		}
 err:
 		scpi_free_channels(dev, scpi_chan, idx);


But then there are two copies of almost exactly the same driver! If there was an
API for determining if a channel was unidirectional or bidirectional, than it
would be trivial to support both models in one driver:


--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -953,7 +955,7 @@

 static int scpi_probe(struct platform_device *pdev)
 {
-	int count, idx, ret;
+	int count, idx, mbox_idx, ret;
 	struct resource res;
 	struct scpi_chan *scpi_chan;
 	struct device *dev = &pdev->dev;
@@ -971,13 +973,12 @@
 		dev_err(dev, "no mboxes property in '%pOF'\n", np);
 		return -ENODEV;
 	}
-	count /= 2;

 	scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL);
 	if (!scpi_chan)
 		return -ENOMEM;

-	for (idx = 0; idx < count; idx++) {
+	for (idx = 0, mbox_idx = 0; mbox_idx < count; idx++) {
 		resource_size_t size;
 		struct scpi_chan *pchan = scpi_chan + idx;
 		struct mbox_client *cl = &pchan->cl;
@@ -1014,7 +1015,13 @@
 		ret = scpi_alloc_xfer_list(dev, pchan);
 		if (!ret) {
 			pchan->tx_chan = mbox_request_channel(cl, idx * 2);
-			pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1);
+			if (mbox_chan_is_bidirectional(pchan->tx_chan)) {
+				pchan->rx_chan = pchan->tx_chan;
+				mbox_idx += 1;
+			} else {
+				pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1);
+				mbox_idx += 2;
+			}
 			/* This isn't quite right, but the idea stands. */
 			if (!IS_ERR(pchan->tx_chan) && !IS_ERR(pchan->rx_chan))
 				continue;
@@ -1034,7 +1041,7 @@
 	}

 	scpi_info->channels = scpi_chan;
-	scpi_info->num_chans = count;
+	scpi_info->num_chans = idx;
 	scpi_info->commands = scpi_std_commands;

 	platform_set_drvdata(pdev, scpi_info);


Obviously this is just proof-of-concept code and would need to be cleaned up a bit.

The are platform-agnostic protocols using mailbox hardware. There's no reason
that the drivers for them need to be platform-specific. I'd really like to avoid
duplicating large amounts of code unnecessarily. I'm willing to prepare a patch
series adding this functionality to the mailbox API, if it would be accepted.

Something like:
bool mbox_chan_is_bidirectional(struct mbox_chan *chan);

Implemented in drivers like:
struct mbox_controller {
	...
	bool bidirectional_chans;
};

or:
struct mbox_chan_ops {
	...
	bool (*is_bidirectional)(struct mbox_chan *chan);
};

> thanks

Regards,
Samuel

[1]:
http://infocenter.arm.com/help/topic/com.arm.doc.dui0922g/scp_message_interface_v1_2_DUI0922G_en.pdf
[2]:
http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/DEN0056A_System_Control_and_Management_Interface.pdf

Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Jassi Brar]

On Thu, Mar 1, 2018 at 12:26 AM, Samuel Holland <samuel@sholland.org> wrote:
> On 02/28/18 12:14, Jassi Brar wrote:
>> On Wed, Feb 28, 2018 at 11:21 PM, Samuel Holland <samuel@sholland.org> wrote:
>>> Hi,
>>>
>>> On 02/28/18 03:16, Jassi Brar wrote:
>>>> On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
>>>> ....
>>>>
>>>>> +/*
>>>>> + * The message box hardware provides 8 unidirectional channels. As the mailbox
>>>>> + * framework expects them to be bidirectional
>>>>>
>>>> That is incorrect. Mailbox framework does not require a channel to be
>>>> TX and RX capable.
>>>
>>> Sorry, it would be more accurate to say that the intended mailbox _client_
>>> expects the channels to be bidirectional.
>>>
>> Mailbox clients are very mailbox provider specific. Your client driver
>> is unlikely to be reuseable over another controller+remote combo.
>> Your client has to know already what a physical channel can do (RX, TX
>> or RXTX). There is no api to provide that info.
>
> There's a public specification for SCPI available[1]. I'm writing the remote
> endpoint to follow that protocol specification. (There's even an explicitly
> platform-agnostic update to the protocol called SCMI[2]). Ideally, I should be
> able to reuse the existing Linux client drivers for that protocol.
>
Thanks, I have already read those specs. Please note there are two
parts of the spec - Chapter-4-Protocol and Chapter-5-Transport.

You should definitely be able to reuse the Protocol implementation.

However, transport can vary with platform - that is why it is
separated out from Protocol. And mailbox is but one instance of
transport.
  Consider a platform with "power manager" chip connected via, say,
I2C - one could still make use of SCMI protocol. Or consider another
platform that does have mailbox link to the "power manager" but there
is no shared memory between them - you could then pass packets via the
mailbox FIFOs.
  So no, we can't have common transport implementation for every platform.

> Are you
> suggesting that I need to write a copy of the arm_scpi driver, completely
> identical except that it requests two mailbox channels per client (sending on
> one and receiving on the other) instead of one? In the >1000 line file, this is
> all that I would need to change:
>
I did unsuccessfully try to convince Sudeep to break the
implementation into platform-agnostic protocol and platform-specific
transport drivers. That way there would be no duplication of code.

Also please realise that you should not be writing controller driver,
keeping just one client driver (SCMI/SCPI) in mind. What if another
platform with same mailbox controller needs 8 TX "broadcast" links?
The h/w supports the usecase, but your driver wouldn't.

Thanks

Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Jassi Brar]

On Thu, Mar 1, 2018 at 12:26 AM, Samuel Holland <samuel@sholland.org> wrote:
> On 02/28/18 12:14, Jassi Brar wrote:
>> On Wed, Feb 28, 2018 at 11:21 PM, Samuel Holland <samuel@sholland.org> wrote:
>>> Hi,
>>>
>>> On 02/28/18 03:16, Jassi Brar wrote:
>>>> On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
>>>> ....
>>>>
>>>>> +/*
>>>>> + * The message box hardware provides 8 unidirectional channels. As the mailbox
>>>>> + * framework expects them to be bidirectional
>>>>>
>>>> That is incorrect. Mailbox framework does not require a channel to be
>>>> TX and RX capable.
>>>
>>> Sorry, it would be more accurate to say that the intended mailbox _client_
>>> expects the channels to be bidirectional.
>>>
>> Mailbox clients are very mailbox provider specific. Your client driver
>> is unlikely to be reuseable over another controller+remote combo.
>> Your client has to know already what a physical channel can do (RX, TX
>> or RXTX). There is no api to provide that info.
>
> There's a public specification for SCPI available[1]. I'm writing the remote
> endpoint to follow that protocol specification. (There's even an explicitly
> platform-agnostic update to the protocol called SCMI[2]). Ideally, I should be
> able to reuse the existing Linux client drivers for that protocol.
>
Thanks, I have already read those specs. Please note there are two
parts of the spec - Chapter-4-Protocol and Chapter-5-Transport.

You should definitely be able to reuse the Protocol implementation.

However, transport can vary with platform - that is why it is
separated out from Protocol. And mailbox is but one instance of
transport.
  Consider a platform with "power manager" chip connected via, say,
I2C - one could still make use of SCMI protocol. Or consider another
platform that does have mailbox link to the "power manager" but there
is no shared memory between them - you could then pass packets via the
mailbox FIFOs.
  So no, we can't have common transport implementation for every platform.

> Are you
> suggesting that I need to write a copy of the arm_scpi driver, completely
> identical except that it requests two mailbox channels per client (sending on
> one and receiving on the other) instead of one? In the >1000 line file, this is
> all that I would need to change:
>
I did unsuccessfully try to convince Sudeep to break the
implementation into platform-agnostic protocol and platform-specific
transport drivers. That way there would be no duplication of code.

Also please realise that you should not be writing controller driver,
keeping just one client driver (SCMI/SCPI) in mind. What if another
platform with same mailbox controller needs 8 TX "broadcast" links?
The h/w supports the usecase, but your driver wouldn't.

Thanks

[PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Jassi Brar]

On Thu, Mar 1, 2018 at 12:26 AM, Samuel Holland <samuel@sholland.org> wrote:
> On 02/28/18 12:14, Jassi Brar wrote:
>> On Wed, Feb 28, 2018 at 11:21 PM, Samuel Holland <samuel@sholland.org> wrote:
>>> Hi,
>>>
>>> On 02/28/18 03:16, Jassi Brar wrote:
>>>> On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
>>>> ....
>>>>
>>>>> +/*
>>>>> + * The message box hardware provides 8 unidirectional channels. As the mailbox
>>>>> + * framework expects them to be bidirectional
>>>>>
>>>> That is incorrect. Mailbox framework does not require a channel to be
>>>> TX and RX capable.
>>>
>>> Sorry, it would be more accurate to say that the intended mailbox _client_
>>> expects the channels to be bidirectional.
>>>
>> Mailbox clients are very mailbox provider specific. Your client driver
>> is unlikely to be reuseable over another controller+remote combo.
>> Your client has to know already what a physical channel can do (RX, TX
>> or RXTX). There is no api to provide that info.
>
> There's a public specification for SCPI available[1]. I'm writing the remote
> endpoint to follow that protocol specification. (There's even an explicitly
> platform-agnostic update to the protocol called SCMI[2]). Ideally, I should be
> able to reuse the existing Linux client drivers for that protocol.
>
Thanks, I have already read those specs. Please note there are two
parts of the spec - Chapter-4-Protocol and Chapter-5-Transport.

You should definitely be able to reuse the Protocol implementation.

However, transport can vary with platform - that is why it is
separated out from Protocol. And mailbox is but one instance of
transport.
  Consider a platform with "power manager" chip connected via, say,
I2C - one could still make use of SCMI protocol. Or consider another
platform that does have mailbox link to the "power manager" but there
is no shared memory between them - you could then pass packets via the
mailbox FIFOs.
  So no, we can't have common transport implementation for every platform.

> Are you
> suggesting that I need to write a copy of the arm_scpi driver, completely
> identical except that it requests two mailbox channels per client (sending on
> one and receiving on the other) instead of one? In the >1000 line file, this is
> all that I would need to change:
>
I did unsuccessfully try to convince Sudeep to break the
implementation into platform-agnostic protocol and platform-specific
transport drivers. That way there would be no duplication of code.

Also please realise that you should not be writing controller driver,
keeping just one client driver (SCMI/SCPI) in mind. What if another
platform with same mailbox controller needs 8 TX "broadcast" links?
The h/w supports the usecase, but your driver wouldn't.

Thanks

Re: [PATCH 1/3] dt-bindings: Add a binding for the sunxi message box

[Maxime Ripard]

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

On Wed, Feb 28, 2018 at 05:17:03PM +0000, Andre Przywara wrote:
> Hi Samuel,
> 
> thank you very much for writing and posting this!
> 
> On 28/02/18 08:28, Maxime Ripard wrote:
> > Hi,
> > 
> > On Tue, Feb 27, 2018 at 08:27:12PM -0600, Samuel Holland wrote:
> >> This mailbox hardware is present in several Allwinner sun8i and sun50i
> >> SoCs. Add a device tree binding for it.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>  .../devicetree/bindings/mailbox/sunxi-msgbox.txt   | 40 ++++++++++++++++++++++
> >>  1 file changed, 40 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
> >> new file mode 100644
> >> index 000000000000..3b3ed7f870a0
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
> >> @@ -0,0 +1,40 @@
> >> +Allwinner sunxi Message Box
> >> +===========================
> >> +
> >> +The hardware message box on sunxi SoCs is a two-user mailbox controller
> >> +containing 8 unidirectional FIFOs bonded into 4 bidirectional mailbox channels.
> >> +An interrupt is raised for received messages, but software must poll to know
> >> +when a transmitted message has been acknowledged by the remote user.
> >> +
> >> +Refer to ./mailbox.txt for generic information about mailbox device-tree
> >> +bindings.
> >> +
> >> +Mailbox Device Node:
> >> +====================
> >> +
> >> +Required properties:
> >> +--------------------
> >> +- compatible:		Must be "allwinner,sunxi-msgbox".
> > 
> > The IP change quite often in the Allwinner SoCs, so it would be better
> > to use a more specific compatible there. IIRC that IP was introduced
> > with the A31, so what about sun6i-a31-msgbox?
> 
> As much as I like generic compatible strings ;-) I agree with Maxime
> here. Can you change this to something like:
> 
> - compatible: Must contain at least "allwinner,sun6i-a31-msgbox".
>               Should also contain one of the more specific strings, when
> 	      used on other SoCs which are compatible:
> 
> And then list the strings for the SoCs which we know of, ideally have
> been tested also?
> 
> The rationale behind this is that by listing those strings here we
> reserve them, to cover for future extensions or bugs found in those
> SoCs. So even though the driver would only match on
> "allwinner,sun6i-a31-msgbox" right now, we can extend it in the future.
> The actual DTs would always use:
> 	compatible = "allwinner,sun50i-a64-msgbox",
> 		     "allwinner,sun6i-a31-msgbox";
> Right now the first string would not match anything, but the second
> would. The driver would use the generic driver functionality.
> Should we find a bug or speciality in the A64, we can add the a64 string
> to the driver and connect it to the workaround, and the actual DTs would
> not need to be touched.
> Does this make sense?

It does to me :)

> >> +- reg:			Contains the mailbox register address range (base
> >> +			address and length).
> >> +- clocks:		phandle for the clock controller and specifier.
> >> +- clock-names:		Must be "bus".
> 
> Maxime:
> Out of curiosity: There is only one clock, and just to gate the APB
> clock to the MMIO registers. So do we need clock-names?

We don't.

> Or is this good practise these days to allow extending the clocks
> later in a compatible manner? (Thinking of the sunxi pinctrl here.)

This is always doable, since if you ever have multiple clocks, you
will have clock-names, so any newer clock will have a name, and the
first one will usually be the first in the index. This is not as nice
as having it from the start, but it's definitely something we can have
in the binding as well.

> And is "bus" the correct naming? I see "apb" being used as well, is this
> legacy?

So we were using apb/ahb at the beginning, but we've had some IPs
moving from one generation to the other from the APB to AHB bus iirc
(or the other way around, this was quite some time ago).

Having some more generic name will also help to have genpd handle the
clocks and resets lines, which is something I've wanted to have for
quite some time, but is very low on my TODO-list...

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[PATCH 1/3] dt-bindings: Add a binding for the sunxi message box

[Maxime Ripard]

On Wed, Feb 28, 2018 at 05:17:03PM +0000, Andre Przywara wrote:
> Hi Samuel,
> 
> thank you very much for writing and posting this!
> 
> On 28/02/18 08:28, Maxime Ripard wrote:
> > Hi,
> > 
> > On Tue, Feb 27, 2018 at 08:27:12PM -0600, Samuel Holland wrote:
> >> This mailbox hardware is present in several Allwinner sun8i and sun50i
> >> SoCs. Add a device tree binding for it.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>  .../devicetree/bindings/mailbox/sunxi-msgbox.txt   | 40 ++++++++++++++++++++++
> >>  1 file changed, 40 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
> >> new file mode 100644
> >> index 000000000000..3b3ed7f870a0
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mailbox/sunxi-msgbox.txt
> >> @@ -0,0 +1,40 @@
> >> +Allwinner sunxi Message Box
> >> +===========================
> >> +
> >> +The hardware message box on sunxi SoCs is a two-user mailbox controller
> >> +containing 8 unidirectional FIFOs bonded into 4 bidirectional mailbox channels.
> >> +An interrupt is raised for received messages, but software must poll to know
> >> +when a transmitted message has been acknowledged by the remote user.
> >> +
> >> +Refer to ./mailbox.txt for generic information about mailbox device-tree
> >> +bindings.
> >> +
> >> +Mailbox Device Node:
> >> +====================
> >> +
> >> +Required properties:
> >> +--------------------
> >> +- compatible:		Must be "allwinner,sunxi-msgbox".
> > 
> > The IP change quite often in the Allwinner SoCs, so it would be better
> > to use a more specific compatible there. IIRC that IP was introduced
> > with the A31, so what about sun6i-a31-msgbox?
> 
> As much as I like generic compatible strings ;-) I agree with Maxime
> here. Can you change this to something like:
> 
> - compatible: Must contain at least "allwinner,sun6i-a31-msgbox".
>               Should also contain one of the more specific strings, when
> 	      used on other SoCs which are compatible:
> 
> And then list the strings for the SoCs which we know of, ideally have
> been tested also?
> 
> The rationale behind this is that by listing those strings here we
> reserve them, to cover for future extensions or bugs found in those
> SoCs. So even though the driver would only match on
> "allwinner,sun6i-a31-msgbox" right now, we can extend it in the future.
> The actual DTs would always use:
> 	compatible = "allwinner,sun50i-a64-msgbox",
> 		     "allwinner,sun6i-a31-msgbox";
> Right now the first string would not match anything, but the second
> would. The driver would use the generic driver functionality.
> Should we find a bug or speciality in the A64, we can add the a64 string
> to the driver and connect it to the workaround, and the actual DTs would
> not need to be touched.
> Does this make sense?

It does to me :)

> >> +- reg:			Contains the mailbox register address range (base
> >> +			address and length).
> >> +- clocks:		phandle for the clock controller and specifier.
> >> +- clock-names:		Must be "bus".
> 
> Maxime:
> Out of curiosity: There is only one clock, and just to gate the APB
> clock to the MMIO registers. So do we need clock-names?

We don't.

> Or is this good practise these days to allow extending the clocks
> later in a compatible manner? (Thinking of the sunxi pinctrl here.)

This is always doable, since if you ever have multiple clocks, you
will have clock-names, so any newer clock will have a name, and the
first one will usually be the first in the index. This is not as nice
as having it from the start, but it's definitely something we can have
in the binding as well.

> And is "bus" the correct naming? I see "apb" being used as well, is this
> legacy?

So we were using apb/ahb at the beginning, but we've had some IPs
moving from one generation to the other from the APB to AHB bus iirc
(or the other way around, this was quite some time ago).

Having some more generic name will also help to have genpd handle the
clocks and resets lines, which is something I've wanted to have for
quite some time, but is very low on my TODO-list...

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180301/19aad8d7/attachment-0001.sig>

Re: [PATCH 0/3] Allwinner sunxi message box support

[Maxime Ripard]

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

Hi,

On Wed, Feb 28, 2018 at 11:18:50AM -0600, Samuel Holland wrote:
> On 02/28/18 02:24, Maxime Ripard wrote:
> > On Tue, Feb 27, 2018 at 08:27:11PM -0600, Samuel Holland wrote:
> >> This series adds support for the "hardware message box" in recent
> >> Allwinner sunxi 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.
> >>
> >> This driver has been tested with the base arm_scpi driver (which sends
> >> the SCPI_CMD_SCPI_CAPABILITIES command at probe time) and an in-progress
> >> firmware application running on the ARISC CPU. Because no firmware for
> >> the other end of the mailbox is complete at the moment, I have not
> >> included changes to the SoC device trees.
> > 
> > This is not directly related to this series, but what is your end goal
> > with regard to SCPI? What features do you expect to have there?
> 
> Current plans are to support in SCPI:
> - SMP bringup and CPU hotplug (via PSCI in ATF)
> - System reset and poweroff (via PSCI in ATF)
> - System standby and suspend (via PSCI in ATF)
> - DVFS for ARM CPUs
> - Clock control for clocks in R_PRCM (e.g. R_CIR_RX)
> - Thermal sensor reading
> 
> Other planned features of the firmware:
> - System wakeup from poweroff (via GPIO or interrupt to firmware)
> - System power/suspend LED control (connected to GPIO or PMIC GPIO)
> - Polling of thermal sensor in firmware for thermal throttling

Nice.

> > I'm asking because last time we discussed this, SCPI wasn't able to abstract
> > all the features the PMIC provides, and thus Linux needed to still be able to
> > access it.
> 
> This should only be an issue for devices with AXP PMICs.

So this should only be an issue for most devices out there :)

> Other devices with GPIO or simple I²C regulator control, such as H3
> and H5 boards, can have all necessary features described with
> standard SCPI commands. SCPI provides for 127 vendor-defined
> "extended" commands, which could be used to build a full AXP PMIC
> driver, or simply provide commands to read/write PMIC registers.

So I'm not sure whether it was your plan, but the last plan that was
discussed was that the whole RSB bus would be put outside of Linux
control, hence why I was asking.

And in this case the RSB bus is used to access the PMIC, but also its
secondary devices such as the audio codec on the AC100/AC200, so we
really need to have access to the RSB bus in Linux. If you can
implement that through custom SCPI commands, that works for me though.

Another thing that I'm not quire sure about is how would interrupts be
reported throuh that mecanism? We want to use the PMIC as an interrupt
controller for things like VBUS detection, GPIOs or the Jack detect in
the codec. Is that doable?

> For the moment I'm not concerned with battery powered devices. So
> right now the relevant boards either a) don't have a PMIC, or b) can
> have everything but DVFS voltage hard-coded in the firmware.

I wasn't asking you to implement everything from the start, just that
you consider the rest in your design.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[PATCH 0/3] Allwinner sunxi message box support

[Maxime Ripard]

Hi,

On Wed, Feb 28, 2018 at 11:18:50AM -0600, Samuel Holland wrote:
> On 02/28/18 02:24, Maxime Ripard wrote:
> > On Tue, Feb 27, 2018 at 08:27:11PM -0600, Samuel Holland wrote:
> >> This series adds support for the "hardware message box" in recent
> >> Allwinner sunxi 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.
> >>
> >> This driver has been tested with the base arm_scpi driver (which sends
> >> the SCPI_CMD_SCPI_CAPABILITIES command at probe time) and an in-progress
> >> firmware application running on the ARISC CPU. Because no firmware for
> >> the other end of the mailbox is complete at the moment, I have not
> >> included changes to the SoC device trees.
> > 
> > This is not directly related to this series, but what is your end goal
> > with regard to SCPI? What features do you expect to have there?
> 
> Current plans are to support in SCPI:
> - SMP bringup and CPU hotplug (via PSCI in ATF)
> - System reset and poweroff (via PSCI in ATF)
> - System standby and suspend (via PSCI in ATF)
> - DVFS for ARM CPUs
> - Clock control for clocks in R_PRCM (e.g. R_CIR_RX)
> - Thermal sensor reading
> 
> Other planned features of the firmware:
> - System wakeup from poweroff (via GPIO or interrupt to firmware)
> - System power/suspend LED control (connected to GPIO or PMIC GPIO)
> - Polling of thermal sensor in firmware for thermal throttling

Nice.

> > I'm asking because last time we discussed this, SCPI wasn't able to abstract
> > all the features the PMIC provides, and thus Linux needed to still be able to
> > access it.
> 
> This should only be an issue for devices with AXP PMICs.

So this should only be an issue for most devices out there :)

> Other devices with GPIO or simple I?C regulator control, such as H3
> and H5 boards, can have all necessary features described with
> standard SCPI commands. SCPI provides for 127 vendor-defined
> "extended" commands, which could be used to build a full AXP PMIC
> driver, or simply provide commands to read/write PMIC registers.

So I'm not sure whether it was your plan, but the last plan that was
discussed was that the whole RSB bus would be put outside of Linux
control, hence why I was asking.

And in this case the RSB bus is used to access the PMIC, but also its
secondary devices such as the audio codec on the AC100/AC200, so we
really need to have access to the RSB bus in Linux. If you can
implement that through custom SCPI commands, that works for me though.

Another thing that I'm not quire sure about is how would interrupts be
reported throuh that mecanism? We want to use the PMIC as an interrupt
controller for things like VBUS detection, GPIOs or the Jack detect in
the codec. Is that doable?

> For the moment I'm not concerned with battery powered devices. So
> right now the relevant boards either a) don't have a PMIC, or b) can
> have everything but DVFS voltage hard-coded in the firmware.

I wasn't asking you to implement everything from the start, just that
you consider the rest in your design.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180301/25b77551/attachment.sig>

Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Maxime Ripard]

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

On Wed, Feb 28, 2018 at 11:19:11AM -0600, Samuel Holland wrote:
> Hi,
> 
> On 02/28/18 02:32, Maxime Ripard wrote:
> > On Tue, Feb 27, 2018 at 08:27:14PM -0600, Samuel Holland wrote:
> >> +	/*
> >> +	 * The failure path should not disable the clock or assert the reset,
> >> +	 * because the PSCI implementation in firmware relies on this device
> >> +	 * being functional. Claiming the clock in this driver is required to
> >> +	 * prevent Linux from turning it off.
> >> +	 */
> >> +	ret = clk_prepare_enable(clk);
> >> +	if (ret) {
> >> +		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
> >> +		return ret;
> >> +	}
> > 
> > You don't need it to be always on though. You only need it to be
> > enabled when you access the registers (on both sides I guess?). So you
> > could very well enable the clock in your registers accessors in Linux,
> > and do the same in the ARISC firmware. That should work.
> 
> It does need to always be on because the *PSCI* implementation (ATF) also uses
> SCPI concurrently with Linux (on a separate channel). Turning off the clock
> anywhere in Linux could turn it off in the middle of a PSCI SCPI call on a
> different CPU, causing ATF to hang in EL3 (which would be very bad).

Then the above code doesn't fix anything. You should mark the clock
critical, otherwise that clock will be turned off if the driver is
compiled as a module and not loaded (or loaded later), or if the
driver is not even compiled in.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Maxime Ripard]

On Wed, Feb 28, 2018 at 11:19:11AM -0600, Samuel Holland wrote:
> Hi,
> 
> On 02/28/18 02:32, Maxime Ripard wrote:
> > On Tue, Feb 27, 2018 at 08:27:14PM -0600, Samuel Holland wrote:
> >> +	/*
> >> +	 * The failure path should not disable the clock or assert the reset,
> >> +	 * because the PSCI implementation in firmware relies on this device
> >> +	 * being functional. Claiming the clock in this driver is required to
> >> +	 * prevent Linux from turning it off.
> >> +	 */
> >> +	ret = clk_prepare_enable(clk);
> >> +	if (ret) {
> >> +		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
> >> +		return ret;
> >> +	}
> > 
> > You don't need it to be always on though. You only need it to be
> > enabled when you access the registers (on both sides I guess?). So you
> > could very well enable the clock in your registers accessors in Linux,
> > and do the same in the ARISC firmware. That should work.
> 
> It does need to always be on because the *PSCI* implementation (ATF) also uses
> SCPI concurrently with Linux (on a separate channel). Turning off the clock
> anywhere in Linux could turn it off in the middle of a PSCI SCPI call on a
> different CPU, causing ATF to hang in EL3 (which would be very bad).

Then the above code doesn't fix anything. You should mark the clock
critical, otherwise that clock will be turned off if the driver is
compiled as a module and not loaded (or loaded later), or if the
driver is not even compiled in.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180301/a6441894/attachment.sig>

Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Andre Przywara]

Hi,

On 01/03/18 10:32, Maxime Ripard wrote:
> On Wed, Feb 28, 2018 at 11:19:11AM -0600, Samuel Holland wrote:
>> Hi,
>>
>> On 02/28/18 02:32, Maxime Ripard wrote:
>>> On Tue, Feb 27, 2018 at 08:27:14PM -0600, Samuel Holland wrote:
>>>> +	/*
>>>> +	 * The failure path should not disable the clock or assert the reset,
>>>> +	 * because the PSCI implementation in firmware relies on this device
>>>> +	 * being functional. Claiming the clock in this driver is required to
>>>> +	 * prevent Linux from turning it off.
>>>> +	 */
>>>> +	ret = clk_prepare_enable(clk);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>
>>> You don't need it to be always on though. You only need it to be
>>> enabled when you access the registers (on both sides I guess?). So you
>>> could very well enable the clock in your registers accessors in Linux,
>>> and do the same in the ARISC firmware. That should work.
>>
>> It does need to always be on because the *PSCI* implementation (ATF) also uses
>> SCPI concurrently with Linux (on a separate channel). Turning off the clock
>> anywhere in Linux could turn it off in the middle of a PSCI SCPI call on a
>> different CPU, causing ATF to hang in EL3 (which would be very bad).
> 
> Then the above code doesn't fix anything. You should mark the clock
> critical, otherwise that clock will be turned off if the driver is
> compiled as a module and not loaded (or loaded later), or if the
> driver is not even compiled in.

... or if the module gets unloaded for some reason. So yes, I agree.
Actually I was hitting this problem before on several occasions:
- If firmware (either ATF or on the ARISC) wants to check temperatures,
it needs to rely on Linux not turning off the THS clock - which it does
at the moment when there is no Linux driver.
- If an EFI framebuffer needs to keep running in Linux, we should not
turn off the HDMI and DE clocks. simplefb solves this, but efifb has no
simple way of copying this approach.
- If a type 1 hypervisor like Xen uses the serial console (and exports a
virtual console to the guest), Linux turns off the now apparently unused
UART0 gate clock, killing Xen's console. So booting Xen on Allwinner
requires clk_ignore_unused at the moment.

There are ways to mitigate or workaround each one of these, but I was
wondering if we should look at a more general approach.

The most flexible seems to have some "clock victim" DT node, somewhat
mimicking the simplefb solution: One DT node which just references
clocks that are used by other (firmware) components in the system and
which can't be protected otherwise.
Normally this node would be empty, but firmware can add clock references
the moment it needs one clock. So ATF could add the THS clock, and Xen
could add the UART0 gate clock. This could be done at runtime by the
firmware or hypervisor. Xen for instance already amends the DT before
passing it on to Dom0, so copying all the clock references from the UART
to this node would be easy to implement.

Does that sound worthwhile to pursue? I could sketch up something if
that makes sense.

Cheers,
Andre.

[PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Andre Przywara]

Hi,

On 01/03/18 10:32, Maxime Ripard wrote:
> On Wed, Feb 28, 2018 at 11:19:11AM -0600, Samuel Holland wrote:
>> Hi,
>>
>> On 02/28/18 02:32, Maxime Ripard wrote:
>>> On Tue, Feb 27, 2018 at 08:27:14PM -0600, Samuel Holland wrote:
>>>> +	/*
>>>> +	 * The failure path should not disable the clock or assert the reset,
>>>> +	 * because the PSCI implementation in firmware relies on this device
>>>> +	 * being functional. Claiming the clock in this driver is required to
>>>> +	 * prevent Linux from turning it off.
>>>> +	 */
>>>> +	ret = clk_prepare_enable(clk);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>
>>> You don't need it to be always on though. You only need it to be
>>> enabled when you access the registers (on both sides I guess?). So you
>>> could very well enable the clock in your registers accessors in Linux,
>>> and do the same in the ARISC firmware. That should work.
>>
>> It does need to always be on because the *PSCI* implementation (ATF) also uses
>> SCPI concurrently with Linux (on a separate channel). Turning off the clock
>> anywhere in Linux could turn it off in the middle of a PSCI SCPI call on a
>> different CPU, causing ATF to hang in EL3 (which would be very bad).
> 
> Then the above code doesn't fix anything. You should mark the clock
> critical, otherwise that clock will be turned off if the driver is
> compiled as a module and not loaded (or loaded later), or if the
> driver is not even compiled in.

... or if the module gets unloaded for some reason. So yes, I agree.
Actually I was hitting this problem before on several occasions:
- If firmware (either ATF or on the ARISC) wants to check temperatures,
it needs to rely on Linux not turning off the THS clock - which it does
at the moment when there is no Linux driver.
- If an EFI framebuffer needs to keep running in Linux, we should not
turn off the HDMI and DE clocks. simplefb solves this, but efifb has no
simple way of copying this approach.
- If a type 1 hypervisor like Xen uses the serial console (and exports a
virtual console to the guest), Linux turns off the now apparently unused
UART0 gate clock, killing Xen's console. So booting Xen on Allwinner
requires clk_ignore_unused at the moment.

There are ways to mitigate or workaround each one of these, but I was
wondering if we should look at a more general approach.

The most flexible seems to have some "clock victim" DT node, somewhat
mimicking the simplefb solution: One DT node which just references
clocks that are used by other (firmware) components in the system and
which can't be protected otherwise.
Normally this node would be empty, but firmware can add clock references
the moment it needs one clock. So ATF could add the THS clock, and Xen
could add the UART0 gate clock. This could be done at runtime by the
firmware or hypervisor. Xen for instance already amends the DT before
passing it on to Dom0, so copying all the clock references from the UART
to this node would be easy to implement.

Does that sound worthwhile to pursue? I could sketch up something if
that makes sense.

Cheers,
Andre.

Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Maxime Ripard]

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

On Thu, Mar 01, 2018 at 11:32:32AM +0000, Andre Przywara wrote:
> Hi,
> 
> On 01/03/18 10:32, Maxime Ripard wrote:
> > On Wed, Feb 28, 2018 at 11:19:11AM -0600, Samuel Holland wrote:
> >> Hi,
> >>
> >> On 02/28/18 02:32, Maxime Ripard wrote:
> >>> On Tue, Feb 27, 2018 at 08:27:14PM -0600, Samuel Holland wrote:
> >>>> +	/*
> >>>> +	 * The failure path should not disable the clock or assert the reset,
> >>>> +	 * because the PSCI implementation in firmware relies on this device
> >>>> +	 * being functional. Claiming the clock in this driver is required to
> >>>> +	 * prevent Linux from turning it off.
> >>>> +	 */
> >>>> +	ret = clk_prepare_enable(clk);
> >>>> +	if (ret) {
> >>>> +		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
> >>>> +		return ret;
> >>>> +	}
> >>>
> >>> You don't need it to be always on though. You only need it to be
> >>> enabled when you access the registers (on both sides I guess?). So you
> >>> could very well enable the clock in your registers accessors in Linux,
> >>> and do the same in the ARISC firmware. That should work.
> >>
> >> It does need to always be on because the *PSCI* implementation (ATF) also uses
> >> SCPI concurrently with Linux (on a separate channel). Turning off the clock
> >> anywhere in Linux could turn it off in the middle of a PSCI SCPI call on a
> >> different CPU, causing ATF to hang in EL3 (which would be very bad).
> > 
> > Then the above code doesn't fix anything. You should mark the clock
> > critical, otherwise that clock will be turned off if the driver is
> > compiled as a module and not loaded (or loaded later), or if the
> > driver is not even compiled in.
> 
> ... or if the module gets unloaded for some reason. So yes, I agree.
> Actually I was hitting this problem before on several occasions:
> - If firmware (either ATF or on the ARISC) wants to check temperatures,
> it needs to rely on Linux not turning off the THS clock - which it does
> at the moment when there is no Linux driver.
> - If an EFI framebuffer needs to keep running in Linux, we should not
> turn off the HDMI and DE clocks. simplefb solves this, but efifb has no
> simple way of copying this approach.
> - If a type 1 hypervisor like Xen uses the serial console (and exports a
> virtual console to the guest), Linux turns off the now apparently unused
> UART0 gate clock, killing Xen's console. So booting Xen on Allwinner
> requires clk_ignore_unused at the moment.
> 
> There are ways to mitigate or workaround each one of these, but I was
> wondering if we should look at a more general approach.
> 
> The most flexible seems to have some "clock victim" DT node, somewhat
> mimicking the simplefb solution: One DT node which just references
> clocks that are used by other (firmware) components in the system and
> which can't be protected otherwise.
> Normally this node would be empty, but firmware can add clock references
> the moment it needs one clock. So ATF could add the THS clock, and Xen
> could add the UART0 gate clock. This could be done at runtime by the
> firmware or hypervisor. Xen for instance already amends the DT before
> passing it on to Dom0, so copying all the clock references from the UART
> to this node would be easy to implement.
> 
> Does that sound worthwhile to pursue? I could sketch up something if
> that makes sense.

This makes sense, but I remember how heated the simplefb discussion
has been to introduce the clocks and regulator properties, so I'm not
sure this would be nice to encourage you to do that :)

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver

[Maxime Ripard]

On Thu, Mar 01, 2018 at 11:32:32AM +0000, Andre Przywara wrote:
> Hi,
> 
> On 01/03/18 10:32, Maxime Ripard wrote:
> > On Wed, Feb 28, 2018 at 11:19:11AM -0600, Samuel Holland wrote:
> >> Hi,
> >>
> >> On 02/28/18 02:32, Maxime Ripard wrote:
> >>> On Tue, Feb 27, 2018 at 08:27:14PM -0600, Samuel Holland wrote:
> >>>> +	/*
> >>>> +	 * The failure path should not disable the clock or assert the reset,
> >>>> +	 * because the PSCI implementation in firmware relies on this device
> >>>> +	 * being functional. Claiming the clock in this driver is required to
> >>>> +	 * prevent Linux from turning it off.
> >>>> +	 */
> >>>> +	ret = clk_prepare_enable(clk);
> >>>> +	if (ret) {
> >>>> +		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
> >>>> +		return ret;
> >>>> +	}
> >>>
> >>> You don't need it to be always on though. You only need it to be
> >>> enabled when you access the registers (on both sides I guess?). So you
> >>> could very well enable the clock in your registers accessors in Linux,
> >>> and do the same in the ARISC firmware. That should work.
> >>
> >> It does need to always be on because the *PSCI* implementation (ATF) also uses
> >> SCPI concurrently with Linux (on a separate channel). Turning off the clock
> >> anywhere in Linux could turn it off in the middle of a PSCI SCPI call on a
> >> different CPU, causing ATF to hang in EL3 (which would be very bad).
> > 
> > Then the above code doesn't fix anything. You should mark the clock
> > critical, otherwise that clock will be turned off if the driver is
> > compiled as a module and not loaded (or loaded later), or if the
> > driver is not even compiled in.
> 
> ... or if the module gets unloaded for some reason. So yes, I agree.
> Actually I was hitting this problem before on several occasions:
> - If firmware (either ATF or on the ARISC) wants to check temperatures,
> it needs to rely on Linux not turning off the THS clock - which it does
> at the moment when there is no Linux driver.
> - If an EFI framebuffer needs to keep running in Linux, we should not
> turn off the HDMI and DE clocks. simplefb solves this, but efifb has no
> simple way of copying this approach.
> - If a type 1 hypervisor like Xen uses the serial console (and exports a
> virtual console to the guest), Linux turns off the now apparently unused
> UART0 gate clock, killing Xen's console. So booting Xen on Allwinner
> requires clk_ignore_unused at the moment.
> 
> There are ways to mitigate or workaround each one of these, but I was
> wondering if we should look at a more general approach.
> 
> The most flexible seems to have some "clock victim" DT node, somewhat
> mimicking the simplefb solution: One DT node which just references
> clocks that are used by other (firmware) components in the system and
> which can't be protected otherwise.
> Normally this node would be empty, but firmware can add clock references
> the moment it needs one clock. So ATF could add the THS clock, and Xen
> could add the UART0 gate clock. This could be done at runtime by the
> firmware or hypervisor. Xen for instance already amends the DT before
> passing it on to Dom0, so copying all the clock references from the UART
> to this node would be easy to implement.
> 
> Does that sound worthwhile to pursue? I could sketch up something if
> that makes sense.

This makes sense, but I remember how heated the simplefb discussion
has been to introduce the clocks and regulator properties, so I'm not
sure this would be nice to encourage you to do that :)

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180301/3862db33/attachment.sig>

Re: [PATCH 2/3] mailbox: Avoid NULL dereference in mbox_chan_received_data

[Jassi Brar]

On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
> If a reception IRQ is pending when a mailbox channel is shut down (for
> example, if the controller uses threaded interrupts), it is possible for
> mbox_chan_received_data to be called while chan->cl is NULL.
>
We can add a check in mailbox.c, but that is like shoving it under the carpet.
How does your code look like?  mbox_chan_received_data() is meant to
be called from interrupt context as its documentation says.
Also the controller driver is responsible for ceasing any transient
xfer before returning from shutdown() callback.

Cheers!

[PATCH 2/3] mailbox: Avoid NULL dereference in mbox_chan_received_data

[Jassi Brar]

On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <samuel@sholland.org> wrote:
> If a reception IRQ is pending when a mailbox channel is shut down (for
> example, if the controller uses threaded interrupts), it is possible for
> mbox_chan_received_data to be called while chan->cl is NULL.
>
We can add a check in mailbox.c, but that is like shoving it under the carpet.
How does your code look like?  mbox_chan_received_data() is meant to
be called from interrupt context as its documentation says.
Also the controller driver is responsible for ceasing any transient
xfer before returning from shutdown() callback.

Cheers!