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

[PATCH 0/4] 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 library calls
1) Implementation of the IPC functions based on MUs (client side).
2) SCU firmware services APIs implementation ased on RPC calls which
   are mostly generated by SCU firmware

Dong Aisheng (4):
  soc: imx: add mu library functions support
  dt-bindings: arm: fsl: add mu binding doc
  dt-bindings: arm: fsl: add scu binding doc
  soc: imx: add SC firmware IPC and APIs

 .../devicetree/bindings/arm/freescale/fsl,mu.txt   |  33 +
 .../devicetree/bindings/arm/freescale/fsl,scu.txt  |  40 ++
 drivers/soc/imx/Kconfig                            |   7 +
 drivers/soc/imx/Makefile                           |   2 +
 drivers/soc/imx/imx_mu.c                           | 125 ++++
 drivers/soc/imx/sc/Makefile                        |   8 +
 drivers/soc/imx/sc/main/ipc.c                      | 270 ++++++++
 drivers/soc/imx/sc/main/rpc.h                      | 123 ++++
 drivers/soc/imx/sc/svc/irq/rpc.h                   |  41 ++
 drivers/soc/imx/sc/svc/irq/rpc_clnt.c              |  58 ++
 drivers/soc/imx/sc/svc/misc/rpc.h                  |  58 ++
 drivers/soc/imx/sc/svc/misc/rpc_clnt.c             | 368 ++++++++++
 drivers/soc/imx/sc/svc/pad/rpc.h                   |  55 ++
 drivers/soc/imx/sc/svc/pad/rpc_clnt.c              | 412 +++++++++++
 drivers/soc/imx/sc/svc/pm/rpc.h                    |  58 ++
 drivers/soc/imx/sc/svc/pm/rpc_clnt.c               | 393 +++++++++++
 drivers/soc/imx/sc/svc/rm/rpc.h                    |  70 ++
 drivers/soc/imx/sc/svc/rm/rpc_clnt.c               | 612 +++++++++++++++++
 drivers/soc/imx/sc/svc/timer/rpc.h                 |  52 ++
 drivers/soc/imx/sc/svc/timer/rpc_clnt.c            | 295 ++++++++
 include/soc/imx/mu.h                               |  21 +
 include/soc/imx/sc/ipc.h                           |  46 ++
 include/soc/imx/sc/scfw.h                          |  24 +
 include/soc/imx/sc/sci.h                           |  35 +
 include/soc/imx/sc/svc/irq/api.h                   | 139 ++++
 include/soc/imx/sc/svc/misc/api.h                  | 395 +++++++++++
 include/soc/imx/sc/svc/pad/api.h                   | 536 +++++++++++++++
 include/soc/imx/sc/svc/pm/api.h                    | 559 +++++++++++++++
 include/soc/imx/sc/svc/rm/api.h                    | 726 ++++++++++++++++++++
 include/soc/imx/sc/svc/timer/api.h                 | 265 +++++++
 include/soc/imx/sc/types.h                         | 764 +++++++++++++++++++++
 31 files changed, 6590 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
 create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
 create mode 100644 drivers/soc/imx/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 drivers/soc/imx/sc/svc/irq/rpc.h
 create mode 100644 drivers/soc/imx/sc/svc/irq/rpc_clnt.c
 create mode 100644 drivers/soc/imx/sc/svc/misc/rpc.h
 create mode 100644 drivers/soc/imx/sc/svc/misc/rpc_clnt.c
 create mode 100644 drivers/soc/imx/sc/svc/pad/rpc.h
 create mode 100644 drivers/soc/imx/sc/svc/pad/rpc_clnt.c
 create mode 100644 drivers/soc/imx/sc/svc/pm/rpc.h
 create mode 100644 drivers/soc/imx/sc/svc/pm/rpc_clnt.c
 create mode 100644 drivers/soc/imx/sc/svc/rm/rpc.h
 create mode 100644 drivers/soc/imx/sc/svc/rm/rpc_clnt.c
 create mode 100644 drivers/soc/imx/sc/svc/timer/rpc.h
 create mode 100644 drivers/soc/imx/sc/svc/timer/rpc_clnt.c
 create mode 100644 include/soc/imx/mu.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/sci.h
 create mode 100644 include/soc/imx/sc/svc/irq/api.h
 create mode 100644 include/soc/imx/sc/svc/misc/api.h
 create mode 100644 include/soc/imx/sc/svc/pad/api.h
 create mode 100644 include/soc/imx/sc/svc/pm/api.h
 create mode 100644 include/soc/imx/sc/svc/rm/api.h
 create mode 100644 include/soc/imx/sc/svc/timer/api.h
 create mode 100644 include/soc/imx/sc/types.h

-- 
2.7.4

[PATCH 1/4] soc: imx: add mu library functions support

[Dong Aisheng]

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

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>
---
 drivers/soc/imx/Kconfig  |   3 ++
 drivers/soc/imx/Makefile |   1 +
 drivers/soc/imx/imx_mu.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++
 include/soc/imx/mu.h     |  21 ++++++++
 4 files changed, 150 insertions(+)
 create mode 100644 drivers/soc/imx/imx_mu.c
 create mode 100644 include/soc/imx/mu.h

diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig
index a5b86a2..4858cd7 100644
--- a/drivers/soc/imx/Kconfig
+++ b/drivers/soc/imx/Kconfig
@@ -7,4 +7,7 @@ config IMX7_PM_DOMAINS
 	select PM_GENERIC_DOMAINS
 	default y if SOC_IMX7D
 
+config HAVE_IMX_MU
+	bool
+
 endmenu
diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
index aab41a5c..113dc7f 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_MU) += imx_mu.o
diff --git a/drivers/soc/imx/imx_mu.c b/drivers/soc/imx/imx_mu.c
new file mode 100644
index 0000000..e5f3f47
--- /dev/null
+++ b/drivers/soc/imx/imx_mu.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017~2018 NXP
+ *	Dong Aisheng <aisheng.dong@nxp.com>
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+
+#define MU_ATR0			0x0
+#define MU_ARR0			0x10
+#define MU_ASR			0x20
+#define MU_ACR			0x24
+
+#define MU_CR_GIEn_MASK		(0xf << 28)
+#define MU_CR_RIEn_MASK		(0xf << 24)
+#define MU_CR_TIEn_MASK		(0xf << 20)
+#define MU_CR_GIRn_MASK		(0xf << 16)
+#define MU_CR_NMI_MASK		(1 << 3)
+#define MU_CR_Fn_MASK		0x7
+
+#define MU_SR_TE0_MASK		BIT(23)
+#define MU_SR_RF0_MASK		BIT(27)
+
+#define MU_CR_RIE0_MASK		BIT(27)
+#define MU_CR_GIE0_MASK		BIT(31)
+
+/*
+ * This function sets the Flag n of the MU.
+ */
+int32_t mu_set_fn(void __iomem *base, uint32_t fn)
+{
+	uint32_t reg;
+
+	reg = fn & (~MU_CR_Fn_MASK);
+	if (reg > 0)
+		return -EINVAL;
+
+	reg = readl_relaxed(base + MU_ACR);
+	/*  Clear ABFn. */
+	reg &= ~MU_CR_Fn_MASK;
+	reg |= fn;
+	writel_relaxed(reg, base + MU_ACR);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mu_set_fn);
+
+/*
+ * This function reads the status from status register.
+ */
+uint32_t mu_read_status(void __iomem *base)
+{
+	return readl_relaxed(base + MU_ASR);
+}
+EXPORT_SYMBOL_GPL(mu_read_status);
+
+/*
+ * This function enables specific RX full interrupt.
+ */
+void mu_enable_rx_full_int(void __iomem *base, uint32_t index)
+{
+	uint32_t reg;
+
+	reg = readl_relaxed(base + MU_ACR);
+	reg &= ~(MU_CR_GIRn_MASK | MU_CR_NMI_MASK);
+	reg |= MU_CR_RIE0_MASK >> index;
+	writel_relaxed(reg, base + MU_ACR);
+}
+EXPORT_SYMBOL_GPL(mu_enable_rx_full_int);
+
+/*
+ * This function enables specific general purpose interrupt.
+ */
+void mu_enable_general_int(void __iomem *base, uint32_t index)
+{
+	uint32_t reg;
+
+	reg = readl_relaxed(base + MU_ACR);
+	reg &= ~(MU_CR_GIRn_MASK | MU_CR_NMI_MASK);
+	reg |= MU_CR_GIE0_MASK >> index;
+	writel_relaxed(reg, base + MU_ACR);
+}
+EXPORT_SYMBOL_GPL(mu_enable_general_int);
+
+/*
+ * Wait and send message to the other core.
+ */
+void mu_send_msg(void __iomem *base, uint32_t index, uint32_t msg)
+{
+	uint32_t mask = MU_SR_TE0_MASK >> index;
+
+	/* Wait TX register to be empty. */
+	while (!(readl_relaxed(base + MU_ASR) & mask))
+		;
+	writel_relaxed(msg, base + MU_ATR0  + (index * 4));
+}
+EXPORT_SYMBOL_GPL(mu_send_msg);
+
+/*
+ * Wait to receive message from the other core.
+ */
+void mu_receive_msg(void __iomem *base, uint32_t index, uint32_t *msg)
+{
+	uint32_t mask = MU_SR_RF0_MASK >> index;
+
+	/* Wait RX register to be full. */
+	while (!(readl_relaxed(base + MU_ASR) & mask))
+		;
+	*msg = readl_relaxed(base + MU_ARR0 + (index * 4));
+}
+EXPORT_SYMBOL_GPL(mu_receive_msg);
+
+void mu_init(void __iomem *base)
+{
+	uint32_t reg;
+
+	reg = readl_relaxed(base + MU_ACR);
+	/* Clear GIEn, RIEn, TIEn, GIRn and ABFn. */
+	reg &= ~(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_relaxed(reg, base + MU_ACR);
+}
+EXPORT_SYMBOL_GPL(mu_init);
diff --git a/include/soc/imx/mu.h b/include/soc/imx/mu.h
new file mode 100644
index 0000000..1f54667
--- /dev/null
+++ b/include/soc/imx/mu.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017-2018 NXP
+ */
+
+#ifndef IMX_MU_H
+#define IMX_MU_H
+
+#define MU_SIZE			0x10000
+#define MU_TR_COUNT		4
+#define MU_RR_COUNT		4
+
+void mu_init(void __iomem *base);
+void mu_send_msg(void __iomem *base, uint32_t index, uint32_t msg);
+void mu_receive_msg(void __iomem *base, uint32_t index, uint32_t *msg);
+void mu_enable_general_int(void __iomem *base, uint32_t index);
+void mu_enable_rx_full_int(void __iomem *base, uint32_t index);
+uint32_t mu_read_status(void __iomem *base);
+int32_t mu_set_fn(void __iomem *base, uint32_t Fn);
+#endif
-- 
2.7.4

[PATCH 2/4] 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>
---
 .../devicetree/bindings/arm/freescale/fsl,mu.txt   | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
new file mode 100644
index 0000000..a7ceb1f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
@@ -0,0 +1,33 @@
+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 Bfacing).
+
+Messaging Unit Device Node:
+=============================
+
+Required properties:
+-------------------
+- compatible :	should be "fsl,<chip>-mu", the supported chips include
+		imx6sx, imx7d, imx7ulp, imx8qxp, imx8qm, imx8mq.
+- reg :		Should contain the registers location and length
+- interrupts :	Interrupt number. The interrupt specifier format depends
+		on the interrupt controller parent.
+
+Examples:
+--------
+lsio_mu0: mu@5d1b0000 {
+	compatible = "fsl,imx8qxp-mu", "fsl,imx6sx-mu";
+	reg = <0x0 0x5d1b0000 0x0 0x10000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	status = "okay";
+};
-- 
2.7.4

[PATCH 2/4] 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>
---
 .../devicetree/bindings/arm/freescale/fsl,mu.txt   | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
new file mode 100644
index 0000000..a7ceb1f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
@@ -0,0 +1,33 @@
+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 Bfacing).
+
+Messaging Unit Device Node:
+=============================
+
+Required properties:
+-------------------
+- compatible :	should be "fsl,<chip>-mu", the supported chips include
+		imx6sx, imx7d, imx7ulp, imx8qxp, imx8qm, imx8mq.
+- reg :		Should contain the registers location and length
+- interrupts :	Interrupt number. The interrupt specifier format depends
+		on the interrupt controller parent.
+
+Examples:
+--------
+lsio_mu0: mu at 5d1b0000 {
+	compatible = "fsl,imx8qxp-mu", "fsl,imx6sx-mu";
+	reg = <0x0 0x5d1b0000 0x0 0x10000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	status = "okay";
+};
-- 
2.7.4

[PATCH 3/4] 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>
---
 .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 40 ++++++++++++++++++++++
 1 file changed, 40 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..3b51d82
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -0,0 +1,40 @@
+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,imx8qxp-scu" or "fsl,imx8qm-scu"
+- fsl,mu: a phandle to the Message Unit used by SCU. 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_mu0: mu@5d1b0000 {
+	compatible = "fsl,imx8qxp-mu", "fsl,imx6sx-mu";
+	reg = <0x0 0x5d1b0000 0x0 0x10000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	status = "okay";
+};
+
+scu {
+	compatible = "fsl,imx8qxp-scu";
+	fsl,mu = <&lsio_mu0>;
+	status = "okay";
+};
-- 
2.7.4

[PATCH 3/4] 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>
---
 .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 40 ++++++++++++++++++++++
 1 file changed, 40 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..3b51d82
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -0,0 +1,40 @@
+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,imx8qxp-scu" or "fsl,imx8qm-scu"
+- fsl,mu: a phandle to the Message Unit used by SCU. 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_mu0: mu at 5d1b0000 {
+	compatible = "fsl,imx8qxp-mu", "fsl,imx6sx-mu";
+	reg = <0x0 0x5d1b0000 0x0 0x10000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	status = "okay";
+};
+
+scu {
+	compatible = "fsl,imx8qxp-scu";
+	fsl,mu = <&lsio_mu0>;
+	status = "okay";
+};
-- 
2.7.4

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

[Oleksij Rempel]

Hi,

On Sat, Apr 28, 2018 at 02:46:12AM +0800, 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 library calls
> 1) Implementation of the IPC functions based on MUs (client side).
> 2) SCU firmware services APIs implementation ased on RPC calls which
>    are mostly generated by SCU firmware


Hm... I fail to see the difference between remoteproc, virtio, rpmsg and other
existing frameworks in kernel with this functionality. Why do we need
vendor/soc specific framework once again?

> Dong Aisheng (4):
>   soc: imx: add mu library functions support
>   dt-bindings: arm: fsl: add mu binding doc
>   dt-bindings: arm: fsl: add scu binding doc
>   soc: imx: add SC firmware IPC and APIs
> 
>  .../devicetree/bindings/arm/freescale/fsl,mu.txt   |  33 +
>  .../devicetree/bindings/arm/freescale/fsl,scu.txt  |  40 ++
>  drivers/soc/imx/Kconfig                            |   7 +
>  drivers/soc/imx/Makefile                           |   2 +
>  drivers/soc/imx/imx_mu.c                           | 125 ++++
>  drivers/soc/imx/sc/Makefile                        |   8 +
>  drivers/soc/imx/sc/main/ipc.c                      | 270 ++++++++
>  drivers/soc/imx/sc/main/rpc.h                      | 123 ++++
>  drivers/soc/imx/sc/svc/irq/rpc.h                   |  41 ++
>  drivers/soc/imx/sc/svc/irq/rpc_clnt.c              |  58 ++
>  drivers/soc/imx/sc/svc/misc/rpc.h                  |  58 ++
>  drivers/soc/imx/sc/svc/misc/rpc_clnt.c             | 368 ++++++++++
>  drivers/soc/imx/sc/svc/pad/rpc.h                   |  55 ++
>  drivers/soc/imx/sc/svc/pad/rpc_clnt.c              | 412 +++++++++++
>  drivers/soc/imx/sc/svc/pm/rpc.h                    |  58 ++
>  drivers/soc/imx/sc/svc/pm/rpc_clnt.c               | 393 +++++++++++
>  drivers/soc/imx/sc/svc/rm/rpc.h                    |  70 ++
>  drivers/soc/imx/sc/svc/rm/rpc_clnt.c               | 612 +++++++++++++++++
>  drivers/soc/imx/sc/svc/timer/rpc.h                 |  52 ++
>  drivers/soc/imx/sc/svc/timer/rpc_clnt.c            | 295 ++++++++
>  include/soc/imx/mu.h                               |  21 +
>  include/soc/imx/sc/ipc.h                           |  46 ++
>  include/soc/imx/sc/scfw.h                          |  24 +
>  include/soc/imx/sc/sci.h                           |  35 +
>  include/soc/imx/sc/svc/irq/api.h                   | 139 ++++
>  include/soc/imx/sc/svc/misc/api.h                  | 395 +++++++++++
>  include/soc/imx/sc/svc/pad/api.h                   | 536 +++++++++++++++
>  include/soc/imx/sc/svc/pm/api.h                    | 559 +++++++++++++++
>  include/soc/imx/sc/svc/rm/api.h                    | 726 ++++++++++++++++++++
>  include/soc/imx/sc/svc/timer/api.h                 | 265 +++++++
>  include/soc/imx/sc/types.h                         | 764 +++++++++++++++++++++
>  31 files changed, 6590 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
>  create mode 100644 drivers/soc/imx/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 drivers/soc/imx/sc/svc/irq/rpc.h
>  create mode 100644 drivers/soc/imx/sc/svc/irq/rpc_clnt.c
>  create mode 100644 drivers/soc/imx/sc/svc/misc/rpc.h
>  create mode 100644 drivers/soc/imx/sc/svc/misc/rpc_clnt.c
>  create mode 100644 drivers/soc/imx/sc/svc/pad/rpc.h
>  create mode 100644 drivers/soc/imx/sc/svc/pad/rpc_clnt.c
>  create mode 100644 drivers/soc/imx/sc/svc/pm/rpc.h
>  create mode 100644 drivers/soc/imx/sc/svc/pm/rpc_clnt.c
>  create mode 100644 drivers/soc/imx/sc/svc/rm/rpc.h
>  create mode 100644 drivers/soc/imx/sc/svc/rm/rpc_clnt.c
>  create mode 100644 drivers/soc/imx/sc/svc/timer/rpc.h
>  create mode 100644 drivers/soc/imx/sc/svc/timer/rpc_clnt.c
>  create mode 100644 include/soc/imx/mu.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/sci.h
>  create mode 100644 include/soc/imx/sc/svc/irq/api.h
>  create mode 100644 include/soc/imx/sc/svc/misc/api.h
>  create mode 100644 include/soc/imx/sc/svc/pad/api.h
>  create mode 100644 include/soc/imx/sc/svc/pm/api.h
>  create mode 100644 include/soc/imx/sc/svc/rm/api.h
>  create mode 100644 include/soc/imx/sc/svc/timer/api.h
>  create mode 100644 include/soc/imx/sc/types.h
> 
> -- 
> 2.7.4
> 
> 
> 

