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

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

[Dong Aisheng]

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

It mainly consists of below parts:
1) MU 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 (5):
  dt-bindings: mailbox: allow mbox-cells to be equal to 0
  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   |  34 +
 .../devicetree/bindings/arm/freescale/fsl,scu.txt  |  39 ++
 .../devicetree/bindings/mailbox/mailbox.txt        |   3 +-
 drivers/soc/imx/Kconfig                            |   7 +
 drivers/soc/imx/Makefile                           |   2 +
 drivers/soc/imx/imx_mu.c                           | 184 +++++
 drivers/soc/imx/sc/Makefile                        |   8 +
 drivers/soc/imx/sc/main/ipc.c                      | 264 +++++++
 drivers/soc/imx/sc/main/rpc.h                      |  81 +++
 drivers/soc/imx/sc/svc/irq/rpc.h                   |  23 +
 drivers/soc/imx/sc/svc/irq/rpc_clnt.c              |  58 ++
 drivers/soc/imx/sc/svc/misc/rpc.h                  |  40 ++
 drivers/soc/imx/sc/svc/misc/rpc_clnt.c             | 368 ++++++++++
 drivers/soc/imx/sc/svc/pad/rpc.h                   |  37 +
 drivers/soc/imx/sc/svc/pad/rpc_clnt.c              |  53 ++
 drivers/soc/imx/sc/svc/pm/rpc.h                    |  40 ++
 drivers/soc/imx/sc/svc/pm/rpc_clnt.c               | 393 +++++++++++
 drivers/soc/imx/sc/svc/rm/rpc.h                    |  52 ++
 drivers/soc/imx/sc/svc/rm/rpc_clnt.c               | 612 +++++++++++++++++
 drivers/soc/imx/sc/svc/timer/rpc.h                 |  34 +
 drivers/soc/imx/sc/svc/timer/rpc_clnt.c            | 295 ++++++++
 include/soc/imx/mu.h                               |  24 +
 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                   |  61 ++
 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 +++++++++++++++++++++
 32 files changed, 5663 insertions(+), 2 deletions(-)
 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 V3 1/5] dt-bindings: mailbox: allow mbox-cells to be equal to 0

[Dong Aisheng]

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

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

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

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

[Dong Aisheng]

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

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

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

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

[Rob Herring]

On Fri, Jun 22, 2018 at 10:11:56PM +0800, Dong Aisheng wrote:
> Mailbox devices may have only one channel which means the mbox-cells
> at least 1 does not make sense for this type devices. Let's remove
> that limitation to allow the mbox-cells to be equal to 0.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> ChangeLog:
>  * New patch introduced in v3 series
>  * Actually there're already users in kernel with mbox-cells set to 0.
>    See:
>    arch/arm/boot/dts/bcm283x.dtsi:145: #mbox-cells = <0>;
> ---
>  Documentation/devicetree/bindings/mailbox/mailbox.txt | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

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

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

[Rob Herring]

On Fri, Jun 22, 2018 at 10:11:56PM +0800, Dong Aisheng wrote:
> Mailbox devices may have only one channel which means the mbox-cells
> at least 1 does not make sense for this type devices. Let's remove
> that limitation to allow the mbox-cells to be equal to 0.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: devicetree at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> ChangeLog:
>  * New patch introduced in v3 series
>  * Actually there're already users in kernel with mbox-cells set to 0.
>    See:
>    arch/arm/boot/dts/bcm283x.dtsi:145: #mbox-cells = <0>;
> ---
>  Documentation/devicetree/bindings/mailbox/mailbox.txt | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

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

[PATCH V3 2/5] 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>
---
v2->v3:
 * introduce timeout mechanism
 * uint32_t and int32_t changed to u32 and int
 * Change readl/writel_relaxed to readl/writel

v1->v2:
 * introduce struct mu_priv to keep the private iomem info
 * add the corresponding mu_exit()
---
 drivers/soc/imx/Kconfig  |   3 +
 drivers/soc/imx/Makefile |   1 +
 drivers/soc/imx/imx_mu.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++
 include/soc/imx/mu.h     |  24 +++++++
 4 files changed, 212 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..1ab4095
--- /dev/null
+++ b/drivers/soc/imx/imx_mu.c
@@ -0,0 +1,184 @@
+// 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>
+#include <linux/iopoll.h>
+#include <linux/of_address.h>
+#include <linux/slab.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)
+
+#define MU_DATA_TIME_OUT_US	(100 * USEC_PER_MSEC)
+
+struct mu_priv {
+	struct device_node *np;
+	void __iomem *base;
+};
+
+/*
+ * This function sets the Flag n of the MU.
+ */
+int mu_set_fn(struct mu_priv *priv, u32 fn)
+{
+	u32 reg;
+
+	reg = fn & (~MU_CR_Fn_MASK);
+	if (reg > 0)
+		return -EINVAL;
+
+	reg = readl(priv->base + MU_ACR);
+	/*  Clear ABFn. */
+	reg &= ~MU_CR_Fn_MASK;
+	reg |= fn;
+	writel(reg, priv->base + MU_ACR);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mu_set_fn);
+
+/*
+ * This function reads the status from status register.
+ */
+u32 mu_read_status(struct mu_priv *priv)
+{
+	return readl(priv->base + MU_ASR);
+}
+EXPORT_SYMBOL_GPL(mu_read_status);
+
+/*
+ * This function enables specific RX full interrupt.
+ */
+void mu_enable_rx_full_int(struct mu_priv *priv, u32 index)
+{
+	u32 reg;
+
+	reg = readl(priv->base + MU_ACR);
+	reg &= ~(MU_CR_GIRn_MASK | MU_CR_NMI_MASK);
+	reg |= MU_CR_RIE0_MASK >> index;
+	writel(reg, priv->base + MU_ACR);
+}
+EXPORT_SYMBOL_GPL(mu_enable_rx_full_int);
+
+/*
+ * This function enables specific general purpose interrupt.
+ */
+void mu_enable_general_int(struct mu_priv *priv, u32 index)
+{
+	u32 reg;
+
+	reg = readl(priv->base + MU_ACR);
+	reg &= ~(MU_CR_GIRn_MASK | MU_CR_NMI_MASK);
+	reg |= MU_CR_GIE0_MASK >> index;
+	writel(reg, priv->base + MU_ACR);
+}
+EXPORT_SYMBOL_GPL(mu_enable_general_int);
+
+/*
+ * Wait and send message to the other core.
+ */
+int mu_send_msg(struct mu_priv *priv, u32 index, u32 msg)
+{
+	u32 mask, asr;
+	int ret;
+
+	mask = MU_SR_TE0_MASK >> index;
+
+	/* Wait TX register to be empty. */
+	ret = readl_poll_timeout_atomic(priv->base + MU_ASR, asr, asr & mask,
+					0, MU_DATA_TIME_OUT_US);
+	if (ret)
+		return ret;
+
+	writel(msg, priv->base + MU_ATR0  + (index * 4));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mu_send_msg);
+
+/*
+ * Wait to receive message from the other core.
+ */
+int mu_receive_msg(struct mu_priv *priv, u32 index, u32 *msg)
+{
+	u32 mask, asr;
+	int ret;
+
+	mask = MU_SR_RF0_MASK >> index;
+
+	/* Wait RX register to be full. */
+	ret = readl_poll_timeout_atomic(priv->base + MU_ASR, asr, asr & mask,
+					0, MU_DATA_TIME_OUT_US);
+	if (ret)
+		return ret;
+
+	*msg = readl(priv->base + MU_ARR0 + (index * 4));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mu_receive_msg);
+
+struct device_node *mu_node(struct mu_priv *priv)
+{
+	return priv ? priv->np : NULL;
+}
+EXPORT_SYMBOL_GPL(mu_node);
+
+struct mu_priv *mu_init(struct device_node *np)
+{
+	struct mu_priv *priv;
+	u32 reg;
+
+	if (WARN_ON(!np))
+		return NULL;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return ERR_PTR(-ENOMEM);
+
+	priv->np = np;
+	priv->base = of_iomap(np, 0);
+	if (!priv->base) {
+		kfree(priv);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	reg = readl(priv->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(reg, priv->base + MU_ACR);
+
+	return priv;
+}
+EXPORT_SYMBOL_GPL(mu_init);
+
+void mu_exit(struct mu_priv *priv)
+{
+	if (WARN_ON(!priv))
+		return;
+
+	iounmap(priv->base);
+	kfree(priv);
+}
+EXPORT_SYMBOL_GPL(mu_exit);
diff --git a/include/soc/imx/mu.h b/include/soc/imx/mu.h
new file mode 100644
index 0000000..44537b8
--- /dev/null
+++ b/include/soc/imx/mu.h
@@ -0,0 +1,24 @@
+/* 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_TR_COUNT		4
+#define MU_RR_COUNT		4
+
+struct mu_priv;
+
+void mu_exit(struct mu_priv *priv);
+struct mu_priv *mu_init(struct device_node *np);
+struct device_node *mu_node(struct mu_priv *priv);
+int mu_send_msg(struct mu_priv *priv, u32 index, u32 msg);
+int mu_receive_msg(struct mu_priv *priv, u32 index, u32 *msg);
+void mu_enable_general_int(struct mu_priv *priv, u32 index);
+void mu_enable_rx_full_int(struct mu_priv *priv, u32 index);
+u32 mu_read_status(struct mu_priv *priv);
+int mu_set_fn(struct mu_priv *priv, u32 Fn);
+#endif
-- 
2.7.4

[PATCH V3 2/5] soc: imx: add mu library functions support

[Sascha Hauer]

On Fri, Jun 22, 2018 at 10:11:57PM +0800, Dong Aisheng wrote:
> This is used for i.MX multi core communication.
> e.g. A core to SCU firmware(M core) on MX8.

I still believe that a generic driver for the MU should be used here.
Handling hardware resources under the hood of the driver framework is a
hack. Preventing the generic driver from matching against the SCU MU by
adding some #mbox-cells = <0>; to the MU device node is even more a
hack.

We should really handle the MU in a driver and look for a way how the
SCU case can coexist with other usages of the MUs.

Your main argument against using the mailbox framework is that it can't
handle polling in the way you need it and that the mailbox framework
provides things that you do not need. I don't buy this argument. In the
end the mailbox framework is around 500 lines of code, it shouldn't be
that hard to add the missing features. From the transmit side I don't
see any showstoppers, the mailbox frameworks could be used as ist. What
is missing is a synchronous wait-for-new-messages and receive-message
call, the currently existing asynchronous rx callback is indeed not
suitable for the SCU. But as said, it should be doable to add that.

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 V3 2/5] soc: imx: add mu library functions support

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Monday, June 25, 2018 5:35 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 V3 2/5] soc: imx: add mu library functions support
> 
> On Fri, Jun 22, 2018 at 10:11:57PM +0800, Dong Aisheng wrote:
> > This is used for i.MX multi core communication.
> > e.g. A core to SCU firmware(M core) on MX8.
> 
> I still believe that a generic driver for the MU should be used here.
> Handling hardware resources under the hood of the driver framework is a
> hack. Preventing the generic driver from matching against the SCU MU by
> adding some #mbox-cells = <0>; to the MU device node is even more a hack.
> 

That is not a hack from a HW point of view. The MU HW does not have multi
channels according to Reference Manual. Even we switch to mailbox, we probably
may still prefer mbox-cell = <0> as the virtual channels do not fit for SCU MU.

> We should really handle the MU in a driver and look for a way how the SCU
> case can coexist with other usages of the MUs.
> 
> Your main argument against using the mailbox framework is that it can't
> handle polling in the way you need it and that the mailbox framework
> provides things that you do not need. I don't buy this argument. In the end
> the mailbox framework is around 500 lines of code, it shouldn't be that hard
> to add the missing features. From the transmit side I don't see any
> showstoppers, the mailbox frameworks could be used as ist. What is missing
> is a synchronous wait-for-new-messages and receive-message call, the
> currently existing asynchronous rx callback is indeed not suitable for the SCU.
> But as said, it should be doable to add that.
> 

Besides the mailbox framework may not suitable for SCU, another important
Reason is that even we force to switch to mailbox, it's still can't be generic driver
and it can only be used by SCU MU. 

Let's see the mailbox doc where it also highlights it may somehow depend on mailbox
communication protocol.

Documentation/mailbox.txt
----------------------------------------------------------------------------------------
This document aims to help developers write client and controller
drivers for the API. But before we start, let us note that the
client (especially) and controller drivers are likely going to be
very platform specific because the remote firmware is likely to be
proprietary and implement non-standard protocol.
.....
----------------------------------------------------------------------------------------

So the question to us is: If it can't be generic driver which can be
used by others as well and it introduces much unnecessary complexities,
why do we need do that? What's real benefits we can have?

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%7Cfe5
> 2cb3aecd3461fab3d08d5da7eefc6%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C636655161075403401&sdata=bRnnlrq82OAlxoli5IHAtXuwieLGJM
> 4raDRvaV8y5gs%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 V3 2/5] soc: imx: add mu library functions support

[A.s. Dong]

> -----Original Message-----
> From: A.s. Dong
> Sent: Monday, June 25, 2018 7:59 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 V3 2/5] soc: imx: add mu library functions support
> 
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Monday, June 25, 2018 5:35 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 V3 2/5] soc: imx: add mu library functions support
> >
> > On Fri, Jun 22, 2018 at 10:11:57PM +0800, Dong Aisheng wrote:
> > > This is used for i.MX multi core communication.
> > > e.g. A core to SCU firmware(M core) on MX8.
> >
> > I still believe that a generic driver for the MU should be used here.
> > Handling hardware resources under the hood of the driver framework is
> > a hack. Preventing the generic driver from matching against the SCU MU
> > by adding some #mbox-cells = <0>; to the MU device node is even more a
> hack.
> >
> 
> That is not a hack from a HW point of view. The MU HW does not have multi
> channels according to Reference Manual. Even we switch to mailbox, we
> probably may still prefer mbox-cell = <0> as the virtual channels do not fit for
> SCU MU.
> 
> > We should really handle the MU in a driver and look for a way how the
> > SCU case can coexist with other usages of the MUs.
> >
> > Your main argument against using the mailbox framework is that it
> > can't handle polling in the way you need it and that the mailbox
> > framework provides things that you do not need. I don't buy this
> > argument. In the end the mailbox framework is around 500 lines of
> > code, it shouldn't be that hard to add the missing features. From the
> > transmit side I don't see any showstoppers, the mailbox frameworks
> > could be used as ist. What is missing is a synchronous
> > wait-for-new-messages and receive-message call, the currently existing
> asynchronous rx callback is indeed not suitable for the SCU.
> > But as said, it should be doable to add that.
> >
> 
> Besides the mailbox framework may not suitable for SCU, another important
> Reason is that even we force to switch to mailbox, it's still can't be generic
> driver and it can only be used by SCU MU.
> 
> Let's see the mailbox doc where it also highlights it may somehow depend on
> mailbox communication protocol.
> 
> Documentation/mailbox.txt
> ----------------------------------------------------------------------------------------
> This document aims to help developers write client and controller drivers for
> the API. But before we start, let us note that the client (especially) and
> controller drivers are likely going to be very platform specific because the
> remote firmware is likely to be proprietary and implement non-standard
> protocol.
> .....
> ----------------------------------------------------------------------------------------
> 
> So the question to us is: If it can't be generic driver which can be used by
> others as well and it introduces much unnecessary complexities, why do we
> need do that? What's real benefits we can have?
> 

Another option is we can try to improve mailbox framework to fit
for SCU MU user case, but that may be a long time which will block all
QXP bits upstreaming as most QXP parts are based on SCU API call.
How about do that as an improvement investigation later? As the underlying
IPC communication mechanism is somehow separate and independent of
the high layer service and client users and current implementation is quite
light one, won't cause much extra code.
That won't block QXP upstream and can also have an in-tree comparison
to the old ways.

How do you think of it?

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%7Cfe5
> > 2cb3aecd3461fab3d08d5da7eefc6%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> >
> 7C0%7C0%7C636655161075403401&sdata=bRnnlrq82OAlxoli5IHAtXuwieLGJM
> > 4raDRvaV8y5gs%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 V3 2/5] soc: imx: add mu library functions support

[Sascha Hauer]

On Mon, Jun 25, 2018 at 11:59:04AM +0000, A.s. Dong wrote:
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Monday, June 25, 2018 5:35 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 V3 2/5] soc: imx: add mu library functions support
> > 
> > On Fri, Jun 22, 2018 at 10:11:57PM +0800, Dong Aisheng wrote:
> > > This is used for i.MX multi core communication.
> > > e.g. A core to SCU firmware(M core) on MX8.
> > 
> > I still believe that a generic driver for the MU should be used here.
> > Handling hardware resources under the hood of the driver framework is a
> > hack. Preventing the generic driver from matching against the SCU MU by
> > adding some #mbox-cells = <0>; to the MU device node is even more a hack.
> > 
> 
> That is not a hack from a HW point of view. The MU HW does not have multi
> channels according to Reference Manual. Even we switch to mailbox, we probably
> may still prefer mbox-cell = <0> as the virtual channels do not fit for SCU MU.

If you switch to mailbox then you'll need something for the driver to
distinguish between the different transfer modes. One possibility would
be to introduce channels like I suggested earlier, so one channel could
simply mean "transfer in SCU mode".

> 
> > We should really handle the MU in a driver and look for a way how the SCU
> > case can coexist with other usages of the MUs.
> > 
> > Your main argument against using the mailbox framework is that it can't
> > handle polling in the way you need it and that the mailbox framework
> > provides things that you do not need. I don't buy this argument. In the end
> > the mailbox framework is around 500 lines of code, it shouldn't be that hard
> > to add the missing features. From the transmit side I don't see any
> > showstoppers, the mailbox frameworks could be used as ist. What is missing
> > is a synchronous wait-for-new-messages and receive-message call, the
> > currently existing asynchronous rx callback is indeed not suitable for the SCU.
> > But as said, it should be doable to add that.
> > 
> 
> Besides the mailbox framework may not suitable for SCU, another important
> Reason is that even we force to switch to mailbox, it's still can't be generic driver
> and it can only be used by SCU MU.

You claim that the driver can't be generic. I claim the opposite though.

> 
> Let's see the mailbox doc where it also highlights it may somehow depend on mailbox
> communication protocol.
> 
> Documentation/mailbox.txt
> ----------------------------------------------------------------------------------------
> This document aims to help developers write client and controller
> drivers for the API. But before we start, let us note that the
> client (especially) and controller drivers are likely going to be
> very platform specific because the remote firmware is likely to be
> proprietary and implement non-standard protocol.
> .....

Read on:

| So even if two platforms employ, say, PL320 controller, the client drivers
| can't be shared across them. Even the PL320 driver might need to accommodate
| some platform specific quirks.

Here the MU would be the PL320 and the SCU mode would be the platform
specific quirks.

> ----------------------------------------------------------------------------------------
> 
> So the question to us is: If it can't be generic driver which can be
> used by others as well and it introduces much unnecessary complexities,
> why do we need do that? What's real benefits we can have?

The driver can be used by others, the additional complexity is not that
much and your code may be in a more upstreamable shape.

We are interested in a MU driver that is working for the generic M4
case and we are not interested in cleaning up after you when the adhoc
MU access code is merged and in the way of a proper driver.

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 V3 2/5] soc: imx: add mu library functions support

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Monday, June 25, 2018 9:47 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 V3 2/5] soc: imx: add mu library functions support
> 
> On Mon, Jun 25, 2018 at 11:59:04AM +0000, A.s. Dong wrote:
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > Sent: Monday, June 25, 2018 5:35 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 V3 2/5] soc: imx: add mu library functions
> > > support
> > >
> > > On Fri, Jun 22, 2018 at 10:11:57PM +0800, Dong Aisheng wrote:
> > > > This is used for i.MX multi core communication.
> > > > e.g. A core to SCU firmware(M core) on MX8.
> > >
> > > I still believe that a generic driver for the MU should be used here.
> > > Handling hardware resources under the hood of the driver framework
> > > is a hack. Preventing the generic driver from matching against the
> > > SCU MU by adding some #mbox-cells = <0>; to the MU device node is
> even more a hack.
> > >
> >
> > That is not a hack from a HW point of view. The MU HW does not have
> > multi channels according to Reference Manual. Even we switch to
> > mailbox, we probably may still prefer mbox-cell = <0> as the virtual
> channels do not fit for SCU MU.
> 
> If you switch to mailbox then you'll need something for the driver to
> distinguish between the different transfer modes. One possibility would be
> to introduce channels like I suggested earlier, so one channel could simply
> mean "transfer in SCU mode".
> 

I feel that may be a little strange as device tree is used to description HW.
Mbox-cells = <0> is more reflecting the real HW, right?

> >
> > > We should really handle the MU in a driver and look for a way how
> > > the SCU case can coexist with other usages of the MUs.
> > >
> > > Your main argument against using the mailbox framework is that it
> > > can't handle polling in the way you need it and that the mailbox
> > > framework provides things that you do not need. I don't buy this
> > > argument. In the end the mailbox framework is around 500 lines of
> > > code, it shouldn't be that hard to add the missing features. From
> > > the transmit side I don't see any showstoppers, the mailbox
> > > frameworks could be used as ist. What is missing is a synchronous
> > > wait-for-new-messages and receive-message call, the currently existing
> asynchronous rx callback is indeed not suitable for the SCU.
> > > But as said, it should be doable to add that.
> > >
> >
> > Besides the mailbox framework may not suitable for SCU, another
> > important Reason is that even we force to switch to mailbox, it's
> > still can't be generic driver and it can only be used by SCU MU.
> 
> You claim that the driver can't be generic. I claim the opposite though.
> 

The 'generic' I  mean here is a generic driver without any sense of protocol.
It only transfer a number of data. That means we need develop a generic
data transfer protocol to send only data without knowing any protocol
specific things.

AFAIK SCU MU used data transfer protocol is different from M4 in Oleksij
Rempel's patch series. That means we need some specific hacks to support
both protocols. Then it's not a generic to me. 

But it might be acceptable if the hack is quite small. However, I can't see
that up till now as M4 protocol based on virtual channel is quite different
from SCU MU, that means the hack will be big which may lose the 'generic'
meaning.

> >
> > Let's see the mailbox doc where it also highlights it may somehow
> > depend on mailbox communication protocol.
> >
> > Documentation/mailbox.txt
> > ----------------------------------------------------------------------
> > ------------------ This document aims to help developers write client
> > and controller drivers for the API. But before we start, let us note
> > that the client (especially) and controller drivers are likely going
> > to be very platform specific because the remote firmware is likely to
> > be proprietary and implement non-standard protocol.
> > .....
> 
> Read on:
> 
> | So even if two platforms employ, say, PL320 controller, the client
> | drivers can't be shared across them. Even the PL320 driver might need
> | to accommodate some platform specific quirks.
> 
> Here the MU would be the PL320 and the SCU mode would be the platform
> specific quirks.
> 

SCU can be platform specific, but how to make MU generic to be unware of SCU
transfer protocol?

The current MU transfer protocol is already specific to SCU.
See:
typedef struct sc_rpc_msg_s {
        uint8_t version;
        uint8_t size; 
        uint8_t svc;
        uint8_t func;
        union {
                int32_t i32[(SC_RPC_MAX_MSG - 1)];
                int16_t i16[(SC_RPC_MAX_MSG - 1) * 2];
                int8_t i8[(SC_RPC_MAX_MSG - 1) * 4];
                uint32_t u32[(SC_RPC_MAX_MSG - 1)];
                uint16_t u16[(SC_RPC_MAX_MSG - 1) * 2];
                uint8_t u8[(SC_RPC_MAX_MSG - 1) * 4];
        } DATA;
} sc_rpc_msg_t;

Where MU knows the size to transfer.

> > ----------------------------------------------------------------------
> > ------------------
> >
> > So the question to us is: If it can't be generic driver which can be
> > used by others as well and it introduces much unnecessary
> > complexities, why do we need do that? What's real benefits we can have?
> 
> The driver can be used by others, the additional complexity is not that much
> and your code may be in a more upstreamable shape.
> 

Okay, then let's try to see the complexity if trying to support two type protocols
in a 'generic' driver:

As I said earlier, we internally have tried mailbox already, and the implementation is:
1) One channel per MU
2) Tx using the same way to send msg as sc_ipc_write() in polling mode
3) Rx enables the first word interrupt and polling for the rest of them in a hrtimer.

M4 is quite different in Oleksij Rempel's patch.
The questions to us is:
1) what's generic data transfer protocol should we use?
In Oleksij's patch, MU is unware of the data size and only send one word each time.
For data size, we probably can handle in client driver, but that means the client can
only send one word per time which loses the performance a lot. If we want the MU
driver to support send multi word data, MU must know how many words to send.
then a new data transfer protocol support needs to be added for SCU alike clients.
And we still need to support new polling way to handle multi words transfer.
BE NOTED that, SCU can transfer over 4 words, receiving is the same.
This is a big complexity comparing to Oleksij's patch.

To support this type protocol, one reference may be Ti:
include/linux/soc/ti/ti-msgmgr.h
struct ti_msgmgr_message {
        size_t len;
        u8 *buf;
};  
But we still need a simple hack for SCU specific as SCU size is the second u8 type.
If (!is_scu) 
   Buf = msg->buf;
   Len = Msg->len
Else
   Buf = msg_scu->buf;
   Len = msg_scu->size;

2) how to handle Rx in a generic way?
The same issue as above, for SCU alike clients, receiving by one word a time
Is low effiency. We need a high speed polling way as we did in library way now.
That means we need implement an active polling way in 'generic' MU for SCU alike
clients where current mailbox framework seems still not support well.
That's another totally brand new and complicated bits comparing to
Oleksij's patch.

All those things obviously are not small as you assumed!

They are much more complicated than the simple interrupt driven way
in Oleksij's patch. Just looks like to re-implement another new MU driver
specific to SCU alike clients, but do we really have another SCU alike clients?
And we need force to merge those much bigger bits into a generic MU driver
for M4 which actually won't be used by M4 just in order to be a mailbox alike driver
and may sacrifice the performance due to interrupt latency and etc.

It's really hard to believe it's a worth thing to do, especially at this stage because
that may blocks us at least a couple of month for MX8 upstreaming and waiting that
do be done first before anything else can keep going on.

IMHO unless we can find another real requirement for SCU alike protocols client,
Otherwise, it's really not worth to implement those bits into generic MU drivers
at this point. Or we fully change to TI alike protocol as above and don't support
virtual channels? That's at least more like SCU protocol.

> We are interested in a MU driver that is working for the generic M4 case and
> we are not interested in cleaning up after you when the adhoc MU access
> code is merged and in the way of a proper driver.
> 

This patch is not aim to support M4 as current kernel still does not support M4.
We don't have to figure out everything in one patch, right?
That will make us hard to do even a simple thing.

Mailbox support obviously is another thing which is independent of this patch
series. They're not conflict thing. You obviously could extend your MU driver
to support SCU as well. If it's proved to better than current way, we definitely
could switch to it later. It's not clean up old code, it's just simply switch to a
new 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%7C39f
> 8ac2353c44259e29108d5daa21a48%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C636655312115361407&sdata=LwI83IgradQL2AYSnQMg64vxTqVV9
> TTJlWgpRN1VxdI%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 V3 2/5] soc: imx: add mu library functions support

[Sascha Hauer]

On Tue, Jun 26, 2018 at 08:53:55AM +0000, A.s. Dong wrote:
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Monday, June 25, 2018 9:47 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 V3 2/5] soc: imx: add mu library functions support
> > 
> > On Mon, Jun 25, 2018 at 11:59:04AM +0000, A.s. Dong wrote:
> > > > -----Original Message-----
> > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > Sent: Monday, June 25, 2018 5:35 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 V3 2/5] soc: imx: add mu library functions
> > > > support
> > > >
> > > > On Fri, Jun 22, 2018 at 10:11:57PM +0800, Dong Aisheng wrote:
> > > > > This is used for i.MX multi core communication.
> > > > > e.g. A core to SCU firmware(M core) on MX8.
> > > >
> > > > I still believe that a generic driver for the MU should be used here.
> > > > Handling hardware resources under the hood of the driver framework
> > > > is a hack. Preventing the generic driver from matching against the
> > > > SCU MU by adding some #mbox-cells = <0>; to the MU device node is
> > even more a hack.
> > > >
> > >
> > > That is not a hack from a HW point of view. The MU HW does not have
> > > multi channels according to Reference Manual. Even we switch to
> > > mailbox, we probably may still prefer mbox-cell = <0> as the virtual
> > channels do not fit for SCU MU.
> > 
> > If you switch to mailbox then you'll need something for the driver to
> > distinguish between the different transfer modes. One possibility would be
> > to introduce channels like I suggested earlier, so one channel could simply
> > mean "transfer in SCU mode".
> > 
> 
> I feel that may be a little strange as device tree is used to description HW.
> Mbox-cells = <0> is more reflecting the real HW, right?

The SCU code is Software, so once we try to bind a driver to the SCU we
are leaving hardware description anyway. The number of channels we have
is defined by software. Consider the case when SCU would expect pinmux
calls in MU tx register 0, clock control in register 1 and other calls
in registers 2 and 3. You actually *can* see the MU as a unit with
multiple channels, so why shouldn't we describe this in the device tree?

> 
> > >
> > > > We should really handle the MU in a driver and look for a way how
> > > > the SCU case can coexist with other usages of the MUs.
> > > >
> > > > Your main argument against using the mailbox framework is that it
> > > > can't handle polling in the way you need it and that the mailbox
> > > > framework provides things that you do not need. I don't buy this
> > > > argument. In the end the mailbox framework is around 500 lines of
> > > > code, it shouldn't be that hard to add the missing features. From
> > > > the transmit side I don't see any showstoppers, the mailbox
> > > > frameworks could be used as ist. What is missing is a synchronous
> > > > wait-for-new-messages and receive-message call, the currently existing
> > asynchronous rx callback is indeed not suitable for the SCU.
> > > > But as said, it should be doable to add that.
> > > >
> > >
> > > Besides the mailbox framework may not suitable for SCU, another
> > > important Reason is that even we force to switch to mailbox, it's
> > > still can't be generic driver and it can only be used by SCU MU.
> > 
> > You claim that the driver can't be generic. I claim the opposite though.
> > 
> 
> The 'generic' I  mean here is a generic driver without any sense of protocol.
> It only transfer a number of data. That means we need develop a generic
> data transfer protocol to send only data without knowing any protocol
> specific things.
> 
> AFAIK SCU MU used data transfer protocol is different from M4 in Oleksij
> Rempel's patch series. That means we need some specific hacks to support
> both protocols. Then it's not a generic to me.

Yes, indeed. You'll need to be SCU mode aware in the driver. No doubt
about this.

> 
> But it might be acceptable if the hack is quite small. However, I can't see
> that up till now as M4 protocol based on virtual channel is quite different
> from SCU MU, that means the hack will be big which may lose the 'generic'
> meaning.
> 
> > >
> > > Let's see the mailbox doc where it also highlights it may somehow
> > > depend on mailbox communication protocol.
> > >
> > > Documentation/mailbox.txt
> > > ----------------------------------------------------------------------
> > > ------------------ This document aims to help developers write client
> > > and controller drivers for the API. But before we start, let us note
> > > that the client (especially) and controller drivers are likely going
> > > to be very platform specific because the remote firmware is likely to
> > > be proprietary and implement non-standard protocol.
> > > .....
> > 
> > Read on:
> > 
> > | So even if two platforms employ, say, PL320 controller, the client
> > | drivers can't be shared across them. Even the PL320 driver might need
> > | to accommodate some platform specific quirks.
> > 
> > Here the MU would be the PL320 and the SCU mode would be the platform
> > specific quirks.
> > 
> 
> SCU can be platform specific, but how to make MU generic to be unware of SCU
> transfer protocol?
> 
> The current MU transfer protocol is already specific to SCU.
> See:
> typedef struct sc_rpc_msg_s {
>         uint8_t version;
>         uint8_t size; 
>         uint8_t svc;
>         uint8_t func;
>         union {
>                 int32_t i32[(SC_RPC_MAX_MSG - 1)];
>                 int16_t i16[(SC_RPC_MAX_MSG - 1) * 2];
>                 int8_t i8[(SC_RPC_MAX_MSG - 1) * 4];
>                 uint32_t u32[(SC_RPC_MAX_MSG - 1)];
>                 uint16_t u16[(SC_RPC_MAX_MSG - 1) * 2];
>                 uint8_t u8[(SC_RPC_MAX_MSG - 1) * 4];
>         } DATA;
> } sc_rpc_msg_t;
> 
> Where MU knows the size to transfer.

The mailbox send hook takes a void *. You can interpret this pointer how
you like, that's part of the design decision with the mailbox framework.
There's no need to interpret this void * always in the same manner in
your driver, nothing prevent us from casting it to some SCU message for
the SCU case and to something different in !SCU case.

> 
> > > ----------------------------------------------------------------------
> > > ------------------
> > >
> > > So the question to us is: If it can't be generic driver which can be
> > > used by others as well and it introduces much unnecessary
> > > complexities, why do we need do that? What's real benefits we can have?
> > 
> > The driver can be used by others, the additional complexity is not that much
> > and your code may be in a more upstreamable shape.
> > 
> 
> Okay, then let's try to see the complexity if trying to support two type protocols
> in a 'generic' driver:
> 
> As I said earlier, we internally have tried mailbox already, and the implementation is:
> 1) One channel per MU
> 2) Tx using the same way to send msg as sc_ipc_write() in polling mode
> 3) Rx enables the first word interrupt and polling for the rest of them in a hrtimer.

And that's where the current mailbox framework lacks real polling mode.

> 
> M4 is quite different in Oleksij Rempel's patch.
> The questions to us is:
> 1) what's generic data transfer protocol should we use?

None. As mentioned above, in SCU case cast it to whatever you need.

> 
> This patch is not aim to support M4 as current kernel still does not support M4.
> We don't have to figure out everything in one patch, right?
> That will make us hard to do even a simple thing.

No, and I don't expect you to implement M4 support, and in fact it is
not clear if Oleksijs patch implements the M4 support in a way we
actually need it later.

> 
> Mailbox support obviously is another thing which is independent of this patch
> series. They're not conflict thing. You obviously could extend your MU driver
> to support SCU as well. If it's proved to better than current way, we definitely
> could switch to it later. It's not clean up old code, it's just simply switch to a
> new way.

Provided you don't use a mailbox driver then we'll have to prevent a
mailbox driver from binding to the device that is used for the SCU. And
yes, that's what I consider having to cleanup after you.

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 V3 2/5] soc: imx: add mu library functions support

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Tuesday, June 26, 2018 8:01 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 V3 2/5] soc: imx: add mu library functions support
> 
> On Tue, Jun 26, 2018 at 08:53:55AM +0000, A.s. Dong wrote:
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > Sent: Monday, June 25, 2018 9:47 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 V3 2/5] soc: imx: add mu library functions
> > > support
> > >
> > > On Mon, Jun 25, 2018 at 11:59:04AM +0000, A.s. Dong wrote:
> > > > > -----Original Message-----
> > > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > > Sent: Monday, June 25, 2018 5:35 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 V3 2/5] soc: imx: add mu library functions
> > > > > support
> > > > >
> > > > > On Fri, Jun 22, 2018 at 10:11:57PM +0800, Dong Aisheng wrote:
> > > > > > This is used for i.MX multi core communication.
> > > > > > e.g. A core to SCU firmware(M core) on MX8.
> > > > >
> > > > > I still believe that a generic driver for the MU should be used here.
> > > > > Handling hardware resources under the hood of the driver
> > > > > framework is a hack. Preventing the generic driver from matching
> > > > > against the SCU MU by adding some #mbox-cells = <0>; to the MU
> > > > > device node is
> > > even more a hack.
> > > > >
> > > >
> > > > That is not a hack from a HW point of view. The MU HW does not
> > > > have multi channels according to Reference Manual. Even we switch
> > > > to mailbox, we probably may still prefer mbox-cell = <0> as the
> > > > virtual
> > > channels do not fit for SCU MU.
> > >
> > > If you switch to mailbox then you'll need something for the driver
> > > to distinguish between the different transfer modes. One possibility
> > > would be to introduce channels like I suggested earlier, so one
> > > channel could simply mean "transfer in SCU mode".
> > >
> >
> > I feel that may be a little strange as device tree is used to description HW.
> > Mbox-cells = <0> is more reflecting the real HW, right?
> 
> The SCU code is Software, so once we try to bind a driver to the SCU we are
> leaving hardware description anyway. 

I'm not quite agree with this. SCU is just using MU to send messages.
We are still describing the MU in device tree from HW point of view.
SCU does not change the MU HW characteristics.

> The number of channels we have is defined by software.
> Consider the case when SCU would expect pinmux calls
> in MU tx register 0, clock control in register 1 and other calls in registers 2 and
> 3. You actually *can* see the MU as a unit with multiple channels, so why
> shouldn't we describe this in the device tree?
> 

Yes, SW does can abstract virtual channels according to HW capability.
That why I'm not against the virtual channels in Oleksij's patch.
But it's just not quite suitable for SCU MU case.

> >
> > > >
> > > > > We should really handle the MU in a driver and look for a way
> > > > > how the SCU case can coexist with other usages of the MUs.
> > > > >
> > > > > Your main argument against using the mailbox framework is that
> > > > > it can't handle polling in the way you need it and that the
> > > > > mailbox framework provides things that you do not need. I don't
> > > > > buy this argument. In the end the mailbox framework is around
> > > > > 500 lines of code, it shouldn't be that hard to add the missing
> > > > > features. From the transmit side I don't see any showstoppers,
> > > > > the mailbox frameworks could be used as ist. What is missing is
> > > > > a synchronous wait-for-new-messages and receive-message call,
> > > > > the currently existing
> > > asynchronous rx callback is indeed not suitable for the SCU.
> > > > > But as said, it should be doable to add that.
> > > > >
> > > >
> > > > Besides the mailbox framework may not suitable for SCU, another
> > > > important Reason is that even we force to switch to mailbox, it's
> > > > still can't be generic driver and it can only be used by SCU MU.
> > >
> > > You claim that the driver can't be generic. I claim the opposite though.
> > >
> >
> > The 'generic' I  mean here is a generic driver without any sense of protocol.
> > It only transfer a number of data. That means we need develop a
> > generic data transfer protocol to send only data without knowing any
> > protocol specific things.
> >
> > AFAIK SCU MU used data transfer protocol is different from M4 in
> > Oleksij Rempel's patch series. That means we need some specific hacks
> > to support both protocols. Then it's not a generic to me.
> 
> Yes, indeed. You'll need to be SCU mode aware in the driver. No doubt about
> this.
> 
> >
> > But it might be acceptable if the hack is quite small. However, I
> > can't see that up till now as M4 protocol based on virtual channel is
> > quite different from SCU MU, that means the hack will be big which may
> lose the 'generic'
> > meaning.
> >
> > > >
> > > > Let's see the mailbox doc where it also highlights it may somehow
> > > > depend on mailbox communication protocol.
> > > >
> > > > Documentation/mailbox.txt
> > > > ------------------------------------------------------------------
> > > > ----
> > > > ------------------ This document aims to help developers write
> > > > client and controller drivers for the API. But before we start,
> > > > let us note that the client (especially) and controller drivers
> > > > are likely going to be very platform specific because the remote
> > > > firmware is likely to be proprietary and implement non-standard
> protocol.
> > > > .....
> > >
> > > Read on:
> > >
> > > | So even if two platforms employ, say, PL320 controller, the client
> > > | drivers can't be shared across them. Even the PL320 driver might
> > > | need to accommodate some platform specific quirks.
> > >
> > > Here the MU would be the PL320 and the SCU mode would be the
> > > platform specific quirks.
> > >
> >
> > SCU can be platform specific, but how to make MU generic to be unware
> > of SCU transfer protocol?
> >
> > The current MU transfer protocol is already specific to SCU.
> > See:
> > typedef struct sc_rpc_msg_s {
> >         uint8_t version;
> >         uint8_t size;
> >         uint8_t svc;
> >         uint8_t func;
> >         union {
> >                 int32_t i32[(SC_RPC_MAX_MSG - 1)];
> >                 int16_t i16[(SC_RPC_MAX_MSG - 1) * 2];
> >                 int8_t i8[(SC_RPC_MAX_MSG - 1) * 4];
> >                 uint32_t u32[(SC_RPC_MAX_MSG - 1)];
> >                 uint16_t u16[(SC_RPC_MAX_MSG - 1) * 2];
> >                 uint8_t u8[(SC_RPC_MAX_MSG - 1) * 4];
> >         } DATA;
> > } sc_rpc_msg_t;
> >
> > Where MU knows the size to transfer.
> 
> The mailbox send hook takes a void *. You can interpret this pointer how you
> like, that's part of the design decision with the mailbox framework.
> There's no need to interpret this void * always in the same manner in your
> driver, nothing prevent us from casting it to some SCU message for the SCU
> case and to something different in !SCU case.

The point is that means we need handle protocol specific thing in MU driver
which should be protocol unware.
Looking at arm mhu and ti mu:
drivers/mailbox/arm_mhu.c
drivers/mailbox/ti-msgmgr.c
I've studies them carefully and none of them are protocol aware.

But I'm not strongly against it if the protocol specific bits is not much.
The problem is how do we handle them in a proper way to avoid too much
complexities added in the MU driver which should be generic.

> 
> >
> > > > ------------------------------------------------------------------
> > > > ----
> > > > ------------------
> > > >
> > > > So the question to us is: If it can't be generic driver which can
> > > > be used by others as well and it introduces much unnecessary
> > > > complexities, why do we need do that? What's real benefits we can
> have?
> > >
> > > The driver can be used by others, the additional complexity is not
> > > that much and your code may be in a more upstreamable shape.
> > >
> >
> > Okay, then let's try to see the complexity if trying to support two
> > type protocols in a 'generic' driver:
> >
> > As I said earlier, we internally have tried mailbox already, and the
> implementation is:
> > 1) One channel per MU
> > 2) Tx using the same way to send msg as sc_ipc_write() in polling mode
> > 3) Rx enables the first word interrupt and polling for the rest of them in a
> hrtimer.
> 
> And that's where the current mailbox framework lacks real polling mode.
> 

Yes, that means we need extend it to support SCU alike clients well.

> >
> > M4 is quite different in Oleksij Rempel's patch.
> > The questions to us is:
> > 1) what's generic data transfer protocol should we use?
> 
> None. As mentioned above, in SCU case cast it to whatever you need.
> 

Same as I explained above. Cast it in MU driver means we bring protocol
specific bits into generic MU driver. Especially if we want to support
more than one protocols.

> >
> > This patch is not aim to support M4 as current kernel still does not support
> M4.
> > We don't have to figure out everything in one patch, right?
> > That will make us hard to do even a simple thing.
> 
> No, and I don't expect you to implement M4 support, and in fact it is not clear
> if Oleksijs patch implements the M4 support in a way we actually need it later.
> 

That did confuse me before as I thought you wanted that way.

> >
> > Mailbox support obviously is another thing which is independent of
> > this patch series. They're not conflict thing. You obviously could
> > extend your MU driver to support SCU as well. If it's proved to better
> > than current way, we definitely could switch to it later. It's not
> > clean up old code, it's just simply switch to a new way.
> 
> Provided you don't use a mailbox driver then we'll have to prevent a mailbox
> driver from binding to the device that is used for the SCU. And yes, that's
> what I consider having to cleanup after you.
> 

That's really a neglectable thing which could be simply by passed for SCU MU nodes.

Anyway, I guess current discussing way based on assumptions is not efficient.
To avoid stopping at this point, I'm going to send the mailbox driver we've
tested internally. Then we can discuss against it instead of talking with the air.
Hope that will help us about this blocking issue.

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%7C465
> 6c47737ad474699a708d5db5c8bc3%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C636656112894065518&sdata=dLVJIZKy8YPSFSNfJX4wpMAiu2GD
> Wbonqo37U7PKGH4%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 V3 3/5] dt-bindings: arm: fsl: add mu binding doc

[Dong Aisheng]

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

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v2->v3:
 * change to mailbox binding
   Currently mbox-cells has to be 0 for SCU MU.
   Generic MU Mailbox support binding could be extended later.
v1->v2:
 * typo fixes
 * remove status property
 * remove imx6&7 compatible string which may be added later for
   the generic mailbox binding
---
 .../devicetree/bindings/arm/freescale/fsl,mu.txt   | 34 ++++++++++++++++++++++
 1 file changed, 34 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..90e4905
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
@@ -0,0 +1,34 @@
+NXP i.MX Messaging Unit (MU)
+--------------------------------------------------------------------
+
+The Messaging Unit module enables two processors within the SoC to
+communicate and coordinate by passing messages (e.g. data, status
+and control) through the MU interface. The MU also provides the ability
+for one processor to signal the other processor using interrupts.
+
+Because the MU manages the messaging between processors, the MU uses
+different clocks (from each side of the different peripheral buses).
+Therefore, the MU must synchronize the accesses from one side to the
+other. The MU accomplishes synchronization using two sets of matching
+registers (Processor A-facing, Processor B-facing).
+
+Messaging Unit Device Node:
+=============================
+
+Required properties:
+-------------------
+- compatible :	should be "fsl,<chip>-mu", the supported chips include
+		imx8qxp, imx8qm.
+- reg :		Should contain the registers location and length
+- interrupts :	Interrupt number. The interrupt specifier format depends
+		on the interrupt controller parent.
+- #mbox-cells:  Must be 0. Number of cells in a mailbox
+
+Examples:
+--------
+lsio_mu0: mailbox@5d1b0000 {
+	compatible = "fsl,imx8qxp-mu";
+	reg = <0x0 0x5d1b0000 0x0 0x10000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	#mbox-cells = <0>;
+};
-- 
2.7.4

[PATCH V3 3/5] dt-bindings: arm: fsl: add mu binding doc

[Dong Aisheng]

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

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree at vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v2->v3:
 * change to mailbox binding
   Currently mbox-cells has to be 0 for SCU MU.
   Generic MU Mailbox support binding could be extended later.
v1->v2:
 * typo fixes
 * remove status property
 * remove imx6&7 compatible string which may be added later for
   the generic mailbox binding
---
 .../devicetree/bindings/arm/freescale/fsl,mu.txt   | 34 ++++++++++++++++++++++
 1 file changed, 34 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..90e4905
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,mu.txt
@@ -0,0 +1,34 @@
+NXP i.MX Messaging Unit (MU)
+--------------------------------------------------------------------
+
+The Messaging Unit module enables two processors within the SoC to
+communicate and coordinate by passing messages (e.g. data, status
+and control) through the MU interface. The MU also provides the ability
+for one processor to signal the other processor using interrupts.
+
+Because the MU manages the messaging between processors, the MU uses
+different clocks (from each side of the different peripheral buses).
+Therefore, the MU must synchronize the accesses from one side to the
+other. The MU accomplishes synchronization using two sets of matching
+registers (Processor A-facing, Processor B-facing).
+
+Messaging Unit Device Node:
+=============================
+
+Required properties:
+-------------------
+- compatible :	should be "fsl,<chip>-mu", the supported chips include
+		imx8qxp, imx8qm.
+- reg :		Should contain the registers location and length
+- interrupts :	Interrupt number. The interrupt specifier format depends
+		on the interrupt controller parent.
+- #mbox-cells:  Must be 0. Number of cells in a mailbox
+
+Examples:
+--------
+lsio_mu0: mailbox at 5d1b0000 {
+	compatible = "fsl,imx8qxp-mu";
+	reg = <0x0 0x5d1b0000 0x0 0x10000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	#mbox-cells = <0>;
+};
-- 
2.7.4

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

[Dong Aisheng]

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

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v2->v3:
 * update a bit to mailbox binding
v1->v2:
 * remove status
 * changed to mu1
---
 .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 39 ++++++++++++++++++++++
 1 file changed, 39 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..b51f9d1
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -0,0 +1,39 @@
+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_mu1: mailbox@5d1c0000 {
+	compatible = "fsl,imx8qxp-mu";
+	reg = <0x0 0x5d1c0000 0x0 0x10000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	#mbox-cells = <0>;
+};
+
+scu {
+	compatible = "fsl,imx8qxp-scu";
+	fsl,mu = <&lsio_mu1>;
+};
-- 
2.7.4

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

[Dong Aisheng]

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

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree at vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v2->v3:
 * update a bit to mailbox binding
v1->v2:
 * remove status
 * changed to mu1
---
 .../devicetree/bindings/arm/freescale/fsl,scu.txt  | 39 ++++++++++++++++++++++
 1 file changed, 39 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..b51f9d1
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -0,0 +1,39 @@
+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_mu1: mailbox at 5d1c0000 {
+	compatible = "fsl,imx8qxp-mu";
+	reg = <0x0 0x5d1c0000 0x0 0x10000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	#mbox-cells = <0>;
+};
+
+scu {
+	compatible = "fsl,imx8qxp-scu";
+	fsl,mu = <&lsio_mu1>;
+};
-- 
2.7.4

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

[Sascha Hauer]

On Fri, Jun 22, 2018 at 10:12:00PM +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).
> 
> This patch adds the SC firmware service APIs used by the system.
> It mainly consists of two parts:
> 1) Implementation of the IPC functions using MUs (client side).
> 2) SCU firmware services APIs implemented based on RPC calls
> 
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> v2->v3:
>  * add the ipc read/write error check
> v1->v2:
>  * change the type of sc_ipc_t from uint32_t to unsigned long.
>    This can make it be capable of storing 64 bit pointer in ARMv8 system.
>  * Update to use the new MU library implemenation (handle iomem privately)
>  * remove unused delcarations e.g. sc_rpc_dispatch and sc_rpc_xlate.
>  * remove unneeded pad ctl functions
> ---
> +static sc_ipc_t scu_ipc_handle;
> +static DEFINE_SPINLOCK(ipc_list_lock);
> +static struct list_head ipc_list = LIST_HEAD_INIT(ipc_list);
> +
> +/*
> + * This function reads a message from an IPC channel.
> + *
> + * @param[in]     sc_ipc      IPC channel handle
> + * @param[out]    data        pointer to message buffer to read
> + *
> + * This function will block if no message is available to be read.
> + */
> +static void sc_ipc_read(struct sc_ipc *sc_ipc, void *data)
> +{
> +	sc_rpc_msg_t *msg = (sc_rpc_msg_t *)data;
> +	uint8_t count = 0;
> +	int ret;
> +
> +	/* Read first word */
> +	ret = mu_receive_msg(sc_ipc->mu, 0, (uint32_t *)msg);
> +	if (ret)
> +		pr_warn("%s: RPC(MU %pOF) read failed\n", __func__, mu_node(sc_ipc->mu));

So mu_receive_msg() can now timeout. This of course does not help when
all you do is to printk the error. You should forward it.

> +
> +	count++;
> +
> +	/* Check size */
> +	if (msg->size > SC_RPC_MAX_MSG) {
> +		*((uint32_t *)msg) = 0;
> +		return;
> +	}

That's an error. It shouldn't be silently ignored. By that I do not mean
that you should print a message. Go and error out, let your callers know
there an error.

> +
> +	/* Read remaining words */
> +	while (count < msg->size) {
> +		ret = mu_receive_msg(sc_ipc->mu, count % MU_RR_COUNT,
> +				     &(msg->DATA.u32[count - 1]));
> +		if (ret)
> +			pr_warn("%s: RPC(MU %pOF) read failed\n", __func__,
> +				mu_node(sc_ipc->mu));
> +		count++;
> +	}
> +}
> +
> +/*
> + * This function writes a message to an IPC channel.
> + *
> + * @param[in]     sc_ipc      IPC channel handle
> + * @param[in]     data        pointer to message buffer to write
> + *
> + * This function will block if the outgoing buffer is full.
> + */
> +static void sc_ipc_write(struct sc_ipc *sc_ipc, void *data)
> +{
> +	sc_rpc_msg_t *msg = (sc_rpc_msg_t *)data;
> +	uint8_t count = 0;
> +	int ret;
> +
> +	/* Check size */
> +	if (msg->size > SC_RPC_MAX_MSG)
> +		return;
> +
> +	/* Write first word */
> +	ret = mu_send_msg(sc_ipc->mu, 0, *((uint32_t *)msg));
> +	if (ret)
> +		pr_warn("%s: RPC(MU %pOF) write failed\n", __func__, mu_node(sc_ipc->mu));
> +
> +	count++;
> +
> +	/* Write remaining words */
> +	while (count < msg->size) {
> +		ret = mu_send_msg(sc_ipc->mu, count % MU_TR_COUNT,
> +				  msg->DATA.u32[count - 1]);
> +		if (ret)
> +			pr_warn("%s: RPC(MU %pOF) write failed\n", __func__,
> +				mu_node(sc_ipc->mu));
> +		count++;
> +	}
> +}
> +
> +/*
> + * RPC command/response
> + */
> +void sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp)
> +{
> +	struct sc_ipc *sc_ipc = (struct sc_ipc *)ipc;

This typedef is just unnecessary. In the include file for the consumer
just do a:

struct sc_ipc *sc_ipc;
void sc_call_rpc(struct sc_ipc *sc_ipc, sc_rpc_msg_t *msg, bool no_resp);

And here in the implementation you define struct sc_ipc.

That's it. No need to cast back and forth any types.

> +
> +	WARN_ON(in_interrupt());
> +
> +	pr_debug("%s: RPC(MU %pOF) SVC %u FUNC %u\n", __func__,
> +		 mu_node(sc_ipc->mu), RPC_SVC(msg), RPC_FUNC(msg));
> +
> +	if (WARN_ON(!sc_ipc || !msg))
> +		return;
> +
> +	mutex_lock(&sc_ipc->lock);
> +
> +	sc_ipc_write(sc_ipc, msg);
> +	if (!no_resp)
> +		sc_ipc_read(sc_ipc, msg);
> +
> +	mutex_unlock(&sc_ipc->lock);
> +
> +	pr_debug("%s: RPC(MU %pOF) done\n", __func__, mu_node(sc_ipc->mu));
> +}
> +
> +/*
> + * Get the default handle used by SCU
> + */
> +int sc_ipc_get_handle(sc_ipc_t *ipc)
> +{
> +	if (!scu_ipc_handle)
> +		return -EPROBE_DEFER;
> +
> +	*ipc = scu_ipc_handle;
> +	return SC_ERR_NONE;
> +}

What is wrong when one function returns -EPROBE_DEFER *and* custom error
codes?

I had a look at the pinctrl consumer of this function and it does:

int imx_pinctrl_sc_ipc_init(struct platform_device *pdev)
{
	sc_err_t sci_err;

	sci_err = sc_ipc_get_handle(&pinctrl_ipc_handle);
	if (sci_err != SC_ERR_NONE) {
		dev_err(&pdev->dev, "can't get sc ipc handle\n");
		return -ENODEV;
	}

	return 0;
}

So you give back -EPROBE_DEFER and then ignore the value.

> +EXPORT_SYMBOL(sc_ipc_get_handle);
> +
> +static struct sc_ipc *sc_ipc_find(sc_ipc_id_t id)
> +{
> +	struct device_node *mu_np = (struct device_node *)id;
> +	struct sc_ipc *sc_ipc;
> +
> +	spin_lock(&ipc_list_lock);
> +	list_for_each_entry(sc_ipc, &ipc_list, node) {
> +		if (mu_node(sc_ipc->mu) == mu_np) {
> +			spin_unlock(&ipc_list_lock);
> +			return sc_ipc;
> +		}
> +	}
> +	spin_unlock(&ipc_list_lock);
> +
> +	return NULL;
> +}
> +
> +/*
> + * Open an IPC channel
> + */
> +sc_err_t sc_ipc_open(sc_ipc_t *ipc, sc_ipc_id_t id)
> +{
> +	struct device_node *mu_np = (struct device_node *)id;

Why is the id argument of type sc_ipc_id_t? It is casted from a struct
device_node * by the caller and casted back to a struct device_node *
here. Doesn't this suggest the argument should be a struct device_node * ?

> +	struct sc_ipc *sc_ipc;
> +
> +	sc_ipc = sc_ipc_find(id);
> +	if (sc_ipc) {
> +		sc_ipc->users++;
> +		*ipc = (sc_ipc_t)sc_ipc;
> +		return SC_ERR_NONE;
> +	}

Which sense does this pseudo resource management do? sc_ipc_open() is
called exactly once during an initcall, and the resulting sc_ipc_t * is
put into a global variable which is then passed to all consumers calling
sc_ipc_get_handle().

What's interesting about this is that in sc_ipc_get_handle() you pass
the handle to your consumers and this place really could use resource
manangement. So resource management is implemented and shortcircuited
afterwards :(

This is racy btw since sc_ipc could be freed below after the call to
sc_ipc_find(). You would then act on an already freed sc_ipc.

> +
> +	sc_ipc = kzalloc(sizeof(*sc_ipc), GFP_KERNEL);
> +	if (!sc_ipc)
> +		return SC_ERR_UNAVAILABLE;
> +
> +	sc_ipc->mu = mu_init(mu_np);
> +	if (IS_ERR_OR_NULL(sc_ipc->mu)) {
> +		kfree(sc_ipc);
> +		return SC_ERR_UNAVAILABLE;
> +	}
> +
> +	mutex_init(&sc_ipc->lock);
> +
> +	spin_lock(&ipc_list_lock);
> +	list_add_tail(&sc_ipc->node, &ipc_list);
> +	spin_unlock(&ipc_list_lock);
> +
> +	sc_ipc->users++;
> +	*ipc = (sc_ipc_t)sc_ipc;
> +
> +	return SC_ERR_NONE;
> +}
> +EXPORT_SYMBOL(sc_ipc_open);
> +
> +/*
> + * Close an IPC channel
> + */
> +void sc_ipc_close(sc_ipc_t ipc)
> +{
> +	struct sc_ipc *sc_ipc = (struct sc_ipc *)ipc;
> +
> +	WARN_ON(!sc_ipc);
> +
> +	if (--sc_ipc->users)
> +		return;
> +
> +	mu_exit(sc_ipc->mu);
> +
> +	spin_lock(&ipc_list_lock);
> +	list_del(&sc_ipc->node);
> +	spin_unlock(&ipc_list_lock);
> +
> +	kfree(sc_ipc);
> +}
> +EXPORT_SYMBOL(sc_ipc_close);
> +
> +/* Initialization of the MU code. */
> +int __init imx8_scu_init(void)

static

> +{
> +	struct device_node *np, *mu_np;
> +	sc_err_t sciErr;
> +
> +	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;
> +
> +	sciErr = sc_ipc_open(&scu_ipc_handle, (sc_ipc_id_t)mu_np);
> +	if (sciErr != SC_ERR_NONE) {
> +		pr_err("Cannot open MU (%pOF) to SCU\n", mu_np);
> +		return -ENODEV;
> +	};
> +
> +	pr_info("NXP i.MX SCU Initialized with MU: %pOF %s\n",
> +		mu_np, mu_np->name);
> +
> +	of_node_put(mu_np);
> +
> +	return 0;
> +}
> +early_initcall(imx8_scu_init);
> diff --git a/drivers/soc/imx/sc/main/rpc.h b/drivers/soc/imx/sc/main/rpc.h
> new file mode 100644
> index 0000000..e957eb5
> --- /dev/null
> +++ b/drivers/soc/imx/sc/main/rpc.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> + * Copyright 2017~2018 NXP
> + *
> + * Header file for the RPC implementation.
> + */
> +
> +#ifndef _SC_RPC_H
> +#define _SC_RPC_H
> +
> +/* Includes */
> +
> +#include <soc/imx/sc/types.h>
> +#include <soc/imx/sc/ipc.h>
> +
> +/* Defines */
> +
> +#define SC_RPC_VERSION		1
> +
> +#define SC_RPC_MAX_MSG		8
> +
> +#define SC_RPC_MAX_COUNT        5
> +
> +#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)])

I thought we agreed that you get rid of these.


> +
> +/* Types */
> +
> +typedef enum sc_rpc_svc_e {
> +	SC_RPC_SVC_UNKNOWN = 0,
> +	SC_RPC_SVC_RETURN = 1,
> +	SC_RPC_SVC_PM = 2,
> +	SC_RPC_SVC_RM = 3,
> +	SC_RPC_SVC_TIMER = 5,
> +	SC_RPC_SVC_PAD = 6,
> +	SC_RPC_SVC_MISC = 7,
> +	SC_RPC_SVC_IRQ = 8,
> +	SC_RPC_SVC_ABORT = 9
> +} sc_rpc_svc_t;
> +
> +typedef struct sc_rpc_msg_s {
> +	uint8_t version;
> +	uint8_t size;
> +	uint8_t svc;
> +	uint8_t func;
> +	union {
> +		int32_t i32[(SC_RPC_MAX_MSG - 1)];
> +		int16_t i16[(SC_RPC_MAX_MSG - 1) * 2];
> +		int8_t i8[(SC_RPC_MAX_MSG - 1) * 4];
> +		uint32_t u32[(SC_RPC_MAX_MSG - 1)];
> +		uint16_t u16[(SC_RPC_MAX_MSG - 1) * 2];
> +		uint8_t u8[(SC_RPC_MAX_MSG - 1) * 4];
> +	} DATA;
> +} sc_rpc_msg_t;
> +
> +/* Functions */
> +
> +/*
> + * This is an internal function to send an RPC message over an IPC
> + * channel. It is called by client-side SCFW API function shims.
> + *
> + * @param[in]     ipc         IPC handle
> + * @param[in,out] msg         handle to a message
> + * @param[in]     no_resp     response flag
> + *
> + * If no_resp is false then this function waits for a response
> + * and returns the result in msg.
> + */
> +void sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp);

Positive logic is usually easier to read. Instead of a 'no_resp' flag you should
have a 'resp' flag.

> +
> +#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..3a6cccd
> --- /dev/null
> +++ b/drivers/soc/imx/sc/svc/irq/rpc.h
> @@ -0,0 +1,23 @@
> +/* 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;
> +
> +#endif /* _SC_IRQ_RPC_H */
> diff --git a/drivers/soc/imx/sc/svc/irq/rpc_clnt.c b/drivers/soc/imx/sc/svc/irq/rpc_clnt.c
> new file mode 100644
> index 0000000..37a9ab4
> --- /dev/null
> +++ b/drivers/soc/imx/sc/svc/irq/rpc_clnt.c
> @@ -0,0 +1,58 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> + * Copyright 2017~2018 NXP
> + *
> + * File containing client-side RPC functions for the IRQ service. These
> + * function are ported to clients that communicate to the SC.
> + *
> + */
> +
> +#include <soc/imx/sc/types.h>
> +#include <soc/imx/sc/svc/rm/api.h>
> +#include <soc/imx/sc/svc/irq/api.h>
> +#include "../../main/rpc.h"
> +#include "rpc.h"
> +
> +sc_err_t sc_irq_enable(sc_ipc_t ipc, sc_rsrc_t resource,
> +		       sc_irq_group_t group, uint32_t mask, bool enable)
> +{
> +	sc_rpc_msg_t msg;
> +	uint8_t result;
> +
> +	RPC_VER(&msg) = SC_RPC_VERSION;
> +	RPC_SVC(&msg) = (uint8_t)SC_RPC_SVC_IRQ;
> +	RPC_FUNC(&msg) = (uint8_t)IRQ_FUNC_ENABLE;
> +	RPC_U32(&msg, 0) = mask;
> +	RPC_U16(&msg, 4) = resource;
> +	RPC_U8(&msg, 6) = group;
> +	RPC_U8(&msg, 7) = (uint8_t)enable;
> +	RPC_SIZE(&msg) = 3;
> +
> +	sc_call_rpc(ipc, &msg, false);

This call can fail...

> +
> +	result = RPC_R8(&msg);

.. in which case RPC_R8(&msg) is still initialized with SC_RPC_VERSION.

> +	return (sc_err_t)result;
> +}
> +

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

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Monday, June 25, 2018 6:58 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 V3 5/5] soc: imx: add SC firmware IPC and APIs
> 
> On Fri, Jun 22, 2018 at 10:12:00PM +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).
> >
> > This patch adds the SC firmware service APIs used by the system.
> > It mainly consists of two parts:
> > 1) Implementation of the IPC functions using MUs (client side).
> > 2) SCU firmware services APIs implemented based on RPC calls
> >
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> > v2->v3:
> >  * add the ipc read/write error check
> > v1->v2:
> >  * change the type of sc_ipc_t from uint32_t to unsigned long.
> >    This can make it be capable of storing 64 bit pointer in ARMv8 system.
> >  * Update to use the new MU library implemenation (handle iomem
> > privately)
> >  * remove unused delcarations e.g. sc_rpc_dispatch and sc_rpc_xlate.
> >  * remove unneeded pad ctl functions
> > ---
> > +static sc_ipc_t scu_ipc_handle;
> > +static DEFINE_SPINLOCK(ipc_list_lock); static struct list_head
> > +ipc_list = LIST_HEAD_INIT(ipc_list);
> > +
> > +/*
> > + * This function reads a message from an IPC channel.
> > + *
> > + * @param[in]     sc_ipc      IPC channel handle
> > + * @param[out]    data        pointer to message buffer to read
> > + *
> > + * This function will block if no message is available to be read.
> > + */
> > +static void sc_ipc_read(struct sc_ipc *sc_ipc, void *data) {
> > +	sc_rpc_msg_t *msg = (sc_rpc_msg_t *)data;
> > +	uint8_t count = 0;
> > +	int ret;
> > +
> > +	/* Read first word */
> > +	ret = mu_receive_msg(sc_ipc->mu, 0, (uint32_t *)msg);
> > +	if (ret)
> > +		pr_warn("%s: RPC(MU %pOF) read failed\n", __func__,
> > +mu_node(sc_ipc->mu));
> 
> So mu_receive_msg() can now timeout. This of course does not help when
> all you do is to printk the error. You should forward it.
> 

The reason is this timeout case is very low possibility (we've never seen it up till now),
so i just WARN it here to let user know something wrong happened in case it really happens.
Just like many other code in kernel do like a WARN() without return error.

You will see in current SC service API designs, it replies on SC firmware
to return error code, timeout case is not considered.
e.g.
sc_err_t sc_irq_enable(sc_ipc_t ipc, sc_rsrc_t resource,
                       sc_irq_group_t group, uint32_t mask, bool enable)
{
        sc_rpc_msg_t msg;
        uint8_t result;

        RPC_VER(&msg) = SC_RPC_VERSION;
        RPC_SVC(&msg) = (uint8_t)SC_RPC_SVC_IRQ;
....................

        sc_call_rpc(ipc, &msg, false);

        result = RPC_R8(&msg);
        return (sc_err_t)result;
}

If we want an extra check for timeout case, we need change the whole SC services
APIs in all C code files. If you think it's still necessary to do that instead of a warn,
please let me know. I can do it.

> > +
> > +	count++;
> > +
> > +	/* Check size */
> > +	if (msg->size > SC_RPC_MAX_MSG) {
> > +		*((uint32_t *)msg) = 0;
> > +		return;
> > +	}
> 
> That's an error. It shouldn't be silently ignored. By that I do not mean that
> you should print a message. Go and error out, let your callers know there an
> error.

The msg->size are auto generated by SCU firmware script.
But Okay to me if we decide to return an error for 

> 
> > +
> > +	/* Read remaining words */
> > +	while (count < msg->size) {
> > +		ret = mu_receive_msg(sc_ipc->mu, count % MU_RR_COUNT,
> > +				     &(msg->DATA.u32[count - 1]));
> > +		if (ret)
> > +			pr_warn("%s: RPC(MU %pOF) read failed\n",
> __func__,
> > +				mu_node(sc_ipc->mu));
> > +		count++;
> > +	}
> > +}
> > +
> > +/*
> > + * This function writes a message to an IPC channel.
> > + *
> > + * @param[in]     sc_ipc      IPC channel handle
> > + * @param[in]     data        pointer to message buffer to write
> > + *
> > + * This function will block if the outgoing buffer is full.
> > + */
> > +static void sc_ipc_write(struct sc_ipc *sc_ipc, void *data) {
> > +	sc_rpc_msg_t *msg = (sc_rpc_msg_t *)data;
> > +	uint8_t count = 0;
> > +	int ret;
> > +
> > +	/* Check size */
> > +	if (msg->size > SC_RPC_MAX_MSG)
> > +		return;
> > +
> > +	/* Write first word */
> > +	ret = mu_send_msg(sc_ipc->mu, 0, *((uint32_t *)msg));
> > +	if (ret)
> > +		pr_warn("%s: RPC(MU %pOF) write failed\n", __func__,
> > +mu_node(sc_ipc->mu));
> > +
> > +	count++;
> > +
> > +	/* Write remaining words */
> > +	while (count < msg->size) {
> > +		ret = mu_send_msg(sc_ipc->mu, count % MU_TR_COUNT,
> > +				  msg->DATA.u32[count - 1]);
> > +		if (ret)
> > +			pr_warn("%s: RPC(MU %pOF) write failed\n",
> __func__,
> > +				mu_node(sc_ipc->mu));
> > +		count++;
> > +	}
> > +}
> > +
> > +/*
> > + * RPC command/response
> > + */
> > +void sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp) {
> > +	struct sc_ipc *sc_ipc = (struct sc_ipc *)ipc;
> 
> This typedef is just unnecessary. In the include file for the consumer just do a:
> 
> struct sc_ipc *sc_ipc;
> void sc_call_rpc(struct sc_ipc *sc_ipc, sc_rpc_msg_t *msg, bool no_resp);
> 
> And here in the implementation you define struct sc_ipc.
> 
> That's it. No need to cast back and forth any types.
> 
> > +
> > +	WARN_ON(in_interrupt());
> > +
> > +	pr_debug("%s: RPC(MU %pOF) SVC %u FUNC %u\n", __func__,
> > +		 mu_node(sc_ipc->mu), RPC_SVC(msg), RPC_FUNC(msg));
> > +
> > +	if (WARN_ON(!sc_ipc || !msg))
> > +		return;
> > +
> > +	mutex_lock(&sc_ipc->lock);
> > +
> > +	sc_ipc_write(sc_ipc, msg);
> > +	if (!no_resp)
> > +		sc_ipc_read(sc_ipc, msg);
> > +
> > +	mutex_unlock(&sc_ipc->lock);
> > +
> > +	pr_debug("%s: RPC(MU %pOF) done\n", __func__,
> mu_node(sc_ipc->mu));
> > +}
> > +
> > +/*
> > + * Get the default handle used by SCU  */ int
> > +sc_ipc_get_handle(sc_ipc_t *ipc) {
> > +	if (!scu_ipc_handle)
> > +		return -EPROBE_DEFER;
> > +
> > +	*ipc = scu_ipc_handle;
> > +	return SC_ERR_NONE;
> > +}
> 
> What is wrong when one function returns -EPROBE_DEFER *and* custom
> error codes?
> 
> I had a look at the pinctrl consumer of this function and it does:
> 
> int imx_pinctrl_sc_ipc_init(struct platform_device *pdev) {
> 	sc_err_t sci_err;
> 
> 	sci_err = sc_ipc_get_handle(&pinctrl_ipc_handle);
> 	if (sci_err != SC_ERR_NONE) {
> 		dev_err(&pdev->dev, "can't get sc ipc handle\n");
> 		return -ENODEV;
> 	}
> 
> 	return 0;
> }
> 
> So you give back -EPROBE_DEFER and then ignore the value.

Good catch.
I probably should return sc_err_t for sc_ipc_get_handle as it's a
SC function. And give a warn and return SC_ERR_UNAVAILABLE here.
That means we drop the EPROBE_DEFER support as we already
Guarantee the SC IPC is initialized before all clients devices.

> 
> > +EXPORT_SYMBOL(sc_ipc_get_handle);
> > +
> > +static struct sc_ipc *sc_ipc_find(sc_ipc_id_t id) {
> > +	struct device_node *mu_np = (struct device_node *)id;
> > +	struct sc_ipc *sc_ipc;
> > +
> > +	spin_lock(&ipc_list_lock);
> > +	list_for_each_entry(sc_ipc, &ipc_list, node) {
> > +		if (mu_node(sc_ipc->mu) == mu_np) {
> > +			spin_unlock(&ipc_list_lock);
> > +			return sc_ipc;
> > +		}
> > +	}
> > +	spin_unlock(&ipc_list_lock);
> > +
> > +	return NULL;
> > +}
> > +
> > +/*
> > + * Open an IPC channel
> > + */
> > +sc_err_t sc_ipc_open(sc_ipc_t *ipc, sc_ipc_id_t id) {
> > +	struct device_node *mu_np = (struct device_node *)id;
> 
> Why is the id argument of type sc_ipc_id_t? It is casted from a struct
> device_node * by the caller and casted back to a struct device_node * here.
> Doesn't this suggest the argument should be a struct device_node * ?
> 

That's more to align with original SC firmware API design to hide
the real data type of sc_ipc_t and sc_ipc_id_t to users which can
easy the future update in case a type change.

But if you dislike it and thought the data should be determinted
already for Linux and won't be changed anymore, I can change them
into an explicit struct pointer.

> > +	struct sc_ipc *sc_ipc;
> > +
> > +	sc_ipc = sc_ipc_find(id);
> > +	if (sc_ipc) {
> > +		sc_ipc->users++;
> > +		*ipc = (sc_ipc_t)sc_ipc;
> > +		return SC_ERR_NONE;
> > +	}
> 
> Which sense does this pseudo resource management do? sc_ipc_open() is
> called exactly once during an initcall, and the resulting sc_ipc_t * is put into a
> global variable which is then passed to all consumers calling
> sc_ipc_get_handle().
> 

This is designed to support multi channels simultaneously.
For other users, they do similar thing like default SC IPC to open another IPC channel:
e.g.
        mu_np = of_parse_phandle(np, "fsl,mu", 0);
       ....
        sciErr = sc_ipc_open(&client_ipc_handle, (sc_ipc_id_t)mu_np);
Then operate on client_ipc_handle.

They can also free it after using by calling sc_ipc_close().

> What's interesting about this is that in sc_ipc_get_handle() you pass the
> handle to your consumers and this place really could use resource
> manangement. So resource management is implemented and shortcircuited
> afterwards :(

The API sc_ipc_open design does not have device pointer. So we don't
use the resource management.

> 
> This is racy btw since sc_ipc could be freed below after the call to
> sc_ipc_find(). You would then act on an already freed sc_ipc.
>

Good catch. That would be improved.
 
> > +
> > +	sc_ipc = kzalloc(sizeof(*sc_ipc), GFP_KERNEL);
> > +	if (!sc_ipc)
> > +		return SC_ERR_UNAVAILABLE;
> > +
> > +	sc_ipc->mu = mu_init(mu_np);
> > +	if (IS_ERR_OR_NULL(sc_ipc->mu)) {
> > +		kfree(sc_ipc);
> > +		return SC_ERR_UNAVAILABLE;
> > +	}
> > +
> > +	mutex_init(&sc_ipc->lock);
> > +
> > +	spin_lock(&ipc_list_lock);
> > +	list_add_tail(&sc_ipc->node, &ipc_list);
> > +	spin_unlock(&ipc_list_lock);
> > +
> > +	sc_ipc->users++;
> > +	*ipc = (sc_ipc_t)sc_ipc;
> > +
> > +	return SC_ERR_NONE;
> > +}
> > +EXPORT_SYMBOL(sc_ipc_open);
> > +
> > +/*
> > + * Close an IPC channel
> > + */
> > +void sc_ipc_close(sc_ipc_t ipc)
> > +{
> > +	struct sc_ipc *sc_ipc = (struct sc_ipc *)ipc;
> > +
> > +	WARN_ON(!sc_ipc);
> > +
> > +	if (--sc_ipc->users)
> > +		return;
> > +
> > +	mu_exit(sc_ipc->mu);
> > +
> > +	spin_lock(&ipc_list_lock);
> > +	list_del(&sc_ipc->node);
> > +	spin_unlock(&ipc_list_lock);
> > +
> > +	kfree(sc_ipc);
> > +}
> > +EXPORT_SYMBOL(sc_ipc_close);
> > +
> > +/* Initialization of the MU code. */
> > +int __init imx8_scu_init(void)
> 
> static
> 

Got it.

> > +{
> > +	struct device_node *np, *mu_np;
> > +	sc_err_t sciErr;
> > +
> > +	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;
> > +
> > +	sciErr = sc_ipc_open(&scu_ipc_handle, (sc_ipc_id_t)mu_np);
> > +	if (sciErr != SC_ERR_NONE) {
> > +		pr_err("Cannot open MU (%pOF) to SCU\n", mu_np);
> > +		return -ENODEV;
> > +	};
> > +
> > +	pr_info("NXP i.MX SCU Initialized with MU: %pOF %s\n",
> > +		mu_np, mu_np->name);
> > +
> > +	of_node_put(mu_np);
> > +
> > +	return 0;
> > +}
> > +early_initcall(imx8_scu_init);
> > diff --git a/drivers/soc/imx/sc/main/rpc.h
> > b/drivers/soc/imx/sc/main/rpc.h new file mode 100644 index
> > 0000000..e957eb5
> > --- /dev/null
> > +++ b/drivers/soc/imx/sc/main/rpc.h
> > @@ -0,0 +1,81 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> > + * Copyright 2017~2018 NXP
> > + *
> > + * Header file for the RPC implementation.
> > + */
> > +
> > +#ifndef _SC_RPC_H
> > +#define _SC_RPC_H
> > +
> > +/* Includes */
> > +
> > +#include <soc/imx/sc/types.h>
> > +#include <soc/imx/sc/ipc.h>
> > +
> > +/* Defines */
> > +
> > +#define SC_RPC_VERSION		1
> > +
> > +#define SC_RPC_MAX_MSG		8
> > +
> > +#define SC_RPC_MAX_COUNT        5
> > +
> > +#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)])
> 
> I thought we agreed that you get rid of these.
> 

Your earlier reply is only changing head files if needed.
This requires change all over the code. 
As I said before, that's part of code is auto generated by SCU firmware. 
(Only drivers/soc/imx/sc/main/ipc.c and drivers/soc/imx/sc/main/rpc.h
Is newly implemented for Linux). Changing it will result in maintain troubles
(we then need to change them all the time in the future in case a update for
new features and etc).
So the original purpose is we'd like to preserve it unless it's a big issue.
What's your opinion?

> 
> > +
> > +/* Types */
> > +
> > +typedef enum sc_rpc_svc_e {
> > +	SC_RPC_SVC_UNKNOWN = 0,
> > +	SC_RPC_SVC_RETURN = 1,
> > +	SC_RPC_SVC_PM = 2,
> > +	SC_RPC_SVC_RM = 3,
> > +	SC_RPC_SVC_TIMER = 5,
> > +	SC_RPC_SVC_PAD = 6,
> > +	SC_RPC_SVC_MISC = 7,
> > +	SC_RPC_SVC_IRQ = 8,
> > +	SC_RPC_SVC_ABORT = 9
> > +} sc_rpc_svc_t;
> > +
> > +typedef struct sc_rpc_msg_s {
> > +	uint8_t version;
> > +	uint8_t size;
> > +	uint8_t svc;
> > +	uint8_t func;
> > +	union {
> > +		int32_t i32[(SC_RPC_MAX_MSG - 1)];
> > +		int16_t i16[(SC_RPC_MAX_MSG - 1) * 2];
> > +		int8_t i8[(SC_RPC_MAX_MSG - 1) * 4];
> > +		uint32_t u32[(SC_RPC_MAX_MSG - 1)];
> > +		uint16_t u16[(SC_RPC_MAX_MSG - 1) * 2];
> > +		uint8_t u8[(SC_RPC_MAX_MSG - 1) * 4];
> > +	} DATA;
> > +} sc_rpc_msg_t;
> > +
> > +/* Functions */
> > +
> > +/*
> > + * This is an internal function to send an RPC message over an IPC
> > + * channel. It is called by client-side SCFW API function shims.
> > + *
> > + * @param[in]     ipc         IPC handle
> > + * @param[in,out] msg         handle to a message
> > + * @param[in]     no_resp     response flag
> > + *
> > + * If no_resp is false then this function waits for a response
> > + * and returns the result in msg.
> > + */
> > +void sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp);
> 
> Positive logic is usually easier to read. Instead of a 'no_resp' flag you should
> have a 'resp' flag.

Same explain as above one.

> 
> > +
> > +#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..3a6cccd
> > --- /dev/null
> > +++ b/drivers/soc/imx/sc/svc/irq/rpc.h
> > @@ -0,0 +1,23 @@
> > +/* 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;
> > +
> > +#endif /* _SC_IRQ_RPC_H */
> > diff --git a/drivers/soc/imx/sc/svc/irq/rpc_clnt.c
> > b/drivers/soc/imx/sc/svc/irq/rpc_clnt.c
> > new file mode 100644
> > index 0000000..37a9ab4
> > --- /dev/null
> > +++ b/drivers/soc/imx/sc/svc/irq/rpc_clnt.c
> > @@ -0,0 +1,58 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> > + * Copyright 2017~2018 NXP
> > + *
> > + * File containing client-side RPC functions for the IRQ service.
> > +These
> > + * function are ported to clients that communicate to the SC.
> > + *
> > + */
> > +
> > +#include <soc/imx/sc/types.h>
> > +#include <soc/imx/sc/svc/rm/api.h>
> > +#include <soc/imx/sc/svc/irq/api.h>
> > +#include "../../main/rpc.h"
> > +#include "rpc.h"
> > +
> > +sc_err_t sc_irq_enable(sc_ipc_t ipc, sc_rsrc_t resource,
> > +		       sc_irq_group_t group, uint32_t mask, bool enable) {
> > +	sc_rpc_msg_t msg;
> > +	uint8_t result;
> > +
> > +	RPC_VER(&msg) = SC_RPC_VERSION;
> > +	RPC_SVC(&msg) = (uint8_t)SC_RPC_SVC_IRQ;
> > +	RPC_FUNC(&msg) = (uint8_t)IRQ_FUNC_ENABLE;
> > +	RPC_U32(&msg, 0) = mask;
> > +	RPC_U16(&msg, 4) = resource;
> > +	RPC_U8(&msg, 6) = group;
> > +	RPC_U8(&msg, 7) = (uint8_t)enable;
> > +	RPC_SIZE(&msg) = 3;
> > +
> > +	sc_call_rpc(ipc, &msg, false);
> 
> This call can fail...
> 

See the similar reply above.

Thanks for the kind and professional review.

Regards
Dong Aisheng

> > +
> > +	result = RPC_R8(&msg);
> 
> .. in which case RPC_R8(&msg) is still initialized with SC_RPC_VERSION.
> 
> > +	return (sc_err_t)result;
> > +}
> > +
> 
> 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%7Ca36
> dc1e6a1d8406625be08d5da8a8a2c%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C636655210928677179&sdata=mW2IA5PwSgTwcIkdf2Tx4yvBG8R
> qCcWwfP0KG%2F99MIk%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 V3 5/5] soc: imx: add SC firmware IPC and APIs

[Sascha Hauer]

On Mon, Jun 25, 2018 at 01:04:38PM +0000, A.s. Dong wrote:
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Monday, June 25, 2018 6:58 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 V3 5/5] soc: imx: add SC firmware IPC and APIs
> > 
> > On Fri, Jun 22, 2018 at 10:12:00PM +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).
> > >
> > > This patch adds the SC firmware service APIs used by the system.
> > > It mainly consists of two parts:
> > > 1) Implementation of the IPC functions using MUs (client side).
> > > 2) SCU firmware services APIs implemented based on RPC calls
> > >
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > > v2->v3:
> > >  * add the ipc read/write error check
> > > v1->v2:
> > >  * change the type of sc_ipc_t from uint32_t to unsigned long.
> > >    This can make it be capable of storing 64 bit pointer in ARMv8 system.
> > >  * Update to use the new MU library implemenation (handle iomem
> > > privately)
> > >  * remove unused delcarations e.g. sc_rpc_dispatch and sc_rpc_xlate.
> > >  * remove unneeded pad ctl functions
> > > ---
> > > +static sc_ipc_t scu_ipc_handle;
> > > +static DEFINE_SPINLOCK(ipc_list_lock); static struct list_head
> > > +ipc_list = LIST_HEAD_INIT(ipc_list);
> > > +
> > > +/*
> > > + * This function reads a message from an IPC channel.
> > > + *
> > > + * @param[in]     sc_ipc      IPC channel handle
> > > + * @param[out]    data        pointer to message buffer to read
> > > + *
> > > + * This function will block if no message is available to be read.
> > > + */
> > > +static void sc_ipc_read(struct sc_ipc *sc_ipc, void *data) {
> > > +	sc_rpc_msg_t *msg = (sc_rpc_msg_t *)data;
> > > +	uint8_t count = 0;
> > > +	int ret;
> > > +
> > > +	/* Read first word */
> > > +	ret = mu_receive_msg(sc_ipc->mu, 0, (uint32_t *)msg);
> > > +	if (ret)
> > > +		pr_warn("%s: RPC(MU %pOF) read failed\n", __func__,
> > > +mu_node(sc_ipc->mu));
> > 
> > So mu_receive_msg() can now timeout. This of course does not help when
> > all you do is to printk the error. You should forward it.
> > 
> 
> The reason is this timeout case is very low possibility (we've never seen it up till now),

That's no reason to not handle this case.

> so i just WARN it here to let user know something wrong happened in case it really happens.
> Just like many other code in kernel do like a WARN() without return error.
> 
> You will see in current SC service API designs, it replies on SC firmware
> to return error code, timeout case is not considered.
> e.g.
> sc_err_t sc_irq_enable(sc_ipc_t ipc, sc_rsrc_t resource,
>                        sc_irq_group_t group, uint32_t mask, bool enable)
> {
>         sc_rpc_msg_t msg;
>         uint8_t result;
> 
>         RPC_VER(&msg) = SC_RPC_VERSION;
>         RPC_SVC(&msg) = (uint8_t)SC_RPC_SVC_IRQ;
> ....................
> 
>         sc_call_rpc(ipc, &msg, false);
> 
>         result = RPC_R8(&msg);
>         return (sc_err_t)result;
> }
> 
> If we want an extra check for timeout case, we need change the whole SC services
> APIs in all C code files. If you think it's still necessary to do that instead of a warn,
> please let me know. I can do it.

Speaking of which, if you had documented the message format rather than
a C API then we wouldn't be confronted with a ton of function
declarations not being in a non Linux conform state. I'm inclined to say
that you should throw all the header files away and put the message
format into your consumers. With the example of the clock_set_parent
call the clock driver could look like:

struct sc_set_clock_parent {
	struct sc_msg_common sc;
	uint16_t resource;
	uint8_t clk;
	uint8_t parent;
};

int foo_clk_set_parent()
{
	struct sc_set_clock_parent sc = {
		.sc = {
			.version = SC_RPC_VERSION,
			.size = sizeof(struct sc_set_clock_parent),
			.svc = SC_RPC_SVC_PM,
			.func = PM_FUNC_SET_CLOCK_PARENT,
		},
		.resource = resource,
		.clk = clk,
		.parent = parent,
	};

	sc_call_rpc(sc_ipc_t ipc, &sc, ...);
}

(with struct sc_msg_common defined at some common place)

> > 
> > That's an error. It shouldn't be silently ignored. By that I do not mean that
> > you should print a message. Go and error out, let your callers know there an
> > error.
> 
> The msg->size are auto generated by SCU firmware script.
> But Okay to me if we decide to return an error for

It may be generated by a script, but you probably cannot reject cleanup
patches to the kernel with the argument that the code is autogenerated.

> > > +	spin_lock(&ipc_list_lock);
> > > +	list_for_each_entry(sc_ipc, &ipc_list, node) {
> > > +		if (mu_node(sc_ipc->mu) == mu_np) {
> > > +			spin_unlock(&ipc_list_lock);
> > > +			return sc_ipc;
> > > +		}
> > > +	}
> > > +	spin_unlock(&ipc_list_lock);
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > > +/*
> > > + * Open an IPC channel
> > > + */
> > > +sc_err_t sc_ipc_open(sc_ipc_t *ipc, sc_ipc_id_t id) {
> > > +	struct device_node *mu_np = (struct device_node *)id;
> > 
> > Why is the id argument of type sc_ipc_id_t? It is casted from a struct
> > device_node * by the caller and casted back to a struct device_node * here.
> > Doesn't this suggest the argument should be a struct device_node * ?
> > 
> 
> That's more to align with original SC firmware API design to hide
> the real data type of sc_ipc_t and sc_ipc_id_t to users which can
> easy the future update in case a type change.

This API is completely inside the kernel. We can change both sides at
the same time.

> > > +	if (sc_ipc) {
> > > +		sc_ipc->users++;
> > > +		*ipc = (sc_ipc_t)sc_ipc;
> > > +		return SC_ERR_NONE;
> > > +	}
> > 
> > Which sense does this pseudo resource management do? sc_ipc_open() is
> > called exactly once during an initcall, and the resulting sc_ipc_t * is put into a
> > global variable which is then passed to all consumers calling
> > sc_ipc_get_handle().
> > 
> 
> This is designed to support multi channels simultaneously.
> For other users, they do similar thing like default SC IPC to open another IPC channel:
> e.g.
>         mu_np = of_parse_phandle(np, "fsl,mu", 0);
>        ....
>         sciErr = sc_ipc_open(&client_ipc_handle, (sc_ipc_id_t)mu_np);
> Then operate on client_ipc_handle.

Yes, they can then operate on the very same handle they would also get
if they called sc_ipc_get_handle().

You could say that the device node argument could be a different one,
but then again you would only use one call to sc_ipc_open() and share
the same handle across the users.

That makes no sense, please just throw it away.

> They can also free it after using by calling sc_ipc_close().
> 
> > What's interesting about this is that in sc_ipc_get_handle() you pass the
> > handle to your consumers and this place really could use resource
> > manangement. So resource management is implemented and shortcircuited
> > afterwards :(
> 
> The API sc_ipc_open design does not have device pointer. So we don't
> use the resource management.

If you don't use it then throw it away. Maybe later when you really have
the need for it then you have the arguments at hand why you need it and
then we can see how to implement it. Until then this is only ballast.

> > > +/* Includes */
> > > +
> > > +#include <soc/imx/sc/types.h>
> > > +#include <soc/imx/sc/ipc.h>
> > > +
> > > +/* Defines */
> > > +
> > > +#define SC_RPC_VERSION		1
> > > +
> > > +#define SC_RPC_MAX_MSG		8
> > > +
> > > +#define SC_RPC_MAX_COUNT        5
> > > +
> > > +#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)])
> > 
> > I thought we agreed that you get rid of these.
> > 
> 
> Your earlier reply is only changing head files if needed.
> This requires change all over the code. 
> As I said before, that's part of code is auto generated by SCU firmware. 
> (Only drivers/soc/imx/sc/main/ipc.c and drivers/soc/imx/sc/main/rpc.h
> Is newly implemented for Linux). Changing it will result in maintain troubles
> (we then need to change them all the time in the future in case a update for
> new features and etc).
> So the original purpose is we'd like to preserve it unless it's a big issue.
> What's your opinion?

> > > +/*
> > > + * 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);
> > 
> > Positive logic is usually easier to read. Instead of a 'no_resp' flag you should
> > have a 'resp' flag.
> 
> Same explain as above one.
> 
> > > +	sc_call_rpc(ipc, &msg, false);
> > 
> > This call can fail...
> > 
> 
> See the similar reply above.

My opinion to this is that the "we want to stay in sync with our internal
code" argument is not valid. With that argument you could sell anything
and I think I repeat myself when I say that Linux would look different
when we would merge everything a vendor throws over the fence.

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

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Tuesday, June 26, 2018 3:44 AM
> 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 V3 5/5] soc: imx: add SC firmware IPC and APIs
> 
> On Mon, Jun 25, 2018 at 01:04:38PM +0000, A.s. Dong wrote:
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > Sent: Monday, June 25, 2018 6:58 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 V3 5/5] soc: imx: add SC firmware IPC and APIs
> > >
> > > On Fri, Jun 22, 2018 at 10:12:00PM +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).
> > > >
> > > > This patch adds the SC firmware service APIs used by the system.
> > > > It mainly consists of two parts:
> > > > 1) Implementation of the IPC functions using MUs (client side).
> > > > 2) SCU firmware services APIs implemented based on RPC calls
> > > >
> > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > > ---
> > > > v2->v3:
> > > >  * add the ipc read/write error check
> > > > v1->v2:
> > > >  * change the type of sc_ipc_t from uint32_t to unsigned long.
> > > >    This can make it be capable of storing 64 bit pointer in ARMv8 system.
> > > >  * Update to use the new MU library implemenation (handle iomem
> > > > privately)
> > > >  * remove unused delcarations e.g. sc_rpc_dispatch and sc_rpc_xlate.
> > > >  * remove unneeded pad ctl functions
> > > > ---
> > > > +static sc_ipc_t scu_ipc_handle;
> > > > +static DEFINE_SPINLOCK(ipc_list_lock); static struct list_head
> > > > +ipc_list = LIST_HEAD_INIT(ipc_list);
> > > > +
> > > > +/*
> > > > + * This function reads a message from an IPC channel.
> > > > + *
> > > > + * @param[in]     sc_ipc      IPC channel handle
> > > > + * @param[out]    data        pointer to message buffer to read
> > > > + *
> > > > + * This function will block if no message is available to be read.
> > > > + */
> > > > +static void sc_ipc_read(struct sc_ipc *sc_ipc, void *data) {
> > > > +	sc_rpc_msg_t *msg = (sc_rpc_msg_t *)data;
> > > > +	uint8_t count = 0;
> > > > +	int ret;
> > > > +
> > > > +	/* Read first word */
> > > > +	ret = mu_receive_msg(sc_ipc->mu, 0, (uint32_t *)msg);
> > > > +	if (ret)
> > > > +		pr_warn("%s: RPC(MU %pOF) read failed\n", __func__,
> > > > +mu_node(sc_ipc->mu));
> > >
> > > So mu_receive_msg() can now timeout. This of course does not help
> > > when all you do is to printk the error. You should forward it.
> > >
> >
> > The reason is this timeout case is very low possibility (we've never
> > seen it up till now),
> 
> That's no reason to not handle this case.
> 

Okay, I will handle it.

> > so i just WARN it here to let user know something wrong happened in case
> it really happens.
> > Just like many other code in kernel do like a WARN() without return error.
> >
> > You will see in current SC service API designs, it replies on SC
> > firmware to return error code, timeout case is not considered.
> > e.g.
> > sc_err_t sc_irq_enable(sc_ipc_t ipc, sc_rsrc_t resource,
> >                        sc_irq_group_t group, uint32_t mask, bool
> > enable) {
> >         sc_rpc_msg_t msg;
> >         uint8_t result;
> >
> >         RPC_VER(&msg) = SC_RPC_VERSION;
> >         RPC_SVC(&msg) = (uint8_t)SC_RPC_SVC_IRQ; ....................
> >
> >         sc_call_rpc(ipc, &msg, false);
> >
> >         result = RPC_R8(&msg);
> >         return (sc_err_t)result;
> > }
> >
> > If we want an extra check for timeout case, we need change the whole
> > SC services APIs in all C code files. If you think it's still
> > necessary to do that instead of a warn, please let me know. I can do it.
> 
> Speaking of which, if you had documented the message format rather than a
> C API then we wouldn't be confronted with a ton of function declarations not
> being in a non Linux conform state. I'm inclined to say that you should throw
> all the header files away and put the message format into your consumers.
> With the example of the clock_set_parent call the clock driver could look like:
> 

Yes, the message format has not been documented. They're hide to
consumers. Consumers only call the APIs provided by SCU.

> struct sc_set_clock_parent {
> 	struct sc_msg_common sc;
> 	uint16_t resource;
> 	uint8_t clk;
> 	uint8_t parent;
> };
> 
> int foo_clk_set_parent()
> {
> 	struct sc_set_clock_parent sc = {
> 		.sc = {
> 			.version = SC_RPC_VERSION,
> 			.size = sizeof(struct sc_set_clock_parent),
> 			.svc = SC_RPC_SVC_PM,
> 			.func = PM_FUNC_SET_CLOCK_PARENT,
> 		},
> 		.resource = resource,
> 		.clk = clk,
> 		.parent = parent,
> 	};
> 
> 	sc_call_rpc(sc_ipc_t ipc, &sc, ...);
> }
> 
> (with struct sc_msg_common defined at some common place)
> 
> > >
> > > That's an error. It shouldn't be silently ignored. By that I do not
> > > mean that you should print a message. Go and error out, let your
> > > callers know there an error.
> >
> > The msg->size are auto generated by SCU firmware script.
> > But Okay to me if we decide to return an error for
> 
> It may be generated by a script, but you probably cannot reject cleanup
> patches to the kernel with the argument that the code is autogenerated.
> 
> > > > +	spin_lock(&ipc_list_lock);
> > > > +	list_for_each_entry(sc_ipc, &ipc_list, node) {
> > > > +		if (mu_node(sc_ipc->mu) == mu_np) {
> > > > +			spin_unlock(&ipc_list_lock);
> > > > +			return sc_ipc;
> > > > +		}
> > > > +	}
> > > > +	spin_unlock(&ipc_list_lock);
> > > > +
> > > > +	return NULL;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Open an IPC channel
> > > > + */
> > > > +sc_err_t sc_ipc_open(sc_ipc_t *ipc, sc_ipc_id_t id) {
> > > > +	struct device_node *mu_np = (struct device_node *)id;
> > >
> > > Why is the id argument of type sc_ipc_id_t? It is casted from a
> > > struct device_node * by the caller and casted back to a struct
> device_node * here.
> > > Doesn't this suggest the argument should be a struct device_node * ?
> > >
> >
> > That's more to align with original SC firmware API design to hide the
> > real data type of sc_ipc_t and sc_ipc_id_t to users which can easy the
> > future update in case a type change.
> 
> This API is completely inside the kernel. We can change both sides at the
> same time.
> 

Okay, I can change them.

> > > > +	if (sc_ipc) {
> > > > +		sc_ipc->users++;
> > > > +		*ipc = (sc_ipc_t)sc_ipc;
> > > > +		return SC_ERR_NONE;
> > > > +	}
> > >
> > > Which sense does this pseudo resource management do? sc_ipc_open()
> > > is called exactly once during an initcall, and the resulting
> > > sc_ipc_t * is put into a global variable which is then passed to all
> > > consumers calling sc_ipc_get_handle().
> > >
> >
> > This is designed to support multi channels simultaneously.
> > For other users, they do similar thing like default SC IPC to open another
> IPC channel:
> > e.g.
> >         mu_np = of_parse_phandle(np, "fsl,mu", 0);
> >        ....
> >         sciErr = sc_ipc_open(&client_ipc_handle, (sc_ipc_id_t)mu_np);
> > Then operate on client_ipc_handle.
> 
> Yes, they can then operate on the very same handle they would also get if
> they called sc_ipc_get_handle().
> 
> You could say that the device node argument could be a different one, but
> then again you would only use one call to sc_ipc_open() and share the same
> handle across the users.
> 
> That makes no sense, please just throw it away.
> 

Sorry, I'm not quite understand your point. What did you suggest to throw away?
They're used to support multi channels for different consumers at the same time
to reduce contention time.
e.g.
SCU CLK: MU1 
SCU PD: MU2
...

> > They can also free it after using by calling sc_ipc_close().
> >
> > > What's interesting about this is that in sc_ipc_get_handle() you
> > > pass the handle to your consumers and this place really could use
> > > resource manangement. So resource management is implemented and
> > > shortcircuited afterwards :(
> >
> > The API sc_ipc_open design does not have device pointer. So we don't
> > use the resource management.
> 
> If you don't use it then throw it away. Maybe later when you really have the
> need for it then you have the arguments at hand why you need it and then
> we can see how to implement it. Until then this is only ballast.
> 

Would you be more specific what to throw way?

> > > > +/* Includes */
> > > > +
> > > > +#include <soc/imx/sc/types.h>
> > > > +#include <soc/imx/sc/ipc.h>
> > > > +
> > > > +/* Defines */
> > > > +
> > > > +#define SC_RPC_VERSION		1
> > > > +
> > > > +#define SC_RPC_MAX_MSG		8
> > > > +
> > > > +#define SC_RPC_MAX_COUNT        5
> > > > +
> > > > +#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)])
> > >
> > > I thought we agreed that you get rid of these.
> > >
> >
> > Your earlier reply is only changing head files if needed.
> > This requires change all over the code.
> > As I said before, that's part of code is auto generated by SCU firmware.
> > (Only drivers/soc/imx/sc/main/ipc.c and drivers/soc/imx/sc/main/rpc.h
> > Is newly implemented for Linux). Changing it will result in maintain
> > troubles (we then need to change them all the time in the future in
> > case a update for new features and etc).
> > So the original purpose is we'd like to preserve it unless it's a big issue.
> > What's your opinion?
> 
> > > > +/*
> > > > + * 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);
> > >
> > > Positive logic is usually easier to read. Instead of a 'no_resp'
> > > flag you should have a 'resp' flag.
> >
> > Same explain as above one.
> >
> > > > +	sc_call_rpc(ipc, &msg, false);
> > >
> > > This call can fail...
> > >
> >
> > See the similar reply above.
> 
> My opinion to this is that the "we want to stay in sync with our internal code"
> argument is not valid. With that argument you could sell anything and I think I
> repeat myself when I say that Linux would look different when we would
> merge everything a vendor throws over the fence.
> 

I'm not meaning 'we must stay in sync with our internal code.'
We definitely can change everything which violates Linux rule a lot.
My original meaning is if it's not big issue and is acceptable by Linux,
then we may prefer to not change as the sync helps a lot for the
future maintain.

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%7Ca5f
> 9765f641f48bab6ee08d5dad40bee%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C636655526628046336&sdata=V0Ghz6WPs8vlMfO0LiATb8Iv5Wq
> VXDN4T9GYjTFt7VI%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 V3 5/5] soc: imx: add SC firmware IPC and APIs

[Sascha Hauer]

On Tue, Jun 26, 2018 at 09:54:10AM +0000, A.s. Dong wrote:
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Tuesday, June 26, 2018 3:44 AM
> > 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 V3 5/5] soc: imx: add SC firmware IPC and APIs
> > 
> > On Mon, Jun 25, 2018 at 01:04:38PM +0000, A.s. Dong wrote:
> > > > -----Original Message-----
> > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > Sent: Monday, June 25, 2018 6:58 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 V3 5/5] soc: imx: add SC firmware IPC and APIs
> > > >
> > > > On Fri, Jun 22, 2018 at 10:12:00PM +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).
> > > > >
> > > > > This patch adds the SC firmware service APIs used by the system.
> > > > > It mainly consists of two parts:
> > > > > 1) Implementation of the IPC functions using MUs (client side).
> > > > > 2) SCU firmware services APIs implemented based on RPC calls
> > > > >
> > > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > > > ---
> > > > > v2->v3:
> > > > >  * add the ipc read/write error check
> > > > > v1->v2:
> > > > >  * change the type of sc_ipc_t from uint32_t to unsigned long.
> > > > >    This can make it be capable of storing 64 bit pointer in ARMv8 system.
> > > > >  * Update to use the new MU library implemenation (handle iomem
> > > > > privately)
> > > > >  * remove unused delcarations e.g. sc_rpc_dispatch and sc_rpc_xlate.
> > > > >  * remove unneeded pad ctl functions
> > > > > ---
> > > > > +static sc_ipc_t scu_ipc_handle;
> > > > > +static DEFINE_SPINLOCK(ipc_list_lock); static struct list_head
> > > > > +ipc_list = LIST_HEAD_INIT(ipc_list);
> > > > > +
> > > > > +/*
> > > > > + * This function reads a message from an IPC channel.
> > > > > + *
> > > > > + * @param[in]     sc_ipc      IPC channel handle
> > > > > + * @param[out]    data        pointer to message buffer to read
> > > > > + *
> > > > > + * This function will block if no message is available to be read.
> > > > > + */
> > > > > +static void sc_ipc_read(struct sc_ipc *sc_ipc, void *data) {
> > > > > +	sc_rpc_msg_t *msg = (sc_rpc_msg_t *)data;
> > > > > +	uint8_t count = 0;
> > > > > +	int ret;
> > > > > +
> > > > > +	/* Read first word */
> > > > > +	ret = mu_receive_msg(sc_ipc->mu, 0, (uint32_t *)msg);
> > > > > +	if (ret)
> > > > > +		pr_warn("%s: RPC(MU %pOF) read failed\n", __func__,
> > > > > +mu_node(sc_ipc->mu));
> > > >
> > > > So mu_receive_msg() can now timeout. This of course does not help
> > > > when all you do is to printk the error. You should forward it.
> > > >
> > >
> > > The reason is this timeout case is very low possibility (we've never
> > > seen it up till now),
> > 
> > That's no reason to not handle this case.
> > 
> 
> Okay, I will handle it.
> 
> > > so i just WARN it here to let user know something wrong happened in case
> > it really happens.
> > > Just like many other code in kernel do like a WARN() without return error.
> > >
> > > You will see in current SC service API designs, it replies on SC
> > > firmware to return error code, timeout case is not considered.
> > > e.g.
> > > sc_err_t sc_irq_enable(sc_ipc_t ipc, sc_rsrc_t resource,
> > >                        sc_irq_group_t group, uint32_t mask, bool
> > > enable) {
> > >         sc_rpc_msg_t msg;
> > >         uint8_t result;
> > >
> > >         RPC_VER(&msg) = SC_RPC_VERSION;
> > >         RPC_SVC(&msg) = (uint8_t)SC_RPC_SVC_IRQ; ....................
> > >
> > >         sc_call_rpc(ipc, &msg, false);
> > >
> > >         result = RPC_R8(&msg);
> > >         return (sc_err_t)result;
> > > }
> > >
> > > If we want an extra check for timeout case, we need change the whole
> > > SC services APIs in all C code files. If you think it's still
> > > necessary to do that instead of a warn, please let me know. I can do it.
> > 
> > Speaking of which, if you had documented the message format rather than a
> > C API then we wouldn't be confronted with a ton of function declarations not
> > being in a non Linux conform state. I'm inclined to say that you should throw
> > all the header files away and put the message format into your consumers.
> > With the example of the clock_set_parent call the clock driver could look like:
> > 
> 
> Yes, the message format has not been documented. They're hide to
> consumers. Consumers only call the APIs provided by SCU.
> 
> > struct sc_set_clock_parent {
> > 	struct sc_msg_common sc;
> > 	uint16_t resource;
> > 	uint8_t clk;
> > 	uint8_t parent;
> > };
> > 
> > int foo_clk_set_parent()
> > {
> > 	struct sc_set_clock_parent sc = {
> > 		.sc = {
> > 			.version = SC_RPC_VERSION,
> > 			.size = sizeof(struct sc_set_clock_parent),
> > 			.svc = SC_RPC_SVC_PM,
> > 			.func = PM_FUNC_SET_CLOCK_PARENT,
> > 		},
> > 		.resource = resource,
> > 		.clk = clk,
> > 		.parent = parent,
> > 	};
> > 
> > 	sc_call_rpc(sc_ipc_t ipc, &sc, ...);
> > }
> > 
> > (with struct sc_msg_common defined at some common place)
> > 
> > > >
> > > > That's an error. It shouldn't be silently ignored. By that I do not
> > > > mean that you should print a message. Go and error out, let your
> > > > callers know there an error.
> > >
> > > The msg->size are auto generated by SCU firmware script.
> > > But Okay to me if we decide to return an error for
> > 
> > It may be generated by a script, but you probably cannot reject cleanup
> > patches to the kernel with the argument that the code is autogenerated.
> > 
> > > > > +	spin_lock(&ipc_list_lock);
> > > > > +	list_for_each_entry(sc_ipc, &ipc_list, node) {
> > > > > +		if (mu_node(sc_ipc->mu) == mu_np) {
> > > > > +			spin_unlock(&ipc_list_lock);
> > > > > +			return sc_ipc;
> > > > > +		}
> > > > > +	}
> > > > > +	spin_unlock(&ipc_list_lock);
> > > > > +
> > > > > +	return NULL;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Open an IPC channel
> > > > > + */
> > > > > +sc_err_t sc_ipc_open(sc_ipc_t *ipc, sc_ipc_id_t id) {
> > > > > +	struct device_node *mu_np = (struct device_node *)id;
> > > >
> > > > Why is the id argument of type sc_ipc_id_t? It is casted from a
> > > > struct device_node * by the caller and casted back to a struct
> > device_node * here.
> > > > Doesn't this suggest the argument should be a struct device_node * ?
> > > >
> > >
> > > That's more to align with original SC firmware API design to hide the
> > > real data type of sc_ipc_t and sc_ipc_id_t to users which can easy the
> > > future update in case a type change.
> > 
> > This API is completely inside the kernel. We can change both sides at the
> > same time.
> > 
> 
> Okay, I can change them.
> 
> > > > > +	if (sc_ipc) {
> > > > > +		sc_ipc->users++;
> > > > > +		*ipc = (sc_ipc_t)sc_ipc;
> > > > > +		return SC_ERR_NONE;
> > > > > +	}
> > > >
> > > > Which sense does this pseudo resource management do? sc_ipc_open()
> > > > is called exactly once during an initcall, and the resulting
> > > > sc_ipc_t * is put into a global variable which is then passed to all
> > > > consumers calling sc_ipc_get_handle().
> > > >
> > >
> > > This is designed to support multi channels simultaneously.
> > > For other users, they do similar thing like default SC IPC to open another
> > IPC channel:
> > > e.g.
> > >         mu_np = of_parse_phandle(np, "fsl,mu", 0);
> > >        ....
> > >         sciErr = sc_ipc_open(&client_ipc_handle, (sc_ipc_id_t)mu_np);
> > > Then operate on client_ipc_handle.
> > 
> > Yes, they can then operate on the very same handle they would also get if
> > they called sc_ipc_get_handle().
> > 
> > You could say that the device node argument could be a different one, but
> > then again you would only use one call to sc_ipc_open() and share the same
> > handle across the users.
> > 
> > That makes no sense, please just throw it away.
> > 
> 
> Sorry, I'm not quite understand your point. What did you suggest to throw away?
> They're used to support multi channels for different consumers at the same time
> to reduce contention time.
> e.g.
> SCU CLK: MU1 
> SCU PD: MU2

You already have multiple users on the same MU and still call
sc_ipc_open() only once. With your current code there is no need
to call sc_ipc_open() for the same MU twice, because you share
the very same instance to your users using sc_ipc_get_handle().

If you get more consumers on a different MU, then why should you
suddenly have to call sc_ipc_open() multiple times on the same MU
then?

> ...
> 
> > > They can also free it after using by calling sc_ipc_close().
> > >
> > > > What's interesting about this is that in sc_ipc_get_handle() you
> > > > pass the handle to your consumers and this place really could use
> > > > resource manangement. So resource management is implemented and
> > > > shortcircuited afterwards :(
> > >
> > > The API sc_ipc_open design does not have device pointer. So we don't
> > > use the resource management.
> > 
> > If you don't use it then throw it away. Maybe later when you really have the
> > need for it then you have the arguments at hand why you need it and then
> > we can see how to implement it. Until then this is only ballast.
> > 
> 
> Would you be more specific what to throw way?

sc_err_t sc_ipc_open(sc_ipc_t *ipc, sc_ipc_id_t id)
{
       struct device_node *mu_np = (struct device_node *)id;
       struct sc_ipc *sc_ipc;

       sc_ipc = sc_ipc_find(id);
       if (sc_ipc) {
               sc_ipc->users++;
               *ipc = (sc_ipc_t)sc_ipc;
               return SC_ERR_NONE;
       }

       ^^^^^^^^^^^^^^^^^^^

This. You only need one user per sc_ipc_t() since even multiple
users do not call sc_ipc_open() again but instead use
sc_ipc_get_handle() to get the instance already allocated.

> > > Your earlier reply is only changing head files if needed.
> > > This requires change all over the code.
> > > As I said before, that's part of code is auto generated by SCU firmware.
> > > (Only drivers/soc/imx/sc/main/ipc.c and drivers/soc/imx/sc/main/rpc.h
> > > Is newly implemented for Linux). Changing it will result in maintain
> > > troubles (we then need to change them all the time in the future in
> > > case a update for new features and etc).
> > > So the original purpose is we'd like to preserve it unless it's a big issue.
> > > What's your opinion?
> > 
> > > > > +/*
> > > > > + * 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);
> > > >
> > > > Positive logic is usually easier to read. Instead of a 'no_resp'
> > > > flag you should have a 'resp' flag.
> > >
> > > Same explain as above one.
> > >
> > > > > +	sc_call_rpc(ipc, &msg, false);
> > > >
> > > > This call can fail...
> > > >
> > >
> > > See the similar reply above.
> > 
> > My opinion to this is that the "we want to stay in sync with our internal code"
> > argument is not valid. With that argument you could sell anything and I think I
> > repeat myself when I say that Linux would look different when we would
> > merge everything a vendor throws over the fence.
> > 
> 
> I'm not meaning 'we must stay in sync with our internal code.'
> We definitely can change everything which violates Linux rule a lot.
> My original meaning is if it's not big issue and is acceptable by Linux,
> then we may prefer to not change as the sync helps a lot for the
> future maintain.

Ok, understood.

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

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Tuesday, June 26, 2018 8:14 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 V3 5/5] soc: imx: add SC firmware IPC and APIs
> 
> On Tue, Jun 26, 2018 at 09:54:10AM +0000, A.s. Dong wrote:
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > Sent: Tuesday, June 26, 2018 3:44 AM
> > > 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 V3 5/5] soc: imx: add SC firmware IPC and APIs
> > >
> > > On Mon, Jun 25, 2018 at 01:04:38PM +0000, A.s. Dong wrote:
> > > > > -----Original Message-----
> > > > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > > > Sent: Monday, June 25, 2018 6:58 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 V3 5/5] soc: imx: add SC firmware IPC and
> > > > > APIs
> > > > >
> > > > > On Fri, Jun 22, 2018 at 10:12:00PM +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).
> > > > > >
> > > > > > This patch adds the SC firmware service APIs used by the system.
> > > > > > It mainly consists of two parts:
> > > > > > 1) Implementation of the IPC functions using MUs (client side).
> > > > > > 2) SCU firmware services APIs implemented based on RPC calls
> > > > > >
> > > > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > > > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > > > > ---
> > > > > > v2->v3:
> > > > > >  * add the ipc read/write error check
> > > > > > v1->v2:
> > > > > >  * change the type of sc_ipc_t from uint32_t to unsigned long.
> > > > > >    This can make it be capable of storing 64 bit pointer in ARMv8
> system.
> > > > > >  * Update to use the new MU library implemenation (handle
> > > > > > iomem
> > > > > > privately)
> > > > > >  * remove unused delcarations e.g. sc_rpc_dispatch and
> sc_rpc_xlate.
> > > > > >  * remove unneeded pad ctl functions
> > > > > > ---
> > > > > > +static sc_ipc_t scu_ipc_handle; static
> > > > > > +DEFINE_SPINLOCK(ipc_list_lock); static struct list_head
> > > > > > +ipc_list = LIST_HEAD_INIT(ipc_list);
> > > > > > +
> > > > > > +/*
> > > > > > + * This function reads a message from an IPC channel.
> > > > > > + *
> > > > > > + * @param[in]     sc_ipc      IPC channel handle
> > > > > > + * @param[out]    data        pointer to message buffer to read
> > > > > > + *
> > > > > > + * This function will block if no message is available to be read.
> > > > > > + */
> > > > > > +static void sc_ipc_read(struct sc_ipc *sc_ipc, void *data) {
> > > > > > +	sc_rpc_msg_t *msg = (sc_rpc_msg_t *)data;
> > > > > > +	uint8_t count = 0;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	/* Read first word */
> > > > > > +	ret = mu_receive_msg(sc_ipc->mu, 0, (uint32_t *)msg);
> > > > > > +	if (ret)
> > > > > > +		pr_warn("%s: RPC(MU %pOF) read failed\n",
> __func__,
> > > > > > +mu_node(sc_ipc->mu));
> > > > >
> > > > > So mu_receive_msg() can now timeout. This of course does not
> > > > > help when all you do is to printk the error. You should forward it.
> > > > >
> > > >
> > > > The reason is this timeout case is very low possibility (we've
> > > > never seen it up till now),
> > >
> > > That's no reason to not handle this case.
> > >
> >
> > Okay, I will handle it.
> >
> > > > so i just WARN it here to let user know something wrong happened
> > > > in case
> > > it really happens.
> > > > Just like many other code in kernel do like a WARN() without return
> error.
> > > >
> > > > You will see in current SC service API designs, it replies on SC
> > > > firmware to return error code, timeout case is not considered.
> > > > e.g.
> > > > sc_err_t sc_irq_enable(sc_ipc_t ipc, sc_rsrc_t resource,
> > > >                        sc_irq_group_t group, uint32_t mask, bool
> > > > enable) {
> > > >         sc_rpc_msg_t msg;
> > > >         uint8_t result;
> > > >
> > > >         RPC_VER(&msg) = SC_RPC_VERSION;
> > > >         RPC_SVC(&msg) = (uint8_t)SC_RPC_SVC_IRQ; ....................
> > > >
> > > >         sc_call_rpc(ipc, &msg, false);
> > > >
> > > >         result = RPC_R8(&msg);
> > > >         return (sc_err_t)result;
> > > > }
> > > >
> > > > If we want an extra check for timeout case, we need change the
> > > > whole SC services APIs in all C code files. If you think it's
> > > > still necessary to do that instead of a warn, please let me know. I can do
> it.
> > >
> > > Speaking of which, if you had documented the message format rather
> > > than a C API then we wouldn't be confronted with a ton of function
> > > declarations not being in a non Linux conform state. I'm inclined to
> > > say that you should throw all the header files away and put the message
> format into your consumers.
> > > With the example of the clock_set_parent call the clock driver could look
> like:
> > >
> >
> > Yes, the message format has not been documented. They're hide to
> > consumers. Consumers only call the APIs provided by SCU.
> >
> > > struct sc_set_clock_parent {
> > > 	struct sc_msg_common sc;
> > > 	uint16_t resource;
> > > 	uint8_t clk;
> > > 	uint8_t parent;
> > > };
> > >
> > > int foo_clk_set_parent()
> > > {
> > > 	struct sc_set_clock_parent sc = {
> > > 		.sc = {
> > > 			.version = SC_RPC_VERSION,
> > > 			.size = sizeof(struct sc_set_clock_parent),
> > > 			.svc = SC_RPC_SVC_PM,
> > > 			.func = PM_FUNC_SET_CLOCK_PARENT,
> > > 		},
> > > 		.resource = resource,
> > > 		.clk = clk,
> > > 		.parent = parent,
> > > 	};
> > >
> > > 	sc_call_rpc(sc_ipc_t ipc, &sc, ...); }
> > >
> > > (with struct sc_msg_common defined at some common place)
> > >
> > > > >
> > > > > That's an error. It shouldn't be silently ignored. By that I do
> > > > > not mean that you should print a message. Go and error out, let
> > > > > your callers know there an error.
> > > >
> > > > The msg->size are auto generated by SCU firmware script.
> > > > But Okay to me if we decide to return an error for
> > >
> > > It may be generated by a script, but you probably cannot reject
> > > cleanup patches to the kernel with the argument that the code is
> autogenerated.
> > >
> > > > > > +	spin_lock(&ipc_list_lock);
> > > > > > +	list_for_each_entry(sc_ipc, &ipc_list, node) {
> > > > > > +		if (mu_node(sc_ipc->mu) == mu_np) {
> > > > > > +			spin_unlock(&ipc_list_lock);
> > > > > > +			return sc_ipc;
> > > > > > +		}
> > > > > > +	}
> > > > > > +	spin_unlock(&ipc_list_lock);
> > > > > > +
> > > > > > +	return NULL;
> > > > > > +}
> > > > > > +
> > > > > > +/*
> > > > > > + * Open an IPC channel
> > > > > > + */
> > > > > > +sc_err_t sc_ipc_open(sc_ipc_t *ipc, sc_ipc_id_t id) {
> > > > > > +	struct device_node *mu_np = (struct device_node *)id;
> > > > >
> > > > > Why is the id argument of type sc_ipc_id_t? It is casted from a
> > > > > struct device_node * by the caller and casted back to a struct
> > > device_node * here.
> > > > > Doesn't this suggest the argument should be a struct device_node * ?
> > > > >
> > > >
> > > > That's more to align with original SC firmware API design to hide
> > > > the real data type of sc_ipc_t and sc_ipc_id_t to users which can
> > > > easy the future update in case a type change.
> > >
> > > This API is completely inside the kernel. We can change both sides
> > > at the same time.
> > >
> >
> > Okay, I can change them.
> >
> > > > > > +	if (sc_ipc) {
> > > > > > +		sc_ipc->users++;
> > > > > > +		*ipc = (sc_ipc_t)sc_ipc;
> > > > > > +		return SC_ERR_NONE;
> > > > > > +	}
> > > > >
> > > > > Which sense does this pseudo resource management do?
> > > > > sc_ipc_open() is called exactly once during an initcall, and the
> > > > > resulting sc_ipc_t * is put into a global variable which is then
> > > > > passed to all consumers calling sc_ipc_get_handle().
> > > > >
> > > >
> > > > This is designed to support multi channels simultaneously.
> > > > For other users, they do similar thing like default SC IPC to open
> > > > another
> > > IPC channel:
> > > > e.g.
> > > >         mu_np = of_parse_phandle(np, "fsl,mu", 0);
> > > >        ....
> > > >         sciErr = sc_ipc_open(&client_ipc_handle,
> > > > (sc_ipc_id_t)mu_np); Then operate on client_ipc_handle.
> > >
> > > Yes, they can then operate on the very same handle they would also
> > > get if they called sc_ipc_get_handle().
> > >
> > > You could say that the device node argument could be a different
> > > one, but then again you would only use one call to sc_ipc_open() and
> > > share the same handle across the users.
> > >
> > > That makes no sense, please just throw it away.
> > >
> >
> > Sorry, I'm not quite understand your point. What did you suggest to throw
> away?
> > They're used to support multi channels for different consumers at the
> > same time to reduce contention time.
> > e.g.
> > SCU CLK: MU1
> > SCU PD: MU2
> 
> You already have multiple users on the same MU and still call
> sc_ipc_open() only once. With your current code there is no need to call
> sc_ipc_open() for the same MU twice, because you share the very same
> instance to your users using sc_ipc_get_handle().
> 

Yes. As the default SCU IPC handle won't be freed.

> If you get more consumers on a different MU, then why should you
> suddenly have to call sc_ipc_open() multiple times on the same MU then?
>

Other IPC channel could be freed if not used anymore.
e.g.
CLK: MU2
PD: MU2
 
> > ...
> >
> > > > They can also free it after using by calling sc_ipc_close().
> > > >
> > > > > What's interesting about this is that in sc_ipc_get_handle() you
> > > > > pass the handle to your consumers and this place really could
> > > > > use resource manangement. So resource management is
> implemented
> > > > > and shortcircuited afterwards :(
> > > >
> > > > The API sc_ipc_open design does not have device pointer. So we
> > > > don't use the resource management.
> > >
> > > If you don't use it then throw it away. Maybe later when you really
> > > have the need for it then you have the arguments at hand why you
> > > need it and then we can see how to implement it. Until then this is only
> ballast.
> > >
> >
> > Would you be more specific what to throw way?
> 
> sc_err_t sc_ipc_open(sc_ipc_t *ipc, sc_ipc_id_t id) {
>        struct device_node *mu_np = (struct device_node *)id;
>        struct sc_ipc *sc_ipc;
> 
>        sc_ipc = sc_ipc_find(id);
>        if (sc_ipc) {
>                sc_ipc->users++;
>                *ipc = (sc_ipc_t)sc_ipc;
>                return SC_ERR_NONE;
>        }
> 
>        ^^^^^^^^^^^^^^^^^^^
> 
> This. You only need one user per sc_ipc_t() since even multiple users do not
> call sc_ipc_open() again but instead use
> sc_ipc_get_handle() to get the instance already allocated.
> 

That's used to support other MU channels in case users don't
want to use the default one.

Do you think we need improve it?

Regards
Dong Aisheng

> > > > Your earlier reply is only changing head files if needed.
> > > > This requires change all over the code.
> > > > As I said before, that's part of code is auto generated by SCU firmware.
> > > > (Only drivers/soc/imx/sc/main/ipc.c and
> > > > drivers/soc/imx/sc/main/rpc.h Is newly implemented for Linux).
> > > > Changing it will result in maintain troubles (we then need to
> > > > change them all the time in the future in case a update for new
> features and etc).
> > > > So the original purpose is we'd like to preserve it unless it's a big issue.
> > > > What's your opinion?
> > >
> > > > > > +/*
> > > > > > + * 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);
> > > > >
> > > > > Positive logic is usually easier to read. Instead of a 'no_resp'
> > > > > flag you should have a 'resp' flag.
> > > >
> > > > Same explain as above one.
> > > >
> > > > > > +	sc_call_rpc(ipc, &msg, false);
> > > > >
> > > > > This call can fail...
> > > > >
> > > >
> > > > See the similar reply above.
> > >
> > > My opinion to this is that the "we want to stay in sync with our internal
> code"
> > > argument is not valid. With that argument you could sell anything
> > > and I think I repeat myself when I say that Linux would look
> > > different when we would merge everything a vendor throws over the
> fence.
> > >
> >
> > I'm not meaning 'we must stay in sync with our internal code.'
> > We definitely can change everything which violates Linux rule a lot.
> > My original meaning is if it's not big issue and is acceptable by
> > Linux, then we may prefer to not change as the sync helps a lot for
> > the future maintain.
> 
> Ok, understood.
> 
> 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%7C203
> b41c5ce344254fa8108d5db5e4b26%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C636656120391792099&sdata=UJ%2B%2F0atqFkIq5gsopPBui%2B
> QeYOlRhwIPM7oOfrhZE4I%3D&reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |