[PATCH V4 0/8] genpd: imx: relocate scu-pd and misc update

[PATCH V4 0/8] genpd: imx: relocate scu-pd and misc update

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

V4:
 Update commit message in patch 4

V3:
 return -EBUSY instead of return 0 in patch 4

V2:
Move drivers/firmware/imx/scu-pd.c to drivers/genpd/imx

This patchset is to upstream NXP downstream scu-pd driver patches.
patch is to relocate scu-pd to genpd
patch 2,3 is to support more PDs
patch 4 is to not power off console when no console suspend
patch 5 is to suppress bind
patch 6 is to make genpd align with HW state
patch 7 is to support LP mode in runtime suspend, OFF mode in system suspend.
patch 8 is to change init level to avoid uneccessary defer probe

V1:
This patchset is to upstream NXP downstream scu-pd driver patches.
patch 1,2 is to support more PDs
patch 3 is to not power off console when no console suspend
patch 4 is to suppress bind
patch 5 is to make genpd align with HW state
patch 6 is to support LP mode in runtime suspend, OFF mode in system suspend.
patch 7 is to change init level to avoid uneccessary defer probe


Dong Aisheng (1):
  genpd: imx: scu-pd: change init level to subsys_initcall

Peng Fan (7):
  genpd: imx: relocate scu-pd under genpd
  genpd: imx: scu-pd: enlarge PD range
  genpd: imx: scu-pd: add more PDs
  genpd: imx: scu-pd: do not power off console if no_console_suspend
  genpd: imx: scu-pd: Suppress bind attrs
  genpd: imx: scu-pd: initialize is_off according to HW state
  genpd: imx: scu-pd: add multi states support

 drivers/firmware/imx/Makefile            |   1 -
 drivers/genpd/imx/Makefile               |   1 +
 drivers/{firmware => genpd}/imx/scu-pd.c | 193 +++++++++++++++++++++--
 3 files changed, 183 insertions(+), 12 deletions(-)
 rename drivers/{firmware => genpd}/imx/scu-pd.c (70%)

-- 
2.37.1

[PATCH V4 1/8] genpd: imx: relocate scu-pd under genpd

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Move scu-pd driver under genpd directory where the driver
should be.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/imx/Makefile            | 1 -
 drivers/genpd/imx/Makefile               | 1 +
 drivers/{firmware => genpd}/imx/scu-pd.c | 0
 3 files changed, 1 insertion(+), 1 deletion(-)
 rename drivers/{firmware => genpd}/imx/scu-pd.c (100%)

diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
index b76acbade2a0..8f9f04a513a8 100644
--- a/drivers/firmware/imx/Makefile
+++ b/drivers/firmware/imx/Makefile
@@ -1,4 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_IMX_DSP)		+= imx-dsp.o
 obj-$(CONFIG_IMX_SCU)		+= imx-scu.o misc.o imx-scu-irq.o rm.o imx-scu-soc.o
-obj-$(CONFIG_IMX_SCU_PD)	+= scu-pd.o
diff --git a/drivers/genpd/imx/Makefile b/drivers/genpd/imx/Makefile
index 5f012717a666..52d2629014a7 100644
--- a/drivers/genpd/imx/Makefile
+++ b/drivers/genpd/imx/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
 obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
+obj-$(CONFIG_IMX_SCU_PD) += scu-pd.o
 obj-$(CONFIG_IMX8M_BLK_CTRL) += imx8m-blk-ctrl.o
 obj-$(CONFIG_IMX8M_BLK_CTRL) += imx8mp-blk-ctrl.o
 obj-$(CONFIG_SOC_IMX9) += imx93-pd.o
diff --git a/drivers/firmware/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
similarity index 100%
rename from drivers/firmware/imx/scu-pd.c
rename to drivers/genpd/imx/scu-pd.c
-- 
2.37.1

[PATCH V4 2/8] genpd: imx: scu-pd: enlarge PD range

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

There are 5 LPI2C, 5 LPUART and 32 DMA0 Channel resources per imx_rsrc.h,
and they are in i.MX8QM, so enlarge the PD range for them.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/genpd/imx/scu-pd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
index 84b673427073..5a28f5af592a 100644
--- a/drivers/genpd/imx/scu-pd.c
+++ b/drivers/genpd/imx/scu-pd.c
@@ -121,9 +121,9 @@ static const struct imx_sc_pd_range imx8qxp_scu_pd_ranges[] = {
 	{ "audio-pll1", IMX_SC_R_AUDIO_PLL_1, 1, false, 0 },
 	{ "audio-clk-0", IMX_SC_R_AUDIO_CLK_0, 1, false, 0 },
 	{ "audio-clk-1", IMX_SC_R_AUDIO_CLK_1, 1, false, 0 },
-	{ "dma0-ch", IMX_SC_R_DMA_0_CH0, 16, true, 0 },
+	{ "dma0-ch", IMX_SC_R_DMA_0_CH0, 32, true, 0 },
 	{ "dma1-ch", IMX_SC_R_DMA_1_CH0, 16, true, 0 },
-	{ "dma2-ch", IMX_SC_R_DMA_2_CH0, 5, true, 0 },
+	{ "dma2-ch", IMX_SC_R_DMA_2_CH0, 32, true, 0 },
 	{ "asrc0", IMX_SC_R_ASRC_0, 1, false, 0 },
 	{ "asrc1", IMX_SC_R_ASRC_1, 1, false, 0 },
 	{ "esai0", IMX_SC_R_ESAI_0, 1, false, 0 },
@@ -143,11 +143,11 @@ static const struct imx_sc_pd_range imx8qxp_scu_pd_ranges[] = {
 	/* DMA SS */
 	{ "can", IMX_SC_R_CAN_0, 3, true, 0 },
 	{ "ftm", IMX_SC_R_FTM_0, 2, true, 0 },
-	{ "lpi2c", IMX_SC_R_I2C_0, 4, true, 0 },
+	{ "lpi2c", IMX_SC_R_I2C_0, 5, true, 0 },
 	{ "adc", IMX_SC_R_ADC_0, 2, true, 0 },
 	{ "lcd", IMX_SC_R_LCD_0, 1, true, 0 },
 	{ "lcd0-pwm", IMX_SC_R_LCD_0_PWM_0, 1, true, 0 },
-	{ "lpuart", IMX_SC_R_UART_0, 4, true, 0 },
+	{ "lpuart", IMX_SC_R_UART_0, 5, true, 0 },
 	{ "lpspi", IMX_SC_R_SPI_0, 4, true, 0 },
 	{ "irqstr_dsp", IMX_SC_R_IRQSTR_DSP, 1, false, 0 },
 
-- 
2.37.1

[PATCH V4 3/8] genpd: imx: scu-pd: add more PDs

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Add more PDs for i.MX8QM and i.MX8DXL, including
dma-ch, esai, gpu1, v2x-mu, seco-mu, hdmi, img and etc.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/genpd/imx/scu-pd.c | 65 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
index 5a28f5af592a..08583a10ac62 100644
--- a/drivers/genpd/imx/scu-pd.c
+++ b/drivers/genpd/imx/scu-pd.c
@@ -121,12 +121,16 @@ static const struct imx_sc_pd_range imx8qxp_scu_pd_ranges[] = {
 	{ "audio-pll1", IMX_SC_R_AUDIO_PLL_1, 1, false, 0 },
 	{ "audio-clk-0", IMX_SC_R_AUDIO_CLK_0, 1, false, 0 },
 	{ "audio-clk-1", IMX_SC_R_AUDIO_CLK_1, 1, false, 0 },
+	{ "mclk-out-0", IMX_SC_R_MCLK_OUT_0, 1, false, 0 },
+	{ "mclk-out-1", IMX_SC_R_MCLK_OUT_1, 1, false, 0 },
 	{ "dma0-ch", IMX_SC_R_DMA_0_CH0, 32, true, 0 },
 	{ "dma1-ch", IMX_SC_R_DMA_1_CH0, 16, true, 0 },
 	{ "dma2-ch", IMX_SC_R_DMA_2_CH0, 32, true, 0 },
+	{ "dma3-ch", IMX_SC_R_DMA_3_CH0, 32, true, 0 },
 	{ "asrc0", IMX_SC_R_ASRC_0, 1, false, 0 },
 	{ "asrc1", IMX_SC_R_ASRC_1, 1, false, 0 },
 	{ "esai0", IMX_SC_R_ESAI_0, 1, false, 0 },
+	{ "esai1", IMX_SC_R_ESAI_1, 1, false, 0 },
 	{ "spdif0", IMX_SC_R_SPDIF_0, 1, false, 0 },
 	{ "spdif1", IMX_SC_R_SPDIF_1, 1, false, 0 },
 	{ "sai", IMX_SC_R_SAI_0, 3, true, 0 },
@@ -146,8 +150,10 @@ static const struct imx_sc_pd_range imx8qxp_scu_pd_ranges[] = {
 	{ "lpi2c", IMX_SC_R_I2C_0, 5, true, 0 },
 	{ "adc", IMX_SC_R_ADC_0, 2, true, 0 },
 	{ "lcd", IMX_SC_R_LCD_0, 1, true, 0 },
+	{ "lcd-pll", IMX_SC_R_ELCDIF_PLL, 1, true, 0 },
 	{ "lcd0-pwm", IMX_SC_R_LCD_0_PWM_0, 1, true, 0 },
 	{ "lpuart", IMX_SC_R_UART_0, 5, true, 0 },
+	{ "sim", IMX_SC_R_EMVSIM_0, 2, true, 0 },
 	{ "lpspi", IMX_SC_R_SPI_0, 4, true, 0 },
 	{ "irqstr_dsp", IMX_SC_R_IRQSTR_DSP, 1, false, 0 },
 
@@ -163,10 +169,15 @@ static const struct imx_sc_pd_range imx8qxp_scu_pd_ranges[] = {
 
 	/* GPU SS */
 	{ "gpu0-pid", IMX_SC_R_GPU_0_PID0, 4, true, 0 },
+	{ "gpu1-pid", IMX_SC_R_GPU_1_PID0, 4, true, 0 },
+
 
 	/* HSIO SS */
+	{ "pcie-a", IMX_SC_R_PCIE_A, 1, false, 0 },
+	{ "serdes-0", IMX_SC_R_SERDES_0, 1, false, 0 },
 	{ "pcie-b", IMX_SC_R_PCIE_B, 1, false, 0 },
 	{ "serdes-1", IMX_SC_R_SERDES_1, 1, false, 0 },
+	{ "sata-0", IMX_SC_R_SATA_0, 1, false, 0 },
 	{ "hsio-gpio", IMX_SC_R_HSIO_GPIO, 1, false, 0 },
 
 	/* MIPI SS */
@@ -186,11 +197,20 @@ static const struct imx_sc_pd_range imx8qxp_scu_pd_ranges[] = {
 	{ "lvds1-pwm", IMX_SC_R_LVDS_1_PWM_0, 1, false, 0 },
 	{ "lvds1-lpi2c", IMX_SC_R_LVDS_1_I2C_0, 2, true, 0 },
 
+	{ "mipi1", IMX_SC_R_MIPI_1, 1, 0 },
+	{ "mipi1-pwm0", IMX_SC_R_MIPI_1_PWM_0, 1, 0 },
+	{ "mipi1-i2c", IMX_SC_R_MIPI_1_I2C_0, 2, 1 },
+	{ "lvds1", IMX_SC_R_LVDS_1, 1, 0 },
+
 	/* DC SS */
 	{ "dc0", IMX_SC_R_DC_0, 1, false, 0 },
 	{ "dc0-pll", IMX_SC_R_DC_0_PLL_0, 2, true, 0 },
 	{ "dc0-video", IMX_SC_R_DC_0_VIDEO0, 2, true, 0 },
 
+	{ "dc1", IMX_SC_R_DC_1, 1, false, 0 },
+	{ "dc1-pll", IMX_SC_R_DC_1_PLL_0, 2, true, 0 },
+	{ "dc1-video", IMX_SC_R_DC_1_VIDEO0, 2, true, 0 },
+
 	/* CM40 SS */
 	{ "cm40-i2c", IMX_SC_R_M4_0_I2C, 1, false, 0 },
 	{ "cm40-intmux", IMX_SC_R_M4_0_INTMUX, 1, false, 0 },
@@ -205,11 +225,56 @@ static const struct imx_sc_pd_range imx8qxp_scu_pd_ranges[] = {
 	{ "cm41-mu-a1", IMX_SC_R_M4_1_MU_1A, 1, false, 0},
 	{ "cm41-lpuart", IMX_SC_R_M4_1_UART, 1, false, 0},
 
+	/* CM41 SS */
+	{ "cm41_i2c", IMX_SC_R_M4_1_I2C, 1, false, 0 },
+	{ "cm41_intmux", IMX_SC_R_M4_1_INTMUX, 1, false, 0 },
+
+	/* DB SS */
+	{ "perf", IMX_SC_R_PERF, 1, false, 0},
+
 	/* IMAGE SS */
 	{ "img-jpegdec-mp", IMX_SC_R_MJPEG_DEC_MP, 1, false, 0 },
 	{ "img-jpegdec-s0", IMX_SC_R_MJPEG_DEC_S0, 4, true, 0 },
 	{ "img-jpegenc-mp", IMX_SC_R_MJPEG_ENC_MP, 1, false, 0 },
 	{ "img-jpegenc-s0", IMX_SC_R_MJPEG_ENC_S0, 4, true, 0 },
+
+	/* SECO SS */
+	{ "seco_mu", IMX_SC_R_SECO_MU_2, 3, true, 2},
+
+	/* V2X SS */
+	{ "v2x_mu", IMX_SC_R_V2X_MU_0, 2, true, 0},
+	{ "v2x_mu", IMX_SC_R_V2X_MU_2, 1, true, 2},
+	{ "v2x_mu", IMX_SC_R_V2X_MU_3, 2, true, 3},
+	{ "img-pdma", IMX_SC_R_ISI_CH0, 8, true, 0 },
+	{ "img-csi0", IMX_SC_R_CSI_0, 1, false, 0 },
+	{ "img-csi0-i2c0", IMX_SC_R_CSI_0_I2C_0, 1, false, 0 },
+	{ "img-csi0-pwm0", IMX_SC_R_CSI_0_PWM_0, 1, false, 0 },
+	{ "img-csi1", IMX_SC_R_CSI_1, 1, false, 0 },
+	{ "img-csi1-i2c0", IMX_SC_R_CSI_1_I2C_0, 1, false, 0 },
+	{ "img-csi1-pwm0", IMX_SC_R_CSI_1_PWM_0, 1, false, 0 },
+	{ "img-parallel", IMX_SC_R_PI_0, 1, false, 0 },
+	{ "img-parallel-i2c0", IMX_SC_R_PI_0_I2C_0, 1, false, 0 },
+	{ "img-parallel-pwm0", IMX_SC_R_PI_0_PWM_0, 2, true, 0 },
+	{ "img-parallel-pll", IMX_SC_R_PI_0_PLL, 1, false, 0 },
+
+	/* HDMI TX SS */
+	{ "hdmi-tx", IMX_SC_R_HDMI, 1, false, 0},
+	{ "hdmi-tx-i2s", IMX_SC_R_HDMI_I2S, 1, false, 0},
+	{ "hdmi-tx-i2c0", IMX_SC_R_HDMI_I2C_0, 1, false, 0},
+	{ "hdmi-tx-pll0", IMX_SC_R_HDMI_PLL_0, 1, false, 0},
+	{ "hdmi-tx-pll1", IMX_SC_R_HDMI_PLL_1, 1, false, 0},
+
+	/* HDMI RX SS */
+	{ "hdmi-rx", IMX_SC_R_HDMI_RX, 1, false, 0},
+	{ "hdmi-rx-pwm", IMX_SC_R_HDMI_RX_PWM_0, 1, false, 0},
+	{ "hdmi-rx-i2c0", IMX_SC_R_HDMI_RX_I2C_0, 1, false, 0},
+	{ "hdmi-rx-bypass", IMX_SC_R_HDMI_RX_BYPASS, 1, false, 0},
+
+	/* SECURITY SS */
+	{ "sec-jr", IMX_SC_R_CAAM_JR2, 2, true, 2},
+
+	/* BOARD SS */
+	{ "board", IMX_SC_R_BOARD_R0, 8, true, 0},
 };
 
 static const struct imx_sc_pd_soc imx8qxp_scu_pd = {
-- 
2.37.1

[PATCH V4 4/8] genpd: imx: scu-pd: do not power off console if no_console_suspend

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Do not power off console if no_console_suspend, this will leave
the serial device's corresponding PM domain on during system suspend,
which is useful for debug system suspend.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/genpd/imx/scu-pd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
index 08583a10ac62..d69da79d3130 100644
--- a/drivers/genpd/imx/scu-pd.c
+++ b/drivers/genpd/imx/scu-pd.c
@@ -52,6 +52,7 @@
  */
 
 #include <dt-bindings/firmware/imx/rsrc.h>
+#include <linux/console.h>
 #include <linux/firmware/imx/sci.h>
 #include <linux/firmware/imx/svc/rm.h>
 #include <linux/io.h>
@@ -324,6 +325,10 @@ static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on)
 	msg.resource = pd->rsrc;
 	msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : IMX_SC_PM_PW_MODE_LP;
 
+	/* keep uart console power on for no_console_suspend */
+	if (imx_con_rsrc == pd->rsrc && !console_suspend_enabled && !power_on)
+		return -EBUSY;
+
 	ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
 	if (ret)
 		dev_err(&domain->dev, "failed to power %s resource %d ret %d\n",
-- 
2.37.1

[PATCH V4 5/8] genpd: imx: scu-pd: Suppress bind attrs

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

This driver is registered as platform driver, but removing and binding
again would cause system not workable. So suppress bind attrs.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/genpd/imx/scu-pd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
index d69da79d3130..fa840ebe38c5 100644
--- a/drivers/genpd/imx/scu-pd.c
+++ b/drivers/genpd/imx/scu-pd.c
@@ -488,6 +488,7 @@ static struct platform_driver imx_sc_pd_driver = {
 	.driver = {
 		.name = "imx-scu-pd",
 		.of_match_table = imx_sc_pd_match,
+		.suppress_bind_attrs = true,
 	},
 	.probe = imx_sc_pd_probe,
 };
-- 
2.37.1

[PATCH V4 6/8] genpd: imx: scu-pd: initialize is_off according to HW state

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

The current code default set is_off to true except console resource,
this implies bootloader should power off all the resources it uses.

But this is not always true, let's check the HW state and set is_off.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/genpd/imx/scu-pd.c | 59 +++++++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 4 deletions(-)

diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
index fa840ebe38c5..2f693b67ddb4 100644
--- a/drivers/genpd/imx/scu-pd.c
+++ b/drivers/genpd/imx/scu-pd.c
@@ -72,6 +72,22 @@ struct imx_sc_msg_req_set_resource_power_mode {
 	u8 mode;
 } __packed __aligned(4);
 
+struct req_get_resource_mode {
+	u16 resource;
+};
+
+struct resp_get_resource_mode {
+	u8 mode;
+};
+
+struct imx_sc_msg_req_get_resource_power_mode {
+	struct imx_sc_rpc_msg hdr;
+	union {
+		struct req_get_resource_mode req;
+		struct resp_get_resource_mode resp;
+	} data;
+} __packed __aligned(4);
+
 #define IMX_SCU_PD_NAME_SIZE 20
 struct imx_sc_pm_domain {
 	struct generic_pm_domain pd;
@@ -96,6 +112,14 @@ struct imx_sc_pd_soc {
 
 static int imx_con_rsrc;
 
+/* Align with the IMX_SC_PM_PW_MODE_[OFF,STBY,LP,ON] macros */
+static const char * const imx_sc_pm_mode[] = {
+	"IMX_SC_PM_PW_MODE_OFF",
+	"IMX_SC_PM_PW_MODE_STBY",
+	"IMX_SC_PM_PW_MODE_LP",
+	"IMX_SC_PM_PW_MODE_ON"
+};
+
 static const struct imx_sc_pd_range imx8qxp_scu_pd_ranges[] = {
 	/* LSIO SS */
 	{ "pwm", IMX_SC_R_PWM_0, 8, true, 0 },
@@ -308,6 +332,27 @@ static void imx_sc_pd_get_console_rsrc(void)
 	imx_con_rsrc = specs.args[0];
 }
 
+static int imx_sc_get_pd_power(struct device *dev, u32 rsrc)
+{
+	struct imx_sc_msg_req_get_resource_power_mode msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	int ret;
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = IMX_SC_RPC_SVC_PM;
+	hdr->func = IMX_SC_PM_FUNC_GET_RESOURCE_POWER_MODE;
+	hdr->size = 2;
+
+	msg.data.req.resource = rsrc;
+
+	ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
+	if (ret)
+		dev_err(dev, "failed to get power resource %d mode, ret %d\n",
+			rsrc, ret);
+
+	return msg.data.resp.mode;
+}
+
 static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on)
 {
 	struct imx_sc_msg_req_set_resource_power_mode msg;
@@ -372,8 +417,8 @@ imx_scu_add_pm_domain(struct device *dev, int idx,
 		      const struct imx_sc_pd_range *pd_ranges)
 {
 	struct imx_sc_pm_domain *sc_pd;
-	bool is_off = true;
-	int ret;
+	bool is_off;
+	int mode, ret;
 
 	if (!imx_sc_rm_is_resource_owned(pm_ipc_handle, pd_ranges->rsrc + idx))
 		return NULL;
@@ -394,10 +439,16 @@ imx_scu_add_pm_domain(struct device *dev, int idx,
 			 "%s", pd_ranges->name);
 
 	sc_pd->pd.name = sc_pd->name;
-	if (imx_con_rsrc == sc_pd->rsrc) {
+	if (imx_con_rsrc == sc_pd->rsrc)
 		sc_pd->pd.flags = GENPD_FLAG_RPM_ALWAYS_ON;
+
+	mode = imx_sc_get_pd_power(dev, pd_ranges->rsrc + idx);
+	if (mode == IMX_SC_PM_PW_MODE_ON)
 		is_off = false;
-	}
+	else
+		is_off = true;
+
+	dev_dbg(dev, "%s : %s\n", sc_pd->name, imx_sc_pm_mode[mode]);
 
 	if (sc_pd->rsrc >= IMX_SC_R_LAST) {
 		dev_warn(dev, "invalid pd %s rsrc id %d found",
-- 
2.37.1

[PATCH V4 7/8] genpd: imx: scu-pd: add multi states support

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Add multi states support, this is to support devices could run in LP mode
when runtime suspend, and OFF mode when system suspend.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/genpd/imx/scu-pd.c | 48 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
index 2f693b67ddb4..30da101119eb 100644
--- a/drivers/genpd/imx/scu-pd.c
+++ b/drivers/genpd/imx/scu-pd.c
@@ -65,6 +65,12 @@
 #include <linux/pm_domain.h>
 #include <linux/slab.h>
 
+enum {
+	PD_STATE_LP,
+	PD_STATE_OFF,
+	PD_STATE_MAX
+};
+
 /* SCU Power Mode Protocol definition */
 struct imx_sc_msg_req_set_resource_power_mode {
 	struct imx_sc_rpc_msg hdr;
@@ -368,7 +374,8 @@ static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on)
 	hdr->size = 2;
 
 	msg.resource = pd->rsrc;
-	msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : IMX_SC_PM_PW_MODE_LP;
+	msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : pd->pd.state_idx ?
+		   IMX_SC_PM_PW_MODE_OFF : IMX_SC_PM_PW_MODE_LP;
 
 	/* keep uart console power on for no_console_suspend */
 	if (imx_con_rsrc == pd->rsrc && !console_suspend_enabled && !power_on)
@@ -412,11 +419,33 @@ static struct generic_pm_domain *imx_scu_pd_xlate(struct of_phandle_args *spec,
 	return domain;
 }
 
+static bool imx_sc_pd_suspend_ok(struct device *dev)
+{
+	/* Always true */
+	return true;
+}
+
+static bool imx_sc_pd_power_down_ok(struct dev_pm_domain *pd)
+{
+	struct generic_pm_domain *genpd = pd_to_genpd(pd);
+
+	/* For runtime suspend, choose LP mode */
+	genpd->state_idx = 0;
+
+	return true;
+}
+
+struct dev_power_governor imx_sc_pd_qos_governor = {
+	.suspend_ok = imx_sc_pd_suspend_ok,
+	.power_down_ok = imx_sc_pd_power_down_ok,
+};
+
 static struct imx_sc_pm_domain *
 imx_scu_add_pm_domain(struct device *dev, int idx,
 		      const struct imx_sc_pd_range *pd_ranges)
 {
 	struct imx_sc_pm_domain *sc_pd;
+	struct genpd_power_state *states;
 	bool is_off;
 	int mode, ret;
 
@@ -427,9 +456,22 @@ imx_scu_add_pm_domain(struct device *dev, int idx,
 	if (!sc_pd)
 		return ERR_PTR(-ENOMEM);
 
+	states = devm_kcalloc(dev, PD_STATE_MAX, sizeof(*states), GFP_KERNEL);
+	if (!states) {
+		devm_kfree(dev, sc_pd);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	sc_pd->rsrc = pd_ranges->rsrc + idx;
 	sc_pd->pd.power_off = imx_sc_pd_power_off;
 	sc_pd->pd.power_on = imx_sc_pd_power_on;
+	states[PD_STATE_LP].power_off_latency_ns = 25000;
+	states[PD_STATE_LP].power_on_latency_ns =  25000;
+	states[PD_STATE_OFF].power_off_latency_ns = 2500000;
+	states[PD_STATE_OFF].power_on_latency_ns =  2500000;
+
+	sc_pd->pd.states = states;
+	sc_pd->pd.state_count = PD_STATE_MAX;
 
 	if (pd_ranges->postfix)
 		snprintf(sc_pd->name, sizeof(sc_pd->name),
@@ -455,14 +497,16 @@ imx_scu_add_pm_domain(struct device *dev, int idx,
 			 sc_pd->name, sc_pd->rsrc);
 
 		devm_kfree(dev, sc_pd);
+		devm_kfree(dev, states);
 		return NULL;
 	}
 
-	ret = pm_genpd_init(&sc_pd->pd, NULL, is_off);
+	ret = pm_genpd_init(&sc_pd->pd, &imx_sc_pd_qos_governor, is_off);
 	if (ret) {
 		dev_warn(dev, "failed to init pd %s rsrc id %d",
 			 sc_pd->name, sc_pd->rsrc);
 		devm_kfree(dev, sc_pd);
+		devm_kfree(dev, states);
 		return NULL;
 	}
 
-- 
2.37.1

[PATCH V4 8/8] genpd: imx: scu-pd: change init level to subsys_initcall

[Peng Fan (OSS)]

From: Dong Aisheng <aisheng.dong@nxp.com>

Change power domain init level to subsys_initcall to ensure it's probed
before most devices to avoid unnecessary defer probe.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/genpd/imx/scu-pd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
index 30da101119eb..0cda0999a1f2 100644
--- a/drivers/genpd/imx/scu-pd.c
+++ b/drivers/genpd/imx/scu-pd.c
@@ -587,7 +587,12 @@ static struct platform_driver imx_sc_pd_driver = {
 	},
 	.probe = imx_sc_pd_probe,
 };
-builtin_platform_driver(imx_sc_pd_driver);
+
+static int __init imx_sc_pd_driver_init(void)
+{
+	return platform_driver_register(&imx_sc_pd_driver);
+}
+subsys_initcall(imx_sc_pd_driver_init);
 
 MODULE_AUTHOR("Dong Aisheng <aisheng.dong@nxp.com>");
 MODULE_DESCRIPTION("IMX SCU Power Domain driver");
-- 
2.37.1

Re: [PATCH V4 7/8] genpd: imx: scu-pd: add multi states support

[Ulf Hansson]

On Mon, 14 Aug 2023 at 12:37, Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> Add multi states support, this is to support devices could run in LP mode
> when runtime suspend, and OFF mode when system suspend.

For my understanding, is there a functional problem to support OFF at
runtime suspend too?

>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/genpd/imx/scu-pd.c | 48 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
> index 2f693b67ddb4..30da101119eb 100644
> --- a/drivers/genpd/imx/scu-pd.c
> +++ b/drivers/genpd/imx/scu-pd.c
> @@ -65,6 +65,12 @@
>  #include <linux/pm_domain.h>
>  #include <linux/slab.h>
>
> +enum {
> +       PD_STATE_LP,
> +       PD_STATE_OFF,
> +       PD_STATE_MAX
> +};
> +
>  /* SCU Power Mode Protocol definition */
>  struct imx_sc_msg_req_set_resource_power_mode {
>         struct imx_sc_rpc_msg hdr;
> @@ -368,7 +374,8 @@ static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on)
>         hdr->size = 2;
>
>         msg.resource = pd->rsrc;
> -       msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : IMX_SC_PM_PW_MODE_LP;
> +       msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : pd->pd.state_idx ?
> +                  IMX_SC_PM_PW_MODE_OFF : IMX_SC_PM_PW_MODE_LP;
>
>         /* keep uart console power on for no_console_suspend */
>         if (imx_con_rsrc == pd->rsrc && !console_suspend_enabled && !power_on)
> @@ -412,11 +419,33 @@ static struct generic_pm_domain *imx_scu_pd_xlate(struct of_phandle_args *spec,
>         return domain;
>  }
>
> +static bool imx_sc_pd_suspend_ok(struct device *dev)
> +{
> +       /* Always true */
> +       return true;
> +}
> +
> +static bool imx_sc_pd_power_down_ok(struct dev_pm_domain *pd)
> +{
> +       struct generic_pm_domain *genpd = pd_to_genpd(pd);
> +
> +       /* For runtime suspend, choose LP mode */
> +       genpd->state_idx = 0;
> +
> +       return true;
> +}

I am wondering if we couldn't use the simple_qos_governor here
instead. In principle it looks like we want a QoS latency constraint
to be set during runtime, to prevent the OFF state.

During system wide suspend, the deepest state is always selected by genpd.

> +
> +struct dev_power_governor imx_sc_pd_qos_governor = {
> +       .suspend_ok = imx_sc_pd_suspend_ok,
> +       .power_down_ok = imx_sc_pd_power_down_ok,
> +};
> +
>  static struct imx_sc_pm_domain *
>  imx_scu_add_pm_domain(struct device *dev, int idx,
>                       const struct imx_sc_pd_range *pd_ranges)
>  {
>         struct imx_sc_pm_domain *sc_pd;
> +       struct genpd_power_state *states;
>         bool is_off;
>         int mode, ret;
>
> @@ -427,9 +456,22 @@ imx_scu_add_pm_domain(struct device *dev, int idx,
>         if (!sc_pd)
>                 return ERR_PTR(-ENOMEM);
>
> +       states = devm_kcalloc(dev, PD_STATE_MAX, sizeof(*states), GFP_KERNEL);
> +       if (!states) {
> +               devm_kfree(dev, sc_pd);
> +               return ERR_PTR(-ENOMEM);
> +       }
> +
>         sc_pd->rsrc = pd_ranges->rsrc + idx;
>         sc_pd->pd.power_off = imx_sc_pd_power_off;
>         sc_pd->pd.power_on = imx_sc_pd_power_on;
> +       states[PD_STATE_LP].power_off_latency_ns = 25000;
> +       states[PD_STATE_LP].power_on_latency_ns =  25000;
> +       states[PD_STATE_OFF].power_off_latency_ns = 2500000;
> +       states[PD_STATE_OFF].power_on_latency_ns =  2500000;

We should probably describe these in DT instead? The
domain-idle-states bindings allows us to do this. See
Documentation/devicetree/bindings/power/domain-idle-state.yaml.

Then we have of_genpd_parse_idle_states(), a helper that parses the values.

> +
> +       sc_pd->pd.states = states;
> +       sc_pd->pd.state_count = PD_STATE_MAX;
>
>         if (pd_ranges->postfix)
>                 snprintf(sc_pd->name, sizeof(sc_pd->name),
> @@ -455,14 +497,16 @@ imx_scu_add_pm_domain(struct device *dev, int idx,
>                          sc_pd->name, sc_pd->rsrc);
>
>                 devm_kfree(dev, sc_pd);
> +               devm_kfree(dev, states);
>                 return NULL;
>         }
>
> -       ret = pm_genpd_init(&sc_pd->pd, NULL, is_off);
> +       ret = pm_genpd_init(&sc_pd->pd, &imx_sc_pd_qos_governor, is_off);
>         if (ret) {
>                 dev_warn(dev, "failed to init pd %s rsrc id %d",
>                          sc_pd->name, sc_pd->rsrc);
>                 devm_kfree(dev, sc_pd);
> +               devm_kfree(dev, states);
>                 return NULL;
>         }
>
> --
> 2.37.1
>

Kind regards
Uffe

Re: [PATCH V4 0/8] genpd: imx: relocate scu-pd and misc update

[Ulf Hansson]

On Mon, 14 Aug 2023 at 12:36, Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> V4:
>  Update commit message in patch 4
>
> V3:
>  return -EBUSY instead of return 0 in patch 4
>
> V2:
> Move drivers/firmware/imx/scu-pd.c to drivers/genpd/imx
>
> This patchset is to upstream NXP downstream scu-pd driver patches.
> patch is to relocate scu-pd to genpd
> patch 2,3 is to support more PDs
> patch 4 is to not power off console when no console suspend
> patch 5 is to suppress bind
> patch 6 is to make genpd align with HW state
> patch 7 is to support LP mode in runtime suspend, OFF mode in system suspend.
> patch 8 is to change init level to avoid uneccessary defer probe
>
> V1:
> This patchset is to upstream NXP downstream scu-pd driver patches.
> patch 1,2 is to support more PDs
> patch 3 is to not power off console when no console suspend
> patch 4 is to suppress bind
> patch 5 is to make genpd align with HW state
> patch 6 is to support LP mode in runtime suspend, OFF mode in system suspend.
> patch 7 is to change init level to avoid uneccessary defer probe
>
>
> Dong Aisheng (1):
>   genpd: imx: scu-pd: change init level to subsys_initcall
>
> Peng Fan (7):
>   genpd: imx: relocate scu-pd under genpd
>   genpd: imx: scu-pd: enlarge PD range
>   genpd: imx: scu-pd: add more PDs
>   genpd: imx: scu-pd: do not power off console if no_console_suspend
>   genpd: imx: scu-pd: Suppress bind attrs
>   genpd: imx: scu-pd: initialize is_off according to HW state
>   genpd: imx: scu-pd: add multi states support
>
>  drivers/firmware/imx/Makefile            |   1 -
>  drivers/genpd/imx/Makefile               |   1 +
>  drivers/{firmware => genpd}/imx/scu-pd.c | 193 +++++++++++++++++++++--
>  3 files changed, 183 insertions(+), 12 deletions(-)
>  rename drivers/{firmware => genpd}/imx/scu-pd.c (70%)
>

I am fine to pick up patch 1 -> patch 6, to let them go in for v6.6.
Should we do that or defer until the complete series is ready?

Kind regards
Uffe

RE: [PATCH V4 0/8] genpd: imx: relocate scu-pd and misc update

[Peng Fan]

> Subject: Re: [PATCH V4 0/8] genpd: imx: relocate scu-pd and misc update
> 
> On Mon, 14 Aug 2023 at 12:36, Peng Fan (OSS) <peng.fan@oss.nxp.com>
> wrote:
> >
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > V4:
> >  Update commit message in patch 4
> >
> > V3:
> >  return -EBUSY instead of return 0 in patch 4
> >
> > V2:
> > Move drivers/firmware/imx/scu-pd.c to drivers/genpd/imx
> >
> > This patchset is to upstream NXP downstream scu-pd driver patches.
> > patch is to relocate scu-pd to genpd
> > patch 2,3 is to support more PDs
> > patch 4 is to not power off console when no console suspend patch 5 is
> > to suppress bind patch 6 is to make genpd align with HW state patch 7
> > is to support LP mode in runtime suspend, OFF mode in system suspend.
> > patch 8 is to change init level to avoid uneccessary defer probe
> >
> > V1:
> > This patchset is to upstream NXP downstream scu-pd driver patches.
> > patch 1,2 is to support more PDs
> > patch 3 is to not power off console when no console suspend patch 4 is
> > to suppress bind patch 5 is to make genpd align with HW state patch 6
> > is to support LP mode in runtime suspend, OFF mode in system suspend.
> > patch 7 is to change init level to avoid uneccessary defer probe
> >
> >
> > Dong Aisheng (1):
> >   genpd: imx: scu-pd: change init level to subsys_initcall
> >
> > Peng Fan (7):
> >   genpd: imx: relocate scu-pd under genpd
> >   genpd: imx: scu-pd: enlarge PD range
> >   genpd: imx: scu-pd: add more PDs
> >   genpd: imx: scu-pd: do not power off console if no_console_suspend
> >   genpd: imx: scu-pd: Suppress bind attrs
> >   genpd: imx: scu-pd: initialize is_off according to HW state
> >   genpd: imx: scu-pd: add multi states support
> >
> >  drivers/firmware/imx/Makefile            |   1 -
> >  drivers/genpd/imx/Makefile               |   1 +
> >  drivers/{firmware => genpd}/imx/scu-pd.c | 193
> > +++++++++++++++++++++--
> >  3 files changed, 183 insertions(+), 12 deletions(-)  rename
> > drivers/{firmware => genpd}/imx/scu-pd.c (70%)
> >
> 
> I am fine to pick up patch 1 -> patch 6, to let them go in for v6.6.
> Should we do that or defer until the complete series is ready?

Please take patch 1-6 first. I could handle patch 7 issue in a separate
patch, since patch 7 is orthogonal to other patches.

Thanks,
Peng.

> 
> Kind regards
> Uffe

RE: [PATCH V4 7/8] genpd: imx: scu-pd: add multi states support

[Peng Fan]

> Subject: Re: [PATCH V4 7/8] genpd: imx: scu-pd: add multi states support
> 
> On Mon, 14 Aug 2023 at 12:37, Peng Fan (OSS) <peng.fan@oss.nxp.com>
> wrote:
> >
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add multi states support, this is to support devices could run in LP
> > mode when runtime suspend, and OFF mode when system suspend.
> 
> For my understanding, is there a functional problem to support OFF at
> runtime suspend too?

In OFF mode, the HW state is lost, so the clks that exported by this(Subsystem)
genpd is lost. While in LF mode, no need handle clks recover.


Such as subsystem LSIO has clks output, has GPIO, has LPUART.

The clks are in drivers/clk/imx/clk-imx8qxp*, which relies on the scu pd.

If scu-pd is off, the clks will lose state.

> 
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/genpd/imx/scu-pd.c | 48
> > ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
> > index 2f693b67ddb4..30da101119eb 100644
> > --- a/drivers/genpd/imx/scu-pd.c
> > +++ b/drivers/genpd/imx/scu-pd.c
> > @@ -65,6 +65,12 @@
> >  #include <linux/pm_domain.h>
> >  #include <linux/slab.h>
> >
> > +enum {
> > +       PD_STATE_LP,
> > +       PD_STATE_OFF,
> > +       PD_STATE_MAX
> > +};
> > +
> >  /* SCU Power Mode Protocol definition */  struct
> > imx_sc_msg_req_set_resource_power_mode {
> >         struct imx_sc_rpc_msg hdr;
> > @@ -368,7 +374,8 @@ static int imx_sc_pd_power(struct
> generic_pm_domain *domain, bool power_on)
> >         hdr->size = 2;
> >
> >         msg.resource = pd->rsrc;
> > -       msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON :
> IMX_SC_PM_PW_MODE_LP;
> > +       msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : pd-
> >pd.state_idx ?
> > +                  IMX_SC_PM_PW_MODE_OFF : IMX_SC_PM_PW_MODE_LP;
> >
> >         /* keep uart console power on for no_console_suspend */
> >         if (imx_con_rsrc == pd->rsrc && !console_suspend_enabled &&
> > !power_on) @@ -412,11 +419,33 @@ static struct generic_pm_domain
> *imx_scu_pd_xlate(struct of_phandle_args *spec,
> >         return domain;
> >  }
> >
> > +static bool imx_sc_pd_suspend_ok(struct device *dev) {
> > +       /* Always true */
> > +       return true;
> > +}
> > +
> > +static bool imx_sc_pd_power_down_ok(struct dev_pm_domain *pd) {
> > +       struct generic_pm_domain *genpd = pd_to_genpd(pd);
> > +
> > +       /* For runtime suspend, choose LP mode */
> > +       genpd->state_idx = 0;
> > +
> > +       return true;
> > +}
> 
> I am wondering if we couldn't use the simple_qos_governor here instead. In
> principle it looks like we want a QoS latency constraint to be set during
> runtime, to prevent the OFF state.

LP mode indeed could save resume time, but the major problem is to avoid
save/restore clks.
> 
> During system wide suspend, the deepest state is always selected by genpd.
> 
> > +
> > +struct dev_power_governor imx_sc_pd_qos_governor = {
> > +       .suspend_ok = imx_sc_pd_suspend_ok,
> > +       .power_down_ok = imx_sc_pd_power_down_ok, };
> > +
> >  static struct imx_sc_pm_domain *
> >  imx_scu_add_pm_domain(struct device *dev, int idx,
> >                       const struct imx_sc_pd_range *pd_ranges)  {
> >         struct imx_sc_pm_domain *sc_pd;
> > +       struct genpd_power_state *states;
> >         bool is_off;
> >         int mode, ret;
> >
> > @@ -427,9 +456,22 @@ imx_scu_add_pm_domain(struct device *dev, int
> idx,
> >         if (!sc_pd)
> >                 return ERR_PTR(-ENOMEM);
> >
> > +       states = devm_kcalloc(dev, PD_STATE_MAX, sizeof(*states),
> GFP_KERNEL);
> > +       if (!states) {
> > +               devm_kfree(dev, sc_pd);
> > +               return ERR_PTR(-ENOMEM);
> > +       }
> > +
> >         sc_pd->rsrc = pd_ranges->rsrc + idx;
> >         sc_pd->pd.power_off = imx_sc_pd_power_off;
> >         sc_pd->pd.power_on = imx_sc_pd_power_on;
> > +       states[PD_STATE_LP].power_off_latency_ns = 25000;
> > +       states[PD_STATE_LP].power_on_latency_ns =  25000;
> > +       states[PD_STATE_OFF].power_off_latency_ns = 2500000;
> > +       states[PD_STATE_OFF].power_on_latency_ns =  2500000;
> 
> We should probably describe these in DT instead? The domain-idle-states
> bindings allows us to do this. See
> Documentation/devicetree/bindings/power/domain-idle-state.yaml.

The scu-pd is a firmware function node, there is no sub-genpd node inside it.

Just like scmi pd, there is no sub-genpd in it.


Thanks,
Peng.

> 
> Then we have of_genpd_parse_idle_states(), a helper that parses the values.
> 
> > +
> > +       sc_pd->pd.states = states;
> > +       sc_pd->pd.state_count = PD_STATE_MAX;
> >
> >         if (pd_ranges->postfix)
> >                 snprintf(sc_pd->name, sizeof(sc_pd->name), @@ -455,14
> > +497,16 @@ imx_scu_add_pm_domain(struct device *dev, int idx,
> >                          sc_pd->name, sc_pd->rsrc);
> >
> >                 devm_kfree(dev, sc_pd);
> > +               devm_kfree(dev, states);
> >                 return NULL;
> >         }
> >
> > -       ret = pm_genpd_init(&sc_pd->pd, NULL, is_off);
> > +       ret = pm_genpd_init(&sc_pd->pd, &imx_sc_pd_qos_governor,
> > + is_off);
> >         if (ret) {
> >                 dev_warn(dev, "failed to init pd %s rsrc id %d",
> >                          sc_pd->name, sc_pd->rsrc);
> >                 devm_kfree(dev, sc_pd);
> > +               devm_kfree(dev, states);
> >                 return NULL;
> >         }
> >
> > --
> > 2.37.1
> >
> 
> Kind regards
> Uffe

Re: [PATCH V4 7/8] genpd: imx: scu-pd: add multi states support

[Ulf Hansson]

On Mon, 14 Aug 2023 at 13:52, Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: Re: [PATCH V4 7/8] genpd: imx: scu-pd: add multi states support
> >
> > On Mon, 14 Aug 2023 at 12:37, Peng Fan (OSS) <peng.fan@oss.nxp.com>
> > wrote:
> > >
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > Add multi states support, this is to support devices could run in LP
> > > mode when runtime suspend, and OFF mode when system suspend.
> >
> > For my understanding, is there a functional problem to support OFF at
> > runtime suspend too?
>
> In OFF mode, the HW state is lost, so the clks that exported by this(Subsystem)
> genpd is lost. While in LF mode, no need handle clks recover.
>
>
> Such as subsystem LSIO has clks output, has GPIO, has LPUART.
>
> The clks are in drivers/clk/imx/clk-imx8qxp*, which relies on the scu pd.
>
> If scu-pd is off, the clks will lose state.

Thanks for clarifying, much appreciated! So it sounds like it's the
clock provider(s) that has these requirements then. Can we let the
clock provider set a QoS latency constraint for its device that is
attached to the genpd then? To prevent the deeper OFF state?

Another option would be to enable runtime PM support for the clock
provider (to manage the save/restore from runtime PM callbacks), but
whether that's feasible sounds like a separate discussion.

>
> >
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/genpd/imx/scu-pd.c | 48
> > > ++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 46 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
> > > index 2f693b67ddb4..30da101119eb 100644
> > > --- a/drivers/genpd/imx/scu-pd.c
> > > +++ b/drivers/genpd/imx/scu-pd.c
> > > @@ -65,6 +65,12 @@
> > >  #include <linux/pm_domain.h>
> > >  #include <linux/slab.h>
> > >
> > > +enum {
> > > +       PD_STATE_LP,
> > > +       PD_STATE_OFF,
> > > +       PD_STATE_MAX
> > > +};
> > > +
> > >  /* SCU Power Mode Protocol definition */  struct
> > > imx_sc_msg_req_set_resource_power_mode {
> > >         struct imx_sc_rpc_msg hdr;
> > > @@ -368,7 +374,8 @@ static int imx_sc_pd_power(struct
> > generic_pm_domain *domain, bool power_on)
> > >         hdr->size = 2;
> > >
> > >         msg.resource = pd->rsrc;
> > > -       msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON :
> > IMX_SC_PM_PW_MODE_LP;
> > > +       msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : pd-
> > >pd.state_idx ?
> > > +                  IMX_SC_PM_PW_MODE_OFF : IMX_SC_PM_PW_MODE_LP;
> > >
> > >         /* keep uart console power on for no_console_suspend */
> > >         if (imx_con_rsrc == pd->rsrc && !console_suspend_enabled &&
> > > !power_on) @@ -412,11 +419,33 @@ static struct generic_pm_domain
> > *imx_scu_pd_xlate(struct of_phandle_args *spec,
> > >         return domain;
> > >  }
> > >
> > > +static bool imx_sc_pd_suspend_ok(struct device *dev) {
> > > +       /* Always true */
> > > +       return true;
> > > +}
> > > +
> > > +static bool imx_sc_pd_power_down_ok(struct dev_pm_domain *pd) {
> > > +       struct generic_pm_domain *genpd = pd_to_genpd(pd);
> > > +
> > > +       /* For runtime suspend, choose LP mode */
> > > +       genpd->state_idx = 0;
> > > +
> > > +       return true;
> > > +}
> >
> > I am wondering if we couldn't use the simple_qos_governor here instead. In
> > principle it looks like we want a QoS latency constraint to be set during
> > runtime, to prevent the OFF state.
>
> LP mode indeed could save resume time, but the major problem is to avoid
> save/restore clks.

Okay. So it still sounds like a QoS latency constraint (for the clock
provider) sounds like the correct thing to do.

If/when the clock provider gets runtime PM support, we can remove the
QoS latency constraints. That should work, right?

> >
> > During system wide suspend, the deepest state is always selected by genpd.
> >
> > > +
> > > +struct dev_power_governor imx_sc_pd_qos_governor = {
> > > +       .suspend_ok = imx_sc_pd_suspend_ok,
> > > +       .power_down_ok = imx_sc_pd_power_down_ok, };
> > > +
> > >  static struct imx_sc_pm_domain *
> > >  imx_scu_add_pm_domain(struct device *dev, int idx,
> > >                       const struct imx_sc_pd_range *pd_ranges)  {
> > >         struct imx_sc_pm_domain *sc_pd;
> > > +       struct genpd_power_state *states;
> > >         bool is_off;
> > >         int mode, ret;
> > >
> > > @@ -427,9 +456,22 @@ imx_scu_add_pm_domain(struct device *dev, int
> > idx,
> > >         if (!sc_pd)
> > >                 return ERR_PTR(-ENOMEM);
> > >
> > > +       states = devm_kcalloc(dev, PD_STATE_MAX, sizeof(*states),
> > GFP_KERNEL);
> > > +       if (!states) {
> > > +               devm_kfree(dev, sc_pd);
> > > +               return ERR_PTR(-ENOMEM);
> > > +       }
> > > +
> > >         sc_pd->rsrc = pd_ranges->rsrc + idx;
> > >         sc_pd->pd.power_off = imx_sc_pd_power_off;
> > >         sc_pd->pd.power_on = imx_sc_pd_power_on;
> > > +       states[PD_STATE_LP].power_off_latency_ns = 25000;
> > > +       states[PD_STATE_LP].power_on_latency_ns =  25000;
> > > +       states[PD_STATE_OFF].power_off_latency_ns = 2500000;
> > > +       states[PD_STATE_OFF].power_on_latency_ns =  2500000;
> >
> > We should probably describe these in DT instead? The domain-idle-states
> > bindings allows us to do this. See
> > Documentation/devicetree/bindings/power/domain-idle-state.yaml.
>
> The scu-pd is a firmware function node, there is no sub-genpd node inside it.
>
> Just like scmi pd, there is no sub-genpd in it.

Not sure I got your point. We don't need a sub-genpd node to describe
this. This is how it could look like:

domain-idle-states {
    domain_retention: domain-retention {
        compatible = "domain-idle-state";
        entry-latency-us = <25>;
        exit-latency-us = <25>;
    };
    domain_off: domain-off {
        compatible = "domain-idle-state";
        entry-latency-us = <2500>;
        exit-latency-us = <2500>;
    };
};

power-controller {
    compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd";
    #power-domain-cells = <1>;
    domain-idle-states = <&domain_retention>, <&domain_off>;
};

[...]

Kind regards
Uffe

Re: [PATCH V4 7/8] genpd: imx: scu-pd: add multi states support

[Ulf Hansson]

On Mon, 14 Aug 2023 at 14:23, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 14 Aug 2023 at 13:52, Peng Fan <peng.fan@nxp.com> wrote:
> >
> > > Subject: Re: [PATCH V4 7/8] genpd: imx: scu-pd: add multi states support
> > >
> > > On Mon, 14 Aug 2023 at 12:37, Peng Fan (OSS) <peng.fan@oss.nxp.com>
> > > wrote:
> > > >
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > Add multi states support, this is to support devices could run in LP
> > > > mode when runtime suspend, and OFF mode when system suspend.
> > >
> > > For my understanding, is there a functional problem to support OFF at
> > > runtime suspend too?
> >
> > In OFF mode, the HW state is lost, so the clks that exported by this(Subsystem)
> > genpd is lost. While in LF mode, no need handle clks recover.
> >
> >
> > Such as subsystem LSIO has clks output, has GPIO, has LPUART.
> >
> > The clks are in drivers/clk/imx/clk-imx8qxp*, which relies on the scu pd.
> >
> > If scu-pd is off, the clks will lose state.
>
> Thanks for clarifying, much appreciated! So it sounds like it's the
> clock provider(s) that has these requirements then. Can we let the
> clock provider set a QoS latency constraint for its device that is
> attached to the genpd then? To prevent the deeper OFF state?
>
> Another option would be to enable runtime PM support for the clock
> provider (to manage the save/restore from runtime PM callbacks), but
> whether that's feasible sounds like a separate discussion.
>
> >
> > >
> > > >
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > >  drivers/genpd/imx/scu-pd.c | 48
> > > > ++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 46 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
> > > > index 2f693b67ddb4..30da101119eb 100644
> > > > --- a/drivers/genpd/imx/scu-pd.c
> > > > +++ b/drivers/genpd/imx/scu-pd.c
> > > > @@ -65,6 +65,12 @@
> > > >  #include <linux/pm_domain.h>
> > > >  #include <linux/slab.h>
> > > >
> > > > +enum {
> > > > +       PD_STATE_LP,
> > > > +       PD_STATE_OFF,
> > > > +       PD_STATE_MAX
> > > > +};
> > > > +
> > > >  /* SCU Power Mode Protocol definition */  struct
> > > > imx_sc_msg_req_set_resource_power_mode {
> > > >         struct imx_sc_rpc_msg hdr;
> > > > @@ -368,7 +374,8 @@ static int imx_sc_pd_power(struct
> > > generic_pm_domain *domain, bool power_on)
> > > >         hdr->size = 2;
> > > >
> > > >         msg.resource = pd->rsrc;
> > > > -       msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON :
> > > IMX_SC_PM_PW_MODE_LP;
> > > > +       msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : pd-
> > > >pd.state_idx ?
> > > > +                  IMX_SC_PM_PW_MODE_OFF : IMX_SC_PM_PW_MODE_LP;
> > > >
> > > >         /* keep uart console power on for no_console_suspend */
> > > >         if (imx_con_rsrc == pd->rsrc && !console_suspend_enabled &&
> > > > !power_on) @@ -412,11 +419,33 @@ static struct generic_pm_domain
> > > *imx_scu_pd_xlate(struct of_phandle_args *spec,
> > > >         return domain;
> > > >  }
> > > >
> > > > +static bool imx_sc_pd_suspend_ok(struct device *dev) {
> > > > +       /* Always true */
> > > > +       return true;
> > > > +}
> > > > +
> > > > +static bool imx_sc_pd_power_down_ok(struct dev_pm_domain *pd) {
> > > > +       struct generic_pm_domain *genpd = pd_to_genpd(pd);
> > > > +
> > > > +       /* For runtime suspend, choose LP mode */
> > > > +       genpd->state_idx = 0;
> > > > +
> > > > +       return true;
> > > > +}
> > >
> > > I am wondering if we couldn't use the simple_qos_governor here instead. In
> > > principle it looks like we want a QoS latency constraint to be set during
> > > runtime, to prevent the OFF state.
> >
> > LP mode indeed could save resume time, but the major problem is to avoid
> > save/restore clks.
>
> Okay. So it still sounds like a QoS latency constraint (for the clock
> provider) sounds like the correct thing to do.
>
> If/when the clock provider gets runtime PM support, we can remove the
> QoS latency constraints. That should work, right?
>
> > >
> > > During system wide suspend, the deepest state is always selected by genpd.
> > >
> > > > +
> > > > +struct dev_power_governor imx_sc_pd_qos_governor = {
> > > > +       .suspend_ok = imx_sc_pd_suspend_ok,
> > > > +       .power_down_ok = imx_sc_pd_power_down_ok, };
> > > > +
> > > >  static struct imx_sc_pm_domain *
> > > >  imx_scu_add_pm_domain(struct device *dev, int idx,
> > > >                       const struct imx_sc_pd_range *pd_ranges)  {
> > > >         struct imx_sc_pm_domain *sc_pd;
> > > > +       struct genpd_power_state *states;
> > > >         bool is_off;
> > > >         int mode, ret;
> > > >
> > > > @@ -427,9 +456,22 @@ imx_scu_add_pm_domain(struct device *dev, int
> > > idx,
> > > >         if (!sc_pd)
> > > >                 return ERR_PTR(-ENOMEM);
> > > >
> > > > +       states = devm_kcalloc(dev, PD_STATE_MAX, sizeof(*states),
> > > GFP_KERNEL);
> > > > +       if (!states) {
> > > > +               devm_kfree(dev, sc_pd);
> > > > +               return ERR_PTR(-ENOMEM);
> > > > +       }
> > > > +
> > > >         sc_pd->rsrc = pd_ranges->rsrc + idx;
> > > >         sc_pd->pd.power_off = imx_sc_pd_power_off;
> > > >         sc_pd->pd.power_on = imx_sc_pd_power_on;
> > > > +       states[PD_STATE_LP].power_off_latency_ns = 25000;
> > > > +       states[PD_STATE_LP].power_on_latency_ns =  25000;
> > > > +       states[PD_STATE_OFF].power_off_latency_ns = 2500000;
> > > > +       states[PD_STATE_OFF].power_on_latency_ns =  2500000;
> > >
> > > We should probably describe these in DT instead? The domain-idle-states
> > > bindings allows us to do this. See
> > > Documentation/devicetree/bindings/power/domain-idle-state.yaml.
> >
> > The scu-pd is a firmware function node, there is no sub-genpd node inside it.
> >
> > Just like scmi pd, there is no sub-genpd in it.
>
> Not sure I got your point. We don't need a sub-genpd node to describe
> this. This is how it could look like:
>
> domain-idle-states {
>     domain_retention: domain-retention {
>         compatible = "domain-idle-state";
>         entry-latency-us = <25>;
>         exit-latency-us = <25>;
>     };
>     domain_off: domain-off {
>         compatible = "domain-idle-state";
>         entry-latency-us = <2500>;
>         exit-latency-us = <2500>;
>     };
> };
>
> power-controller {
>     compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd";
>     #power-domain-cells = <1>;
>     domain-idle-states = <&domain_retention>, <&domain_off>;
> };

Ahh, now I think I got your point. The domain-idle-states need a
corresponding power-domain specifier too, right?

Can we do something along the lines of the below:

domain-idle-states = <&domain_retention domain-id>, <&domain_off domain-id>;

Anyway, I don't have a strong opinion about moving this to the DT, if
you want to keep the values in the code, that works too.

Kind regards
Uffe

Re: [PATCH V4 0/8] genpd: imx: relocate scu-pd and misc update

[Ulf Hansson]

On Mon, 14 Aug 2023 at 13:46, Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: Re: [PATCH V4 0/8] genpd: imx: relocate scu-pd and misc update
> >
> > On Mon, 14 Aug 2023 at 12:36, Peng Fan (OSS) <peng.fan@oss.nxp.com>
> > wrote:
> > >
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > V4:
> > >  Update commit message in patch 4
> > >
> > > V3:
> > >  return -EBUSY instead of return 0 in patch 4
> > >
> > > V2:
> > > Move drivers/firmware/imx/scu-pd.c to drivers/genpd/imx
> > >
> > > This patchset is to upstream NXP downstream scu-pd driver patches.
> > > patch is to relocate scu-pd to genpd
> > > patch 2,3 is to support more PDs
> > > patch 4 is to not power off console when no console suspend patch 5 is
> > > to suppress bind patch 6 is to make genpd align with HW state patch 7
> > > is to support LP mode in runtime suspend, OFF mode in system suspend.
> > > patch 8 is to change init level to avoid uneccessary defer probe
> > >
> > > V1:
> > > This patchset is to upstream NXP downstream scu-pd driver patches.
> > > patch 1,2 is to support more PDs
> > > patch 3 is to not power off console when no console suspend patch 4 is
> > > to suppress bind patch 5 is to make genpd align with HW state patch 6
> > > is to support LP mode in runtime suspend, OFF mode in system suspend.
> > > patch 7 is to change init level to avoid uneccessary defer probe
> > >
> > >
> > > Dong Aisheng (1):
> > >   genpd: imx: scu-pd: change init level to subsys_initcall
> > >
> > > Peng Fan (7):
> > >   genpd: imx: relocate scu-pd under genpd
> > >   genpd: imx: scu-pd: enlarge PD range
> > >   genpd: imx: scu-pd: add more PDs
> > >   genpd: imx: scu-pd: do not power off console if no_console_suspend
> > >   genpd: imx: scu-pd: Suppress bind attrs
> > >   genpd: imx: scu-pd: initialize is_off according to HW state
> > >   genpd: imx: scu-pd: add multi states support
> > >
> > >  drivers/firmware/imx/Makefile            |   1 -
> > >  drivers/genpd/imx/Makefile               |   1 +
> > >  drivers/{firmware => genpd}/imx/scu-pd.c | 193
> > > +++++++++++++++++++++--
> > >  3 files changed, 183 insertions(+), 12 deletions(-)  rename
> > > drivers/{firmware => genpd}/imx/scu-pd.c (70%)
> > >
> >
> > I am fine to pick up patch 1 -> patch 6, to let them go in for v6.6.
> > Should we do that or defer until the complete series is ready?
>
> Please take patch 1-6 first. I could handle patch 7 issue in a separate
> patch, since patch 7 is orthogonal to other patches.

Okay, I have now queued up patch 1-6 for v6.6 via my genpd tree.

In regards to patch4 (no_console_suspend), did you manage to have a
look at the patch [1] I sent a few days ago?

Kind regards
Uffe

https://lore.kernel.org/all/20230810162119.152589-1-ulf.hansson@linaro.org/

[PATCH V4 0/8] genpd: imx: relocate scu-pd and misc update

[Peng Fan]

> Subject: Re: [PATCH V4 0/8] genpd: imx: relocate scu-pd and misc update
> 
> On Mon, 14 Aug 2023 at 13:46, Peng Fan <peng.fan@nxp.com> wrote:
> >
> > > Subject: Re: [PATCH V4 0/8] genpd: imx: relocate scu-pd and misc
> > > update
> > >
> > > On Mon, 14 Aug 2023 at 12:36, Peng Fan (OSS) <peng.fan@oss.nxp.com>
> > > wrote:
> > > >
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > V4:
> > > >  Update commit message in patch 4
> > > >
> > > > V3:
> > > >  return -EBUSY instead of return 0 in patch 4
> > > >
> > > > V2:
> > > > Move drivers/firmware/imx/scu-pd.c to drivers/genpd/imx
> > > >
> > > > This patchset is to upstream NXP downstream scu-pd driver patches.
> > > > patch is to relocate scu-pd to genpd patch 2,3 is to support more
> > > > PDs patch 4 is to not power off console when no console suspend
> > > > patch 5 is to suppress bind patch 6 is to make genpd align with HW
> > > > state patch 7 is to support LP mode in runtime suspend, OFF mode
> > > > in system suspend.
> > > > patch 8 is to change init level to avoid uneccessary defer probe
> > > >
> > > > V1:
> > > > This patchset is to upstream NXP downstream scu-pd driver patches.
> > > > patch 1,2 is to support more PDs
> > > > patch 3 is to not power off console when no console suspend patch
> > > > 4 is to suppress bind patch 5 is to make genpd align with HW state
> > > > patch 6 is to support LP mode in runtime suspend, OFF mode in system
> suspend.
> > > > patch 7 is to change init level to avoid uneccessary defer probe
> > > >
> > > >
> > > > Dong Aisheng (1):
> > > >   genpd: imx: scu-pd: change init level to subsys_initcall
> > > >
> > > > Peng Fan (7):
> > > >   genpd: imx: relocate scu-pd under genpd
> > > >   genpd: imx: scu-pd: enlarge PD range
> > > >   genpd: imx: scu-pd: add more PDs
> > > >   genpd: imx: scu-pd: do not power off console if no_console_suspend
> > > >   genpd: imx: scu-pd: Suppress bind attrs
> > > >   genpd: imx: scu-pd: initialize is_off according to HW state
> > > >   genpd: imx: scu-pd: add multi states support
> > > >
> > > >  drivers/firmware/imx/Makefile            |   1 -
> > > >  drivers/genpd/imx/Makefile               |   1 +
> > > >  drivers/{firmware => genpd}/imx/scu-pd.c | 193
> > > > +++++++++++++++++++++--
> > > >  3 files changed, 183 insertions(+), 12 deletions(-)  rename
> > > > drivers/{firmware => genpd}/imx/scu-pd.c (70%)
> > > >
> > >
> > > I am fine to pick up patch 1 -> patch 6, to let them go in for v6.6.
> > > Should we do that or defer until the complete series is ready?
> >
> > Please take patch 1-6 first. I could handle patch 7 issue in a
> > separate patch, since patch 7 is orthogonal to other patches.
> 
> Okay, I have now queued up patch 1-6 for v6.6 via my genpd tree.
> 

Thanks for your work.

> In regards to patch4 (no_console_suspend), did you manage to have a look
> at the patch [1] I sent a few days ago?

Yeah, the patch looks good to me, but I have not find time to work
on the serial driver part following your suggestion, so still not test.

I will reply with R-b there.

Thanks,
Peng.
> 
> Kind regards
> Uffe
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Fall%2F20230810162119.152589-1-
> ulf.hansson%40linaro.org%2F&data=05%7C01%7Cpeng.fan%40nxp.com%7
> C6d64514fd9b84f1d6a8108db9f054956%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C638278617269111369%7CUnknown%7CTWFpbGZsb3d8
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C3000%7C%7C%7C&sdata=InU4ltSc26z2WJYFHTwLdyaoo9WqF6cZ1wyX
> 7FDpKkk%3D&reserved=0