[PATCH V4 0/5] soc: imx: add scu firmware api support

[PATCH V4 0/5] soc: imx: add scu firmware api support

[Dong Aisheng]

Unlike the former i.MX Architectures, the new generation i.MX8 SoCs
(e.g. MX8QXP and MX8QM) contain a system controller which runs on a
dedicated Cortex-M core to provide power, clock, Pad, and resource
management. Communication between the host processor running
an OS and the system controller happens through a SCU protocol.
This patchset adds the SCU APIs which is implemented based on MU
and will be used by different system components.

It mainly consists of below parts:
1) MU mailbox driver
1) Implementation of the IPC functions based on MUs (client side).

Dong Aisheng (5):
  dt-bindings: mailbox: allow mbox-cells to be equal to 0
  dt-bindings: arm: fsl: add mu binding doc
  mailbox: imx: add imx mu support
  dt-bindings: arm: fsl: add scu binding doc
  soc: imx: add SC firmware IPC and APIs

 .../devicetree/bindings/arm/freescale/fsl,scu.txt  |  65 ++
 .../devicetree/bindings/mailbox/fsl,mu.txt         |  34 +
 .../devicetree/bindings/mailbox/mailbox.txt        |   3 +-
 drivers/mailbox/Kconfig                            |   8 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/imx-mu.c                           | 341 +++++++++
 drivers/soc/imx/Kconfig                            |   4 +
 drivers/soc/imx/Makefile                           |   1 +
 drivers/soc/imx/sc/Makefile                        |   2 +
 drivers/soc/imx/sc/main/ipc.c                      | 185 +++++
 drivers/soc/imx/sc/main/rpc.h                      |  79 +++
 include/soc/imx/sc/ipc.h                           |  45 ++
 include/soc/imx/sc/scfw.h                          |  24 +
 include/soc/imx/sc/types.h                         | 764 +++++++++++++++++++++
 14 files changed, 1555 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
 create mode 100644 Documentation/devicetree/bindings/mailbox/fsl,mu.txt
 create mode 100644 drivers/mailbox/imx-mu.c
 create mode 100644 drivers/soc/imx/sc/Makefile
 create mode 100644 drivers/soc/imx/sc/main/ipc.c
 create mode 100644 drivers/soc/imx/sc/main/rpc.h
 create mode 100644 include/soc/imx/sc/ipc.h
 create mode 100644 include/soc/imx/sc/scfw.h
 create mode 100644 include/soc/imx/sc/types.h

-- 
2.7.4

[PATCH V4 1/5] dt-bindings: mailbox: allow mbox-cells to be equal to 0

[Dong Aisheng]

Mailbox devices may have only one channel which means the mbox-cells
at least 1 does not make sense for this type devices. Let's remove
that limitation to allow the mbox-cells to be equal to 0.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
 * New patch introduced in v3 series
 * Actually there're already users in kernel with mbox-cells set to 0.
   See:
   arch/arm/boot/dts/bcm283x.dtsi:145: #mbox-cells = <0>;
---
 Documentation/devicetree/bindings/mailbox/mailbox.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/mailbox.txt b/Documentation/devicetree/bindings/mailbox/mailbox.txt
index af8ecee..c2fcd05 100644
--- a/Documentation/devicetree/bindings/mailbox/mailbox.txt
+++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt
@@ -6,8 +6,7 @@ assign appropriate mailbox channel to client drivers.
 * Mailbox Controller
 
 Required property:
-- #mbox-cells: Must be at least 1. Number of cells in a mailbox
-		specifier.
+- #mbox-cells: Number of cells in a mailbox specifier.
 
 Example:
 	mailbox: mailbox {
-- 
2.7.4

[PATCH V4 1/5] dt-bindings: mailbox: allow mbox-cells to be equal to 0

[Dong Aisheng]

Mailbox devices may have only one channel which means the mbox-cells
at least 1 does not make sense for this type devices. Let's remove
that limitation to allow the mbox-cells to be equal to 0.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: devicetree at vger.kernel.org
Cc: linux-kernel at vger.kernel.org
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
 * New patch introduced in v3 series
 * Actually there're already users in kernel with mbox-cells set to 0.
   See:
   arch/arm/boot/dts/bcm283x.dtsi:145: #mbox-cells = <0>;
---
 Documentation/devicetree/bindings/mailbox/mailbox.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/mailbox.txt b/Documentation/devicetree/bindings/mailbox/mailbox.txt
index af8ecee..c2fcd05 100644
--- a/Documentation/devicetree/bindings/mailbox/mailbox.txt
+++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt
@@ -6,8 +6,7 @@ assign appropriate mailbox channel to client drivers.
 * Mailbox Controller
 
 Required property:
-- #mbox-cells: Must be at least 1. Number of cells in a mailbox
-		specifier.
+- #mbox-cells: Number of cells in a mailbox specifier.
 
 Example:
 	mailbox: mailbox {
-- 
2.7.4

[PATCH V4 2/5] dt-bindings: arm: fsl: add mu binding doc

The Messaging Unit module enables two processors within
the SoC to communicate and coordinate by passing messages
(e.g. data, status and control) through the MU interface.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v3->v4:
 * move into Documentation/devicetree/bindings/mailbox/fsl,mu.txt
v2->v3:
 * change to mailbox binding
   Currently mbox-cells has to be 0 for SCU MU.
   Generic MU Mailbox support binding could be extended later.
v1->v2:
 * typo fixes
 * remove status property
 * remove imx6&7 compatible string which may be added later for
   the generic mailbox binding
---
 .../devicetree/bindings/mailbox/fsl,mu.txt         | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/fsl,mu.txt

diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
new file mode 100644
index 0000000..90e4905
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
@@ -0,0 +1,34 @@
+NXP i.MX Messaging Unit (MU)
+--------------------------------------------------------------------
+
+The Messaging Unit module enables two processors within the SoC to
+communicate and coordinate by passing messages (e.g. data, status
+and control) through the MU interface. The MU also provides the ability
+for one processor to signal the other processor using interrupts.
+
+Because the MU manages the messaging between processors, the MU uses
+different clocks (from each side of the different peripheral buses).
+Therefore, the MU must synchronize the accesses from one side to the
+other. The MU accomplishes synchronization using two sets of matching
+registers (Processor A-facing, Processor B-facing).
+
+Messaging Unit Device Node:
+=============================
+
+Required properties:
+-------------------
+- compatible :	should be "fsl,<chip>-mu", the supported chips include
+		imx8qxp, imx8qm.
+- reg :		Should contain the registers location and length
+- interrupts :	Interrupt number. The interrupt specifier format depends
+		on the interrupt controller parent.
+- #mbox-cells:  Must be 0. Number of cells in a mailbox
+
+Examples:
+--------
+lsio_mu0: mailbox@5d1b0000 {
+	compatible = "fsl,imx8qxp-mu";
+	reg = <0x0 0x5d1b0000 0x0 0x10000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	#mbox-cells = <0>;
+};
-- 
2.7.4

[PATCH V4 2/5] dt-bindings: arm: fsl: add mu binding doc

[Dong Aisheng]

The Messaging Unit module enables two processors within
the SoC to communicate and coordinate by passing messages
(e.g. data, status and control) through the MU interface.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree at vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v3->v4:
 * move into Documentation/devicetree/bindings/mailbox/fsl,mu.txt
v2->v3:
 * change to mailbox binding
   Currently mbox-cells has to be 0 for SCU MU.
   Generic MU Mailbox support binding could be extended later.
v1->v2:
 * typo fixes
 * remove status property
 * remove imx6&7 compatible string which may be added later for
   the generic mailbox binding
---
 .../devicetree/bindings/mailbox/fsl,mu.txt         | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/fsl,mu.txt

diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
new file mode 100644
index 0000000..90e4905
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
@@ -0,0 +1,34 @@
+NXP i.MX Messaging Unit (MU)
+--------------------------------------------------------------------
+
+The Messaging Unit module enables two processors within the SoC to
+communicate and coordinate by passing messages (e.g. data, status
+and control) through the MU interface. The MU also provides the ability
+for one processor to signal the other processor using interrupts.
+
+Because the MU manages the messaging between processors, the MU uses
+different clocks (from each side of the different peripheral buses).
+Therefore, the MU must synchronize the accesses from one side to the
+other. The MU accomplishes synchronization using two sets of matching
+registers (Processor A-facing, Processor B-facing).
+
+Messaging Unit Device Node:
+=============================
+
+Required properties:
+-------------------
+- compatible :	should be "fsl,<chip>-mu", the supported chips include
+		imx8qxp, imx8qm.
+- reg :		Should contain the registers location and length
+- interrupts :	Interrupt number. The interrupt specifier format depends
+		on the interrupt controller parent.
+- #mbox-cells:  Must be 0. Number of cells in a mailbox
+
+Examples:
+--------
+lsio_mu0: mailbox at 5d1b0000 {
+	compatible = "fsl,imx8qxp-mu";
+	reg = <0x0 0x5d1b0000 0x0 0x10000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	#mbox-cells = <0>;
+};
-- 
2.7.4

Re: [PATCH V4 2/5] dt-bindings: arm: fsl: add mu binding doc

[Rob Herring]

On Sun, Jul 08, 2018 at 10:56:54PM +0800, Dong Aisheng wrote:
> The Messaging Unit module enables two processors within
> the SoC to communicate and coordinate by passing messages
> (e.g. data, status and control) through the MU interface.
> 
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> v3->v4:
>  * move into Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> v2->v3:
>  * change to mailbox binding
>    Currently mbox-cells has to be 0 for SCU MU.
>    Generic MU Mailbox support binding could be extended later.
> v1->v2:
>  * typo fixes
>  * remove status property
>  * remove imx6&7 compatible string which may be added later for
>    the generic mailbox binding
> ---
>  .../devicetree/bindings/mailbox/fsl,mu.txt         | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/fsl,mu.txt

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

[PATCH V4 2/5] dt-bindings: arm: fsl: add mu binding doc

[Rob Herring]

On Sun, Jul 08, 2018 at 10:56:54PM +0800, Dong Aisheng wrote:
> The Messaging Unit module enables two processors within
> the SoC to communicate and coordinate by passing messages
> (e.g. data, status and control) through the MU interface.
> 
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree at vger.kernel.org
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> v3->v4:
>  * move into Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> v2->v3:
>  * change to mailbox binding
>    Currently mbox-cells has to be 0 for SCU MU.
>    Generic MU Mailbox support binding could be extended later.
> v1->v2:
>  * typo fixes
>  * remove status property
>  * remove imx6&7 compatible string which may be added later for
>    the generic mailbox binding
> ---
>  .../devicetree/bindings/mailbox/fsl,mu.txt         | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/fsl,mu.txt

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

[PATCH V4 3/5] mailbox: imx: add imx mu support

[Dong Aisheng]

This is used for i.MX multi core communication.
e.g. A core to SCU firmware(M core) on MX8.

Tx is using polling mode while Rx is interrupt driven and
schedule a hrtimer to receive remain words if have more than
4 words.

Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: linux-kernel@vger.kernel.org
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v3->v4:
 * New patch
   Old MU library dropped
---
 drivers/mailbox/Kconfig  |   8 ++
 drivers/mailbox/Makefile |   2 +
 drivers/mailbox/imx-mu.c | 341 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 351 insertions(+)
 create mode 100644 drivers/mailbox/imx-mu.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index a2bb274..4dd4823 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -124,6 +124,14 @@ config HI6220_MBOX
 	  between application processors and MCU. Say Y here if you want to
 	  build Hi6220 mailbox controller driver.
 
+config IMX_MU
+	tristate "IMX MU Mailbox"
+	depends on ARCH_MXC && OF
+	help
+	  An implementation of the i.MX MU Mailbox. It is used to send message
+	  between application processors and other processors/MCU/DSP. Select
+	  Y here if you want to use i.MX MU Mailbox controller.
+
 config MAILBOX_TEST
 	tristate "Mailbox Test Client"
 	depends on OF
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index cc23c3a..fa62f07 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -31,6 +31,8 @@ obj-$(CONFIG_HI3660_MBOX)	+= hi3660-mailbox.o
 
 obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
 
+obj-$(CONFIG_IMX_MU)		+= imx-mu.o
+
 obj-$(CONFIG_BCM_PDC_MBOX)	+= bcm-pdc-mailbox.o
 
 obj-$(CONFIG_BCM_FLEXRM_MBOX)	+= bcm-flexrm-mailbox.o
diff --git a/drivers/mailbox/imx-mu.c b/drivers/mailbox/imx-mu.c
new file mode 100644
index 0000000..339dfa9
--- /dev/null
+++ b/drivers/mailbox/imx-mu.c
@@ -0,0 +1,341 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018 NXP.
+ *
+ *  Author: Dong Aisheng <aisheng.dong@nxp.com>
+ *
+ */
+
+#include <linux/bitops.h>
+#include <linux/hrtimer.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define MU_xTR0		0x0
+#define MU_xRR0		0x10
+#define MU_xSR		0x20
+#define MU_xCR		0x24
+
+#define MU_TR_COUNT		4
+#define MU_RR_COUNT		4
+
+#define MU_CR_GIEn_MASK		GENMASK(37, 28)
+#define MU_CR_RIEn_MASK		GENMASK(27, 24)
+#define MU_CR_TIEn_MASK		GENMASK(23, 20)
+#define MU_CR_GIRn_MASK		GENMASK(19, 16)
+#define MU_CR_NMI_MASK		BIT(3)
+#define MU_CR_Fn_MASK		GENMASK(2, 0)
+#define MU_CR_RIE0_SHIFT	27
+#define MU_CR_TIE0_SHIFT	23
+#define MU_CR_RIE0_MASK		BIT(27)
+#define MU_CR_GIE0_MASK		BIT(31)
+
+#define MU_SR_TE0_MASK		BIT(23)
+#define MU_SR_RF0_MASK		BIT(27)
+
+#define MU_SCU_MAX_MSG		8
+#define MU_DATA_TIME_OUT_US	(100 * USEC_PER_MSEC)
+
+struct imx_mu {
+	void __iomem *regs;
+	struct mbox_controller mbox;
+	struct mbox_chan chans;
+	struct hrtimer poll_hrt;
+	/* for runtime scu msg store */
+	u32 *msg;
+};
+
+/*
+ * Wait to receive message from the other core.
+ */
+static int imx_mu_receive_msg(struct mbox_chan *chan, u32 index, u32 *msg)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u32 mask, sr;
+	int ret;
+
+	mask = MU_SR_RF0_MASK >> index;
+
+	/* Wait RX register to be full. */
+	ret = readl_poll_timeout_atomic(mu->regs + MU_xSR, sr, sr & mask,
+					0, MU_DATA_TIME_OUT_US);
+	if (ret) {
+		dev_err(chan->mbox->dev,
+			"Waiting MU receive register (%u) full timeout!\n",
+			index);
+		return ret;
+	}
+
+	*msg = readl(mu->regs + MU_xRR0 + (index * 4));
+
+	return 0;
+}
+
+/*
+ * Wait and send message to the other core.
+ */
+static int imx_mu_send_msg(struct mbox_chan *chan, u32 index, u32 msg)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u32 mask, sr;
+	int ret;
+
+	mask = MU_SR_TE0_MASK >> index;
+
+	/* Wait TX register to be empty. */
+	ret = readl_poll_timeout_atomic(mu->regs + MU_xSR, sr, sr & mask,
+					0, MU_DATA_TIME_OUT_US);
+	if (ret) {
+		dev_err(chan->mbox->dev,
+			"Waiting MU transmit register (%u) empty timeout!\n",
+			index);
+		return ret;
+	}
+
+	writel(msg, mu->regs + MU_xTR0  + (index * 4));
+
+	return 0;
+}
+
+static enum hrtimer_restart imx_chan_recv_hrtimer(struct hrtimer *hrtimer)
+{
+	struct imx_mu *mu = container_of(hrtimer, struct imx_mu, poll_hrt);
+	u8 *raw_data = (u8 *)mu->msg;
+	int size;
+	int ret;
+	int i;
+
+	/* check msg size */
+	raw_data = (u8 *)mu->msg;
+	size = raw_data[1];
+
+	dev_dbg(mu->mbox.dev, "receive the remain %d words\n", size - 1);
+
+	for (i = 1; i < size; i++) {
+		ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
+		if (ret)
+			break;
+	}
+
+	mbox_chan_received_data(&mu->chans, (void *)mu->msg);
+
+	return HRTIMER_NORESTART;
+}
+
+static irqreturn_t imx_channel_irq(int irq, void *data)
+{
+	struct imx_mu *mu = data;
+	u32 status, mask;
+	u8 *raw_data;
+	int size;
+	int i;
+
+	/* Only enabled RIE0 interrupt */
+	status = readl(mu->regs + MU_xSR);
+	if (!(status & MU_SR_RF0_MASK))
+		return IRQ_NONE;
+
+	/* Get the first data */
+	*mu->msg = readl(mu->regs + MU_xRR0);
+
+	/* check msg size */
+	raw_data = (u8 *)mu->msg;
+	size = raw_data[1];
+
+	dev_dbg(mu->mbox.dev, "receive data: head 0x%x\n", *mu->msg);
+
+	if (size > MU_SCU_MAX_MSG) {
+		dev_err(mu->mbox.dev,
+			"exceed the maximum scu msg size : %d\n", size);
+		return IRQ_HANDLED;
+
+	} else if (size > 1 && size <= 4) {
+		/* check remain data mask */
+		mask = GENMASK(MU_CR_RIE0_SHIFT	- 1, MU_CR_RIE0_SHIFT - size + 1);
+		status = readl(mu->regs + MU_xSR);
+
+		if ((status & mask) != mask) {
+			dev_dbg(mu->mbox.dev,
+				"start hrtimer to receive the remain %d words\n", size - 1);
+			hrtimer_start(&mu->poll_hrt, ktime_set(0, 0),
+				      HRTIMER_MODE_REL);
+			return IRQ_HANDLED;
+		}
+
+		dev_dbg(mu->mbox.dev,
+			"receive data: size %d mask 0x%x status 0x%x\n",
+			size, mask, status);
+
+		for (i = 1; i < size; i++)
+			mu->msg[i] = readl(mu->regs + MU_xRR0 + i * 4);
+
+	} else if (size > 4) {
+		dev_dbg(mu->mbox.dev,
+			"start hrtimer to receive the remain %d words\n", size - 1);
+		hrtimer_start(&mu->poll_hrt, ktime_set(0, 0),
+			      HRTIMER_MODE_REL);
+
+		return IRQ_HANDLED;
+	}
+
+	mbox_chan_received_data(&mu->chans, (void *)mu->msg);
+
+	return IRQ_HANDLED;
+}
+
+static int imx_mu_chan_send_data(struct mbox_chan *chan, void *data)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u8 *raw_data = data;
+	int i, ret;
+	int size;
+
+	if (!data)
+		return -EINVAL;
+
+	mu->msg = data;
+
+	/* SCU protocol size position is at the second u8 */
+	size = raw_data[1];
+
+	dev_dbg(mu->mbox.dev, "send data (size %d)\n", size);
+
+	for (i = 0; i < size; i++) {
+		ret = imx_mu_send_msg(chan, i % 4, *(mu->msg + i));
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int imx_mu_chan_startup(struct mbox_chan *chan)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u32 val;
+
+	/* Enable RIE0 interrupt */
+	val = readl(mu->regs + MU_xCR);
+	val |= MU_CR_RIE0_MASK;
+	writel(val, mu->regs + MU_xCR);
+
+	return 0;
+}
+
+static void imx_mu_chan_shutdown(struct mbox_chan *chan)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u32 val;
+
+	/* Clear RIEn, TIEn, GIRn and ABFn. */
+	val = readl(mu->regs + MU_xCR);
+	val &= ~(MU_CR_RIEn_MASK | MU_CR_TIEn_MASK |
+		 MU_CR_GIRn_MASK | MU_CR_NMI_MASK | MU_CR_Fn_MASK);
+	writel(val, mu->regs + MU_xCR);
+}
+
+static struct mbox_chan_ops imx_mu_chan_ops = {
+	.send_data = imx_mu_chan_send_data,
+	.startup = imx_mu_chan_startup,
+	.shutdown = imx_mu_chan_shutdown,
+};
+
+static struct mbox_chan *imx_mu_index_xlate(struct mbox_controller *mbox,
+					    const struct of_phandle_args *sp)
+{
+	if (sp->args_count != 0) {
+		dev_err(mbox->dev,
+			"incorrect mu channel specified in devicetree\n");
+		return NULL;
+	}
+
+	return &mbox->chans[0];
+}
+
+static int imx_mu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mbox_controller *mbox;
+	struct resource *res;
+	struct imx_mu *mu;
+	int irq, err;
+	u32 val;
+
+	mu = devm_kzalloc(&pdev->dev, sizeof(*mu), GFP_KERNEL);
+	if (!mu)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mu->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(mu->regs))
+		return PTR_ERR(mu->regs);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "can't get irq number\n");
+		return irq;
+	}
+
+	err = devm_request_irq(dev, irq, imx_channel_irq, 0,
+			       dev_name(dev), mu);
+	if (err < 0) {
+		dev_err(dev, "Failed to request IRQ#%u: %d\n", irq, err);
+		return err;
+	}
+
+	hrtimer_init(&mu->poll_hrt, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	mu->poll_hrt.function = imx_chan_recv_hrtimer;
+
+	platform_set_drvdata(pdev, mu);
+
+	mbox = &mu->mbox;
+	mbox->dev = dev;
+	mbox->num_chans = 1;
+	mbox->txdone_irq = false;
+	mbox->txdone_poll = false;
+	mbox->chans = &mu->chans;
+	mbox->ops = &imx_mu_chan_ops;
+	mbox->of_xlate = &imx_mu_index_xlate;
+	mu->chans.con_priv = mu;
+
+	/* Init MU */
+	val = readl(mu->regs + MU_xCR);
+	/* Clear GIEn, RIEn, TIEn, GIRn and ABFn. */
+	val &= ~(MU_CR_GIEn_MASK | MU_CR_RIEn_MASK | MU_CR_TIEn_MASK |
+		 MU_CR_GIRn_MASK | MU_CR_NMI_MASK | MU_CR_Fn_MASK);
+	writel(val, mu->regs + MU_xCR);
+
+	err = mbox_controller_register(mbox);
+	if (err) {
+		dev_err(&pdev->dev, "failed to register mailbox: %d\n", err);
+		return err;
+	}
+
+	dev_info(dev, "registered mailbox\n");
+
+	return 0;
+}
+
+static const struct of_device_id imx_mu_match[] = {
+	{ .compatible = "fsl,imx8qxp-mu", },
+	{ /* Sentinel */ }
+};
+
+static struct platform_driver imx_mu_driver = {
+	.driver = {
+		.name = "imx-mu",
+		.of_match_table = imx_mu_match,
+	},
+	.probe = imx_mu_probe,
+};
+
+static int __init imx_mu_init(void)
+{
+	return platform_driver_register(&imx_mu_driver);
+}
+core_initcall(imx_mu_init);
-- 
2.7.4

[PATCH V4 3/5] mailbox: imx: add imx mu support

[Dong Aisheng]

This is used for i.MX multi core communication.
e.g. A core to SCU firmware(M core) on MX8.

Tx is using polling mode while Rx is interrupt driven and
schedule a hrtimer to receive remain words if have more than
4 words.

Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: linux-kernel at vger.kernel.org
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v3->v4:
 * New patch
   Old MU library dropped
---
 drivers/mailbox/Kconfig  |   8 ++
 drivers/mailbox/Makefile |   2 +
 drivers/mailbox/imx-mu.c | 341 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 351 insertions(+)
 create mode 100644 drivers/mailbox/imx-mu.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index a2bb274..4dd4823 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -124,6 +124,14 @@ config HI6220_MBOX
 	  between application processors and MCU. Say Y here if you want to
 	  build Hi6220 mailbox controller driver.
 
+config IMX_MU
+	tristate "IMX MU Mailbox"
+	depends on ARCH_MXC && OF
+	help
+	  An implementation of the i.MX MU Mailbox. It is used to send message
+	  between application processors and other processors/MCU/DSP. Select
+	  Y here if you want to use i.MX MU Mailbox controller.
+
 config MAILBOX_TEST
 	tristate "Mailbox Test Client"
 	depends on OF
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index cc23c3a..fa62f07 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -31,6 +31,8 @@ obj-$(CONFIG_HI3660_MBOX)	+= hi3660-mailbox.o
 
 obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
 
+obj-$(CONFIG_IMX_MU)		+= imx-mu.o
+
 obj-$(CONFIG_BCM_PDC_MBOX)	+= bcm-pdc-mailbox.o
 
 obj-$(CONFIG_BCM_FLEXRM_MBOX)	+= bcm-flexrm-mailbox.o
diff --git a/drivers/mailbox/imx-mu.c b/drivers/mailbox/imx-mu.c
new file mode 100644
index 0000000..339dfa9
--- /dev/null
+++ b/drivers/mailbox/imx-mu.c
@@ -0,0 +1,341 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018 NXP.
+ *
+ *  Author: Dong Aisheng <aisheng.dong@nxp.com>
+ *
+ */
+
+#include <linux/bitops.h>
+#include <linux/hrtimer.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define MU_xTR0		0x0
+#define MU_xRR0		0x10
+#define MU_xSR		0x20
+#define MU_xCR		0x24
+
+#define MU_TR_COUNT		4
+#define MU_RR_COUNT		4
+
+#define MU_CR_GIEn_MASK		GENMASK(37, 28)
+#define MU_CR_RIEn_MASK		GENMASK(27, 24)
+#define MU_CR_TIEn_MASK		GENMASK(23, 20)
+#define MU_CR_GIRn_MASK		GENMASK(19, 16)
+#define MU_CR_NMI_MASK		BIT(3)
+#define MU_CR_Fn_MASK		GENMASK(2, 0)
+#define MU_CR_RIE0_SHIFT	27
+#define MU_CR_TIE0_SHIFT	23
+#define MU_CR_RIE0_MASK		BIT(27)
+#define MU_CR_GIE0_MASK		BIT(31)
+
+#define MU_SR_TE0_MASK		BIT(23)
+#define MU_SR_RF0_MASK		BIT(27)
+
+#define MU_SCU_MAX_MSG		8
+#define MU_DATA_TIME_OUT_US	(100 * USEC_PER_MSEC)
+
+struct imx_mu {
+	void __iomem *regs;
+	struct mbox_controller mbox;
+	struct mbox_chan chans;
+	struct hrtimer poll_hrt;
+	/* for runtime scu msg store */
+	u32 *msg;
+};
+
+/*
+ * Wait to receive message from the other core.
+ */
+static int imx_mu_receive_msg(struct mbox_chan *chan, u32 index, u32 *msg)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u32 mask, sr;
+	int ret;
+
+	mask = MU_SR_RF0_MASK >> index;
+
+	/* Wait RX register to be full. */
+	ret = readl_poll_timeout_atomic(mu->regs + MU_xSR, sr, sr & mask,
+					0, MU_DATA_TIME_OUT_US);
+	if (ret) {
+		dev_err(chan->mbox->dev,
+			"Waiting MU receive register (%u) full timeout!\n",
+			index);
+		return ret;
+	}
+
+	*msg = readl(mu->regs + MU_xRR0 + (index * 4));
+
+	return 0;
+}
+
+/*
+ * Wait and send message to the other core.
+ */
+static int imx_mu_send_msg(struct mbox_chan *chan, u32 index, u32 msg)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u32 mask, sr;
+	int ret;
+
+	mask = MU_SR_TE0_MASK >> index;
+
+	/* Wait TX register to be empty. */
+	ret = readl_poll_timeout_atomic(mu->regs + MU_xSR, sr, sr & mask,
+					0, MU_DATA_TIME_OUT_US);
+	if (ret) {
+		dev_err(chan->mbox->dev,
+			"Waiting MU transmit register (%u) empty timeout!\n",
+			index);
+		return ret;
+	}
+
+	writel(msg, mu->regs + MU_xTR0  + (index * 4));
+
+	return 0;
+}
+
+static enum hrtimer_restart imx_chan_recv_hrtimer(struct hrtimer *hrtimer)
+{
+	struct imx_mu *mu = container_of(hrtimer, struct imx_mu, poll_hrt);
+	u8 *raw_data = (u8 *)mu->msg;
+	int size;
+	int ret;
+	int i;
+
+	/* check msg size */
+	raw_data = (u8 *)mu->msg;
+	size = raw_data[1];
+
+	dev_dbg(mu->mbox.dev, "receive the remain %d words\n", size - 1);
+
+	for (i = 1; i < size; i++) {
+		ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
+		if (ret)
+			break;
+	}
+
+	mbox_chan_received_data(&mu->chans, (void *)mu->msg);
+
+	return HRTIMER_NORESTART;
+}
+
+static irqreturn_t imx_channel_irq(int irq, void *data)
+{
+	struct imx_mu *mu = data;
+	u32 status, mask;
+	u8 *raw_data;
+	int size;
+	int i;
+
+	/* Only enabled RIE0 interrupt */
+	status = readl(mu->regs + MU_xSR);
+	if (!(status & MU_SR_RF0_MASK))
+		return IRQ_NONE;
+
+	/* Get the first data */
+	*mu->msg = readl(mu->regs + MU_xRR0);
+
+	/* check msg size */
+	raw_data = (u8 *)mu->msg;
+	size = raw_data[1];
+
+	dev_dbg(mu->mbox.dev, "receive data: head 0x%x\n", *mu->msg);
+
+	if (size > MU_SCU_MAX_MSG) {
+		dev_err(mu->mbox.dev,
+			"exceed the maximum scu msg size : %d\n", size);
+		return IRQ_HANDLED;
+
+	} else if (size > 1 && size <= 4) {
+		/* check remain data mask */
+		mask = GENMASK(MU_CR_RIE0_SHIFT	- 1, MU_CR_RIE0_SHIFT - size + 1);
+		status = readl(mu->regs + MU_xSR);
+
+		if ((status & mask) != mask) {
+			dev_dbg(mu->mbox.dev,
+				"start hrtimer to receive the remain %d words\n", size - 1);
+			hrtimer_start(&mu->poll_hrt, ktime_set(0, 0),
+				      HRTIMER_MODE_REL);
+			return IRQ_HANDLED;
+		}
+
+		dev_dbg(mu->mbox.dev,
+			"receive data: size %d mask 0x%x status 0x%x\n",
+			size, mask, status);
+
+		for (i = 1; i < size; i++)
+			mu->msg[i] = readl(mu->regs + MU_xRR0 + i * 4);
+
+	} else if (size > 4) {
+		dev_dbg(mu->mbox.dev,
+			"start hrtimer to receive the remain %d words\n", size - 1);
+		hrtimer_start(&mu->poll_hrt, ktime_set(0, 0),
+			      HRTIMER_MODE_REL);
+
+		return IRQ_HANDLED;
+	}
+
+	mbox_chan_received_data(&mu->chans, (void *)mu->msg);
+
+	return IRQ_HANDLED;
+}
+
+static int imx_mu_chan_send_data(struct mbox_chan *chan, void *data)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u8 *raw_data = data;
+	int i, ret;
+	int size;
+
+	if (!data)
+		return -EINVAL;
+
+	mu->msg = data;
+
+	/* SCU protocol size position is at the second u8 */
+	size = raw_data[1];
+
+	dev_dbg(mu->mbox.dev, "send data (size %d)\n", size);
+
+	for (i = 0; i < size; i++) {
+		ret = imx_mu_send_msg(chan, i % 4, *(mu->msg + i));
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int imx_mu_chan_startup(struct mbox_chan *chan)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u32 val;
+
+	/* Enable RIE0 interrupt */
+	val = readl(mu->regs + MU_xCR);
+	val |= MU_CR_RIE0_MASK;
+	writel(val, mu->regs + MU_xCR);
+
+	return 0;
+}
+
+static void imx_mu_chan_shutdown(struct mbox_chan *chan)
+{
+	struct imx_mu *mu = chan->con_priv;
+	u32 val;
+
+	/* Clear RIEn, TIEn, GIRn and ABFn. */
+	val = readl(mu->regs + MU_xCR);
+	val &= ~(MU_CR_RIEn_MASK | MU_CR_TIEn_MASK |
+		 MU_CR_GIRn_MASK | MU_CR_NMI_MASK | MU_CR_Fn_MASK);
+	writel(val, mu->regs + MU_xCR);
+}
+
+static struct mbox_chan_ops imx_mu_chan_ops = {
+	.send_data = imx_mu_chan_send_data,
+	.startup = imx_mu_chan_startup,
+	.shutdown = imx_mu_chan_shutdown,
+};
+
+static struct mbox_chan *imx_mu_index_xlate(struct mbox_controller *mbox,
+					    const struct of_phandle_args *sp)
+{
+	if (sp->args_count != 0) {
+		dev_err(mbox->dev,
+			"incorrect mu channel specified in devicetree\n");
+		return NULL;
+	}
+
+	return &mbox->chans[0];
+}
+
+static int imx_mu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mbox_controller *mbox;
+	struct resource *res;
+	struct imx_mu *mu;
+	int irq, err;
+	u32 val;
+
+	mu = devm_kzalloc(&pdev->dev, sizeof(*mu), GFP_KERNEL);
+	if (!mu)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mu->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(mu->regs))
+		return PTR_ERR(mu->regs);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "can't get irq number\n");
+		return irq;
+	}
+
+	err = devm_request_irq(dev, irq, imx_channel_irq, 0,
+			       dev_name(dev), mu);
+	if (err < 0) {
+		dev_err(dev, "Failed to request IRQ#%u: %d\n", irq, err);
+		return err;
+	}
+
+	hrtimer_init(&mu->poll_hrt, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	mu->poll_hrt.function = imx_chan_recv_hrtimer;
+
+	platform_set_drvdata(pdev, mu);
+
+	mbox = &mu->mbox;
+	mbox->dev = dev;
+	mbox->num_chans = 1;
+	mbox->txdone_irq = false;
+	mbox->txdone_poll = false;
+	mbox->chans = &mu->chans;
+	mbox->ops = &imx_mu_chan_ops;
+	mbox->of_xlate = &imx_mu_index_xlate;
+	mu->chans.con_priv = mu;
+
+	/* Init MU */
+	val = readl(mu->regs + MU_xCR);
+	/* Clear GIEn, RIEn, TIEn, GIRn and ABFn. */
+	val &= ~(MU_CR_GIEn_MASK | MU_CR_RIEn_MASK | MU_CR_TIEn_MASK |
+		 MU_CR_GIRn_MASK | MU_CR_NMI_MASK | MU_CR_Fn_MASK);
+	writel(val, mu->regs + MU_xCR);
+
+	err = mbox_controller_register(mbox);
+	if (err) {
+		dev_err(&pdev->dev, "failed to register mailbox: %d\n", err);
+		return err;
+	}
+
+	dev_info(dev, "registered mailbox\n");
+
+	return 0;
+}
+
+static const struct of_device_id imx_mu_match[] = {
+	{ .compatible = "fsl,imx8qxp-mu", },
+	{ /* Sentinel */ }
+};
+
+static struct platform_driver imx_mu_driver = {
+	.driver = {
+		.name = "imx-mu",
+		.of_match_table = imx_mu_match,
+	},
+	.probe = imx_mu_probe,
+};
+
+static int __init imx_mu_init(void)
+{
+	return platform_driver_register(&imx_mu_driver);
+}
+core_initcall(imx_mu_init);
-- 
2.7.4

Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

[Sascha Hauer]

Hi,

On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> This is used for i.MX multi core communication.
> e.g. A core to SCU firmware(M core) on MX8.
> 
> Tx is using polling mode while Rx is interrupt driven and
> schedule a hrtimer to receive remain words if have more than
> 4 words.

You told us that using interrupts is not possible due to miserable
performance, we then provided you a way with which you could poll.  Why
are you using interrupts now?

We also suggested a way how the SCU mode could be integrated into the
generic MU support driver Oleksij posted and now you send a driver which
uses the same name as Oleksijs driver, but it only and exclusively works
in SCU mode. This doesn't bring us forward.

We suggested a binding that allows coexisting of the SCU mode and the
generic mode of the MU by putting the mode information into the second
mbox-cell. Why don't you use this?

I don't think it's necessary to rewrite Oleksijs driver, instead it
should rather be extended with the code I already provided as an
example. With that we could make both of us happy since we can both
have a suitable driver and even share most of the MU code.

Regards,
Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH V4 3/5] mailbox: imx: add imx mu support

[Sascha Hauer]

Hi,

On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> This is used for i.MX multi core communication.
> e.g. A core to SCU firmware(M core) on MX8.
> 
> Tx is using polling mode while Rx is interrupt driven and
> schedule a hrtimer to receive remain words if have more than
> 4 words.

You told us that using interrupts is not possible due to miserable
performance, we then provided you a way with which you could poll.  Why
are you using interrupts now?

We also suggested a way how the SCU mode could be integrated into the
generic MU support driver Oleksij posted and now you send a driver which
uses the same name as Oleksijs driver, but it only and exclusively works
in SCU mode. This doesn't bring us forward.

We suggested a binding that allows coexisting of the SCU mode and the
generic mode of the MU by putting the mode information into the second
mbox-cell. Why don't you use this?

I don't think it's necessary to rewrite Oleksijs driver, instead it
should rather be extended with the code I already provided as an
example. With that we could make both of us happy since we can both
have a suitable driver and even share most of the MU code.

Regards,
Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

Hi Sascha,

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> Sent: Tuesday, July 10, 2018 10:20 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi Brar
> <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org; Oleksij Rempel
> <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> shawnguo@kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> Hi,
> 
> On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > This is used for i.MX multi core communication.
> > e.g. A core to SCU firmware(M core) on MX8.
> >
> > Tx is using polling mode while Rx is interrupt driven and schedule a
> > hrtimer to receive remain words if have more than
> > 4 words.
> 
> You told us that using interrupts is not possible due to miserable
> performance, we then provided you a way with which you could poll.  Why
> are you using interrupts now?
> 

Because mailbox framework does not support sync rx now, I think we do not
need to wait for that feature done first as it's independent and  separate
features of framework. 

So for now, we're just using the common way in kernel as arm scpi and ti sci. 
When framework supports it, we can easily switch to it.

I optimized the performance a bit by removing the unnecessary memcopy
between tx/tx messages. The test result of booting time shows there's no
obvious regressions. I'm not sure whether it's due to we're booting a minimum
system or the extra cost is very minor to be noticed due to not too much cmds
sent during booting.
(Copy Peng to comments more as he tried and reported that performance drop
with vendor tree)

From the time measurement of sc_call_rpc, we can see that most rpc command
In polling mode can finish within 10us and very rare ones over 20us.
If switched to irq mode, those 10us cmds will change to about 20us. 

But the overall booting time did not increase much. Maybe the irq mode
also saves some CPU MIPS to execute other works in parallel?

> We also suggested a way how the SCU mode could be integrated into the
> generic MU support driver Oleksij posted and now you send a driver which
> uses the same name as Oleksijs driver, but it only and exclusively works in
> SCU mode. This doesn't bring us forward.
> 

Can Oleksij's patch be implemented against this one?
As I remember you said we've still not determined whether Oleksij's approach
is the most suitable way and it's still under discussion.
(Actually TI's approach looks better which is more simiar as SCU way?)

Furthermore, from this patch, you will notice that Oleksij's patch almost
did not work for SCU at all. I have to totally rewrite one for SCU.
So I did not write against his patch as it does not help.
And Oleksij's patch is quite simple while the SCU one is much complicated
than his one. So we probably better get SCU done first.

> We suggested a binding that allows coexisting of the SCU mode and the
> generic mode of the MU by putting the mode information into the second
> mbox-cell. Why don't you use this?
> 

You mean this?
+#define IMX_MU_CHANNEL0		0
+#define IMX_MU_CHANNEL1		1
+#define IMX_MU_CHANNEL2		2
+#define IMX_MU_CHANNEL3		3
+#define IMX_MU_CHANNEL_IMX8_SCU 4

It's hard for me to believe it's correct and it's over abstract to HW.
So I thought using mbox-cells to distinguish seems to be better.

> I don't think it's necessary to rewrite Oleksijs driver, instead it should rather
> be extended with the code I already provided as an example. With that we
> could make both of us happy since we can both have a suitable driver and
> even share most of the MU code.

As I said above, I even can't reuse 90%+ code of Oleksijs driver. So I can't
see the meaning to demo the code on top of this driver. We can review
the SCU implementation directly with this driver which is more easy.
Then we can decide how to merge them together.

Regards
Dong Aisheng

> 
> Regards,
> Sascha
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.pengutronix.de%2F&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7
> Cb359a3eddee54bf1b40a08d5e6702f22%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C636668291863846639&amp;sdata=hSucaLRfCB1j1McwlfO%
> 2FL0921QXiHg68sl%2B23CvEp4Q%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

Hi Sascha,

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Tuesday, July 10, 2018 10:20 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; Jassi Brar
> <jassisinghbrar@gmail.com>; linux-kernel at vger.kernel.org; Oleksij Rempel
> <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> shawnguo at kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> Hi,
> 
> On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > This is used for i.MX multi core communication.
> > e.g. A core to SCU firmware(M core) on MX8.
> >
> > Tx is using polling mode while Rx is interrupt driven and schedule a
> > hrtimer to receive remain words if have more than
> > 4 words.
> 
> You told us that using interrupts is not possible due to miserable
> performance, we then provided you a way with which you could poll.  Why
> are you using interrupts now?
> 

Because mailbox framework does not support sync rx now, I think we do not
need to wait for that feature done first as it's independent and  separate
features of framework. 

So for now, we're just using the common way in kernel as arm scpi and ti sci. 
When framework supports it, we can easily switch to it.

I optimized the performance a bit by removing the unnecessary memcopy
between tx/tx messages. The test result of booting time shows there's no
obvious regressions. I'm not sure whether it's due to we're booting a minimum
system or the extra cost is very minor to be noticed due to not too much cmds
sent during booting.
(Copy Peng to comments more as he tried and reported that performance drop
with vendor tree)

>From the time measurement of sc_call_rpc, we can see that most rpc command
In polling mode can finish within 10us and very rare ones over 20us.
If switched to irq mode, those 10us cmds will change to about 20us. 

But the overall booting time did not increase much. Maybe the irq mode
also saves some CPU MIPS to execute other works in parallel?

> We also suggested a way how the SCU mode could be integrated into the
> generic MU support driver Oleksij posted and now you send a driver which
> uses the same name as Oleksijs driver, but it only and exclusively works in
> SCU mode. This doesn't bring us forward.
> 

Can Oleksij's patch be implemented against this one?
As I remember you said we've still not determined whether Oleksij's approach
is the most suitable way and it's still under discussion.
(Actually TI's approach looks better which is more simiar as SCU way?)

Furthermore, from this patch, you will notice that Oleksij's patch almost
did not work for SCU at all. I have to totally rewrite one for SCU.
So I did not write against his patch as it does not help.
And Oleksij's patch is quite simple while the SCU one is much complicated
than his one. So we probably better get SCU done first.

> We suggested a binding that allows coexisting of the SCU mode and the
> generic mode of the MU by putting the mode information into the second
> mbox-cell. Why don't you use this?
> 

You mean this?
+#define IMX_MU_CHANNEL0		0
+#define IMX_MU_CHANNEL1		1
+#define IMX_MU_CHANNEL2		2
+#define IMX_MU_CHANNEL3		3
+#define IMX_MU_CHANNEL_IMX8_SCU 4

It's hard for me to believe it's correct and it's over abstract to HW.
So I thought using mbox-cells to distinguish seems to be better.

> I don't think it's necessary to rewrite Oleksijs driver, instead it should rather
> be extended with the code I already provided as an example. With that we
> could make both of us happy since we can both have a suitable driver and
> even share most of the MU code.

As I said above, I even can't reuse 90%+ code of Oleksijs driver. So I can't
see the meaning to demo the code on top of this driver. We can review
the SCU implementation directly with this driver which is more easy.
Then we can decide how to merge them together.

Regards
Dong Aisheng

> 
> Regards,
> Sascha
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.pengutronix.de%2F&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7
> Cb359a3eddee54bf1b40a08d5e6702f22%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C636668291863846639&amp;sdata=hSucaLRfCB1j1McwlfO%
> 2FL0921QXiHg68sl%2B23CvEp4Q%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

[Sascha Hauer]

On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> Hi Sascha,
> 
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > Sent: Tuesday, July 10, 2018 10:20 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi Brar
> > <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org; Oleksij Rempel
> > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > shawnguo@kernel.org
> > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > 
> > Hi,
> > 
> > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > This is used for i.MX multi core communication.
> > > e.g. A core to SCU firmware(M core) on MX8.
> > >
> > > Tx is using polling mode while Rx is interrupt driven and schedule a
> > > hrtimer to receive remain words if have more than
> > > 4 words.
> > 
> > You told us that using interrupts is not possible due to miserable
> > performance, we then provided you a way with which you could poll.  Why
> > are you using interrupts now?
> > 
> 
> Because mailbox framework does not support sync rx now, I think we do not
> need to wait for that feature done first as it's independent and  separate
> features of framework.

You can wait forever for this feature, nobody will add it for you. It's
up to you to add support for that feature. Who else should add this
feature if not you? And when will you add that feature if not now when
you actually need it?
It is common practice that you adjust the frameworks to your needs
rather than working around them.

> 
> So for now, we're just using the common way in kernel as arm scpi and ti sci. 
> When framework supports it, we can easily switch to it.
> 
> I optimized the performance a bit by removing the unnecessary memcopy
> between tx/tx messages. The test result of booting time shows there's no
> obvious regressions. I'm not sure whether it's due to we're booting a minimum
> system or the extra cost is very minor to be noticed due to not too much cmds
> sent during booting.

Your boot time argument seems to be a very weak one. unnecessary memcpys
of a few bytes only take a fraction of the time interrupt latencies
introduce.

> (Copy Peng to comments more as he tried and reported that performance drop
> with vendor tree)
> 
> From the time measurement of sc_call_rpc, we can see that most rpc command
> In polling mode can finish within 10us and very rare ones over 20us.
> If switched to irq mode, those 10us cmds will change to about 20us. 
> 
> But the overall booting time did not increase much. Maybe the irq mode
> also saves some CPU MIPS to execute other works in parallel?
> 
> > We also suggested a way how the SCU mode could be integrated into the
> > generic MU support driver Oleksij posted and now you send a driver which
> > uses the same name as Oleksijs driver, but it only and exclusively works in
> > SCU mode. This doesn't bring us forward.
> > 
> 
> Can Oleksij's patch be implemented against this one?
> As I remember you said we've still not determined whether Oleksij's approach
> is the most suitable way and it's still under discussion.
> (Actually TI's approach looks better which is more simiar as SCU way?)
> 
> Furthermore, from this patch, you will notice that Oleksij's patch almost
> did not work for SCU at all. I have to totally rewrite one for SCU.

You are kidding. You have to rewrite it because you didn't follow my
suggestion to implement polling in the mailbox framework.

> So I did not write against his patch as it does not help.
> And Oleksij's patch is quite simple while the SCU one is much complicated
> than his one. So we probably better get SCU done first.
> 
> > We suggested a binding that allows coexisting of the SCU mode and the
> > generic mode of the MU by putting the mode information into the second
> > mbox-cell. Why don't you use this?
> > 
> 
> You mean this?
> +#define IMX_MU_CHANNEL0		0
> +#define IMX_MU_CHANNEL1		1
> +#define IMX_MU_CHANNEL2		2
> +#define IMX_MU_CHANNEL3		3
> +#define IMX_MU_CHANNEL_IMX8_SCU 4

Yes.

> 
> It's hard for me to believe it's correct and it's over abstract to HW.
> So I thought using mbox-cells to distinguish seems to be better.

The MU has four rx registers, four tx registers and four interrupts.
What else should this be than four channels? The number of channels the
MU has is obviously defined by the software running at the other end
of the MU. Yes, the SCU makes one channel from it, in other cases it's
four channels.

> 
> > I don't think it's necessary to rewrite Oleksijs driver, instead it should rather
> > be extended with the code I already provided as an example. With that we
> > could make both of us happy since we can both have a suitable driver and
> > even share most of the MU code.
> 
> As I said above, I even can't reuse 90%+ code of Oleksijs driver. So I can't
> see the meaning to demo the code on top of this driver. We can review
> the SCU implementation directly with this driver which is more easy.
> Then we can decide how to merge them together.

You can only not share the code because the mailbox framework does not
support polling. And yes, I even provided some code which implements SCU
mode ontop of Oleksijs driver.

Honestly, what I expect from you is no rocket science and we would
already be there if we didn't spend our time discussing this.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH V4 3/5] mailbox: imx: add imx mu support

[Sascha Hauer]

On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> Hi Sascha,
> 
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Tuesday, July 10, 2018 10:20 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; Jassi Brar
> > <jassisinghbrar@gmail.com>; linux-kernel at vger.kernel.org; Oleksij Rempel
> > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > shawnguo at kernel.org
> > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > 
> > Hi,
> > 
> > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > This is used for i.MX multi core communication.
> > > e.g. A core to SCU firmware(M core) on MX8.
> > >
> > > Tx is using polling mode while Rx is interrupt driven and schedule a
> > > hrtimer to receive remain words if have more than
> > > 4 words.
> > 
> > You told us that using interrupts is not possible due to miserable
> > performance, we then provided you a way with which you could poll.  Why
> > are you using interrupts now?
> > 
> 
> Because mailbox framework does not support sync rx now, I think we do not
> need to wait for that feature done first as it's independent and  separate
> features of framework.

You can wait forever for this feature, nobody will add it for you. It's
up to you to add support for that feature. Who else should add this
feature if not you? And when will you add that feature if not now when
you actually need it?
It is common practice that you adjust the frameworks to your needs
rather than working around them.

> 
> So for now, we're just using the common way in kernel as arm scpi and ti sci. 
> When framework supports it, we can easily switch to it.
> 
> I optimized the performance a bit by removing the unnecessary memcopy
> between tx/tx messages. The test result of booting time shows there's no
> obvious regressions. I'm not sure whether it's due to we're booting a minimum
> system or the extra cost is very minor to be noticed due to not too much cmds
> sent during booting.

Your boot time argument seems to be a very weak one. unnecessary memcpys
of a few bytes only take a fraction of the time interrupt latencies
introduce.

> (Copy Peng to comments more as he tried and reported that performance drop
> with vendor tree)
> 
> From the time measurement of sc_call_rpc, we can see that most rpc command
> In polling mode can finish within 10us and very rare ones over 20us.
> If switched to irq mode, those 10us cmds will change to about 20us. 
> 
> But the overall booting time did not increase much. Maybe the irq mode
> also saves some CPU MIPS to execute other works in parallel?
> 
> > We also suggested a way how the SCU mode could be integrated into the
> > generic MU support driver Oleksij posted and now you send a driver which
> > uses the same name as Oleksijs driver, but it only and exclusively works in
> > SCU mode. This doesn't bring us forward.
> > 
> 
> Can Oleksij's patch be implemented against this one?
> As I remember you said we've still not determined whether Oleksij's approach
> is the most suitable way and it's still under discussion.
> (Actually TI's approach looks better which is more simiar as SCU way?)
> 
> Furthermore, from this patch, you will notice that Oleksij's patch almost
> did not work for SCU at all. I have to totally rewrite one for SCU.

You are kidding. You have to rewrite it because you didn't follow my
suggestion to implement polling in the mailbox framework.

> So I did not write against his patch as it does not help.
> And Oleksij's patch is quite simple while the SCU one is much complicated
> than his one. So we probably better get SCU done first.
> 
> > We suggested a binding that allows coexisting of the SCU mode and the
> > generic mode of the MU by putting the mode information into the second
> > mbox-cell. Why don't you use this?
> > 
> 
> You mean this?
> +#define IMX_MU_CHANNEL0		0
> +#define IMX_MU_CHANNEL1		1
> +#define IMX_MU_CHANNEL2		2
> +#define IMX_MU_CHANNEL3		3
> +#define IMX_MU_CHANNEL_IMX8_SCU 4

Yes.

> 
> It's hard for me to believe it's correct and it's over abstract to HW.
> So I thought using mbox-cells to distinguish seems to be better.

The MU has four rx registers, four tx registers and four interrupts.
What else should this be than four channels? The number of channels the
MU has is obviously defined by the software running at the other end
of the MU. Yes, the SCU makes one channel from it, in other cases it's
four channels.

> 
> > I don't think it's necessary to rewrite Oleksijs driver, instead it should rather
> > be extended with the code I already provided as an example. With that we
> > could make both of us happy since we can both have a suitable driver and
> > even share most of the MU code.
> 
> As I said above, I even can't reuse 90%+ code of Oleksijs driver. So I can't
> see the meaning to demo the code on top of this driver. We can review
> the SCU implementation directly with this driver which is more easy.
> Then we can decide how to merge them together.

You can only not share the code because the mailbox framework does not
support polling. And yes, I even provided some code which implements SCU
mode ontop of Oleksijs driver.

Honestly, what I expect from you is no rocket science and we would
already be there if we didn't spend our time discussing this.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> Sent: Wednesday, July 11, 2018 3:55 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi Brar
> <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org; Oleksij Rempel
> <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> shawnguo@kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> > Hi Sascha,
> >
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > > Sent: Tuesday, July 10, 2018 10:20 PM
> > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi
> > > Brar <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org;
> > > Oleksij Rempel <o.rempel@pengutronix.de>; dl-linux-imx
> > > <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> > > <fabio.estevam@nxp.com>; shawnguo@kernel.org
> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > >
> > > Hi,
> > >
> > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > > This is used for i.MX multi core communication.
> > > > e.g. A core to SCU firmware(M core) on MX8.
> > > >
> > > > Tx is using polling mode while Rx is interrupt driven and schedule
> > > > a hrtimer to receive remain words if have more than
> > > > 4 words.
> > >
> > > You told us that using interrupts is not possible due to miserable
> > > performance, we then provided you a way with which you could poll.
> > > Why are you using interrupts now?
> > >
> >
> > Because mailbox framework does not support sync rx now, I think we do
> > not need to wait for that feature done first as it's independent and
> > separate features of framework.
> 
> You can wait forever for this feature, nobody will add it for you. It's up to you
> to add support for that feature. Who else should add this feature if not you?
> And when will you add that feature if not now when you actually need it?
> It is common practice that you adjust the frameworks to your needs rather
> than working around them.
> 

I'm willing to add it. Just because you said Jassi already had the idea on how to
Implement it and does not add much complexity. So I just want to see his patches.
But if he did not work on it, I can also help on it.

Another reason is i did not observe regression as before, so it looks like the irq
is an applicable way currently and not must use polling at this point as that's
an independent new feature of framework. So irq mode is the quickest way
to demo the mbox using  as we already had experience before.

> >
> > So for now, we're just using the common way in kernel as arm scpi and ti sci.
> > When framework supports it, we can easily switch to it.
> >
> > I optimized the performance a bit by removing the unnecessary memcopy
> > between tx/tx messages. The test result of booting time shows there's
> > no obvious regressions. I'm not sure whether it's due to we're booting
> > a minimum system or the extra cost is very minor to be noticed due to
> > not too much cmds sent during booting.
> 
> Your boot time argument seems to be a very weak one. unnecessary
> memcpys of a few bytes only take a fraction of the time interrupt latencies
> introduce.
> 

Yes, interrupt latency may be the major part.

> > (Copy Peng to comments more as he tried and reported that performance
> > drop with vendor tree)
> >
> > From the time measurement of sc_call_rpc, we can see that most rpc
> > command In polling mode can finish within 10us and very rare ones over
> 20us.
> > If switched to irq mode, those 10us cmds will change to about 20us.
> >
> > But the overall booting time did not increase much. Maybe the irq mode
> > also saves some CPU MIPS to execute other works in parallel?
> >
> > > We also suggested a way how the SCU mode could be integrated into
> > > the generic MU support driver Oleksij posted and now you send a
> > > driver which uses the same name as Oleksijs driver, but it only and
> > > exclusively works in SCU mode. This doesn't bring us forward.
> > >
> >
> > Can Oleksij's patch be implemented against this one?
> > As I remember you said we've still not determined whether Oleksij's
> > approach is the most suitable way and it's still under discussion.
> > (Actually TI's approach looks better which is more simiar as SCU way?)
> >
> > Furthermore, from this patch, you will notice that Oleksij's patch
> > almost did not work for SCU at all. I have to totally rewrite one for SCU.
> 
> You are kidding. You have to rewrite it because you didn't follow my
> suggestion to implement polling in the mailbox framework.
> 

As I said, I did not observe regression when switch to irq way.
And irq way is the common mbox approach in kernel. So i just use
it to quickly demo the mbox using which we already implemented.
It's simple!

But I got your meaning that we must switch to polling mode
now, because we want to sync with M4, right?

> > So I did not write against his patch as it does not help.
> > And Oleksij's patch is quite simple while the SCU one is much
> > complicated than his one. So we probably better get SCU done first.
> >
> > > We suggested a binding that allows coexisting of the SCU mode and
> > > the generic mode of the MU by putting the mode information into the
> > > second mbox-cell. Why don't you use this?
> > >
> >
> > You mean this?
> > +#define IMX_MU_CHANNEL0		0
> > +#define IMX_MU_CHANNEL1		1
> > +#define IMX_MU_CHANNEL2		2
> > +#define IMX_MU_CHANNEL3		3
> > +#define IMX_MU_CHANNEL_IMX8_SCU 4
> 
> Yes.
> 
> >
> > It's hard for me to believe it's correct and it's over abstract to HW.
> > So I thought using mbox-cells to distinguish seems to be better.
> 
> The MU has four rx registers, four tx registers and four interrupts.
> What else should this be than four channels? The number of channels the
> MU has is obviously defined by the software running at the other end of the
> MU. Yes, the SCU makes one channel from it, in other cases it's four channels.
>

I'm not against 4 four channels. Just feel SCU using is strange because
SCU is using it as one channel from HW point of view. Not four.
And make them into four channels for M4 also lose the HW capability to send
Up to 4 words at one time. That's also an issue. 
(I guess we're assuming no actual users of multi words sending, right?)

So using mbox-cells to distinguish seems more reasonable.
e.g.
For one channel case: (communicate with SCU)
Mbox-cells = <0>;

Client_node {
	....
	Mbox =<&lsio_mu0>;
}

For 4 channel case: (communicate with M4)
Mbox-cells = <1>;

Client_node {
	....
	mbox =<&mu0 1>;
}

Anything wrong with it?

It seems more reflect the real HW and will not introduce confusion.

> >
> > > I don't think it's necessary to rewrite Oleksijs driver, instead it
> > > should rather be extended with the code I already provided as an
> > > example. With that we could make both of us happy since we can both
> > > have a suitable driver and even share most of the MU code.
> >
> > As I said above, I even can't reuse 90%+ code of Oleksijs driver. So I
> > can't see the meaning to demo the code on top of this driver. We can
> > review the SCU implementation directly with this driver which is more easy.
> > Then we can decide how to merge them together.
> 
> You can only not share the code because the mailbox framework does not
> support polling. And yes, I even provided some code which implements SCU
> mode ontop of Oleksijs driver.
> 
> Honestly, what I expect from you is no rocket science and we would already
> be there if we didn't spend our time discussing this.
> 

I actually spent a lot time on it to switch to mbox to demo the using step by step,
and try to merge them with M4 users later. Although I don't think it's quite worthy
to do that, but I still do it Just because I respect you guys.

From our point of view, we just tried to be more careful about the change request
as we have more information than outside.

I also don't want to stop at this point. So I will try to add sync polling in mailbox
Framework and switch to it now.

Regards
Dong Aisheng

> Sascha
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.pengutronix.de%2F&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7
> Cb49570c607cf4057563d08d5e7039a70%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C636668925017826212&amp;sdata=UT5AhUX%2FV7mCtUit0
> 1KSgJtf%2FdmOmmDMVz8EVebBaVk%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Wednesday, July 11, 2018 3:55 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; Jassi Brar
> <jassisinghbrar@gmail.com>; linux-kernel at vger.kernel.org; Oleksij Rempel
> <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> shawnguo at kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> > Hi Sascha,
> >
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > Sent: Tuesday, July 10, 2018 10:20 PM
> > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; Jassi
> > > Brar <jassisinghbrar@gmail.com>; linux-kernel at vger.kernel.org;
> > > Oleksij Rempel <o.rempel@pengutronix.de>; dl-linux-imx
> > > <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
> > > <fabio.estevam@nxp.com>; shawnguo at kernel.org
> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > >
> > > Hi,
> > >
> > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > > This is used for i.MX multi core communication.
> > > > e.g. A core to SCU firmware(M core) on MX8.
> > > >
> > > > Tx is using polling mode while Rx is interrupt driven and schedule
> > > > a hrtimer to receive remain words if have more than
> > > > 4 words.
> > >
> > > You told us that using interrupts is not possible due to miserable
> > > performance, we then provided you a way with which you could poll.
> > > Why are you using interrupts now?
> > >
> >
> > Because mailbox framework does not support sync rx now, I think we do
> > not need to wait for that feature done first as it's independent and
> > separate features of framework.
> 
> You can wait forever for this feature, nobody will add it for you. It's up to you
> to add support for that feature. Who else should add this feature if not you?
> And when will you add that feature if not now when you actually need it?
> It is common practice that you adjust the frameworks to your needs rather
> than working around them.
> 

I'm willing to add it. Just because you said Jassi already had the idea on how to
Implement it and does not add much complexity. So I just want to see his patches.
But if he did not work on it, I can also help on it.

Another reason is i did not observe regression as before, so it looks like the irq
is an applicable way currently and not must use polling at this point as that's
an independent new feature of framework. So irq mode is the quickest way
to demo the mbox using  as we already had experience before.

> >
> > So for now, we're just using the common way in kernel as arm scpi and ti sci.
> > When framework supports it, we can easily switch to it.
> >
> > I optimized the performance a bit by removing the unnecessary memcopy
> > between tx/tx messages. The test result of booting time shows there's
> > no obvious regressions. I'm not sure whether it's due to we're booting
> > a minimum system or the extra cost is very minor to be noticed due to
> > not too much cmds sent during booting.
> 
> Your boot time argument seems to be a very weak one. unnecessary
> memcpys of a few bytes only take a fraction of the time interrupt latencies
> introduce.
> 

Yes, interrupt latency may be the major part.

> > (Copy Peng to comments more as he tried and reported that performance
> > drop with vendor tree)
> >
> > From the time measurement of sc_call_rpc, we can see that most rpc
> > command In polling mode can finish within 10us and very rare ones over
> 20us.
> > If switched to irq mode, those 10us cmds will change to about 20us.
> >
> > But the overall booting time did not increase much. Maybe the irq mode
> > also saves some CPU MIPS to execute other works in parallel?
> >
> > > We also suggested a way how the SCU mode could be integrated into
> > > the generic MU support driver Oleksij posted and now you send a
> > > driver which uses the same name as Oleksijs driver, but it only and
> > > exclusively works in SCU mode. This doesn't bring us forward.
> > >
> >
> > Can Oleksij's patch be implemented against this one?
> > As I remember you said we've still not determined whether Oleksij's
> > approach is the most suitable way and it's still under discussion.
> > (Actually TI's approach looks better which is more simiar as SCU way?)
> >
> > Furthermore, from this patch, you will notice that Oleksij's patch
> > almost did not work for SCU at all. I have to totally rewrite one for SCU.
> 
> You are kidding. You have to rewrite it because you didn't follow my
> suggestion to implement polling in the mailbox framework.
> 

As I said, I did not observe regression when switch to irq way.
And irq way is the common mbox approach in kernel. So i just use
it to quickly demo the mbox using which we already implemented.
It's simple!

But I got your meaning that we must switch to polling mode
now, because we want to sync with M4, right?

> > So I did not write against his patch as it does not help.
> > And Oleksij's patch is quite simple while the SCU one is much
> > complicated than his one. So we probably better get SCU done first.
> >
> > > We suggested a binding that allows coexisting of the SCU mode and
> > > the generic mode of the MU by putting the mode information into the
> > > second mbox-cell. Why don't you use this?
> > >
> >
> > You mean this?
> > +#define IMX_MU_CHANNEL0		0
> > +#define IMX_MU_CHANNEL1		1
> > +#define IMX_MU_CHANNEL2		2
> > +#define IMX_MU_CHANNEL3		3
> > +#define IMX_MU_CHANNEL_IMX8_SCU 4
> 
> Yes.
> 
> >
> > It's hard for me to believe it's correct and it's over abstract to HW.
> > So I thought using mbox-cells to distinguish seems to be better.
> 
> The MU has four rx registers, four tx registers and four interrupts.
> What else should this be than four channels? The number of channels the
> MU has is obviously defined by the software running at the other end of the
> MU. Yes, the SCU makes one channel from it, in other cases it's four channels.
>

I'm not against 4 four channels. Just feel SCU using is strange because
SCU is using it as one channel from HW point of view. Not four.
And make them into four channels for M4 also lose the HW capability to send
Up to 4 words at one time. That's also an issue. 
(I guess we're assuming no actual users of multi words sending, right?)

So using mbox-cells to distinguish seems more reasonable.
e.g.
For one channel case: (communicate with SCU)
Mbox-cells = <0>;

Client_node {
	....
	Mbox =<&lsio_mu0>;
}

For 4 channel case: (communicate with M4)
Mbox-cells = <1>;

Client_node {
	....
	mbox =<&mu0 1>;
}

Anything wrong with it?

It seems more reflect the real HW and will not introduce confusion.

> >
> > > I don't think it's necessary to rewrite Oleksijs driver, instead it
> > > should rather be extended with the code I already provided as an
> > > example. With that we could make both of us happy since we can both
> > > have a suitable driver and even share most of the MU code.
> >
> > As I said above, I even can't reuse 90%+ code of Oleksijs driver. So I
> > can't see the meaning to demo the code on top of this driver. We can
> > review the SCU implementation directly with this driver which is more easy.
> > Then we can decide how to merge them together.
> 
> You can only not share the code because the mailbox framework does not
> support polling. And yes, I even provided some code which implements SCU
> mode ontop of Oleksijs driver.
> 
> Honestly, what I expect from you is no rocket science and we would already
> be there if we didn't spend our time discussing this.
> 

I actually spent a lot time on it to switch to mbox to demo the using step by step,
and try to merge them with M4 users later. Although I don't think it's quite worthy
to do that, but I still do it Just because I respect you guys.

>From our point of view, we just tried to be more careful about the change request
as we have more information than outside.

I also don't want to stop at this point. So I will try to add sync polling in mailbox
Framework and switch to it now.

Regards
Dong Aisheng

> Sascha
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.pengutronix.de%2F&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7
> Cb49570c607cf4057563d08d5e7039a70%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C636668925017826212&amp;sdata=UT5AhUX%2FV7mCtUit0
> 1KSgJtf%2FdmOmmDMVz8EVebBaVk%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

[Jassi Brar]

On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > Sent: Wednesday, July 11, 2018 3:55 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi Brar
> > <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org; Oleksij Rempel
> > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > shawnguo@kernel.org
> > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >
> > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> > > Hi Sascha,
> > >
> > > > -----Original Message-----
> > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > > > Sent: Tuesday, July 10, 2018 10:20 PM
> > > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi
> > > > Brar <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org;
> > > > Oleksij Rempel <o.rempel@pengutronix.de>; dl-linux-imx
> > > > <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> > > > <fabio.estevam@nxp.com>; shawnguo@kernel.org
> > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > > >
> > > > Hi,
> > > >
> > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > > > This is used for i.MX multi core communication.
> > > > > e.g. A core to SCU firmware(M core) on MX8.
> > > > >
> > > > > Tx is using polling mode while Rx is interrupt driven and schedule
> > > > > a hrtimer to receive remain words if have more than
> > > > > 4 words.
> > > >
> > > > You told us that using interrupts is not possible due to miserable
> > > > performance, we then provided you a way with which you could poll.
> > > > Why are you using interrupts now?
> > > >
> > >
> > > Because mailbox framework does not support sync rx now, I think we do
> > > not need to wait for that feature done first as it's independent and
> > > separate features of framework.
> >
> > You can wait forever for this feature, nobody will add it for you. It's up to you
> > to add support for that feature. Who else should add this feature if not you?
> > And when will you add that feature if not now when you actually need it?
> > It is common practice that you adjust the frameworks to your needs rather
> > than working around them.
> >
>
> I'm willing to add it. Just because you said Jassi already had the idea on how to
> Implement it and does not add much complexity. So I just want to see his patches.
> But if he did not work on it, I can also help on it.
>
I am not much aware of the history of this conversation... but it
seems you need to make use of mbox_chan_ops.peek_data().

If not that, please let me know the requirement.

Cheers!

[PATCH V4 3/5] mailbox: imx: add imx mu support

[Jassi Brar]

On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Wednesday, July 11, 2018 3:55 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; Jassi Brar
> > <jassisinghbrar@gmail.com>; linux-kernel at vger.kernel.org; Oleksij Rempel
> > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > shawnguo at kernel.org
> > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >
> > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> > > Hi Sascha,
> > >
> > > > -----Original Message-----
> > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > Sent: Tuesday, July 10, 2018 10:20 PM
> > > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; Jassi
> > > > Brar <jassisinghbrar@gmail.com>; linux-kernel at vger.kernel.org;
> > > > Oleksij Rempel <o.rempel@pengutronix.de>; dl-linux-imx
> > > > <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
> > > > <fabio.estevam@nxp.com>; shawnguo at kernel.org
> > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > > >
> > > > Hi,
> > > >
> > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > > > This is used for i.MX multi core communication.
> > > > > e.g. A core to SCU firmware(M core) on MX8.
> > > > >
> > > > > Tx is using polling mode while Rx is interrupt driven and schedule
> > > > > a hrtimer to receive remain words if have more than
> > > > > 4 words.
> > > >
> > > > You told us that using interrupts is not possible due to miserable
> > > > performance, we then provided you a way with which you could poll.
> > > > Why are you using interrupts now?
> > > >
> > >
> > > Because mailbox framework does not support sync rx now, I think we do
> > > not need to wait for that feature done first as it's independent and
> > > separate features of framework.
> >
> > You can wait forever for this feature, nobody will add it for you. It's up to you
> > to add support for that feature. Who else should add this feature if not you?
> > And when will you add that feature if not now when you actually need it?
> > It is common practice that you adjust the frameworks to your needs rather
> > than working around them.
> >
>
> I'm willing to add it. Just because you said Jassi already had the idea on how to
> Implement it and does not add much complexity. So I just want to see his patches.
> But if he did not work on it, I can also help on it.
>
I am not much aware of the history of this conversation... but it
seems you need to make use of mbox_chan_ops.peek_data().

If not that, please let me know the requirement.

Cheers!

RE: [PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Wednesday, July 11, 2018 6:44 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
> kernel@lists.infradead.org; dongas86@gmail.com; linux-
> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo@kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> >
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > > Sent: Wednesday, July 11, 2018 3:55 PM
> > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi
> > > Brar <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org;
> > > Oleksij Rempel <o.rempel@pengutronix.de>; dl-linux-imx
> > > <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> > > <fabio.estevam@nxp.com>; shawnguo@kernel.org
> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > >
> > > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> > > > Hi Sascha,
> > > >
> > > > > -----Original Message-----
> > > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > > > > Sent: Tuesday, July 10, 2018 10:20 PM
> > > > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com;
> > > > > Jassi Brar <jassisinghbrar@gmail.com>;
> > > > > linux-kernel@vger.kernel.org; Oleksij Rempel
> > > > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > > > > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > > > > shawnguo@kernel.org
> > > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > > > > This is used for i.MX multi core communication.
> > > > > > e.g. A core to SCU firmware(M core) on MX8.
> > > > > >
> > > > > > Tx is using polling mode while Rx is interrupt driven and
> > > > > > schedule a hrtimer to receive remain words if have more than
> > > > > > 4 words.
> > > > >
> > > > > You told us that using interrupts is not possible due to
> > > > > miserable performance, we then provided you a way with which you
> could poll.
> > > > > Why are you using interrupts now?
> > > > >
> > > >
> > > > Because mailbox framework does not support sync rx now, I think we
> > > > do not need to wait for that feature done first as it's
> > > > independent and separate features of framework.
> > >
> > > You can wait forever for this feature, nobody will add it for you.
> > > It's up to you to add support for that feature. Who else should add this
> feature if not you?
> > > And when will you add that feature if not now when you actually need it?
> > > It is common practice that you adjust the frameworks to your needs
> > > rather than working around them.
> > >
> >
> > I'm willing to add it. Just because you said Jassi already had the
> > idea on how to Implement it and does not add much complexity. So I just
> want to see his patches.
> > But if he did not work on it, I can also help on it.
> >
> I am not much aware of the history of this conversation... but it seems you
> need to make use of mbox_chan_ops.peek_data().
> 
> If not that, please let me know the requirement.
> 

Thanks for the suggestion.
It looks to me may work.

From the definition, it seems it's used to pull data from remote side.
/**
 * mbox_client_peek_data - A way for client driver to pull data
 *                      received from remote by the controller.
 * @chan: Mailbox channel assigned to this client.
 *          
 * A poke to controller driver for any received data.
 * The data is actually passed onto client via the
 * mbox_chan_received_data()
 * The call can be made from atomic context, so the controller's
 * implementation of peek_data() must not sleep.
 *
 * Return: True, if controller has, and is going to push after this,
 *          some data.
 *         False, if controller doesn't have any data to be read.
 */     
bool mbox_client_peek_data(struct mbox_chan *chan)
{
        if (chan->mbox->ops->peek_data)
                return chan->mbox->ops->peek_data(chan);
        
        return false;
}       
EXPORT_SYMBOL_GPL(mbox_client_peek_data);
But it seems most users in kernel simply implement it as a data available
Checking rather than receiving it.
See:
drivers/mailbox/ti-msgmgr.c
drivers/mailbox/mailbox-altera.c

Only bcm uses it to receive data.
drivers/mailbox/bcm-flexrm-mailbox.c

For our requirement, we want to implement sync receiving protocol like:
Sc_call_rpc()
{
	mbox_send_message(chan, msg)
	If (!no_resp)
		// rx also stored in msg
		mbox_receive_msg_in_polling(chan, msg);
	mbox_client_txdone();
}

If using peek_data, it can be:
Sc_call_rpc()
{
	mbox_send_message(chan, msg)
	If (!no_resp)
		// rx also stored in msg
		Mbox_client_peek_data(chan);
	mbox_client_txdone();
}

And for mu controller driver .peek_data():
imx_mu_peek_data(chan)
{
        // get first word and parse data size
        imx_mu_receive_msg(&mu->chans, 0, mu->msg);

        raw_data = (u8 *)mu->msg;
        size = raw_data[1];

        // receive rest of them 
        for (i = 1; i < size; i++) {
                ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
                if (ret)
                        return false;
        }

        mbox_chan_received_data(&mu->chans, (void *)mu->msg);

        return true;
}
It is much specific to SCU.
Do you think such peek_data using for sync receiving mode is ok?

Regards
Dong Aisheng

> Cheers!

[PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar at gmail.com]
> Sent: Wednesday, July 11, 2018 6:44 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
> kernel at lists.infradead.org; dongas86 at gmail.com; linux-
> kernel at vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
> linux-imx <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo at kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> >
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > Sent: Wednesday, July 11, 2018 3:55 PM
> > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; Jassi
> > > Brar <jassisinghbrar@gmail.com>; linux-kernel at vger.kernel.org;
> > > Oleksij Rempel <o.rempel@pengutronix.de>; dl-linux-imx
> > > <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
> > > <fabio.estevam@nxp.com>; shawnguo at kernel.org
> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > >
> > > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> > > > Hi Sascha,
> > > >
> > > > > -----Original Message-----
> > > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > > Sent: Tuesday, July 10, 2018 10:20 PM
> > > > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > > > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com;
> > > > > Jassi Brar <jassisinghbrar@gmail.com>;
> > > > > linux-kernel at vger.kernel.org; Oleksij Rempel
> > > > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > > > > kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > > > > shawnguo at kernel.org
> > > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > > > > This is used for i.MX multi core communication.
> > > > > > e.g. A core to SCU firmware(M core) on MX8.
> > > > > >
> > > > > > Tx is using polling mode while Rx is interrupt driven and
> > > > > > schedule a hrtimer to receive remain words if have more than
> > > > > > 4 words.
> > > > >
> > > > > You told us that using interrupts is not possible due to
> > > > > miserable performance, we then provided you a way with which you
> could poll.
> > > > > Why are you using interrupts now?
> > > > >
> > > >
> > > > Because mailbox framework does not support sync rx now, I think we
> > > > do not need to wait for that feature done first as it's
> > > > independent and separate features of framework.
> > >
> > > You can wait forever for this feature, nobody will add it for you.
> > > It's up to you to add support for that feature. Who else should add this
> feature if not you?
> > > And when will you add that feature if not now when you actually need it?
> > > It is common practice that you adjust the frameworks to your needs
> > > rather than working around them.
> > >
> >
> > I'm willing to add it. Just because you said Jassi already had the
> > idea on how to Implement it and does not add much complexity. So I just
> want to see his patches.
> > But if he did not work on it, I can also help on it.
> >
> I am not much aware of the history of this conversation... but it seems you
> need to make use of mbox_chan_ops.peek_data().
> 
> If not that, please let me know the requirement.
> 

Thanks for the suggestion.
It looks to me may work.

>From the definition, it seems it's used to pull data from remote side.
/**
 * mbox_client_peek_data - A way for client driver to pull data
 *                      received from remote by the controller.
 * @chan: Mailbox channel assigned to this client.
 *          
 * A poke to controller driver for any received data.
 * The data is actually passed onto client via the
 * mbox_chan_received_data()
 * The call can be made from atomic context, so the controller's
 * implementation of peek_data() must not sleep.
 *
 * Return: True, if controller has, and is going to push after this,
 *          some data.
 *         False, if controller doesn't have any data to be read.
 */     
bool mbox_client_peek_data(struct mbox_chan *chan)
{
        if (chan->mbox->ops->peek_data)
                return chan->mbox->ops->peek_data(chan);
        
        return false;
}       
EXPORT_SYMBOL_GPL(mbox_client_peek_data);
But it seems most users in kernel simply implement it as a data available
Checking rather than receiving it.
See:
drivers/mailbox/ti-msgmgr.c
drivers/mailbox/mailbox-altera.c

Only bcm uses it to receive data.
drivers/mailbox/bcm-flexrm-mailbox.c

For our requirement, we want to implement sync receiving protocol like:
Sc_call_rpc()
{
	mbox_send_message(chan, msg)
	If (!no_resp)
		// rx also stored in msg
		mbox_receive_msg_in_polling(chan, msg);
	mbox_client_txdone();
}

If using peek_data, it can be:
Sc_call_rpc()
{
	mbox_send_message(chan, msg)
	If (!no_resp)
		// rx also stored in msg
		Mbox_client_peek_data(chan);
	mbox_client_txdone();
}

And for mu controller driver .peek_data():
imx_mu_peek_data(chan)
{
        // get first word and parse data size
        imx_mu_receive_msg(&mu->chans, 0, mu->msg);

        raw_data = (u8 *)mu->msg;
        size = raw_data[1];

        // receive rest of them 
        for (i = 1; i < size; i++) {
                ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
                if (ret)
                        return false;
        }

        mbox_chan_received_data(&mu->chans, (void *)mu->msg);

        return true;
}
It is much specific to SCU.
Do you think such peek_data using for sync receiving mode is ok?

Regards
Dong Aisheng

> Cheers!

Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

[Jassi Brar]

On Wed, Jul 11, 2018 at 6:28 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> Hi Jassi,
>
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
>> Sent: Wednesday, July 11, 2018 6:44 PM
>> To: A.s. Dong <aisheng.dong@nxp.com>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
>> kernel@lists.infradead.org; dongas86@gmail.com; linux-
>> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
>> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
>> <fabio.estevam@nxp.com>; shawnguo@kernel.org
>> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>>
>> On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> >
>> > > -----Original Message-----
>> > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
>> > > Sent: Wednesday, July 11, 2018 3:55 PM
>> > > To: A.s. Dong <aisheng.dong@nxp.com>
>> > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi
>> > > Brar <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org;
>> > > Oleksij Rempel <o.rempel@pengutronix.de>; dl-linux-imx
>> > > <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
>> > > <fabio.estevam@nxp.com>; shawnguo@kernel.org
>> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> > >
>> > > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
>> > > > Hi Sascha,
>> > > >
>> > > > > -----Original Message-----
>> > > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
>> > > > > Sent: Tuesday, July 10, 2018 10:20 PM
>> > > > > To: A.s. Dong <aisheng.dong@nxp.com>
>> > > > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com;
>> > > > > Jassi Brar <jassisinghbrar@gmail.com>;
>> > > > > linux-kernel@vger.kernel.org; Oleksij Rempel
>> > > > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
>> > > > > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
>> > > > > shawnguo@kernel.org
>> > > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> > > > >
>> > > > > Hi,
>> > > > >
>> > > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
>> > > > > > This is used for i.MX multi core communication.
>> > > > > > e.g. A core to SCU firmware(M core) on MX8.
>> > > > > >
>> > > > > > Tx is using polling mode while Rx is interrupt driven and
>> > > > > > schedule a hrtimer to receive remain words if have more than
>> > > > > > 4 words.
>> > > > >
>> > > > > You told us that using interrupts is not possible due to
>> > > > > miserable performance, we then provided you a way with which you
>> could poll.
>> > > > > Why are you using interrupts now?
>> > > > >
>> > > >
>> > > > Because mailbox framework does not support sync rx now, I think we
>> > > > do not need to wait for that feature done first as it's
>> > > > independent and separate features of framework.
>> > >
>> > > You can wait forever for this feature, nobody will add it for you.
>> > > It's up to you to add support for that feature. Who else should add this
>> feature if not you?
>> > > And when will you add that feature if not now when you actually need it?
>> > > It is common practice that you adjust the frameworks to your needs
>> > > rather than working around them.
>> > >
>> >
>> > I'm willing to add it. Just because you said Jassi already had the
>> > idea on how to Implement it and does not add much complexity. So I just
>> want to see his patches.
>> > But if he did not work on it, I can also help on it.
>> >
>> I am not much aware of the history of this conversation... but it seems you
>> need to make use of mbox_chan_ops.peek_data().
>>
>> If not that, please let me know the requirement.
>>
>
> Thanks for the suggestion.
> It looks to me may work.
>
> From the definition, it seems it's used to pull data from remote side.
> /**
>  * mbox_client_peek_data - A way for client driver to pull data
>  *                      received from remote by the controller.
>  * @chan: Mailbox channel assigned to this client.
>  *
>  * A poke to controller driver for any received data.
>  * The data is actually passed onto client via the
>  * mbox_chan_received_data()
>  * The call can be made from atomic context, so the controller's
>  * implementation of peek_data() must not sleep.
>  *
>  * Return: True, if controller has, and is going to push after this,
>  *          some data.
>  *         False, if controller doesn't have any data to be read.
>  */
> bool mbox_client_peek_data(struct mbox_chan *chan)
> {
>         if (chan->mbox->ops->peek_data)
>                 return chan->mbox->ops->peek_data(chan);
>
>         return false;
> }
> EXPORT_SYMBOL_GPL(mbox_client_peek_data);
> But it seems most users in kernel simply implement it as a data available
> Checking rather than receiving it.
> See:
> drivers/mailbox/ti-msgmgr.c
> drivers/mailbox/mailbox-altera.c
>
> Only bcm uses it to receive data.
> drivers/mailbox/bcm-flexrm-mailbox.c
>
> For our requirement, we want to implement sync receiving protocol like:
> Sc_call_rpc()
> {
>         mbox_send_message(chan, msg)
>         If (!no_resp)
>                 // rx also stored in msg
>                 mbox_receive_msg_in_polling(chan, msg);
>         mbox_client_txdone();
> }
>
> If using peek_data, it can be:
> Sc_call_rpc()
> {
>         mbox_send_message(chan, msg)
>         If (!no_resp)
>                 // rx also stored in msg
>                 Mbox_client_peek_data(chan);
>
Yes, and you may want to loop for a certain amount of time if peek
keeps returning false.

>         mbox_client_txdone();
> }
>
> And for mu controller driver .peek_data():
> imx_mu_peek_data(chan)
> {
>         // get first word and parse data size
>         imx_mu_receive_msg(&mu->chans, 0, mu->msg);
>
>         raw_data = (u8 *)mu->msg;
>         size = raw_data[1];
>
>         // receive rest of them
>         for (i = 1; i < size; i++) {
>                 ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
>                 if (ret)
>                         return false;
>         }
>
>         mbox_chan_received_data(&mu->chans, (void *)mu->msg);
>
Not sure how your controller works. But the peek() callback only
_checks_ if there is some data available to be read. Please note that
peek() can not sleep.
So if the data fetching doesn't sleep you can do that here, otherwise
peek has to schedule the actual fetching of data from remote and
providing to the client via mbox_chan_received_data.

-jassi

[PATCH V4 3/5] mailbox: imx: add imx mu support

[Jassi Brar]

On Wed, Jul 11, 2018 at 6:28 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> Hi Jassi,
>
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar at gmail.com]
>> Sent: Wednesday, July 11, 2018 6:44 PM
>> To: A.s. Dong <aisheng.dong@nxp.com>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
>> kernel at lists.infradead.org; dongas86 at gmail.com; linux-
>> kernel at vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
>> linux-imx <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
>> <fabio.estevam@nxp.com>; shawnguo at kernel.org
>> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>>
>> On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> >
>> > > -----Original Message-----
>> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
>> > > Sent: Wednesday, July 11, 2018 3:55 PM
>> > > To: A.s. Dong <aisheng.dong@nxp.com>
>> > > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; Jassi
>> > > Brar <jassisinghbrar@gmail.com>; linux-kernel at vger.kernel.org;
>> > > Oleksij Rempel <o.rempel@pengutronix.de>; dl-linux-imx
>> > > <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
>> > > <fabio.estevam@nxp.com>; shawnguo at kernel.org
>> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> > >
>> > > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
>> > > > Hi Sascha,
>> > > >
>> > > > > -----Original Message-----
>> > > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
>> > > > > Sent: Tuesday, July 10, 2018 10:20 PM
>> > > > > To: A.s. Dong <aisheng.dong@nxp.com>
>> > > > > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com;
>> > > > > Jassi Brar <jassisinghbrar@gmail.com>;
>> > > > > linux-kernel at vger.kernel.org; Oleksij Rempel
>> > > > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
>> > > > > kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
>> > > > > shawnguo at kernel.org
>> > > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> > > > >
>> > > > > Hi,
>> > > > >
>> > > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
>> > > > > > This is used for i.MX multi core communication.
>> > > > > > e.g. A core to SCU firmware(M core) on MX8.
>> > > > > >
>> > > > > > Tx is using polling mode while Rx is interrupt driven and
>> > > > > > schedule a hrtimer to receive remain words if have more than
>> > > > > > 4 words.
>> > > > >
>> > > > > You told us that using interrupts is not possible due to
>> > > > > miserable performance, we then provided you a way with which you
>> could poll.
>> > > > > Why are you using interrupts now?
>> > > > >
>> > > >
>> > > > Because mailbox framework does not support sync rx now, I think we
>> > > > do not need to wait for that feature done first as it's
>> > > > independent and separate features of framework.
>> > >
>> > > You can wait forever for this feature, nobody will add it for you.
>> > > It's up to you to add support for that feature. Who else should add this
>> feature if not you?
>> > > And when will you add that feature if not now when you actually need it?
>> > > It is common practice that you adjust the frameworks to your needs
>> > > rather than working around them.
>> > >
>> >
>> > I'm willing to add it. Just because you said Jassi already had the
>> > idea on how to Implement it and does not add much complexity. So I just
>> want to see his patches.
>> > But if he did not work on it, I can also help on it.
>> >
>> I am not much aware of the history of this conversation... but it seems you
>> need to make use of mbox_chan_ops.peek_data().
>>
>> If not that, please let me know the requirement.
>>
>
> Thanks for the suggestion.
> It looks to me may work.
>
> From the definition, it seems it's used to pull data from remote side.
> /**
>  * mbox_client_peek_data - A way for client driver to pull data
>  *                      received from remote by the controller.
>  * @chan: Mailbox channel assigned to this client.
>  *
>  * A poke to controller driver for any received data.
>  * The data is actually passed onto client via the
>  * mbox_chan_received_data()
>  * The call can be made from atomic context, so the controller's
>  * implementation of peek_data() must not sleep.
>  *
>  * Return: True, if controller has, and is going to push after this,
>  *          some data.
>  *         False, if controller doesn't have any data to be read.
>  */
> bool mbox_client_peek_data(struct mbox_chan *chan)
> {
>         if (chan->mbox->ops->peek_data)
>                 return chan->mbox->ops->peek_data(chan);
>
>         return false;
> }
> EXPORT_SYMBOL_GPL(mbox_client_peek_data);
> But it seems most users in kernel simply implement it as a data available
> Checking rather than receiving it.
> See:
> drivers/mailbox/ti-msgmgr.c
> drivers/mailbox/mailbox-altera.c
>
> Only bcm uses it to receive data.
> drivers/mailbox/bcm-flexrm-mailbox.c
>
> For our requirement, we want to implement sync receiving protocol like:
> Sc_call_rpc()
> {
>         mbox_send_message(chan, msg)
>         If (!no_resp)
>                 // rx also stored in msg
>                 mbox_receive_msg_in_polling(chan, msg);
>         mbox_client_txdone();
> }
>
> If using peek_data, it can be:
> Sc_call_rpc()
> {
>         mbox_send_message(chan, msg)
>         If (!no_resp)
>                 // rx also stored in msg
>                 Mbox_client_peek_data(chan);
>
Yes, and you may want to loop for a certain amount of time if peek
keeps returning false.

>         mbox_client_txdone();
> }
>
> And for mu controller driver .peek_data():
> imx_mu_peek_data(chan)
> {
>         // get first word and parse data size
>         imx_mu_receive_msg(&mu->chans, 0, mu->msg);
>
>         raw_data = (u8 *)mu->msg;
>         size = raw_data[1];
>
>         // receive rest of them
>         for (i = 1; i < size; i++) {
>                 ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
>                 if (ret)
>                         return false;
>         }
>
>         mbox_chan_received_data(&mu->chans, (void *)mu->msg);
>
Not sure how your controller works. But the peek() callback only
_checks_ if there is some data available to be read. Please note that
peek() can not sleep.
So if the data fetching doesn't sleep you can do that here, otherwise
peek has to schedule the actual fetching of data from remote and
providing to the client via mbox_chan_received_data.

-jassi

RE: [PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Thursday, July 12, 2018 12:32 AM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
> kernel@lists.infradead.org; dongas86@gmail.com; linux-
> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo@kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> On Wed, Jul 11, 2018 at 6:28 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> > Hi Jassi,
> >
> >> -----Original Message-----
> >> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> >> Sent: Wednesday, July 11, 2018 6:44 PM
> >> To: A.s. Dong <aisheng.dong@nxp.com>
> >> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
> >> kernel@lists.infradead.org; dongas86@gmail.com; linux-
> >> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
> >> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> >> <fabio.estevam@nxp.com>; shawnguo@kernel.org
> >> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >>
> >> On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com>
> wrote:
> >> >
> >> > > -----Original Message-----
> >> > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> >> > > Sent: Wednesday, July 11, 2018 3:55 PM
> >> > > To: A.s. Dong <aisheng.dong@nxp.com>
> >> > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com;
> >> > > Jassi Brar <jassisinghbrar@gmail.com>;
> >> > > linux-kernel@vger.kernel.org; Oleksij Rempel
> >> > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> >> > > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> >> > > shawnguo@kernel.org
> >> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >> > >
> >> > > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> >> > > > Hi Sascha,
> >> > > >
> >> > > > > -----Original Message-----
> >> > > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> >> > > > > Sent: Tuesday, July 10, 2018 10:20 PM
> >> > > > > To: A.s. Dong <aisheng.dong@nxp.com>
> >> > > > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com;
> >> > > > > Jassi Brar <jassisinghbrar@gmail.com>;
> >> > > > > linux-kernel@vger.kernel.org; Oleksij Rempel
> >> > > > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> >> > > > > kernel@pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>;
> >> > > > > shawnguo@kernel.org
> >> > > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >> > > > >
> >> > > > > Hi,
> >> > > > >
> >> > > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> >> > > > > > This is used for i.MX multi core communication.
> >> > > > > > e.g. A core to SCU firmware(M core) on MX8.
> >> > > > > >
> >> > > > > > Tx is using polling mode while Rx is interrupt driven and
> >> > > > > > schedule a hrtimer to receive remain words if have more
> >> > > > > > than
> >> > > > > > 4 words.
> >> > > > >
> >> > > > > You told us that using interrupts is not possible due to
> >> > > > > miserable performance, we then provided you a way with which
> >> > > > > you
> >> could poll.
> >> > > > > Why are you using interrupts now?
> >> > > > >
> >> > > >
> >> > > > Because mailbox framework does not support sync rx now, I think
> >> > > > we do not need to wait for that feature done first as it's
> >> > > > independent and separate features of framework.
> >> > >
> >> > > You can wait forever for this feature, nobody will add it for you.
> >> > > It's up to you to add support for that feature. Who else should
> >> > > add this
> >> feature if not you?
> >> > > And when will you add that feature if not now when you actually need
> it?
> >> > > It is common practice that you adjust the frameworks to your
> >> > > needs rather than working around them.
> >> > >
> >> >
> >> > I'm willing to add it. Just because you said Jassi already had the
> >> > idea on how to Implement it and does not add much complexity. So I
> >> > just
> >> want to see his patches.
> >> > But if he did not work on it, I can also help on it.
> >> >
> >> I am not much aware of the history of this conversation... but it
> >> seems you need to make use of mbox_chan_ops.peek_data().
> >>
> >> If not that, please let me know the requirement.
> >>
> >
> > Thanks for the suggestion.
> > It looks to me may work.
> >
> > From the definition, it seems it's used to pull data from remote side.
> > /**
> >  * mbox_client_peek_data - A way for client driver to pull data
> >  *                      received from remote by the controller.
> >  * @chan: Mailbox channel assigned to this client.
> >  *
> >  * A poke to controller driver for any received data.
> >  * The data is actually passed onto client via the
> >  * mbox_chan_received_data()
> >  * The call can be made from atomic context, so the controller's
> >  * implementation of peek_data() must not sleep.
> >  *
> >  * Return: True, if controller has, and is going to push after this,
> >  *          some data.
> >  *         False, if controller doesn't have any data to be read.
> >  */
> > bool mbox_client_peek_data(struct mbox_chan *chan) {
> >         if (chan->mbox->ops->peek_data)
> >                 return chan->mbox->ops->peek_data(chan);
> >
> >         return false;
> > }
> > EXPORT_SYMBOL_GPL(mbox_client_peek_data);
> > But it seems most users in kernel simply implement it as a data
> > available Checking rather than receiving it.
> > See:
> > drivers/mailbox/ti-msgmgr.c
> > drivers/mailbox/mailbox-altera.c
> >
> > Only bcm uses it to receive data.
> > drivers/mailbox/bcm-flexrm-mailbox.c
> >
> > For our requirement, we want to implement sync receiving protocol like:
> > Sc_call_rpc()
> > {
> >         mbox_send_message(chan, msg)
> >         If (!no_resp)
> >                 // rx also stored in msg
> >                 mbox_receive_msg_in_polling(chan, msg);
> >         mbox_client_txdone();
> > }
> >
> > If using peek_data, it can be:
> > Sc_call_rpc()
> > {
> >         mbox_send_message(chan, msg)
> >         If (!no_resp)
> >                 // rx also stored in msg
> >                 Mbox_client_peek_data(chan);
> >
> Yes, and you may want to loop for a certain amount of time if peek keeps
> returning false.
> 
> >         mbox_client_txdone();
> > }
> >
> > And for mu controller driver .peek_data():
> > imx_mu_peek_data(chan)
> > {
> >         // get first word and parse data size
> >         imx_mu_receive_msg(&mu->chans, 0, mu->msg);
> >
> >         raw_data = (u8 *)mu->msg;
> >         size = raw_data[1];
> >
> >         // receive rest of them
> >         for (i = 1; i < size; i++) {
> >                 ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
> >                 if (ret)
> >                         return false;
> >         }
> >
> >         mbox_chan_received_data(&mu->chans, (void *)mu->msg);
> >
> Not sure how your controller works. But the peek() callback only _checks_ if
> there is some data available to be read. Please note that
> peek() can not sleep.
> So if the data fetching doesn't sleep you can do that here, otherwise peek
> has to schedule the actual fetching of data from remote and providing to the
> client via mbox_chan_received_data.
> 

bcm seems is using peek to receive data, not only checking the data availability,
right?
drivers/mailbox/bcm-flexrm-mailbox.c

So I did the similar way for i.MX. I just sent out that new patch series.
Please help review if any problem of it.
BTW i.MX peek is using busy polling, so won't sleep.

Regards
Dong Aisheng

> -jassi

[PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar at gmail.com]
> Sent: Thursday, July 12, 2018 12:32 AM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
> kernel at lists.infradead.org; dongas86 at gmail.com; linux-
> kernel at vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
> linux-imx <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo at kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> On Wed, Jul 11, 2018 at 6:28 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> > Hi Jassi,
> >
> >> -----Original Message-----
> >> From: Jassi Brar [mailto:jassisinghbrar at gmail.com]
> >> Sent: Wednesday, July 11, 2018 6:44 PM
> >> To: A.s. Dong <aisheng.dong@nxp.com>
> >> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
> >> kernel at lists.infradead.org; dongas86 at gmail.com; linux-
> >> kernel at vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
> >> linux-imx <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
> >> <fabio.estevam@nxp.com>; shawnguo at kernel.org
> >> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >>
> >> On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com>
> wrote:
> >> >
> >> > > -----Original Message-----
> >> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> >> > > Sent: Wednesday, July 11, 2018 3:55 PM
> >> > > To: A.s. Dong <aisheng.dong@nxp.com>
> >> > > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com;
> >> > > Jassi Brar <jassisinghbrar@gmail.com>;
> >> > > linux-kernel at vger.kernel.org; Oleksij Rempel
> >> > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> >> > > kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> >> > > shawnguo at kernel.org
> >> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >> > >
> >> > > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
> >> > > > Hi Sascha,
> >> > > >
> >> > > > > -----Original Message-----
> >> > > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> >> > > > > Sent: Tuesday, July 10, 2018 10:20 PM
> >> > > > > To: A.s. Dong <aisheng.dong@nxp.com>
> >> > > > > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com;
> >> > > > > Jassi Brar <jassisinghbrar@gmail.com>;
> >> > > > > linux-kernel at vger.kernel.org; Oleksij Rempel
> >> > > > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> >> > > > > kernel at pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>;
> >> > > > > shawnguo at kernel.org
> >> > > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >> > > > >
> >> > > > > Hi,
> >> > > > >
> >> > > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> >> > > > > > This is used for i.MX multi core communication.
> >> > > > > > e.g. A core to SCU firmware(M core) on MX8.
> >> > > > > >
> >> > > > > > Tx is using polling mode while Rx is interrupt driven and
> >> > > > > > schedule a hrtimer to receive remain words if have more
> >> > > > > > than
> >> > > > > > 4 words.
> >> > > > >
> >> > > > > You told us that using interrupts is not possible due to
> >> > > > > miserable performance, we then provided you a way with which
> >> > > > > you
> >> could poll.
> >> > > > > Why are you using interrupts now?
> >> > > > >
> >> > > >
> >> > > > Because mailbox framework does not support sync rx now, I think
> >> > > > we do not need to wait for that feature done first as it's
> >> > > > independent and separate features of framework.
> >> > >
> >> > > You can wait forever for this feature, nobody will add it for you.
> >> > > It's up to you to add support for that feature. Who else should
> >> > > add this
> >> feature if not you?
> >> > > And when will you add that feature if not now when you actually need
> it?
> >> > > It is common practice that you adjust the frameworks to your
> >> > > needs rather than working around them.
> >> > >
> >> >
> >> > I'm willing to add it. Just because you said Jassi already had the
> >> > idea on how to Implement it and does not add much complexity. So I
> >> > just
> >> want to see his patches.
> >> > But if he did not work on it, I can also help on it.
> >> >
> >> I am not much aware of the history of this conversation... but it
> >> seems you need to make use of mbox_chan_ops.peek_data().
> >>
> >> If not that, please let me know the requirement.
> >>
> >
> > Thanks for the suggestion.
> > It looks to me may work.
> >
> > From the definition, it seems it's used to pull data from remote side.
> > /**
> >  * mbox_client_peek_data - A way for client driver to pull data
> >  *                      received from remote by the controller.
> >  * @chan: Mailbox channel assigned to this client.
> >  *
> >  * A poke to controller driver for any received data.
> >  * The data is actually passed onto client via the
> >  * mbox_chan_received_data()
> >  * The call can be made from atomic context, so the controller's
> >  * implementation of peek_data() must not sleep.
> >  *
> >  * Return: True, if controller has, and is going to push after this,
> >  *          some data.
> >  *         False, if controller doesn't have any data to be read.
> >  */
> > bool mbox_client_peek_data(struct mbox_chan *chan) {
> >         if (chan->mbox->ops->peek_data)
> >                 return chan->mbox->ops->peek_data(chan);
> >
> >         return false;
> > }
> > EXPORT_SYMBOL_GPL(mbox_client_peek_data);
> > But it seems most users in kernel simply implement it as a data
> > available Checking rather than receiving it.
> > See:
> > drivers/mailbox/ti-msgmgr.c
> > drivers/mailbox/mailbox-altera.c
> >
> > Only bcm uses it to receive data.
> > drivers/mailbox/bcm-flexrm-mailbox.c
> >
> > For our requirement, we want to implement sync receiving protocol like:
> > Sc_call_rpc()
> > {
> >         mbox_send_message(chan, msg)
> >         If (!no_resp)
> >                 // rx also stored in msg
> >                 mbox_receive_msg_in_polling(chan, msg);
> >         mbox_client_txdone();
> > }
> >
> > If using peek_data, it can be:
> > Sc_call_rpc()
> > {
> >         mbox_send_message(chan, msg)
> >         If (!no_resp)
> >                 // rx also stored in msg
> >                 Mbox_client_peek_data(chan);
> >
> Yes, and you may want to loop for a certain amount of time if peek keeps
> returning false.
> 
> >         mbox_client_txdone();
> > }
> >
> > And for mu controller driver .peek_data():
> > imx_mu_peek_data(chan)
> > {
> >         // get first word and parse data size
> >         imx_mu_receive_msg(&mu->chans, 0, mu->msg);
> >
> >         raw_data = (u8 *)mu->msg;
> >         size = raw_data[1];
> >
> >         // receive rest of them
> >         for (i = 1; i < size; i++) {
> >                 ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
> >                 if (ret)
> >                         return false;
> >         }
> >
> >         mbox_chan_received_data(&mu->chans, (void *)mu->msg);
> >
> Not sure how your controller works. But the peek() callback only _checks_ if
> there is some data available to be read. Please note that
> peek() can not sleep.
> So if the data fetching doesn't sleep you can do that here, otherwise peek
> has to schedule the actual fetching of data from remote and providing to the
> client via mbox_chan_received_data.
> 

bcm seems is using peek to receive data, not only checking the data availability,
right?
drivers/mailbox/bcm-flexrm-mailbox.c

So I did the similar way for i.MX. I just sent out that new patch series.
Please help review if any problem of it.
BTW i.MX peek is using busy polling, so won't sleep.

Regards
Dong Aisheng

> -jassi

Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

[Jassi Brar]

On Wed, Jul 11, 2018 at 10:11 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> Hi Jassi,
>
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
>> Sent: Thursday, July 12, 2018 12:32 AM
>> To: A.s. Dong <aisheng.dong@nxp.com>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
>> kernel@lists.infradead.org; dongas86@gmail.com; linux-
>> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
>> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
>> <fabio.estevam@nxp.com>; shawnguo@kernel.org
>> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>>
>> On Wed, Jul 11, 2018 at 6:28 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> > Hi Jassi,
>> >
>> >> -----Original Message-----
>> >> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
>> >> Sent: Wednesday, July 11, 2018 6:44 PM
>> >> To: A.s. Dong <aisheng.dong@nxp.com>
>> >> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
>> >> kernel@lists.infradead.org; dongas86@gmail.com; linux-
>> >> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
>> >> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
>> >> <fabio.estevam@nxp.com>; shawnguo@kernel.org
>> >> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> >>
>> >> On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com>
>> wrote:
>> >> >
>> >> > > -----Original Message-----
>> >> > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
>> >> > > Sent: Wednesday, July 11, 2018 3:55 PM
>> >> > > To: A.s. Dong <aisheng.dong@nxp.com>
>> >> > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com;
>> >> > > Jassi Brar <jassisinghbrar@gmail.com>;
>> >> > > linux-kernel@vger.kernel.org; Oleksij Rempel
>> >> > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
>> >> > > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
>> >> > > shawnguo@kernel.org
>> >> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> >> > >
>> >> > > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
>> >> > > > Hi Sascha,
>> >> > > >
>> >> > > > > -----Original Message-----
>> >> > > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
>> >> > > > > Sent: Tuesday, July 10, 2018 10:20 PM
>> >> > > > > To: A.s. Dong <aisheng.dong@nxp.com>
>> >> > > > > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com;
>> >> > > > > Jassi Brar <jassisinghbrar@gmail.com>;
>> >> > > > > linux-kernel@vger.kernel.org; Oleksij Rempel
>> >> > > > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
>> >> > > > > kernel@pengutronix.de; Fabio Estevam
>> <fabio.estevam@nxp.com>;
>> >> > > > > shawnguo@kernel.org
>> >> > > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> >> > > > >
>> >> > > > > Hi,
>> >> > > > >
>> >> > > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
>> >> > > > > > This is used for i.MX multi core communication.
>> >> > > > > > e.g. A core to SCU firmware(M core) on MX8.
>> >> > > > > >
>> >> > > > > > Tx is using polling mode while Rx is interrupt driven and
>> >> > > > > > schedule a hrtimer to receive remain words if have more
>> >> > > > > > than
>> >> > > > > > 4 words.
>> >> > > > >
>> >> > > > > You told us that using interrupts is not possible due to
>> >> > > > > miserable performance, we then provided you a way with which
>> >> > > > > you
>> >> could poll.
>> >> > > > > Why are you using interrupts now?
>> >> > > > >
>> >> > > >
>> >> > > > Because mailbox framework does not support sync rx now, I think
>> >> > > > we do not need to wait for that feature done first as it's
>> >> > > > independent and separate features of framework.
>> >> > >
>> >> > > You can wait forever for this feature, nobody will add it for you.
>> >> > > It's up to you to add support for that feature. Who else should
>> >> > > add this
>> >> feature if not you?
>> >> > > And when will you add that feature if not now when you actually need
>> it?
>> >> > > It is common practice that you adjust the frameworks to your
>> >> > > needs rather than working around them.
>> >> > >
>> >> >
>> >> > I'm willing to add it. Just because you said Jassi already had the
>> >> > idea on how to Implement it and does not add much complexity. So I
>> >> > just
>> >> want to see his patches.
>> >> > But if he did not work on it, I can also help on it.
>> >> >
>> >> I am not much aware of the history of this conversation... but it
>> >> seems you need to make use of mbox_chan_ops.peek_data().
>> >>
>> >> If not that, please let me know the requirement.
>> >>
>> >
>> > Thanks for the suggestion.
>> > It looks to me may work.
>> >
>> > From the definition, it seems it's used to pull data from remote side.
>> > /**
>> >  * mbox_client_peek_data - A way for client driver to pull data
>> >  *                      received from remote by the controller.
>> >  * @chan: Mailbox channel assigned to this client.
>> >  *
>> >  * A poke to controller driver for any received data.
>> >  * The data is actually passed onto client via the
>> >  * mbox_chan_received_data()
>> >  * The call can be made from atomic context, so the controller's
>> >  * implementation of peek_data() must not sleep.
>> >  *
>> >  * Return: True, if controller has, and is going to push after this,
>> >  *          some data.
>> >  *         False, if controller doesn't have any data to be read.
>> >  */
>> > bool mbox_client_peek_data(struct mbox_chan *chan) {
>> >         if (chan->mbox->ops->peek_data)
>> >                 return chan->mbox->ops->peek_data(chan);
>> >
>> >         return false;
>> > }
>> > EXPORT_SYMBOL_GPL(mbox_client_peek_data);
>> > But it seems most users in kernel simply implement it as a data
>> > available Checking rather than receiving it.
>> > See:
>> > drivers/mailbox/ti-msgmgr.c
>> > drivers/mailbox/mailbox-altera.c
>> >
>> > Only bcm uses it to receive data.
>> > drivers/mailbox/bcm-flexrm-mailbox.c
>> >
>> > For our requirement, we want to implement sync receiving protocol like:
>> > Sc_call_rpc()
>> > {
>> >         mbox_send_message(chan, msg)
>> >         If (!no_resp)
>> >                 // rx also stored in msg
>> >                 mbox_receive_msg_in_polling(chan, msg);
>> >         mbox_client_txdone();
>> > }
>> >
>> > If using peek_data, it can be:
>> > Sc_call_rpc()
>> > {
>> >         mbox_send_message(chan, msg)
>> >         If (!no_resp)
>> >                 // rx also stored in msg
>> >                 Mbox_client_peek_data(chan);
>> >
>> Yes, and you may want to loop for a certain amount of time if peek keeps
>> returning false.
>>
>> >         mbox_client_txdone();
>> > }
>> >
>> > And for mu controller driver .peek_data():
>> > imx_mu_peek_data(chan)
>> > {
>> >         // get first word and parse data size
>> >         imx_mu_receive_msg(&mu->chans, 0, mu->msg);
>> >
>> >         raw_data = (u8 *)mu->msg;
>> >         size = raw_data[1];
>> >
>> >         // receive rest of them
>> >         for (i = 1; i < size; i++) {
>> >                 ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
>> >                 if (ret)
>> >                         return false;
>> >         }
>> >
>> >         mbox_chan_received_data(&mu->chans, (void *)mu->msg);
>> >
>> Not sure how your controller works. But the peek() callback only _checks_ if
>> there is some data available to be read. Please note that
>> peek() can not sleep.
>> So if the data fetching doesn't sleep you can do that here, otherwise peek
>> has to schedule the actual fetching of data from remote and providing to the
>> client via mbox_chan_received_data.
>>
>
> bcm seems is using peek to receive data, not only checking the data availability,
> right?
> drivers/mailbox/bcm-flexrm-mailbox.c
>
As I said, if fetching data from remote don't need to sleep, you can
call mbox_chan_received_data() from peek(). Otherwise not.

[PATCH V4 3/5] mailbox: imx: add imx mu support

[Jassi Brar]

On Wed, Jul 11, 2018 at 10:11 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> Hi Jassi,
>
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar at gmail.com]
>> Sent: Thursday, July 12, 2018 12:32 AM
>> To: A.s. Dong <aisheng.dong@nxp.com>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
>> kernel at lists.infradead.org; dongas86 at gmail.com; linux-
>> kernel at vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
>> linux-imx <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
>> <fabio.estevam@nxp.com>; shawnguo at kernel.org
>> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>>
>> On Wed, Jul 11, 2018 at 6:28 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> > Hi Jassi,
>> >
>> >> -----Original Message-----
>> >> From: Jassi Brar [mailto:jassisinghbrar at gmail.com]
>> >> Sent: Wednesday, July 11, 2018 6:44 PM
>> >> To: A.s. Dong <aisheng.dong@nxp.com>
>> >> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
>> >> kernel at lists.infradead.org; dongas86 at gmail.com; linux-
>> >> kernel at vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
>> >> linux-imx <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
>> >> <fabio.estevam@nxp.com>; shawnguo at kernel.org
>> >> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> >>
>> >> On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@nxp.com>
>> wrote:
>> >> >
>> >> > > -----Original Message-----
>> >> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
>> >> > > Sent: Wednesday, July 11, 2018 3:55 PM
>> >> > > To: A.s. Dong <aisheng.dong@nxp.com>
>> >> > > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com;
>> >> > > Jassi Brar <jassisinghbrar@gmail.com>;
>> >> > > linux-kernel at vger.kernel.org; Oleksij Rempel
>> >> > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
>> >> > > kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
>> >> > > shawnguo at kernel.org
>> >> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> >> > >
>> >> > > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
>> >> > > > Hi Sascha,
>> >> > > >
>> >> > > > > -----Original Message-----
>> >> > > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
>> >> > > > > Sent: Tuesday, July 10, 2018 10:20 PM
>> >> > > > > To: A.s. Dong <aisheng.dong@nxp.com>
>> >> > > > > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com;
>> >> > > > > Jassi Brar <jassisinghbrar@gmail.com>;
>> >> > > > > linux-kernel at vger.kernel.org; Oleksij Rempel
>> >> > > > > <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
>> >> > > > > kernel at pengutronix.de; Fabio Estevam
>> <fabio.estevam@nxp.com>;
>> >> > > > > shawnguo at kernel.org
>> >> > > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> >> > > > >
>> >> > > > > Hi,
>> >> > > > >
>> >> > > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
>> >> > > > > > This is used for i.MX multi core communication.
>> >> > > > > > e.g. A core to SCU firmware(M core) on MX8.
>> >> > > > > >
>> >> > > > > > Tx is using polling mode while Rx is interrupt driven and
>> >> > > > > > schedule a hrtimer to receive remain words if have more
>> >> > > > > > than
>> >> > > > > > 4 words.
>> >> > > > >
>> >> > > > > You told us that using interrupts is not possible due to
>> >> > > > > miserable performance, we then provided you a way with which
>> >> > > > > you
>> >> could poll.
>> >> > > > > Why are you using interrupts now?
>> >> > > > >
>> >> > > >
>> >> > > > Because mailbox framework does not support sync rx now, I think
>> >> > > > we do not need to wait for that feature done first as it's
>> >> > > > independent and separate features of framework.
>> >> > >
>> >> > > You can wait forever for this feature, nobody will add it for you.
>> >> > > It's up to you to add support for that feature. Who else should
>> >> > > add this
>> >> feature if not you?
>> >> > > And when will you add that feature if not now when you actually need
>> it?
>> >> > > It is common practice that you adjust the frameworks to your
>> >> > > needs rather than working around them.
>> >> > >
>> >> >
>> >> > I'm willing to add it. Just because you said Jassi already had the
>> >> > idea on how to Implement it and does not add much complexity. So I
>> >> > just
>> >> want to see his patches.
>> >> > But if he did not work on it, I can also help on it.
>> >> >
>> >> I am not much aware of the history of this conversation... but it
>> >> seems you need to make use of mbox_chan_ops.peek_data().
>> >>
>> >> If not that, please let me know the requirement.
>> >>
>> >
>> > Thanks for the suggestion.
>> > It looks to me may work.
>> >
>> > From the definition, it seems it's used to pull data from remote side.
>> > /**
>> >  * mbox_client_peek_data - A way for client driver to pull data
>> >  *                      received from remote by the controller.
>> >  * @chan: Mailbox channel assigned to this client.
>> >  *
>> >  * A poke to controller driver for any received data.
>> >  * The data is actually passed onto client via the
>> >  * mbox_chan_received_data()
>> >  * The call can be made from atomic context, so the controller's
>> >  * implementation of peek_data() must not sleep.
>> >  *
>> >  * Return: True, if controller has, and is going to push after this,
>> >  *          some data.
>> >  *         False, if controller doesn't have any data to be read.
>> >  */
>> > bool mbox_client_peek_data(struct mbox_chan *chan) {
>> >         if (chan->mbox->ops->peek_data)
>> >                 return chan->mbox->ops->peek_data(chan);
>> >
>> >         return false;
>> > }
>> > EXPORT_SYMBOL_GPL(mbox_client_peek_data);
>> > But it seems most users in kernel simply implement it as a data
>> > available Checking rather than receiving it.
>> > See:
>> > drivers/mailbox/ti-msgmgr.c
>> > drivers/mailbox/mailbox-altera.c
>> >
>> > Only bcm uses it to receive data.
>> > drivers/mailbox/bcm-flexrm-mailbox.c
>> >
>> > For our requirement, we want to implement sync receiving protocol like:
>> > Sc_call_rpc()
>> > {
>> >         mbox_send_message(chan, msg)
>> >         If (!no_resp)
>> >                 // rx also stored in msg
>> >                 mbox_receive_msg_in_polling(chan, msg);
>> >         mbox_client_txdone();
>> > }
>> >
>> > If using peek_data, it can be:
>> > Sc_call_rpc()
>> > {
>> >         mbox_send_message(chan, msg)
>> >         If (!no_resp)
>> >                 // rx also stored in msg
>> >                 Mbox_client_peek_data(chan);
>> >
>> Yes, and you may want to loop for a certain amount of time if peek keeps
>> returning false.
>>
>> >         mbox_client_txdone();
>> > }
>> >
>> > And for mu controller driver .peek_data():
>> > imx_mu_peek_data(chan)
>> > {
>> >         // get first word and parse data size
>> >         imx_mu_receive_msg(&mu->chans, 0, mu->msg);
>> >
>> >         raw_data = (u8 *)mu->msg;
>> >         size = raw_data[1];
>> >
>> >         // receive rest of them
>> >         for (i = 1; i < size; i++) {
>> >                 ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
>> >                 if (ret)
>> >                         return false;
>> >         }
>> >
>> >         mbox_chan_received_data(&mu->chans, (void *)mu->msg);
>> >
>> Not sure how your controller works. But the peek() callback only _checks_ if
>> there is some data available to be read. Please note that
>> peek() can not sleep.
>> So if the data fetching doesn't sleep you can do that here, otherwise peek
>> has to schedule the actual fetching of data from remote and providing to the
>> client via mbox_chan_received_data.
>>
>
> bcm seems is using peek to receive data, not only checking the data availability,
> right?
> drivers/mailbox/bcm-flexrm-mailbox.c
>
As I said, if fetching data from remote don't need to sleep, you can
call mbox_chan_received_data() from peek(). Otherwise not.

RE: [PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Thursday, July 12, 2018 1:01 AM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
> kernel@lists.infradead.org; dongas86@gmail.com; linux-
> kernel@vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
> linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo@kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

....

> >
> > bcm seems is using peek to receive data, not only checking the data
> > availability, right?
> > drivers/mailbox/bcm-flexrm-mailbox.c
> >
> As I said, if fetching data from remote don't need to sleep, you can call
> mbox_chan_received_data() from peek(). Otherwise not.

Got it. So it seems work for i.MX.
Thanks.

Regards
Dong Aisheng

[PATCH V4 3/5] mailbox: imx: add imx mu support

[A.s. Dong]

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar at gmail.com]
> Sent: Thursday, July 12, 2018 1:01 AM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>; linux-arm-
> kernel at lists.infradead.org; dongas86 at gmail.com; linux-
> kernel at vger.kernel.org; Oleksij Rempel <o.rempel@pengutronix.de>; dl-
> linux-imx <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo at kernel.org
> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

....

> >
> > bcm seems is using peek to receive data, not only checking the data
> > availability, right?
> > drivers/mailbox/bcm-flexrm-mailbox.c
> >
> As I said, if fetching data from remote don't need to sleep, you can call
> mbox_chan_received_data() from peek(). Otherwise not.

Got it. So it seems work for i.MX.
Thanks.

Regards
Dong Aisheng

RE: [PATCH V4 3/5] mailbox: imx: add imx mu support

[Peng Fan]

Hi A.S

> -----Original Message-----
> From: A.s. Dong
> Sent: 2018年7月11日 15:30
> To: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi Brar
> <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org; Oleksij Rempel
> <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> shawnguo@kernel.org
> Subject: RE: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> Hi Sascha,
> 
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > Sent: Tuesday, July 10, 2018 10:20 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com; Jassi
> > Brar <jassisinghbrar@gmail.com>; linux-kernel@vger.kernel.org; Oleksij
> > Rempel <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > shawnguo@kernel.org
> > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >
> > Hi,
> >
> > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > This is used for i.MX multi core communication.
> > > e.g. A core to SCU firmware(M core) on MX8.
> > >
> > > Tx is using polling mode while Rx is interrupt driven and schedule a
> > > hrtimer to receive remain words if have more than
> > > 4 words.
> >
> > You told us that using interrupts is not possible due to miserable
> > performance, we then provided you a way with which you could poll.
> > Why are you using interrupts now?
> >
> 
> Because mailbox framework does not support sync rx now, I think we do not
> need to wait for that feature done first as it's independent and  separate
> features of framework.
> 
> So for now, we're just using the common way in kernel as arm scpi and ti sci.
> When framework supports it, we can easily switch to it.
> 
> I optimized the performance a bit by removing the unnecessary memcopy
> between tx/tx messages. The test result of booting time shows there's no
> obvious regressions. I'm not sure whether it's due to we're booting a minimum
> system or the extra cost is very minor to be noticed due to not too much cmds
> sent during booting.
> (Copy Peng to comments more as he tried and reported that performance drop
> with vendor tree)

The txpoll_period is set 1, the minimum is 1ms. So it introduces latency in the initial mailbox
for SCU communication.

Regards,
Peng

> 
> From the time measurement of sc_call_rpc, we can see that most rpc command
> In polling mode can finish within 10us and very rare ones over 20us.
> If switched to irq mode, those 10us cmds will change to about 20us.
> 
> But the overall booting time did not increase much. Maybe the irq mode also
> saves some CPU MIPS to execute other works in parallel?
> 
> > We also suggested a way how the SCU mode could be integrated into the
> > generic MU support driver Oleksij posted and now you send a driver
> > which uses the same name as Oleksijs driver, but it only and
> > exclusively works in SCU mode. This doesn't bring us forward.
> >
> 
> Can Oleksij's patch be implemented against this one?
> As I remember you said we've still not determined whether Oleksij's approach is
> the most suitable way and it's still under discussion.
> (Actually TI's approach looks better which is more simiar as SCU way?)
> 
> Furthermore, from this patch, you will notice that Oleksij's patch almost did not
> work for SCU at all. I have to totally rewrite one for SCU.
> So I did not write against his patch as it does not help.
> And Oleksij's patch is quite simple while the SCU one is much complicated than
> his one. So we probably better get SCU done first.
> 
> > We suggested a binding that allows coexisting of the SCU mode and the
> > generic mode of the MU by putting the mode information into the second
> > mbox-cell. Why don't you use this?
> >
> 
> You mean this?
> +#define IMX_MU_CHANNEL0		0
> +#define IMX_MU_CHANNEL1		1
> +#define IMX_MU_CHANNEL2		2
> +#define IMX_MU_CHANNEL3		3
> +#define IMX_MU_CHANNEL_IMX8_SCU 4
> 
> It's hard for me to believe it's correct and it's over abstract to HW.
> So I thought using mbox-cells to distinguish seems to be better.
> 
> > I don't think it's necessary to rewrite Oleksijs driver, instead it
> > should rather be extended with the code I already provided as an
> > example. With that we could make both of us happy since we can both
> > have a suitable driver and even share most of the MU code.
> 
> As I said above, I even can't reuse 90%+ code of Oleksijs driver. So I can't see the
> meaning to demo the code on top of this driver. We can review the SCU
> implementation directly with this driver which is more easy.
> Then we can decide how to merge them together.
> 
> Regards
> Dong Aisheng
> 
> >
> > Regards,
> > Sascha
> >
> > --
> > Pengutronix e.K.                           |
> |
> > Industrial Linux Solutions                 |
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> > w.pengutronix.de%2F&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7
> >
> Cb359a3eddee54bf1b40a08d5e6702f22%7C686ea1d3bc2b4c6fa92cd99c5c301
> >
> 635%7C0%7C0%7C636668291863846639&amp;sdata=hSucaLRfCB1j1McwlfO%
> > 2FL0921QXiHg68sl%2B23CvEp4Q%3D&amp;reserved=0  |
> > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

[PATCH V4 3/5] mailbox: imx: add imx mu support

[Peng Fan]

Hi A.S

> -----Original Message-----
> From: A.s. Dong
> Sent: 2018?7?11? 15:30
> To: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; Jassi Brar
> <jassisinghbrar@gmail.com>; linux-kernel at vger.kernel.org; Oleksij Rempel
> <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> shawnguo at kernel.org
> Subject: RE: [PATCH V4 3/5] mailbox: imx: add imx mu support
> 
> Hi Sascha,
> 
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Tuesday, July 10, 2018 10:20 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; Jassi
> > Brar <jassisinghbrar@gmail.com>; linux-kernel at vger.kernel.org; Oleksij
> > Rempel <o.rempel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > shawnguo at kernel.org
> > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
> >
> > Hi,
> >
> > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
> > > This is used for i.MX multi core communication.
> > > e.g. A core to SCU firmware(M core) on MX8.
> > >
> > > Tx is using polling mode while Rx is interrupt driven and schedule a
> > > hrtimer to receive remain words if have more than
> > > 4 words.
> >
> > You told us that using interrupts is not possible due to miserable
> > performance, we then provided you a way with which you could poll.
> > Why are you using interrupts now?
> >
> 
> Because mailbox framework does not support sync rx now, I think we do not
> need to wait for that feature done first as it's independent and  separate
> features of framework.
> 
> So for now, we're just using the common way in kernel as arm scpi and ti sci.
> When framework supports it, we can easily switch to it.
> 
> I optimized the performance a bit by removing the unnecessary memcopy
> between tx/tx messages. The test result of booting time shows there's no
> obvious regressions. I'm not sure whether it's due to we're booting a minimum
> system or the extra cost is very minor to be noticed due to not too much cmds
> sent during booting.
> (Copy Peng to comments more as he tried and reported that performance drop
> with vendor tree)

The txpoll_period is set 1, the minimum is 1ms. So it introduces latency in the initial mailbox
for SCU communication.

Regards,
Peng

> 
> From the time measurement of sc_call_rpc, we can see that most rpc command
> In polling mode can finish within 10us and very rare ones over 20us.
> If switched to irq mode, those 10us cmds will change to about 20us.
> 
> But the overall booting time did not increase much. Maybe the irq mode also
> saves some CPU MIPS to execute other works in parallel?
> 
> > We also suggested a way how the SCU mode could be integrated into the
> > generic MU support driver Oleksij posted and now you send a driver
> > which uses the same name as Oleksijs driver, but it only and
> > exclusively works in SCU mode. This doesn't bring us forward.
> >
> 
> Can Oleksij's patch be implemented against this one?
> As I remember you said we've still not determined whether Oleksij's approach is
> the most suitable way and it's still under discussion.
> (Actually TI's approach looks better which is more simiar as SCU way?)
> 
> Furthermore, from this patch, you will notice that Oleksij's patch almost did not
> work for SCU at all. I have to totally rewrite one for SCU.
> So I did not write against his patch as it does not help.
> And Oleksij's patch is quite simple while the SCU one is much complicated than
> his one. So we probably better get SCU done first.
> 
> > We suggested a binding that allows coexisting of the SCU mode and the
> > generic mode of the MU by putting the mode information into the second
> > mbox-cell. Why don't you use this?
> >
> 
> You mean this?
> +#define IMX_MU_CHANNEL0		0
> +#define IMX_MU_CHANNEL1		1
> +#define IMX_MU_CHANNEL2		2
> +#define IMX_MU_CHANNEL3		3
> +#define IMX_MU_CHANNEL_IMX8_SCU 4
> 
> It's hard for me to believe it's correct and it's over abstract to HW.
> So I thought using mbox-cells to distinguish seems to be better.
> 
> > I don't think it's necessary to rewrite Oleksijs driver, instead it
> > should rather be extended with the code I already provided as an
> > example. With that we could make both of us happy since we can both
> > have a suitable driver and even share most of the MU code.
> 
> As I said above, I even can't reuse 90%+ code of Oleksijs driver. So I can't see the
> meaning to demo the code on top of this driver. We can review the SCU
> implementation directly with this driver which is more easy.
> Then we can decide how to merge them together.
> 
> Regards
> Dong Aisheng
> 
> >
> > Regards,
> > Sascha
> >
> > --
> > Pengutronix e.K.                           |
> |
> > Industrial Linux Solutions                 |
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> > w.pengutronix.de%2F&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7
> >
> Cb359a3eddee54bf1b40a08d5e6702f22%7C686ea1d3bc2b4c6fa92cd99c5c301
> >
> 635%7C0%7C0%7C636668291863846639&amp;sdata=hSucaLRfCB1j1McwlfO%
> > 2FL0921QXiHg68sl%2B23CvEp4Q%3D&amp;reserved=0  |
> > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

[PATCH V4 4/5] dt-bindings: arm: fsl: add scu binding doc

[Dong Aisheng]

The System Controller Firmware (SCFW) is a low-level system function
which runs on a dedicated Cortex-M core to provide power, clock, and
resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
(QM, QP), and i.MX8QX (QXP, DX).

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v3->v4:
 * fully change to mailbox binding
 * add child node description
v2->v3:
 * update a bit to mailbox binding
v1->v2:
 * remove status
 * changed to mu1
---
 .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 65 ++++++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
new file mode 100644
index 0000000..11e732a
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -0,0 +1,65 @@
+NXP i.MX System Controller Firmware (SCFW)
+--------------------------------------------------------------------
+
+The System Controller Firmware (SCFW) is a low-level system function
+which runs on a dedicated Cortex-M core to provide power, clock, and
+resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
+(QM, QP), and i.MX8QX (QXP, DX).
+
+The AP communicates with the SC using a multi-ported MU module found
+in the LSIO subsystem. The current definition of this MU module provides
+5 remote AP connections to the SC to support up to 5 execution environments
+(TZ, HV, standard Linux, etc.). The SC side of this MU module interfaces
+with the LSIO DSC IP bus. The SC firmware will communicate with this MU
+using the MSI bus.
+
+System Controller Device Node:
+=============================
+
+Required properties:
+-------------------
+- compatible: should be "fsl,imx-scu"
+- mboxes: List of phandle of MU mailbox. Should be one of LSIO
+	  MU0~M4 for imx8qxp and imx8qm. Users need to make
+	  sure not use the one which is conflict with other
+	  execution environments. e.g. ATF.
+
+Examples:
+--------
+lsio_mu1: mailbox@5d1c0000 {
+	compatible = "fsl,imx8qxp-mu";
+	reg = <0x0 0x5d1c0000 0x0 0x10000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	#mbox-cells = <0>;
+};
+
+scu {
+	compatible = "fsl,imx-scu";
+	mboxes = <&lsio_mu1>;
+};
+
+
+i.MX SCU Client Device Node:
+=========================
+
+Client nodes are maintained as children of the relevant IMX-SCU device node.
+
+Example (imx8qxp):
+-------------
+scu {
+	compatible = "fsl,imx-scu";
+	...
+
+	clk: clk {
+		...
+	};
+
+	iomuxc: iomuxc {
+		...
+	};
+
+	imx8qx-pm {
+		...
+	}
+	...
+};
-- 
2.7.4

[PATCH V4 4/5] dt-bindings: arm: fsl: add scu binding doc

[Dong Aisheng]

The System Controller Firmware (SCFW) is a low-level system function
which runs on a dedicated Cortex-M core to provide power, clock, and
resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
(QM, QP), and i.MX8QX (QXP, DX).

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree at vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v3->v4:
 * fully change to mailbox binding
 * add child node description
v2->v3:
 * update a bit to mailbox binding
v1->v2:
 * remove status
 * changed to mu1
---
 .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 65 ++++++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
new file mode 100644
index 0000000..11e732a
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -0,0 +1,65 @@
+NXP i.MX System Controller Firmware (SCFW)
+--------------------------------------------------------------------
+
+The System Controller Firmware (SCFW) is a low-level system function
+which runs on a dedicated Cortex-M core to provide power, clock, and
+resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
+(QM, QP), and i.MX8QX (QXP, DX).
+
+The AP communicates with the SC using a multi-ported MU module found
+in the LSIO subsystem. The current definition of this MU module provides
+5 remote AP connections to the SC to support up to 5 execution environments
+(TZ, HV, standard Linux, etc.). The SC side of this MU module interfaces
+with the LSIO DSC IP bus. The SC firmware will communicate with this MU
+using the MSI bus.
+
+System Controller Device Node:
+=============================
+
+Required properties:
+-------------------
+- compatible: should be "fsl,imx-scu"
+- mboxes: List of phandle of MU mailbox. Should be one of LSIO
+	  MU0~M4 for imx8qxp and imx8qm. Users need to make
+	  sure not use the one which is conflict with other
+	  execution environments. e.g. ATF.
+
+Examples:
+--------
+lsio_mu1: mailbox at 5d1c0000 {
+	compatible = "fsl,imx8qxp-mu";
+	reg = <0x0 0x5d1c0000 0x0 0x10000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	#mbox-cells = <0>;
+};
+
+scu {
+	compatible = "fsl,imx-scu";
+	mboxes = <&lsio_mu1>;
+};
+
+
+i.MX SCU Client Device Node:
+=========================
+
+Client nodes are maintained as children of the relevant IMX-SCU device node.
+
+Example (imx8qxp):
+-------------
+scu {
+	compatible = "fsl,imx-scu";
+	...
+
+	clk: clk {
+		...
+	};
+
+	iomuxc: iomuxc {
+		...
+	};
+
+	imx8qx-pm {
+		...
+	}
+	...
+};
-- 
2.7.4

Re: [PATCH V4 4/5] dt-bindings: arm: fsl: add scu binding doc

[Rob Herring]

On Sun, Jul 08, 2018 at 10:56:56PM +0800, Dong Aisheng wrote:
> The System Controller Firmware (SCFW) is a low-level system function
> which runs on a dedicated Cortex-M core to provide power, clock, and
> resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
> (QM, QP), and i.MX8QX (QXP, DX).
> 
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> v3->v4:
>  * fully change to mailbox binding
>  * add child node description
> v2->v3:
>  * update a bit to mailbox binding
> v1->v2:
>  * remove status
>  * changed to mu1
> ---
>  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 65 ++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> new file mode 100644
> index 0000000..11e732a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> @@ -0,0 +1,65 @@
> +NXP i.MX System Controller Firmware (SCFW)
> +--------------------------------------------------------------------
> +
> +The System Controller Firmware (SCFW) is a low-level system function
> +which runs on a dedicated Cortex-M core to provide power, clock, and
> +resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
> +(QM, QP), and i.MX8QX (QXP, DX).
> +
> +The AP communicates with the SC using a multi-ported MU module found
> +in the LSIO subsystem. The current definition of this MU module provides
> +5 remote AP connections to the SC to support up to 5 execution environments
> +(TZ, HV, standard Linux, etc.). The SC side of this MU module interfaces
> +with the LSIO DSC IP bus. The SC firmware will communicate with this MU
> +using the MSI bus.
> +
> +System Controller Device Node:
> +=============================

Need to specify this is a child of /firmware node.

> +
> +Required properties:
> +-------------------
> +- compatible: should be "fsl,imx-scu"
> +- mboxes: List of phandle of MU mailbox. Should be one of LSIO
> +	  MU0~M4 for imx8qxp and imx8qm. Users need to make
> +	  sure not use the one which is conflict with other
> +	  execution environments. e.g. ATF.
> +
> +Examples:
> +--------
> +lsio_mu1: mailbox@5d1c0000 {
> +	compatible = "fsl,imx8qxp-mu";
> +	reg = <0x0 0x5d1c0000 0x0 0x10000>;
> +	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> +	#mbox-cells = <0>;
> +};
> +
> +scu {
> +	compatible = "fsl,imx-scu";
> +	mboxes = <&lsio_mu1>;
> +};
> +
> +
> +i.MX SCU Client Device Node:
> +=========================
> +
> +Client nodes are maintained as children of the relevant IMX-SCU device node.
> +
> +Example (imx8qxp):
> +-------------
> +scu {
> +	compatible = "fsl,imx-scu";
> +	...
> +
> +	clk: clk {
> +		...
> +	};
> +
> +	iomuxc: iomuxc {
> +		...
> +	};
> +
> +	imx8qx-pm {
> +		...
> +	}
> +	...

If you plan to have child nodes, you need to document them now. However, 
other than pinctrl, you probably don't need child nodes. The parent node 
can be a clock and power-domain provider.

Rob

[PATCH V4 4/5] dt-bindings: arm: fsl: add scu binding doc

[Rob Herring]

On Sun, Jul 08, 2018 at 10:56:56PM +0800, Dong Aisheng wrote:
> The System Controller Firmware (SCFW) is a low-level system function
> which runs on a dedicated Cortex-M core to provide power, clock, and
> resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
> (QM, QP), and i.MX8QX (QXP, DX).
> 
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree at vger.kernel.org
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> v3->v4:
>  * fully change to mailbox binding
>  * add child node description
> v2->v3:
>  * update a bit to mailbox binding
> v1->v2:
>  * remove status
>  * changed to mu1
> ---
>  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 65 ++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> new file mode 100644
> index 0000000..11e732a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> @@ -0,0 +1,65 @@
> +NXP i.MX System Controller Firmware (SCFW)
> +--------------------------------------------------------------------
> +
> +The System Controller Firmware (SCFW) is a low-level system function
> +which runs on a dedicated Cortex-M core to provide power, clock, and
> +resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
> +(QM, QP), and i.MX8QX (QXP, DX).
> +
> +The AP communicates with the SC using a multi-ported MU module found
> +in the LSIO subsystem. The current definition of this MU module provides
> +5 remote AP connections to the SC to support up to 5 execution environments
> +(TZ, HV, standard Linux, etc.). The SC side of this MU module interfaces
> +with the LSIO DSC IP bus. The SC firmware will communicate with this MU
> +using the MSI bus.
> +
> +System Controller Device Node:
> +=============================

Need to specify this is a child of /firmware node.

> +
> +Required properties:
> +-------------------
> +- compatible: should be "fsl,imx-scu"
> +- mboxes: List of phandle of MU mailbox. Should be one of LSIO
> +	  MU0~M4 for imx8qxp and imx8qm. Users need to make
> +	  sure not use the one which is conflict with other
> +	  execution environments. e.g. ATF.
> +
> +Examples:
> +--------
> +lsio_mu1: mailbox at 5d1c0000 {
> +	compatible = "fsl,imx8qxp-mu";
> +	reg = <0x0 0x5d1c0000 0x0 0x10000>;
> +	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> +	#mbox-cells = <0>;
> +};
> +
> +scu {
> +	compatible = "fsl,imx-scu";
> +	mboxes = <&lsio_mu1>;
> +};
> +
> +
> +i.MX SCU Client Device Node:
> +=========================
> +
> +Client nodes are maintained as children of the relevant IMX-SCU device node.
> +
> +Example (imx8qxp):
> +-------------
> +scu {
> +	compatible = "fsl,imx-scu";
> +	...
> +
> +	clk: clk {
> +		...
> +	};
> +
> +	iomuxc: iomuxc {
> +		...
> +	};
> +
> +	imx8qx-pm {
> +		...
> +	}
> +	...

If you plan to have child nodes, you need to document them now. However, 
other than pinctrl, you probably don't need child nodes. The parent node 
can be a clock and power-domain provider.

Rob

RE: [PATCH V4 4/5] dt-bindings: arm: fsl: add scu binding doc

[A.s. Dong]

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Wednesday, July 11, 2018 11:09 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org; dongas86@gmail.com;
> kernel@pengutronix.de; shawnguo@kernel.org; Fabio Estevam
> <fabio.estevam@nxp.com>; dl-linux-imx <linux-imx@nxp.com>; Mark
> Rutland <mark.rutland@arm.com>; devicetree@vger.kernel.org
> Subject: Re: [PATCH V4 4/5] dt-bindings: arm: fsl: add scu binding doc
> 
> On Sun, Jul 08, 2018 at 10:56:56PM +0800, Dong Aisheng wrote:
> > The System Controller Firmware (SCFW) is a low-level system function
> > which runs on a dedicated Cortex-M core to provide power, clock, and
> > resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
> > (QM, QP), and i.MX8QX (QXP, DX).
> >
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> > v3->v4:
> >  * fully change to mailbox binding
> >  * add child node description
> > v2->v3:
> >  * update a bit to mailbox binding
> > v1->v2:
> >  * remove status
> >  * changed to mu1
> > ---
> >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 65
> > ++++++++++++++++++++++
> >  1 file changed, 65 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > new file mode 100644
> > index 0000000..11e732a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > @@ -0,0 +1,65 @@
> > +NXP i.MX System Controller Firmware (SCFW)
> > +--------------------------------------------------------------------
> > +
> > +The System Controller Firmware (SCFW) is a low-level system function
> > +which runs on a dedicated Cortex-M core to provide power, clock, and
> > +resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
> > +(QM, QP), and i.MX8QX (QXP, DX).
> > +
> > +The AP communicates with the SC using a multi-ported MU module found
> > +in the LSIO subsystem. The current definition of this MU module
> > +provides
> > +5 remote AP connections to the SC to support up to 5 execution
> > +environments (TZ, HV, standard Linux, etc.). The SC side of this MU
> > +module interfaces with the LSIO DSC IP bus. The SC firmware will
> > +communicate with this MU using the MSI bus.
> > +
> > +System Controller Device Node:
> > +=============================
> 
> Need to specify this is a child of /firmware node.
> 

Got it.

> > +
> > +Required properties:
> > +-------------------
> > +- compatible: should be "fsl,imx-scu"
> > +- mboxes: List of phandle of MU mailbox. Should be one of LSIO
> > +	  MU0~M4 for imx8qxp and imx8qm. Users need to make
> > +	  sure not use the one which is conflict with other
> > +	  execution environments. e.g. ATF.
> > +
> > +Examples:
> > +--------
> > +lsio_mu1: mailbox@5d1c0000 {
> > +	compatible = "fsl,imx8qxp-mu";
> > +	reg = <0x0 0x5d1c0000 0x0 0x10000>;
> > +	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> > +	#mbox-cells = <0>;
> > +};
> > +
> > +scu {
> > +	compatible = "fsl,imx-scu";
> > +	mboxes = <&lsio_mu1>;
> > +};
> > +
> > +
> > +i.MX SCU Client Device Node:
> > +=========================
> > +
> > +Client nodes are maintained as children of the relevant IMX-SCU device
> node.
> > +
> > +Example (imx8qxp):
> > +-------------
> > +scu {
> > +	compatible = "fsl,imx-scu";
> > +	...
> > +
> > +	clk: clk {
> > +		...
> > +	};
> > +
> > +	iomuxc: iomuxc {
> > +		...
> > +	};
> > +
> > +	imx8qx-pm {
> > +		...
> > +	}
> > +	...
> 
> If you plan to have child nodes, you need to document them now. However,
> other than pinctrl, you probably don't need child nodes. The parent node can
> be a clock and power-domain provider.
> 

Sorry I did not get the point.
This is referred to what TI does (arm scpi binding seems similar):
Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
Documentation/devicetree/bindings/clock/ti,sci-clk.txt

arch/arm/boot/dts/keystone-k2g.dtsi
pmmc: pmmc@2921c00 {
        compatible = "ti,k2g-sci";
	....

        k2g_pds: power-controller {
                compatible = "ti,sci-pm-domain";
                #power-domain-cells = <1>;
        };

        k2g_clks: clocks {
                compatible = "ti,k2g-sci-clk";
                #clock-cells = <2>;
        };

        k2g_reset: reset-controller {
                compatible = "ti,sci-reset";
                #reset-cells = <2>;
        };
};

For i.MX SCU, it's quite similar:
firmware {
        scu {
                compatible = "fsl,imx-scu";
                mboxes = <&lsio_mu1>;

                clk: clk {
                        compatible = "fsl,imx8qxp-clk";
                        #clock-cells = <1>;
                        mboxes = <&lsio_mu2>; //optinoal
                };

                iomuxc: iomuxc {
                        compatible = "fsl,imx8qxp-iomuxc";
                        fsl,mu = <&lsio_mu3>; //optinoal
                };

                imx8qx-pm {
                        compatible = "fsl,scu-pd";
                        #address-cells = <1>;
                        #size-cells = <0>;
                        fsl,mu = <&lsio_mu4>; //optinoal

                        pd_lsio: PD_LSIO {
                                #power-domain-cells = <0>;
                                #address-cells = <1>;
                                #size-cells = <0>;

                                pd_lsio_pwm0: lsio_pwm0@SC_R_PWM_0 {
                                        reg = <SC_R_PWM_0>;
                                        #power-domain-cells = <0>;
                                        power-domains = <&pd_lsio>;
                                };
                                ...
                        };

                        pd_dma: dma_power_domain {
                                #power-domain-cells = <0>;
                                #address-cells = <1>;
                                #size-cells = <0>;
                        };
                        ...
                };

                other_funcs {
                        compatible = "xxx";
                        ...
                };
        };
};

1. The child allows to specify a different mbox to use (planned optional)
2. we have separate clk, pinctrl, power domain drivers and etc.
3. we need define child pd nodes under the power domain node

Am I understanding correct that you are saying we can merge the clk and pd
driver into scu driver then scu behaviors both clk and pd provider?
It means we need implement both clk driver and pd driver in scu driver
which may bloat the scu driver a lot and make us no able to specify different
mbox for subnodes.
And clk driver has a lot clock type definitions which is under clock drivers.
Not sure suitable to be moved into scu.
See:
https://patchwork.ozlabs.org/cover/905920/

It will be like:
firmware {                              
        scu {                           
                compatible = "fsl,imx-scu";
                mboxes = <&lsio_mu1>;
                #clock-cells = <1>;
                #address-cells = <1>;
                #size-cells = <0>;

                iomuxc: iomuxc {
                        compatible = "fsl,imx8qxp-iomuxc";
                        fsl,mu = <&lsio_mu3>;
                };

                other_funcs: {
                        compatible = "thermal...";
                        ...
                };

                pd_lsio: PD_LSIO {
                        #power-domain-cells = <0>;
                        #address-cells = <1>;
                        #size-cells = <0>;

                        pd_lsio_pwm0: lsio_pwm0@SC_R_PWM_0 {
                                reg = <SC_R_PWM_0>;
                                #power-domain-cells = <0>;
                                power-domains = <&pd_lsio>;
                        };
                        ...
                };

                pd_vpu: vpu-power-domain@SC_R_VPU_PID0 {
                        reg = <SC_R_VPU_PID0>;
                        #power-domain-cells = <0>;
                        #address-cells = <1>;
                        #size-cells = <0>;
                };

                pd_xxx {
                        ...
                }
                ....
        };
};

How would you suggest for such case?

Regards
Dong Aisheng

> Rob

[PATCH V4 4/5] dt-bindings: arm: fsl: add scu binding doc

[A.s. Dong]

> -----Original Message-----
> From: Rob Herring [mailto:robh at kernel.org]
> Sent: Wednesday, July 11, 2018 11:09 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com;
> kernel at pengutronix.de; shawnguo at kernel.org; Fabio Estevam
> <fabio.estevam@nxp.com>; dl-linux-imx <linux-imx@nxp.com>; Mark
> Rutland <mark.rutland@arm.com>; devicetree at vger.kernel.org
> Subject: Re: [PATCH V4 4/5] dt-bindings: arm: fsl: add scu binding doc
> 
> On Sun, Jul 08, 2018 at 10:56:56PM +0800, Dong Aisheng wrote:
> > The System Controller Firmware (SCFW) is a low-level system function
> > which runs on a dedicated Cortex-M core to provide power, clock, and
> > resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
> > (QM, QP), and i.MX8QX (QXP, DX).
> >
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: devicetree at vger.kernel.org
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> > v3->v4:
> >  * fully change to mailbox binding
> >  * add child node description
> > v2->v3:
> >  * update a bit to mailbox binding
> > v1->v2:
> >  * remove status
> >  * changed to mu1
> > ---
> >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 65
> > ++++++++++++++++++++++
> >  1 file changed, 65 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > new file mode 100644
> > index 0000000..11e732a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > @@ -0,0 +1,65 @@
> > +NXP i.MX System Controller Firmware (SCFW)
> > +--------------------------------------------------------------------
> > +
> > +The System Controller Firmware (SCFW) is a low-level system function
> > +which runs on a dedicated Cortex-M core to provide power, clock, and
> > +resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
> > +(QM, QP), and i.MX8QX (QXP, DX).
> > +
> > +The AP communicates with the SC using a multi-ported MU module found
> > +in the LSIO subsystem. The current definition of this MU module
> > +provides
> > +5 remote AP connections to the SC to support up to 5 execution
> > +environments (TZ, HV, standard Linux, etc.). The SC side of this MU
> > +module interfaces with the LSIO DSC IP bus. The SC firmware will
> > +communicate with this MU using the MSI bus.
> > +
> > +System Controller Device Node:
> > +=============================
> 
> Need to specify this is a child of /firmware node.
> 

Got it.

> > +
> > +Required properties:
> > +-------------------
> > +- compatible: should be "fsl,imx-scu"
> > +- mboxes: List of phandle of MU mailbox. Should be one of LSIO
> > +	  MU0~M4 for imx8qxp and imx8qm. Users need to make
> > +	  sure not use the one which is conflict with other
> > +	  execution environments. e.g. ATF.
> > +
> > +Examples:
> > +--------
> > +lsio_mu1: mailbox at 5d1c0000 {
> > +	compatible = "fsl,imx8qxp-mu";
> > +	reg = <0x0 0x5d1c0000 0x0 0x10000>;
> > +	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> > +	#mbox-cells = <0>;
> > +};
> > +
> > +scu {
> > +	compatible = "fsl,imx-scu";
> > +	mboxes = <&lsio_mu1>;
> > +};
> > +
> > +
> > +i.MX SCU Client Device Node:
> > +=========================
> > +
> > +Client nodes are maintained as children of the relevant IMX-SCU device
> node.
> > +
> > +Example (imx8qxp):
> > +-------------
> > +scu {
> > +	compatible = "fsl,imx-scu";
> > +	...
> > +
> > +	clk: clk {
> > +		...
> > +	};
> > +
> > +	iomuxc: iomuxc {
> > +		...
> > +	};
> > +
> > +	imx8qx-pm {
> > +		...
> > +	}
> > +	...
> 
> If you plan to have child nodes, you need to document them now. However,
> other than pinctrl, you probably don't need child nodes. The parent node can
> be a clock and power-domain provider.
> 

Sorry I did not get the point.
This is referred to what TI does (arm scpi binding seems similar):
Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
Documentation/devicetree/bindings/clock/ti,sci-clk.txt

arch/arm/boot/dts/keystone-k2g.dtsi
pmmc: pmmc at 2921c00 {
        compatible = "ti,k2g-sci";
	....

        k2g_pds: power-controller {
                compatible = "ti,sci-pm-domain";
                #power-domain-cells = <1>;
        };

        k2g_clks: clocks {
                compatible = "ti,k2g-sci-clk";
                #clock-cells = <2>;
        };

        k2g_reset: reset-controller {
                compatible = "ti,sci-reset";
                #reset-cells = <2>;
        };
};

For i.MX SCU, it's quite similar:
firmware {
        scu {
                compatible = "fsl,imx-scu";
                mboxes = <&lsio_mu1>;

                clk: clk {
                        compatible = "fsl,imx8qxp-clk";
                        #clock-cells = <1>;
                        mboxes = <&lsio_mu2>; //optinoal
                };

                iomuxc: iomuxc {
                        compatible = "fsl,imx8qxp-iomuxc";
                        fsl,mu = <&lsio_mu3>; //optinoal
                };

                imx8qx-pm {
                        compatible = "fsl,scu-pd";
                        #address-cells = <1>;
                        #size-cells = <0>;
                        fsl,mu = <&lsio_mu4>; //optinoal

                        pd_lsio: PD_LSIO {
                                #power-domain-cells = <0>;
                                #address-cells = <1>;
                                #size-cells = <0>;

                                pd_lsio_pwm0: lsio_pwm0 at SC_R_PWM_0 {
                                        reg = <SC_R_PWM_0>;
                                        #power-domain-cells = <0>;
                                        power-domains = <&pd_lsio>;
                                };
                                ...
                        };

                        pd_dma: dma_power_domain {
                                #power-domain-cells = <0>;
                                #address-cells = <1>;
                                #size-cells = <0>;
                        };
                        ...
                };

                other_funcs {
                        compatible = "xxx";
                        ...
                };
        };
};

1. The child allows to specify a different mbox to use (planned optional)
2. we have separate clk, pinctrl, power domain drivers and etc.
3. we need define child pd nodes under the power domain node

Am I understanding correct that you are saying we can merge the clk and pd
driver into scu driver then scu behaviors both clk and pd provider?
It means we need implement both clk driver and pd driver in scu driver
which may bloat the scu driver a lot and make us no able to specify different
mbox for subnodes.
And clk driver has a lot clock type definitions which is under clock drivers.
Not sure suitable to be moved into scu.
See:
https://patchwork.ozlabs.org/cover/905920/

It will be like:
firmware {                              
        scu {                           
                compatible = "fsl,imx-scu";
                mboxes = <&lsio_mu1>;
                #clock-cells = <1>;
                #address-cells = <1>;
                #size-cells = <0>;

                iomuxc: iomuxc {
                        compatible = "fsl,imx8qxp-iomuxc";
                        fsl,mu = <&lsio_mu3>;
                };

                other_funcs: {
                        compatible = "thermal...";
                        ...
                };

                pd_lsio: PD_LSIO {
                        #power-domain-cells = <0>;
                        #address-cells = <1>;
                        #size-cells = <0>;

                        pd_lsio_pwm0: lsio_pwm0 at SC_R_PWM_0 {
                                reg = <SC_R_PWM_0>;
                                #power-domain-cells = <0>;
                                power-domains = <&pd_lsio>;
                        };
                        ...
                };

                pd_vpu: vpu-power-domain at SC_R_VPU_PID0 {
                        reg = <SC_R_VPU_PID0>;
                        #power-domain-cells = <0>;
                        #address-cells = <1>;
                        #size-cells = <0>;
                };

                pd_xxx {
                        ...
                }
                ....
        };
};

How would you suggest for such case?

Regards
Dong Aisheng

> Rob

[PATCH V4 5/5] soc: imx: add SC firmware IPC and APIs

[Dong Aisheng]

The System Controller Firmware (SCFW) is a low-level system function
which runs on a dedicated Cortex-M core to provide power, clock, and
resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
(QM, QP), and i.MX8QX (QXP, DX).

This patch adds the SC firmware service APIs used by the system.
It mainly consists of two parts:
1) Implementation of the IPC functions using MUs (client side).
2) SCU firmware services APIs implemented based on RPC calls

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v3->v4:
 * change to use mailbox
 * separate the IPC part and RPC API for easy review
 * headfiles cleanup
v2->v3:
 * add the ipc read/write error check
v1->v2:
 * change the type of sc_ipc_t from uint32_t to unsigned long.
This can make it be capable of storing 64 bit pointer in ARMv8 system.
 * Update to use the new MU library implemenation (handle iomem privately)
 * remove unused delcarations e.g. sc_rpc_dispatch and sc_rpc_xlate.
 * remove unneeded pad ctl functions
---
 drivers/soc/imx/Kconfig       |   4 +
 drivers/soc/imx/Makefile      |   1 +
 drivers/soc/imx/sc/Makefile   |   2 +
 drivers/soc/imx/sc/main/ipc.c | 185 ++++++++++
 drivers/soc/imx/sc/main/rpc.h |  79 +++++
 include/soc/imx/sc/ipc.h      |  45 +++
 include/soc/imx/sc/scfw.h     |  24 ++
 include/soc/imx/sc/types.h    | 764 ++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 1104 insertions(+)
 create mode 100644 drivers/soc/imx/sc/Makefile
 create mode 100644 drivers/soc/imx/sc/main/ipc.c
 create mode 100644 drivers/soc/imx/sc/main/rpc.h
 create mode 100644 include/soc/imx/sc/ipc.h
 create mode 100644 include/soc/imx/sc/scfw.h
 create mode 100644 include/soc/imx/sc/types.h

diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig
index a5b86a2..8a28474 100644
--- a/drivers/soc/imx/Kconfig
+++ b/drivers/soc/imx/Kconfig
@@ -7,4 +7,8 @@ config IMX7_PM_DOMAINS
 	select PM_GENERIC_DOMAINS
 	default y if SOC_IMX7D
 
+config HAVE_IMX_SCU
+	bool
+	depends on IMX_MU
+
 endmenu
diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
index aab41a5c..be79f67 100644
--- a/drivers/soc/imx/Makefile
+++ b/drivers/soc/imx/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
 obj-$(CONFIG_IMX7_PM_DOMAINS) += gpcv2.o
+obj-$(CONFIG_HAVE_IMX_SCU) += sc/
diff --git a/drivers/soc/imx/sc/Makefile b/drivers/soc/imx/sc/Makefile
new file mode 100644
index 0000000..a3db142
--- /dev/null
+++ b/drivers/soc/imx/sc/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-y += main/ipc.o
diff --git a/drivers/soc/imx/sc/main/ipc.c b/drivers/soc/imx/sc/main/ipc.c
new file mode 100644
index 0000000..e805aab
--- /dev/null
+++ b/drivers/soc/imx/sc/main/ipc.c
@@ -0,0 +1,185 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017~2018 NXP
+ *	Dong Aisheng <aisheng.dong@nxp.com>
+ *
+ * Implementation of the IPC functions using MUs (client side).
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_client.h>
+#include <linux/mutex.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#include "rpc.h"
+
+#define MAX_RX_TIMEOUT		(msecs_to_jiffies(30))
+
+/* Local Types */
+struct sc_ipc {
+	struct mbox_client cl;
+	struct mbox_chan *chan;
+	struct mutex lock;
+	struct completion done;
+};
+
+static sc_ipc_t scu_ipc_handle;
+
+/*
+ * Get the default handle used by SCU
+ */
+int sc_ipc_get_handle(sc_ipc_t *ipc)
+{
+	if (!scu_ipc_handle)
+		return -EPROBE_DEFER;
+
+	*ipc = scu_ipc_handle;
+	return 0;
+}
+EXPORT_SYMBOL(sc_ipc_get_handle);
+
+static void sc_rx_callback(struct mbox_client *cl, void *mssg)
+{
+	struct sc_ipc *sc_ipc = container_of(cl, struct sc_ipc, cl);
+
+	complete(&sc_ipc->done);
+}
+
+/*
+ * RPC command/response
+ */
+int sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp)
+{
+	struct sc_ipc *sc_ipc = (struct sc_ipc *)ipc;
+	int ret;
+
+	if (WARN_ON(!sc_ipc || !msg))
+		return -EINVAL;
+
+	/* Check size */
+	if (msg->size > SC_RPC_MAX_MSG)
+		return -EINVAL;
+
+	dev_dbg(sc_ipc->cl.dev, "RPC SVC %u FUNC %u SIZE %u\n", RPC_SVC(msg),
+		RPC_FUNC(msg), RPC_SIZE(msg));
+
+	mutex_lock(&sc_ipc->lock);
+
+	reinit_completion(&sc_ipc->done);
+
+	ret = mbox_send_message(sc_ipc->chan, msg);
+	if (ret < 0) {
+		dev_err(sc_ipc->cl.dev, "sc send msg failed: %d\n", ret);
+		goto out;
+	}
+
+	ret = 0;
+
+	if (!no_resp) {
+		/* waiting for rx process done */
+		if (!wait_for_completion_timeout(&sc_ipc->done, MAX_RX_TIMEOUT))
+			ret = -ETIMEDOUT;
+	}
+
+	mbox_client_txdone(sc_ipc->chan, ret);
+
+out:
+	mutex_unlock(&sc_ipc->lock);
+
+	dev_dbg(sc_ipc->cl.dev, "RPC SVC done\n");
+
+	return ret;
+}
+
+/*
+ * Open an IPC channel
+ */
+int sc_ipc_open(sc_ipc_t *ipc, struct device *dev)
+{
+	struct sc_ipc *sc_ipc;
+	int ret;
+
+	sc_ipc = devm_kzalloc(dev, sizeof(*sc_ipc), GFP_KERNEL);
+	if (!sc_ipc)
+		return -ENOMEM;
+
+	sc_ipc->cl.dev = dev;
+	sc_ipc->cl.rx_callback = sc_rx_callback;
+	sc_ipc->cl.tx_block = false;
+	sc_ipc->cl.knows_txdone = true;
+
+	sc_ipc->chan = mbox_request_channel(&sc_ipc->cl, 0);
+	if (IS_ERR(sc_ipc->chan)) {
+		ret = PTR_ERR(sc_ipc->chan);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get mbox %d\n", ret);
+
+		return ret;
+	}
+
+	mutex_init(&sc_ipc->lock);
+	init_completion(&sc_ipc->done);
+
+	*ipc = (sc_ipc_t)sc_ipc;
+
+	return 0;
+}
+EXPORT_SYMBOL(sc_ipc_open);
+
+/*
+ * Close an IPC channel
+ */
+void sc_ipc_close(sc_ipc_t ipc)
+{
+	struct sc_ipc *sc_ipc = (struct sc_ipc *)ipc;
+
+	WARN_ON(!sc_ipc);
+
+	/* we do not free the default ipc handle */
+	if (ipc == scu_ipc_handle)
+		return;
+
+	mbox_free_channel(sc_ipc->chan);
+	devm_kfree(sc_ipc->cl.dev, sc_ipc);
+}
+EXPORT_SYMBOL(sc_ipc_close);
+
+/* Initialization of the MU code. */
+static int imx_sc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = sc_ipc_open(&scu_ipc_handle, dev);
+	if (ret)
+		return ret;
+
+	pr_info("NXP i.MX SCU Initialized\n");
+
+	return devm_of_platform_populate(dev);
+}
+
+static const struct of_device_id imx_sc_match[] = {
+	{ .compatible = "fsl,imx-scu", },
+	{ /* Sentinel */ }
+};
+
+static struct platform_driver imx_sc_driver = {
+	.driver = {
+		.name = "imx-scu",
+		.of_match_table = imx_sc_match,
+	},
+	.probe = imx_sc_probe,
+};
+
+static int __init imx_sc_init(void)
+{
+	return platform_driver_register(&imx_sc_driver);
+}
+core_initcall(imx_sc_init);
diff --git a/drivers/soc/imx/sc/main/rpc.h b/drivers/soc/imx/sc/main/rpc.h
new file mode 100644
index 0000000..43b8cec
--- /dev/null
+++ b/drivers/soc/imx/sc/main/rpc.h
@@ -0,0 +1,79 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017~2018 NXP
+ *
+ * Header file for the RPC implementation.
+ */
+
+#ifndef _SC_RPC_H
+#define _SC_RPC_H
+
+/* Includes */
+
+#include <soc/imx/sc/types.h>
+#include <soc/imx/sc/ipc.h>
+
+/* Defines */
+
+#define SC_RPC_VERSION		1
+
+#define SC_RPC_MAX_MSG		8
+
+#define RPC_VER(MSG)            ((MSG)->version)
+#define RPC_SIZE(MSG)           ((MSG)->size)
+#define RPC_SVC(MSG)            ((MSG)->svc)
+#define RPC_FUNC(MSG)           ((MSG)->func)
+#define RPC_R8(MSG)             ((MSG)->func)
+#define RPC_I32(MSG, IDX)       ((MSG)->DATA.i32[(IDX) / 4])
+#define RPC_I16(MSG, IDX)       ((MSG)->DATA.i16[(IDX) / 2])
+#define RPC_I8(MSG, IDX)        ((MSG)->DATA.i8[(IDX)])
+#define RPC_U32(MSG, IDX)       ((MSG)->DATA.u32[(IDX) / 4])
+#define RPC_U16(MSG, IDX)       ((MSG)->DATA.u16[(IDX) / 2])
+#define RPC_U8(MSG, IDX)        ((MSG)->DATA.u8[(IDX)])
+
+/* Types */
+
+typedef enum sc_rpc_svc_e {
+	SC_RPC_SVC_UNKNOWN = 0,
+	SC_RPC_SVC_RETURN = 1,
+	SC_RPC_SVC_PM = 2,
+	SC_RPC_SVC_RM = 3,
+	SC_RPC_SVC_TIMER = 5,
+	SC_RPC_SVC_PAD = 6,
+	SC_RPC_SVC_MISC = 7,
+	SC_RPC_SVC_IRQ = 8,
+	SC_RPC_SVC_ABORT = 9
+} sc_rpc_svc_t;
+
+typedef struct sc_rpc_msg_s {
+	uint8_t version;
+	uint8_t size;
+	uint8_t svc;
+	uint8_t func;
+	union {
+		int32_t i32[(SC_RPC_MAX_MSG - 1)];
+		int16_t i16[(SC_RPC_MAX_MSG - 1) * 2];
+		int8_t i8[(SC_RPC_MAX_MSG - 1) * 4];
+		uint32_t u32[(SC_RPC_MAX_MSG - 1)];
+		uint16_t u16[(SC_RPC_MAX_MSG - 1) * 2];
+		uint8_t u8[(SC_RPC_MAX_MSG - 1) * 4];
+	} DATA;
+} sc_rpc_msg_t;
+
+/* Functions */
+
+/*
+ * This is an internal function to send an RPC message over an IPC
+ * channel. It is called by client-side SCFW API function shims.
+ *
+ * @param[in]     ipc         IPC handle
+ * @param[in,out] msg         handle to a message
+ * @param[in]     no_resp     response flag
+ *
+ * If no_resp is false then this function waits for a response
+ * and returns the result in msg.
+ */
+int sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp);
+
+#endif /* _SC_RPC_H */
diff --git a/include/soc/imx/sc/ipc.h b/include/soc/imx/sc/ipc.h
new file mode 100644
index 0000000..24436b4
--- /dev/null
+++ b/include/soc/imx/sc/ipc.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017~2018 NXP
+ *
+ * Header file for the IPC implementation.
+ */
+
+#ifndef _SC_IPC_H
+#define _SC_IPC_H
+
+/* Includes */
+
+#include <linux/device.h>
+
+#include <soc/imx/sc/types.h>
+
+/* Functions */
+
+/*
+ * This function opens an IPC channel.
+ *
+ * @param[out]    ipc         return pointer for ipc handle
+ * @param[in]     dev         device to open mu channel
+ *
+ * @return Returns an error code (0 = success, failed if < 0)
+ */
+int sc_ipc_open(sc_ipc_t *ipc, struct device *dev);
+
+/*
+ * This function closes an IPC channel.
+ *
+ * @param[in]     ipc         id of channel to close
+ */
+void sc_ipc_close(sc_ipc_t ipc);
+
+/*
+ * This function gets the default ipc handle used by SCU
+ *
+ * @param[out]     ipc         id of channel to close
+ *
+ * @return Returns an error code (0 = success, failed if < 0)
+ */
+int sc_ipc_get_handle(sc_ipc_t *ipc);
+#endif /* _SC_IPC_H */
diff --git a/include/soc/imx/sc/scfw.h b/include/soc/imx/sc/scfw.h
new file mode 100644
index 0000000..b66925b
--- /dev/null
+++ b/include/soc/imx/sc/scfw.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017~2018 NXP
+ */
+
+#ifndef _SC_SCFW_H
+#define _SC_SCFW_H
+
+#include <linux/types.h>
+
+/*
+ * This type is used to declare a handle for an IPC communication
+ * channel. Its meaning is specific to the IPC implementation.
+ */
+typedef unsigned long sc_ipc_t;
+
+/*
+ * This type is used to declare an ID for an IPC communication
+ * channel. Its meaning is specific to the IPC implementation.
+ */
+typedef unsigned long sc_ipc_id_t;
+
+#endif /* _SC_SCFW_H */
diff --git a/include/soc/imx/sc/types.h b/include/soc/imx/sc/types.h
new file mode 100644
index 0000000..343efe6
--- /dev/null
+++ b/include/soc/imx/sc/types.h
@@ -0,0 +1,764 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017~2018 NXP
+ *
+ * Header file containing types used across multiple service APIs.
+ */
+
+#ifndef _SC_TYPES_H
+#define _SC_TYPES_H
+
+/* Includes */
+
+#include <soc/imx/sc/scfw.h>
+
+/* Defines */
+
+/*
+ * @name Defines for common frequencies
+ */
+#define SC_32KHZ            32768	/* 32KHz */
+#define SC_10MHZ         10000000	/* 10MHz */
+#define SC_20MHZ         20000000	/* 20MHz */
+#define SC_25MHZ         25000000	/* 25MHz */
+#define SC_27MHZ         27000000	/* 27MHz */
+#define SC_40MHZ         40000000	/* 40MHz */
+#define SC_45MHZ         45000000	/* 45MHz */
+#define SC_50MHZ         50000000	/* 50MHz */
+#define SC_60MHZ         60000000	/* 60MHz */
+#define SC_66MHZ         66666666	/* 66MHz */
+#define SC_74MHZ         74250000	/* 74.25MHz */
+#define SC_80MHZ         80000000	/* 80MHz */
+#define SC_83MHZ         83333333	/* 83MHz */
+#define SC_84MHZ         84375000	/* 84.37MHz */
+#define SC_100MHZ       100000000	/* 100MHz */
+#define SC_125MHZ       125000000	/* 125MHz */
+#define SC_133MHZ       133333333	/* 133MHz */
+#define SC_135MHZ       135000000	/* 135MHz */
+#define SC_150MHZ       150000000	/* 150MHz */
+#define SC_160MHZ       160000000	/* 160MHz */
+#define SC_166MHZ       166666666	/* 160MHz */
+#define SC_175MHZ       175000000	/* 175MHz */
+#define SC_180MHZ       180000000	/* 180MHz */
+#define SC_200MHZ       200000000	/* 200MHz */
+#define SC_250MHZ       250000000	/* 250MHz */
+#define SC_266MHZ       266666666	/* 266MHz */
+#define SC_300MHZ       300000000	/* 300MHz */
+#define SC_320MHZ       320000000	/* 320MHz */
+#define SC_325MHZ       325000000	/* 325MHz */
+#define SC_333MHZ       333333333	/* 333MHz */
+#define SC_350MHZ       350000000	/* 350MHz */
+#define SC_372MHZ       372000000	/* 372MHz */
+#define SC_375MHZ       375000000	/* 375MHz */
+#define SC_400MHZ       400000000	/* 400MHz */
+#define SC_500MHZ       500000000	/* 500MHz */
+#define SC_594MHZ       594000000	/* 594MHz */
+#define SC_650MHZ       650000000	/* 650MHz */
+#define SC_667MHZ       666666667	/* 667MHz */
+#define SC_675MHZ       675000000	/* 675MHz */
+#define SC_700MHZ       700000000	/* 700MHz */
+#define SC_720MHZ       720000000	/* 720MHz */
+#define SC_750MHZ       750000000	/* 750MHz */
+#define SC_800MHZ       800000000	/* 800MHz */
+#define SC_850MHZ       850000000	/* 850MHz */
+#define SC_900MHZ       900000000	/* 900MHz */
+#define SC_1000MHZ     1000000000	/* 1GHz */
+#define SC_1056MHZ     1056000000	/* 1.056GHz */
+#define SC_1188MHZ     1188000000	/* 1.188GHz */
+#define SC_1260MHZ     1260000000	/* 1.26GHz */
+#define SC_1300MHZ     1300000000	/* 1.3GHz */
+#define SC_1400MHZ     1400000000	/* 1.4GHz */
+#define SC_1500MHZ     1500000000	/* 1.5GHz */
+#define SC_1600MHZ     1600000000	/* 1.6GHz */
+#define SC_1800MHZ     1800000000	/* 1.8GHz */
+#define SC_2000MHZ     2000000000	/* 2.0GHz */
+#define SC_2112MHZ     2112000000	/* 2.12GHz */
+
+/*
+ * @name Defines for 24M related frequencies
+ */
+#define SC_8MHZ           8000000	/* 8MHz */
+#define SC_12MHZ         12000000	/* 12MHz */
+#define SC_19MHZ         19800000	/* 19.8MHz */
+#define SC_24MHZ         24000000	/* 24MHz */
+#define SC_48MHZ         48000000	/* 48MHz */
+#define SC_120MHZ       120000000	/* 120MHz */
+#define SC_132MHZ       132000000	/* 132MHz */
+#define SC_144MHZ       144000000	/* 144MHz */
+#define SC_192MHZ       192000000	/* 192MHz */
+#define SC_211MHZ       211200000	/* 211.2MHz */
+#define SC_240MHZ       240000000	/* 240MHz */
+#define SC_264MHZ       264000000	/* 264MHz */
+#define SC_352MHZ       352000000	/* 352MHz */
+#define SC_360MHZ       360000000	/* 360MHz */
+#define SC_384MHZ       384000000	/* 384MHz */
+#define SC_396MHZ       396000000	/* 396MHz */
+#define SC_432MHZ       432000000	/* 432MHz */
+#define SC_480MHZ       480000000	/* 480MHz */
+#define SC_600MHZ       600000000	/* 600MHz */
+#define SC_744MHZ       744000000	/* 744MHz */
+#define SC_792MHZ       792000000	/* 792MHz */
+#define SC_864MHZ       864000000	/* 864MHz */
+#define SC_960MHZ       960000000	/* 960MHz */
+#define SC_1056MHZ     1056000000	/* 1056MHz */
+#define SC_1200MHZ     1200000000	/* 1.2GHz */
+#define SC_1464MHZ     1464000000	/* 1.464GHz */
+#define SC_2400MHZ     2400000000	/* 2.4GHz */
+
+/*
+ * @name Defines for A/V related frequencies
+ */
+#define SC_62MHZ         62937500	/* 62.9375MHz */
+#define SC_755MHZ       755250000	/* 755.25MHz */
+
+/*
+ * @name Defines for type widths
+ */
+#define SC_FADDR_W      36	/* Width of sc_faddr_t */
+#define SC_BOOL_W       1	/* Width of bool */
+#define SC_ERR_W        4	/* Width of sc_err_t */
+#define SC_RSRC_W       10	/* Width of sc_rsrc_t */
+#define SC_CTRL_W       6	/* Width of sc_ctrl_t */
+
+#define SC_R_ALL        UINT16_MAX	/* All resources */
+#define SC_P_ALL        UINT16_MAX	/* All pads */
+
+/*
+ * This type is used to store a system (full-size) address.
+ */
+typedef uint64_t sc_faddr_t;
+
+/*
+ * This type is used to indicate error response for most functions.
+ */
+typedef enum sc_err_e {
+	SC_ERR_NONE = 0,	/* Success */
+	SC_ERR_VERSION = 1,	/* Incompatible API version */
+	SC_ERR_CONFIG = 2,	/* Configuration error */
+	SC_ERR_PARM = 3,	/* Bad parameter */
+	SC_ERR_NOACCESS = 4,	/* Permission error (no access) */
+	SC_ERR_LOCKED = 5,	/* Permission error (locked) */
+	SC_ERR_UNAVAILABLE = 6,	/* Unavailable (out of resources) */
+	SC_ERR_NOTFOUND = 7,	/* Not found */
+	SC_ERR_NOPOWER = 8,	/* No power */
+	SC_ERR_IPC = 9,		/* Generic IPC error */
+	SC_ERR_BUSY = 10,	/* Resource is currently busy/active */
+	SC_ERR_FAIL = 11,	/* General I/O failure */
+	SC_ERR_LAST
+} sc_err_t;
+
+/*
+ * This type is used to indicate a resource. Resources include peripherals
+ * and bus masters (but not memory regions). Note items from list should
+ * never be changed or removed (only added to at the end of the list).
+ */
+typedef enum sc_rsrc_e {
+	SC_R_A53 = 0,
+	SC_R_A53_0 = 1,
+	SC_R_A53_1 = 2,
+	SC_R_A53_2 = 3,
+	SC_R_A53_3 = 4,
+	SC_R_A72 = 5,
+	SC_R_A72_0 = 6,
+	SC_R_A72_1 = 7,
+	SC_R_A72_2 = 8,
+	SC_R_A72_3 = 9,
+	SC_R_CCI = 10,
+	SC_R_DB = 11,
+	SC_R_DRC_0 = 12,
+	SC_R_DRC_1 = 13,
+	SC_R_GIC_SMMU = 14,
+	SC_R_IRQSTR_M4_0 = 15,
+	SC_R_IRQSTR_M4_1 = 16,
+	SC_R_SMMU = 17,
+	SC_R_GIC = 18,
+	SC_R_DC_0_BLIT0 = 19,
+	SC_R_DC_0_BLIT1 = 20,
+	SC_R_DC_0_BLIT2 = 21,
+	SC_R_DC_0_BLIT_OUT = 22,
+	SC_R_DC_0_CAPTURE0 = 23,
+	SC_R_DC_0_CAPTURE1 = 24,
+	SC_R_DC_0_WARP = 25,
+	SC_R_DC_0_INTEGRAL0 = 26,
+	SC_R_DC_0_INTEGRAL1 = 27,
+	SC_R_DC_0_VIDEO0 = 28,
+	SC_R_DC_0_VIDEO1 = 29,
+	SC_R_DC_0_FRAC0 = 30,
+	SC_R_DC_0_FRAC1 = 31,
+	SC_R_DC_0 = 32,
+	SC_R_GPU_2_PID0 = 33,
+	SC_R_DC_0_PLL_0 = 34,
+	SC_R_DC_0_PLL_1 = 35,
+	SC_R_DC_1_BLIT0 = 36,
+	SC_R_DC_1_BLIT1 = 37,
+	SC_R_DC_1_BLIT2 = 38,
+	SC_R_DC_1_BLIT_OUT = 39,
+	SC_R_DC_1_CAPTURE0 = 40,
+	SC_R_DC_1_CAPTURE1 = 41,
+	SC_R_DC_1_WARP = 42,
+	SC_R_DC_1_INTEGRAL0 = 43,
+	SC_R_DC_1_INTEGRAL1 = 44,
+	SC_R_DC_1_VIDEO0 = 45,
+	SC_R_DC_1_VIDEO1 = 46,
+	SC_R_DC_1_FRAC0 = 47,
+	SC_R_DC_1_FRAC1 = 48,
+	SC_R_DC_1 = 49,
+	SC_R_GPU_3_PID0 = 50,
+	SC_R_DC_1_PLL_0 = 51,
+	SC_R_DC_1_PLL_1 = 52,
+	SC_R_SPI_0 = 53,
+	SC_R_SPI_1 = 54,
+	SC_R_SPI_2 = 55,
+	SC_R_SPI_3 = 56,
+	SC_R_UART_0 = 57,
+	SC_R_UART_1 = 58,
+	SC_R_UART_2 = 59,
+	SC_R_UART_3 = 60,
+	SC_R_UART_4 = 61,
+	SC_R_EMVSIM_0 = 62,
+	SC_R_EMVSIM_1 = 63,
+	SC_R_DMA_0_CH0 = 64,
+	SC_R_DMA_0_CH1 = 65,
+	SC_R_DMA_0_CH2 = 66,
+	SC_R_DMA_0_CH3 = 67,
+	SC_R_DMA_0_CH4 = 68,
+	SC_R_DMA_0_CH5 = 69,
+	SC_R_DMA_0_CH6 = 70,
+	SC_R_DMA_0_CH7 = 71,
+	SC_R_DMA_0_CH8 = 72,
+	SC_R_DMA_0_CH9 = 73,
+	SC_R_DMA_0_CH10 = 74,
+	SC_R_DMA_0_CH11 = 75,
+	SC_R_DMA_0_CH12 = 76,
+	SC_R_DMA_0_CH13 = 77,
+	SC_R_DMA_0_CH14 = 78,
+	SC_R_DMA_0_CH15 = 79,
+	SC_R_DMA_0_CH16 = 80,
+	SC_R_DMA_0_CH17 = 81,
+	SC_R_DMA_0_CH18 = 82,
+	SC_R_DMA_0_CH19 = 83,
+	SC_R_DMA_0_CH20 = 84,
+	SC_R_DMA_0_CH21 = 85,
+	SC_R_DMA_0_CH22 = 86,
+	SC_R_DMA_0_CH23 = 87,
+	SC_R_DMA_0_CH24 = 88,
+	SC_R_DMA_0_CH25 = 89,
+	SC_R_DMA_0_CH26 = 90,
+	SC_R_DMA_0_CH27 = 91,
+	SC_R_DMA_0_CH28 = 92,
+	SC_R_DMA_0_CH29 = 93,
+	SC_R_DMA_0_CH30 = 94,
+	SC_R_DMA_0_CH31 = 95,
+	SC_R_I2C_0 = 96,
+	SC_R_I2C_1 = 97,
+	SC_R_I2C_2 = 98,
+	SC_R_I2C_3 = 99,
+	SC_R_I2C_4 = 100,
+	SC_R_ADC_0 = 101,
+	SC_R_ADC_1 = 102,
+	SC_R_FTM_0 = 103,
+	SC_R_FTM_1 = 104,
+	SC_R_CAN_0 = 105,
+	SC_R_CAN_1 = 106,
+	SC_R_CAN_2 = 107,
+	SC_R_DMA_1_CH0 = 108,
+	SC_R_DMA_1_CH1 = 109,
+	SC_R_DMA_1_CH2 = 110,
+	SC_R_DMA_1_CH3 = 111,
+	SC_R_DMA_1_CH4 = 112,
+	SC_R_DMA_1_CH5 = 113,
+	SC_R_DMA_1_CH6 = 114,
+	SC_R_DMA_1_CH7 = 115,
+	SC_R_DMA_1_CH8 = 116,
+	SC_R_DMA_1_CH9 = 117,
+	SC_R_DMA_1_CH10 = 118,
+	SC_R_DMA_1_CH11 = 119,
+	SC_R_DMA_1_CH12 = 120,
+	SC_R_DMA_1_CH13 = 121,
+	SC_R_DMA_1_CH14 = 122,
+	SC_R_DMA_1_CH15 = 123,
+	SC_R_DMA_1_CH16 = 124,
+	SC_R_DMA_1_CH17 = 125,
+	SC_R_DMA_1_CH18 = 126,
+	SC_R_DMA_1_CH19 = 127,
+	SC_R_DMA_1_CH20 = 128,
+	SC_R_DMA_1_CH21 = 129,
+	SC_R_DMA_1_CH22 = 130,
+	SC_R_DMA_1_CH23 = 131,
+	SC_R_DMA_1_CH24 = 132,
+	SC_R_DMA_1_CH25 = 133,
+	SC_R_DMA_1_CH26 = 134,
+	SC_R_DMA_1_CH27 = 135,
+	SC_R_DMA_1_CH28 = 136,
+	SC_R_DMA_1_CH29 = 137,
+	SC_R_DMA_1_CH30 = 138,
+	SC_R_DMA_1_CH31 = 139,
+	SC_R_UNUSED1 = 140,
+	SC_R_UNUSED2 = 141,
+	SC_R_UNUSED3 = 142,
+	SC_R_UNUSED4 = 143,
+	SC_R_GPU_0_PID0 = 144,
+	SC_R_GPU_0_PID1 = 145,
+	SC_R_GPU_0_PID2 = 146,
+	SC_R_GPU_0_PID3 = 147,
+	SC_R_GPU_1_PID0 = 148,
+	SC_R_GPU_1_PID1 = 149,
+	SC_R_GPU_1_PID2 = 150,
+	SC_R_GPU_1_PID3 = 151,
+	SC_R_PCIE_A = 152,
+	SC_R_SERDES_0 = 153,
+	SC_R_MATCH_0 = 154,
+	SC_R_MATCH_1 = 155,
+	SC_R_MATCH_2 = 156,
+	SC_R_MATCH_3 = 157,
+	SC_R_MATCH_4 = 158,
+	SC_R_MATCH_5 = 159,
+	SC_R_MATCH_6 = 160,
+	SC_R_MATCH_7 = 161,
+	SC_R_MATCH_8 = 162,
+	SC_R_MATCH_9 = 163,
+	SC_R_MATCH_10 = 164,
+	SC_R_MATCH_11 = 165,
+	SC_R_MATCH_12 = 166,
+	SC_R_MATCH_13 = 167,
+	SC_R_MATCH_14 = 168,
+	SC_R_PCIE_B = 169,
+	SC_R_SATA_0 = 170,
+	SC_R_SERDES_1 = 171,
+	SC_R_HSIO_GPIO = 172,
+	SC_R_MATCH_15 = 173,
+	SC_R_MATCH_16 = 174,
+	SC_R_MATCH_17 = 175,
+	SC_R_MATCH_18 = 176,
+	SC_R_MATCH_19 = 177,
+	SC_R_MATCH_20 = 178,
+	SC_R_MATCH_21 = 179,
+	SC_R_MATCH_22 = 180,
+	SC_R_MATCH_23 = 181,
+	SC_R_MATCH_24 = 182,
+	SC_R_MATCH_25 = 183,
+	SC_R_MATCH_26 = 184,
+	SC_R_MATCH_27 = 185,
+	SC_R_MATCH_28 = 186,
+	SC_R_LCD_0 = 187,
+	SC_R_LCD_0_PWM_0 = 188,
+	SC_R_LCD_0_I2C_0 = 189,
+	SC_R_LCD_0_I2C_1 = 190,
+	SC_R_PWM_0 = 191,
+	SC_R_PWM_1 = 192,
+	SC_R_PWM_2 = 193,
+	SC_R_PWM_3 = 194,
+	SC_R_PWM_4 = 195,
+	SC_R_PWM_5 = 196,
+	SC_R_PWM_6 = 197,
+	SC_R_PWM_7 = 198,
+	SC_R_GPIO_0 = 199,
+	SC_R_GPIO_1 = 200,
+	SC_R_GPIO_2 = 201,
+	SC_R_GPIO_3 = 202,
+	SC_R_GPIO_4 = 203,
+	SC_R_GPIO_5 = 204,
+	SC_R_GPIO_6 = 205,
+	SC_R_GPIO_7 = 206,
+	SC_R_GPT_0 = 207,
+	SC_R_GPT_1 = 208,
+	SC_R_GPT_2 = 209,
+	SC_R_GPT_3 = 210,
+	SC_R_GPT_4 = 211,
+	SC_R_KPP = 212,
+	SC_R_MU_0A = 213,
+	SC_R_MU_1A = 214,
+	SC_R_MU_2A = 215,
+	SC_R_MU_3A = 216,
+	SC_R_MU_4A = 217,
+	SC_R_MU_5A = 218,
+	SC_R_MU_6A = 219,
+	SC_R_MU_7A = 220,
+	SC_R_MU_8A = 221,
+	SC_R_MU_9A = 222,
+	SC_R_MU_10A = 223,
+	SC_R_MU_11A = 224,
+	SC_R_MU_12A = 225,
+	SC_R_MU_13A = 226,
+	SC_R_MU_5B = 227,
+	SC_R_MU_6B = 228,
+	SC_R_MU_7B = 229,
+	SC_R_MU_8B = 230,
+	SC_R_MU_9B = 231,
+	SC_R_MU_10B = 232,
+	SC_R_MU_11B = 233,
+	SC_R_MU_12B = 234,
+	SC_R_MU_13B = 235,
+	SC_R_ROM_0 = 236,
+	SC_R_FSPI_0 = 237,
+	SC_R_FSPI_1 = 238,
+	SC_R_IEE = 239,
+	SC_R_IEE_R0 = 240,
+	SC_R_IEE_R1 = 241,
+	SC_R_IEE_R2 = 242,
+	SC_R_IEE_R3 = 243,
+	SC_R_IEE_R4 = 244,
+	SC_R_IEE_R5 = 245,
+	SC_R_IEE_R6 = 246,
+	SC_R_IEE_R7 = 247,
+	SC_R_SDHC_0 = 248,
+	SC_R_SDHC_1 = 249,
+	SC_R_SDHC_2 = 250,
+	SC_R_ENET_0 = 251,
+	SC_R_ENET_1 = 252,
+	SC_R_MLB_0 = 253,
+	SC_R_DMA_2_CH0 = 254,
+	SC_R_DMA_2_CH1 = 255,
+	SC_R_DMA_2_CH2 = 256,
+	SC_R_DMA_2_CH3 = 257,
+	SC_R_DMA_2_CH4 = 258,
+	SC_R_USB_0 = 259,
+	SC_R_USB_1 = 260,
+	SC_R_USB_0_PHY = 261,
+	SC_R_USB_2 = 262,
+	SC_R_USB_2_PHY = 263,
+	SC_R_DTCP = 264,
+	SC_R_NAND = 265,
+	SC_R_LVDS_0 = 266,
+	SC_R_LVDS_0_PWM_0 = 267,
+	SC_R_LVDS_0_I2C_0 = 268,
+	SC_R_LVDS_0_I2C_1 = 269,
+	SC_R_LVDS_1 = 270,
+	SC_R_LVDS_1_PWM_0 = 271,
+	SC_R_LVDS_1_I2C_0 = 272,
+	SC_R_LVDS_1_I2C_1 = 273,
+	SC_R_LVDS_2 = 274,
+	SC_R_LVDS_2_PWM_0 = 275,
+	SC_R_LVDS_2_I2C_0 = 276,
+	SC_R_LVDS_2_I2C_1 = 277,
+	SC_R_M4_0_PID0 = 278,
+	SC_R_M4_0_PID1 = 279,
+	SC_R_M4_0_PID2 = 280,
+	SC_R_M4_0_PID3 = 281,
+	SC_R_M4_0_PID4 = 282,
+	SC_R_M4_0_RGPIO = 283,
+	SC_R_M4_0_SEMA42 = 284,
+	SC_R_M4_0_TPM = 285,
+	SC_R_M4_0_PIT = 286,
+	SC_R_M4_0_UART = 287,
+	SC_R_M4_0_I2C = 288,
+	SC_R_M4_0_INTMUX = 289,
+	SC_R_M4_0_SIM = 290,
+	SC_R_M4_0_WDOG = 291,
+	SC_R_M4_0_MU_0B = 292,
+	SC_R_M4_0_MU_0A0 = 293,
+	SC_R_M4_0_MU_0A1 = 294,
+	SC_R_M4_0_MU_0A2 = 295,
+	SC_R_M4_0_MU_0A3 = 296,
+	SC_R_M4_0_MU_1A = 297,
+	SC_R_M4_1_PID0 = 298,
+	SC_R_M4_1_PID1 = 299,
+	SC_R_M4_1_PID2 = 300,
+	SC_R_M4_1_PID3 = 301,
+	SC_R_M4_1_PID4 = 302,
+	SC_R_M4_1_RGPIO = 303,
+	SC_R_M4_1_SEMA42 = 304,
+	SC_R_M4_1_TPM = 305,
+	SC_R_M4_1_PIT = 306,
+	SC_R_M4_1_UART = 307,
+	SC_R_M4_1_I2C = 308,
+	SC_R_M4_1_INTMUX = 309,
+	SC_R_M4_1_SIM = 310,
+	SC_R_M4_1_WDOG = 311,
+	SC_R_M4_1_MU_0B = 312,
+	SC_R_M4_1_MU_0A0 = 313,
+	SC_R_M4_1_MU_0A1 = 314,
+	SC_R_M4_1_MU_0A2 = 315,
+	SC_R_M4_1_MU_0A3 = 316,
+	SC_R_M4_1_MU_1A = 317,
+	SC_R_SAI_0 = 318,
+	SC_R_SAI_1 = 319,
+	SC_R_SAI_2 = 320,
+	SC_R_IRQSTR_SCU2 = 321,
+	SC_R_IRQSTR_DSP = 322,
+	SC_R_UNUSED5 = 323,
+	SC_R_UNUSED6 = 324,
+	SC_R_AUDIO_PLL_0 = 325,
+	SC_R_PI_0 = 326,
+	SC_R_PI_0_PWM_0 = 327,
+	SC_R_PI_0_PWM_1 = 328,
+	SC_R_PI_0_I2C_0 = 329,
+	SC_R_PI_0_PLL = 330,
+	SC_R_PI_1 = 331,
+	SC_R_PI_1_PWM_0 = 332,
+	SC_R_PI_1_PWM_1 = 333,
+	SC_R_PI_1_I2C_0 = 334,
+	SC_R_PI_1_PLL = 335,
+	SC_R_SC_PID0 = 336,
+	SC_R_SC_PID1 = 337,
+	SC_R_SC_PID2 = 338,
+	SC_R_SC_PID3 = 339,
+	SC_R_SC_PID4 = 340,
+	SC_R_SC_SEMA42 = 341,
+	SC_R_SC_TPM = 342,
+	SC_R_SC_PIT = 343,
+	SC_R_SC_UART = 344,
+	SC_R_SC_I2C = 345,
+	SC_R_SC_MU_0B = 346,
+	SC_R_SC_MU_0A0 = 347,
+	SC_R_SC_MU_0A1 = 348,
+	SC_R_SC_MU_0A2 = 349,
+	SC_R_SC_MU_0A3 = 350,
+	SC_R_SC_MU_1A = 351,
+	SC_R_SYSCNT_RD = 352,
+	SC_R_SYSCNT_CMP = 353,
+	SC_R_DEBUG = 354,
+	SC_R_SYSTEM = 355,
+	SC_R_SNVS = 356,
+	SC_R_OTP = 357,
+	SC_R_VPU_PID0 = 358,
+	SC_R_VPU_PID1 = 359,
+	SC_R_VPU_PID2 = 360,
+	SC_R_VPU_PID3 = 361,
+	SC_R_VPU_PID4 = 362,
+	SC_R_VPU_PID5 = 363,
+	SC_R_VPU_PID6 = 364,
+	SC_R_VPU_PID7 = 365,
+	SC_R_VPU_UART = 366,
+	SC_R_VPUCORE = 367,
+	SC_R_VPUCORE_0 = 368,
+	SC_R_VPUCORE_1 = 369,
+	SC_R_VPUCORE_2 = 370,
+	SC_R_VPUCORE_3 = 371,
+	SC_R_DMA_4_CH0 = 372,
+	SC_R_DMA_4_CH1 = 373,
+	SC_R_DMA_4_CH2 = 374,
+	SC_R_DMA_4_CH3 = 375,
+	SC_R_DMA_4_CH4 = 376,
+	SC_R_ISI_CH0 = 377,
+	SC_R_ISI_CH1 = 378,
+	SC_R_ISI_CH2 = 379,
+	SC_R_ISI_CH3 = 380,
+	SC_R_ISI_CH4 = 381,
+	SC_R_ISI_CH5 = 382,
+	SC_R_ISI_CH6 = 383,
+	SC_R_ISI_CH7 = 384,
+	SC_R_MJPEG_DEC_S0 = 385,
+	SC_R_MJPEG_DEC_S1 = 386,
+	SC_R_MJPEG_DEC_S2 = 387,
+	SC_R_MJPEG_DEC_S3 = 388,
+	SC_R_MJPEG_ENC_S0 = 389,
+	SC_R_MJPEG_ENC_S1 = 390,
+	SC_R_MJPEG_ENC_S2 = 391,
+	SC_R_MJPEG_ENC_S3 = 392,
+	SC_R_MIPI_0 = 393,
+	SC_R_MIPI_0_PWM_0 = 394,
+	SC_R_MIPI_0_I2C_0 = 395,
+	SC_R_MIPI_0_I2C_1 = 396,
+	SC_R_MIPI_1 = 397,
+	SC_R_MIPI_1_PWM_0 = 398,
+	SC_R_MIPI_1_I2C_0 = 399,
+	SC_R_MIPI_1_I2C_1 = 400,
+	SC_R_CSI_0 = 401,
+	SC_R_CSI_0_PWM_0 = 402,
+	SC_R_CSI_0_I2C_0 = 403,
+	SC_R_CSI_1 = 404,
+	SC_R_CSI_1_PWM_0 = 405,
+	SC_R_CSI_1_I2C_0 = 406,
+	SC_R_HDMI = 407,
+	SC_R_HDMI_I2S = 408,
+	SC_R_HDMI_I2C_0 = 409,
+	SC_R_HDMI_PLL_0 = 410,
+	SC_R_HDMI_RX = 411,
+	SC_R_HDMI_RX_BYPASS = 412,
+	SC_R_HDMI_RX_I2C_0 = 413,
+	SC_R_ASRC_0 = 414,
+	SC_R_ESAI_0 = 415,
+	SC_R_SPDIF_0 = 416,
+	SC_R_SPDIF_1 = 417,
+	SC_R_SAI_3 = 418,
+	SC_R_SAI_4 = 419,
+	SC_R_SAI_5 = 420,
+	SC_R_GPT_5 = 421,
+	SC_R_GPT_6 = 422,
+	SC_R_GPT_7 = 423,
+	SC_R_GPT_8 = 424,
+	SC_R_GPT_9 = 425,
+	SC_R_GPT_10 = 426,
+	SC_R_DMA_2_CH5 = 427,
+	SC_R_DMA_2_CH6 = 428,
+	SC_R_DMA_2_CH7 = 429,
+	SC_R_DMA_2_CH8 = 430,
+	SC_R_DMA_2_CH9 = 431,
+	SC_R_DMA_2_CH10 = 432,
+	SC_R_DMA_2_CH11 = 433,
+	SC_R_DMA_2_CH12 = 434,
+	SC_R_DMA_2_CH13 = 435,
+	SC_R_DMA_2_CH14 = 436,
+	SC_R_DMA_2_CH15 = 437,
+	SC_R_DMA_2_CH16 = 438,
+	SC_R_DMA_2_CH17 = 439,
+	SC_R_DMA_2_CH18 = 440,
+	SC_R_DMA_2_CH19 = 441,
+	SC_R_DMA_2_CH20 = 442,
+	SC_R_DMA_2_CH21 = 443,
+	SC_R_DMA_2_CH22 = 444,
+	SC_R_DMA_2_CH23 = 445,
+	SC_R_DMA_2_CH24 = 446,
+	SC_R_DMA_2_CH25 = 447,
+	SC_R_DMA_2_CH26 = 448,
+	SC_R_DMA_2_CH27 = 449,
+	SC_R_DMA_2_CH28 = 450,
+	SC_R_DMA_2_CH29 = 451,
+	SC_R_DMA_2_CH30 = 452,
+	SC_R_DMA_2_CH31 = 453,
+	SC_R_ASRC_1 = 454,
+	SC_R_ESAI_1 = 455,
+	SC_R_SAI_6 = 456,
+	SC_R_SAI_7 = 457,
+	SC_R_AMIX = 458,
+	SC_R_MQS_0 = 459,
+	SC_R_DMA_3_CH0 = 460,
+	SC_R_DMA_3_CH1 = 461,
+	SC_R_DMA_3_CH2 = 462,
+	SC_R_DMA_3_CH3 = 463,
+	SC_R_DMA_3_CH4 = 464,
+	SC_R_DMA_3_CH5 = 465,
+	SC_R_DMA_3_CH6 = 466,
+	SC_R_DMA_3_CH7 = 467,
+	SC_R_DMA_3_CH8 = 468,
+	SC_R_DMA_3_CH9 = 469,
+	SC_R_DMA_3_CH10 = 470,
+	SC_R_DMA_3_CH11 = 471,
+	SC_R_DMA_3_CH12 = 472,
+	SC_R_DMA_3_CH13 = 473,
+	SC_R_DMA_3_CH14 = 474,
+	SC_R_DMA_3_CH15 = 475,
+	SC_R_DMA_3_CH16 = 476,
+	SC_R_DMA_3_CH17 = 477,
+	SC_R_DMA_3_CH18 = 478,
+	SC_R_DMA_3_CH19 = 479,
+	SC_R_DMA_3_CH20 = 480,
+	SC_R_DMA_3_CH21 = 481,
+	SC_R_DMA_3_CH22 = 482,
+	SC_R_DMA_3_CH23 = 483,
+	SC_R_DMA_3_CH24 = 484,
+	SC_R_DMA_3_CH25 = 485,
+	SC_R_DMA_3_CH26 = 486,
+	SC_R_DMA_3_CH27 = 487,
+	SC_R_DMA_3_CH28 = 488,
+	SC_R_DMA_3_CH29 = 489,
+	SC_R_DMA_3_CH30 = 490,
+	SC_R_DMA_3_CH31 = 491,
+	SC_R_AUDIO_PLL_1 = 492,
+	SC_R_AUDIO_CLK_0 = 493,
+	SC_R_AUDIO_CLK_1 = 494,
+	SC_R_MCLK_OUT_0 = 495,
+	SC_R_MCLK_OUT_1 = 496,
+	SC_R_PMIC_0 = 497,
+	SC_R_PMIC_1 = 498,
+	SC_R_SECO = 499,
+	SC_R_CAAM_JR1 = 500,
+	SC_R_CAAM_JR2 = 501,
+	SC_R_CAAM_JR3 = 502,
+	SC_R_SECO_MU_2 = 503,
+	SC_R_SECO_MU_3 = 504,
+	SC_R_SECO_MU_4 = 505,
+	SC_R_HDMI_RX_PWM_0 = 506,
+	SC_R_A35 = 507,
+	SC_R_A35_0 = 508,
+	SC_R_A35_1 = 509,
+	SC_R_A35_2 = 510,
+	SC_R_A35_3 = 511,
+	SC_R_DSP = 512,
+	SC_R_DSP_RAM = 513,
+	SC_R_CAAM_JR1_OUT = 514,
+	SC_R_CAAM_JR2_OUT = 515,
+	SC_R_CAAM_JR3_OUT = 516,
+	SC_R_VPU_DEC_0 = 517,
+	SC_R_VPU_ENC_0 = 518,
+	SC_R_CAAM_JR0 = 519,
+	SC_R_CAAM_JR0_OUT = 520,
+	SC_R_PMIC_2 = 521,
+	SC_R_DBLOGIC = 522,
+	SC_R_HDMI_PLL_1 = 523,
+	SC_R_BOARD_R0 = 524,
+	SC_R_BOARD_R1 = 525,
+	SC_R_BOARD_R2 = 526,
+	SC_R_BOARD_R3 = 527,
+	SC_R_BOARD_R4 = 528,
+	SC_R_BOARD_R5 = 529,
+	SC_R_BOARD_R6 = 530,
+	SC_R_BOARD_R7 = 531,
+	SC_R_MJPEG_DEC_MP = 532,
+	SC_R_MJPEG_ENC_MP = 533,
+	SC_R_VPU_TS_0 = 534,
+	SC_R_VPU_MU_0 = 535,
+	SC_R_VPU_MU_1 = 536,
+	SC_R_VPU_MU_2 = 537,
+	SC_R_VPU_MU_3 = 538,
+	SC_R_VPU_ENC_1 = 539,
+	SC_R_VPU = 540,
+	SC_R_LAST
+} sc_rsrc_t;
+
+/* NOTE - please add by replacing some of the UNUSED from above! */
+
+/*
+ * This type is used to indicate a control.
+ */
+typedef enum sc_ctrl_e {
+
+	SC_C_TEMP = 0,
+	SC_C_TEMP_HI = 1,
+	SC_C_TEMP_LOW = 2,
+	SC_C_PXL_LINK_MST1_ADDR = 3,
+	SC_C_PXL_LINK_MST2_ADDR = 4,
+	SC_C_PXL_LINK_MST_ENB = 5,
+	SC_C_PXL_LINK_MST1_ENB = 6,
+	SC_C_PXL_LINK_MST2_ENB = 7,
+	SC_C_PXL_LINK_SLV1_ADDR = 8,
+	SC_C_PXL_LINK_SLV2_ADDR = 9,
+	SC_C_PXL_LINK_MST_VLD = 10,
+	SC_C_PXL_LINK_MST1_VLD = 11,
+	SC_C_PXL_LINK_MST2_VLD = 12,
+	SC_C_SINGLE_MODE = 13,
+	SC_C_ID = 14,
+	SC_C_PXL_CLK_POLARITY = 15,
+	SC_C_LINESTATE = 16,
+	SC_C_PCIE_G_RST = 17,
+	SC_C_PCIE_BUTTON_RST = 18,
+	SC_C_PCIE_PERST = 19,
+	SC_C_PHY_RESET = 20,
+	SC_C_PXL_LINK_RATE_CORRECTION = 21,
+	SC_C_PANIC = 22,
+	SC_C_PRIORITY_GROUP = 23,
+	SC_C_TXCLK = 24,
+	SC_C_CLKDIV = 25,
+	SC_C_DISABLE_50 = 26,
+	SC_C_DISABLE_125 = 27,
+	SC_C_SEL_125 = 28,
+	SC_C_MODE = 29,
+	SC_C_SYNC_CTRL0 = 30,
+	SC_C_KACHUNK_CNT = 31,
+	SC_C_KACHUNK_SEL = 32,
+	SC_C_SYNC_CTRL1 = 33,
+	SC_C_DPI_RESET = 34,
+	SC_C_MIPI_RESET = 35,
+	SC_C_DUAL_MODE = 36,
+	SC_C_VOLTAGE = 37,
+	SC_C_PXL_LINK_SEL = 38,
+	SC_C_OFS_SEL = 39,
+	SC_C_OFS_AUDIO = 40,
+	SC_C_OFS_PERIPH = 41,
+	SC_C_OFS_IRQ = 42,
+	SC_C_RST0 = 43,
+	SC_C_RST1 = 44,
+	SC_C_SEL0 = 45,
+	SC_C_LAST
+} sc_ctrl_t;
+
+/*
+ * This type is used to indicate a pad. Valid values are SoC specific.
+ *
+ * Refer to the SoC [Pad List](@ref PADS) for valid pad values.
+ */
+typedef uint16_t sc_pad_t;
+
+#endif /* _SC_TYPES_H */
-- 
2.7.4

[PATCH V4 5/5] soc: imx: add SC firmware IPC and APIs

[Sascha Hauer]

Hi,

Thank you for removing the majority of this patch. It really helps
readability.

On Sun, Jul 08, 2018 at 10:56:57PM +0800, Dong Aisheng wrote:
> +/* Initialization of the MU code. */
> +static int imx_sc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ret = sc_ipc_open(&scu_ipc_handle, dev);
> +	if (ret)
> +		return ret;
> +
> +	pr_info("NXP i.MX SCU Initialized\n");
> +
> +	return devm_of_platform_populate(dev);
> +}

I see that you have turned the SCU code into a driver and I really
welcome that. Now when you have a driver a global scu_ipc_handle is no
longer necessary. You can call platform_set_drvdata(pdev, sc_ipc); and
can get that from the child devices using
dev_get_drvdata(pdev->dev.parent) like for example most mfd devices do.


> +	{ /* Sentinel */ }
> +};
> +
> +static struct platform_driver imx_sc_driver = {
> +	.driver = {
> +		.name = "imx-scu",
> +		.of_match_table = imx_sc_match,
> +	},
> +	.probe = imx_sc_probe,
> +};
> +
> +static int __init imx_sc_init(void)
> +{
> +	return platform_driver_register(&imx_sc_driver);
> +}
> +core_initcall(imx_sc_init);

...

> +
> +#define RPC_VER(MSG)            ((MSG)->version)
> +#define RPC_SIZE(MSG)           ((MSG)->size)
> +#define RPC_SVC(MSG)            ((MSG)->svc)
> +#define RPC_FUNC(MSG)           ((MSG)->func)
> +#define RPC_R8(MSG)             ((MSG)->func)
> +#define RPC_I32(MSG, IDX)       ((MSG)->DATA.i32[(IDX) / 4])
> +#define RPC_I16(MSG, IDX)       ((MSG)->DATA.i16[(IDX) / 2])
> +#define RPC_I8(MSG, IDX)        ((MSG)->DATA.i8[(IDX)])
> +#define RPC_U32(MSG, IDX)       ((MSG)->DATA.u32[(IDX) / 4])
> +#define RPC_U16(MSG, IDX)       ((MSG)->DATA.u16[(IDX) / 2])
> +#define RPC_U8(MSG, IDX)        ((MSG)->DATA.u8[(IDX)])

Same as I said more than once: These macros only reduce readability of
the code. Please drop them.

> +typedef struct sc_rpc_msg_s {
> +	uint8_t version;
> +	uint8_t size;
> +	uint8_t svc;
> +	uint8_t func;
> +	union {
> +		int32_t i32[(SC_RPC_MAX_MSG - 1)];
> +		int16_t i16[(SC_RPC_MAX_MSG - 1) * 2];
> +		int8_t i8[(SC_RPC_MAX_MSG - 1) * 4];
> +		uint32_t u32[(SC_RPC_MAX_MSG - 1)];
> +		uint16_t u16[(SC_RPC_MAX_MSG - 1) * 2];
> +		uint8_t u8[(SC_RPC_MAX_MSG - 1) * 4];
> +	} DATA;
> +} sc_rpc_msg_t;

I am pretty sure now that you should only define a common header struct,
like:

struct sc_rpc_msg {
	uint8_t version;
	uint8_t size;
	uint8_t svc;
	uint8_t func;
};

This can be embedded into structs that the consumers (like clock driver)
use.

> +
> +/*
> + * @name Defines for common frequencies
> + */
> +#define SC_32KHZ            32768	/* 32KHz */
> +#define SC_10MHZ         10000000	/* 10MHz */
> +#define SC_20MHZ         20000000	/* 20MHz */
> +#define SC_25MHZ         25000000	/* 25MHz */

What is the usecase for these defines? Can we add them along with the
users of the defines so that we can decide then if we really want to
have them?

Regards
 Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH V4 5/5] soc: imx: add SC firmware IPC and APIs

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Tuesday, July 10, 2018 10:45 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; dl-linux-imx
> <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo at kernel.org
> Subject: Re: [PATCH V4 5/5] soc: imx: add SC firmware IPC and APIs
> 
> Hi,
> 
> Thank you for removing the majority of this patch. It really helps readability.
> 
> On Sun, Jul 08, 2018 at 10:56:57PM +0800, Dong Aisheng wrote:
> > +/* Initialization of the MU code. */
> > +static int imx_sc_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	int ret;
> > +
> > +	ret = sc_ipc_open(&scu_ipc_handle, dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pr_info("NXP i.MX SCU Initialized\n");
> > +
> > +	return devm_of_platform_populate(dev); }
> 
> I see that you have turned the SCU code into a driver and I really welcome
> that. Now when you have a driver a global scu_ipc_handle is no longer
> necessary. You can call platform_set_drvdata(pdev, sc_ipc); and can get that
> from the child devices using
> dev_get_drvdata(pdev->dev.parent) like for example most mfd devices do.
> 

It looks like a good idea.
One question: That means that the SCU users much have a node under SCU node
and have struct device point, it seems ok for all normal devices like clk, pinctrl, pd.
But I found some special device like soc device which does not have node and struct
device, but also use SCU. How to treat for those devices? Possible anymore of them?

> 
> > +	{ /* Sentinel */ }
> > +};
> > +
> > +static struct platform_driver imx_sc_driver = {
> > +	.driver = {
> > +		.name = "imx-scu",
> > +		.of_match_table = imx_sc_match,
> > +	},
> > +	.probe = imx_sc_probe,
> > +};
> > +
> > +static int __init imx_sc_init(void)
> > +{
> > +	return platform_driver_register(&imx_sc_driver);
> > +}
> > +core_initcall(imx_sc_init);
> 
> ...
> 
> > +
> > +#define RPC_VER(MSG)            ((MSG)->version)
> > +#define RPC_SIZE(MSG)           ((MSG)->size)
> > +#define RPC_SVC(MSG)            ((MSG)->svc)
> > +#define RPC_FUNC(MSG)           ((MSG)->func)
> > +#define RPC_R8(MSG)             ((MSG)->func)
> > +#define RPC_I32(MSG, IDX)       ((MSG)->DATA.i32[(IDX) / 4])
> > +#define RPC_I16(MSG, IDX)       ((MSG)->DATA.i16[(IDX) / 2])
> > +#define RPC_I8(MSG, IDX)        ((MSG)->DATA.i8[(IDX)])
> > +#define RPC_U32(MSG, IDX)       ((MSG)->DATA.u32[(IDX) / 4])
> > +#define RPC_U16(MSG, IDX)       ((MSG)->DATA.u16[(IDX) / 2])
> > +#define RPC_U8(MSG, IDX)        ((MSG)->DATA.u8[(IDX)])
> 
> Same as I said more than once: These macros only reduce readability of the
> code. Please drop them.
> 
> > +typedef struct sc_rpc_msg_s {
> > +	uint8_t version;
> > +	uint8_t size;
> > +	uint8_t svc;
> > +	uint8_t func;
> > +	union {
> > +		int32_t i32[(SC_RPC_MAX_MSG - 1)];
> > +		int16_t i16[(SC_RPC_MAX_MSG - 1) * 2];
> > +		int8_t i8[(SC_RPC_MAX_MSG - 1) * 4];
> > +		uint32_t u32[(SC_RPC_MAX_MSG - 1)];
> > +		uint16_t u16[(SC_RPC_MAX_MSG - 1) * 2];
> > +		uint8_t u8[(SC_RPC_MAX_MSG - 1) * 4];
> > +	} DATA;
> > +} sc_rpc_msg_t;
> 
> I am pretty sure now that you should only define a common header struct,
> like:
> 
> struct sc_rpc_msg {
> 	uint8_t version;
> 	uint8_t size;
> 	uint8_t svc;
> 	uint8_t func;
> };
> 
> This can be embedded into structs that the consumers (like clock driver) use.
> 

Did you mean something like below (similar as TI does):

struct sc_rpc_msg {
        uint8_t ver;
        uint8_t size; 
        uint8_t svc;
        uint8_t func; 
};

// we need have two structures for each type SCU request and response       
struct imx_sc_msg_req_get_clock_rate {
        struct sc_rpc_msg hdr;
        u16 resource;
        u8 clk;
} __packed;

struct imx_sc_msg_resp_get_clock_rate {
        struct sc_rpc_msg hdr;
        u32 rate;
} __packed;

sc_err_t sc_pm_get_clock_rate(sc_ipc_t ipc, sc_rsrc_t resource,
                              sc_pm_clk_t clk, sc_pm_clock_rate_t *rate)
{
        struct imx_sc_msg_req_get_clock_rate *msg;
        struct imx_sc_msg_resp_get_clock_rate *resp;
        struct sc_rpc_msg *hdr = &msg->hdr;
        uint8_t result;
        int ret;

        hdr->ver = SC_RPC_VERSION;
        hdr->svc = (uint8_t)SC_RPC_SVC_PM;
        hdr->func = (uint8_t)PM_FUNC_GET_CLOCK_RATE;
        hdr->size = 2;

        msg->resource = resource;
        msg->clk = clk;

        ret = sc_call_rpc(ipc, &msg, false);
        if (ret)
                return SC_ERR_FAIL;

        resp = (struct imx_sc_msg_resp_get_clock_rate *)msg;
        if (rate != NULL)
                *rate = resp->rate;

        result = resp->hdr.func;
        return (sc_err_t)result;
}

Are you okay with this?

> > +
> > +/*
> > + * @name Defines for common frequencies  */
> > +#define SC_32KHZ            32768	/* 32KHz */
> > +#define SC_10MHZ         10000000	/* 10MHz */
> > +#define SC_20MHZ         20000000	/* 20MHz */
> > +#define SC_25MHZ         25000000	/* 25MHz */
> 
> What is the usecase for these defines? Can we add them along with the
> users of the defines so that we can decide then if we really want to have
> them?

Okay, I could remove them from this patch.

Regards
Dong Aisheng

> 
> Regards
>  Sascha
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.pengutronix.de%2F&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7
> C8f4321d863924a5d649b08d5e673b523%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C636668306989011249&amp;sdata=pG94K1r%2F%2Feyng%2
> F1reGHfO0TEYLNJKtYTa5y%2FZqNfJjc%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH V4 5/5] soc: imx: add SC firmware IPC and APIs

[Sascha Hauer]

On Wed, Jul 11, 2018 at 09:18:06AM +0000, A.s. Dong wrote:
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Tuesday, July 10, 2018 10:45 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; dl-linux-imx
> > <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
> > <fabio.estevam@nxp.com>; shawnguo at kernel.org
> > Subject: Re: [PATCH V4 5/5] soc: imx: add SC firmware IPC and APIs
> > 
> > Hi,
> > 
> > Thank you for removing the majority of this patch. It really helps readability.
> > 
> > On Sun, Jul 08, 2018 at 10:56:57PM +0800, Dong Aisheng wrote:
> > > +/* Initialization of the MU code. */
> > > +static int imx_sc_probe(struct platform_device *pdev) {
> > > +	struct device *dev = &pdev->dev;
> > > +	int ret;
> > > +
> > > +	ret = sc_ipc_open(&scu_ipc_handle, dev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	pr_info("NXP i.MX SCU Initialized\n");
> > > +
> > > +	return devm_of_platform_populate(dev); }
> > 
> > I see that you have turned the SCU code into a driver and I really welcome
> > that. Now when you have a driver a global scu_ipc_handle is no longer
> > necessary. You can call platform_set_drvdata(pdev, sc_ipc); and can get that
> > from the child devices using
> > dev_get_drvdata(pdev->dev.parent) like for example most mfd devices do.
> > 
> 
> It looks like a good idea.
> One question: That means that the SCU users much have a node under SCU node
> and have struct device point, it seems ok for all normal devices like clk, pinctrl, pd.
> But I found some special device like soc device which does not have node and struct
> device, but also use SCU. How to treat for those devices? Possible anymore of them?

Not sure. What kind of device is it? I'm sure we'll find a solution so
maybe ask again once we are there?

> > This can be embedded into structs that the consumers (like clock driver) use.
> > 
> 
> Did you mean something like below (similar as TI does):
> 
> struct sc_rpc_msg {
>         uint8_t ver;
>         uint8_t size; 
>         uint8_t svc;
>         uint8_t func; 
> };
> 
> // we need have two structures for each type SCU request and response       
> struct imx_sc_msg_req_get_clock_rate {
>         struct sc_rpc_msg hdr;
>         u16 resource;
>         u8 clk;
> } __packed;
> 
> struct imx_sc_msg_resp_get_clock_rate {
>         struct sc_rpc_msg hdr;
>         u32 rate;
> } __packed;
> 
> sc_err_t sc_pm_get_clock_rate(sc_ipc_t ipc, sc_rsrc_t resource,
>                               sc_pm_clk_t clk, sc_pm_clock_rate_t *rate)
> {
>         struct imx_sc_msg_req_get_clock_rate *msg;
>         struct imx_sc_msg_resp_get_clock_rate *resp;
>         struct sc_rpc_msg *hdr = &msg->hdr;
>         uint8_t result;
>         int ret;
> 
>         hdr->ver = SC_RPC_VERSION;
>         hdr->svc = (uint8_t)SC_RPC_SVC_PM;
>         hdr->func = (uint8_t)PM_FUNC_GET_CLOCK_RATE;
>         hdr->size = 2;
> 
>         msg->resource = resource;
>         msg->clk = clk;
> 
>         ret = sc_call_rpc(ipc, &msg, false);
>         if (ret)
>                 return SC_ERR_FAIL;
> 
>         resp = (struct imx_sc_msg_resp_get_clock_rate *)msg;
>         if (rate != NULL)
>                 *rate = resp->rate;
> 
>         result = resp->hdr.func;
>         return (sc_err_t)result;
> }
> 
> Are you okay with this?

Yes, that's what I meant. Put this function into the clock driver
directly rather than in the SCU code and we are done.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH V4 5/5] soc: imx: add SC firmware IPC and APIs

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Wednesday, July 11, 2018 6:32 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; dl-linux-imx
> <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo at kernel.org
> Subject: Re: [PATCH V4 5/5] soc: imx: add SC firmware IPC and APIs
> 
> On Wed, Jul 11, 2018 at 09:18:06AM +0000, A.s. Dong wrote:
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > Sent: Tuesday, July 10, 2018 10:45 PM
> > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com;
> > > dl-linux-imx <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio
> > > Estevam <fabio.estevam@nxp.com>; shawnguo at kernel.org
> > > Subject: Re: [PATCH V4 5/5] soc: imx: add SC firmware IPC and APIs
> > >
> > > Hi,
> > >
> > > Thank you for removing the majority of this patch. It really helps
> readability.
> > >
> > > On Sun, Jul 08, 2018 at 10:56:57PM +0800, Dong Aisheng wrote:
> > > > +/* Initialization of the MU code. */ static int
> > > > +imx_sc_probe(struct platform_device *pdev) {
> > > > +	struct device *dev = &pdev->dev;
> > > > +	int ret;
> > > > +
> > > > +	ret = sc_ipc_open(&scu_ipc_handle, dev);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	pr_info("NXP i.MX SCU Initialized\n");
> > > > +
> > > > +	return devm_of_platform_populate(dev); }
> > >
> > > I see that you have turned the SCU code into a driver and I really
> > > welcome that. Now when you have a driver a global scu_ipc_handle is
> > > no longer necessary. You can call platform_set_drvdata(pdev,
> > > sc_ipc); and can get that from the child devices using
> > > dev_get_drvdata(pdev->dev.parent) like for example most mfd devices
> do.
> > >
> >
> > It looks like a good idea.
> > One question: That means that the SCU users much have a node under
> SCU
> > node and have struct device point, it seems ok for all normal devices like clk,
> pinctrl, pd.
> > But I found some special device like soc device which does not have
> > node and struct device, but also use SCU. How to treat for those devices?
> Possible anymore of them?
> 
> Not sure. What kind of device is it? I'm sure we'll find a solution so maybe ask
> again once we are there?
> 

See:
include/linux/sys_soc.h
arch/arm/mach-imx/cpu.c: imx_soc_device_init().
struct soc_device_attribute {
        const char *machine;
        const char *family;
        const char *revision;   
        const char *soc_id;
        const void *data;
};

And I found there's also SC services used by other normal devices
Which is not under SCU node. So I wonder dev_get_drvdata(parent)
may not work for them quite well.

> > > This can be embedded into structs that the consumers (like clock driver)
> use.
> > >
> >
> > Did you mean something like below (similar as TI does):
> >
> > struct sc_rpc_msg {
> >         uint8_t ver;
> >         uint8_t size;
> >         uint8_t svc;
> >         uint8_t func;
> > };
> >
> > // we need have two structures for each type SCU request and response
> > struct imx_sc_msg_req_get_clock_rate {
> >         struct sc_rpc_msg hdr;
> >         u16 resource;
> >         u8 clk;
> > } __packed;
> >
> > struct imx_sc_msg_resp_get_clock_rate {
> >         struct sc_rpc_msg hdr;
> >         u32 rate;
> > } __packed;
> >
> > sc_err_t sc_pm_get_clock_rate(sc_ipc_t ipc, sc_rsrc_t resource,
> >                               sc_pm_clk_t clk, sc_pm_clock_rate_t
> > *rate) {
> >         struct imx_sc_msg_req_get_clock_rate *msg;
> >         struct imx_sc_msg_resp_get_clock_rate *resp;
> >         struct sc_rpc_msg *hdr = &msg->hdr;
> >         uint8_t result;
> >         int ret;
> >
> >         hdr->ver = SC_RPC_VERSION;
> >         hdr->svc = (uint8_t)SC_RPC_SVC_PM;
> >         hdr->func = (uint8_t)PM_FUNC_GET_CLOCK_RATE;
> >         hdr->size = 2;
> >
> >         msg->resource = resource;
> >         msg->clk = clk;
> >
> >         ret = sc_call_rpc(ipc, &msg, false);
> >         if (ret)
> >                 return SC_ERR_FAIL;
> >
> >         resp = (struct imx_sc_msg_resp_get_clock_rate *)msg;
> >         if (rate != NULL)
> >                 *rate = resp->rate;
> >
> >         result = resp->hdr.func;
> >         return (sc_err_t)result;
> > }
> >
> > Are you okay with this?
> 
> Yes, that's what I meant. Put this function into the clock driver directly rather
> than in the SCU code and we are done.
> 

It seems those are procotol services,  both arm scpi and ti sci
Implemented them in scu driver.
drivers/firmware/ti_sci.c
drivers/firmware/arm_scpi.c
You do want to implement in each induvial client drivers?

Regards
Dong Aisheng

> Sascha
> 
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.pengutronix.de%2F&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7
> C4d1cdc88f66c4c6e8b2c08d5e7198a09%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C636669019236875606&amp;sdata=lpP6lpObOOKajQFz9mTai
> BbqLklnoLbWGrMzPxhrjpU%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH V4 0/5] soc: imx: add scu firmware api support

[Sudeep Holla]

On 08/07/18 15:56, Dong Aisheng wrote:
> Unlike the former i.MX Architectures, the new generation i.MX8 SoCs
> (e.g. MX8QXP and MX8QM) contain a system controller which runs on a
> dedicated Cortex-M core to provide power, clock, Pad, and resource
> management. Communication between the host processor running
> an OS and the system controller happens through a SCU protocol.
> This patchset adds the SCU APIs which is implemented based on MU
> and will be used by different system components.
> 
> It mainly consists of below parts:
> 1) MU mailbox driver
> 1) Implementation of the IPC functions based on MUs (client side).
> 
> 

I do understand the firmware might be already released for the current
generation of platforms, but instead of adding to the fragmentation, it
would be good to migrated to ARM SCMI for future platforms.

This system controller communication protocol is already so fragmented
(we have TI SCI, QCOM RPM, Nvidia Tegra BPMP, ARM SCPI and so on..
and now this i.MX specific one added to that), the main intention of ARM
SCMI is to stop that. The idea is for any Power Control System
Architecture(PCSA) compliant systems or any similar systems, SCMI should
be the standard communication protocol.

-- 
Regards,
Sudeep

[PATCH V4 0/5] soc: imx: add scu firmware api support

[A.s. Dong]

Hi Sudeep,

> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla at arm.com]
> Sent: Thursday, July 12, 2018 12:49 AM
> To: A.s. Dong <aisheng.dong@nxp.com>; linux-arm-
> kernel at lists.infradead.org
> Cc: Sudeep Holla <sudeep.holla@arm.com>; dongas86 at gmail.com; dl-linux-
> imx <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo at kernel.org; Cl?ment Faure
> <clement.faure@nxp.com>
> Subject: Re: [PATCH V4 0/5] soc: imx: add scu firmware api support
> 
> On 08/07/18 15:56, Dong Aisheng wrote:
> > Unlike the former i.MX Architectures, the new generation i.MX8 SoCs
> > (e.g. MX8QXP and MX8QM) contain a system controller which runs on a
> > dedicated Cortex-M core to provide power, clock, Pad, and resource
> > management. Communication between the host processor running an OS
> and
> > the system controller happens through a SCU protocol.
> > This patchset adds the SCU APIs which is implemented based on MU and
> > will be used by different system components.
> >
> > It mainly consists of below parts:
> > 1) MU mailbox driver
> > 1) Implementation of the IPC functions based on MUs (client side).
> >
> >
> 
> I do understand the firmware might be already released for the current
> generation of platforms, but instead of adding to the fragmentation, it would
> be good to migrated to ARM SCMI for future platforms.
> 
> This system controller communication protocol is already so fragmented (we
> have TI SCI, QCOM RPM, Nvidia Tegra BPMP, ARM SCPI and so on..
> and now this i.MX specific one added to that), the main intention of ARM
> SCMI is to stop that. The idea is for any Power Control System
> Architecture(PCSA) compliant systems or any similar systems, SCMI should
> be the standard communication protocol.

That sounds good and worth a look.
Copy SCU firmware guys if more comments.
BTW, i.MX SCU provides more services than just power related. 
Not sure if ARM SCMI can cover it.

Regards
Dong Aisheng

> 
> --
> Regards,
> Sudeep

[PATCH V4 0/5] soc: imx: add scu firmware api support

[Sudeep Holla]

On 12/07/18 04:46, A.s. Dong wrote:
> Hi Sudeep,
> 
>> -----Original Message-----
>> From: Sudeep Holla [mailto:sudeep.holla at arm.com]
>> Sent: Thursday, July 12, 2018 12:49 AM
>> To: A.s. Dong <aisheng.dong@nxp.com>; linux-arm-
>> kernel at lists.infradead.org
>> Cc: Sudeep Holla <sudeep.holla@arm.com>; dongas86 at gmail.com; dl-linux-
>> imx <linux-imx@nxp.com>; kernel at pengutronix.de; Fabio Estevam
>> <fabio.estevam@nxp.com>; shawnguo at kernel.org; Cl?ment Faure
>> <clement.faure@nxp.com>
>> Subject: Re: [PATCH V4 0/5] soc: imx: add scu firmware api support
>>
>> On 08/07/18 15:56, Dong Aisheng wrote:
>>> Unlike the former i.MX Architectures, the new generation i.MX8 SoCs
>>> (e.g. MX8QXP and MX8QM) contain a system controller which runs on a
>>> dedicated Cortex-M core to provide power, clock, Pad, and resource
>>> management. Communication between the host processor running an OS
>> and
>>> the system controller happens through a SCU protocol.
>>> This patchset adds the SCU APIs which is implemented based on MU and
>>> will be used by different system components.
>>>
>>> It mainly consists of below parts:
>>> 1) MU mailbox driver
>>> 1) Implementation of the IPC functions based on MUs (client side).
>>>
>>>
>>
>> I do understand the firmware might be already released for the current
>> generation of platforms, but instead of adding to the fragmentation, it would
>> be good to migrated to ARM SCMI for future platforms.
>>
>> This system controller communication protocol is already so fragmented (we
>> have TI SCI, QCOM RPM, Nvidia Tegra BPMP, ARM SCPI and so on..
>> and now this i.MX specific one added to that), the main intention of ARM
>> SCMI is to stop that. The idea is for any Power Control System
>> Architecture(PCSA) compliant systems or any similar systems, SCMI should
>> be the standard communication protocol.
> 
> That sounds good and worth a look.
> Copy SCU firmware guys if more comments.
> BTW, i.MX SCU provides more services than just power related. 
> Not sure if ARM SCMI can cover it.
> 

Sure, we can extend the specification to cover those as generic
protocols. If they are too NXP specific, then SCMI has options to
use vendor specific protocols. I am interested to know the requirements.
-- 
Regards,
Sudeep

[PATCH V4 0/5] soc: imx: add scu firmware api support

[Leonard Crestez]

On Thu, 2018-07-12 at 11:32 +0100, Sudeep Holla wrote:
> On 12/07/18 04:46, A.s. Dong wrote:
> > Hi Sudeep,
> > > On 08/07/18 15:56, Dong Aisheng wrote:
> > > > Unlike the former i.MX Architectures, the new generation i.MX8 SoCs
> > > > (e.g. MX8QXP and MX8QM) contain a system controller which runs on a
> > > > dedicated Cortex-M core to provide power, clock, Pad, and resource
> > > > management. Communication between the host processor running an OS and
> > > > the system controller happens through a SCU protocol.
> > > > This patchset adds the SCU APIs which is implemented based on MU and
> > > > will be used by different system components.
> > > > 
> > > > It mainly consists of below parts:
> > > > 1) MU mailbox driver
> > > > 1) Implementation of the IPC functions based on MUs (client side).
> > > 
> > > I do understand the firmware might be already released for the current
> > > generation of platforms, but instead of adding to the fragmentation, it would
> > > be good to migrated to ARM SCMI for future platforms.
> > > 
> > > This system controller communication protocol is already so fragmented (we
> > > have TI SCI, QCOM RPM, Nvidia Tegra BPMP, ARM SCPI and so on..
> > > and now this i.MX specific one added to that), the main intention of ARM
> > > SCMI is to stop that. The idea is for any Power Control System
> > > Architecture(PCSA) compliant systems or any similar systems, SCMI should
> > > be the standard communication protocol.
> > 
> > That sounds good and worth a look.
> > Copy SCU firmware guys if more comments.
> > BTW, i.MX SCU provides more services than just power related. 
> > Not sure if ARM SCMI can cover it.
> 
> Sure, we can extend the specification to cover those as generic
> protocols. If they are too NXP specific, then SCMI has options to
> use vendor specific protocols. I am interested to know the requirements.

On major thing that is missing is pin control: on 8qm/8qx this is done
through the SCU but SCMI doesn't define a protocol for this. It would
be a big thing to implement as a vendor extension and is required for
basic functionality. Maybe SCMI will handle this in the future? If SCMI
eventually includes this imx might be asked to rewrite the vendor-
specific stuff anyway.

Another issue is that there doesn't seem to be any way to implement
clk_set_parent through SCMI (SC API does have this). This might be
intentional ("protocol does not cover discovery of clock dependencies,
which must be described through firmware tables instead"). But how
would the "assigned-clock-parents" DT property be implemented through
SCMI?

Even if SCMI doesn't define a clock tree it might be still have
something for explicit clk mux control?

One possible path to eventually using SCMI would be to define a SMC
"transport" and implement SCMI calls inside ATF on top of SC API. This
way ATF would hide clk implementation details in the same way that it
hides hotplug and suspend details. Does this make sense?