-- 
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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180501/d310bbdb/attachment.sig>

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

[Rob Herring]

On Sat, Apr 28, 2018 at 02:46:14AM +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>
> ---
>  .../devicetree/bindings/arm/freescale/fsl,mu.txt   | 33 ++++++++++++++++++++++

bindings/mailbox/ ?

Why aren't you using the mailbox binding?

>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> new file mode 100644
> index 0000000..a7ceb1f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> @@ -0,0 +1,33 @@
> +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 Bfacing).

B-facing

> +
> +Messaging Unit Device Node:
> +=============================
> +
> +Required properties:
> +-------------------
> +- compatible :	should be "fsl,<chip>-mu", the supported chips include
> +		imx6sx, imx7d, imx7ulp, imx8qxp, imx8qm, imx8mq.

'qm' and 'mq' are really both parts?

> +- reg :		Should contain the registers location and length
> +- interrupts :	Interrupt number. The interrupt specifier format depends
> +		on the interrupt controller parent.
> +
> +Examples:
> +--------
> +lsio_mu0: mu@5d1b0000 {
> +	compatible = "fsl,imx8qxp-mu", "fsl,imx6sx-mu";

It is not clear from the definition above that fsl,imx6sx-mu is a 
fallback. Is that true for all the other chips too?

> +	reg = <0x0 0x5d1b0000 0x0 0x10000>;

Really has 64KB of registers? You are just wasting virtual address space 
which is scarce on 32-bit processors with GBs of RAM.

> +	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> +	status = "okay";

Don't show status in examples.

> +};
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rob Herring]

On Sat, Apr 28, 2018 at 02:46:14AM +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>
> ---
>  .../devicetree/bindings/arm/freescale/fsl,mu.txt   | 33 ++++++++++++++++++++++

bindings/mailbox/ ?

Why aren't you using the mailbox binding?

>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> new file mode 100644
> index 0000000..a7ceb1f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> @@ -0,0 +1,33 @@
> +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 Bfacing).

B-facing

> +
> +Messaging Unit Device Node:
> +=============================
> +
> +Required properties:
> +-------------------
> +- compatible :	should be "fsl,<chip>-mu", the supported chips include
> +		imx6sx, imx7d, imx7ulp, imx8qxp, imx8qm, imx8mq.

'qm' and 'mq' are really both parts?

> +- reg :		Should contain the registers location and length
> +- interrupts :	Interrupt number. The interrupt specifier format depends
> +		on the interrupt controller parent.
> +
> +Examples:
> +--------
> +lsio_mu0: mu at 5d1b0000 {
> +	compatible = "fsl,imx8qxp-mu", "fsl,imx6sx-mu";

It is not clear from the definition above that fsl,imx6sx-mu is a 
fallback. Is that true for all the other chips too?

> +	reg = <0x0 0x5d1b0000 0x0 0x10000>;

Really has 64KB of registers? You are just wasting virtual address space 
which is scarce on 32-bit processors with GBs of RAM.

> +	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> +	status = "okay";

Don't show status in examples.

> +};
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rob Herring]

On Sat, Apr 28, 2018 at 02:46:15AM +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>
> ---
>  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 40 ++++++++++++++++++++++
>  1 file changed, 40 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..3b51d82
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> @@ -0,0 +1,40 @@
> +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,imx8qxp-scu" or "fsl,imx8qm-scu"
> +- fsl,mu: a phandle to the Message Unit used by SCU. Should be

Use the mailbox binding.

> +	  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_mu0: mu@5d1b0000 {
> +	compatible = "fsl,imx8qxp-mu", "fsl,imx6sx-mu";
> +	reg = <0x0 0x5d1b0000 0x0 0x10000>;
> +	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> +	status = "okay";
> +};
> +
> +scu {
> +	compatible = "fsl,imx8qxp-scu";
> +	fsl,mu = <&lsio_mu0>;
> +	status = "okay";

Don't show status in examples.

> +};
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rob Herring]

On Sat, Apr 28, 2018 at 02:46:15AM +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>
> ---
>  .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 40 ++++++++++++++++++++++
>  1 file changed, 40 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..3b51d82
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> @@ -0,0 +1,40 @@
> +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,imx8qxp-scu" or "fsl,imx8qm-scu"
> +- fsl,mu: a phandle to the Message Unit used by SCU. Should be

Use the mailbox binding.

> +	  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_mu0: mu at 5d1b0000 {
> +	compatible = "fsl,imx8qxp-mu", "fsl,imx6sx-mu";
> +	reg = <0x0 0x5d1b0000 0x0 0x10000>;
> +	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> +	status = "okay";
> +};
> +
> +scu {
> +	compatible = "fsl,imx8qxp-scu";
> +	fsl,mu = <&lsio_mu0>;
> +	status = "okay";

Don't show status in examples.

> +};
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sascha Hauer]

On Sat, Apr 28, 2018 at 02:46:16AM +0800, Dong Aisheng wrote:
> +/* Initialization of the MU code. */
> +int __init imx8_scu_init(void)
> +{
> +	struct device_node *np, *mu_np;
> +	struct resource mu_res;
> +	sc_err_t sci_err;
> +	int ret;
> +
> +	if (!of_machine_is_compatible("fsl,imx8qxp"))
> +		return 0;
> +
> +	np = of_find_compatible_node(NULL, NULL, "nxp,imx8qxp-scu");
> +	if (!np)
> +		return -ENODEV;
> +
> +	mu_np = of_parse_phandle(np, "fsl,mu", 0);
> +	if (WARN_ON(!mu_np))
> +		return -EINVAL;
> +
> +	ret = of_address_to_resource(mu_np, 0, &mu_res);
> +	if (WARN_ON(ret))
> +		return -EINVAL;
> +
> +	/* we use mu physical address as IPC communication channel ID */
> +	sci_err = sc_ipc_open(&scu_ipc_handle, (sc_ipc_id_t)(mu_res.start));
> +	if (sci_err != SC_ERR_NONE) {
> +		pr_err("Cannot open MU channel to SCU\n");
> +		return sci_err;
> +	};

Introducing private error codes always has the risk of just forwarding
them as errno codes as done here.

> +
> +	of_node_put(mu_np);
> +
> +	pr_info("NXP i.MX SCU Initialized (scu_ipc %u)\n", scu_ipc_handle);
> +
> +	return 0;
> +}
> +early_initcall(imx8_scu_init);

This code shows that the separate 'scu' device node shouldn't exist. It is
only used as a stepping stone to find the 'mu' node. Instead of
providing a proper driver for the 'mu' node the scu code registers it
with its physical address.
I don't think it makes sense to separate mu and scu code in this way.

> +#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)])

These macros only make the code less readable, please drop them. Plain
m->version is easier to read and forces the reader through less
indirections. IMO m->DATA.u32[0] = foo; m->DATA.u8[4] = bar is still
better readable than doing the division by size in macros.

> +typedef enum sc_rpc_async_state_e {
> +	SC_RPC_ASYNC_STATE_RD_START = 0,
> +	SC_RPC_ASYNC_STATE_RD_ACTIVE = 1,
> +	SC_RPC_ASYNC_STATE_RD_DONE = 2,
> +	SC_RPC_ASYNC_STATE_WR_START = 3,
> +	SC_RPC_ASYNC_STATE_WR_ACTIVE = 4,
> +	SC_RPC_ASYNC_STATE_WR_DONE = 5,
> +} sc_rpc_async_state_t;
> +
> +typedef struct sc_rpc_async_msg_s {
> +	sc_rpc_async_state_t state;
> +	uint8_t wordIdx;
> +	sc_rpc_msg_t msg;
> +	uint32_t timeStamp;
> +} sc_rpc_async_msg_t;

We normally do not typedef struct types. Any good reasons to do so here?

> +
> +/* 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.
> + */
> +void sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp);
> +
> +/*
> + * This is an internal function to dispath an RPC call that has
> + * arrived via IPC over an MU. It is called by server-side SCFW.
> + *
> + * @param[in]     mu          MU message arrived on
> + * @param[in,out] msg         handle to a message
> + *
> + * The function result is returned in msg.
> + */
> +void sc_rpc_dispatch(sc_rsrc_t mu, sc_rpc_msg_t *msg);

Declared but never defined.

> +
> +/*
> + * This function translates an RPC message and forwards on to the
> + * normal RPC API.  It is used only by hypervisors.
> + *
> + * @param[in]     ipc         IPC handle
> + * @param[in,out] msg         handle to a message
> + *
> + * This function decodes a message, calls macros to translate the
> + * resources, pins, addresses, partitions, memory regions, etc. and
> + * then forwards on to the hypervisors SCFW API.Return results are
> + * translated back abd placed back into the message to be returned
> + * to the original API.
> + */
> +void sc_rpc_xlate(sc_ipc_t ipc, sc_rpc_msg_t *msg);

ditto.

> +
> +#endif /* _SC_RPC_H */
> diff --git a/drivers/soc/imx/sc/svc/irq/rpc.h b/drivers/soc/imx/sc/svc/irq/rpc.h
> new file mode 100644
> index 0000000..0de8718
> --- /dev/null
> +++ b/drivers/soc/imx/sc/svc/irq/rpc.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> + * Copyright 2017~2018 NXP
> + *
> + * Header file for the IRQ RPC implementation.
> + */
> +
> +#ifndef _SC_IRQ_RPC_H
> +#define _SC_IRQ_RPC_H
> +
> +/* Types */
> +
> +/*!
> + * This type is used to indicate RPC IRQ function calls.
> + */
> +typedef enum irq_func_e {
> +	IRQ_FUNC_UNKNOWN = 0,	/* Unknown function */
> +	IRQ_FUNC_ENABLE = 1,	/* Index for irq_enable() RPC call */
> +	IRQ_FUNC_STATUS = 2,	/* Index for irq_status() RPC call */
> +} irq_func_t;
> +
> +/* Functions */
> +
> +/*!
> + * This function dispatches an incoming IRQ RPC request.
> + *
> + * @param[in]     caller_pt   caller partition
> + * @param[in]     msg         pointer to RPC message
> + */
> +void irq_dispatch(sc_rm_pt_t caller_pt, sc_rpc_msg_t *msg);

ditto.

> +
> +/*!
> + * This function translates and dispatches an IRQ RPC request.
> + *
> + * @param[in]     ipc         IPC handle
> + * @param[in]     msg         pointer to RPC message
> + */
> +void irq_xlate(sc_ipc_t ipc, sc_rpc_msg_t *msg);

ditto.

> +++ b/include/soc/imx/sc/svc/timer/api.h
> @@ -0,0 +1,265 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> + * Copyright 2017~2018 NXP
> + *
> + * Header file containing the public API for the System Controller (SC)
> + * Timer function.
> + *
> + * TIMER_SVC (SVC) Timer Service
> + *
> + * Module for the Timer service. This includes support for the watchdog, RTC,
> + * and system counter. Note every resource partition has a watchdog it can
> + * use.
> + */
> +
> +#ifndef _SC_TIMER_API_H
> +#define _SC_TIMER_API_H
> +
> +/* Includes */
> +
> +#include <soc/imx/sc/types.h>
> +#include <soc/imx/sc/svc/rm/api.h>
> +
> +/* Defines */
> +
> +/*
> + * @name Defines for type widths
> + */
> +#define SC_TIMER_ACTION_W   3	/* Width of sc_timer_wdog_action_t */
> +
> +/*
> + * @name Defines for sc_timer_wdog_action_t
> + */
> +#define SC_TIMER_WDOG_ACTION_PARTITION      0	/* Reset partition */
> +#define SC_TIMER_WDOG_ACTION_WARM           1	/* Warm reset system */
> +#define SC_TIMER_WDOG_ACTION_COLD           2	/* Cold reset system */
> +#define SC_TIMER_WDOG_ACTION_BOARD          3	/* Reset board */
> +#define SC_TIMER_WDOG_ACTION_IRQ            4	/* Only generate IRQs */
> +
> +/* Types */
> +
> +/*
> + * This type is used to configure the watchdog action.
> + */
> +typedef uint8_t sc_timer_wdog_action_t;

Mixing an unnecessary typedef and defines doesn't look good. This should
be

typedef enum {
	SC_TIMER_WDOG_ACTION_PARTITION,
} sc_timer_wdog_action_t;

The same pattern is used elsewhere in this patch.

> +
> +/*
> + * This type is used to declare a watchdog time value in milliseconds.
> + */
> +typedef uint32_t sc_timer_wdog_time_t;

Why an extra type for 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 2/4] dt-bindings: arm: fsl: add mu binding doc

[A.s. Dong]

Hi Rob,

Thanks for the review.

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Tuesday, May 1, 2018 11:26 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 2/4] dt-bindings: arm: fsl: add mu binding doc
> 
> On Sat, Apr 28, 2018 at 02:46:14AM +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>
> > ---
> >  .../devicetree/bindings/arm/freescale/fsl,mu.txt   | 33
> ++++++++++++++++++++++
> 
> bindings/mailbox/ ?
> 
> Why aren't you using the mailbox binding?
> 

It's mainly because MU is not implemented as a mailbox driver,
Instead, they're provided in library calls to gain higher efficiency.

For this case, do i still need to change to mailbox binding?

BTW i.MX MU has only one channel. But the binding doc claims
#mbox-cells must be at least 1. That means the index cells is meaningless
to i.MX MUs.
(Probably mailbox binding could be extended to cover this case?
 E.g. #mbox-cells = <0>)

> >  1 file changed, 33 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> > new file mode 100644
> > index 0000000..a7ceb1f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> > @@ -0,0 +1,33 @@
> > +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 Bfacing).
> 
> B-facing
> 

Got it

> > +
> > +Messaging Unit Device Node:
> > +=============================
> > +
> > +Required properties:
> > +-------------------
> > +- compatible :	should be "fsl,<chip>-mu", the supported chips
> include
> > +		imx6sx, imx7d, imx7ulp, imx8qxp, imx8qm, imx8mq.
> 
> 'qm' and 'mq' are really both parts?
> 

Yes, they're two SoCs.

> > +- reg :		Should contain the registers location and length
> > +- interrupts :	Interrupt number. The interrupt specifier format
> depends
> > +		on the interrupt controller parent.
> > +
> > +Examples:
> > +--------
> > +lsio_mu0: mu@5d1b0000 {
> > +	compatible = "fsl,imx8qxp-mu", "fsl,imx6sx-mu";
> 
> It is not clear from the definition above that fsl,imx6sx-mu is a fallback. Is that
> true for all the other chips too?
> 

Mx6sx is the first one introducing MU while all others chips are almost using the
same one except MX7ULP with a bit differences.

> > +	reg = <0x0 0x5d1b0000 0x0 0x10000>;
> 
> Really has 64KB of registers? You are just wasting virtual address space which
> is scarce on 32-bit processors with GBs of RAM.
> 

For 32-bit processors, it's 16KB.
For 64-bit processors, yes, according to RM, it's 64KB one slot in the memory
map chapter. But actually It has only 10 registers for user to access,
do you think we need to reduce to the real register size?

> > +	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> > +	status = "okay";
> 
> Don't show status in examples.
> 

Got it.

Regards
Dong Aisheng

> > +};
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree"
> > in the body of a message to majordomo@vger.kernel.org More
> majordomo
> > info at
> >
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger
> > .kernel.org%2Fmajordomo-
> info.html&data=02%7C01%7Caisheng.dong%40nxp.co
> >
> m%7C94ea748130d84ea27d0e08d5af77d20a%7C686ea1d3bc2b4c6fa92cd99c5
> c30163
> >
> 5%7C0%7C0%7C636607851512054847&sdata=%2BFi05UUHdS77rlQpcFqqxg2Z
> VDx60ci
> > Zemf%2BM4lHlpg%3D&reserved=0

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

[A.s. Dong]

Hi Rob,

Thanks for the review.

> -----Original Message-----
> From: Rob Herring [mailto:robh at kernel.org]
> Sent: Tuesday, May 1, 2018 11:26 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 2/4] dt-bindings: arm: fsl: add mu binding doc
> 
> On Sat, Apr 28, 2018 at 02:46:14AM +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>
> > ---
> >  .../devicetree/bindings/arm/freescale/fsl,mu.txt   | 33
> ++++++++++++++++++++++
> 
> bindings/mailbox/ ?
> 
> Why aren't you using the mailbox binding?
> 

It's mainly because MU is not implemented as a mailbox driver,
Instead, they're provided in library calls to gain higher efficiency.

For this case, do i still need to change to mailbox binding?

BTW i.MX MU has only one channel. But the binding doc claims
#mbox-cells must be at least 1. That means the index cells is meaningless
to i.MX MUs.
(Probably mailbox binding could be extended to cover this case?
 E.g. #mbox-cells = <0>)

> >  1 file changed, 33 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> > new file mode 100644
> > index 0000000..a7ceb1f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> > @@ -0,0 +1,33 @@
> > +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 Bfacing).
> 
> B-facing
> 

Got it

> > +
> > +Messaging Unit Device Node:
> > +=============================
> > +
> > +Required properties:
> > +-------------------
> > +- compatible :	should be "fsl,<chip>-mu", the supported chips
> include
> > +		imx6sx, imx7d, imx7ulp, imx8qxp, imx8qm, imx8mq.
> 
> 'qm' and 'mq' are really both parts?
> 

Yes, they're two SoCs.

