[PATCH V6 0/3] soc: imx: add scu power domain driver

[PATCH V6 0/3] soc: imx: add scu power domain driver

[A.s. Dong]

This patch set adds the scu based power domain driver.
It depends on SCU driver.

Change Log:
v5->v6:
 * only compatible string name updated from fsl,scu-pd to fsl,imx8qxp-scu-pd
   accordingly
v4->v5:
 * some minor improvements according to Sascha's suggestion
   Note: did not use dev_* print functions due to we already have proper
         prefix by redefining pr_fmt. So it seems not quite neccesary
	 to pass in a struct device * pointer for debug purpose only in
	 each functions.
v3->v4:
 * only scu headfile path update

v2->v3:
 * structure and enums name update
 * api usage update

Dong Aisheng (3):
  dt-bindings: fsl: scu: make power domain compatible string SoC
    specific
  firmware: imx: add pm svc headfile
  firmware: imx: add SCU power domain driver

 .../devicetree/bindings/arm/freescale/fsl,scu.txt  |   4 +-
 drivers/firmware/imx/Kconfig                       |   6 +
 drivers/firmware/imx/Makefile                      |   3 +-
 drivers/firmware/imx/scu-pd.c                      | 164 +++++++++++++++++++++
 include/linux/firmware/imx/sci.h                   |   1 +
 include/linux/firmware/imx/svc/pm.h                |  85 +++++++++++
 6 files changed, 260 insertions(+), 3 deletions(-)
 create mode 100644 drivers/firmware/imx/scu-pd.c
 create mode 100644 include/linux/firmware/imx/svc/pm.h

-- 
2.7.4

[PATCH V6 0/3] soc: imx: add scu power domain driver

[A.s. Dong]

This patch set adds the scu based power domain driver.
It depends on SCU driver.

Change Log:
v5->v6:
 * only compatible string name updated from fsl,scu-pd to fsl,imx8qxp-scu-pd
   accordingly
v4->v5:
 * some minor improvements according to Sascha's suggestion
   Note: did not use dev_* print functions due to we already have proper
         prefix by redefining pr_fmt. So it seems not quite neccesary
	 to pass in a struct device * pointer for debug purpose only in
	 each functions.
v3->v4:
 * only scu headfile path update

v2->v3:
 * structure and enums name update
 * api usage update

Dong Aisheng (3):
  dt-bindings: fsl: scu: make power domain compatible string SoC
    specific
  firmware: imx: add pm svc headfile
  firmware: imx: add SCU power domain driver

 .../devicetree/bindings/arm/freescale/fsl,scu.txt  |   4 +-
 drivers/firmware/imx/Kconfig                       |   6 +
 drivers/firmware/imx/Makefile                      |   3 +-
 drivers/firmware/imx/scu-pd.c                      | 164 +++++++++++++++++++++
 include/linux/firmware/imx/sci.h                   |   1 +
 include/linux/firmware/imx/svc/pm.h                |  85 +++++++++++
 6 files changed, 260 insertions(+), 3 deletions(-)
 create mode 100644 drivers/firmware/imx/scu-pd.c
 create mode 100644 include/linux/firmware/imx/svc/pm.h

-- 
2.7.4

[PATCH V6 1/3] dt-bindings: fsl: scu: make power domain compatible string SoC specific

[A.s. Dong]

As the power domain API might change in the future for new SoCs, although
in a very low possibility, it's still better to make the compatible string
more SoC specific to avoid the possible version change for new SoCs.

Due to there're still no users in kernel, it's safe to update it without
breaking anything.

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
Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
v5->v6:
 * new patch
---
 Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
index 46d0af1..87fc4b4 100644
--- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -58,7 +58,7 @@ This binding for the SCU power domain providers uses the generic power
 domain binding[2].
 
 Required properties:
-- compatible:		Should be "fsl,scu-pd".
+- compatible:		Should be "fsl,imx8qxp-scu-pd".
 - #address-cells:	Should be 1.
 - #size-cells:		Should be 0.
 
@@ -153,7 +153,7 @@ firmware {
 		};
 
 		imx8qx-pm {
-			compatible = "fsl,scu-pd";
+			compatible = "fsl,imx8qxp-scu-pd";
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-- 
2.7.4

[PATCH V6 1/3] dt-bindings: fsl: scu: make power domain compatible string SoC specific

[A.s. Dong]

As the power domain API might change in the future for new SoCs, although
in a very low possibility, it's still better to make the compatible string
more SoC specific to avoid the possible version change for new SoCs.

Due to there're still no users in kernel, it's safe to update it without
breaking anything.

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
Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
v5->v6:
 * new patch
---
 Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
index 46d0af1..87fc4b4 100644
--- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -58,7 +58,7 @@ This binding for the SCU power domain providers uses the generic power
 domain binding[2].
 
 Required properties:
-- compatible:		Should be "fsl,scu-pd".
+- compatible:		Should be "fsl,imx8qxp-scu-pd".
 - #address-cells:	Should be 1.
 - #size-cells:		Should be 0.
 
@@ -153,7 +153,7 @@ firmware {
 		};
 
 		imx8qx-pm {
-			compatible = "fsl,scu-pd";
+			compatible = "fsl,imx8qxp-scu-pd";
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-- 
2.7.4

[PATCH V6 2/3] firmware: imx: add pm svc headfile

[A.s. Dong]

Add SCU PM SVC related protocol definitions which will be used by a number
of PM functions like Power Domain, Clock, Reset and etc. The detailed
implementation of each function will put in the individual function drivers.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v3->v4:
 * update firmware headfile patch from include/soc/imx to
   include/linux/firmware/imx
v2->v3:
 * name updated with IMX_SC prefix
v1->v2:
 * new introduced
---
 include/linux/firmware/imx/sci.h    |  1 +
 include/linux/firmware/imx/svc/pm.h | 85 +++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+)
 create mode 100644 include/linux/firmware/imx/svc/pm.h

diff --git a/include/linux/firmware/imx/sci.h b/include/linux/firmware/imx/sci.h
index 29ada60..ebc5509 100644
--- a/include/linux/firmware/imx/sci.h
+++ b/include/linux/firmware/imx/sci.h
@@ -14,4 +14,5 @@
 #include <linux/firmware/imx/types.h>
 
 #include <linux/firmware/imx/svc/misc.h>
+#include <linux/firmware/imx/svc/pm.h>
 #endif /* _SC_SCI_H */
diff --git a/include/linux/firmware/imx/svc/pm.h b/include/linux/firmware/imx/svc/pm.h
new file mode 100644
index 0000000..ee5a7bb
--- /dev/null
+++ b/include/linux/firmware/imx/svc/pm.h
@@ -0,0 +1,85 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017~2018 NXP
+ *
+ * Header file containing the public API for the System Controller (SC)
+ * Power Management (PM) function. This includes functions for power state
+ * control, clock control, reset control, and wake-up event control.
+ *
+ * PM_SVC (SVC) Power Management Service
+ *
+ * Module for the Power Management (PM) service.
+ */
+
+#ifndef _SC_PM_API_H
+#define _SC_PM_API_H
+
+#include <linux/firmware/imx/sci.h>
+
+/*
+ * This type is used to indicate RPC PM function calls.
+ */
+enum imx_sc_pm_func {
+	IMX_SC_PM_FUNC_UNKNOWN = 0,
+	IMX_SC_PM_FUNC_SET_SYS_POWER_MODE = 19,
+	IMX_SC_PM_FUNC_SET_PARTITION_POWER_MODE = 1,
+	IMX_SC_PM_FUNC_GET_SYS_POWER_MODE = 2,
+	IMX_SC_PM_FUNC_SET_RESOURCE_POWER_MODE = 3,
+	IMX_SC_PM_FUNC_GET_RESOURCE_POWER_MODE = 4,
+	IMX_SC_PM_FUNC_REQ_LOW_POWER_MODE = 16,
+	IMX_SC_PM_FUNC_SET_CPU_RESUME_ADDR = 17,
+	IMX_SC_PM_FUNC_REQ_SYS_IF_POWER_MODE = 18,
+	IMX_SC_PM_FUNC_SET_CLOCK_RATE = 5,
+	IMX_SC_PM_FUNC_GET_CLOCK_RATE = 6,
+	IMX_SC_PM_FUNC_CLOCK_ENABLE = 7,
+	IMX_SC_PM_FUNC_SET_CLOCK_PARENT = 14,
+	IMX_SC_PM_FUNC_GET_CLOCK_PARENT = 15,
+	IMX_SC_PM_FUNC_RESET = 13,
+	IMX_SC_PM_FUNC_RESET_REASON = 10,
+	IMX_SC_PM_FUNC_BOOT = 8,
+	IMX_SC_PM_FUNC_REBOOT = 9,
+	IMX_SC_PM_FUNC_REBOOT_PARTITION = 12,
+	IMX_SC_PM_FUNC_CPU_START = 11,
+};
+
+/*
+ * Defines for ALL parameters
+ */
+#define IMX_SC_PM_CLK_ALL		UINT8_MAX	/* All clocks */
+
+/*
+ * Defines for SC PM Power Mode
+ */
+#define IMX_SC_PM_PW_MODE_OFF	0	/* Power off */
+#define IMX_SC_PM_PW_MODE_STBY	1	/* Power in standby */
+#define IMX_SC_PM_PW_MODE_LP	2	/* Power in low-power */
+#define IMX_SC_PM_PW_MODE_ON	3	/* Power on */
+
+/*
+ * Defines for SC PM CLK
+ */
+#define IMX_SC_PM_CLK_SLV_BUS	0	/* Slave bus clock */
+#define IMX_SC_PM_CLK_MST_BUS	1	/* Master bus clock */
+#define IMX_SC_PM_CLK_PER	2	/* Peripheral clock */
+#define IMX_SC_PM_CLK_PHY	3	/* Phy clock */
+#define IMX_SC_PM_CLK_MISC	4	/* Misc clock */
+#define IMX_SC_PM_CLK_MISC0	0	/* Misc 0 clock */
+#define IMX_SC_PM_CLK_MISC1	1	/* Misc 1 clock */
+#define IMX_SC_PM_CLK_MISC2	2	/* Misc 2 clock */
+#define IMX_SC_PM_CLK_MISC3	3	/* Misc 3 clock */
+#define IMX_SC_PM_CLK_MISC4	4	/* Misc 4 clock */
+#define IMX_SC_PM_CLK_CPU	2	/* CPU clock */
+#define IMX_SC_PM_CLK_PLL	4	/* PLL */
+#define IMX_SC_PM_CLK_BYPASS	4	/* Bypass clock */
+
+/*
+ * Defines for SC PM CLK Parent
+ */
+#define IMX_SC_PM_PARENT_XTAL	0	/* Parent is XTAL. */
+#define IMX_SC_PM_PARENT_PLL0	1	/* Parent is PLL0 */
+#define IMX_SC_PM_PARENT_PLL1	2	/* Parent is PLL1 or PLL0/2 */
+#define IMX_SC_PM_PARENT_PLL2	3	/* Parent in PLL2 or PLL0/4 */
+#define IMX_SC_PM_PARENT_BYPS	4	/* Parent is a bypass clock. */
+
+#endif /* _SC_PM_API_H */
-- 
2.7.4

[PATCH V6 2/3] firmware: imx: add pm svc headfile

[A.s. Dong]

Add SCU PM SVC related protocol definitions which will be used by a number
of PM functions like Power Domain, Clock, Reset and etc. The detailed
implementation of each function will put in the individual function drivers.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v3->v4:
 * update firmware headfile patch from include/soc/imx to
   include/linux/firmware/imx
v2->v3:
 * name updated with IMX_SC prefix
v1->v2:
 * new introduced
---
 include/linux/firmware/imx/sci.h    |  1 +
 include/linux/firmware/imx/svc/pm.h | 85 +++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+)
 create mode 100644 include/linux/firmware/imx/svc/pm.h

diff --git a/include/linux/firmware/imx/sci.h b/include/linux/firmware/imx/sci.h
index 29ada60..ebc5509 100644
--- a/include/linux/firmware/imx/sci.h
+++ b/include/linux/firmware/imx/sci.h
@@ -14,4 +14,5 @@
 #include <linux/firmware/imx/types.h>
 
 #include <linux/firmware/imx/svc/misc.h>
+#include <linux/firmware/imx/svc/pm.h>
 #endif /* _SC_SCI_H */
diff --git a/include/linux/firmware/imx/svc/pm.h b/include/linux/firmware/imx/svc/pm.h
new file mode 100644
index 0000000..ee5a7bb
--- /dev/null
+++ b/include/linux/firmware/imx/svc/pm.h
@@ -0,0 +1,85 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017~2018 NXP
+ *
+ * Header file containing the public API for the System Controller (SC)
+ * Power Management (PM) function. This includes functions for power state
+ * control, clock control, reset control, and wake-up event control.
+ *
+ * PM_SVC (SVC) Power Management Service
+ *
+ * Module for the Power Management (PM) service.
+ */
+
+#ifndef _SC_PM_API_H
+#define _SC_PM_API_H
+
+#include <linux/firmware/imx/sci.h>
+
+/*
+ * This type is used to indicate RPC PM function calls.
+ */
+enum imx_sc_pm_func {
+	IMX_SC_PM_FUNC_UNKNOWN = 0,
+	IMX_SC_PM_FUNC_SET_SYS_POWER_MODE = 19,
+	IMX_SC_PM_FUNC_SET_PARTITION_POWER_MODE = 1,
+	IMX_SC_PM_FUNC_GET_SYS_POWER_MODE = 2,
+	IMX_SC_PM_FUNC_SET_RESOURCE_POWER_MODE = 3,
+	IMX_SC_PM_FUNC_GET_RESOURCE_POWER_MODE = 4,
+	IMX_SC_PM_FUNC_REQ_LOW_POWER_MODE = 16,
+	IMX_SC_PM_FUNC_SET_CPU_RESUME_ADDR = 17,
+	IMX_SC_PM_FUNC_REQ_SYS_IF_POWER_MODE = 18,
+	IMX_SC_PM_FUNC_SET_CLOCK_RATE = 5,
+	IMX_SC_PM_FUNC_GET_CLOCK_RATE = 6,
+	IMX_SC_PM_FUNC_CLOCK_ENABLE = 7,
+	IMX_SC_PM_FUNC_SET_CLOCK_PARENT = 14,
+	IMX_SC_PM_FUNC_GET_CLOCK_PARENT = 15,
+	IMX_SC_PM_FUNC_RESET = 13,
+	IMX_SC_PM_FUNC_RESET_REASON = 10,
+	IMX_SC_PM_FUNC_BOOT = 8,
+	IMX_SC_PM_FUNC_REBOOT = 9,
+	IMX_SC_PM_FUNC_REBOOT_PARTITION = 12,
+	IMX_SC_PM_FUNC_CPU_START = 11,
+};
+
+/*
+ * Defines for ALL parameters
+ */
+#define IMX_SC_PM_CLK_ALL		UINT8_MAX	/* All clocks */
+
+/*
+ * Defines for SC PM Power Mode
+ */
+#define IMX_SC_PM_PW_MODE_OFF	0	/* Power off */
+#define IMX_SC_PM_PW_MODE_STBY	1	/* Power in standby */
+#define IMX_SC_PM_PW_MODE_LP	2	/* Power in low-power */
+#define IMX_SC_PM_PW_MODE_ON	3	/* Power on */
+
+/*
+ * Defines for SC PM CLK
+ */
+#define IMX_SC_PM_CLK_SLV_BUS	0	/* Slave bus clock */
+#define IMX_SC_PM_CLK_MST_BUS	1	/* Master bus clock */
+#define IMX_SC_PM_CLK_PER	2	/* Peripheral clock */
+#define IMX_SC_PM_CLK_PHY	3	/* Phy clock */
+#define IMX_SC_PM_CLK_MISC	4	/* Misc clock */
+#define IMX_SC_PM_CLK_MISC0	0	/* Misc 0 clock */
+#define IMX_SC_PM_CLK_MISC1	1	/* Misc 1 clock */
+#define IMX_SC_PM_CLK_MISC2	2	/* Misc 2 clock */
+#define IMX_SC_PM_CLK_MISC3	3	/* Misc 3 clock */
+#define IMX_SC_PM_CLK_MISC4	4	/* Misc 4 clock */
+#define IMX_SC_PM_CLK_CPU	2	/* CPU clock */
+#define IMX_SC_PM_CLK_PLL	4	/* PLL */
+#define IMX_SC_PM_CLK_BYPASS	4	/* Bypass clock */
+
+/*
+ * Defines for SC PM CLK Parent
+ */
+#define IMX_SC_PM_PARENT_XTAL	0	/* Parent is XTAL. */
+#define IMX_SC_PM_PARENT_PLL0	1	/* Parent is PLL0 */
+#define IMX_SC_PM_PARENT_PLL1	2	/* Parent is PLL1 or PLL0/2 */
+#define IMX_SC_PM_PARENT_PLL2	3	/* Parent in PLL2 or PLL0/4 */
+#define IMX_SC_PM_PARENT_BYPS	4	/* Parent is a bypass clock. */
+
+#endif /* _SC_PM_API_H */
-- 
2.7.4

[PATCH V6 3/3] firmware: imx: add SCU power domain driver

[A.s. Dong]

Some i.MX SoCs contain a system controller that is responsible for
controlling the state of the IPs that are present. Communication
between the host processor running an OS and the system controller
happens through a SCU protocol. This patch adds SCU protocol based
power domains drivers.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: linux-pm@vger.kernel.org
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
v5->v6:
 * only compatible string name updated from fsl,scu-pd to fsl,imx8qxp-scu-pd
   accordingly
v4->v5:
 * minor improvements according to Sascha's suggestions
v3->v4:
 * update firmware headfile patch from include/soc/imx to
   include/linux/firmware/imx
v2->v3:
 * name of structures/enums updated with imx_sc prefix
v1->v2:
 * move into drivers/firmware/imx
 * Implement sc_pm_set_resource_power_mode() API in the driver instead
   of call it via SCU API according to Sascha's suggestion
---
 drivers/firmware/imx/Kconfig  |   6 ++
 drivers/firmware/imx/Makefile |   3 +-
 drivers/firmware/imx/scu-pd.c | 164 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 172 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/imx/scu-pd.c

diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
index b170c28..6a7a7c2 100644
--- a/drivers/firmware/imx/Kconfig
+++ b/drivers/firmware/imx/Kconfig
@@ -9,3 +9,9 @@ config IMX_SCU
 
 	  This driver manages the IPC interface between host CPU and the
 	  SCU firmware running on M4.
+
+config IMX_SCU_PD
+	bool "IMX SCU Power Domain driver"
+	depends on IMX_SCU
+	help
+	  The System Controller Firmware (SCFW) based power domain driver.
diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
index 0ac04df..1b2e15b 100644
--- a/drivers/firmware/imx/Makefile
+++ b/drivers/firmware/imx/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_IMX_SCU)	+= imx-scu.o misc.o
+obj-$(CONFIG_IMX_SCU)		+= imx-scu.o misc.o
+obj-$(CONFIG_IMX_SCU_PD)	+= scu-pd.o
diff --git a/drivers/firmware/imx/scu-pd.c b/drivers/firmware/imx/scu-pd.c
new file mode 100644
index 0000000..1fbf4fe
--- /dev/null
+++ b/drivers/firmware/imx/scu-pd.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017~2018 NXP
+ *	Dong Aisheng <aisheng.dong@nxp.com>
+ *
+ * Implementation of the SCU based Power Domains
+ */
+
+#define pr_fmt(fmt) "scu_pd: " fmt
+
+#include <linux/firmware/imx/sci.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_domain.h>
+#include <linux/slab.h>
+
+struct imx_sc_pm_domain {
+	struct generic_pm_domain pd;
+	u32 rsrc_id;
+};
+
+static struct imx_sc_ipc *pm_ipc_handle;
+
+/* SCU Power Mode Protocol definition */
+struct imx_sc_msg_req_set_resource_power_mode {
+	struct imx_sc_rpc_msg hdr;
+	u16 resource;
+	u8 mode;
+} __packed;
+
+static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on)
+{
+	struct imx_sc_msg_req_set_resource_power_mode msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	struct imx_sc_pm_domain *pd;
+	int ret;
+
+	pd = container_of(domain, struct imx_sc_pm_domain, pd);
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM;
+	hdr->func = (uint8_t)IMX_SC_PM_FUNC_SET_RESOURCE_POWER_MODE;
+	hdr->size = 2;
+
+	msg.resource = pd->rsrc_id;
+	msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : IMX_SC_PM_PW_MODE_LP;
+
+	ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
+	if (ret)
+		pr_err("failed to power %s resource %d ret %d\n",
+			power_on ? "up" : "off", pd->rsrc_id, ret);
+
+	return ret;
+}
+
+static int imx_sc_pd_power_on(struct generic_pm_domain *domain)
+{
+	return imx_sc_pd_power(domain, true);
+}
+
+static int imx_sc_pd_power_off(struct generic_pm_domain *domain)
+{
+	return imx_sc_pd_power(domain, false);
+}
+
+static struct generic_pm_domain *imx_sc_pm_add_one_domain(
+					struct device_node *np,
+					struct generic_pm_domain *genpd_parent)
+{
+	struct imx_sc_pm_domain *imx_sc_pd;
+	u32 rsrc_id;
+	int ret;
+
+	imx_sc_pd = kzalloc(sizeof(*imx_sc_pd), GFP_KERNEL);
+	if (!imx_sc_pd)
+		return ERR_PTR(-ENOMEM);
+
+	if (!of_property_read_u32(np, "reg", &rsrc_id)) {
+		if (rsrc_id >= IMX_SC_R_LAST) {
+			pr_warn("%pOF: invalid rsrc id %d found", np, rsrc_id);
+			ret = -EINVAL;
+			goto err;
+		}
+		imx_sc_pd->rsrc_id = rsrc_id;
+		imx_sc_pd->pd.power_off = imx_sc_pd_power_off;
+		imx_sc_pd->pd.power_on = imx_sc_pd_power_on;
+	} else {
+		imx_sc_pd->rsrc_id = IMX_SC_R_LAST;
+	}
+
+	imx_sc_pd->pd.name = np->name;
+
+	ret = pm_genpd_init(&imx_sc_pd->pd, NULL, true);
+	if (ret < 0)
+		goto err;
+
+	if (genpd_parent) {
+		ret = pm_genpd_add_subdomain(genpd_parent, &imx_sc_pd->pd);
+		if (ret)
+			goto err;
+	}
+
+	ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
+	if (!ret)
+		return &imx_sc_pd->pd;
+
+	pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
+
+err:
+	pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
+	kfree(imx_sc_pd);
+	return ERR_PTR(ret);
+}
+
+static void imx_sc_pm_add_pm_domains(struct device_node *parent,
+				     struct generic_pm_domain *genpd_parent)
+{
+	struct generic_pm_domain *pd;
+	struct device_node *np;
+
+	for_each_child_of_node(parent, np) {
+		pd = imx_sc_pm_add_one_domain(np, genpd_parent);
+		if (!IS_ERR(pd))
+			imx_sc_pm_add_pm_domains(np, pd);
+	}
+}
+
+static int imx_sc_pd_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int ret;
+
+	ret = imx_scu_get_handle(&pm_ipc_handle);
+	if (ret)
+		return ret;
+
+	imx_sc_pm_add_pm_domains(np, NULL);
+
+	return 0;
+}
+
+static const struct of_device_id imx_sc_pd_match[] = {
+	{ .compatible = "fsl,imx8qxp-scu-pd", },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver imx_sc_pd_driver = {
+	.driver = {
+		.name = "imx-scu-pd",
+		.of_match_table = imx_sc_pd_match,
+	},
+	.probe = imx_sc_pd_probe,
+};
+builtin_platform_driver(imx_sc_pd_driver);
+
+MODULE_AUTHOR("Dong Aisheng <aisheng.dong@nxp.com>");
+MODULE_DESCRIPTION("IMX SCU Power Domain driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH V6 3/3] firmware: imx: add SCU power domain driver

[A.s. Dong]

Some i.MX SoCs contain a system controller that is responsible for
controlling the state of the IPs that are present. Communication
between the host processor running an OS and the system controller
happens through a SCU protocol. This patch adds SCU protocol based
power domains drivers.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: linux-pm at vger.kernel.org
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
v5->v6:
 * only compatible string name updated from fsl,scu-pd to fsl,imx8qxp-scu-pd
   accordingly
v4->v5:
 * minor improvements according to Sascha's suggestions
v3->v4:
 * update firmware headfile patch from include/soc/imx to
   include/linux/firmware/imx
v2->v3:
 * name of structures/enums updated with imx_sc prefix
v1->v2:
 * move into drivers/firmware/imx
 * Implement sc_pm_set_resource_power_mode() API in the driver instead
   of call it via SCU API according to Sascha's suggestion
---
 drivers/firmware/imx/Kconfig  |   6 ++
 drivers/firmware/imx/Makefile |   3 +-
 drivers/firmware/imx/scu-pd.c | 164 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 172 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/imx/scu-pd.c

diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
index b170c28..6a7a7c2 100644
--- a/drivers/firmware/imx/Kconfig
+++ b/drivers/firmware/imx/Kconfig
@@ -9,3 +9,9 @@ config IMX_SCU
 
 	  This driver manages the IPC interface between host CPU and the
 	  SCU firmware running on M4.
+
+config IMX_SCU_PD
+	bool "IMX SCU Power Domain driver"
+	depends on IMX_SCU
+	help
+	  The System Controller Firmware (SCFW) based power domain driver.
diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
index 0ac04df..1b2e15b 100644
--- a/drivers/firmware/imx/Makefile
+++ b/drivers/firmware/imx/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_IMX_SCU)	+= imx-scu.o misc.o
+obj-$(CONFIG_IMX_SCU)		+= imx-scu.o misc.o
+obj-$(CONFIG_IMX_SCU_PD)	+= scu-pd.o
diff --git a/drivers/firmware/imx/scu-pd.c b/drivers/firmware/imx/scu-pd.c
new file mode 100644
index 0000000..1fbf4fe
--- /dev/null
+++ b/drivers/firmware/imx/scu-pd.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2017~2018 NXP
+ *	Dong Aisheng <aisheng.dong@nxp.com>
+ *
+ * Implementation of the SCU based Power Domains
+ */
+
+#define pr_fmt(fmt) "scu_pd: " fmt
+
+#include <linux/firmware/imx/sci.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_domain.h>
+#include <linux/slab.h>
+
+struct imx_sc_pm_domain {
+	struct generic_pm_domain pd;
+	u32 rsrc_id;
+};
+
+static struct imx_sc_ipc *pm_ipc_handle;
+
+/* SCU Power Mode Protocol definition */
+struct imx_sc_msg_req_set_resource_power_mode {
+	struct imx_sc_rpc_msg hdr;
+	u16 resource;
+	u8 mode;
+} __packed;
+
+static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on)
+{
+	struct imx_sc_msg_req_set_resource_power_mode msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	struct imx_sc_pm_domain *pd;
+	int ret;
+
+	pd = container_of(domain, struct imx_sc_pm_domain, pd);
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM;
+	hdr->func = (uint8_t)IMX_SC_PM_FUNC_SET_RESOURCE_POWER_MODE;
+	hdr->size = 2;
+
+	msg.resource = pd->rsrc_id;
+	msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : IMX_SC_PM_PW_MODE_LP;
+
+	ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
+	if (ret)
+		pr_err("failed to power %s resource %d ret %d\n",
+			power_on ? "up" : "off", pd->rsrc_id, ret);
+
+	return ret;
+}
+
+static int imx_sc_pd_power_on(struct generic_pm_domain *domain)
+{
+	return imx_sc_pd_power(domain, true);
+}
+
+static int imx_sc_pd_power_off(struct generic_pm_domain *domain)
+{
+	return imx_sc_pd_power(domain, false);
+}
+
+static struct generic_pm_domain *imx_sc_pm_add_one_domain(
+					struct device_node *np,
+					struct generic_pm_domain *genpd_parent)
+{
+	struct imx_sc_pm_domain *imx_sc_pd;
+	u32 rsrc_id;
+	int ret;
+
+	imx_sc_pd = kzalloc(sizeof(*imx_sc_pd), GFP_KERNEL);
+	if (!imx_sc_pd)
+		return ERR_PTR(-ENOMEM);
+
+	if (!of_property_read_u32(np, "reg", &rsrc_id)) {
+		if (rsrc_id >= IMX_SC_R_LAST) {
+			pr_warn("%pOF: invalid rsrc id %d found", np, rsrc_id);
+			ret = -EINVAL;
+			goto err;
+		}
+		imx_sc_pd->rsrc_id = rsrc_id;
+		imx_sc_pd->pd.power_off = imx_sc_pd_power_off;
+		imx_sc_pd->pd.power_on = imx_sc_pd_power_on;
+	} else {
+		imx_sc_pd->rsrc_id = IMX_SC_R_LAST;
+	}
+
+	imx_sc_pd->pd.name = np->name;
+
+	ret = pm_genpd_init(&imx_sc_pd->pd, NULL, true);
+	if (ret < 0)
+		goto err;
+
+	if (genpd_parent) {
+		ret = pm_genpd_add_subdomain(genpd_parent, &imx_sc_pd->pd);
+		if (ret)
+			goto err;
+	}
+
+	ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
+	if (!ret)
+		return &imx_sc_pd->pd;
+
+	pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
+
+err:
+	pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
+	kfree(imx_sc_pd);
+	return ERR_PTR(ret);
+}
+
+static void imx_sc_pm_add_pm_domains(struct device_node *parent,
+				     struct generic_pm_domain *genpd_parent)
+{
+	struct generic_pm_domain *pd;
+	struct device_node *np;
+
+	for_each_child_of_node(parent, np) {
+		pd = imx_sc_pm_add_one_domain(np, genpd_parent);
+		if (!IS_ERR(pd))
+			imx_sc_pm_add_pm_domains(np, pd);
+	}
+}
+
+static int imx_sc_pd_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int ret;
+
+	ret = imx_scu_get_handle(&pm_ipc_handle);
+	if (ret)
+		return ret;
+
+	imx_sc_pm_add_pm_domains(np, NULL);
+
+	return 0;
+}
+
+static const struct of_device_id imx_sc_pd_match[] = {
+	{ .compatible = "fsl,imx8qxp-scu-pd", },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver imx_sc_pd_driver = {
+	.driver = {
+		.name = "imx-scu-pd",
+		.of_match_table = imx_sc_pd_match,
+	},
+	.probe = imx_sc_pd_probe,
+};
+builtin_platform_driver(imx_sc_pd_driver);
+
+MODULE_AUTHOR("Dong Aisheng <aisheng.dong@nxp.com>");
+MODULE_DESCRIPTION("IMX SCU Power Domain driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH V6 1/3] dt-bindings: fsl: scu: make power domain compatible string SoC specific

[Rob Herring]

On Wed, Oct 17, 2018 at 07:24:40AM +0000, A.s. Dong wrote:
> As the power domain API might change in the future for new SoCs, although
> in a very low possibility, it's still better to make the compatible string
> more SoC specific to avoid the possible version change for new SoCs.
> 
> Due to there're still no users in kernel, it's safe to update it without
> breaking anything.
> 
> 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
> Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

checkpatch says:

WARNING: Missing Signed-off-by: line by nominal patch author 'A.s. Dong <aisheng.dong@nxp.com>'

> ---
> ChangeLog:
> v5->v6:
>  * new patch
> ---
>  Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> index 46d0af1..87fc4b4 100644
> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> @@ -58,7 +58,7 @@ This binding for the SCU power domain providers uses the generic power
>  domain binding[2].
>  
>  Required properties:
> -- compatible:		Should be "fsl,scu-pd".
> +- compatible:		Should be "fsl,imx8qxp-scu-pd".

You can keep both if you think future SoCs will be compatible.

>  - #address-cells:	Should be 1.
>  - #size-cells:		Should be 0.
>  
> @@ -153,7 +153,7 @@ firmware {
>  		};
>  
>  		imx8qx-pm {
> -			compatible = "fsl,scu-pd";
> +			compatible = "fsl,imx8qxp-scu-pd";
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  
> -- 
> 2.7.4
> 

[PATCH V6 1/3] dt-bindings: fsl: scu: make power domain compatible string SoC specific

[Rob Herring]

On Wed, Oct 17, 2018 at 07:24:40AM +0000, A.s. Dong wrote:
> As the power domain API might change in the future for new SoCs, although
> in a very low possibility, it's still better to make the compatible string
> more SoC specific to avoid the possible version change for new SoCs.
> 
> Due to there're still no users in kernel, it's safe to update it without
> breaking anything.
> 
> 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
> Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

checkpatch says:

WARNING: Missing Signed-off-by: line by nominal patch author 'A.s. Dong <aisheng.dong@nxp.com>'

> ---
> ChangeLog:
> v5->v6:
>  * new patch
> ---
>  Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> index 46d0af1..87fc4b4 100644
> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> @@ -58,7 +58,7 @@ This binding for the SCU power domain providers uses the generic power
>  domain binding[2].
>  
>  Required properties:
> -- compatible:		Should be "fsl,scu-pd".
> +- compatible:		Should be "fsl,imx8qxp-scu-pd".

You can keep both if you think future SoCs will be compatible.

>  - #address-cells:	Should be 1.
>  - #size-cells:		Should be 0.
>  
> @@ -153,7 +153,7 @@ firmware {
>  		};
>  
>  		imx8qx-pm {
> -			compatible = "fsl,scu-pd";
> +			compatible = "fsl,imx8qxp-scu-pd";
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  
> -- 
> 2.7.4
> 

RE: [PATCH V6 1/3] dt-bindings: fsl: scu: make power domain compatible string SoC specific

[A.s. Dong]

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Thursday, October 18, 2018 10:34 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
[...]
> On Wed, Oct 17, 2018 at 07:24:40AM +0000, A.s. Dong wrote:
> > As the power domain API might change in the future for new SoCs,
> > although in a very low possibility, it's still better to make the
> > compatible string more SoC specific to avoid the possible version change for
> new SoCs.
> >
> > Due to there're still no users in kernel, it's safe to update it
> > without breaking anything.
> >
> > 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
> > Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> 
> checkpatch says:
> 
> WARNING: Missing Signed-off-by: line by nominal patch author 'A.s. Dong
> <aisheng.dong@nxp.com>'
> 

Do you think if we can ignore this warning?

It seems checkpatch checks the email sender name as nominal patch author
which is abbreviated as 'A.S. Dong'. But I always use the full name in
upstreaming patches.

> > ---
> > ChangeLog:
> > v5->v6:
> >  * new patch
> > ---
> >  Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > index 46d0af1..87fc4b4 100644
> > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > @@ -58,7 +58,7 @@ This binding for the SCU power domain providers uses
> > the generic power  domain binding[2].
> >
> >  Required properties:
> > -- compatible:		Should be "fsl,scu-pd".
> > +- compatible:		Should be "fsl,imx8qxp-scu-pd".
> 
> You can keep both if you think future SoCs will be compatible.
> 

They're likely to be compatible AFAIK.

Just to be clear, you mean keep both of them?
e.g.
compatible: Should be "fsl,imx8qxp-scu-pd" or "fsl,scu-pd"
Can you please help clarify a bit more on why it's better to do that as I'm not quite
Understand?

And for later when mx8qm is supported, should we add it again as follows?
compatible: Should be "fsl,imx8qm-scu-pd", "fsl,imx8qxp-scu-pd" or "fsl,scu-pd"

Regards
Dong Aisheng

> >  - #address-cells:	Should be 1.
> >  - #size-cells:		Should be 0.
> >
> > @@ -153,7 +153,7 @@ firmware {
> >  		};
> >
> >  		imx8qx-pm {
> > -			compatible = "fsl,scu-pd";
> > +			compatible = "fsl,imx8qxp-scu-pd";
> >  			#address-cells = <1>;
> >  			#size-cells = <0>;
> >
> > --
> > 2.7.4
> >

[PATCH V6 1/3] dt-bindings: fsl: scu: make power domain compatible string SoC specific

[A.s. Dong]

> -----Original Message-----
> From: Rob Herring [mailto:robh at kernel.org]
> Sent: Thursday, October 18, 2018 10:34 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
[...]
> On Wed, Oct 17, 2018 at 07:24:40AM +0000, A.s. Dong wrote:
> > As the power domain API might change in the future for new SoCs,
> > although in a very low possibility, it's still better to make the
> > compatible string more SoC specific to avoid the possible version change for
> new SoCs.
> >
> > Due to there're still no users in kernel, it's safe to update it
> > without breaking anything.
> >
> > 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
> > Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> 
> checkpatch says:
> 
> WARNING: Missing Signed-off-by: line by nominal patch author 'A.s. Dong
> <aisheng.dong@nxp.com>'
> 

Do you think if we can ignore this warning?

It seems checkpatch checks the email sender name as nominal patch author
which is abbreviated as 'A.S. Dong'. But I always use the full name in
upstreaming patches.

> > ---
> > ChangeLog:
> > v5->v6:
> >  * new patch
> > ---
> >  Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > index 46d0af1..87fc4b4 100644
> > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > @@ -58,7 +58,7 @@ This binding for the SCU power domain providers uses
> > the generic power  domain binding[2].
> >
> >  Required properties:
> > -- compatible:		Should be "fsl,scu-pd".
> > +- compatible:		Should be "fsl,imx8qxp-scu-pd".
> 
> You can keep both if you think future SoCs will be compatible.
> 

They're likely to be compatible AFAIK.

Just to be clear, you mean keep both of them?
e.g.
compatible: Should be "fsl,imx8qxp-scu-pd" or "fsl,scu-pd"
Can you please help clarify a bit more on why it's better to do that as I'm not quite
Understand?

And for later when mx8qm is supported, should we add it again as follows?
compatible: Should be "fsl,imx8qm-scu-pd", "fsl,imx8qxp-scu-pd" or "fsl,scu-pd"

Regards
Dong Aisheng

> >  - #address-cells:	Should be 1.
> >  - #size-cells:		Should be 0.
> >
> > @@ -153,7 +153,7 @@ firmware {
> >  		};
> >
> >  		imx8qx-pm {
> > -			compatible = "fsl,scu-pd";
> > +			compatible = "fsl,imx8qxp-scu-pd";
> >  			#address-cells = <1>;
> >  			#size-cells = <0>;
> >
> > --
> > 2.7.4
> >

Re: [PATCH V6 1/3] dt-bindings: fsl: scu: make power domain compatible string SoC specific

[Rob Herring]

On Thu, Oct 18, 2018 at 10:08 AM A.s. Dong <aisheng.dong@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Rob Herring [mailto:robh@kernel.org]
> > Sent: Thursday, October 18, 2018 10:34 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> [...]
> > On Wed, Oct 17, 2018 at 07:24:40AM +0000, A.s. Dong wrote:
> > > As the power domain API might change in the future for new SoCs,
> > > although in a very low possibility, it's still better to make the
> > > compatible string more SoC specific to avoid the possible version change for
> > new SoCs.
> > >
> > > Due to there're still no users in kernel, it's safe to update it
> > > without breaking anything.
> > >
> > > 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
> > > Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> >
> > checkpatch says:
> >
> > WARNING: Missing Signed-off-by: line by nominal patch author 'A.s. Dong
> > <aisheng.dong@nxp.com>'
> >
>
> Do you think if we can ignore this warning?

Probably, but better if you can fix it.

> It seems checkpatch checks the email sender name as nominal patch author
> which is abbreviated as 'A.S. Dong'. But I always use the full name in
> upstreaming patches.

The patch contents should start with a 'From:' line if your sending
email doesn't match the patch author (such as when you sent another
author's patch). Maybe it did match and your email server is
overwriting it. I would imagine that can be fixed.

> > > ---
> > > ChangeLog:
> > > v5->v6:
> > >  * new patch
> > > ---
> > >  Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > index 46d0af1..87fc4b4 100644
> > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > @@ -58,7 +58,7 @@ This binding for the SCU power domain providers uses
> > > the generic power  domain binding[2].
> > >
> > >  Required properties:
> > > -- compatible:              Should be "fsl,scu-pd".
> > > +- compatible:              Should be "fsl,imx8qxp-scu-pd".
> >
> > You can keep both if you think future SoCs will be compatible.
> >
>
> They're likely to be compatible AFAIK.
>
> Just to be clear, you mean keep both of them?

Yes, but...
> e.g.
> compatible: Should be "fsl,imx8qxp-scu-pd" or "fsl,scu-pd"

Not exactly, "fsl,scu-pd" should be a fallback and the driver can
match on that (until you need to distinguish). So like this:

Should be one of:
  "fsl,imx8qxp-scu-pd"
followed by "fsl,scu-pd"

> Can you please help clarify a bit more on why it's better to do that as I'm not quite
> Understand?
>
> And for later when mx8qm is supported, should we add it again as follows?
> compatible: Should be "fsl,imx8qm-scu-pd", "fsl,imx8qxp-scu-pd" or "fsl,scu-pd"

If formatted as above, then it is a one line change to add:

Should be one of:
  "fsl,imx8qm-scu-pd"
  "fsl,imx8qxp-scu-pd"
followed by "fsl,scu-pd"

Rob

[PATCH V6 1/3] dt-bindings: fsl: scu: make power domain compatible string SoC specific

[Rob Herring]

On Thu, Oct 18, 2018 at 10:08 AM A.s. Dong <aisheng.dong@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Rob Herring [mailto:robh at kernel.org]
> > Sent: Thursday, October 18, 2018 10:34 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> [...]
> > On Wed, Oct 17, 2018 at 07:24:40AM +0000, A.s. Dong wrote:
> > > As the power domain API might change in the future for new SoCs,
> > > although in a very low possibility, it's still better to make the
> > > compatible string more SoC specific to avoid the possible version change for
> > new SoCs.
> > >
> > > Due to there're still no users in kernel, it's safe to update it
> > > without breaking anything.
> > >
> > > 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
> > > Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> >
> > checkpatch says:
> >
> > WARNING: Missing Signed-off-by: line by nominal patch author 'A.s. Dong
> > <aisheng.dong@nxp.com>'
> >
>
> Do you think if we can ignore this warning?

Probably, but better if you can fix it.

> It seems checkpatch checks the email sender name as nominal patch author
> which is abbreviated as 'A.S. Dong'. But I always use the full name in
> upstreaming patches.

The patch contents should start with a 'From:' line if your sending
email doesn't match the patch author (such as when you sent another
author's patch). Maybe it did match and your email server is
overwriting it. I would imagine that can be fixed.

> > > ---
> > > ChangeLog:
> > > v5->v6:
> > >  * new patch
> > > ---
> > >  Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > index 46d0af1..87fc4b4 100644
> > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > @@ -58,7 +58,7 @@ This binding for the SCU power domain providers uses
> > > the generic power  domain binding[2].
> > >
> > >  Required properties:
> > > -- compatible:              Should be "fsl,scu-pd".
> > > +- compatible:              Should be "fsl,imx8qxp-scu-pd".
> >
> > You can keep both if you think future SoCs will be compatible.
> >
>
> They're likely to be compatible AFAIK.
>
> Just to be clear, you mean keep both of them?

Yes, but...
> e.g.
> compatible: Should be "fsl,imx8qxp-scu-pd" or "fsl,scu-pd"

Not exactly, "fsl,scu-pd" should be a fallback and the driver can
match on that (until you need to distinguish). So like this:

Should be one of:
  "fsl,imx8qxp-scu-pd"
followed by "fsl,scu-pd"

> Can you please help clarify a bit more on why it's better to do that as I'm not quite
> Understand?
>
> And for later when mx8qm is supported, should we add it again as follows?
> compatible: Should be "fsl,imx8qm-scu-pd", "fsl,imx8qxp-scu-pd" or "fsl,scu-pd"

If formatted as above, then it is a one line change to add:

Should be one of:
  "fsl,imx8qm-scu-pd"
  "fsl,imx8qxp-scu-pd"
followed by "fsl,scu-pd"

Rob

RE: [PATCH V6 1/3] dt-bindings: fsl: scu: make power domain compatible string SoC specific

[A.s. Dong]

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Thursday, October 18, 2018 11:39 PM
[...]
> On Thu, Oct 18, 2018 at 10:08 AM A.s. Dong <aisheng.dong@nxp.com> wrote:
> >
> > > -----Original Message-----
> > > From: Rob Herring [mailto:robh@kernel.org]
> > > Sent: Thursday, October 18, 2018 10:34 PM
> > > To: A.s. Dong <aisheng.dong@nxp.com>
> > [...]
> > > On Wed, Oct 17, 2018 at 07:24:40AM +0000, A.s. Dong wrote:
> > > > As the power domain API might change in the future for new SoCs,
> > > > although in a very low possibility, it's still better to make the
> > > > compatible string more SoC specific to avoid the possible version
> > > > change for
> > > new SoCs.
> > > >
> > > > Due to there're still no users in kernel, it's safe to update it
> > > > without breaking anything.
> > > >
> > > > 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
> > > > Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > >
> > > checkpatch says:
> > >
> > > WARNING: Missing Signed-off-by: line by nominal patch author 'A.s.
> > > Dong <aisheng.dong@nxp.com>'
> > >
> >
> > Do you think if we can ignore this warning?
> 
> Probably, but better if you can fix it.
> 
> > It seems checkpatch checks the email sender name as nominal patch
> > author which is abbreviated as 'A.S. Dong'. But I always use the full
> > name in upstreaming patches.
> 
> The patch contents should start with a 'From:' line if your sending email
> doesn't match the patch author (such as when you sent another author's
> patch). Maybe it did match and your email server is overwriting it. I would
> imagine that can be fixed.
> 

Yes, I will try to fix it. Thanks for the suggestion.
As this may be related to our email server, probably may not be able to fixe so quick
for this patch resend. Please let me know if you mind it.

> > > > ---
> > > > ChangeLog:
> > > > v5->v6:
> > > >  * new patch
> > > > ---
> > > >  Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 4
> > > > ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > index 46d0af1..87fc4b4 100644
> > > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > @@ -58,7 +58,7 @@ This binding for the SCU power domain providers
> > > > uses the generic power  domain binding[2].
> > > >
> > > >  Required properties:
> > > > -- compatible:              Should be "fsl,scu-pd".
> > > > +- compatible:              Should be "fsl,imx8qxp-scu-pd".
> > >
> > > You can keep both if you think future SoCs will be compatible.
> > >
> >
> > They're likely to be compatible AFAIK.
> >
> > Just to be clear, you mean keep both of them?
> 
> Yes, but...
> > e.g.
> > compatible: Should be "fsl,imx8qxp-scu-pd" or "fsl,scu-pd"
> 
> Not exactly, "fsl,scu-pd" should be a fallback and the driver can match on that
> (until you need to distinguish). So like this:
> 
> Should be one of:
>   "fsl,imx8qxp-scu-pd"
> followed by "fsl,scu-pd"
> 
> > Can you please help clarify a bit more on why it's better to do that
> > as I'm not quite Understand?
> >
> > And for later when mx8qm is supported, should we add it again as follows?
> > compatible: Should be "fsl,imx8qm-scu-pd", "fsl,imx8qxp-scu-pd" or
> "fsl,scu-pd"
> 
> If formatted as above, then it is a one line change to add:
> 
> Should be one of:
>   "fsl,imx8qm-scu-pd"
>   "fsl,imx8qxp-scu-pd"
> followed by "fsl,scu-pd"
> 

Much clear now.
Thanks for the clarification.

Regards
Dong Aisheng

> Rob

[PATCH V6 1/3] dt-bindings: fsl: scu: make power domain compatible string SoC specific

[A.s. Dong]

> -----Original Message-----
> From: Rob Herring [mailto:robh at kernel.org]
> Sent: Thursday, October 18, 2018 11:39 PM
[...]
> On Thu, Oct 18, 2018 at 10:08 AM A.s. Dong <aisheng.dong@nxp.com> wrote:
> >
> > > -----Original Message-----
> > > From: Rob Herring [mailto:robh at kernel.org]
> > > Sent: Thursday, October 18, 2018 10:34 PM
> > > To: A.s. Dong <aisheng.dong@nxp.com>
> > [...]
> > > On Wed, Oct 17, 2018 at 07:24:40AM +0000, A.s. Dong wrote:
> > > > As the power domain API might change in the future for new SoCs,
> > > > although in a very low possibility, it's still better to make the
> > > > compatible string more SoC specific to avoid the possible version
> > > > change for
> > > new SoCs.
> > > >
> > > > Due to there're still no users in kernel, it's safe to update it
> > > > without breaking anything.
> > > >
> > > > 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
> > > > Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > >
> > > checkpatch says:
> > >
> > > WARNING: Missing Signed-off-by: line by nominal patch author 'A.s.
> > > Dong <aisheng.dong@nxp.com>'
> > >
> >
> > Do you think if we can ignore this warning?
> 
> Probably, but better if you can fix it.
> 
> > It seems checkpatch checks the email sender name as nominal patch
> > author which is abbreviated as 'A.S. Dong'. But I always use the full
> > name in upstreaming patches.
> 
> The patch contents should start with a 'From:' line if your sending email
> doesn't match the patch author (such as when you sent another author's
> patch). Maybe it did match and your email server is overwriting it. I would
> imagine that can be fixed.
> 

Yes, I will try to fix it. Thanks for the suggestion.
As this may be related to our email server, probably may not be able to fixe so quick
for this patch resend. Please let me know if you mind it.

> > > > ---
> > > > ChangeLog:
> > > > v5->v6:
> > > >  * new patch
> > > > ---
> > > >  Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 4
> > > > ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > index 46d0af1..87fc4b4 100644
> > > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > @@ -58,7 +58,7 @@ This binding for the SCU power domain providers
> > > > uses the generic power  domain binding[2].
> > > >
> > > >  Required properties:
> > > > -- compatible:              Should be "fsl,scu-pd".
> > > > +- compatible:              Should be "fsl,imx8qxp-scu-pd".
> > >
> > > You can keep both if you think future SoCs will be compatible.
> > >
> >
> > They're likely to be compatible AFAIK.
> >
> > Just to be clear, you mean keep both of them?
> 
> Yes, but...
> > e.g.
> > compatible: Should be "fsl,imx8qxp-scu-pd" or "fsl,scu-pd"
> 
> Not exactly, "fsl,scu-pd" should be a fallback and the driver can match on that
> (until you need to distinguish). So like this:
> 
> Should be one of:
>   "fsl,imx8qxp-scu-pd"
> followed by "fsl,scu-pd"
> 
> > Can you please help clarify a bit more on why it's better to do that
> > as I'm not quite Understand?
> >
> > And for later when mx8qm is supported, should we add it again as follows?
> > compatible: Should be "fsl,imx8qm-scu-pd", "fsl,imx8qxp-scu-pd" or
> "fsl,scu-pd"
> 
> If formatted as above, then it is a one line change to add:
> 
> Should be one of:
>   "fsl,imx8qm-scu-pd"
>   "fsl,imx8qxp-scu-pd"
> followed by "fsl,scu-pd"
> 

Much clear now.
Thanks for the clarification.

Regards
Dong Aisheng

> Rob