> > +- reg :		Should contain the registers location and length
> > +- interrupts :	Interrupt number. The interrupt specifier format
> depends
> > +		on the interrupt controller parent.
> > +
> > +Examples:
> > +--------
> > +lsio_mu0: mu at 5d1b0000 {
> > +	compatible = "fsl,imx8qxp-mu", "fsl,imx6sx-mu";
> 
> It is not clear from the definition above that fsl,imx6sx-mu is a fallback. Is that
> true for all the other chips too?
> 

Mx6sx is the first one introducing MU while all others chips are almost using the
same one except MX7ULP with a bit differences.

> > +	reg = <0x0 0x5d1b0000 0x0 0x10000>;
> 
> Really has 64KB of registers? You are just wasting virtual address space which
> is scarce on 32-bit processors with GBs of RAM.
> 

For 32-bit processors, it's 16KB.
For 64-bit processors, yes, according to RM, it's 64KB one slot in the memory
map chapter. But actually It has only 10 registers for user to access,
do you think we need to reduce to the real register size?

> > +	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> > +	status = "okay";
> 
> Don't show status in examples.
> 

Got it.

Regards
Dong Aisheng

> > +};
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree"
> > in the body of a message to majordomo at vger.kernel.org More
> majordomo
> > info at
> >
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger
> > .kernel.org%2Fmajordomo-
> info.html&data=02%7C01%7Caisheng.dong%40nxp.co
> >
> m%7C94ea748130d84ea27d0e08d5af77d20a%7C686ea1d3bc2b4c6fa92cd99c5
> c30163
> >
> 5%7C0%7C0%7C636607851512054847&sdata=%2BFi05UUHdS77rlQpcFqqxg2Z
> VDx60ci
> > Zemf%2BM4lHlpg%3D&reserved=0

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

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Wednesday, May 2, 2018 6:04 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 4/4] soc: imx: add SC firmware IPC and APIs
> 
> On Sat, Apr 28, 2018 at 02:46:16AM +0800, Dong Aisheng wrote:
> > +/* Initialization of the MU code. */
> > +int __init imx8_scu_init(void)
> > +{
> > +	struct device_node *np, *mu_np;
> > +	struct resource mu_res;
> > +	sc_err_t sci_err;
> > +	int ret;
> > +
> > +	if (!of_machine_is_compatible("fsl,imx8qxp"))
> > +		return 0;
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "nxp,imx8qxp-scu");
> > +	if (!np)
> > +		return -ENODEV;
> > +
> > +	mu_np = of_parse_phandle(np, "fsl,mu", 0);
> > +	if (WARN_ON(!mu_np))
> > +		return -EINVAL;
> > +
> > +	ret = of_address_to_resource(mu_np, 0, &mu_res);
> > +	if (WARN_ON(ret))
> > +		return -EINVAL;
> > +
> > +	/* we use mu physical address as IPC communication channel ID */
> > +	sci_err = sc_ipc_open(&scu_ipc_handle, (sc_ipc_id_t)(mu_res.start));
> > +	if (sci_err != SC_ERR_NONE) {
> > +		pr_err("Cannot open MU channel to SCU\n");
> > +		return sci_err;
> > +	};
> 
> Introducing private error codes always has the risk of just forwarding them as
> errno codes as done here.
> 

Yes, you're right.
We probably could do the same as scpi_to_linux_errno in
drivers/firmware/arm_scpi.c.
However, may can't fix the issue permanently.

> > +
> > +	of_node_put(mu_np);
> > +
> > +	pr_info("NXP i.MX SCU Initialized (scu_ipc %u)\n", scu_ipc_handle);
> > +
> > +	return 0;
> > +}
> > +early_initcall(imx8_scu_init);
> 
> This code shows that the separate 'scu' device node shouldn't exist. It is only
> used as a stepping stone to find the 'mu' node. Instead of providing a proper
> driver for the 'mu' node the scu code registers it with its physical address.
> I don't think it makes sense to separate mu and scu code in this way.
> 

I agree that may not look quite well. 
It mainly because we want to use the MU physical address as a MU ID. 
(can't use virtual address as sc_ipc_id_t is u32 defined by SCU firmware.

If you have any better suggestion please let me know.

> > +#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)])
> 
> These macros only make the code less readable, please drop them. Plain
> m->version is easier to read and forces the reader through less
> indirections. IMO m->DATA.u32[0] = foo; m->DATA.u8[4] = bar is still better
> readable than doing the division by size in macros.
> 

I somehow agree with you.
See below...

> > +typedef enum sc_rpc_async_state_e {
> > +	SC_RPC_ASYNC_STATE_RD_START = 0,
> > +	SC_RPC_ASYNC_STATE_RD_ACTIVE = 1,
> > +	SC_RPC_ASYNC_STATE_RD_DONE = 2,
> > +	SC_RPC_ASYNC_STATE_WR_START = 3,
> > +	SC_RPC_ASYNC_STATE_WR_ACTIVE = 4,
> > +	SC_RPC_ASYNC_STATE_WR_DONE = 5,
> > +} sc_rpc_async_state_t;
> > +
> > +typedef struct sc_rpc_async_msg_s {
> > +	sc_rpc_async_state_t state;
> > +	uint8_t wordIdx;
> > +	sc_rpc_msg_t msg;
> > +	uint32_t timeStamp;
> > +} sc_rpc_async_msg_t;
> 
> We normally do not typedef struct types. Any good reasons to do so here?
> 

Yes, I did not some checkpatch warnings about this.

For this patch, I only implemented IPC function calls based on MU which is in
drivers/soc/imx/sc/main/ipc.c. All other files actually are generated by SCU firmware
script automatically for Linux OS. They're tightly couple with SCU firmware side
implementation. In order to gain better maintainability (easy upgrade and sync with
SCU firmware), we're trying to not change those APIs too much unless we have strong
reasons. Does it make sense to you?

> > +
> > +/* 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.
> > + */
> > +void sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp);
> > +
> > +/*
> > + * This is an internal function to dispath an RPC call that has
> > + * arrived via IPC over an MU. It is called by server-side SCFW.
> > + *
> > + * @param[in]     mu          MU message arrived on
> > + * @param[in,out] msg         handle to a message
> > + *
> > + * The function result is returned in msg.
> > + */
> > +void sc_rpc_dispatch(sc_rsrc_t mu, sc_rpc_msg_t *msg);
> 
> Declared but never defined.
> 

Good catch. 

> > +
> > +/*
> > + * This function translates an RPC message and forwards on to the
> > + * normal RPC API.  It is used only by hypervisors.
> > + *
> > + * @param[in]     ipc         IPC handle
> > + * @param[in,out] msg         handle to a message
> > + *
> > + * This function decodes a message, calls macros to translate the
> > + * resources, pins, addresses, partitions, memory regions, etc. and
> > + * then forwards on to the hypervisors SCFW API.Return results are
> > + * translated back abd placed back into the message to be returned
> > + * to the original API.
> > + */
> > +void sc_rpc_xlate(sc_ipc_t ipc, sc_rpc_msg_t *msg);
> 
> ditto.
> 
> > +
> > +#endif /* _SC_RPC_H */
> > diff --git a/drivers/soc/imx/sc/svc/irq/rpc.h
> > b/drivers/soc/imx/sc/svc/irq/rpc.h
> > new file mode 100644
> > index 0000000..0de8718
> > --- /dev/null
> > +++ b/drivers/soc/imx/sc/svc/irq/rpc.h
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> > + * Copyright 2017~2018 NXP
> > + *
> > + * Header file for the IRQ RPC implementation.
> > + */
> > +
> > +#ifndef _SC_IRQ_RPC_H
> > +#define _SC_IRQ_RPC_H
> > +
> > +/* Types */
> > +
> > +/*!
> > + * This type is used to indicate RPC IRQ function calls.
> > + */
> > +typedef enum irq_func_e {
> > +	IRQ_FUNC_UNKNOWN = 0,	/* Unknown function */
> > +	IRQ_FUNC_ENABLE = 1,	/* Index for irq_enable() RPC call */
> > +	IRQ_FUNC_STATUS = 2,	/* Index for irq_status() RPC call */
> > +} irq_func_t;
> > +
> > +/* Functions */
> > +
> > +/*!
> > + * This function dispatches an incoming IRQ RPC request.
> > + *
> > + * @param[in]     caller_pt   caller partition
> > + * @param[in]     msg         pointer to RPC message
> > + */
> > +void irq_dispatch(sc_rm_pt_t caller_pt, sc_rpc_msg_t *msg);
> 
> ditto.
> 
> > +
> > +/*!
> > + * This function translates and dispatches an IRQ RPC request.
> > + *
> > + * @param[in]     ipc         IPC handle
> > + * @param[in]     msg         pointer to RPC message
> > + */
> > +void irq_xlate(sc_ipc_t ipc, sc_rpc_msg_t *msg);
> 
> ditto.
> 

Those APIs seems are over generated by SCU firmware.
I will clean up it.

> > +++ b/include/soc/imx/sc/svc/timer/api.h
> > @@ -0,0 +1,265 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> > + * Copyright 2017~2018 NXP
> > + *
> > + * Header file containing the public API for the System Controller
> > +(SC)
> > + * Timer function.
> > + *
> > + * TIMER_SVC (SVC) Timer Service
> > + *
> > + * Module for the Timer service. This includes support for the
> > +watchdog, RTC,
> > + * and system counter. Note every resource partition has a watchdog
> > +it can
> > + * use.
> > + */
> > +
> > +#ifndef _SC_TIMER_API_H
> > +#define _SC_TIMER_API_H
> > +
> > +/* Includes */
> > +
> > +#include <soc/imx/sc/types.h>
> > +#include <soc/imx/sc/svc/rm/api.h>
> > +
> > +/* Defines */
> > +
> > +/*
> > + * @name Defines for type widths
> > + */
> > +#define SC_TIMER_ACTION_W   3	/* Width of sc_timer_wdog_action_t
> */
> > +
> > +/*
> > + * @name Defines for sc_timer_wdog_action_t  */
> > +#define SC_TIMER_WDOG_ACTION_PARTITION      0	/* Reset
> partition */
> > +#define SC_TIMER_WDOG_ACTION_WARM           1	/* Warm reset
> system */
> > +#define SC_TIMER_WDOG_ACTION_COLD           2	/* Cold reset system
> */
> > +#define SC_TIMER_WDOG_ACTION_BOARD          3	/* Reset board */
> > +#define SC_TIMER_WDOG_ACTION_IRQ            4	/* Only generate
> IRQs */
> > +
> > +/* Types */
> > +
> > +/*
> > + * This type is used to configure the watchdog action.
> > + */
> > +typedef uint8_t sc_timer_wdog_action_t;
> 
> Mixing an unnecessary typedef and defines doesn't look good. This should
> be
> 
> typedef enum {
> 	SC_TIMER_WDOG_ACTION_PARTITION,
> } sc_timer_wdog_action_t;
> 
> The same pattern is used elsewhere in this patch.
> 

Yes, I understand.
Do you think we need change them all?

> > +
> > +/*
> > + * This type is used to declare a watchdog time value in milliseconds.
> > + */
> > +typedef uint32_t sc_timer_wdog_time_t;
> 
> Why an extra type for this?
> 
> Sascha
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.pengutronix.de%2F&data=02%7C01%7Caisheng.dong%40nxp.com%7C2d0
> 5ba17b0d24957022a08d5b0140906%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C636608522458461041&sdata=WMYanmj1AZkESu0fbXrspidjPbVc
> t3sKQYNhGt5J9ME%3D&reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel at pengutronix.de]
> Sent: Tuesday, May 1, 2018 1:59 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 0/4] soc: imx: add scu firmware api support
> 
> Hi,
> 
> On Sat, Apr 28, 2018 at 02:46:12AM +0800, 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 library calls
> > 1) Implementation of the IPC functions based on MUs (client side).
> > 2) SCU firmware services APIs implementation ased on RPC calls which
> >    are mostly generated by SCU firmware
> 
> 
> Hm... I fail to see the difference between remoteproc, virtio, rpmsg and
> other existing frameworks in kernel with this functionality. Why do we need
> vendor/soc specific framework once again?
> 

Hmm.. It seems like not a vendor specific framework...
It mainly SCU firmware APIs generated by SCU firmware script for
Linux OS components to use.
And those APIs are implemented base on MU simple polling mode
With better performance.

NXP internally has tried mailbox but got performance regression,
So we're still not sure whether Mailbox is quit suitable for i.MX
SCU Firmware. Needs more time to investigate.

And I'm still not quite familiar with remoteproc, virtio, rpmsg.
May need spend more time to investigate later.
And it would be good if you can provide suggestions and sharing
Informations about them.
e.g. what's requirement of i.MX to switch to them? Benefit? Or 
something else valuable?

Regards
Dong Aisheng

> > Dong Aisheng (4):
> >   soc: imx: add mu library functions support
> >   dt-bindings: arm: fsl: add mu binding doc
> >   dt-bindings: arm: fsl: add scu binding doc
> >   soc: imx: add SC firmware IPC and APIs
> >
> >  .../devicetree/bindings/arm/freescale/fsl,mu.txt   |  33 +
> >  .../devicetree/bindings/arm/freescale/fsl,scu.txt  |  40 ++
> >  drivers/soc/imx/Kconfig                            |   7 +
> >  drivers/soc/imx/Makefile                           |   2 +
> >  drivers/soc/imx/imx_mu.c                           | 125 ++++
> >  drivers/soc/imx/sc/Makefile                        |   8 +
> >  drivers/soc/imx/sc/main/ipc.c                      | 270 ++++++++
> >  drivers/soc/imx/sc/main/rpc.h                      | 123 ++++
> >  drivers/soc/imx/sc/svc/irq/rpc.h                   |  41 ++
> >  drivers/soc/imx/sc/svc/irq/rpc_clnt.c              |  58 ++
> >  drivers/soc/imx/sc/svc/misc/rpc.h                  |  58 ++
> >  drivers/soc/imx/sc/svc/misc/rpc_clnt.c             | 368 ++++++++++
> >  drivers/soc/imx/sc/svc/pad/rpc.h                   |  55 ++
> >  drivers/soc/imx/sc/svc/pad/rpc_clnt.c              | 412 +++++++++++
> >  drivers/soc/imx/sc/svc/pm/rpc.h                    |  58 ++
> >  drivers/soc/imx/sc/svc/pm/rpc_clnt.c               | 393 +++++++++++
> >  drivers/soc/imx/sc/svc/rm/rpc.h                    |  70 ++
> >  drivers/soc/imx/sc/svc/rm/rpc_clnt.c               | 612 +++++++++++++++++
> >  drivers/soc/imx/sc/svc/timer/rpc.h                 |  52 ++
> >  drivers/soc/imx/sc/svc/timer/rpc_clnt.c            | 295 ++++++++
> >  include/soc/imx/mu.h                               |  21 +
> >  include/soc/imx/sc/ipc.h                           |  46 ++
> >  include/soc/imx/sc/scfw.h                          |  24 +
> >  include/soc/imx/sc/sci.h                           |  35 +
> >  include/soc/imx/sc/svc/irq/api.h                   | 139 ++++
> >  include/soc/imx/sc/svc/misc/api.h                  | 395 +++++++++++
> >  include/soc/imx/sc/svc/pad/api.h                   | 536 +++++++++++++++
> >  include/soc/imx/sc/svc/pm/api.h                    | 559 +++++++++++++++
> >  include/soc/imx/sc/svc/rm/api.h                    | 726 ++++++++++++++++++++
> >  include/soc/imx/sc/svc/timer/api.h                 | 265 +++++++
> >  include/soc/imx/sc/types.h                         | 764 +++++++++++++++++++++
> >  31 files changed, 6590 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
> >  create mode 100644
> > Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >  create mode 100644 drivers/soc/imx/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
> > drivers/soc/imx/sc/svc/irq/rpc.h  create mode 100644
> > drivers/soc/imx/sc/svc/irq/rpc_clnt.c
> >  create mode 100644 drivers/soc/imx/sc/svc/misc/rpc.h  create mode
> > 100644 drivers/soc/imx/sc/svc/misc/rpc_clnt.c
> >  create mode 100644 drivers/soc/imx/sc/svc/pad/rpc.h  create mode
> > 100644 drivers/soc/imx/sc/svc/pad/rpc_clnt.c
> >  create mode 100644 drivers/soc/imx/sc/svc/pm/rpc.h  create mode
> > 100644 drivers/soc/imx/sc/svc/pm/rpc_clnt.c
> >  create mode 100644 drivers/soc/imx/sc/svc/rm/rpc.h  create mode
> > 100644 drivers/soc/imx/sc/svc/rm/rpc_clnt.c
> >  create mode 100644 drivers/soc/imx/sc/svc/timer/rpc.h
> >  create mode 100644 drivers/soc/imx/sc/svc/timer/rpc_clnt.c
> >  create mode 100644 include/soc/imx/mu.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/sci.h  create mode 100644
> > include/soc/imx/sc/svc/irq/api.h  create mode 100644
> > include/soc/imx/sc/svc/misc/api.h  create mode 100644
> > include/soc/imx/sc/svc/pad/api.h  create mode 100644
> > include/soc/imx/sc/svc/pm/api.h  create mode 100644
> > include/soc/imx/sc/svc/rm/api.h  create mode 100644
> > include/soc/imx/sc/svc/timer/api.h
> >  create mode 100644 include/soc/imx/sc/types.h
> >
> > --
> > 2.7.4
> >
> >
> >
> 
> --
> 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 0/4] soc: imx: add scu firmware api support

[Oleksij Rempel]

On 02.05.2018 20:54, A.s. Dong wrote:
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel at pengutronix.de]
>> Sent: Tuesday, May 1, 2018 1:59 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 0/4] soc: imx: add scu firmware api support
>>
>> Hi,
>>
>> On Sat, Apr 28, 2018 at 02:46:12AM +0800, 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 library calls
>>> 1) Implementation of the IPC functions based on MUs (client side).
>>> 2) SCU firmware services APIs implementation ased on RPC calls which
>>>    are mostly generated by SCU firmware
>>
>>
>> Hm... I fail to see the difference between remoteproc, virtio, rpmsg and
>> other existing frameworks in kernel with this functionality. Why do we need
>> vendor/soc specific framework once again?
>>
> 
> Hmm.. It seems like not a vendor specific framework...
> It mainly SCU firmware APIs generated by SCU firmware script for
> Linux OS components to use.
> And those APIs are implemented base on MU simple polling mode
> With better performance.
> 
> NXP internally has tried mailbox but got performance regression,
> So we're still not sure whether Mailbox is quit suitable for i.MX
> SCU Firmware. Needs more time to investigate.
> 
> And I'm still not quite familiar with remoteproc, virtio, rpmsg.
> May need spend more time to investigate later.
> And it would be good if you can provide suggestions and sharing
> Informations about them.
> e.g. what's requirement of i.MX to switch to them? Benefit? Or 
> something else valuable?

Before starting with suggestions I would like to get some more
information :)

> "And those APIs are implemented base on MU simple polling mode
> With better performance."

was this implementation tested in complex system configuration? Mostly
polling is a one task optimization. All other corners of the system
usually get dramatic performance degradation.


> "All other files actually are generated by SCU firmware script
> automatically for Linux OS. They're tightly couple with SCU firmware
> side implementation. In order to gain better maintainability (easy
> upgrade and sync with SCU firmware), we're trying to not change those
> APIs too much unless we have strong reasons."

Means, updates and changes of Firmware API are already included by
design? Since it is start-of-life for this SoC family, number of API
changes is expected risk?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180503/37fecab4/attachment.sig>

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

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel at pengutronix.de]
> Sent: Thursday, May 3, 2018 2:24 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: 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; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 0/4] soc: imx: add scu firmware api support
> 
> 
> 
> On 02.05.2018 20:54, A.s. Dong wrote:
> >> -----Original Message-----
> >> From: Oleksij Rempel [mailto:o.rempel at pengutronix.de]
> >> Sent: Tuesday, May 1, 2018 1:59 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 0/4] soc: imx: add scu firmware api support
> >>
> >> Hi,
> >>
> >> On Sat, Apr 28, 2018 at 02:46:12AM +0800, 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 library calls
> >>> 1) Implementation of the IPC functions based on MUs (client side).
> >>> 2) SCU firmware services APIs implementation ased on RPC calls which
> >>>    are mostly generated by SCU firmware
> >>
> >>
> >> Hm... I fail to see the difference between remoteproc, virtio, rpmsg
> >> and other existing frameworks in kernel with this functionality. Why
> >> do we need vendor/soc specific framework once again?
> >>
> >
> > Hmm.. It seems like not a vendor specific framework...
> > It mainly SCU firmware APIs generated by SCU firmware script for Linux
> > OS components to use.
> > And those APIs are implemented base on MU simple polling mode With
> > better performance.
> >
> > NXP internally has tried mailbox but got performance regression, So
> > we're still not sure whether Mailbox is quit suitable for i.MX SCU
> > Firmware. Needs more time to investigate.
> >
> > And I'm still not quite familiar with remoteproc, virtio, rpmsg.
> > May need spend more time to investigate later.
> > And it would be good if you can provide suggestions and sharing
> > Informations about them.
> > e.g. what's requirement of i.MX to switch to them? Benefit? Or
> > something else valuable?
> 
> Before starting with suggestions I would like to get some more information :)
> 
> > "And those APIs are implemented base on MU simple polling mode With
> > better performance."
> 
> was this implementation tested in complex system configuration? Mostly
> polling is a one task optimization. All other corners of the system usually get
> dramatic performance degradation.
> 

Yes, the whole MX8QM/QXP system actually is very complicated already when
busy running with Graphic & Video IO features (e.g. GPU/VPU/HDMI and etc.)
But we did not make specific comparing of another IPC handling way. E.g. mailbox
Or rpmsg.

We actually tried mailbox before but see boot time regression(no much more features test).
AFAIK SCU message handling usually is quite fast in micro seconds. So simple polling may
be a better choice.

If we want to switch to Mailbox or something else, we probably need do robust investigation
to void any performance regression. That may need a lot time.

So maybe we can go with the current simple way to make QXP upstreamed first.
Then we can have more time to investigate it carefully later, and better for comparison.

Anyway, even if we want to switch to Mailbox, that seems to be just a IPC implementation
Change(ipc.c), won't affect other part. So looks like a applicable way to me
which won't block QXP support too long. 

> 
> > "All other files actually are generated by SCU firmware script
> > automatically for Linux OS. They're tightly couple with SCU firmware
> > side implementation. In order to gain better maintainability (easy
> > upgrade and sync with SCU firmware), we're trying to not change those
> > APIs too much unless we have strong reasons."
> 
> Means, updates and changes of Firmware API are already included by design?
> Since it is start-of-life for this SoC family, number of API changes is expected
> risk?

The SCU firmware protocol (API) usually won't change.  It did happen during
early time development internally. But it's much stable now after years
of evolution . From what I see, we may need update it if we need a new feature
supported by a new version of SCU firmware.

Regards
Dong Aisheng

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

[Sascha Hauer]

On Wed, May 02, 2018 at 06:40:03PM +0000, A.s. Dong wrote:
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Wednesday, May 2, 2018 6:04 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 4/4] soc: imx: add SC firmware IPC and APIs
> > 
> > On Sat, Apr 28, 2018 at 02:46:16AM +0800, Dong Aisheng wrote:
> > > +/* Initialization of the MU code. */
> > > +int __init imx8_scu_init(void)
> > > +{
> > > +	struct device_node *np, *mu_np;
> > > +	struct resource mu_res;
> > > +	sc_err_t sci_err;
> > > +	int ret;
> > > +
> > > +	if (!of_machine_is_compatible("fsl,imx8qxp"))
> > > +		return 0;
> > > +
> > > +	np = of_find_compatible_node(NULL, NULL, "nxp,imx8qxp-scu");
> > > +	if (!np)
> > > +		return -ENODEV;
> > > +
> > > +	mu_np = of_parse_phandle(np, "fsl,mu", 0);
> > > +	if (WARN_ON(!mu_np))
> > > +		return -EINVAL;
> > > +
> > > +	ret = of_address_to_resource(mu_np, 0, &mu_res);
> > > +	if (WARN_ON(ret))
> > > +		return -EINVAL;
> > > +
> > > +	/* we use mu physical address as IPC communication channel ID */
> > > +	sci_err = sc_ipc_open(&scu_ipc_handle, (sc_ipc_id_t)(mu_res.start));
> > > +	if (sci_err != SC_ERR_NONE) {
> > > +		pr_err("Cannot open MU channel to SCU\n");
> > > +		return sci_err;
> > > +	};
> > 
> > Introducing private error codes always has the risk of just forwarding them as
> > errno codes as done here.
> > 
> 
> Yes, you're right.
> We probably could do the same as scpi_to_linux_errno in
> drivers/firmware/arm_scpi.c.
> However, may can't fix the issue permanently.
> 
> > > +
> > > +	of_node_put(mu_np);
> > > +
> > > +	pr_info("NXP i.MX SCU Initialized (scu_ipc %u)\n", scu_ipc_handle);
> > > +
> > > +	return 0;
> > > +}
> > > +early_initcall(imx8_scu_init);
> > 
> > This code shows that the separate 'scu' device node shouldn't exist. It is only
> > used as a stepping stone to find the 'mu' node. Instead of providing a proper
> > driver for the 'mu' node the scu code registers it with its physical address.
> > I don't think it makes sense to separate mu and scu code in this way.
> > 
> 
> I agree that may not look quite well. 
> It mainly because we want to use the MU physical address as a MU ID. 
> (can't use virtual address as sc_ipc_id_t is u32 defined by SCU firmware.
> 
> If you have any better suggestion please let me know.

What I'm suggesting is a single node:

	scu at 5d1b0000 {
		compatible = "fsl,imx8qxp-scu";
		reg = <0x0 0x5d1b0000 0x0 0x10000>;
	};

Attach your code to this one, without any further separation between mu and scu code.

We discussed this internally and came to the conclusion that the SCU is
not a generic user of a MU. The MU is designed to work together with a piece
of SRAM to form a mailbox system. Instead of working as designed the SCU
morses the messages through the doorbell (what the MU really is). For
anything that uses the MU in the way it is designed I would suggest
using the mailbox interface from drivers/mailbox/ along with the
binding from Documentation/devicetree/bindings/mailbox/mailbox.txt.

In the way I suggest there is no need for any MU ID.

> 
> > > +#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)])
> > 
> > These macros only make the code less readable, please drop them. Plain
> > m->version is easier to read and forces the reader through less
> > indirections. IMO m->DATA.u32[0] = foo; m->DATA.u8[4] = bar is still better
> > readable than doing the division by size in macros.
> > 
> 
> I somehow agree with you.
> See below...
> 
> > > +typedef enum sc_rpc_async_state_e {
> > > +	SC_RPC_ASYNC_STATE_RD_START = 0,
> > > +	SC_RPC_ASYNC_STATE_RD_ACTIVE = 1,
> > > +	SC_RPC_ASYNC_STATE_RD_DONE = 2,
> > > +	SC_RPC_ASYNC_STATE_WR_START = 3,
> > > +	SC_RPC_ASYNC_STATE_WR_ACTIVE = 4,
> > > +	SC_RPC_ASYNC_STATE_WR_DONE = 5,
> > > +} sc_rpc_async_state_t;
> > > +
> > > +typedef struct sc_rpc_async_msg_s {
> > > +	sc_rpc_async_state_t state;
> > > +	uint8_t wordIdx;
> > > +	sc_rpc_msg_t msg;
> > > +	uint32_t timeStamp;
> > > +} sc_rpc_async_msg_t;
> > 
> > We normally do not typedef struct types. Any good reasons to do so here?
> > 
> 
> Yes, I did not some checkpatch warnings about this.
> 
> For this patch, I only implemented IPC function calls based on MU which is in
> drivers/soc/imx/sc/main/ipc.c. All other files actually are generated by SCU firmware
> script automatically for Linux OS. They're tightly couple with SCU firmware side
> implementation. In order to gain better maintainability (easy upgrade and sync with
> SCU firmware), we're trying to not change those APIs too much unless we have strong
> reasons. Does it make sense to you?

Hm, I'm not suggesting to change the API, only the header files. In
another mail you say that the SCU firmware is stable, so there shouldn't
be any strong need to sync the header files from your internal
repositories, right?
Besides, you could also update your internal repositories with the
suggestions I make ;)

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 4/4] soc: imx: add SC firmware IPC and APIs

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Thursday, May 3, 2018 7:06 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 4/4] soc: imx: add SC firmware IPC and APIs
> 
> On Wed, May 02, 2018 at 06:40:03PM +0000, A.s. Dong wrote:
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > Sent: Wednesday, May 2, 2018 6:04 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 4/4] soc: imx: add SC firmware IPC and APIs
> > >
> > > On Sat, Apr 28, 2018 at 02:46:16AM +0800, Dong Aisheng wrote:
> > > > +/* Initialization of the MU code. */ int __init
> > > > +imx8_scu_init(void) {
> > > > +	struct device_node *np, *mu_np;
> > > > +	struct resource mu_res;
> > > > +	sc_err_t sci_err;
> > > > +	int ret;
> > > > +
> > > > +	if (!of_machine_is_compatible("fsl,imx8qxp"))
> > > > +		return 0;
> > > > +
> > > > +	np = of_find_compatible_node(NULL, NULL, "nxp,imx8qxp-scu");
> > > > +	if (!np)
> > > > +		return -ENODEV;
> > > > +
> > > > +	mu_np = of_parse_phandle(np, "fsl,mu", 0);
> > > > +	if (WARN_ON(!mu_np))
> > > > +		return -EINVAL;
> > > > +
> > > > +	ret = of_address_to_resource(mu_np, 0, &mu_res);
> > > > +	if (WARN_ON(ret))
> > > > +		return -EINVAL;
> > > > +
> > > > +	/* we use mu physical address as IPC communication channel ID */
> > > > +	sci_err = sc_ipc_open(&scu_ipc_handle, (sc_ipc_id_t)(mu_res.start));
> > > > +	if (sci_err != SC_ERR_NONE) {
> > > > +		pr_err("Cannot open MU channel to SCU\n");
> > > > +		return sci_err;
> > > > +	};
> > >
> > > Introducing private error codes always has the risk of just
> > > forwarding them as errno codes as done here.
> > >
> >
> > Yes, you're right.
> > We probably could do the same as scpi_to_linux_errno in
> > drivers/firmware/arm_scpi.c.
> > However, may can't fix the issue permanently.
> >
> > > > +
> > > > +	of_node_put(mu_np);
> > > > +
> > > > +	pr_info("NXP i.MX SCU Initialized (scu_ipc %u)\n",
> > > > +scu_ipc_handle);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +early_initcall(imx8_scu_init);
> > >
> > > This code shows that the separate 'scu' device node shouldn't exist.
> > > It is only used as a stepping stone to find the 'mu' node. Instead
> > > of providing a proper driver for the 'mu' node the scu code registers it
> with its physical address.
> > > I don't think it makes sense to separate mu and scu code in this way.
> > >
> >
> > I agree that may not look quite well.
> > It mainly because we want to use the MU physical address as a MU ID.
> > (can't use virtual address as sc_ipc_id_t is u32 defined by SCU firmware.
> >
> > If you have any better suggestion please let me know.
> 
> What I'm suggesting is a single node:
> 
> 	scu at 5d1b0000 {
> 		compatible = "fsl,imx8qxp-scu";
> 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> 	};
> 
> Attach your code to this one, without any further separation between mu
> and scu code.
> 

A bit confused. You're suggesting a single node here without mu or mailbox node
phandle in it? Then how SCU use MU?

> We discussed this internally and came to the conclusion that the SCU is not a
> generic user of a MU. The MU is designed to work together with a piece of
> SRAM to form a mailbox system. Instead of working as designed the SCU
> morses the messages through the doorbell (what the MU really is). For
> anything that uses the MU in the way it is designed I would suggest using the
> mailbox interface from drivers/mailbox/ along with the binding from
> Documentation/devicetree/bindings/mailbox/mailbox.txt.
> 
> In the way I suggest there is no need for any MU ID.
> 

So you're suggesting switch to use mailbox instead of private MU library
function calls?
Something like:
        scu {
                compatible = "nxp,imx8qxp-scu", "simple-bus";
                mboxes = <&mailbox 0>;         
        }
Then IPC is implemented based on mailbox?

As I replied Oleksij Rempel in another mail in this thread, we've tried mailbox
but got performance regression issue and need more time to investigate
whether it's really quite suitable for i.MX SCU firmware as SCU handling
message quite fast in micro seconds. (Mailbox polling method has much
more latency than current MU sample polling we used)
and we want to avoid the SCU firmware API changes.

> >
> > > > +#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)])
> > >
> > > These macros only make the code less readable, please drop them.
> > > Plain
> > > m->version is easier to read and forces the reader through less
> > > indirections. IMO m->DATA.u32[0] = foo; m->DATA.u8[4] = bar is still
> > > better readable than doing the division by size in macros.
> > >
> >
> > I somehow agree with you.
> > See below...
> >
> > > > +typedef enum sc_rpc_async_state_e {
> > > > +	SC_RPC_ASYNC_STATE_RD_START = 0,
> > > > +	SC_RPC_ASYNC_STATE_RD_ACTIVE = 1,
> > > > +	SC_RPC_ASYNC_STATE_RD_DONE = 2,
> > > > +	SC_RPC_ASYNC_STATE_WR_START = 3,
> > > > +	SC_RPC_ASYNC_STATE_WR_ACTIVE = 4,
> > > > +	SC_RPC_ASYNC_STATE_WR_DONE = 5,
> > > > +} sc_rpc_async_state_t;
> > > > +
> > > > +typedef struct sc_rpc_async_msg_s {
> > > > +	sc_rpc_async_state_t state;
> > > > +	uint8_t wordIdx;
> > > > +	sc_rpc_msg_t msg;
> > > > +	uint32_t timeStamp;
> > > > +} sc_rpc_async_msg_t;
> > >
> > > We normally do not typedef struct types. Any good reasons to do so here?
> > >
> >
> > Yes, I did not some checkpatch warnings about this.
> >
> > For this patch, I only implemented IPC function calls based on MU
> > which is in drivers/soc/imx/sc/main/ipc.c. All other files actually
> > are generated by SCU firmware script automatically for Linux OS.
> > They're tightly couple with SCU firmware side implementation. In order
> > to gain better maintainability (easy upgrade and sync with SCU
> > firmware), we're trying to not change those APIs too much unless we have
> strong reasons. Does it make sense to you?
> 
> Hm, I'm not suggesting to change the API, only the header files. In another
> mail you say that the SCU firmware is stable, so there shouldn't be any strong
> need to sync the header files from your internal repositories, right?
> Besides, you could also update your internal repositories with the
> suggestions I make ;)
> 

Yes, I will try to update it if no big side effect.
Thanks for the suggestion.

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&data=02%7C01%7Caisheng.dong%40nxp.com%7Cfeb
> 56093d6964d31a97c08d5b0e5e28c%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C636609423746219869&sdata=l6hKOW1WakB4tpB4%2Bfcp6dLt4t
> 97Q51Feb%2FTkKHZF3g%3D&reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Sascha Hauer]

On Thu, May 03, 2018 at 12:29:40PM +0000, A.s. Dong wrote:
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Thursday, May 3, 2018 7:06 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 4/4] soc: imx: add SC firmware IPC and APIs
> > 
> > On Wed, May 02, 2018 at 06:40:03PM +0000, A.s. Dong wrote:
> > > > -----Original Message-----
> > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > Sent: Wednesday, May 2, 2018 6:04 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 4/4] soc: imx: add SC firmware IPC and APIs
> > > >
> > > > On Sat, Apr 28, 2018 at 02:46:16AM +0800, Dong Aisheng wrote:
> > > > > +/* Initialization of the MU code. */ int __init
> > > > > +imx8_scu_init(void) {
> > > > > +	struct device_node *np, *mu_np;
> > > > > +	struct resource mu_res;
> > > > > +	sc_err_t sci_err;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (!of_machine_is_compatible("fsl,imx8qxp"))
> > > > > +		return 0;
> > > > > +
> > > > > +	np = of_find_compatible_node(NULL, NULL, "nxp,imx8qxp-scu");
> > > > > +	if (!np)
> > > > > +		return -ENODEV;
> > > > > +
> > > > > +	mu_np = of_parse_phandle(np, "fsl,mu", 0);
> > > > > +	if (WARN_ON(!mu_np))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	ret = of_address_to_resource(mu_np, 0, &mu_res);
> > > > > +	if (WARN_ON(ret))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	/* we use mu physical address as IPC communication channel ID */
> > > > > +	sci_err = sc_ipc_open(&scu_ipc_handle, (sc_ipc_id_t)(mu_res.start));
> > > > > +	if (sci_err != SC_ERR_NONE) {
> > > > > +		pr_err("Cannot open MU channel to SCU\n");
> > > > > +		return sci_err;
> > > > > +	};
> > > >
> > > > Introducing private error codes always has the risk of just
> > > > forwarding them as errno codes as done here.
> > > >
> > >
> > > Yes, you're right.
> > > We probably could do the same as scpi_to_linux_errno in
> > > drivers/firmware/arm_scpi.c.
> > > However, may can't fix the issue permanently.
> > >
> > > > > +
> > > > > +	of_node_put(mu_np);
> > > > > +
> > > > > +	pr_info("NXP i.MX SCU Initialized (scu_ipc %u)\n",
> > > > > +scu_ipc_handle);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +early_initcall(imx8_scu_init);
> > > >
> > > > This code shows that the separate 'scu' device node shouldn't exist.
> > > > It is only used as a stepping stone to find the 'mu' node. Instead
> > > > of providing a proper driver for the 'mu' node the scu code registers it
> > with its physical address.
> > > > I don't think it makes sense to separate mu and scu code in this way.
> > > >
> > >
> > > I agree that may not look quite well.
> > > It mainly because we want to use the MU physical address as a MU ID.
> > > (can't use virtual address as sc_ipc_id_t is u32 defined by SCU firmware.
> > >
> > > If you have any better suggestion please let me know.
> > 
> > What I'm suggesting is a single node:
> > 
> > 	scu at 5d1b0000 {
> > 		compatible = "fsl,imx8qxp-scu";
> > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > 	};
> > 
> > Attach your code to this one, without any further separation between mu
> > and scu code.
> > 
> 
> A bit confused. You're suggesting a single node here without mu or mailbox node
> phandle in it? Then how SCU use MU?

ioremap the address and mu_receive_msg()/mu_send_msg() on it, just like
you do already.

> 
> > We discussed this internally and came to the conclusion that the SCU is not a
> > generic user of a MU. The MU is designed to work together with a piece of
> > SRAM to form a mailbox system. Instead of working as designed the SCU
> > morses the messages through the doorbell (what the MU really is). For
> > anything that uses the MU in the way it is designed I would suggest using the
> > mailbox interface from drivers/mailbox/ along with the binding from
> > Documentation/devicetree/bindings/mailbox/mailbox.txt.
> > 
> > In the way I suggest there is no need for any MU ID.
> > 
> 
> So you're suggesting switch to use mailbox instead of private MU library
> function calls?
> Something like:
>         scu {
>                 compatible = "nxp,imx8qxp-scu", "simple-bus";
>                 mboxes = <&mailbox 0>;         
>         }
> Then IPC is implemented based on mailbox?
> 
> As I replied Oleksij Rempel in another mail in this thread, we've tried mailbox
> but got performance regression issue and need more time to investigate
> whether it's really quite suitable for i.MX SCU firmware as SCU handling
> message quite fast in micro seconds. (Mailbox polling method has much
> more latency than current MU sample polling we used)
> and we want to avoid the SCU firmware API changes.

I am not suggesting to do mailboxing (using shared memory) for the SCU.
I am also not suggesting any API update for the SCU communication.
I am just mentioning that doing mailboxing is the way the MU was
originally designed for. Looking at the reference manual I see many MUs
on the i.MX8. I guess most of them are for communication between the
different cores on the system. For these it's probably worth writing
some generic MU driver. The way the MU is used for communication with
the SCU though is so special that it's not worth writing a generic
driver, so just integrate the MU access functions in the SCU code.

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 4/4] soc: imx: add SC firmware IPC and APIs

[A.s. Dong]

Hi Sascha,

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Thursday, May 3, 2018 8:41 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 4/4] soc: imx: add SC firmware IPC and APIs
> 
> On Thu, May 03, 2018 at 12:29:40PM +0000, A.s. Dong wrote:
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > Sent: Thursday, May 3, 2018 7:06 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 4/4] soc: imx: add SC firmware IPC and APIs
> > >
> > > On Wed, May 02, 2018 at 06:40:03PM +0000, A.s. Dong wrote:
> > > > > -----Original Message-----
> > > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > > Sent: Wednesday, May 2, 2018 6:04 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 4/4] soc: imx: add SC firmware IPC and APIs
> > > > >
> > > > > On Sat, Apr 28, 2018 at 02:46:16AM +0800, Dong Aisheng wrote:
> > > > > > +/* Initialization of the MU code. */ int __init
> > > > > > +imx8_scu_init(void) {
> > > > > > +	struct device_node *np, *mu_np;
> > > > > > +	struct resource mu_res;
> > > > > > +	sc_err_t sci_err;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	if (!of_machine_is_compatible("fsl,imx8qxp"))
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	np = of_find_compatible_node(NULL, NULL, "nxp,imx8qxp-
> scu");
> > > > > > +	if (!np)
> > > > > > +		return -ENODEV;
> > > > > > +
> > > > > > +	mu_np = of_parse_phandle(np, "fsl,mu", 0);
> > > > > > +	if (WARN_ON(!mu_np))
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	ret = of_address_to_resource(mu_np, 0, &mu_res);
> > > > > > +	if (WARN_ON(ret))
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	/* we use mu physical address as IPC communication channel
> ID */
> > > > > > +	sci_err = sc_ipc_open(&scu_ipc_handle,
> (sc_ipc_id_t)(mu_res.start));
> > > > > > +	if (sci_err != SC_ERR_NONE) {
> > > > > > +		pr_err("Cannot open MU channel to SCU\n");
> > > > > > +		return sci_err;
> > > > > > +	};
> > > > >
> > > > > Introducing private error codes always has the risk of just
> > > > > forwarding them as errno codes as done here.
> > > > >
> > > >
> > > > Yes, you're right.
> > > > We probably could do the same as scpi_to_linux_errno in
> > > > drivers/firmware/arm_scpi.c.
> > > > However, may can't fix the issue permanently.
> > > >
> > > > > > +
> > > > > > +	of_node_put(mu_np);
> > > > > > +
> > > > > > +	pr_info("NXP i.MX SCU Initialized (scu_ipc %u)\n",
> > > > > > +scu_ipc_handle);
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +early_initcall(imx8_scu_init);
> > > > >
> > > > > This code shows that the separate 'scu' device node shouldn't exist.
> > > > > It is only used as a stepping stone to find the 'mu' node.
> > > > > Instead of providing a proper driver for the 'mu' node the scu
> > > > > code registers it
> > > with its physical address.
> > > > > I don't think it makes sense to separate mu and scu code in this way.
> > > > >
> > > >
> > > > I agree that may not look quite well.
> > > > It mainly because we want to use the MU physical address as a MU ID.
> > > > (can't use virtual address as sc_ipc_id_t is u32 defined by SCU firmware.
> > > >
> > > > If you have any better suggestion please let me know.
> > >
> > > What I'm suggesting is a single node:
> > >
> > > 	scu at 5d1b0000 {
> > > 		compatible = "fsl,imx8qxp-scu";
> > > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > > 	};
> > >
> > > Attach your code to this one, without any further separation between
> > > mu and scu code.
> > >
> >
> > A bit confused. You're suggesting a single node here without mu or
> > mailbox node phandle in it? Then how SCU use MU?
> 
> ioremap the address and mu_receive_msg()/mu_send_msg() on it, just like
> you do already.
> 
> >
> > > We discussed this internally and came to the conclusion that the SCU
> > > is not a generic user of a MU. The MU is designed to work together
> > > with a piece of SRAM to form a mailbox system. Instead of working as
> > > designed the SCU morses the messages through the doorbell (what the
> > > MU really is). For anything that uses the MU in the way it is
> > > designed I would suggest using the mailbox interface from
> > > drivers/mailbox/ along with the binding from
> Documentation/devicetree/bindings/mailbox/mailbox.txt.
> > >
> > > In the way I suggest there is no need for any MU ID.
> > >
> >
> > So you're suggesting switch to use mailbox instead of private MU
> > library function calls?
> > Something like:
> >         scu {
> >                 compatible = "nxp,imx8qxp-scu", "simple-bus";
> >                 mboxes = <&mailbox 0>;
> >         }
> > Then IPC is implemented based on mailbox?
> >
> > As I replied Oleksij Rempel in another mail in this thread, we've
> > tried mailbox but got performance regression issue and need more time
> > to investigate whether it's really quite suitable for i.MX SCU
> > firmware as SCU handling message quite fast in micro seconds. (Mailbox
> > polling method has much more latency than current MU sample polling we
> > used) and we want to avoid the SCU firmware API changes.
> 
> I am not suggesting to do mailboxing (using shared memory) for the SCU.
> I am also not suggesting any API update for the SCU communication.
> I am just mentioning that doing mailboxing is the way the MU was originally
> designed for. Looking at the reference manual I see many MUs on the i.MX8.
> I guess most of them are for communication between the different cores on
> the system. For these it's probably worth writing some generic MU driver.
> The way the MU is used for communication with the SCU though is so special
> that it's not worth writing a generic driver, so just integrate the MU access
> functions in the SCU code.
> 

I understand it.

One problem of the way you suggested may be that:
If we doing like below, we may lose flexibility to change the MU used
for SCU firmware communication.
	scu at 5d1b0000 {
		compatible = "fsl,imx8qxp-scu";
		reg = <0x0 0x5d1b0000 0x0 0x10000>;
	};

And current design is that the system supports multiple MU channels used
by various users at the same time, e.g. SCU, Power Domain, Clock and others.
User can flexibly change it under their nodes: And each MU channel is protected
by their private lock and not affect each others.

e.g.
        scu {
                compatible = "nxp,imx8qxp-scu", "simple-bus";
                fsl,mu = <&lsio_mu0>;

                clk: clk {
                        compatible = "fsl,imx8qxp-clk";
                        #clock-cells = <1>;
                };

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

                imx8qx-pm {
                        #address-cells = <1>;
                        #size-cells = <0>;
                        fsl,mu = <&lsio_mu4>;
	.............
        }

The default code only uses MU0 which is used by SCU.

The change may affect this design. Any ideas?
Do you think we can keep the current way?

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&data=02%7C01%7Caisheng.dong%40nxp.com%7C266
> 24a5c38e5488a80b708d5b0f3107b%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C636609480354404338&sdata=XP%2BYdt9I7zURrQun2jRbempLhC
> XrYtMVMb3dEWrZuro%3D&reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[A.s. Dong]

Hi Sascha,

> -----Original Message-----
> From: A.s. Dong
> Sent: Thursday, May 24, 2018 4:56 PM
> To: Sascha Hauer <s.hauer@pengutronix.de>
> 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 4/4] soc: imx: add SC firmware IPC and APIs
> 
> Hi Sascha,
> 
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Thursday, May 3, 2018 8:41 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 4/4] soc: imx: add SC firmware IPC and APIs
> >
> > On Thu, May 03, 2018 at 12:29:40PM +0000, A.s. Dong wrote:
> > > > -----Original Message-----
> > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > Sent: Thursday, May 3, 2018 7:06 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 4/4] soc: imx: add SC firmware IPC and APIs
> > > >
> > > > On Wed, May 02, 2018 at 06:40:03PM +0000, A.s. Dong wrote:
> > > > > > -----Original Message-----
> > > > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > > > Sent: Wednesday, May 2, 2018 6:04 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 4/4] soc: imx: add SC firmware IPC and
> > > > > > APIs
> > > > > >
> > > > > > On Sat, Apr 28, 2018 at 02:46:16AM +0800, Dong Aisheng wrote:
> > > > > > > +/* Initialization of the MU code. */ int __init
> > > > > > > +imx8_scu_init(void) {
> > > > > > > +	struct device_node *np, *mu_np;
> > > > > > > +	struct resource mu_res;
> > > > > > > +	sc_err_t sci_err;
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	if (!of_machine_is_compatible("fsl,imx8qxp"))
> > > > > > > +		return 0;
> > > > > > > +
> > > > > > > +	np = of_find_compatible_node(NULL, NULL, "nxp,imx8qxp-
> > scu");
> > > > > > > +	if (!np)
> > > > > > > +		return -ENODEV;
> > > > > > > +
> > > > > > > +	mu_np = of_parse_phandle(np, "fsl,mu", 0);
> > > > > > > +	if (WARN_ON(!mu_np))
> > > > > > > +		return -EINVAL;
> > > > > > > +
> > > > > > > +	ret = of_address_to_resource(mu_np, 0, &mu_res);
> > > > > > > +	if (WARN_ON(ret))
> > > > > > > +		return -EINVAL;
> > > > > > > +
> > > > > > > +	/* we use mu physical address as IPC communication channel
> > ID */
> > > > > > > +	sci_err = sc_ipc_open(&scu_ipc_handle,
> > (sc_ipc_id_t)(mu_res.start));
> > > > > > > +	if (sci_err != SC_ERR_NONE) {
> > > > > > > +		pr_err("Cannot open MU channel to SCU\n");
> > > > > > > +		return sci_err;
> > > > > > > +	};
> > > > > >
> > > > > > Introducing private error codes always has the risk of just
> > > > > > forwarding them as errno codes as done here.
> > > > > >
> > > > >
> > > > > Yes, you're right.
> > > > > We probably could do the same as scpi_to_linux_errno in
> > > > > drivers/firmware/arm_scpi.c.
> > > > > However, may can't fix the issue permanently.
> > > > >
> > > > > > > +
> > > > > > > +	of_node_put(mu_np);
> > > > > > > +
> > > > > > > +	pr_info("NXP i.MX SCU Initialized (scu_ipc %u)\n",
> > > > > > > +scu_ipc_handle);
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +early_initcall(imx8_scu_init);
> > > > > >
> > > > > > This code shows that the separate 'scu' device node shouldn't exist.
> > > > > > It is only used as a stepping stone to find the 'mu' node.
> > > > > > Instead of providing a proper driver for the 'mu' node the scu
> > > > > > code registers it
> > > > with its physical address.
> > > > > > I don't think it makes sense to separate mu and scu code in this way.
> > > > > >
> > > > >
> > > > > I agree that may not look quite well.
> > > > > It mainly because we want to use the MU physical address as a MU ID.
> > > > > (can't use virtual address as sc_ipc_id_t is u32 defined by SCU
> firmware.
> > > > >
> > > > > If you have any better suggestion please let me know.
> > > >
> > > > What I'm suggesting is a single node:
> > > >
> > > > 	scu at 5d1b0000 {
> > > > 		compatible = "fsl,imx8qxp-scu";
> > > > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > > > 	};
> > > >
> > > > Attach your code to this one, without any further separation
> > > > between mu and scu code.
> > > >
> > >
> > > A bit confused. You're suggesting a single node here without mu or
> > > mailbox node phandle in it? Then how SCU use MU?
> >
> > ioremap the address and mu_receive_msg()/mu_send_msg() on it, just
> > like you do already.
> >
> > >
> > > > We discussed this internally and came to the conclusion that the
> > > > SCU is not a generic user of a MU. The MU is designed to work
> > > > together with a piece of SRAM to form a mailbox system. Instead of
> > > > working as designed the SCU morses the messages through the
> > > > doorbell (what the MU really is). For anything that uses the MU in
> > > > the way it is designed I would suggest using the mailbox interface
> > > > from drivers/mailbox/ along with the binding from
> > Documentation/devicetree/bindings/mailbox/mailbox.txt.
> > > >
> > > > In the way I suggest there is no need for any MU ID.
> > > >
> > >
> > > So you're suggesting switch to use mailbox instead of private MU
> > > library function calls?
> > > Something like:
> > >         scu {
> > >                 compatible = "nxp,imx8qxp-scu", "simple-bus";
> > >                 mboxes = <&mailbox 0>;
> > >         }
> > > Then IPC is implemented based on mailbox?
> > >
> > > As I replied Oleksij Rempel in another mail in this thread, we've
> > > tried mailbox but got performance regression issue and need more
> > > time to investigate whether it's really quite suitable for i.MX SCU
> > > firmware as SCU handling message quite fast in micro seconds.
> > > (Mailbox polling method has much more latency than current MU sample
> > > polling we
> > > used) and we want to avoid the SCU firmware API changes.
> >
> > I am not suggesting to do mailboxing (using shared memory) for the SCU.
> > I am also not suggesting any API update for the SCU communication.
> > I am just mentioning that doing mailboxing is the way the MU was
> > originally designed for. Looking at the reference manual I see many MUs on
> the i.MX8.
> > I guess most of them are for communication between the different cores
> > on the system. For these it's probably worth writing some generic MU
> driver.
> > The way the MU is used for communication with the SCU though is so
> > special that it's not worth writing a generic driver, so just
> > integrate the MU access functions in the SCU code.
> >
> 
> I understand it.
> 
> One problem of the way you suggested may be that:
> If we doing like below, we may lose flexibility to change the MU used for SCU
> firmware communication.
> 	scu at 5d1b0000 {
> 		compatible = "fsl,imx8qxp-scu";
> 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> 	};
> 
> And current design is that the system supports multiple MU channels used
> by various users at the same time, e.g. SCU, Power Domain, Clock and others.
> User can flexibly change it under their nodes: And each MU channel is
> protected by their private lock and not affect each others.
> 
> e.g.
>         scu {
>                 compatible = "nxp,imx8qxp-scu", "simple-bus";
>                 fsl,mu = <&lsio_mu0>;
> 
>                 clk: clk {
>                         compatible = "fsl,imx8qxp-clk";
>                         #clock-cells = <1>;
>                 };
> 
>                 iomuxc: iomuxc {
>                         compatible = "fsl,imx8qxp-iomuxc";
>                         fsl,mu = <&lsio_mu3>;
>                 };
> 
>                 imx8qx-pm {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>                         fsl,mu = <&lsio_mu4>;
> 	.............
>         }
> 
> The default code only uses MU0 which is used by SCU.
> 
> The change may affect this design. Any ideas?
> Do you think we can keep the current way?
> 

Any comments about this?

Regards
Dong Aisheng

> 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&data=02%7C01%7Caisheng.dong%40nxp.com%7C266
> > 24a5c38e5488a80b708d5b0f3107b%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> >
> 7C0%7C0%7C636609480354404338&sdata=XP%2BYdt9I7zURrQun2jRbempLhC
> > XrYtMVMb3dEWrZuro%3D&reserved=0  |
> > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[A.s. Dong]

Hi Sascha,

> -----Original Message-----
> From: A.s. Dong
> Sent: Monday, May 28, 2018 5:25 PM
> To: 'Sascha Hauer' <s.hauer@pengutronix.de>
> Cc: 'linux-arm-kernel at lists.infradead.org' <linux-arm-
> kernel at lists.infradead.org>; 'dongas86 at gmail.com' <dongas86@gmail.com>;
> dl-linux-imx <linux-imx@nxp.com>; 'kernel at pengutronix.de'
> <kernel@pengutronix.de>; Fabio Estevam <fabio.estevam@nxp.com>;
> 'shawnguo at kernel.org' <shawnguo@kernel.org>
> Subject: RE: [PATCH 4/4] soc: imx: add SC firmware IPC and APIs
> 
...

> > > > > We discussed this internally and came to the conclusion that the
> > > > > SCU is not a generic user of a MU. The MU is designed to work
> > > > > together with a piece of SRAM to form a mailbox system. Instead
> > > > > of working as designed the SCU morses the messages through the
> > > > > doorbell (what the MU really is). For anything that uses the MU
> > > > > in the way it is designed I would suggest using the mailbox
> > > > > interface from drivers/mailbox/ along with the binding from
> > > Documentation/devicetree/bindings/mailbox/mailbox.txt.
> > > > >
> > > > > In the way I suggest there is no need for any MU ID.
> > > > >
> > > >
> > > > So you're suggesting switch to use mailbox instead of private MU
> > > > library function calls?
> > > > Something like:
> > > >         scu {
> > > >                 compatible = "nxp,imx8qxp-scu", "simple-bus";
> > > >                 mboxes = <&mailbox 0>;
> > > >         }
> > > > Then IPC is implemented based on mailbox?
> > > >
> > > > As I replied Oleksij Rempel in another mail in this thread, we've
> > > > tried mailbox but got performance regression issue and need more
> > > > time to investigate whether it's really quite suitable for i.MX
> > > > SCU firmware as SCU handling message quite fast in micro seconds.
> > > > (Mailbox polling method has much more latency than current MU
> > > > sample polling we
> > > > used) and we want to avoid the SCU firmware API changes.
> > >
> > > I am not suggesting to do mailboxing (using shared memory) for the SCU.
> > > I am also not suggesting any API update for the SCU communication.
> > > I am just mentioning that doing mailboxing is the way the MU was
> > > originally designed for. Looking at the reference manual I see many
> > > MUs on
> > the i.MX8.
> > > I guess most of them are for communication between the different
> > > cores on the system. For these it's probably worth writing some
> > > generic MU
> > driver.
> > > The way the MU is used for communication with the SCU though is so
> > > special that it's not worth writing a generic driver, so just
> > > integrate the MU access functions in the SCU code.
> > >
> >
> > I understand it.
> >
> > One problem of the way you suggested may be that:
> > If we doing like below, we may lose flexibility to change the MU used
> > for SCU firmware communication.
> > 	scu at 5d1b0000 {
> > 		compatible = "fsl,imx8qxp-scu";
> > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > 	};
> >
> > And current design is that the system supports multiple MU channels
> > used by various users at the same time, e.g. SCU, Power Domain, Clock and
> others.
> > User can flexibly change it under their nodes: And each MU channel is
> > protected by their private lock and not affect each others.
> >
> > e.g.
> >         scu {
> >                 compatible = "nxp,imx8qxp-scu", "simple-bus";
> >                 fsl,mu = <&lsio_mu0>;
> >
> >                 clk: clk {
> >                         compatible = "fsl,imx8qxp-clk";
> >                         #clock-cells = <1>;
> >                 };
> >
> >                 iomuxc: iomuxc {
> >                         compatible = "fsl,imx8qxp-iomuxc";
> >                         fsl,mu = <&lsio_mu3>;
> >                 };
> >
> >                 imx8qx-pm {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> >                         fsl,mu = <&lsio_mu4>;
> > 	.............
> >         }
> >
> > The default code only uses MU0 which is used by SCU.
> >
> > The change may affect this design. Any ideas?
> > Do you think we can keep the current way?
> >
> 
> Any comments about this?
> 

Have you got a chance to help look at it?
We need identify the direction to go next.

Regards
Dong Aisheng

> Regards
> Dong Aisheng
> 
> > 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&data=02%7C01%7Caisheng.dong%40nxp.com%7C266
> > >
> 24a5c38e5488a80b708d5b0f3107b%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> > >
> >
> 7C0%7C0%7C636609480354404338&sdata=XP%2BYdt9I7zURrQun2jRbempLhC
> > > XrYtMVMb3dEWrZuro%3D&reserved=0  |
> > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> > > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Sascha Hauer]

On Thu, May 24, 2018 at 08:56:15AM +0000, A.s. Dong wrote:
> Hi Sascha,
> 
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Thursday, May 3, 2018 8:41 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 4/4] soc: imx: add SC firmware IPC and APIs
> > 
> > On Thu, May 03, 2018 at 12:29:40PM +0000, A.s. Dong wrote:
> > > > -----Original Message-----
> > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > Sent: Thursday, May 3, 2018 7:06 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 4/4] soc: imx: add SC firmware IPC and APIs
> > > >
> > > > On Wed, May 02, 2018 at 06:40:03PM +0000, A.s. Dong wrote:
> > > > > > -----Original Message-----
> > > > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > > > Sent: Wednesday, May 2, 2018 6:04 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 4/4] soc: imx: add SC firmware IPC and APIs
> > > > > >
> > > > > > On Sat, Apr 28, 2018 at 02:46:16AM +0800, Dong Aisheng wrote:
> > > > > > > +/* Initialization of the MU code. */ int __init
> > > > > > > +imx8_scu_init(void) {
> > > > > > > +	struct device_node *np, *mu_np;
> > > > > > > +	struct resource mu_res;
> > > > > > > +	sc_err_t sci_err;
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	if (!of_machine_is_compatible("fsl,imx8qxp"))
> > > > > > > +		return 0;
> > > > > > > +
> > > > > > > +	np = of_find_compatible_node(NULL, NULL, "nxp,imx8qxp-
> > scu");
> > > > > > > +	if (!np)
> > > > > > > +		return -ENODEV;
> > > > > > > +
> > > > > > > +	mu_np = of_parse_phandle(np, "fsl,mu", 0);
> > > > > > > +	if (WARN_ON(!mu_np))
> > > > > > > +		return -EINVAL;
> > > > > > > +
> > > > > > > +	ret = of_address_to_resource(mu_np, 0, &mu_res);
> > > > > > > +	if (WARN_ON(ret))
> > > > > > > +		return -EINVAL;
> > > > > > > +
> > > > > > > +	/* we use mu physical address as IPC communication channel
> > ID */
> > > > > > > +	sci_err = sc_ipc_open(&scu_ipc_handle,
> > (sc_ipc_id_t)(mu_res.start));
> > > > > > > +	if (sci_err != SC_ERR_NONE) {
> > > > > > > +		pr_err("Cannot open MU channel to SCU\n");
> > > > > > > +		return sci_err;
> > > > > > > +	};
> > > > > >
> > > > > > Introducing private error codes always has the risk of just
> > > > > > forwarding them as errno codes as done here.
> > > > > >
> > > > >
> > > > > Yes, you're right.
> > > > > We probably could do the same as scpi_to_linux_errno in
> > > > > drivers/firmware/arm_scpi.c.
> > > > > However, may can't fix the issue permanently.
> > > > >
> > > > > > > +
> > > > > > > +	of_node_put(mu_np);
> > > > > > > +
> > > > > > > +	pr_info("NXP i.MX SCU Initialized (scu_ipc %u)\n",
> > > > > > > +scu_ipc_handle);
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +early_initcall(imx8_scu_init);
> > > > > >
> > > > > > This code shows that the separate 'scu' device node shouldn't exist.
> > > > > > It is only used as a stepping stone to find the 'mu' node.
> > > > > > Instead of providing a proper driver for the 'mu' node the scu
> > > > > > code registers it
> > > > with its physical address.
> > > > > > I don't think it makes sense to separate mu and scu code in this way.
> > > > > >
> > > > >
> > > > > I agree that may not look quite well.
> > > > > It mainly because we want to use the MU physical address as a MU ID.
> > > > > (can't use virtual address as sc_ipc_id_t is u32 defined by SCU firmware.
> > > > >
> > > > > If you have any better suggestion please let me know.
> > > >
> > > > What I'm suggesting is a single node:
> > > >
> > > > 	scu at 5d1b0000 {
> > > > 		compatible = "fsl,imx8qxp-scu";
> > > > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > > > 	};
> > > >
> > > > Attach your code to this one, without any further separation between
> > > > mu and scu code.
> > > >
> > >
> > > A bit confused. You're suggesting a single node here without mu or
> > > mailbox node phandle in it? Then how SCU use MU?
> > 
> > ioremap the address and mu_receive_msg()/mu_send_msg() on it, just like
> > you do already.
> > 
> > >
> > > > We discussed this internally and came to the conclusion that the SCU
> > > > is not a generic user of a MU. The MU is designed to work together
> > > > with a piece of SRAM to form a mailbox system. Instead of working as
> > > > designed the SCU morses the messages through the doorbell (what the
> > > > MU really is). For anything that uses the MU in the way it is
> > > > designed I would suggest using the mailbox interface from
> > > > drivers/mailbox/ along with the binding from
> > Documentation/devicetree/bindings/mailbox/mailbox.txt.
> > > >
> > > > In the way I suggest there is no need for any MU ID.
> > > >
> > >
> > > So you're suggesting switch to use mailbox instead of private MU
> > > library function calls?
> > > Something like:
> > >         scu {
> > >                 compatible = "nxp,imx8qxp-scu", "simple-bus";
> > >                 mboxes = <&mailbox 0>;
> > >         }
> > > Then IPC is implemented based on mailbox?
> > >
> > > As I replied Oleksij Rempel in another mail in this thread, we've
> > > tried mailbox but got performance regression issue and need more time
> > > to investigate whether it's really quite suitable for i.MX SCU
> > > firmware as SCU handling message quite fast in micro seconds. (Mailbox
> > > polling method has much more latency than current MU sample polling we
> > > used) and we want to avoid the SCU firmware API changes.
> > 
> > I am not suggesting to do mailboxing (using shared memory) for the SCU.
> > I am also not suggesting any API update for the SCU communication.
> > I am just mentioning that doing mailboxing is the way the MU was originally
> > designed for. Looking at the reference manual I see many MUs on the i.MX8.
> > I guess most of them are for communication between the different cores on
> > the system. For these it's probably worth writing some generic MU driver.
> > The way the MU is used for communication with the SCU though is so special
> > that it's not worth writing a generic driver, so just integrate the MU access
> > functions in the SCU code.
> > 
> 
> I understand it.
> 
> One problem of the way you suggested may be that:
> If we doing like below, we may lose flexibility to change the MU used
> for SCU firmware communication.
> 	scu at 5d1b0000 {
> 		compatible = "fsl,imx8qxp-scu";
> 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> 	};
> 
> And current design is that the system supports multiple MU channels used
> by various users at the same time, e.g. SCU, Power Domain, Clock and others.
> User can flexibly change it under their nodes: And each MU channel is protected
> by their private lock and not affect each others.
> 
> e.g.
>         scu {
>                 compatible = "nxp,imx8qxp-scu", "simple-bus";
>                 fsl,mu = <&lsio_mu0>;
> 
>                 clk: clk {
>                         compatible = "fsl,imx8qxp-clk";
>                         #clock-cells = <1>;
>                 };
> 
>                 iomuxc: iomuxc {
>                         compatible = "fsl,imx8qxp-iomuxc";
>                         fsl,mu = <&lsio_mu3>;
>                 };
> 
>                 imx8qx-pm {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>                         fsl,mu = <&lsio_mu4>;
> 	.............
>         }
> 
> The default code only uses MU0 which is used by SCU.
> 
> The change may affect this design. Any ideas?

Sorry for the delay.

You can add the child nodes to the mu nodes they should use:

	scu1 {
        	compatible = "nxp,imx8qxp-scu";
		reg = <0x0 0x5d1b0000 0x0 0x10000>;

		clk: clk {
			compatible = "fsl,imx8qxp-clk";
			#clock-cells = <1>;
		};

		...
	};

	scu2 {
		compatible = "nxp,imx8qxp-scu";
		reg = <0x0 someothermu 0x0 0x10000>;

		iomuxc: iomuxc {
			compatible = "fsl,imx8qxp-iomuxc";
		};

		...
	};

So instead of adding all possible children to a single mu and phandle to
other mu's, just add the right children to each mu.

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 4/4] soc: imx: add SC firmware IPC and APIs

[A.s. Dong]

Hi Sascha,

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Wednesday, June 6, 2018 10:15 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: 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; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 4/4] soc: imx: add SC firmware IPC and APIs
> 
> On Thu, May 24, 2018 at 08:56:15AM +0000, A.s. Dong wrote:
> > Hi Sascha,
> >
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > Sent: Thursday, May 3, 2018 8:41 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 4/4] soc: imx: add SC firmware IPC and APIs
> > >
> > > On Thu, May 03, 2018 at 12:29:40PM +0000, A.s. Dong wrote:
> > > > > -----Original Message-----
> > > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > > Sent: Thursday, May 3, 2018 7:06 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 4/4] soc: imx: add SC firmware IPC and APIs
> > > > >
> > > > > On Wed, May 02, 2018 at 06:40:03PM +0000, A.s. Dong wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > > > > Sent: Wednesday, May 2, 2018 6:04 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 4/4] soc: imx: add SC firmware IPC and
> > > > > > > APIs
> > > > > > >
> > > > > > > On Sat, Apr 28, 2018 at 02:46:16AM +0800, Dong Aisheng wrote:
> > > > > > > > +/* Initialization of the MU code. */ int __init
> > > > > > > > +imx8_scu_init(void) {
> > > > > > > > +	struct device_node *np, *mu_np;
> > > > > > > > +	struct resource mu_res;
> > > > > > > > +	sc_err_t sci_err;
> > > > > > > > +	int ret;
> > > > > > > > +
> > > > > > > > +	if (!of_machine_is_compatible("fsl,imx8qxp"))
> > > > > > > > +		return 0;
> > > > > > > > +
> > > > > > > > +	np = of_find_compatible_node(NULL, NULL, "nxp,imx8qxp-
> > > scu");
> > > > > > > > +	if (!np)
> > > > > > > > +		return -ENODEV;
> > > > > > > > +
> > > > > > > > +	mu_np = of_parse_phandle(np, "fsl,mu", 0);
> > > > > > > > +	if (WARN_ON(!mu_np))
> > > > > > > > +		return -EINVAL;
> > > > > > > > +
> > > > > > > > +	ret = of_address_to_resource(mu_np, 0, &mu_res);
> > > > > > > > +	if (WARN_ON(ret))
> > > > > > > > +		return -EINVAL;
> > > > > > > > +
> > > > > > > > +	/* we use mu physical address as IPC communication
> > > > > > > > +channel
> > > ID */
> > > > > > > > +	sci_err = sc_ipc_open(&scu_ipc_handle,
> > > (sc_ipc_id_t)(mu_res.start));
> > > > > > > > +	if (sci_err != SC_ERR_NONE) {
> > > > > > > > +		pr_err("Cannot open MU channel to SCU\n");
> > > > > > > > +		return sci_err;
> > > > > > > > +	};
> > > > > > >
> > > > > > > Introducing private error codes always has the risk of just
> > > > > > > forwarding them as errno codes as done here.
> > > > > > >
> > > > > >
> > > > > > Yes, you're right.
> > > > > > We probably could do the same as scpi_to_linux_errno in
> > > > > > drivers/firmware/arm_scpi.c.
> > > > > > However, may can't fix the issue permanently.
> > > > > >
> > > > > > > > +
> > > > > > > > +	of_node_put(mu_np);
> > > > > > > > +
> > > > > > > > +	pr_info("NXP i.MX SCU Initialized (scu_ipc %u)\n",
> > > > > > > > +scu_ipc_handle);
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > > +early_initcall(imx8_scu_init);
> > > > > > >
> > > > > > > This code shows that the separate 'scu' device node shouldn't
> exist.
> > > > > > > It is only used as a stepping stone to find the 'mu' node.
> > > > > > > Instead of providing a proper driver for the 'mu' node the
> > > > > > > scu code registers it
> > > > > with its physical address.
> > > > > > > I don't think it makes sense to separate mu and scu code in this
> way.
> > > > > > >
> > > > > >
> > > > > > I agree that may not look quite well.
> > > > > > It mainly because we want to use the MU physical address as a MU
> ID.
> > > > > > (can't use virtual address as sc_ipc_id_t is u32 defined by SCU
> firmware.
> > > > > >
> > > > > > If you have any better suggestion please let me know.
> > > > >
> > > > > What I'm suggesting is a single node:
> > > > >
> > > > > 	scu at 5d1b0000 {
> > > > > 		compatible = "fsl,imx8qxp-scu";
> > > > > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > > > > 	};
> > > > >
> > > > > Attach your code to this one, without any further separation
> > > > > between mu and scu code.
> > > > >
> > > >
> > > > A bit confused. You're suggesting a single node here without mu or
> > > > mailbox node phandle in it? Then how SCU use MU?
> > >
> > > ioremap the address and mu_receive_msg()/mu_send_msg() on it, just
> > > like you do already.
> > >
> > > >
> > > > > We discussed this internally and came to the conclusion that the
> > > > > SCU is not a generic user of a MU. The MU is designed to work
> > > > > together with a piece of SRAM to form a mailbox system. Instead
> > > > > of working as designed the SCU morses the messages through the
> > > > > doorbell (what the MU really is). For anything that uses the MU
> > > > > in the way it is designed I would suggest using the mailbox
> > > > > interface from drivers/mailbox/ along with the binding from
> > > Documentation/devicetree/bindings/mailbox/mailbox.txt.
> > > > >
> > > > > In the way I suggest there is no need for any MU ID.
> > > > >
> > > >
> > > > So you're suggesting switch to use mailbox instead of private MU
> > > > library function calls?
> > > > Something like:
> > > >         scu {
> > > >                 compatible = "nxp,imx8qxp-scu", "simple-bus";
> > > >                 mboxes = <&mailbox 0>;
> > > >         }
> > > > Then IPC is implemented based on mailbox?
> > > >
> > > > As I replied Oleksij Rempel in another mail in this thread, we've
> > > > tried mailbox but got performance regression issue and need more
> > > > time to investigate whether it's really quite suitable for i.MX
> > > > SCU firmware as SCU handling message quite fast in micro seconds.
> > > > (Mailbox polling method has much more latency than current MU
> > > > sample polling we
> > > > used) and we want to avoid the SCU firmware API changes.
> > >
> > > I am not suggesting to do mailboxing (using shared memory) for the SCU.
> > > I am also not suggesting any API update for the SCU communication.
> > > I am just mentioning that doing mailboxing is the way the MU was
> > > originally designed for. Looking at the reference manual I see many MUs
> on the i.MX8.
> > > I guess most of them are for communication between the different
> > > cores on the system. For these it's probably worth writing some generic
> MU driver.
> > > The way the MU is used for communication with the SCU though is so
> > > special that it's not worth writing a generic driver, so just
> > > integrate the MU access functions in the SCU code.
> > >
> >
> > I understand it.
> >
> > One problem of the way you suggested may be that:
> > If we doing like below, we may lose flexibility to change the MU used
> > for SCU firmware communication.
> > 	scu at 5d1b0000 {
> > 		compatible = "fsl,imx8qxp-scu";
> > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > 	};
> >
> > And current design is that the system supports multiple MU channels
> > used by various users at the same time, e.g. SCU, Power Domain, Clock and
> others.
> > User can flexibly change it under their nodes: And each MU channel is
> > protected by their private lock and not affect each others.
> >
> > e.g.
> >         scu {
> >                 compatible = "nxp,imx8qxp-scu", "simple-bus";
> >                 fsl,mu = <&lsio_mu0>;
> >
> >                 clk: clk {
> >                         compatible = "fsl,imx8qxp-clk";
> >                         #clock-cells = <1>;
> >                 };
> >
> >                 iomuxc: iomuxc {
> >                         compatible = "fsl,imx8qxp-iomuxc";
> >                         fsl,mu = <&lsio_mu3>;
> >                 };
> >
> >                 imx8qx-pm {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> >                         fsl,mu = <&lsio_mu4>;
> > 	.............
> >         }
> >
> > The default code only uses MU0 which is used by SCU.
> >
> > The change may affect this design. Any ideas?
> 
> Sorry for the delay.
> 
> You can add the child nodes to the mu nodes they should use:
> 
> 	scu1 {
>         	compatible = "nxp,imx8qxp-scu";
> 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> 
> 		clk: clk {
> 			compatible = "fsl,imx8qxp-clk";
> 			#clock-cells = <1>;
> 		};
> 
> 		...
> 	};
> 
> 	scu2 {
> 		compatible = "nxp,imx8qxp-scu";
> 		reg = <0x0 someothermu 0x0 0x10000>;
> 
> 		iomuxc: iomuxc {
> 			compatible = "fsl,imx8qxp-iomuxc";
> 		};
> 
> 		...
> 	};
> 
> So instead of adding all possible children to a single mu and phandle to other
> mu's, just add the right children to each mu.
> 

I got your point now. But sorry i'm still a bit hestitate to it....

This way increases complexity and looks more like a per-channel binding.
But we normally have only one (abstract) SCU firmware node in a system which may
use different channels to implement different functions like clk, pd and etc.
Multiple faked SCU nodes make people a bit confusing.
Furthermore, it's still lose the flexibility for user to changing a MU to use.

Looking at all exist users in kernel, seems no one to use like this.
See:
Documentation/devicetree/bindings/arm/arm,scpi.txt
Documentation/devicetree/bindings/arm/keystone/ti,sci.txt

All are similar like:
xxx: protocol-node {
                compatible = "xxx-protocal";
	  channel = ...
                ...

                clk_node: clk_node {
                        ...
                };

                pd_node: pd_node {
                        ...
                };
};
The protocol node work is selecting the correct channel, do necessary initialization
and populate the all child function device nodes.

IMHO I'm still a bit like to this common way used in kernel if no stronger objection.
Do you think we can choose this way to go step forward?

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&data=02%7C01%7Caisheng.dong%40nxp.com%7Cec7
> bf490d73542f736a408d5cbb7e5af%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C636638913057159824&sdata=m24oKIKeP1sxGOq9gGShs8Y%2BiB
> Rg4NymKKJEF0IHpGc%3D&reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Sascha Hauer]

On Thu, Jun 07, 2018 at 04:18:54AM +0000, A.s. Dong wrote:
> Hi Sascha,
> 
> > > One problem of the way you suggested may be that:
> > > If we doing like below, we may lose flexibility to change the MU used
> > > for SCU firmware communication.
> > > 	scu at 5d1b0000 {
> > > 		compatible = "fsl,imx8qxp-scu";
> > > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > > 	};
> > >
> > > And current design is that the system supports multiple MU channels
> > > used by various users at the same time, e.g. SCU, Power Domain, Clock and
> > others.
> > > User can flexibly change it under their nodes: And each MU channel is
> > > protected by their private lock and not affect each others.
> > >
> > > e.g.
> > >         scu {
> > >                 compatible = "nxp,imx8qxp-scu", "simple-bus";
> > >                 fsl,mu = <&lsio_mu0>;
> > >
> > >                 clk: clk {
> > >                         compatible = "fsl,imx8qxp-clk";
> > >                         #clock-cells = <1>;
> > >                 };
> > >
> > >                 iomuxc: iomuxc {
> > >                         compatible = "fsl,imx8qxp-iomuxc";
> > >                         fsl,mu = <&lsio_mu3>;
> > >                 };
> > >
> > >                 imx8qx-pm {
> > >                         #address-cells = <1>;
> > >                         #size-cells = <0>;
> > >                         fsl,mu = <&lsio_mu4>;
> > > 	.............
> > >         }
> > >
> > > The default code only uses MU0 which is used by SCU.
> > >
> > > The change may affect this design. Any ideas?
> > 
> > Sorry for the delay.
> > 
> > You can add the child nodes to the mu nodes they should use:
> > 
> > 	scu1 {
> >         	compatible = "nxp,imx8qxp-scu";
> > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > 
> > 		clk: clk {
> > 			compatible = "fsl,imx8qxp-clk";
> > 			#clock-cells = <1>;
> > 		};
> > 
> > 		...
> > 	};
> > 
> > 	scu2 {
> > 		compatible = "nxp,imx8qxp-scu";
> > 		reg = <0x0 someothermu 0x0 0x10000>;
> > 
> > 		iomuxc: iomuxc {
> > 			compatible = "fsl,imx8qxp-iomuxc";
> > 		};
> > 
> > 		...
> > 	};
> > 
> > So instead of adding all possible children to a single mu and phandle to other
> > mu's, just add the right children to each mu.
> > 
> 
> I got your point now. But sorry i'm still a bit hestitate to it....
> 
> This way increases complexity and looks more like a per-channel binding.
> But we normally have only one (abstract) SCU firmware node in a system which may
> use different channels to implement different functions like clk, pd and etc.
> Multiple faked SCU nodes make people a bit confusing.

They are not faked, indeed that's the MU units that physically exist.

> Furthermore, it's still lose the flexibility for user to changing a MU to use.
> 
> Looking at all exist users in kernel, seems no one to use like this.
> See:
> Documentation/devicetree/bindings/arm/arm,scpi.txt
> Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
> 
> All are similar like:
> xxx: protocol-node {
>                 compatible = "xxx-protocal";
> 	  channel = ...
>                 ...
> 
>                 clk_node: clk_node {
>                         ...
>                 };
> 
>                 pd_node: pd_node {
>                         ...
>                 };
> };
> The protocol node work is selecting the correct channel, do necessary initialization
> and populate the all child function device nodes.
> 
> IMHO I'm still a bit like to this common way used in kernel if no stronger objection.
> Do you think we can choose this way to go step forward?

I'm not convinced, but go ahead if you think this is the better way to
proceed.

I think my original point that led to this discussion is the muddled way
the MUs are handled in the code.

To start with in the system controller code you ioremap the physical
address of the MU and later on pass this address as a reference to
the MU library code. There's no way for the MU code to ever create a
private data. It would be much better if you would pass mu_init a
pointer to the device node it shall initialize, let mu_init allocate
a private data struct, ioremap the base and put it in the private data
struct, and return the private data struct.

Then there is this sc_ipc_get_handle() thing that your consumers shall
use to get a handle to the SCU. Instead of returning a struct sc_ipc *
there you return a ida which you first have to search for each time
a consumer wants to do something on the SCU. Please just return a
pointer there (which can be a cookie, i.e. the struct definition is
unknown to the consumer but privately to the SCU code).

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 4/4] soc: imx: add SC firmware IPC and APIs

[A.s. Dong]

Hi Sascha,

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Thursday, June 7, 2018 3:09 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: 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; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 4/4] soc: imx: add SC firmware IPC and APIs
> 
> On Thu, Jun 07, 2018 at 04:18:54AM +0000, A.s. Dong wrote:
> > Hi Sascha,
> >
> > > > One problem of the way you suggested may be that:
> > > > If we doing like below, we may lose flexibility to change the MU
> > > > used for SCU firmware communication.
> > > > 	scu at 5d1b0000 {
> > > > 		compatible = "fsl,imx8qxp-scu";
> > > > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > > > 	};
> > > >
> > > > And current design is that the system supports multiple MU
> > > > channels used by various users at the same time, e.g. SCU, Power
> > > > Domain, Clock and
> > > others.
> > > > User can flexibly change it under their nodes: And each MU channel
> > > > is protected by their private lock and not affect each others.
> > > >
> > > > e.g.
> > > >         scu {
> > > >                 compatible = "nxp,imx8qxp-scu", "simple-bus";
> > > >                 fsl,mu = <&lsio_mu0>;
> > > >
> > > >                 clk: clk {
> > > >                         compatible = "fsl,imx8qxp-clk";
> > > >                         #clock-cells = <1>;
> > > >                 };
> > > >
> > > >                 iomuxc: iomuxc {
> > > >                         compatible = "fsl,imx8qxp-iomuxc";
> > > >                         fsl,mu = <&lsio_mu3>;
> > > >                 };
> > > >
> > > >                 imx8qx-pm {
> > > >                         #address-cells = <1>;
> > > >                         #size-cells = <0>;
> > > >                         fsl,mu = <&lsio_mu4>;
> > > > 	.............
> > > >         }
> > > >
> > > > The default code only uses MU0 which is used by SCU.
> > > >
> > > > The change may affect this design. Any ideas?
> > >
> > > Sorry for the delay.
> > >
> > > You can add the child nodes to the mu nodes they should use:
> > >
> > > 	scu1 {
> > >         	compatible = "nxp,imx8qxp-scu";
> > > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > >
> > > 		clk: clk {
> > > 			compatible = "fsl,imx8qxp-clk";
> > > 			#clock-cells = <1>;
> > > 		};
> > >
> > > 		...
> > > 	};
> > >
> > > 	scu2 {
> > > 		compatible = "nxp,imx8qxp-scu";
> > > 		reg = <0x0 someothermu 0x0 0x10000>;
> > >
> > > 		iomuxc: iomuxc {
> > > 			compatible = "fsl,imx8qxp-iomuxc";
> > > 		};
> > >
> > > 		...
> > > 	};
> > >
> > > So instead of adding all possible children to a single mu and
> > > phandle to other mu's, just add the right children to each mu.
> > >
> >
> > I got your point now. But sorry i'm still a bit hestitate to it....
> >
> > This way increases complexity and looks more like a per-channel binding.
> > But we normally have only one (abstract) SCU firmware node in a system
> > which may use different channels to implement different functions like clk,
> pd and etc.
> > Multiple faked SCU nodes make people a bit confusing.
> 
> They are not faked, indeed that's the MU units that physically exist.
> 
> > Furthermore, it's still lose the flexibility for user to changing a MU to use.
> >
> > Looking at all exist users in kernel, seems no one to use like this.
> > See:
> > Documentation/devicetree/bindings/arm/arm,scpi.txt
> > Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
> >
> > All are similar like:
> > xxx: protocol-node {
> >                 compatible = "xxx-protocal";
> > 	  channel = ...
> >                 ...
> >
> >                 clk_node: clk_node {
> >                         ...
> >                 };
> >
> >                 pd_node: pd_node {
> >                         ...
> >                 };
> > };
> > The protocol node work is selecting the correct channel, do necessary
> > initialization and populate the all child function device nodes.
> >
> > IMHO I'm still a bit like to this common way used in kernel if no stronger
> objection.
> > Do you think we can choose this way to go step forward?
> 
> I'm not convinced, but go ahead if you think this is the better way to proceed.
> 
> I think my original point that led to this discussion is the muddled way the
> MUs are handled in the code.
> 
> To start with in the system controller code you ioremap the physical address
> of the MU and later on pass this address as a reference to the MU library
> code. There's no way for the MU code to ever create a private data. It would
> be much better if you would pass mu_init a pointer to the device node it shall
> initialize, let mu_init allocate a private data struct, ioremap the base and put
> it in the private data struct, and return the private data struct.
> 

Actually I have tried that way initially, but ....

> Then there is this sc_ipc_get_handle() thing that your consumers shall use to
> get a handle to the SCU. Instead of returning a struct sc_ipc * there you
> return a ida which you first have to search for each time a consumer wants to
> do something on the SCU. Please just return a pointer there (which can be a
> cookie, i.e. the struct definition is unknown to the consumer but privately to
> the SCU code).
> 

The problem is that sc_ipc_t is defined as uint32_t.
/*
 * This type is used to declare a handle for an IPC communication
 * channel. Its meaning is specific to the IPC implementation.
 */
typedef uint32_t sc_ipc_t;

which is referenced by the standard rpc call:
void sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp)

I can't return a pointer which is 64bit on ARMv8 platform and used it
for sc_call_rpc directly.

That why I need a way to convert struct sc_ipc_t to struct sc_ipc 
(done by sc_ipc_get(ipc)).

But you're right, that means we have to search for each time a consumer
wants to do something on the SCU.

If we want to void it,  one possible way may be changing the prototype of
both ipc handle sc_ipc_t  and IPC channel ID sc_ipc_id_t to unsigned long,
then we can directly pass them the address pointer.

Although I initially don't want to changing SCU API prototype, but if we
have to, I will do it.

Sounds good to you?

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&data=02%7C01%7Caisheng.dong%40nxp.com%7Cda4
> 4ef30db214900616308d5cc457d56%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C636639521186927654&sdata=hBYUnrXnHQnaqS0Ovd9mYuUALU2
> OpEH%2BseZxE6BgvJw%3D&reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Sascha Hauer]

On Thu, Jun 07, 2018 at 01:59:26PM +0000, A.s. Dong wrote:
> Hi Sascha,
> 
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Thursday, June 7, 2018 3:09 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: 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; linux-arm-kernel at lists.infradead.org
> > Subject: Re: [PATCH 4/4] soc: imx: add SC firmware IPC and APIs
> > 
> > On Thu, Jun 07, 2018 at 04:18:54AM +0000, A.s. Dong wrote:
> > > Hi Sascha,
> > >
> > > > > One problem of the way you suggested may be that:
> > > > > If we doing like below, we may lose flexibility to change the MU
> > > > > used for SCU firmware communication.
> > > > > 	scu at 5d1b0000 {
> > > > > 		compatible = "fsl,imx8qxp-scu";
> > > > > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > > > > 	};
> > > > >
> > > > > And current design is that the system supports multiple MU
> > > > > channels used by various users at the same time, e.g. SCU, Power
> > > > > Domain, Clock and
> > > > others.
> > > > > User can flexibly change it under their nodes: And each MU channel
> > > > > is protected by their private lock and not affect each others.
> > > > >
> > > > > e.g.
> > > > >         scu {
> > > > >                 compatible = "nxp,imx8qxp-scu", "simple-bus";
> > > > >                 fsl,mu = <&lsio_mu0>;
> > > > >
> > > > >                 clk: clk {
> > > > >                         compatible = "fsl,imx8qxp-clk";
> > > > >                         #clock-cells = <1>;
> > > > >                 };
> > > > >
> > > > >                 iomuxc: iomuxc {
> > > > >                         compatible = "fsl,imx8qxp-iomuxc";
> > > > >                         fsl,mu = <&lsio_mu3>;
> > > > >                 };
> > > > >
> > > > >                 imx8qx-pm {
> > > > >                         #address-cells = <1>;
> > > > >                         #size-cells = <0>;
> > > > >                         fsl,mu = <&lsio_mu4>;
> > > > > 	.............
> > > > >         }
> > > > >
> > > > > The default code only uses MU0 which is used by SCU.
> > > > >
> > > > > The change may affect this design. Any ideas?
> > > >
> > > > Sorry for the delay.
> > > >
> > > > You can add the child nodes to the mu nodes they should use:
> > > >
> > > > 	scu1 {
> > > >         	compatible = "nxp,imx8qxp-scu";
> > > > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > > >
> > > > 		clk: clk {
> > > > 			compatible = "fsl,imx8qxp-clk";
> > > > 			#clock-cells = <1>;
> > > > 		};
> > > >
> > > > 		...
> > > > 	};
> > > >
> > > > 	scu2 {
> > > > 		compatible = "nxp,imx8qxp-scu";
> > > > 		reg = <0x0 someothermu 0x0 0x10000>;
> > > >
> > > > 		iomuxc: iomuxc {
> > > > 			compatible = "fsl,imx8qxp-iomuxc";
> > > > 		};
> > > >
> > > > 		...
> > > > 	};
> > > >
> > > > So instead of adding all possible children to a single mu and
> > > > phandle to other mu's, just add the right children to each mu.
> > > >
> > >
> > > I got your point now. But sorry i'm still a bit hestitate to it....
> > >
> > > This way increases complexity and looks more like a per-channel binding.
> > > But we normally have only one (abstract) SCU firmware node in a system
> > > which may use different channels to implement different functions like clk,
> > pd and etc.
> > > Multiple faked SCU nodes make people a bit confusing.
> > 
> > They are not faked, indeed that's the MU units that physically exist.
> > 
> > > Furthermore, it's still lose the flexibility for user to changing a MU to use.
> > >
> > > Looking at all exist users in kernel, seems no one to use like this.
> > > See:
> > > Documentation/devicetree/bindings/arm/arm,scpi.txt
> > > Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
> > >
> > > All are similar like:
> > > xxx: protocol-node {
> > >                 compatible = "xxx-protocal";
> > > 	  channel = ...
> > >                 ...
> > >
> > >                 clk_node: clk_node {
> > >                         ...
> > >                 };
> > >
> > >                 pd_node: pd_node {
> > >                         ...
> > >                 };
> > > };
> > > The protocol node work is selecting the correct channel, do necessary
> > > initialization and populate the all child function device nodes.
> > >
> > > IMHO I'm still a bit like to this common way used in kernel if no stronger
> > objection.
> > > Do you think we can choose this way to go step forward?
> > 
> > I'm not convinced, but go ahead if you think this is the better way to proceed.
> > 
> > I think my original point that led to this discussion is the muddled way the
> > MUs are handled in the code.
> > 
> > To start with in the system controller code you ioremap the physical address
> > of the MU and later on pass this address as a reference to the MU library
> > code. There's no way for the MU code to ever create a private data. It would
> > be much better if you would pass mu_init a pointer to the device node it shall
> > initialize, let mu_init allocate a private data struct, ioremap the base and put
> > it in the private data struct, and return the private data struct.
> > 
> 
> Actually I have tried that way initially, but ....
> 
> > Then there is this sc_ipc_get_handle() thing that your consumers shall use to
> > get a handle to the SCU. Instead of returning a struct sc_ipc * there you
> > return a ida which you first have to search for each time a consumer wants to
> > do something on the SCU. Please just return a pointer there (which can be a
> > cookie, i.e. the struct definition is unknown to the consumer but privately to
> > the SCU code).
> > 
> 
> The problem is that sc_ipc_t is defined as uint32_t.
> /*
>  * This type is used to declare a handle for an IPC communication
>  * channel. Its meaning is specific to the IPC implementation.
>  */
> typedef uint32_t sc_ipc_t;
> 
> which is referenced by the standard rpc call:
> void sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp)
> 
> I can't return a pointer which is 64bit on ARMv8 platform and used it
> for sc_call_rpc directly.
> 
> That why I need a way to convert struct sc_ipc_t to struct sc_ipc 
> (done by sc_ipc_get(ipc)).
> 
> But you're right, that means we have to search for each time a consumer
> wants to do something on the SCU.
> 
> If we want to void it,  one possible way may be changing the prototype of
> both ipc handle sc_ipc_t  and IPC channel ID sc_ipc_id_t to unsigned long,
> then we can directly pass them the address pointer.
> 
> Although I initially don't want to changing SCU API prototype, but if we
> have to, I will do it.

Don't try to push crappy code just because you use the same crappy code
internally elsewhere. Everything you post for the Kernel is subject for
discussion, review and change. If we would follow the
it's-in-sync-with-internal-company-code argument the Kernel would loook
differently now and surely not better.

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 4/4] soc: imx: add SC firmware IPC and APIs

[A.s. Dong]

Hi Sascha,

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Friday, June 8, 2018 12:13 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: 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; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 4/4] soc: imx: add SC firmware IPC and APIs
> 
> On Thu, Jun 07, 2018 at 01:59:26PM +0000, A.s. Dong wrote:
> > Hi Sascha,
> >
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > Sent: Thursday, June 7, 2018 3:09 PM
> > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > Cc: 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; linux-arm-kernel at lists.infradead.org
> > > Subject: Re: [PATCH 4/4] soc: imx: add SC firmware IPC and APIs
> > >
> > > On Thu, Jun 07, 2018 at 04:18:54AM +0000, A.s. Dong wrote:
> > > > Hi Sascha,
> > > >
> > > > > > One problem of the way you suggested may be that:
> > > > > > If we doing like below, we may lose flexibility to change the
> > > > > > MU used for SCU firmware communication.
> > > > > > 	scu at 5d1b0000 {
> > > > > > 		compatible = "fsl,imx8qxp-scu";
> > > > > > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > > > > > 	};
> > > > > >
> > > > > > And current design is that the system supports multiple MU
> > > > > > channels used by various users at the same time, e.g. SCU,
> > > > > > Power Domain, Clock and
> > > > > others.
> > > > > > User can flexibly change it under their nodes: And each MU
> > > > > > channel is protected by their private lock and not affect each others.
> > > > > >
> > > > > > e.g.
> > > > > >         scu {
> > > > > >                 compatible = "nxp,imx8qxp-scu", "simple-bus";
> > > > > >                 fsl,mu = <&lsio_mu0>;
> > > > > >
> > > > > >                 clk: clk {
> > > > > >                         compatible = "fsl,imx8qxp-clk";
> > > > > >                         #clock-cells = <1>;
> > > > > >                 };
> > > > > >
> > > > > >                 iomuxc: iomuxc {
> > > > > >                         compatible = "fsl,imx8qxp-iomuxc";
> > > > > >                         fsl,mu = <&lsio_mu3>;
> > > > > >                 };
> > > > > >
> > > > > >                 imx8qx-pm {
> > > > > >                         #address-cells = <1>;
> > > > > >                         #size-cells = <0>;
> > > > > >                         fsl,mu = <&lsio_mu4>;
> > > > > > 	.............
> > > > > >         }
> > > > > >
> > > > > > The default code only uses MU0 which is used by SCU.
> > > > > >
> > > > > > The change may affect this design. Any ideas?
> > > > >
> > > > > Sorry for the delay.
> > > > >
> > > > > You can add the child nodes to the mu nodes they should use:
> > > > >
> > > > > 	scu1 {
> > > > >         	compatible = "nxp,imx8qxp-scu";
> > > > > 		reg = <0x0 0x5d1b0000 0x0 0x10000>;
> > > > >
> > > > > 		clk: clk {
> > > > > 			compatible = "fsl,imx8qxp-clk";
> > > > > 			#clock-cells = <1>;
> > > > > 		};
> > > > >
> > > > > 		...
> > > > > 	};
> > > > >
> > > > > 	scu2 {
> > > > > 		compatible = "nxp,imx8qxp-scu";
> > > > > 		reg = <0x0 someothermu 0x0 0x10000>;
> > > > >
> > > > > 		iomuxc: iomuxc {
> > > > > 			compatible = "fsl,imx8qxp-iomuxc";
> > > > > 		};
> > > > >
> > > > > 		...
> > > > > 	};
> > > > >
> > > > > So instead of adding all possible children to a single mu and
> > > > > phandle to other mu's, just add the right children to each mu.
> > > > >
> > > >
> > > > I got your point now. But sorry i'm still a bit hestitate to it....
> > > >
> > > > This way increases complexity and looks more like a per-channel
> binding.
> > > > But we normally have only one (abstract) SCU firmware node in a
> > > > system which may use different channels to implement different
> > > > functions like clk,
> > > pd and etc.
> > > > Multiple faked SCU nodes make people a bit confusing.
> > >
> > > They are not faked, indeed that's the MU units that physically exist.
> > >
> > > > Furthermore, it's still lose the flexibility for user to changing a MU to use.
> > > >
> > > > Looking at all exist users in kernel, seems no one to use like this.
> > > > See:
> > > > Documentation/devicetree/bindings/arm/arm,scpi.txt
> > > > Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
> > > >
> > > > All are similar like:
> > > > xxx: protocol-node {
> > > >                 compatible = "xxx-protocal";
> > > > 	  channel = ...
> > > >                 ...
> > > >
> > > >                 clk_node: clk_node {
> > > >                         ...
> > > >                 };
> > > >
> > > >                 pd_node: pd_node {
> > > >                         ...
> > > >                 };
> > > > };
> > > > The protocol node work is selecting the correct channel, do
> > > > necessary initialization and populate the all child function device nodes.
> > > >
> > > > IMHO I'm still a bit like to this common way used in kernel if no
> > > > stronger
> > > objection.
> > > > Do you think we can choose this way to go step forward?
> > >
> > > I'm not convinced, but go ahead if you think this is the better way to
> proceed.
> > >
> > > I think my original point that led to this discussion is the muddled
> > > way the MUs are handled in the code.
> > >
> > > To start with in the system controller code you ioremap the physical
> > > address of the MU and later on pass this address as a reference to
> > > the MU library code. There's no way for the MU code to ever create a
> > > private data. It would be much better if you would pass mu_init a
> > > pointer to the device node it shall initialize, let mu_init allocate
> > > a private data struct, ioremap the base and put it in the private data struct,
> and return the private data struct.
> > >
> >
> > Actually I have tried that way initially, but ....
> >
> > > Then there is this sc_ipc_get_handle() thing that your consumers
> > > shall use to get a handle to the SCU. Instead of returning a struct
> > > sc_ipc * there you return a ida which you first have to search for
> > > each time a consumer wants to do something on the SCU. Please just
> > > return a pointer there (which can be a cookie, i.e. the struct
> > > definition is unknown to the consumer but privately to the SCU code).
> > >
> >
> > The problem is that sc_ipc_t is defined as uint32_t.
> > /*
> >  * This type is used to declare a handle for an IPC communication
> >  * channel. Its meaning is specific to the IPC implementation.
> >  */
> > typedef uint32_t sc_ipc_t;
> >
> > which is referenced by the standard rpc call:
> > void sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp)
> >
> > I can't return a pointer which is 64bit on ARMv8 platform and used it
> > for sc_call_rpc directly.
> >
> > That why I need a way to convert struct sc_ipc_t to struct sc_ipc
> > (done by sc_ipc_get(ipc)).
> >
> > But you're right, that means we have to search for each time a
> > consumer wants to do something on the SCU.
> >
> > If we want to void it,  one possible way may be changing the prototype
> > of both ipc handle sc_ipc_t  and IPC channel ID sc_ipc_id_t to
> > unsigned long, then we can directly pass them the address pointer.
> >
> > Although I initially don't want to changing SCU API prototype, but if
> > we have to, I will do it.
> 
> Don't try to push crappy code just because you use the same crappy code
> internally elsewhere. Everything you post for the Kernel is subject for
> discussion, review and change. If we would follow the it's-in-sync-with-
> internal-company-code argument the Kernel would loook differently now
> and surely not better.
> 

Yes, I do agree with you. That's why I'd also like a change now.
Will do that way, If any issue pleases let me know.
Thanks for the suggestion.

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&data=02%7C01%7Caisheng.dong%40nxp.com%7Cb2b
> c8844a75c4321cde908d5ccf62fa7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C636640280097302814&sdata=T3xyXepjdIsF11sCjn9Xe8A8qAlsAA3
> b%2BQfs%2FCzGNeo%3D&reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[A.s. Dong]

Hi Sascha,

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Wednesday, May 2, 2018 6:04 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 4/4] soc: imx: add SC firmware IPC and APIs
> 
> On Sat, Apr 28, 2018 at 02:46:16AM +0800, Dong Aisheng wrote:
> > +/* Initialization of the MU code. */
> > +int __init imx8_scu_init(void)
> > +{
> > +	struct device_node *np, *mu_np;
> > +	struct resource mu_res;
> > +	sc_err_t sci_err;
> > +	int ret;
> > +
> > +	if (!of_machine_is_compatible("fsl,imx8qxp"))
> > +		return 0;
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "nxp,imx8qxp-scu");
> > +	if (!np)
> > +		return -ENODEV;
> > +
> > +	mu_np = of_parse_phandle(np, "fsl,mu", 0);
> > +	if (WARN_ON(!mu_np))
> > +		return -EINVAL;
> > +
> > +	ret = of_address_to_resource(mu_np, 0, &mu_res);
> > +	if (WARN_ON(ret))
> > +		return -EINVAL;
> > +
> > +	/* we use mu physical address as IPC communication channel ID */
> > +	sci_err = sc_ipc_open(&scu_ipc_handle, (sc_ipc_id_t)(mu_res.start));
> > +	if (sci_err != SC_ERR_NONE) {
> > +		pr_err("Cannot open MU channel to SCU\n");
> > +		return sci_err;
> > +	};
> 
> Introducing private error codes always has the risk of just forwarding them as
> errno codes as done here.
> 
> > +
> > +	of_node_put(mu_np);
> > +
> > +	pr_info("NXP i.MX SCU Initialized (scu_ipc %u)\n", scu_ipc_handle);
> > +
> > +	return 0;
> > +}
> > +early_initcall(imx8_scu_init);
> 
> This code shows that the separate 'scu' device node shouldn't exist. It is only
> used as a stepping stone to find the 'mu' node. Instead of providing a proper
> driver for the 'mu' node the scu code registers it with its physical address.
> I don't think it makes sense to separate mu and scu code in this way.
> 
> > +#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)])
> 
> These macros only make the code less readable, please drop them. Plain
> m->version is easier to read and forces the reader through less
> indirections. IMO m->DATA.u32[0] = foo; m->DATA.u8[4] = bar is still better
> readable than doing the division by size in macros.
> 
> > +typedef enum sc_rpc_async_state_e {
> > +	SC_RPC_ASYNC_STATE_RD_START = 0,
> > +	SC_RPC_ASYNC_STATE_RD_ACTIVE = 1,
> > +	SC_RPC_ASYNC_STATE_RD_DONE = 2,
> > +	SC_RPC_ASYNC_STATE_WR_START = 3,
> > +	SC_RPC_ASYNC_STATE_WR_ACTIVE = 4,
> > +	SC_RPC_ASYNC_STATE_WR_DONE = 5,
> > +} sc_rpc_async_state_t;
> > +
> > +typedef struct sc_rpc_async_msg_s {
> > +	sc_rpc_async_state_t state;
> > +	uint8_t wordIdx;
> > +	sc_rpc_msg_t msg;
> > +	uint32_t timeStamp;
> > +} sc_rpc_async_msg_t;
> 
> We normally do not typedef struct types. Any good reasons to do so here?
> 
> > +
> > +/* 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.
> > + */
> > +void sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp);
> > +
> > +/*
> > + * This is an internal function to dispath an RPC call that has
> > + * arrived via IPC over an MU. It is called by server-side SCFW.
> > + *
> > + * @param[in]     mu          MU message arrived on
> > + * @param[in,out] msg         handle to a message
> > + *
> > + * The function result is returned in msg.
> > + */
> > +void sc_rpc_dispatch(sc_rsrc_t mu, sc_rpc_msg_t *msg);
> 
> Declared but never defined.
> 
> > +
> > +/*
> > + * This function translates an RPC message and forwards on to the
> > + * normal RPC API.  It is used only by hypervisors.
> > + *
> > + * @param[in]     ipc         IPC handle
> > + * @param[in,out] msg         handle to a message
> > + *
> > + * This function decodes a message, calls macros to translate the
> > + * resources, pins, addresses, partitions, memory regions, etc. and
> > + * then forwards on to the hypervisors SCFW API.Return results are
> > + * translated back abd placed back into the message to be returned
> > + * to the original API.
> > + */
> > +void sc_rpc_xlate(sc_ipc_t ipc, sc_rpc_msg_t *msg);
> 
> ditto.
> 
> > +
> > +#endif /* _SC_RPC_H */
> > diff --git a/drivers/soc/imx/sc/svc/irq/rpc.h
> > b/drivers/soc/imx/sc/svc/irq/rpc.h
> > new file mode 100644
> > index 0000000..0de8718
> > --- /dev/null
> > +++ b/drivers/soc/imx/sc/svc/irq/rpc.h
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> > + * Copyright 2017~2018 NXP
> > + *
> > + * Header file for the IRQ RPC implementation.
> > + */
> > +
> > +#ifndef _SC_IRQ_RPC_H
> > +#define _SC_IRQ_RPC_H
> > +
> > +/* Types */
> > +
> > +/*!
> > + * This type is used to indicate RPC IRQ function calls.
> > + */
> > +typedef enum irq_func_e {
> > +	IRQ_FUNC_UNKNOWN = 0,	/* Unknown function */
> > +	IRQ_FUNC_ENABLE = 1,	/* Index for irq_enable() RPC call */
> > +	IRQ_FUNC_STATUS = 2,	/* Index for irq_status() RPC call */
> > +} irq_func_t;
> > +
> > +/* Functions */
> > +
> > +/*!
> > + * This function dispatches an incoming IRQ RPC request.
> > + *
> > + * @param[in]     caller_pt   caller partition
> > + * @param[in]     msg         pointer to RPC message
> > + */
> > +void irq_dispatch(sc_rm_pt_t caller_pt, sc_rpc_msg_t *msg);
> 
> ditto.
> 
> > +
> > +/*!
> > + * This function translates and dispatches an IRQ RPC request.
> > + *
> > + * @param[in]     ipc         IPC handle
> > + * @param[in]     msg         pointer to RPC message
> > + */
> > +void irq_xlate(sc_ipc_t ipc, sc_rpc_msg_t *msg);
> 
> ditto.
> 
> > +++ b/include/soc/imx/sc/svc/timer/api.h
> > @@ -0,0 +1,265 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> > + * Copyright 2017~2018 NXP
> > + *
> > + * Header file containing the public API for the System Controller
> > +(SC)
> > + * Timer function.
> > + *
> > + * TIMER_SVC (SVC) Timer Service
> > + *
> > + * Module for the Timer service. This includes support for the
> > +watchdog, RTC,
> > + * and system counter. Note every resource partition has a watchdog
> > +it can
> > + * use.
> > + */
> > +
> > +#ifndef _SC_TIMER_API_H
> > +#define _SC_TIMER_API_H
> > +
> > +/* Includes */
> > +
> > +#include <soc/imx/sc/types.h>
> > +#include <soc/imx/sc/svc/rm/api.h>
> > +
> > +/* Defines */
> > +
> > +/*
> > + * @name Defines for type widths
> > + */
> > +#define SC_TIMER_ACTION_W   3	/* Width of sc_timer_wdog_action_t
> */
> > +
> > +/*
> > + * @name Defines for sc_timer_wdog_action_t  */
> > +#define SC_TIMER_WDOG_ACTION_PARTITION      0	/* Reset
> partition */
> > +#define SC_TIMER_WDOG_ACTION_WARM           1	/* Warm reset
> system */
> > +#define SC_TIMER_WDOG_ACTION_COLD           2	/* Cold reset system
> */
> > +#define SC_TIMER_WDOG_ACTION_BOARD          3	/* Reset board */
> > +#define SC_TIMER_WDOG_ACTION_IRQ            4	/* Only generate
> IRQs */
> > +
> > +/* Types */
> > +
> > +/*
> > + * This type is used to configure the watchdog action.
> > + */
> > +typedef uint8_t sc_timer_wdog_action_t;
> 
> Mixing an unnecessary typedef and defines doesn't look good. This should
> be
> 
> typedef enum {
> 	SC_TIMER_WDOG_ACTION_PARTITION,
> } sc_timer_wdog_action_t;
> 
> The same pattern is used elsewhere in this patch.
> 

After checking with SCU firmware owner, we know that the SCU firmware actually
did that way before, but changed to mixed typedef and defines due to below reasons:
1) emum size is undefined. Usually is int. But wdog_action_t, others are similar,
is defined as u8 in SCU communication protocol which means we can't use it without
an explicitly casting.

2) Follow MISRA C requirement
For enum, Math is not allowed on them. Cannot iterate over them. Can't hardly use at
all without cast.

That's why we decided to eliminate some using of enum in SC firmware to pass the
MISRA C scan.

And for the RPC_X macros, I still did not remove them due to it's used in all C files
which is rather than a simple head file change.

I already sent out an updated version for review.
Please check it. If you still think it's quite necessary to change, please feel free to
let me know.

Regards
Dong Aisheng

> > +
> > +/*
> > + * This type is used to declare a watchdog time value in milliseconds.
> > + */
> > +typedef uint32_t sc_timer_wdog_time_t;
> 
> Why an extra type for this?
> 
> Sascha
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.pengutronix.de%2F&data=02%7C01%7Caisheng.dong%40nxp.com%7C2d0
> 5ba17b0d24957022a08d5b0140906%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C636608522458461041&sdata=WMYanmj1AZkESu0fbXrspidjPbVc
> t3sKQYNhGt5J9ME%3D&reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |