[PATCH 00/11] i.MX8MM power domain support

[PATCH 00/11] i.MX8MM power domain support

[Lucas Stach]

Hi all,

this adds power domain support for the i.MX8MM to the existing GPCv2
driver. It is not complete yet, as it is still missing the VPU and
display power domains, as those require support for the BLK_CTL
regions of the VPUMIX and DISPLAYMIX domains. A Linux driver for
those regions on the i.MX8MP is currently under development and we
plan to use this as a template for the i.MX8MM when the dust has
settled. The changes in this series have been made with this in
mind, so once the BLK_CTL driver exists it should be a matter of
hooking things together via DT, with no further changes required on
the GPCv2 driver side (famous last words).

Special thanks to Marek Vasut who helped with testing and debugging
of early versions of this code.

Regards,
Lucas

Lucas Stach (11):
  soc: imx: gpcv2: move to more ideomatic error handling in probe
  soc: imx: gpcv2: move domain mapping to domain driver probe
  soc: imx: gpcv2: split power up and power down sequence control
  soc: imx: gpcv2: wait for ADB400 handshake
  soc: imx: gpcv2: add runtime PM support for power-domains
  soc: imx: gpcv2: allow domains without power-sequence control
  soc: imx: gpcv2: add support for optional resets
  dt-bindings: add defines for i.MX8MM power domains
  soc: imx: gpcv2: add support for i.MX8MM power domains
  arm64: dts: imx8mm: add GPC node and power domains
  arm64: dts: imx8mm: put USB controllers into power-domains

 .../bindings/power/fsl,imx-gpcv2.yaml         |   8 +
 arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  59 +++
 drivers/soc/imx/gpcv2.c                       | 501 +++++++++++++++---
 include/dt-bindings/power/imx8mm-power.h      |  22 +
 4 files changed, 516 insertions(+), 74 deletions(-)
 create mode 100644 include/dt-bindings/power/imx8mm-power.h

-- 
2.20.1

[PATCH 01/11] soc: imx: gpcv2: move to more ideomatic error handling in probe

[Lucas Stach]

Switch to "goto out..." error handling in domain driver probe to
avoid repeating all the error paths.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/soc/imx/gpcv2.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 6cf8a7a412bd..8f91ef2d24de 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -507,18 +507,23 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
 	ret = pm_genpd_init(&domain->genpd, NULL, true);
 	if (ret) {
 		dev_err(domain->dev, "Failed to init power domain\n");
-		imx_pgc_put_clocks(domain);
-		return ret;
+		goto out_put_clocks;
 	}
 
 	ret = of_genpd_add_provider_simple(domain->dev->of_node,
 					   &domain->genpd);
 	if (ret) {
 		dev_err(domain->dev, "Failed to add genpd provider\n");
-		pm_genpd_remove(&domain->genpd);
-		imx_pgc_put_clocks(domain);
+		goto out_genpd_remove;
 	}
 
+	return 0;
+
+out_genpd_remove:
+	pm_genpd_remove(&domain->genpd);
+out_put_clocks:
+	imx_pgc_put_clocks(domain);
+
 	return ret;
 }
 
-- 
2.20.1

Re: [PATCH 01/11] soc: imx: gpcv2: move to more ideomatic error handling in probe

[Marek Vasut]

On 9/30/20 5:49 PM, Lucas Stach wrote:
> Switch to "goto out..." error handling in domain driver probe to
> avoid repeating all the error paths.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Marek Vasut <marex@denx.de>

[PATCH 02/11] soc: imx: gpcv2: move domain mapping to domain driver probe

[Lucas Stach]

As long as the power domain driver is active we want power control
over the domain (which is what the mapping bit requests), so there
is no point in whacking it for every power control action, simply
set the bit in driver probe and clear it when the driver is removed.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/soc/imx/gpcv2.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 8f91ef2d24de..dc420734b16c 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -140,14 +140,11 @@ static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
 	int i, ret = 0;
 	u32 pxx_req;
 
-	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
-			   domain->bits.map, domain->bits.map);
-
 	if (has_regulator && on) {
 		ret = regulator_enable(domain->regulator);
 		if (ret) {
 			dev_err(domain->dev, "failed to enable regulator\n");
-			goto unmap;
+			return ret;
 		}
 	}
 
@@ -203,9 +200,7 @@ static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
 		/* Preserve earlier error code */
 		ret = ret ?: err;
 	}
-unmap:
-	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
-			   domain->bits.map, 0);
+
 	return ret;
 }
 
@@ -504,10 +499,13 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
+			   domain->bits.map, domain->bits.map);
+
 	ret = pm_genpd_init(&domain->genpd, NULL, true);
 	if (ret) {
 		dev_err(domain->dev, "Failed to init power domain\n");
-		goto out_put_clocks;
+		goto out_domain_unmap;
 	}
 
 	ret = of_genpd_add_provider_simple(domain->dev->of_node,
@@ -521,7 +519,9 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
 
 out_genpd_remove:
 	pm_genpd_remove(&domain->genpd);
-out_put_clocks:
+out_domain_unmap:
+	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
+			   domain->bits.map, 0);
 	imx_pgc_put_clocks(domain);
 
 	return ret;
@@ -533,6 +533,10 @@ static int imx_pgc_domain_remove(struct platform_device *pdev)
 
 	of_genpd_del_provider(domain->dev->of_node);
 	pm_genpd_remove(&domain->genpd);
+
+	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
+			   domain->bits.map, 0);
+
 	imx_pgc_put_clocks(domain);
 
 	return 0;
-- 
2.20.1

Re: [PATCH 02/11] soc: imx: gpcv2: move domain mapping to domain driver probe

[Marek Vasut]

On 9/30/20 5:49 PM, Lucas Stach wrote:
[...]
> @@ -504,10 +499,13 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> +			   domain->bits.map, domain->bits.map);

Shouldn't this also clear the "top" bits of the mapping, to remove power
control from the CM4 , now that CA53 Linux is in control of those domains ?

In fact, what happens if you boot linux on the CM4 instead ? I think you
can do it on MX7D .

[PATCH 03/11] soc: imx: gpcv2: split power up and power down sequence control

[Lucas Stach]

The current mixed function to control both power up and power down
sequences is very hard to follow and already contains some sequence
errors like triggering the ADB400 handshake at the wrong time due to
this. Split the function into two, which results in slightly more
code, but is way easier to get right.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/soc/imx/gpcv2.c | 149 +++++++++++++++++++++++++---------------
 1 file changed, 95 insertions(+), 54 deletions(-)

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index dc420734b16c..f91063c9fb92 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -127,20 +127,19 @@ struct imx_pgc_domain_data {
 	const struct regmap_access_table *reg_access_table;
 };
 
-static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
-				      bool on)
+static inline struct imx_pgc_domain *
+to_imx_pgc_domain(struct generic_pm_domain *genpd)
 {
-	struct imx_pgc_domain *domain = container_of(genpd,
-						      struct imx_pgc_domain,
-						      genpd);
-	unsigned int offset = on ?
-		GPC_PU_PGC_SW_PUP_REQ : GPC_PU_PGC_SW_PDN_REQ;
-	const bool enable_power_control = !on;
-	const bool has_regulator = !IS_ERR(domain->regulator);
-	int i, ret = 0;
-	u32 pxx_req;
-
-	if (has_regulator && on) {
+	return container_of(genpd, struct imx_pgc_domain, genpd);
+}
+
+static int imx_pgc_power_up(struct generic_pm_domain *genpd)
+{
+	struct imx_pgc_domain *domain = to_imx_pgc_domain(genpd);
+	u32 reg_val;
+	int i, ret;
+
+	if (!IS_ERR(domain->regulator)) {
 		ret = regulator_enable(domain->regulator);
 		if (ret) {
 			dev_err(domain->dev, "failed to enable regulator\n");
@@ -149,69 +148,111 @@ static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
 	}
 
 	/* Enable reset clocks for all devices in the domain */
+	for (i = 0; i < domain->num_clks; i++) {
+		ret = clk_prepare_enable(domain->clk[i]);
+		if (ret) {
+			dev_err(domain->dev, "failed to enable clock %d\n", i);
+			goto out_clk_disable;
+		}
+	}
+
+	/* request the domain to power up */
+	regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PUP_REQ,
+			   domain->bits.pxx, domain->bits.pxx);
+	/*
+	 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
+	 * for PUP_REQ/PDN_REQ bit to be cleared
+	 */
+	ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PGC_SW_PUP_REQ,
+				       reg_val, !(reg_val & domain->bits.pxx),
+				       0, USEC_PER_MSEC);
+	if (ret) {
+		dev_err(domain->dev, "failed to command PGC\n");
+		goto out_clk_disable;
+	}
+
+	/* disable power control */
+	regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
+			   GPC_PGC_CTRL_PCR, 0);
+
+	/* request the ADB400 to power up */
+	if (domain->bits.hsk)
+		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
+				   domain->bits.hsk, domain->bits.hsk);
+
+	/* Disable reset clocks for all devices in the domain */
 	for (i = 0; i < domain->num_clks; i++)
-		clk_prepare_enable(domain->clk[i]);
+		clk_disable_unprepare(domain->clk[i]);
+
+	return 0;
 
-	if (enable_power_control)
-		regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
-				   GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR);
+out_clk_disable:
+	for (i--; i >= 0; i--)
+		clk_disable_unprepare(domain->clk[i]);
+	if (!IS_ERR(domain->regulator))
+		regulator_disable(domain->regulator);
+
+	return ret;
+}
+
+static int imx_pgc_power_down(struct generic_pm_domain *genpd)
+{
+	struct imx_pgc_domain *domain = to_imx_pgc_domain(genpd);
+	u32 reg_val;
+	int i, ret;
 
+	/* Enable reset clocks for all devices in the domain */
+	for (i = 0; i < domain->num_clks; i++) {
+		ret = clk_prepare_enable(domain->clk[i]);
+		if (ret) {
+			dev_err(domain->dev, "failed to enable clock %d\n", i);
+			goto out_clk_disable;
+		}
+	}
+
+	/* request the ADB400 to power down */
 	if (domain->bits.hsk)
 		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
-				   domain->bits.hsk, on ? domain->bits.hsk : 0);
+				   domain->bits.hsk, 0);
 
-	regmap_update_bits(domain->regmap, offset,
-			   domain->bits.pxx, domain->bits.pxx);
+	/* enable power control */
+	regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
+			   GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR);
 
+	/* request the domain to power down */
+	regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PDN_REQ,
+			   domain->bits.pxx, domain->bits.pxx);
 	/*
 	 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
 	 * for PUP_REQ/PDN_REQ bit to be cleared
 	 */
-	ret = regmap_read_poll_timeout(domain->regmap, offset, pxx_req,
-				       !(pxx_req & domain->bits.pxx),
+	ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PGC_SW_PDN_REQ,
+				       reg_val, !(reg_val & domain->bits.pxx),
 				       0, USEC_PER_MSEC);
 	if (ret) {
 		dev_err(domain->dev, "failed to command PGC\n");
-		/*
-		 * If we were in a process of enabling a
-		 * domain and failed we might as well disable
-		 * the regulator we just enabled. And if it
-		 * was the opposite situation and we failed to
-		 * power down -- keep the regulator on
-		 */
-		on = !on;
+		goto out_clk_disable;
 	}
 
-	if (enable_power_control)
-		regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
-				   GPC_PGC_CTRL_PCR, 0);
-
 	/* Disable reset clocks for all devices in the domain */
 	for (i = 0; i < domain->num_clks; i++)
 		clk_disable_unprepare(domain->clk[i]);
 
-	if (has_regulator && !on) {
-		int err;
-
-		err = regulator_disable(domain->regulator);
-		if (err)
-			dev_err(domain->dev,
-				"failed to disable regulator: %d\n", err);
-		/* Preserve earlier error code */
-		ret = ret ?: err;
+	if (!IS_ERR(domain->regulator)) {
+		ret = regulator_disable(domain->regulator);
+		if (ret) {
+			dev_err(domain->dev, "failed to disable regulator\n");
+			return ret;
+		}
 	}
 
-	return ret;
-}
+	return 0;
 
-static int imx_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd)
-{
-	return imx_gpc_pu_pgc_sw_pxx_req(genpd, true);
-}
+out_clk_disable:
+	for (i--; i >= 0; i--)
+		clk_disable_unprepare(domain->clk[i]);
 
-static int imx_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd)
-{
-	return imx_gpc_pu_pgc_sw_pxx_req(genpd, false);
+	return 0;
 }
 
 static const struct imx_pgc_domain imx7_pgc_domains[] = {
@@ -631,8 +672,8 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
 
 		domain = pd_pdev->dev.platform_data;
 		domain->regmap = regmap;
-		domain->genpd.power_on  = imx_gpc_pu_pgc_sw_pup_req;
-		domain->genpd.power_off = imx_gpc_pu_pgc_sw_pdn_req;
+		domain->genpd.power_on  = imx_pgc_power_up;
+		domain->genpd.power_off = imx_pgc_power_down;
 
 		pd_pdev->dev.parent = dev;
 		pd_pdev->dev.of_node = np;
-- 
2.20.1

Re: [PATCH 03/11] soc: imx: gpcv2: split power up and power down sequence control

[Marek Vasut]

On 9/30/20 5:49 PM, Lucas Stach wrote:
[...]

> @@ -149,69 +148,111 @@ static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd,
>  	}
>  
>  	/* Enable reset clocks for all devices in the domain */
> +	for (i = 0; i < domain->num_clks; i++) {
> +		ret = clk_prepare_enable(domain->clk[i]);
> +		if (ret) {
> +			dev_err(domain->dev, "failed to enable clock %d\n", i);
> +			goto out_clk_disable;
> +		}
> +	}

Can we use clk_bulk functions here ?

[...]

[PATCH 04/11] soc: imx: gpcv2: wait for ADB400 handshake

[Lucas Stach]

New reference manuals show that there is actually a status bit for
the ADB400 handshake. Add a poll loop to wait for the ADB400 to
acknowledge our request.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/soc/imx/gpcv2.c | 43 +++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index f91063c9fb92..3cfb8b51c23e 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -69,6 +69,9 @@
 
 #define GPC_PU_PWRHSK			0x1fc
 
+#define IMX8M_GPU_HSK_PWRDNACKN			BIT(26)
+#define IMX8M_VPU_HSK_PWRDNACKN			BIT(25)
+#define IMX8M_DISP_HSK_PWRDNACKN		BIT(24)
 #define IMX8M_GPU_HSK_PWRDNREQN			BIT(6)
 #define IMX8M_VPU_HSK_PWRDNREQN			BIT(5)
 #define IMX8M_DISP_HSK_PWRDNREQN		BIT(4)
@@ -114,7 +117,8 @@ struct imx_pgc_domain {
 	const struct {
 		u32 pxx;
 		u32 map;
-		u32 hsk;
+		u32 hskreq;
+		u32 hskack;
 	} bits;
 
 	const int voltage;
@@ -176,9 +180,19 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
 			   GPC_PGC_CTRL_PCR, 0);
 
 	/* request the ADB400 to power up */
-	if (domain->bits.hsk)
+	if (domain->bits.hskreq) {
 		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
-				   domain->bits.hsk, domain->bits.hsk);
+				   domain->bits.hskreq, domain->bits.hskreq);
+
+		ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PWRHSK,
+					       reg_val,
+					       (reg_val & domain->bits.hskack),
+					       0, USEC_PER_MSEC);
+		if (ret) {
+			dev_err(domain->dev, "failed to power up ADB400\n");
+			goto out_clk_disable;
+		}
+	}
 
 	/* Disable reset clocks for all devices in the domain */
 	for (i = 0; i < domain->num_clks; i++)
@@ -211,9 +225,19 @@ static int imx_pgc_power_down(struct generic_pm_domain *genpd)
 	}
 
 	/* request the ADB400 to power down */
-	if (domain->bits.hsk)
+	if (domain->bits.hskreq) {
 		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
-				   domain->bits.hsk, 0);
+				   domain->bits.hskreq, 0);
+
+		ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PWRHSK,
+					       reg_val,
+					       !(reg_val & domain->bits.hskack),
+					       0, USEC_PER_MSEC);
+		if (ret) {
+			dev_err(domain->dev, "failed to power down ADB400\n");
+			goto out_clk_disable;
+		}
+	}
 
 	/* enable power control */
 	regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
@@ -378,7 +402,8 @@ static const struct imx_pgc_domain imx8m_pgc_domains[] = {
 		.bits  = {
 			.pxx = IMX8M_GPU_SW_Pxx_REQ,
 			.map = IMX8M_GPU_A53_DOMAIN,
-			.hsk = IMX8M_GPU_HSK_PWRDNREQN,
+			.hskreq = IMX8M_GPU_HSK_PWRDNREQN,
+			.hskack = IMX8M_GPU_HSK_PWRDNACKN,
 		},
 		.pgc   = IMX8M_PGC_GPU,
 	},
@@ -390,7 +415,8 @@ static const struct imx_pgc_domain imx8m_pgc_domains[] = {
 		.bits  = {
 			.pxx = IMX8M_VPU_SW_Pxx_REQ,
 			.map = IMX8M_VPU_A53_DOMAIN,
-			.hsk = IMX8M_VPU_HSK_PWRDNREQN,
+			.hskreq = IMX8M_VPU_HSK_PWRDNREQN,
+			.hskack = IMX8M_VPU_HSK_PWRDNACKN,
 		},
 		.pgc   = IMX8M_PGC_VPU,
 	},
@@ -402,7 +428,8 @@ static const struct imx_pgc_domain imx8m_pgc_domains[] = {
 		.bits  = {
 			.pxx = IMX8M_DISP_SW_Pxx_REQ,
 			.map = IMX8M_DISP_A53_DOMAIN,
-			.hsk = IMX8M_DISP_HSK_PWRDNREQN,
+			.hskreq = IMX8M_DISP_HSK_PWRDNREQN,
+			.hskack = IMX8M_DISP_HSK_PWRDNACKN,
 		},
 		.pgc   = IMX8M_PGC_DISP,
 	},
-- 
2.20.1

Re: [PATCH 04/11] soc: imx: gpcv2: wait for ADB400 handshake

[Marek Vasut]

On 9/30/20 5:49 PM, Lucas Stach wrote:

[...]

> @@ -176,9 +180,19 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
>  			   GPC_PGC_CTRL_PCR, 0);
>  
>  	/* request the ADB400 to power up */
> -	if (domain->bits.hsk)
> +	if (domain->bits.hskreq) {
>  		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
> -				   domain->bits.hsk, domain->bits.hsk);
> +				   domain->bits.hskreq, domain->bits.hskreq);
> +
> +		ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PWRHSK,
> +					       reg_val,
> +					       (reg_val & domain->bits.hskack),
> +					       0, USEC_PER_MSEC);
> +		if (ret) {
> +			dev_err(domain->dev, "failed to power up ADB400\n");

The ADB400 is a bus bridge, so the bus is being attached here, not
powered up, right ?

Re: [PATCH 04/11] soc: imx: gpcv2: wait for ADB400 handshake

[Lucas Stach]

On Mi, 2020-09-30 at 18:11 +0200, Marek Vasut wrote:
> On 9/30/20 5:49 PM, Lucas Stach wrote:
> 
> [...]
> 
> > @@ -176,9 +180,19 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
> >  			   GPC_PGC_CTRL_PCR, 0);
> >  
> >  	/* request the ADB400 to power up */
> > -	if (domain->bits.hsk)
> > +	if (domain->bits.hskreq) {
> >  		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
> > -				   domain->bits.hsk, domain->bits.hsk);
> > +				   domain->bits.hskreq, domain->bits.hskreq);
> > +
> > +		ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PWRHSK,
> > +					       reg_val,
> > +					       (reg_val & domain->bits.hskack),
> > +					       0, USEC_PER_MSEC);
> > +		if (ret) {
> > +			dev_err(domain->dev, "failed to power up ADB400\n");
> 
> The ADB400 is a bus bridge, so the bus is being attached here, not
> powered up, right ?

The bits in the PWRHSK register are called "power down" bits, so I kept
this nomenclature. Also I think the ADB400 is mostly isolating the bus
in the power domains from the rest of the NoC, "attaching" of the bus
is really disabling the isolation.

As there are multiple valid naming choices I kept the naming from the
RM.

Regards,
Lucas

Re: [PATCH 04/11] soc: imx: gpcv2: wait for ADB400 handshake

[Marek Vasut]

On 9/30/20 6:19 PM, Lucas Stach wrote:
> On Mi, 2020-09-30 at 18:11 +0200, Marek Vasut wrote:
>> On 9/30/20 5:49 PM, Lucas Stach wrote:
>>
>> [...]
>>
>>> @@ -176,9 +180,19 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
>>>  			   GPC_PGC_CTRL_PCR, 0);
>>>  
>>>  	/* request the ADB400 to power up */
>>> -	if (domain->bits.hsk)
>>> +	if (domain->bits.hskreq) {
>>>  		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
>>> -				   domain->bits.hsk, domain->bits.hsk);
>>> +				   domain->bits.hskreq, domain->bits.hskreq);
>>> +
>>> +		ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PWRHSK,
>>> +					       reg_val,
>>> +					       (reg_val & domain->bits.hskack),
>>> +					       0, USEC_PER_MSEC);
>>> +		if (ret) {
>>> +			dev_err(domain->dev, "failed to power up ADB400\n");
>>
>> The ADB400 is a bus bridge, so the bus is being attached here, not
>> powered up, right ?
> 
> The bits in the PWRHSK register are called "power down" bits, so I kept
> this nomenclature. Also I think the ADB400 is mostly isolating the bus
> in the power domains from the rest of the NoC, "attaching" of the bus
> is really disabling the isolation.
> 
> As there are multiple valid naming choices I kept the naming from the
> RM.

Maybe NXP can finally explain what these bits really do ?

RE: [PATCH 04/11] soc: imx: gpcv2: wait for ADB400 handshake

[Jacky Bai]

> -----Original Message-----
> From: Marek Vasut [mailto:marex@denx.de]
> Sent: Thursday, October 1, 2020 12:23 AM
> To: Lucas Stach <l.stach@pengutronix.de>; Shawn Guo
> <shawnguo@kernel.org>; Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org; Frieder Schrempf
> <frieder.schrempf@kontron.de>; patchwork-lst@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> <festevam@gmail.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 04/11] soc: imx: gpcv2: wait for ADB400 handshake
> 
> On 9/30/20 6:19 PM, Lucas Stach wrote:
> > On Mi, 2020-09-30 at 18:11 +0200, Marek Vasut wrote:
> >> On 9/30/20 5:49 PM, Lucas Stach wrote:
> >>
> >> [...]
> >>
> >>> @@ -176,9 +180,19 @@ static int imx_pgc_power_up(struct
> generic_pm_domain *genpd)
> >>>  			   GPC_PGC_CTRL_PCR, 0);
> >>>
> >>>  	/* request the ADB400 to power up */
> >>> -	if (domain->bits.hsk)
> >>> +	if (domain->bits.hskreq) {
> >>>  		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
> >>> -				   domain->bits.hsk, domain->bits.hsk);
> >>> +				   domain->bits.hskreq, domain->bits.hskreq);
> >>> +
> >>> +		ret = regmap_read_poll_timeout(domain->regmap,
> GPC_PU_PWRHSK,
> >>> +					       reg_val,
> >>> +					       (reg_val & domain->bits.hskack),
> >>> +					       0, USEC_PER_MSEC);
> >>> +		if (ret) {
> >>> +			dev_err(domain->dev, "failed to power up ADB400\n");
> >>
> >> The ADB400 is a bus bridge, so the bus is being attached here, not
> >> powered up, right ?
> >
> > The bits in the PWRHSK register are called "power down" bits, so I
> > kept this nomenclature. Also I think the ADB400 is mostly isolating
> > the bus in the power domains from the rest of the NoC, "attaching" of
> > the bus is really disabling the isolation.
> >
> > As there are multiple valid naming choices I kept the naming from the
> > RM.
> 
> Maybe NXP can finally explain what these bits really do ?

This bit is used to sync the ADB400 bridge to a known status before MIX side power down & isolation.
Detailed info can be find in ARM's ADB400 TRM.

BR
Jacky Bai

Re: [PATCH 04/11] soc: imx: gpcv2: wait for ADB400 handshake

[Marek Vasut]

On 10/9/20 5:05 AM, Jacky Bai wrote:
[...]

>>>>> @@ -176,9 +180,19 @@ static int imx_pgc_power_up(struct
>> generic_pm_domain *genpd)
>>>>>  			   GPC_PGC_CTRL_PCR, 0);
>>>>>
>>>>>  	/* request the ADB400 to power up */
>>>>> -	if (domain->bits.hsk)
>>>>> +	if (domain->bits.hskreq) {
>>>>>  		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
>>>>> -				   domain->bits.hsk, domain->bits.hsk);
>>>>> +				   domain->bits.hskreq, domain->bits.hskreq);
>>>>> +
>>>>> +		ret = regmap_read_poll_timeout(domain->regmap,
>> GPC_PU_PWRHSK,
>>>>> +					       reg_val,
>>>>> +					       (reg_val & domain->bits.hskack),
>>>>> +					       0, USEC_PER_MSEC);
>>>>> +		if (ret) {
>>>>> +			dev_err(domain->dev, "failed to power up ADB400\n");
>>>>
>>>> The ADB400 is a bus bridge, so the bus is being attached here, not
>>>> powered up, right ?
>>>
>>> The bits in the PWRHSK register are called "power down" bits, so I
>>> kept this nomenclature. Also I think the ADB400 is mostly isolating
>>> the bus in the power domains from the rest of the NoC, "attaching" of
>>> the bus is really disabling the isolation.
>>>
>>> As there are multiple valid naming choices I kept the naming from the
>>> RM.
>>
>> Maybe NXP can finally explain what these bits really do ?
> 
> This bit is used to sync the ADB400 bridge to a known status before MIX side power down & isolation.
> Detailed info can be find in ARM's ADB400 TRM.

Is this documentation publicly available ?

RE: [PATCH 04/11] soc: imx: gpcv2: wait for ADB400 handshake

[Jacky Bai]

> -----Original Message-----
> From: Marek Vasut [mailto:marex@denx.de]
> Sent: Friday, October 9, 2020 3:27 PM
> To: Jacky Bai <ping.bai@nxp.com>; Lucas Stach <l.stach@pengutronix.de>;
> Shawn Guo <shawnguo@kernel.org>; Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org; Frieder Schrempf
> <frieder.schrempf@kontron.de>; patchwork-lst@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> <festevam@gmail.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 04/11] soc: imx: gpcv2: wait for ADB400 handshake
> 
> On 10/9/20 5:05 AM, Jacky Bai wrote:
> [...]
> 
> >>>>> @@ -176,9 +180,19 @@ static int imx_pgc_power_up(struct
> >> generic_pm_domain *genpd)
> >>>>>  			   GPC_PGC_CTRL_PCR, 0);
> >>>>>
> >>>>>  	/* request the ADB400 to power up */
> >>>>> -	if (domain->bits.hsk)
> >>>>> +	if (domain->bits.hskreq) {
> >>>>>  		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
> >>>>> -				   domain->bits.hsk, domain->bits.hsk);
> >>>>> +				   domain->bits.hskreq, domain->bits.hskreq);
> >>>>> +
> >>>>> +		ret = regmap_read_poll_timeout(domain->regmap,
> >> GPC_PU_PWRHSK,
> >>>>> +					       reg_val,
> >>>>> +					       (reg_val & domain->bits.hskack),
> >>>>> +					       0, USEC_PER_MSEC);
> >>>>> +		if (ret) {
> >>>>> +			dev_err(domain->dev, "failed to power up ADB400\n");
> >>>>
> >>>> The ADB400 is a bus bridge, so the bus is being attached here, not
> >>>> powered up, right ?
> >>>
> >>> The bits in the PWRHSK register are called "power down" bits, so I
> >>> kept this nomenclature. Also I think the ADB400 is mostly isolating
> >>> the bus in the power domains from the rest of the NoC, "attaching"
> >>> of the bus is really disabling the isolation.
> >>>
> >>> As there are multiple valid naming choices I kept the naming from
> >>> the RM.
> >>
> >> Maybe NXP can finally explain what these bits really do ?
> >
> > This bit is used to sync the ADB400 bridge to a known status before MIX side
> power down & isolation.
> > Detailed info can be find in ARM's ADB400 TRM.
> 
> Is this documentation publicly available ?

Ooh, Sorry, It seems confidential. Some info shared below:

When the powerdown interface is used then the bridge must enter the idle state before either
domain can be reset or powered down:
1. The ADB-400 must be quiescent before a powerdown request. It is a system responsibility
to ensure that all transactions are completed and no new transactions are sent to ADB-400.
2. When the ADB-400 receives the powerdown request signal, pwrdnreqn, LOW it ensures
that all FIFOs are empty, pointers are reset to zero, and no false transactions can be
generated.
3. When the ADB-400 completes the internal shutdown process, it sets the pwrdnackn
signal LOW. The interface is in idle state and powerdown can commence.

BR
Jacky Bai

Re: [PATCH 04/11] soc: imx: gpcv2: wait for ADB400 handshake

[Marek Vasut]

On 10/9/20 9:51 AM, Jacky Bai wrote:

[...]

>>>>>>> @@ -176,9 +180,19 @@ static int imx_pgc_power_up(struct
>>>> generic_pm_domain *genpd)
>>>>>>>  			   GPC_PGC_CTRL_PCR, 0);
>>>>>>>
>>>>>>>  	/* request the ADB400 to power up */
>>>>>>> -	if (domain->bits.hsk)
>>>>>>> +	if (domain->bits.hskreq) {
>>>>>>>  		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
>>>>>>> -				   domain->bits.hsk, domain->bits.hsk);
>>>>>>> +				   domain->bits.hskreq, domain->bits.hskreq);
>>>>>>> +
>>>>>>> +		ret = regmap_read_poll_timeout(domain->regmap,
>>>> GPC_PU_PWRHSK,
>>>>>>> +					       reg_val,
>>>>>>> +					       (reg_val & domain->bits.hskack),
>>>>>>> +					       0, USEC_PER_MSEC);
>>>>>>> +		if (ret) {
>>>>>>> +			dev_err(domain->dev, "failed to power up ADB400\n");
>>>>>>
>>>>>> The ADB400 is a bus bridge, so the bus is being attached here, not
>>>>>> powered up, right ?
>>>>>
>>>>> The bits in the PWRHSK register are called "power down" bits, so I
>>>>> kept this nomenclature. Also I think the ADB400 is mostly isolating
>>>>> the bus in the power domains from the rest of the NoC, "attaching"
>>>>> of the bus is really disabling the isolation.
>>>>>
>>>>> As there are multiple valid naming choices I kept the naming from
>>>>> the RM.
>>>>
>>>> Maybe NXP can finally explain what these bits really do ?
>>>
>>> This bit is used to sync the ADB400 bridge to a known status before MIX side
>> power down & isolation.
>>> Detailed info can be find in ARM's ADB400 TRM.
>>
>> Is this documentation publicly available ?
> 
> Ooh, Sorry, It seems confidential. Some info shared below:
> 
> When the powerdown interface is used then the bridge must enter the idle state before either
> domain can be reset or powered down:
> 1. The ADB-400 must be quiescent before a powerdown request. It is a system responsibility
> to ensure that all transactions are completed and no new transactions are sent to ADB-400.
> 2. When the ADB-400 receives the powerdown request signal, pwrdnreqn, LOW it ensures
> that all FIFOs are empty, pointers are reset to zero, and no false transactions can be
> generated.
> 3. When the ADB-400 completes the internal shutdown process, it sets the pwrdnackn
> signal LOW. The interface is in idle state and powerdown can commence.

Nice, this was useful, thanks !

[PATCH 05/11] soc: imx: gpcv2: add runtime PM support for power-domains

[Lucas Stach]

This allows to nest domains into other power domains and have the
parent domain powered up/down as required by the child domains.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/soc/imx/gpcv2.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 3cfb8b51c23e..5bb7b1cc7c10 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -12,6 +12,7 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/sizes.h>
@@ -143,11 +144,17 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
 	u32 reg_val;
 	int i, ret;
 
+	ret = pm_runtime_get_sync(domain->dev);
+	if (ret) {
+		pm_runtime_put_noidle(domain->dev);
+		return ret;
+	}
+
 	if (!IS_ERR(domain->regulator)) {
 		ret = regulator_enable(domain->regulator);
 		if (ret) {
 			dev_err(domain->dev, "failed to enable regulator\n");
-			return ret;
+			goto out_put_pm;
 		}
 	}
 
@@ -205,6 +212,8 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
 		clk_disable_unprepare(domain->clk[i]);
 	if (!IS_ERR(domain->regulator))
 		regulator_disable(domain->regulator);
+out_put_pm:
+	pm_runtime_put(domain->dev);
 
 	return ret;
 }
@@ -270,6 +279,8 @@ static int imx_pgc_power_down(struct generic_pm_domain *genpd)
 		}
 	}
 
+	pm_runtime_put(domain->dev);
+
 	return 0;
 
 out_clk_disable:
@@ -567,6 +578,8 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	pm_runtime_enable(domain->dev);
+
 	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
 			   domain->bits.map, domain->bits.map);
 
@@ -590,6 +603,7 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
 out_domain_unmap:
 	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
 			   domain->bits.map, 0);
+	pm_runtime_disable(domain->dev);
 	imx_pgc_put_clocks(domain);
 
 	return ret;
@@ -605,6 +619,8 @@ static int imx_pgc_domain_remove(struct platform_device *pdev)
 	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
 			   domain->bits.map, 0);
 
+	pm_runtime_disable(domain->dev);
+
 	imx_pgc_put_clocks(domain);
 
 	return 0;
-- 
2.20.1

Re: [PATCH 05/11] soc: imx: gpcv2: add runtime PM support for power-domains

[Marek Vasut]

On 9/30/20 5:50 PM, Lucas Stach wrote:
[...]
> @@ -143,11 +144,17 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
>  	u32 reg_val;
>  	int i, ret;
>  
> +	ret = pm_runtime_get_sync(domain->dev);
> +	if (ret) {
> +		pm_runtime_put_noidle(domain->dev);
> +		return ret;
> +	}
> +
>  	if (!IS_ERR(domain->regulator)) {
>  		ret = regulator_enable(domain->regulator);
>  		if (ret) {
>  			dev_err(domain->dev, "failed to enable regulator\n");
> -			return ret;
> +			goto out_put_pm;
>  		}
>  	}
>  
> @@ -205,6 +212,8 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
>  		clk_disable_unprepare(domain->clk[i]);
>  	if (!IS_ERR(domain->regulator))
>  		regulator_disable(domain->regulator);
> +out_put_pm:
> +	pm_runtime_put(domain->dev);
>  
>  	return ret;
>  }
Shouldn't this be pm_runtime_put_sync() ?

Re: [PATCH 05/11] soc: imx: gpcv2: add runtime PM support for power-domains

[Lucas Stach]

On Mi, 2020-09-30 at 18:14 +0200, Marek Vasut wrote:
> On 9/30/20 5:50 PM, Lucas Stach wrote:
> [...]
> > @@ -143,11 +144,17 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
> >  	u32 reg_val;
> >  	int i, ret;
> >  
> > +	ret = pm_runtime_get_sync(domain->dev);
> > +	if (ret) {
> > +		pm_runtime_put_noidle(domain->dev);
> > +		return ret;
> > +	}
> > +
> >  	if (!IS_ERR(domain->regulator)) {
> >  		ret = regulator_enable(domain->regulator);
> >  		if (ret) {
> >  			dev_err(domain->dev, "failed to enable regulator\n");
> > -			return ret;
> > +			goto out_put_pm;
> >  		}
> >  	}
> >  
> > @@ -205,6 +212,8 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
> >  		clk_disable_unprepare(domain->clk[i]);
> >  	if (!IS_ERR(domain->regulator))
> >  		regulator_disable(domain->regulator);
> > +out_put_pm:
> > +	pm_runtime_put(domain->dev);
> >  
> >  	return ret;
> >  }
> Shouldn't this be pm_runtime_put_sync() ?

We don't really care when the parent domains powers down, we just care
about it happening sometime, so I guess the code is fine as is, no?

Regards,
Lucas

[PATCH 06/11] soc: imx: gpcv2: allow domains without power-sequence control

[Lucas Stach]

Some of the PGC domains only control the handshake with the ADB400
and don't have any power sequence controls. Make such domains work
by allowing the pxx and map bits to be empty and skip all actions
using those controls.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/soc/imx/gpcv2.c | 89 +++++++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 40 deletions(-)

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 5bb7b1cc7c10..db93fef0c76b 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -167,24 +167,27 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
 		}
 	}
 
-	/* request the domain to power up */
-	regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PUP_REQ,
-			   domain->bits.pxx, domain->bits.pxx);
-	/*
-	 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
-	 * for PUP_REQ/PDN_REQ bit to be cleared
-	 */
-	ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PGC_SW_PUP_REQ,
-				       reg_val, !(reg_val & domain->bits.pxx),
-				       0, USEC_PER_MSEC);
-	if (ret) {
-		dev_err(domain->dev, "failed to command PGC\n");
-		goto out_clk_disable;
-	}
+	if (domain->bits.pxx) {
+		/* request the domain to power up */
+		regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PUP_REQ,
+				   domain->bits.pxx, domain->bits.pxx);
+		/*
+		 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
+		 * for PUP_REQ/PDN_REQ bit to be cleared
+		 */
+		ret = regmap_read_poll_timeout(domain->regmap,
+					       GPC_PU_PGC_SW_PUP_REQ, reg_val,
+					       !(reg_val & domain->bits.pxx),
+					       0, USEC_PER_MSEC);
+		if (ret) {
+			dev_err(domain->dev, "failed to command PGC\n");
+			goto out_clk_disable;
+		}
 
-	/* disable power control */
-	regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
-			   GPC_PGC_CTRL_PCR, 0);
+		/* disable power control */
+		regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
+				   GPC_PGC_CTRL_PCR, 0);
+	}
 
 	/* request the ADB400 to power up */
 	if (domain->bits.hskreq) {
@@ -248,23 +251,26 @@ static int imx_pgc_power_down(struct generic_pm_domain *genpd)
 		}
 	}
 
-	/* enable power control */
-	regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
-			   GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR);
-
-	/* request the domain to power down */
-	regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PDN_REQ,
-			   domain->bits.pxx, domain->bits.pxx);
-	/*
-	 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
-	 * for PUP_REQ/PDN_REQ bit to be cleared
-	 */
-	ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PGC_SW_PDN_REQ,
-				       reg_val, !(reg_val & domain->bits.pxx),
-				       0, USEC_PER_MSEC);
-	if (ret) {
-		dev_err(domain->dev, "failed to command PGC\n");
-		goto out_clk_disable;
+	if (domain->bits.pxx) {
+		/* enable power control */
+		regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
+				   GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR);
+
+		/* request the domain to power down */
+		regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PDN_REQ,
+				   domain->bits.pxx, domain->bits.pxx);
+		/*
+		 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
+		 * for PUP_REQ/PDN_REQ bit to be cleared
+		 */
+		ret = regmap_read_poll_timeout(domain->regmap,
+					       GPC_PU_PGC_SW_PDN_REQ, reg_val,
+					       !(reg_val & domain->bits.pxx),
+					       0, USEC_PER_MSEC);
+		if (ret) {
+			dev_err(domain->dev, "failed to command PGC\n");
+			goto out_clk_disable;
+		}
 	}
 
 	/* Disable reset clocks for all devices in the domain */
@@ -580,8 +586,9 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(domain->dev);
 
-	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
-			   domain->bits.map, domain->bits.map);
+	if (domain->bits.map)
+		regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
+				   domain->bits.map, domain->bits.map);
 
 	ret = pm_genpd_init(&domain->genpd, NULL, true);
 	if (ret) {
@@ -601,8 +608,9 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
 out_genpd_remove:
 	pm_genpd_remove(&domain->genpd);
 out_domain_unmap:
-	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
-			   domain->bits.map, 0);
+	if (domain->bits.map)
+		regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
+				   domain->bits.map, 0);
 	pm_runtime_disable(domain->dev);
 	imx_pgc_put_clocks(domain);
 
@@ -616,8 +624,9 @@ static int imx_pgc_domain_remove(struct platform_device *pdev)
 	of_genpd_del_provider(domain->dev->of_node);
 	pm_genpd_remove(&domain->genpd);
 
-	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
-			   domain->bits.map, 0);
+	if (domain->bits.map)
+		regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
+				   domain->bits.map, 0);
 
 	pm_runtime_disable(domain->dev);
 
-- 
2.20.1

RE: [PATCH 06/11] soc: imx: gpcv2: allow domains without power-sequence control

[Jacky Bai]

> -----Original Message-----
> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Wednesday, September 30, 2020 11:50 PM
> To: Shawn Guo <shawnguo@kernel.org>; Rob Herring <robh+dt@kernel.org>
> Cc: dl-linux-imx <linux-imx@nxp.com>; Fabio Estevam
> <festevam@gmail.com>; Frieder Schrempf <frieder.schrempf@kontron.de>;
> Marek Vasut <marex@denx.de>; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; kernel@pengutronix.de;
> patchwork-lst@pengutronix.de
> Subject: [PATCH 06/11] soc: imx: gpcv2: allow domains without
> power-sequence control
> 
> Some of the PGC domains only control the handshake with the ADB400 and
> don't have any power sequence controls. Make such domains work by
> allowing the pxx and map bits to be empty and skip all actions using those
> controls.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/soc/imx/gpcv2.c | 89 +++++++++++++++++++++++------------------
>  1 file changed, 49 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c index
> 5bb7b1cc7c10..db93fef0c76b 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -167,24 +167,27 @@ static int imx_pgc_power_up(struct
> generic_pm_domain *genpd)
>  		}
>  	}
> 
> -	/* request the domain to power up */
> -	regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PUP_REQ,
> -			   domain->bits.pxx, domain->bits.pxx);
> -	/*
> -	 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
> -	 * for PUP_REQ/PDN_REQ bit to be cleared
> -	 */
> -	ret = regmap_read_poll_timeout(domain->regmap,
> GPC_PU_PGC_SW_PUP_REQ,
> -				       reg_val, !(reg_val & domain->bits.pxx),
> -				       0, USEC_PER_MSEC);
> -	if (ret) {
> -		dev_err(domain->dev, "failed to command PGC\n");
> -		goto out_clk_disable;
> -	}
> +	if (domain->bits.pxx) {

What if the power domain's PUP/PDN control bit define is zero, for example, IMX8M_MIPI_SW_Pxx_REQ? 

BR
Jacky Bai

> +		/* request the domain to power up */
> +		regmap_update_bits(domain->regmap,
> GPC_PU_PGC_SW_PUP_REQ,
> +				   domain->bits.pxx, domain->bits.pxx);
> +		/*
> +		 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
> +		 * for PUP_REQ/PDN_REQ bit to be cleared
> +		 */
> +		ret = regmap_read_poll_timeout(domain->regmap,
> +					       GPC_PU_PGC_SW_PUP_REQ, reg_val,
> +					       !(reg_val & domain->bits.pxx),
> +					       0, USEC_PER_MSEC);
> +		if (ret) {
> +			dev_err(domain->dev, "failed to command PGC\n");
> +			goto out_clk_disable;
> +		}
> 
> -	/* disable power control */
> -	regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
> -			   GPC_PGC_CTRL_PCR, 0);
> +		/* disable power control */
> +		regmap_update_bits(domain->regmap,
> GPC_PGC_CTRL(domain->pgc),
> +				   GPC_PGC_CTRL_PCR, 0);
> +	}
> 
>  	/* request the ADB400 to power up */
>  	if (domain->bits.hskreq) {
> @@ -248,23 +251,26 @@ static int imx_pgc_power_down(struct
> generic_pm_domain *genpd)
>  		}
>  	}
> 
> -	/* enable power control */
> -	regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
> -			   GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR);
> -
> -	/* request the domain to power down */
> -	regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PDN_REQ,
> -			   domain->bits.pxx, domain->bits.pxx);
> -	/*
> -	 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
> -	 * for PUP_REQ/PDN_REQ bit to be cleared
> -	 */
> -	ret = regmap_read_poll_timeout(domain->regmap,
> GPC_PU_PGC_SW_PDN_REQ,
> -				       reg_val, !(reg_val & domain->bits.pxx),
> -				       0, USEC_PER_MSEC);
> -	if (ret) {
> -		dev_err(domain->dev, "failed to command PGC\n");
> -		goto out_clk_disable;
> +	if (domain->bits.pxx) {
> +		/* enable power control */
> +		regmap_update_bits(domain->regmap,
> GPC_PGC_CTRL(domain->pgc),
> +				   GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR);
> +
> +		/* request the domain to power down */
> +		regmap_update_bits(domain->regmap,
> GPC_PU_PGC_SW_PDN_REQ,
> +				   domain->bits.pxx, domain->bits.pxx);
> +		/*
> +		 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
> +		 * for PUP_REQ/PDN_REQ bit to be cleared
> +		 */
> +		ret = regmap_read_poll_timeout(domain->regmap,
> +					       GPC_PU_PGC_SW_PDN_REQ, reg_val,
> +					       !(reg_val & domain->bits.pxx),
> +					       0, USEC_PER_MSEC);
> +		if (ret) {
> +			dev_err(domain->dev, "failed to command PGC\n");
> +			goto out_clk_disable;
> +		}
>  	}
> 
>  	/* Disable reset clocks for all devices in the domain */ @@ -580,8
> +586,9 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
> 
>  	pm_runtime_enable(domain->dev);
> 
> -	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> -			   domain->bits.map, domain->bits.map);
> +	if (domain->bits.map)
> +		regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> +				   domain->bits.map, domain->bits.map);
> 
>  	ret = pm_genpd_init(&domain->genpd, NULL, true);
>  	if (ret) {
> @@ -601,8 +608,9 @@ static int imx_pgc_domain_probe(struct
> platform_device *pdev)
>  out_genpd_remove:
>  	pm_genpd_remove(&domain->genpd);
>  out_domain_unmap:
> -	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> -			   domain->bits.map, 0);
> +	if (domain->bits.map)
> +		regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> +				   domain->bits.map, 0);
>  	pm_runtime_disable(domain->dev);
>  	imx_pgc_put_clocks(domain);
> 
> @@ -616,8 +624,9 @@ static int imx_pgc_domain_remove(struct
> platform_device *pdev)
>  	of_genpd_del_provider(domain->dev->of_node);
>  	pm_genpd_remove(&domain->genpd);
> 
> -	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> -			   domain->bits.map, 0);
> +	if (domain->bits.map)
> +		regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> +				   domain->bits.map, 0);
> 
>  	pm_runtime_disable(domain->dev);
> 
> --
> 2.20.1

RE: [PATCH 06/11] soc: imx: gpcv2: allow domains without power-sequence control

[Jacky Bai]

Sorry, my fault, ignore this comment.

BR
Jacky Bai
> -----Original Message-----
> From: Jacky Bai
> Sent: Friday, October 9, 2020 3:54 PM
> To: Lucas Stach <l.stach@pengutronix.de>; Shawn Guo
> <shawnguo@kernel.org>; Rob Herring <robh+dt@kernel.org>
> Cc: dl-linux-imx <linux-imx@nxp.com>; Fabio Estevam
> <festevam@gmail.com>; Frieder Schrempf <frieder.schrempf@kontron.de>;
> Marek Vasut <marex@denx.de>; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; kernel@pengutronix.de;
> patchwork-lst@pengutronix.de
> Subject: RE: [PATCH 06/11] soc: imx: gpcv2: allow domains without
> power-sequence control
> 
> > -----Original Message-----
> > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > Sent: Wednesday, September 30, 2020 11:50 PM
> > To: Shawn Guo <shawnguo@kernel.org>; Rob Herring
> <robh+dt@kernel.org>
> > Cc: dl-linux-imx <linux-imx@nxp.com>; Fabio Estevam
> > <festevam@gmail.com>; Frieder Schrempf <frieder.schrempf@kontron.de>;
> > Marek Vasut <marex@denx.de>; linux-arm-kernel@lists.infradead.org;
> > devicetree@vger.kernel.org; kernel@pengutronix.de;
> > patchwork-lst@pengutronix.de
> > Subject: [PATCH 06/11] soc: imx: gpcv2: allow domains without
> > power-sequence control
> >
> > Some of the PGC domains only control the handshake with the ADB400 and
> > don't have any power sequence controls. Make such domains work by
> > allowing the pxx and map bits to be empty and skip all actions using
> > those controls.
> >
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/soc/imx/gpcv2.c | 89
> > +++++++++++++++++++++++------------------
> >  1 file changed, 49 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c index
> > 5bb7b1cc7c10..db93fef0c76b 100644
> > --- a/drivers/soc/imx/gpcv2.c
> > +++ b/drivers/soc/imx/gpcv2.c
> > @@ -167,24 +167,27 @@ static int imx_pgc_power_up(struct
> > generic_pm_domain *genpd)
> >  		}
> >  	}
> >
> > -	/* request the domain to power up */
> > -	regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PUP_REQ,
> > -			   domain->bits.pxx, domain->bits.pxx);
> > -	/*
> > -	 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
> > -	 * for PUP_REQ/PDN_REQ bit to be cleared
> > -	 */
> > -	ret = regmap_read_poll_timeout(domain->regmap,
> > GPC_PU_PGC_SW_PUP_REQ,
> > -				       reg_val, !(reg_val & domain->bits.pxx),
> > -				       0, USEC_PER_MSEC);
> > -	if (ret) {
> > -		dev_err(domain->dev, "failed to command PGC\n");
> > -		goto out_clk_disable;
> > -	}
> > +	if (domain->bits.pxx) {
> 
> What if the power domain's PUP/PDN control bit define is zero, for example,
> IMX8M_MIPI_SW_Pxx_REQ?
> 
> BR
> Jacky Bai
> 
> > +		/* request the domain to power up */
> > +		regmap_update_bits(domain->regmap,
> > GPC_PU_PGC_SW_PUP_REQ,
> > +				   domain->bits.pxx, domain->bits.pxx);
> > +		/*
> > +		 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
> > +		 * for PUP_REQ/PDN_REQ bit to be cleared
> > +		 */
> > +		ret = regmap_read_poll_timeout(domain->regmap,
> > +					       GPC_PU_PGC_SW_PUP_REQ, reg_val,
> > +					       !(reg_val & domain->bits.pxx),
> > +					       0, USEC_PER_MSEC);
> > +		if (ret) {
> > +			dev_err(domain->dev, "failed to command PGC\n");
> > +			goto out_clk_disable;
> > +		}
> >
> > -	/* disable power control */
> > -	regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
> > -			   GPC_PGC_CTRL_PCR, 0);
> > +		/* disable power control */
> > +		regmap_update_bits(domain->regmap,
> > GPC_PGC_CTRL(domain->pgc),
> > +				   GPC_PGC_CTRL_PCR, 0);
> > +	}
> >
> >  	/* request the ADB400 to power up */
> >  	if (domain->bits.hskreq) {
> > @@ -248,23 +251,26 @@ static int imx_pgc_power_down(struct
> > generic_pm_domain *genpd)
> >  		}
> >  	}
> >
> > -	/* enable power control */
> > -	regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc),
> > -			   GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR);
> > -
> > -	/* request the domain to power down */
> > -	regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PDN_REQ,
> > -			   domain->bits.pxx, domain->bits.pxx);
> > -	/*
> > -	 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
> > -	 * for PUP_REQ/PDN_REQ bit to be cleared
> > -	 */
> > -	ret = regmap_read_poll_timeout(domain->regmap,
> > GPC_PU_PGC_SW_PDN_REQ,
> > -				       reg_val, !(reg_val & domain->bits.pxx),
> > -				       0, USEC_PER_MSEC);
> > -	if (ret) {
> > -		dev_err(domain->dev, "failed to command PGC\n");
> > -		goto out_clk_disable;
> > +	if (domain->bits.pxx) {
> > +		/* enable power control */
> > +		regmap_update_bits(domain->regmap,
> > GPC_PGC_CTRL(domain->pgc),
> > +				   GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR);
> > +
> > +		/* request the domain to power down */
> > +		regmap_update_bits(domain->regmap,
> > GPC_PU_PGC_SW_PDN_REQ,
> > +				   domain->bits.pxx, domain->bits.pxx);
> > +		/*
> > +		 * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait
> > +		 * for PUP_REQ/PDN_REQ bit to be cleared
> > +		 */
> > +		ret = regmap_read_poll_timeout(domain->regmap,
> > +					       GPC_PU_PGC_SW_PDN_REQ, reg_val,
> > +					       !(reg_val & domain->bits.pxx),
> > +					       0, USEC_PER_MSEC);
> > +		if (ret) {
> > +			dev_err(domain->dev, "failed to command PGC\n");
> > +			goto out_clk_disable;
> > +		}
> >  	}
> >
> >  	/* Disable reset clocks for all devices in the domain */ @@ -580,8
> > +586,9 @@ static int imx_pgc_domain_probe(struct platform_device
> > +*pdev)
> >
> >  	pm_runtime_enable(domain->dev);
> >
> > -	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> > -			   domain->bits.map, domain->bits.map);
> > +	if (domain->bits.map)
> > +		regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> > +				   domain->bits.map, domain->bits.map);
> >
> >  	ret = pm_genpd_init(&domain->genpd, NULL, true);
> >  	if (ret) {
> > @@ -601,8 +608,9 @@ static int imx_pgc_domain_probe(struct
> > platform_device *pdev)
> >  out_genpd_remove:
> >  	pm_genpd_remove(&domain->genpd);
> >  out_domain_unmap:
> > -	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> > -			   domain->bits.map, 0);
> > +	if (domain->bits.map)
> > +		regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> > +				   domain->bits.map, 0);
> >  	pm_runtime_disable(domain->dev);
> >  	imx_pgc_put_clocks(domain);
> >
> > @@ -616,8 +624,9 @@ static int imx_pgc_domain_remove(struct
> > platform_device *pdev)
> >  	of_genpd_del_provider(domain->dev->of_node);
> >  	pm_genpd_remove(&domain->genpd);
> >
> > -	regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> > -			   domain->bits.map, 0);
> > +	if (domain->bits.map)
> > +		regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING,
> > +				   domain->bits.map, 0);
> >
> >  	pm_runtime_disable(domain->dev);
> >
> > --
> > 2.20.1

[PATCH 07/11] soc: imx: gpcv2: add support for optional resets

[Lucas Stach]

Normally the reset for the devices inside the power domain is
triggered automatically from the PGC in the power-up sequencing,
however on i.MX8MM this doesn't work for the GPU power domains.

Add support for triggering the reset explicitly during the power
up sequencing.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 .../devicetree/bindings/power/fsl,imx-gpcv2.yaml    |  6 ++++++
 drivers/soc/imx/gpcv2.c                             | 13 +++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml
index bde09a0b2da3..9773771b9000 100644
--- a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml
+++ b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml
@@ -62,6 +62,12 @@ properties:
 
           power-supply: true
 
+          resets:
+            description: |
+              A number of phandles to resets that need to be asserted during
+              power-up sequencing of the domain.
+            minItems: 1
+
         required:
           - '#power-domain-cells'
           - reg
diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index db93fef0c76b..76aa8a67d8a7 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -15,6 +15,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
+#include <linux/reset.h>
 #include <linux/sizes.h>
 #include <dt-bindings/power/imx7-power.h>
 #include <dt-bindings/power/imx8mq-power.h>
@@ -112,6 +113,7 @@ struct imx_pgc_domain {
 	struct regulator *regulator;
 	struct clk *clk[GPC_CLK_MAX];
 	int num_clks;
+	struct reset_control *reset;
 
 	unsigned int pgc;
 
@@ -167,6 +169,8 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
 		}
 	}
 
+	reset_control_assert(domain->reset);
+
 	if (domain->bits.pxx) {
 		/* request the domain to power up */
 		regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PUP_REQ,
@@ -189,6 +193,8 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
 				   GPC_PGC_CTRL_PCR, 0);
 	}
 
+	reset_control_deassert(domain->reset);
+
 	/* request the ADB400 to power up */
 	if (domain->bits.hskreq) {
 		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
@@ -577,6 +583,13 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
 				      domain->voltage, domain->voltage);
 	}
 
+	domain->reset = devm_reset_control_array_get_optional_exclusive(domain->dev);
+	if (IS_ERR(domain->reset)) {
+		if (PTR_ERR(domain->reset) != -EPROBE_DEFER)
+			dev_err(domain->dev, "Failed to get domain's reset\n");
+		return PTR_ERR(domain->reset);
+	}
+
 	ret = imx_pgc_get_clocks(domain);
 	if (ret) {
 		if (ret != -EPROBE_DEFER)
-- 
2.20.1

Re: [PATCH 07/11] soc: imx: gpcv2: add support for optional resets

[Marek Vasut]

On 9/30/20 5:50 PM, Lucas Stach wrote:
> Normally the reset for the devices inside the power domain is
> triggered automatically from the PGC in the power-up sequencing,
> however on i.MX8MM this doesn't work for the GPU power domains.

One has to wonder whether the VPU power domain has similar hardware bug
on the MX8MM ?

[...]

> @@ -112,6 +113,7 @@ struct imx_pgc_domain {
>  	struct regulator *regulator;
>  	struct clk *clk[GPC_CLK_MAX];
>  	int num_clks;
> +	struct reset_control *reset;

Keep this sorted as reverse xmas tree please.

>  	unsigned int pgc;

[...]

Re: [PATCH 07/11] soc: imx: gpcv2: add support for optional resets

[Lucas Stach]

On Mi, 2020-09-30 at 18:15 +0200, Marek Vasut wrote:
> On 9/30/20 5:50 PM, Lucas Stach wrote:
> > Normally the reset for the devices inside the power domain is
> > triggered automatically from the PGC in the power-up sequencing,
> > however on i.MX8MM this doesn't work for the GPU power domains.
> 
> One has to wonder whether the VPU power domain has similar hardware bug
> on the MX8MM ?

Nope the VPUs have separate reset bits in the BLK_CTL. So after
powering up the VPUMIX domain one can assert/deassert reset to the
individual VPU cores.

Regards,
Lucas

> [...]
> 
> > @@ -112,6 +113,7 @@ struct imx_pgc_domain {
> >  	struct regulator *regulator;
> >  	struct clk *clk[GPC_CLK_MAX];
> >  	int num_clks;
> > +	struct reset_control *reset;
> 
> Keep this sorted as reverse xmas tree please.

Triggering some OCD, here? ;) Will do.

Regards,
Lucas

Re: [PATCH 07/11] soc: imx: gpcv2: add support for optional resets

[Marek Vasut]

On 9/30/20 6:23 PM, Lucas Stach wrote:
> On Mi, 2020-09-30 at 18:15 +0200, Marek Vasut wrote:
>> On 9/30/20 5:50 PM, Lucas Stach wrote:
>>> Normally the reset for the devices inside the power domain is
>>> triggered automatically from the PGC in the power-up sequencing,
>>> however on i.MX8MM this doesn't work for the GPU power domains.
>>
>> One has to wonder whether the VPU power domain has similar hardware bug
>> on the MX8MM ?
> 
> Nope the VPUs have separate reset bits in the BLK_CTL. So after
> powering up the VPUMIX domain one can assert/deassert reset to the
> individual VPU cores.

Is there any documentation for the BLK_CTL on MX8MM ? I can't find any
in the official RM.

And also, the GPUs need to use SRC reset, does the BLK_CTL reset do the
same "degree" of reset to the VPU as the SRC reset does to the GPUs ?

Re: [PATCH 07/11] soc: imx: gpcv2: add support for optional resets

[Lucas Stach]

On Mi, 2020-09-30 at 18:30 +0200, Marek Vasut wrote:
> On 9/30/20 6:23 PM, Lucas Stach wrote:
> > On Mi, 2020-09-30 at 18:15 +0200, Marek Vasut wrote:
> > > On 9/30/20 5:50 PM, Lucas Stach wrote:
> > > > Normally the reset for the devices inside the power domain is
> > > > triggered automatically from the PGC in the power-up sequencing,
> > > > however on i.MX8MM this doesn't work for the GPU power domains.
> > > 
> > > One has to wonder whether the VPU power domain has similar hardware bug
> > > on the MX8MM ?
> > 
> > Nope the VPUs have separate reset bits in the BLK_CTL. So after
> > powering up the VPUMIX domain one can assert/deassert reset to the
> > individual VPU cores.
> 
> Is there any documentation for the BLK_CTL on MX8MM ? I can't find any
> in the official RM.

I'm still waiting on some info from NXP about this. It is not
documented in the RM.

> And also, the GPUs need to use SRC reset, does the BLK_CTL reset do the
> same "degree" of reset to the VPU as the SRC reset does to the GPUs ?

At least that is what I'm assuming.

The fundamental issue with the GPU domain is that there is no BLK_CTL
in the GPUMIX domain and the resets are hooked up to the shared SRC
reset, instead of having GPU BLK_CTL level resets.

Regards,
Lucas

Re: [PATCH 07/11] soc: imx: gpcv2: add support for optional resets

[Marek Vasut]

On 9/30/20 6:34 PM, Lucas Stach wrote:
> On Mi, 2020-09-30 at 18:30 +0200, Marek Vasut wrote:
>> On 9/30/20 6:23 PM, Lucas Stach wrote:
>>> On Mi, 2020-09-30 at 18:15 +0200, Marek Vasut wrote:
>>>> On 9/30/20 5:50 PM, Lucas Stach wrote:
>>>>> Normally the reset for the devices inside the power domain is
>>>>> triggered automatically from the PGC in the power-up sequencing,
>>>>> however on i.MX8MM this doesn't work for the GPU power domains.
>>>>
>>>> One has to wonder whether the VPU power domain has similar hardware bug
>>>> on the MX8MM ?
>>>
>>> Nope the VPUs have separate reset bits in the BLK_CTL. So after
>>> powering up the VPUMIX domain one can assert/deassert reset to the
>>> individual VPU cores.
>>
>> Is there any documentation for the BLK_CTL on MX8MM ? I can't find any
>> in the official RM.
> 
> I'm still waiting on some info from NXP about this. It is not
> documented in the RM.

Yes, I know.

>> And also, the GPUs need to use SRC reset, does the BLK_CTL reset do the
>> same "degree" of reset to the VPU as the SRC reset does to the GPUs ?
> 
> At least that is what I'm assuming.
> 
> The fundamental issue with the GPU domain is that there is no BLK_CTL
> in the GPUMIX domain and the resets are hooked up to the shared SRC
> reset, instead of having GPU BLK_CTL level resets.

Yep.

I'll CC Abel, maybe there is still undocumented way to reset the GPUs on
the MX8MM separately too.

Re: [PATCH 07/11] soc: imx: gpcv2: add support for optional resets

[Krzysztof Kozlowski]

On Wed, 30 Sep 2020 at 17:52, Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Normally the reset for the devices inside the power domain is
> triggered automatically from the PGC in the power-up sequencing,
> however on i.MX8MM this doesn't work for the GPU power domains.
>
> Add support for triggering the reset explicitly during the power
> up sequencing.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../devicetree/bindings/power/fsl,imx-gpcv2.yaml    |  6 ++++++
>  drivers/soc/imx/gpcv2.c                             | 13 +++++++++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml
> index bde09a0b2da3..9773771b9000 100644
> --- a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml
> +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml
> @@ -62,6 +62,12 @@ properties:
>
>            power-supply: true
>
> +          resets:
> +            description: |
> +              A number of phandles to resets that need to be asserted during
> +              power-up sequencing of the domain.
> +            minItems: 1
> +
>          required:
>            - '#power-domain-cells'
>            - reg

Separate patch for dt-bindings, please.

Best regards,
Krzysztof

Re: [PATCH 07/11] soc: imx: gpcv2: add support for optional resets

[Rob Herring]

On Wed, Sep 30, 2020 at 05:50:02PM +0200, Lucas Stach wrote:
> Normally the reset for the devices inside the power domain is
> triggered automatically from the PGC in the power-up sequencing,
> however on i.MX8MM this doesn't work for the GPU power domains.
> 
> Add support for triggering the reset explicitly during the power
> up sequencing.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../devicetree/bindings/power/fsl,imx-gpcv2.yaml    |  6 ++++++
>  drivers/soc/imx/gpcv2.c                             | 13 +++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml
> index bde09a0b2da3..9773771b9000 100644
> --- a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml
> +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml
> @@ -62,6 +62,12 @@ properties:
>  
>            power-supply: true
>  
> +          resets:
> +            description: |
> +              A number of phandles to resets that need to be asserted during
> +              power-up sequencing of the domain.
> +            minItems: 1

What's the max? It's going to default to 1 if you don't say.

> +
>          required:
>            - '#power-domain-cells'
>            - reg
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index db93fef0c76b..76aa8a67d8a7 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -15,6 +15,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
>  #include <linux/sizes.h>
>  #include <dt-bindings/power/imx7-power.h>
>  #include <dt-bindings/power/imx8mq-power.h>
> @@ -112,6 +113,7 @@ struct imx_pgc_domain {
>  	struct regulator *regulator;
>  	struct clk *clk[GPC_CLK_MAX];
>  	int num_clks;
> +	struct reset_control *reset;
>  
>  	unsigned int pgc;
>  
> @@ -167,6 +169,8 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
>  		}
>  	}
>  
> +	reset_control_assert(domain->reset);
> +
>  	if (domain->bits.pxx) {
>  		/* request the domain to power up */
>  		regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PUP_REQ,
> @@ -189,6 +193,8 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
>  				   GPC_PGC_CTRL_PCR, 0);
>  	}
>  
> +	reset_control_deassert(domain->reset);
> +
>  	/* request the ADB400 to power up */
>  	if (domain->bits.hskreq) {
>  		regmap_update_bits(domain->regmap, GPC_PU_PWRHSK,
> @@ -577,6 +583,13 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
>  				      domain->voltage, domain->voltage);
>  	}
>  
> +	domain->reset = devm_reset_control_array_get_optional_exclusive(domain->dev);
> +	if (IS_ERR(domain->reset)) {
> +		if (PTR_ERR(domain->reset) != -EPROBE_DEFER)
> +			dev_err(domain->dev, "Failed to get domain's reset\n");
> +		return PTR_ERR(domain->reset);
> +	}
> +
>  	ret = imx_pgc_get_clocks(domain);
>  	if (ret) {
>  		if (ret != -EPROBE_DEFER)
> -- 
> 2.20.1
> 

[PATCH 08/11] dt-bindings: add defines for i.MX8MM power domains

[Lucas Stach]

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 .../bindings/power/fsl,imx-gpcv2.yaml         |  2 ++
 include/dt-bindings/power/imx8mm-power.h      | 22 +++++++++++++++++++
 2 files changed, 24 insertions(+)
 create mode 100644 include/dt-bindings/power/imx8mm-power.h

diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml
index 9773771b9000..8dd86f67c210 100644
--- a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml
+++ b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml
@@ -26,6 +26,7 @@ properties:
     enum:
       - fsl,imx7d-gpc
       - fsl,imx8mq-gpc
+      - fsl,imx8mm-gpc
 
   reg:
     maxItems: 1
@@ -50,6 +51,7 @@ properties:
               Power domain index. Valid values are defined in
               include/dt-bindings/power/imx7-power.h for fsl,imx7d-gpc and
               include/dt-bindings/power/imx8m-power.h for fsl,imx8mq-gpc
+              include/dt-bindings/power/imx8mm-power.h for fsl,imx8mm-gpc
             maxItems: 1
 
           clocks:
diff --git a/include/dt-bindings/power/imx8mm-power.h b/include/dt-bindings/power/imx8mm-power.h
new file mode 100644
index 000000000000..fc9c2e16aadc
--- /dev/null
+++ b/include/dt-bindings/power/imx8mm-power.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ *  Copyright (C) 2020 Pengutronix, Lucas Stach <kernel@pengutronix.de>
+ */
+
+#ifndef __DT_BINDINGS_IMX8MM_POWER_H__
+#define __DT_BINDINGS_IMX8MM_POWER_H__
+
+#define IMX8MM_POWER_DOMAIN_HSIOMIX	0
+#define IMX8MM_POWER_DOMAIN_PCIE	1
+#define IMX8MM_POWER_DOMAIN_OTG1	2
+#define IMX8MM_POWER_DOMAIN_OTG2	3
+#define IMX8MM_POWER_DOMAIN_GPUMIX	4
+#define IMX8MM_POWER_DOMAIN_GPU		5
+#define IMX8MM_POWER_DOMAIN_VPUMIX	6
+#define IMX8MM_POWER_DOMAIN_VPUG1	7
+#define IMX8MM_POWER_DOMAIN_VPUG2	8
+#define IMX8MM_POWER_DOMAIN_VPUH1	9
+#define IMX8MM_POWER_DOMAIN_DISPMIX	10
+#define IMX8MM_POWER_DOMAIN_MIPI	11
+
+#endif
-- 
2.20.1

Re: [PATCH 08/11] dt-bindings: add defines for i.MX8MM power domains

[Krzysztof Kozlowski]

On Wed, Sep 30, 2020 at 05:50:03PM +0200, Lucas Stach wrote:

Missing commit message. After adding some description:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../bindings/power/fsl,imx-gpcv2.yaml         |  2 ++
>  include/dt-bindings/power/imx8mm-power.h      | 22 +++++++++++++++++++
>  2 files changed, 24 insertions(+)
>  create mode 100644 include/dt-bindings/power/imx8mm-power.h
> 

Re: [PATCH 08/11] dt-bindings: add defines for i.MX8MM power domains

[Rob Herring]

On Wed, 30 Sep 2020 17:50:03 +0200, Lucas Stach wrote:
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../bindings/power/fsl,imx-gpcv2.yaml         |  2 ++
>  include/dt-bindings/power/imx8mm-power.h      | 22 +++++++++++++++++++
>  2 files changed, 24 insertions(+)
>  create mode 100644 include/dt-bindings/power/imx8mm-power.h
> 

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

[PATCH 09/11] soc: imx: gpcv2: add support for i.MX8MM power domains

[Lucas Stach]

This adds support for the power domains founds on i.MX8MM. The 2D and 3D
GPU domains are abstracted as a single domain in the driver, as they can't
be powered up/down individually due to a shared reset.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/soc/imx/gpcv2.c | 238 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 238 insertions(+)

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 76aa8a67d8a7..a118eb9ff8b7 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -19,6 +19,7 @@
 #include <linux/sizes.h>
 #include <dt-bindings/power/imx7-power.h>
 #include <dt-bindings/power/imx8mq-power.h>
+#include <dt-bindings/power/imx8mm-power.h>
 
 #define GPC_LPCR_A_CORE_BSC			0x000
 
@@ -44,6 +45,19 @@
 #define IMX8M_PCIE1_A53_DOMAIN			BIT(3)
 #define IMX8M_MIPI_A53_DOMAIN			BIT(2)
 
+#define IMX8MM_VPUH1_A53_DOMAIN			BIT(15)
+#define IMX8MM_VPUG2_A53_DOMAIN			BIT(14)
+#define IMX8MM_VPUG1_A53_DOMAIN			BIT(13)
+#define IMX8MM_DISPMIX_A53_DOMAIN		BIT(12)
+#define IMX8MM_VPUMIX_A53_DOMAIN		BIT(10)
+#define IMX8MM_GPUMIX_A53_DOMAIN		BIT(9)
+#define IMX8MM_GPU_A53_DOMAIN			(BIT(8) | BIT(11))
+#define IMX8MM_DDR1_A53_DOMAIN			BIT(7)
+#define IMX8MM_OTG2_A53_DOMAIN			BIT(5)
+#define IMX8MM_OTG1_A53_DOMAIN			BIT(4)
+#define IMX8MM_PCIE_A53_DOMAIN			BIT(3)
+#define IMX8MM_MIPI_A53_DOMAIN			BIT(2)
+
 #define GPC_PU_PGC_SW_PUP_REQ		0x0f8
 #define GPC_PU_PGC_SW_PDN_REQ		0x104
 
@@ -67,6 +81,19 @@
 #define IMX8M_PCIE1_SW_Pxx_REQ			BIT(1)
 #define IMX8M_MIPI_SW_Pxx_REQ			BIT(0)
 
+#define IMX8MM_VPUH1_SW_Pxx_REQ			BIT(13)
+#define IMX8MM_VPUG2_SW_Pxx_REQ			BIT(12)
+#define IMX8MM_VPUG1_SW_Pxx_REQ			BIT(11)
+#define IMX8MM_DISPMIX_SW_Pxx_REQ		BIT(10)
+#define IMX8MM_VPUMIX_SW_Pxx_REQ		BIT(8)
+#define IMX8MM_GPUMIX_SW_Pxx_REQ		BIT(7)
+#define IMX8MM_GPU_SW_Pxx_REQ			(BIT(6) | BIT(9))
+#define IMX8MM_DDR1_SW_Pxx_REQ			BIT(5)
+#define IMX8MM_OTG2_SW_Pxx_REQ			BIT(3)
+#define IMX8MM_OTG1_SW_Pxx_REQ			BIT(2)
+#define IMX8MM_PCIE_SW_Pxx_REQ			BIT(1)
+#define IMX8MM_MIPI_SW_Pxx_REQ			BIT(0)
+
 #define GPC_M4_PU_PDN_FLG		0x1bc
 
 #define GPC_PU_PWRHSK			0x1fc
@@ -78,6 +105,17 @@
 #define IMX8M_VPU_HSK_PWRDNREQN			BIT(5)
 #define IMX8M_DISP_HSK_PWRDNREQN		BIT(4)
 
+
+#define IMX8MM_GPUMIX_HSK_PWRDNACKN		BIT(29)
+#define IMX8MM_GPU_HSK_PWRDNACKN		(BIT(27) | BIT(28))
+#define IMX8MM_VPUMIX_HSK_PWRDNACKN		BIT(26)
+#define IMX8MM_DISPMIX_HSK_PWRDNACKN		BIT(25)
+#define IMX8MM_HSIO_HSK_PWRDNACKN		(BIT(23) | BIT(24))
+#define IMX8MM_GPUMIX_HSK_PWRDNREQN		BIT(11)
+#define IMX8MM_GPU_HSK_PWRDNREQN		(BIT(9) | BIT(10))
+#define IMX8MM_VPUMIX_HSK_PWRDNREQN		BIT(8)
+#define IMX8MM_DISPMIX_HSK_PWRDNREQN		BIT(7)
+#define IMX8MM_HSIO_HSK_PWRDNREQN		(BIT(5) | BIT(6))
 /*
  * The PGC offset values in Reference Manual
  * (Rev. 1, 01/2018 and the older ones) GPC chapter's
@@ -100,6 +138,20 @@
 #define IMX8M_PGC_MIPI_CSI2		28
 #define IMX8M_PGC_PCIE2			29
 
+#define IMX8MM_PGC_MIPI			16
+#define IMX8MM_PGC_PCIE			17
+#define IMX8MM_PGC_OTG1			18
+#define IMX8MM_PGC_OTG2			19
+#define IMX8MM_PGC_DDR1			21
+#define IMX8MM_PGC_GPU2D		22
+#define IMX8MM_PGC_GPUMIX		23
+#define IMX8MM_PGC_VPUMIX		24
+#define IMX8MM_PGC_GPU3D		25
+#define IMX8MM_PGC_DISPMIX		26
+#define IMX8MM_PGC_VPUG1		27
+#define IMX8MM_PGC_VPUG2		28
+#define IMX8MM_PGC_VPUH1		29
+
 #define GPC_PGC_CTRL(n)			(0x800 + (n) * 0x40)
 #define GPC_PGC_SR(n)			(GPC_PGC_CTRL(n) + 0xc)
 
@@ -529,6 +581,191 @@ static const struct imx_pgc_domain_data imx8m_pgc_domain_data = {
 	.reg_access_table = &imx8m_access_table,
 };
 
+static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
+	[IMX8MM_POWER_DOMAIN_HSIOMIX] = {
+		.genpd = {
+			.name = "hsiomix",
+		},
+		.bits  = {
+			.pxx = 0, /* no power sequence control */
+			.map = 0, /* no power sequence control */
+			.hskreq = IMX8MM_HSIO_HSK_PWRDNREQN,
+			.hskack = IMX8MM_HSIO_HSK_PWRDNACKN,
+		},
+	},
+
+	[IMX8MM_POWER_DOMAIN_PCIE] = {
+		.genpd = {
+			.name = "pcie",
+		},
+		.bits  = {
+			.pxx = IMX8MM_PCIE_SW_Pxx_REQ,
+			.map = IMX8MM_PCIE_A53_DOMAIN,
+		},
+		.pgc   = IMX8MM_PGC_PCIE,
+	},
+
+	[IMX8MM_POWER_DOMAIN_OTG1] = {
+		.genpd = {
+			.name = "usb-otg1",
+		},
+		.bits  = {
+			.pxx = IMX8MM_OTG1_SW_Pxx_REQ,
+			.map = IMX8MM_OTG1_A53_DOMAIN,
+		},
+		.pgc   = IMX8MM_PGC_OTG1,
+	},
+
+	[IMX8MM_POWER_DOMAIN_OTG2] = {
+		.genpd = {
+			.name = "usb-otg2",
+		},
+		.bits  = {
+			.pxx = IMX8MM_OTG2_SW_Pxx_REQ,
+			.map = IMX8MM_OTG2_A53_DOMAIN,
+		},
+		.pgc   = IMX8MM_PGC_OTG2,
+	},
+
+	[IMX8MM_POWER_DOMAIN_GPUMIX] = {
+		.genpd = {
+			.name = "gpumix",
+		},
+		.bits  = {
+			.pxx = IMX8MM_GPUMIX_SW_Pxx_REQ,
+			.map = IMX8MM_GPUMIX_A53_DOMAIN,
+			.hskreq = IMX8MM_GPUMIX_HSK_PWRDNREQN,
+			.hskack = IMX8MM_GPUMIX_HSK_PWRDNACKN,
+		},
+		.pgc   = IMX8MM_PGC_GPUMIX,
+	},
+
+	[IMX8MM_POWER_DOMAIN_GPU] = {
+		.genpd = {
+			.name = "gpu",
+		},
+		.bits  = {
+			.pxx = IMX8MM_GPU_SW_Pxx_REQ,
+			.map = IMX8MM_GPU_A53_DOMAIN,
+			.hskreq = IMX8MM_GPU_HSK_PWRDNREQN,
+			.hskack = IMX8MM_GPU_HSK_PWRDNACKN,
+		},
+		.pgc   = IMX8MM_PGC_GPU2D,
+	},
+
+	[IMX8MM_POWER_DOMAIN_VPUMIX] = {
+		.genpd = {
+			.name = "vpumix",
+		},
+		.bits  = {
+			.pxx = IMX8MM_VPUMIX_SW_Pxx_REQ,
+			.map = IMX8MM_VPUMIX_A53_DOMAIN,
+			.hskreq = IMX8MM_VPUMIX_HSK_PWRDNREQN,
+			.hskack = IMX8MM_VPUMIX_HSK_PWRDNACKN,
+		},
+		.pgc   = IMX8MM_PGC_VPUMIX,
+	},
+
+	[IMX8MM_POWER_DOMAIN_VPUG1] = {
+		.genpd = {
+			.name = "vpu-g1",
+		},
+		.bits  = {
+			.pxx = IMX8MM_VPUG1_SW_Pxx_REQ,
+			.map = IMX8MM_VPUG1_A53_DOMAIN,
+		},
+		.pgc   = IMX8MM_PGC_VPUG1,
+	},
+
+	[IMX8MM_POWER_DOMAIN_VPUG2] = {
+		.genpd = {
+			.name = "vpu-g2",
+		},
+		.bits  = {
+			.pxx = IMX8MM_VPUG2_SW_Pxx_REQ,
+			.map = IMX8MM_VPUG2_A53_DOMAIN,
+		},
+		.pgc   = IMX8MM_PGC_VPUG2,
+	},
+
+	[IMX8MM_POWER_DOMAIN_VPUH1] = {
+		.genpd = {
+			.name = "vpu-h1",
+		},
+		.bits  = {
+			.pxx = IMX8MM_VPUH1_SW_Pxx_REQ,
+			.map = IMX8MM_VPUH1_A53_DOMAIN,
+		},
+		.pgc   = IMX8MM_PGC_VPUH1,
+	},
+
+	[IMX8MM_POWER_DOMAIN_DISPMIX] = {
+		.genpd = {
+			.name = "dispmix",
+		},
+		.bits  = {
+			.pxx = IMX8MM_DISPMIX_SW_Pxx_REQ,
+			.map = IMX8MM_DISPMIX_A53_DOMAIN,
+			.hskreq = IMX8MM_DISPMIX_HSK_PWRDNREQN,
+			.hskack = IMX8MM_DISPMIX_HSK_PWRDNACKN,
+		},
+		.pgc   = IMX8MM_PGC_DISPMIX,
+	},
+
+	[IMX8MM_POWER_DOMAIN_MIPI] = {
+		.genpd = {
+			.name = "mipi",
+		},
+		.bits  = {
+			.pxx = IMX8MM_MIPI_SW_Pxx_REQ,
+			.map = IMX8MM_MIPI_A53_DOMAIN,
+		},
+		.pgc   = IMX8MM_PGC_MIPI,
+	},
+};
+
+static const struct regmap_range imx8mm_yes_ranges[] = {
+		regmap_reg_range(GPC_LPCR_A_CORE_BSC,
+				 GPC_PU_PWRHSK),
+		regmap_reg_range(GPC_PGC_CTRL(IMX8MM_PGC_MIPI),
+				 GPC_PGC_SR(IMX8MM_PGC_MIPI)),
+		regmap_reg_range(GPC_PGC_CTRL(IMX8MM_PGC_PCIE),
+				 GPC_PGC_SR(IMX8MM_PGC_PCIE)),
+		regmap_reg_range(GPC_PGC_CTRL(IMX8MM_PGC_OTG1),
+				 GPC_PGC_SR(IMX8MM_PGC_OTG1)),
+		regmap_reg_range(GPC_PGC_CTRL(IMX8MM_PGC_OTG2),
+				 GPC_PGC_SR(IMX8MM_PGC_OTG2)),
+		regmap_reg_range(GPC_PGC_CTRL(IMX8MM_PGC_DDR1),
+				 GPC_PGC_SR(IMX8MM_PGC_DDR1)),
+		regmap_reg_range(GPC_PGC_CTRL(IMX8MM_PGC_GPU2D),
+				 GPC_PGC_SR(IMX8MM_PGC_GPU2D)),
+		regmap_reg_range(GPC_PGC_CTRL(IMX8MM_PGC_GPUMIX),
+				 GPC_PGC_SR(IMX8MM_PGC_GPUMIX)),
+		regmap_reg_range(GPC_PGC_CTRL(IMX8MM_PGC_VPUMIX),
+				 GPC_PGC_SR(IMX8MM_PGC_VPUMIX)),
+		regmap_reg_range(GPC_PGC_CTRL(IMX8MM_PGC_GPU3D),
+				 GPC_PGC_SR(IMX8MM_PGC_GPU3D)),
+		regmap_reg_range(GPC_PGC_CTRL(IMX8MM_PGC_DISPMIX),
+				 GPC_PGC_SR(IMX8MM_PGC_DISPMIX)),
+		regmap_reg_range(GPC_PGC_CTRL(IMX8MM_PGC_VPUG1),
+				 GPC_PGC_SR(IMX8MM_PGC_VPUG1)),
+		regmap_reg_range(GPC_PGC_CTRL(IMX8MM_PGC_VPUG2),
+				 GPC_PGC_SR(IMX8MM_PGC_VPUG2)),
+		regmap_reg_range(GPC_PGC_CTRL(IMX8MM_PGC_VPUH1),
+				 GPC_PGC_SR(IMX8MM_PGC_VPUH1)),
+};
+
+static const struct regmap_access_table imx8mm_access_table = {
+	.yes_ranges	= imx8mm_yes_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(imx8mm_yes_ranges),
+};
+
+static const struct imx_pgc_domain_data imx8mm_pgc_domain_data = {
+	.domains = imx8mm_pgc_domains,
+	.domains_num = ARRAY_SIZE(imx8mm_pgc_domains),
+	.reg_access_table = &imx8mm_access_table,
+};
+
 static int imx_pgc_get_clocks(struct imx_pgc_domain *domain)
 {
 	int i, ret;
@@ -757,6 +994,7 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
 static const struct of_device_id imx_gpcv2_dt_ids[] = {
 	{ .compatible = "fsl,imx7d-gpc", .data = &imx7_pgc_domain_data, },
 	{ .compatible = "fsl,imx8mq-gpc", .data = &imx8m_pgc_domain_data, },
+	{ .compatible = "fsl,imx8mm-gpc", .data = &imx8mm_pgc_domain_data, },
 	{ }
 };
 
-- 
2.20.1

Re: [PATCH 09/11] soc: imx: gpcv2: add support for i.MX8MM power domains

[Marek Vasut]

On 9/30/20 5:50 PM, Lucas Stach wrote:
[...]

> @@ -757,6 +994,7 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
>  static const struct of_device_id imx_gpcv2_dt_ids[] = {
>  	{ .compatible = "fsl,imx7d-gpc", .data = &imx7_pgc_domain_data, },
>  	{ .compatible = "fsl,imx8mq-gpc", .data = &imx8m_pgc_domain_data, },
> +	{ .compatible = "fsl,imx8mm-gpc", .data = &imx8mm_pgc_domain_data, },
>  	{ }

Please keep the list sorted (m is before q).

[PATCH 10/11] arm64: dts: imx8mm: add GPC node and power domains

[Lucas Stach]

This adds the DT nodes to describe the power domains available on the
i.MX8MM. Things are a bit more complex compared to other GPCv2 power
domain setups, as there is now a hierarchy of domains where complete
subsystems (HSIO, GPU, DISPLAY) can be gated as a whole, but also
fine granular gating within those subsystems is possible.

Note that this is still incomplete, as both VPU and DISP domains are
missing their reset clocks. Those aren't directly sourced from the CCM,
but have another level of clock gating in the BLKCTL of those domains,
which needs a separate driver.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 57 +++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index 76f040e4be5e..a841fb2d0458 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -4,6 +4,8 @@
  */
 
 #include <dt-bindings/clock/imx8mm-clock.h>
+#include <dt-bindings/power/imx8mm-power.h>
+#include <dt-bindings/reset/imx8mq-reset.h>
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
@@ -547,6 +549,61 @@
 				interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
 				#reset-cells = <1>;
 			};
+
+			gpc: gpc@303a0000 {
+				compatible = "fsl,imx8mm-gpc";
+				reg = <0x303a0000 0x10000>;
+				interrupt-parent = <&gic>;
+				interrupt-controller;
+				#interrupt-cells = <3>;
+
+				pgc {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					pgc_hsiomix: power-domain@0 {
+						#power-domain-cells = <0>;
+						reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>;
+						clocks = <&clk IMX8MM_CLK_USB_BUS>;
+					};
+
+					pgc_pcie: power-domain@1 {
+						#power-domain-cells = <0>;
+						reg = <IMX8MM_POWER_DOMAIN_PCIE>;
+						power-domains = <&pgc_hsiomix>;
+					};
+
+					pgc_otg1: power-domain@2 {
+						#power-domain-cells = <0>;
+						reg = <IMX8MM_POWER_DOMAIN_OTG1>;
+						power-domains = <&pgc_hsiomix>;
+					};
+
+					pgc_otg2: power-domain@3 {
+						#power-domain-cells = <0>;
+						reg = <IMX8MM_POWER_DOMAIN_OTG2>;
+						power-domains = <&pgc_hsiomix>;
+					};
+
+					pgc_gpumix: power-domain@4 {
+						#power-domain-cells = <0>;
+						reg = <IMX8MM_POWER_DOMAIN_GPUMIX>;
+						clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
+						         <&clk IMX8MM_CLK_GPU_AHB>;
+					};
+
+					pgc_gpu: power-domain@5 {
+						#power-domain-cells = <0>;
+						reg = <IMX8MM_POWER_DOMAIN_GPU>;
+						clocks = <&clk IMX8MM_CLK_GPU_AHB>,
+						         <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
+						         <&clk IMX8MM_CLK_GPU2D_ROOT>,
+						         <&clk IMX8MM_CLK_GPU3D_ROOT>;
+						resets = <&src IMX8MQ_RESET_GPU_RESET>;
+						power-domains = <&pgc_gpumix>;
+					};
+				};
+			};
 		};
 
 		aips2: bus@30400000 {
-- 
2.20.1

Re: [PATCH 10/11] arm64: dts: imx8mm: add GPC node and power domains

[Marek Vasut]

On 9/30/20 5:50 PM, Lucas Stach wrote:
> This adds the DT nodes to describe the power domains available on the
> i.MX8MM. Things are a bit more complex compared to other GPCv2 power
> domain setups, as there is now a hierarchy of domains where complete
> subsystems (HSIO, GPU, DISPLAY) can be gated as a whole, but also
> fine granular gating within those subsystems is possible.
> 
> Note that this is still incomplete, as both VPU and DISP domains are
> missing their reset clocks. Those aren't directly sourced from the CCM,
> but have another level of clock gating in the BLKCTL of those domains,
> which needs a separate driver.

I already have a basic draft of that BLK_CTL driver for MX8MM based on
work from Abel, so if we can agree the BLK_CTL is the way forward, I can
post that.

Re: [PATCH 10/11] arm64: dts: imx8mm: add GPC node and power domains

[Krzysztof Kozlowski]

On Wed, Sep 30, 2020 at 05:50:05PM +0200, Lucas Stach wrote:
> This adds the DT nodes to describe the power domains available on the
> i.MX8MM. Things are a bit more complex compared to other GPCv2 power
> domain setups, as there is now a hierarchy of domains where complete
> subsystems (HSIO, GPU, DISPLAY) can be gated as a whole, but also
> fine granular gating within those subsystems is possible.
> 
> Note that this is still incomplete, as both VPU and DISP domains are
> missing their reset clocks. Those aren't directly sourced from the CCM,
> but have another level of clock gating in the BLKCTL of those domains,
> which needs a separate driver.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 57 +++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index 76f040e4be5e..a841fb2d0458 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -4,6 +4,8 @@
>   */
>  
>  #include <dt-bindings/clock/imx8mm-clock.h>
> +#include <dt-bindings/power/imx8mm-power.h>
> +#include <dt-bindings/reset/imx8mq-reset.h>
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/input/input.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> @@ -547,6 +549,61 @@
>  				interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
>  				#reset-cells = <1>;
>  			};
> +
> +			gpc: gpc@303a0000 {
> +				compatible = "fsl,imx8mm-gpc";
> +				reg = <0x303a0000 0x10000>;
> +				interrupt-parent = <&gic>;
> +				interrupt-controller;

This does not validate against your GPC dtschema. Please add DTS which
passes own schema from day one (except existing issues).

Best regards,
Krzysztof

Re: [PATCH 10/11] arm64: dts: imx8mm: add GPC node and power domains

[Adam Ford]

On Wed, Sep 30, 2020 at 10:55 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> This adds the DT nodes to describe the power domains available on the
> i.MX8MM. Things are a bit more complex compared to other GPCv2 power
> domain setups, as there is now a hierarchy of domains where complete
> subsystems (HSIO, GPU, DISPLAY) can be gated as a whole, but also
> fine granular gating within those subsystems is possible.
>
> Note that this is still incomplete, as both VPU and DISP domains are
> missing their reset clocks. Those aren't directly sourced from the CCM,
> but have another level of clock gating in the BLKCTL of those domains,
> which needs a separate driver.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 57 +++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index 76f040e4be5e..a841fb2d0458 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -4,6 +4,8 @@
>   */
>
>  #include <dt-bindings/clock/imx8mm-clock.h>
> +#include <dt-bindings/power/imx8mm-power.h>
> +#include <dt-bindings/reset/imx8mq-reset.h>
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/input/input.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> @@ -547,6 +549,61 @@
>                                 interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
>                                 #reset-cells = <1>;
>                         };
> +
> +                       gpc: gpc@303a0000 {
> +                               compatible = "fsl,imx8mm-gpc";
> +                               reg = <0x303a0000 0x10000>;
> +                               interrupt-parent = <&gic>;
> +                               interrupt-controller;
> +                               #interrupt-cells = <3>;

Does this need an interrupt index within the GIC?
possibly something like:   interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;


> +
> +                               pgc {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +
> +                                       pgc_hsiomix: power-domain@0 {
> +                                               #power-domain-cells = <0>;
> +                                               reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>;
> +                                               clocks = <&clk IMX8MM_CLK_USB_BUS>;
> +                                       };
> +
> +                                       pgc_pcie: power-domain@1 {
> +                                               #power-domain-cells = <0>;
> +                                               reg = <IMX8MM_POWER_DOMAIN_PCIE>;
> +                                               power-domains = <&pgc_hsiomix>;
> +                                       };
> +
> +                                       pgc_otg1: power-domain@2 {
> +                                               #power-domain-cells = <0>;
> +                                               reg = <IMX8MM_POWER_DOMAIN_OTG1>;
> +                                               power-domains = <&pgc_hsiomix>;
> +                                       };
> +
> +                                       pgc_otg2: power-domain@3 {
> +                                               #power-domain-cells = <0>;
> +                                               reg = <IMX8MM_POWER_DOMAIN_OTG2>;
> +                                               power-domains = <&pgc_hsiomix>;
> +                                       };
> +
> +                                       pgc_gpumix: power-domain@4 {
> +                                               #power-domain-cells = <0>;
> +                                               reg = <IMX8MM_POWER_DOMAIN_GPUMIX>;
> +                                               clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
> +                                                        <&clk IMX8MM_CLK_GPU_AHB>;
> +                                       };
> +
> +                                       pgc_gpu: power-domain@5 {
> +                                               #power-domain-cells = <0>;
> +                                               reg = <IMX8MM_POWER_DOMAIN_GPU>;
> +                                               clocks = <&clk IMX8MM_CLK_GPU_AHB>,
> +                                                        <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
> +                                                        <&clk IMX8MM_CLK_GPU2D_ROOT>,
> +                                                        <&clk IMX8MM_CLK_GPU3D_ROOT>;
> +                                               resets = <&src IMX8MQ_RESET_GPU_RESET>;
> +                                               power-domains = <&pgc_gpumix>;
> +                                       };
> +                               };
> +                       };
>                 };
>
>                 aips2: bus@30400000 {
> --
> 2.20.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 10/11] arm64: dts: imx8mm: add GPC node and power domains

[Jacky Bai]

> -----Original Message-----
> From: Adam Ford [mailto:aford173@gmail.com]
> Sent: Friday, October 23, 2020 9:22 PM
> To: Lucas Stach <l.stach@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> Marek Vasut <marex@denx.de>; devicetree <devicetree@vger.kernel.org>;
> Frieder Schrempf <frieder.schrempf@kontron.de>;
> patchwork-lst@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>; Sascha
> Hauer <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>;
> arm-soc <linux-arm-kernel@lists.infradead.org>
> Subject: Re: [PATCH 10/11] arm64: dts: imx8mm: add GPC node and power
> domains
> 
> On Wed, Sep 30, 2020 at 10:55 AM Lucas Stach <l.stach@pengutronix.de>
> wrote:
> >
> > This adds the DT nodes to describe the power domains available on the
> > i.MX8MM. Things are a bit more complex compared to other GPCv2 power
> > domain setups, as there is now a hierarchy of domains where complete
> > subsystems (HSIO, GPU, DISPLAY) can be gated as a whole, but also fine
> > granular gating within those subsystems is possible.
> >
> > Note that this is still incomplete, as both VPU and DISP domains are
> > missing their reset clocks. Those aren't directly sourced from the
> > CCM, but have another level of clock gating in the BLKCTL of those
> > domains, which needs a separate driver.
> >
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 57
> > +++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > index 76f040e4be5e..a841fb2d0458 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > @@ -4,6 +4,8 @@
> >   */
> >
> >  #include <dt-bindings/clock/imx8mm-clock.h>
> > +#include <dt-bindings/power/imx8mm-power.h>
> > +#include <dt-bindings/reset/imx8mq-reset.h>
> >  #include <dt-bindings/gpio/gpio.h>
> >  #include <dt-bindings/input/input.h>
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> > @@ -547,6 +549,61 @@
> >                                 interrupts = <GIC_SPI 89
> IRQ_TYPE_LEVEL_HIGH>;
> >                                 #reset-cells = <1>;
> >                         };
> > +
> > +                       gpc: gpc@303a0000 {
> > +                               compatible = "fsl,imx8mm-gpc";
> > +                               reg = <0x303a0000 0x10000>;
> > +                               interrupt-parent = <&gic>;
> > +                               interrupt-controller;
> > +                               #interrupt-cells = <3>;
> 
> Does this need an interrupt index within the GIC?
> possibly something like:   interrupts = <GIC_SPI 87
> IRQ_TYPE_LEVEL_HIGH>;

For imx8m, except imx8mq, we don’t use gpc as interrupt controller anymore, the propterty for gic controller etc are redundant, I think

BR
Jacky Bai
> 
> 
> > +
> > +                               pgc {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +
> > +                                       pgc_hsiomix:
> power-domain@0 {
> > +
> #power-domain-cells = <0>;
> > +                                               reg =
> <IMX8MM_POWER_DOMAIN_HSIOMIX>;
> > +                                               clocks = <&clk
> IMX8MM_CLK_USB_BUS>;
> > +                                       };
> > +
> > +                                       pgc_pcie:
> power-domain@1 {
> > +
> #power-domain-cells = <0>;
> > +                                               reg =
> <IMX8MM_POWER_DOMAIN_PCIE>;
> > +                                               power-domains
> = <&pgc_hsiomix>;
> > +                                       };
> > +
> > +                                       pgc_otg1:
> power-domain@2 {
> > +
> #power-domain-cells = <0>;
> > +                                               reg =
> <IMX8MM_POWER_DOMAIN_OTG1>;
> > +                                               power-domains
> = <&pgc_hsiomix>;
> > +                                       };
> > +
> > +                                       pgc_otg2:
> power-domain@3 {
> > +
> #power-domain-cells = <0>;
> > +                                               reg =
> <IMX8MM_POWER_DOMAIN_OTG2>;
> > +                                               power-domains
> = <&pgc_hsiomix>;
> > +                                       };
> > +
> > +                                       pgc_gpumix:
> power-domain@4 {
> > +
> #power-domain-cells = <0>;
> > +                                               reg =
> <IMX8MM_POWER_DOMAIN_GPUMIX>;
> > +                                               clocks = <&clk
> IMX8MM_CLK_GPU_BUS_ROOT>,
> > +
> <&clk IMX8MM_CLK_GPU_AHB>;
> > +                                       };
> > +
> > +                                       pgc_gpu:
> power-domain@5 {
> > +
> #power-domain-cells = <0>;
> > +                                               reg =
> <IMX8MM_POWER_DOMAIN_GPU>;
> > +                                               clocks = <&clk
> IMX8MM_CLK_GPU_AHB>,
> > +
> <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
> > +
> <&clk IMX8MM_CLK_GPU2D_ROOT>,
> > +
> <&clk IMX8MM_CLK_GPU3D_ROOT>;
> > +                                               resets = <&src
> IMX8MQ_RESET_GPU_RESET>;
> > +                                               power-domains
> = <&pgc_gpumix>;
> > +                                       };
> > +                               };
> > +                       };
> >                 };
> >
> >                 aips2: bus@30400000 {
> > --
> > 2.20.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists
> > .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&amp;data=04%
> 7C0
> >
> 1%7Cping.bai%40nxp.com%7C412f29610c79470d12c408d87756b072%7C68
> 6ea1d3bc
> >
> 2b4c6fa92cd99c5c301635%7C0%7C0%7C637390561501622327%7CUnknow
> n%7CTWFpbG
> >
> Zsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%
> >
> 3D%7C1000&amp;sdata=AazKzk1Tl6LI1hLYGW1xQ%2FEYc8Ad6vk0aBdkJxwu
> w3A%3D&a
> > mp;reserved=0

Re: [PATCH 10/11] arm64: dts: imx8mm: add GPC node and power domains

[Abel Vesa]

On 20-09-30 17:50:05, Lucas Stach wrote:
> This adds the DT nodes to describe the power domains available on the
> i.MX8MM. Things are a bit more complex compared to other GPCv2 power
> domain setups, as there is now a hierarchy of domains where complete
> subsystems (HSIO, GPU, DISPLAY) can be gated as a whole, but also
> fine granular gating within those subsystems is possible.
> 
> Note that this is still incomplete, as both VPU and DISP domains are
> missing their reset clocks. Those aren't directly sourced from the CCM,
> but have another level of clock gating in the BLKCTL of those domains,
> which needs a separate driver.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 57 +++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index 76f040e4be5e..a841fb2d0458 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -4,6 +4,8 @@
>   */
>  
>  #include <dt-bindings/clock/imx8mm-clock.h>
> +#include <dt-bindings/power/imx8mm-power.h>
> +#include <dt-bindings/reset/imx8mq-reset.h>

Needs to be imx8mm-reset.h, as in 8MM, not 8MQ.

>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/input/input.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> @@ -547,6 +549,61 @@
>  				interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
>  				#reset-cells = <1>;
>  			};
> +
> +			gpc: gpc@303a0000 {
> +				compatible = "fsl,imx8mm-gpc";
> +				reg = <0x303a0000 0x10000>;
> +				interrupt-parent = <&gic>;
> +				interrupt-controller;
> +				#interrupt-cells = <3>;
> +
> +				pgc {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					pgc_hsiomix: power-domain@0 {
> +						#power-domain-cells = <0>;
> +						reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>;
> +						clocks = <&clk IMX8MM_CLK_USB_BUS>;
> +					};
> +
> +					pgc_pcie: power-domain@1 {
> +						#power-domain-cells = <0>;
> +						reg = <IMX8MM_POWER_DOMAIN_PCIE>;
> +						power-domains = <&pgc_hsiomix>;
> +					};
> +
> +					pgc_otg1: power-domain@2 {
> +						#power-domain-cells = <0>;
> +						reg = <IMX8MM_POWER_DOMAIN_OTG1>;
> +						power-domains = <&pgc_hsiomix>;
> +					};
> +
> +					pgc_otg2: power-domain@3 {
> +						#power-domain-cells = <0>;
> +						reg = <IMX8MM_POWER_DOMAIN_OTG2>;
> +						power-domains = <&pgc_hsiomix>;
> +					};
> +
> +					pgc_gpumix: power-domain@4 {
> +						#power-domain-cells = <0>;
> +						reg = <IMX8MM_POWER_DOMAIN_GPUMIX>;
> +						clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
> +						         <&clk IMX8MM_CLK_GPU_AHB>;
> +					};
> +
> +					pgc_gpu: power-domain@5 {
> +						#power-domain-cells = <0>;
> +						reg = <IMX8MM_POWER_DOMAIN_GPU>;
> +						clocks = <&clk IMX8MM_CLK_GPU_AHB>,
> +						         <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
> +						         <&clk IMX8MM_CLK_GPU2D_ROOT>,
> +						         <&clk IMX8MM_CLK_GPU3D_ROOT>;
> +						resets = <&src IMX8MQ_RESET_GPU_RESET>;
> +						power-domains = <&pgc_gpumix>;
> +					};
> +				};
> +			};
>  		};
>  
>  		aips2: bus@30400000 {
> -- 
> 2.20.1
> 

Re: [PATCH 10/11] arm64: dts: imx8mm: add GPC node and power domains

[Abel Vesa]

On 20-10-26 12:56:22, Abel Vesa wrote:
> On 20-09-30 17:50:05, Lucas Stach wrote:
> > This adds the DT nodes to describe the power domains available on the
> > i.MX8MM. Things are a bit more complex compared to other GPCv2 power
> > domain setups, as there is now a hierarchy of domains where complete
> > subsystems (HSIO, GPU, DISPLAY) can be gated as a whole, but also
> > fine granular gating within those subsystems is possible.
> > 
> > Note that this is still incomplete, as both VPU and DISP domains are
> > missing their reset clocks. Those aren't directly sourced from the CCM,
> > but have another level of clock gating in the BLKCTL of those domains,
> > which needs a separate driver.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 57 +++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > index 76f040e4be5e..a841fb2d0458 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > @@ -4,6 +4,8 @@
> >   */
> >  
> >  #include <dt-bindings/clock/imx8mm-clock.h>
> > +#include <dt-bindings/power/imx8mm-power.h>
> > +#include <dt-bindings/reset/imx8mq-reset.h>
> 
> Needs to be imx8mm-reset.h, as in 8MM, not 8MQ.
> 

Actually, now I see what you've done here. You want to use the IMX8MQ_RESET_GPU_RESET.

But I think we should avoid having reset IDs shared between i.MX8M platforms.

I'll try to find another way around this myself.

> >  #include <dt-bindings/gpio/gpio.h>
> >  #include <dt-bindings/input/input.h>
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> > @@ -547,6 +549,61 @@
> >  				interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> >  				#reset-cells = <1>;
> >  			};
> > +
> > +			gpc: gpc@303a0000 {
> > +				compatible = "fsl,imx8mm-gpc";
> > +				reg = <0x303a0000 0x10000>;
> > +				interrupt-parent = <&gic>;
> > +				interrupt-controller;
> > +				#interrupt-cells = <3>;
> > +
> > +				pgc {
> > +					#address-cells = <1>;
> > +					#size-cells = <0>;
> > +
> > +					pgc_hsiomix: power-domain@0 {
> > +						#power-domain-cells = <0>;
> > +						reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>;
> > +						clocks = <&clk IMX8MM_CLK_USB_BUS>;
> > +					};
> > +
> > +					pgc_pcie: power-domain@1 {
> > +						#power-domain-cells = <0>;
> > +						reg = <IMX8MM_POWER_DOMAIN_PCIE>;
> > +						power-domains = <&pgc_hsiomix>;
> > +					};
> > +
> > +					pgc_otg1: power-domain@2 {
> > +						#power-domain-cells = <0>;
> > +						reg = <IMX8MM_POWER_DOMAIN_OTG1>;
> > +						power-domains = <&pgc_hsiomix>;
> > +					};
> > +
> > +					pgc_otg2: power-domain@3 {
> > +						#power-domain-cells = <0>;
> > +						reg = <IMX8MM_POWER_DOMAIN_OTG2>;
> > +						power-domains = <&pgc_hsiomix>;
> > +					};
> > +
> > +					pgc_gpumix: power-domain@4 {
> > +						#power-domain-cells = <0>;
> > +						reg = <IMX8MM_POWER_DOMAIN_GPUMIX>;
> > +						clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
> > +						         <&clk IMX8MM_CLK_GPU_AHB>;
> > +					};
> > +
> > +					pgc_gpu: power-domain@5 {
> > +						#power-domain-cells = <0>;
> > +						reg = <IMX8MM_POWER_DOMAIN_GPU>;
> > +						clocks = <&clk IMX8MM_CLK_GPU_AHB>,
> > +						         <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
> > +						         <&clk IMX8MM_CLK_GPU2D_ROOT>,
> > +						         <&clk IMX8MM_CLK_GPU3D_ROOT>;
> > +						resets = <&src IMX8MQ_RESET_GPU_RESET>;
> > +						power-domains = <&pgc_gpumix>;
> > +					};
> > +				};
> > +			};
> >  		};
> >  
> >  		aips2: bus@30400000 {
> > -- 
> > 2.20.1
> > 

Re: [PATCH 10/11] arm64: dts: imx8mm: add GPC node and power domains

[Adam Ford]

On Mon, Oct 26, 2020 at 6:07 AM Abel Vesa <abel.vesa@nxp.com> wrote:
>
> On 20-10-26 12:56:22, Abel Vesa wrote:
> > On 20-09-30 17:50:05, Lucas Stach wrote:
> > > This adds the DT nodes to describe the power domains available on the
> > > i.MX8MM. Things are a bit more complex compared to other GPCv2 power
> > > domain setups, as there is now a hierarchy of domains where complete
> > > subsystems (HSIO, GPU, DISPLAY) can be gated as a whole, but also
> > > fine granular gating within those subsystems is possible.
> > >
> > > Note that this is still incomplete, as both VPU and DISP domains are
> > > missing their reset clocks. Those aren't directly sourced from the CCM,
> > > but have another level of clock gating in the BLKCTL of those domains,
> > > which needs a separate driver.
> > >
> > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > ---
> > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 57 +++++++++++++++++++++++
> > >  1 file changed, 57 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > index 76f040e4be5e..a841fb2d0458 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > @@ -4,6 +4,8 @@
> > >   */
> > >
> > >  #include <dt-bindings/clock/imx8mm-clock.h>
> > > +#include <dt-bindings/power/imx8mm-power.h>
> > > +#include <dt-bindings/reset/imx8mq-reset.h>
> >
> > Needs to be imx8mm-reset.h, as in 8MM, not 8MQ.
> >
>
> Actually, now I see what you've done here. You want to use the IMX8MQ_RESET_GPU_RESET.
>
> But I think we should avoid having reset IDs shared between i.MX8M platforms.
>
> I'll try to find another way around this myself.

The nano does the same thing as the mini as both appear to be subsets
of the i.MX8MQ.  I spent a fair amount of time reviewing the SRC
driver yesterday.

>
> > >  #include <dt-bindings/gpio/gpio.h>
> > >  #include <dt-bindings/input/input.h>
> > >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > @@ -547,6 +549,61 @@
> > >                             interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> > >                             #reset-cells = <1>;
> > >                     };
> > > +
> > > +                   gpc: gpc@303a0000 {
> > > +                           compatible = "fsl,imx8mm-gpc";
> > > +                           reg = <0x303a0000 0x10000>;
> > > +                           interrupt-parent = <&gic>;
> > > +                           interrupt-controller;
> > > +                           #interrupt-cells = <3>;
> > > +
> > > +                           pgc {
> > > +                                   #address-cells = <1>;
> > > +                                   #size-cells = <0>;
> > > +
> > > +                                   pgc_hsiomix: power-domain@0 {
> > > +                                           #power-domain-cells = <0>;
> > > +                                           reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>;
> > > +                                           clocks = <&clk IMX8MM_CLK_USB_BUS>;
> > > +                                   };
> > > +
> > > +                                   pgc_pcie: power-domain@1 {
> > > +                                           #power-domain-cells = <0>;
> > > +                                           reg = <IMX8MM_POWER_DOMAIN_PCIE>;
> > > +                                           power-domains = <&pgc_hsiomix>;
> > > +                                   };
> > > +
> > > +                                   pgc_otg1: power-domain@2 {
> > > +                                           #power-domain-cells = <0>;
> > > +                                           reg = <IMX8MM_POWER_DOMAIN_OTG1>;
> > > +                                           power-domains = <&pgc_hsiomix>;
> > > +                                   };
> > > +
> > > +                                   pgc_otg2: power-domain@3 {
> > > +                                           #power-domain-cells = <0>;
> > > +                                           reg = <IMX8MM_POWER_DOMAIN_OTG2>;
> > > +                                           power-domains = <&pgc_hsiomix>;
> > > +                                   };
> > > +
> > > +                                   pgc_gpumix: power-domain@4 {
> > > +                                           #power-domain-cells = <0>;
> > > +                                           reg = <IMX8MM_POWER_DOMAIN_GPUMIX>;
> > > +                                           clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
> > > +                                                    <&clk IMX8MM_CLK_GPU_AHB>;
> > > +                                   };
> > > +
> > > +                                   pgc_gpu: power-domain@5 {
> > > +                                           #power-domain-cells = <0>;
> > > +                                           reg = <IMX8MM_POWER_DOMAIN_GPU>;
> > > +                                           clocks = <&clk IMX8MM_CLK_GPU_AHB>,
> > > +                                                    <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
> > > +                                                    <&clk IMX8MM_CLK_GPU2D_ROOT>,
> > > +                                                    <&clk IMX8MM_CLK_GPU3D_ROOT>;
> > > +                                           resets = <&src IMX8MQ_RESET_GPU_RESET>;
> > > +                                           power-domains = <&pgc_gpumix>;
> > > +                                   };
> > > +                           };
> > > +                   };
> > >             };
> > >
> > >             aips2: bus@30400000 {
> > > --
> > > 2.20.1
> > >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 10/11] arm64: dts: imx8mm: add GPC node and power domains

[Lucas Stach]

Hi Abel,

Am Montag, den 26.10.2020, 12:56 +0200 schrieb Abel Vesa:
> On 20-09-30 17:50:05, Lucas Stach wrote:
> > This adds the DT nodes to describe the power domains available on the
> > i.MX8MM. Things are a bit more complex compared to other GPCv2 power
> > domain setups, as there is now a hierarchy of domains where complete
> > subsystems (HSIO, GPU, DISPLAY) can be gated as a whole, but also
> > fine granular gating within those subsystems is possible.
> > 
> > Note that this is still incomplete, as both VPU and DISP domains are
> > missing their reset clocks. Those aren't directly sourced from the CCM,
> > but have another level of clock gating in the BLKCTL of those domains,
> > which needs a separate driver.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 57 +++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > index 76f040e4be5e..a841fb2d0458 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > @@ -4,6 +4,8 @@
> >   */
> >  
> >  #include <dt-bindings/clock/imx8mm-clock.h>
> > +#include <dt-bindings/power/imx8mm-power.h>
> > +#include <dt-bindings/reset/imx8mq-reset.h>
> 
> Needs to be imx8mm-reset.h, as in 8MM, not 8MQ.

It's confusing, but the reset driver uses the same defines for i.MX8MM.
Comments in the imx8mq-reset.h file even specify which resets aren't
supported on i.MX8MM.

Regards,
Lucas

> >  #include <dt-bindings/gpio/gpio.h>
> >  #include <dt-bindings/input/input.h>
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> > @@ -547,6 +549,61 @@
> >  				interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> >  				#reset-cells = <1>;
> >  			};
> > +
> > +			gpc: gpc@303a0000 {
> > +				compatible = "fsl,imx8mm-gpc";
> > +				reg = <0x303a0000 0x10000>;
> > +				interrupt-parent = <&gic>;
> > +				interrupt-controller;
> > +				#interrupt-cells = <3>;
> > +
> > +				pgc {
> > +					#address-cells = <1>;
> > +					#size-cells = <0>;
> > +
> > +					pgc_hsiomix: power-domain@0 {
> > +						#power-domain-cells = <0>;
> > +						reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>;
> > +						clocks = <&clk IMX8MM_CLK_USB_BUS>;
> > +					};
> > +
> > +					pgc_pcie: power-domain@1 {
> > +						#power-domain-cells = <0>;
> > +						reg = <IMX8MM_POWER_DOMAIN_PCIE>;
> > +						power-domains = <&pgc_hsiomix>;
> > +					};
> > +
> > +					pgc_otg1: power-domain@2 {
> > +						#power-domain-cells = <0>;
> > +						reg = <IMX8MM_POWER_DOMAIN_OTG1>;
> > +						power-domains = <&pgc_hsiomix>;
> > +					};
> > +
> > +					pgc_otg2: power-domain@3 {
> > +						#power-domain-cells = <0>;
> > +						reg = <IMX8MM_POWER_DOMAIN_OTG2>;
> > +						power-domains = <&pgc_hsiomix>;
> > +					};
> > +
> > +					pgc_gpumix: power-domain@4 {
> > +						#power-domain-cells = <0>;
> > +						reg = <IMX8MM_POWER_DOMAIN_GPUMIX>;
> > +						clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
> > +						         <&clk IMX8MM_CLK_GPU_AHB>;
> > +					};
> > +
> > +					pgc_gpu: power-domain@5 {
> > +						#power-domain-cells = <0>;
> > +						reg = <IMX8MM_POWER_DOMAIN_GPU>;
> > +						clocks = <&clk IMX8MM_CLK_GPU_AHB>,
> > +						         <&clk IMX8MM_CLK_GPU_BUS_ROOT>,
> > +						         <&clk IMX8MM_CLK_GPU2D_ROOT>,
> > +						         <&clk IMX8MM_CLK_GPU3D_ROOT>;
> > +						resets = <&src IMX8MQ_RESET_GPU_RESET>;
> > +						power-domains = <&pgc_gpumix>;
> > +					};
> > +				};
> > +			};
> >  		};
> >  
> >  		aips2: bus@30400000 {
> > -- 
> > 2.20.1
> > 

[PATCH 11/11] arm64: dts: imx8mm: put USB controllers into power-domains

[Lucas Stach]

Now that we have support for the power domain controller on the i.MX8MM
we can put the USB controllers in their respective power domains to allow
them to power down the PHY when possible.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index a841fb2d0458..ab379d02d4e4 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -951,6 +951,7 @@
 				assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>;
 				fsl,usbphy = <&usbphynop1>;
 				fsl,usbmisc = <&usbmisc1 0>;
+				power-domains = <&pgc_otg1>;
 				status = "disabled";
 			};
 
@@ -970,6 +971,7 @@
 				assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>;
 				fsl,usbphy = <&usbphynop2>;
 				fsl,usbmisc = <&usbmisc2 0>;
+				power-domains = <&pgc_otg2>;
 				status = "disabled";
 			};
 
-- 
2.20.1

Re: [PATCH 00/11] i.MX8MM power domain support

[Frieder Schrempf]

On 30.09.20 17:49, Lucas Stach wrote:
> Hi all,
> 
> this adds power domain support for the i.MX8MM to the existing GPCv2
> driver. It is not complete yet, as it is still missing the VPU and
> display power domains, as those require support for the BLK_CTL
> regions of the VPUMIX and DISPLAYMIX domains. A Linux driver for
> those regions on the i.MX8MP is currently under development and we
> plan to use this as a template for the i.MX8MM when the dust has
> settled. The changes in this series have been made with this in
> mind, so once the BLK_CTL driver exists it should be a matter of
> hooking things together via DT, with no further changes required on
> the GPCv2 driver side (famous last words).
> 
> Special thanks to Marek Vasut who helped with testing and debugging
> of early versions of this code.

I tested this on our i.MX8MM boards by making sure the GPUs and USBs 
come up properly. It works just fine on v5.9-rc6 and also backported to 
5.4. So for the whole series:

Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>

> 
> Regards,
> Lucas
> 
> Lucas Stach (11):
>    soc: imx: gpcv2: move to more ideomatic error handling in probe
>    soc: imx: gpcv2: move domain mapping to domain driver probe
>    soc: imx: gpcv2: split power up and power down sequence control
>    soc: imx: gpcv2: wait for ADB400 handshake
>    soc: imx: gpcv2: add runtime PM support for power-domains
>    soc: imx: gpcv2: allow domains without power-sequence control
>    soc: imx: gpcv2: add support for optional resets
>    dt-bindings: add defines for i.MX8MM power domains
>    soc: imx: gpcv2: add support for i.MX8MM power domains
>    arm64: dts: imx8mm: add GPC node and power domains
>    arm64: dts: imx8mm: put USB controllers into power-domains
> 
>   .../bindings/power/fsl,imx-gpcv2.yaml         |   8 +
>   arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  59 +++
>   drivers/soc/imx/gpcv2.c                       | 501 +++++++++++++++---
>   include/dt-bindings/power/imx8mm-power.h      |  22 +
>   4 files changed, 516 insertions(+), 74 deletions(-)
>   create mode 100644 include/dt-bindings/power/imx8mm-power.h
> 

Re: [PATCH 00/11] i.MX8MM power domain support

[Adam Ford]

On Wed, Sep 30, 2020 at 10:50 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi all,
>
> this adds power domain support for the i.MX8MM to the existing GPCv2
> driver. It is not complete yet, as it is still missing the VPU and
> display power domains, as those require support for the BLK_CTL
> regions of the VPUMIX and DISPLAYMIX domains. A Linux driver for
> those regions on the i.MX8MP is currently under development and we
> plan to use this as a template for the i.MX8MM when the dust has
> settled. The changes in this series have been made with this in
> mind, so once the BLK_CTL driver exists it should be a matter of
> hooking things together via DT, with no further changes required on
> the GPCv2 driver side (famous last words).
>
> Special thanks to Marek Vasut who helped with testing and debugging
> of early versions of this code.
>
> Regards,
> Lucas
>
> Lucas Stach (11):
>   soc: imx: gpcv2: move to more ideomatic error handling in probe
>   soc: imx: gpcv2: move domain mapping to domain driver probe
>   soc: imx: gpcv2: split power up and power down sequence control
>   soc: imx: gpcv2: wait for ADB400 handshake
>   soc: imx: gpcv2: add runtime PM support for power-domains
>   soc: imx: gpcv2: allow domains without power-sequence control
>   soc: imx: gpcv2: add support for optional resets
>   dt-bindings: add defines for i.MX8MM power domains
>   soc: imx: gpcv2: add support for i.MX8MM power domains
>   arm64: dts: imx8mm: add GPC node and power domains
>   arm64: dts: imx8mm: put USB controllers into power-domains

I fully support trying to get this done.  I tried to push something
like this before, but I was turned down by someone at NXP [1].  I have
both a Mini and Nano board at my disposal, so if Nano support isn't
there yet, and this driver patch gets accepted, I'll gladly work to
help and/or test functionality.

[1] - https://lkml.org/lkml/2020/4/30/540

adam

>
>  .../bindings/power/fsl,imx-gpcv2.yaml         |   8 +
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  59 +++
>  drivers/soc/imx/gpcv2.c                       | 501 +++++++++++++++---
>  include/dt-bindings/power/imx8mm-power.h      |  22 +
>  4 files changed, 516 insertions(+), 74 deletions(-)
>  create mode 100644 include/dt-bindings/power/imx8mm-power.h
>
> --
> 2.20.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/11] i.MX8MM power domain support

[Marek Vasut]

On 10/3/20 8:01 PM, Adam Ford wrote:
> On Wed, Sep 30, 2020 at 10:50 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> 
>> Hi all,
>>
>> this adds power domain support for the i.MX8MM to the existing GPCv2
>> driver. It is not complete yet, as it is still missing the VPU and
>> display power domains, as those require support for the BLK_CTL
>> regions of the VPUMIX and DISPLAYMIX domains. A Linux driver for
>> those regions on the i.MX8MP is currently under development and we
>> plan to use this as a template for the i.MX8MM when the dust has
>> settled. The changes in this series have been made with this in
>> mind, so once the BLK_CTL driver exists it should be a matter of
>> hooking things together via DT, with no further changes required on
>> the GPCv2 driver side (famous last words).
>>
>> Special thanks to Marek Vasut who helped with testing and debugging
>> of early versions of this code.
>>
>>
> I fully support trying to get this done.  I tried to push something this
> before, but I was turned down by someone at NXP [1].  I have both a Mini
> and Nano board at my disposal, so if Nano support isn't there yet, and this
> driver patch gets accepted, I'll gladly work to help and/or test
> functionality.
> 
> [1] - https://lkml.org/lkml/2020/4/30/540

Note that the [1] is using poorly defined and buggy SMC interface to
TFA, I agree with Lucas that is not the way to go for MX8M. This
patchset is not using that SMC interface at all, but rather lets Linux
do the power domain handling, which I think is much better.

Re: [PATCH 00/11] i.MX8MM power domain support

[Adam Ford]

On Sat, Oct 3, 2020 at 1:08 PM Marek Vasut <marex@denx.de> wrote:
>
> On 10/3/20 8:01 PM, Adam Ford wrote:
> > On Wed, Sep 30, 2020 at 10:50 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> >
> >> Hi all,
> >>
> >> this adds power domain support for the i.MX8MM to the existing GPCv2
> >> driver. It is not complete yet, as it is still missing the VPU and
> >> display power domains, as those require support for the BLK_CTL
> >> regions of the VPUMIX and DISPLAYMIX domains. A Linux driver for
> >> those regions on the i.MX8MP is currently under development and we
> >> plan to use this as a template for the i.MX8MM when the dust has
> >> settled. The changes in this series have been made with this in
> >> mind, so once the BLK_CTL driver exists it should be a matter of
> >> hooking things together via DT, with no further changes required on
> >> the GPCv2 driver side (famous last words).
> >>
> >> Special thanks to Marek Vasut who helped with testing and debugging
> >> of early versions of this code.
> >>
> >>
> > I fully support trying to get this done.  I tried to push something this
> > before, but I was turned down by someone at NXP [1].  I have both a Mini
> > and Nano board at my disposal, so if Nano support isn't there yet, and this
> > driver patch gets accepted, I'll gladly work to help and/or test
> > functionality.
> >
> > [1] - https://lkml.org/lkml/2020/4/30/540
>
> Note that the [1] is using poorly defined and buggy SMC interface to
> TFA, I agree with Lucas that is not the way to go for MX8M. This
> patchset is not using that SMC interface at all, but rather lets Linux
> do the power domain handling, which I think is much better.

I hope you're more successful in convincing NXP of that than Lucas and
I were before.  Our pleas fell on deaf ears.

adam

Re: [PATCH 00/11] i.MX8MM power domain support

[Adam Ford]

On Wed, Sep 30, 2020 at 10:50 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi all,
>
> this adds power domain support for the i.MX8MM to the existing GPCv2
> driver. It is not complete yet, as it is still missing the VPU and
> display power domains, as those require support for the BLK_CTL
> regions of the VPUMIX and DISPLAYMIX domains. A Linux driver for
> those regions on the i.MX8MP is currently under development and we
> plan to use this as a template for the i.MX8MM when the dust has
> settled. The changes in this series have been made with this in
> mind, so once the BLK_CTL driver exists it should be a matter of
> hooking things together via DT, with no further changes required on
> the GPCv2 driver side (famous last words).

Thanks for improving this.

With this series, I was able to test the USB and it is functional.
Without it, the system would hang when attempting to use USB.

>
> Special thanks to Marek Vasut who helped with testing and debugging
> of early versions of this code.
>

For the series:

Tested-by: Adam Ford <aford173@gmail.com>

> Regards,
> Lucas
>
> Lucas Stach (11):
>   soc: imx: gpcv2: move to more ideomatic error handling in probe
>   soc: imx: gpcv2: move domain mapping to domain driver probe
>   soc: imx: gpcv2: split power up and power down sequence control
>   soc: imx: gpcv2: wait for ADB400 handshake
>   soc: imx: gpcv2: add runtime PM support for power-domains
>   soc: imx: gpcv2: allow domains without power-sequence control
>   soc: imx: gpcv2: add support for optional resets
>   dt-bindings: add defines for i.MX8MM power domains
>   soc: imx: gpcv2: add support for i.MX8MM power domains
>   arm64: dts: imx8mm: add GPC node and power domains
>   arm64: dts: imx8mm: put USB controllers into power-domains
>
>  .../bindings/power/fsl,imx-gpcv2.yaml         |   8 +
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  59 +++
>  drivers/soc/imx/gpcv2.c                       | 501 +++++++++++++++---
>  include/dt-bindings/power/imx8mm-power.h      |  22 +
>  4 files changed, 516 insertions(+), 74 deletions(-)
>  create mode 100644 include/dt-bindings/power/imx8mm-power.h
>
> --
> 2.20.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 00/11] i.MX8MM power domain support

[Jacky Bai]

> -----Original Message-----
> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Wednesday, September 30, 2020 11:50 PM
> To: Shawn Guo <shawnguo@kernel.org>; Rob Herring <robh+dt@kernel.org>
> Cc: dl-linux-imx <linux-imx@nxp.com>; Fabio Estevam
> <festevam@gmail.com>; Frieder Schrempf <frieder.schrempf@kontron.de>;
> Marek Vasut <marex@denx.de>; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; kernel@pengutronix.de;
> patchwork-lst@pengutronix.de
> Subject: [PATCH 00/11] i.MX8MM power domain support
> 
> Hi all,
> 
> this adds power domain support for the i.MX8MM to the existing GPCv2 driver.
> It is not complete yet, as it is still missing the VPU and display power domains,
> as those require support for the BLK_CTL regions of the VPUMIX and
> DISPLAYMIX domains. A Linux driver for those regions on the i.MX8MP is
> currently under development and we plan to use this as a template for the
> i.MX8MM when the dust has settled. The changes in this series have been
> made with this in mind, so once the BLK_CTL driver exists it should be a
> matter of hooking things together via DT, with no further changes required on
> the GPCv2 driver side (famous last words).
> 
> Special thanks to Marek Vasut who helped with testing and debugging of early
> versions of this code.
> 

Lucas,

thanks for working on this, but I think current support for 8MM can NOT 100% work due to HW limitation.
Maybe, we need further discussion before moving forward, otherwise, we will meet awkward situation when NXP
doing LTS upgrade. Below are some info shared.

1. The GPU & VPU related power domains need to do special handling due to HW limitation, can refer to the power domain sequence
  In NXP release.
2. another reason that we do power domain control in TF-A in NXP release is that MAIN NOC power domain can only be controlled by
  TF-A, and before MAIN NOC power domain, we need to check other MIXs' power status. If other power domain is controlled by linux side,
  It is not easy to cross world status sync.
3. either 8MM, 8MN, or 8MP, the power domain design is different, I am not sure if it is the good to add hundreds line of code in GPCv2 each time
  a new SOC is added.

BR
Jacky Bai

> Regards,
> Lucas
> 
> Lucas Stach (11):
>   soc: imx: gpcv2: move to more ideomatic error handling in probe
>   soc: imx: gpcv2: move domain mapping to domain driver probe
>   soc: imx: gpcv2: split power up and power down sequence control
>   soc: imx: gpcv2: wait for ADB400 handshake
>   soc: imx: gpcv2: add runtime PM support for power-domains
>   soc: imx: gpcv2: allow domains without power-sequence control
>   soc: imx: gpcv2: add support for optional resets
>   dt-bindings: add defines for i.MX8MM power domains
>   soc: imx: gpcv2: add support for i.MX8MM power domains
>   arm64: dts: imx8mm: add GPC node and power domains
>   arm64: dts: imx8mm: put USB controllers into power-domains
> 
>  .../bindings/power/fsl,imx-gpcv2.yaml         |   8 +
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  59 +++
>  drivers/soc/imx/gpcv2.c                       | 501
> +++++++++++++++---
>  include/dt-bindings/power/imx8mm-power.h      |  22 +
>  4 files changed, 516 insertions(+), 74 deletions(-)  create mode 100644
> include/dt-bindings/power/imx8mm-power.h
> 
> --
> 2.20.1

Re: [PATCH 00/11] i.MX8MM power domain support

[Lucas Stach]

Hi Jacky,

On Fr, 2020-10-09 at 03:00 +0000, Jacky Bai wrote:
> > -----Original Message-----
> > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > Sent: Wednesday, September 30, 2020 11:50 PM
> > To: Shawn Guo <shawnguo@kernel.org>; Rob Herring <robh+dt@kernel.org>
> > Cc: dl-linux-imx <linux-imx@nxp.com>; Fabio Estevam
> > <festevam@gmail.com>; Frieder Schrempf <frieder.schrempf@kontron.de>;
> > Marek Vasut <marex@denx.de>; linux-arm-kernel@lists.infradead.org;
> > devicetree@vger.kernel.org; kernel@pengutronix.de;
> > patchwork-lst@pengutronix.de
> > Subject: [PATCH 00/11] i.MX8MM power domain support
> > 
> > Hi all,
> > 
> > this adds power domain support for the i.MX8MM to the existing GPCv2 driver.
> > It is not complete yet, as it is still missing the VPU and display power domains,
> > as those require support for the BLK_CTL regions of the VPUMIX and
> > DISPLAYMIX domains. A Linux driver for those regions on the i.MX8MP is
> > currently under development and we plan to use this as a template for the
> > i.MX8MM when the dust has settled. The changes in this series have been
> > made with this in mind, so once the BLK_CTL driver exists it should be a
> > matter of hooking things together via DT, with no further changes required on
> > the GPCv2 driver side (famous last words).
> > 
> > Special thanks to Marek Vasut who helped with testing and debugging of early
> > versions of this code.
> > 
> 
> Lucas,
> 
> thanks for working on this, but I think current support for 8MM can NOT 100% work due to HW limitation.
> Maybe, we need further discussion before moving forward, otherwise, we will meet awkward situation when NXP
> doing LTS upgrade. Below are some info shared.
> 
> 1. The GPU & VPU related power domains need to do special handling due to HW limitation, can refer to the power domain sequence
>   In NXP release.

For the GPU this driver already does the same thing as the TF-A based
implementation by driving the GPU2D and GPU3D domains together and
triggering the SRC reset.

For the VPU I expect that we can do all the necessary syncing with a
proper VPU BLK_CTL driver.

> 2. another reason that we do power domain control in TF-A in NXP release is that MAIN NOC power domain can only be controlled by
>   TF-A, and before MAIN NOC power domain, we need to check other MIXs' power status. If other power domain is controlled by linux side,
>   It is not easy to cross world status sync.

This is a valid concern and I want to learn more about this. When do
you turn off MAIN NOC power in the TF-A? Is it just system suspend? If
so I think it's a valid requirement for the kernel driver to shut down
all the peripheral power domains before entering system suspend.

> 3. either 8MM, 8MN, or 8MP, the power domain design is different, I am not sure if it is the good to add hundreds line of code in GPCv2 each time
>   a new SOC is added.

I don't buy into this argument. We have lots of drivers in the Linux
kernel that require some changes for new SoC generations, that's what
Linux drivers are for. The complexity of the hardware doesn't disappear
just because you push some of the driver bits into TF-A, you just
handle the complexity at a different palce and IMHO that the wrong
place. The power domains have complex interactions with other drivers
in the Linux system, so debugging and deplyong fixes is much easier
when the power domain handling is fully done by a kernel driver.

Regards,
Lucas

> BR
> Jacky Bai
> 
> > Regards,
> > Lucas
> > 
> > Lucas Stach (11):
> >   soc: imx: gpcv2: move to more ideomatic error handling in probe
> >   soc: imx: gpcv2: move domain mapping to domain driver probe
> >   soc: imx: gpcv2: split power up and power down sequence control
> >   soc: imx: gpcv2: wait for ADB400 handshake
> >   soc: imx: gpcv2: add runtime PM support for power-domains
> >   soc: imx: gpcv2: allow domains without power-sequence control
> >   soc: imx: gpcv2: add support for optional resets
> >   dt-bindings: add defines for i.MX8MM power domains
> >   soc: imx: gpcv2: add support for i.MX8MM power domains
> >   arm64: dts: imx8mm: add GPC node and power domains
> >   arm64: dts: imx8mm: put USB controllers into power-domains
> > 
> >  .../bindings/power/fsl,imx-gpcv2.yaml         |   8 +
> >  arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  59 +++
> >  drivers/soc/imx/gpcv2.c                       | 501
> > +++++++++++++++---
> >  include/dt-bindings/power/imx8mm-power.h      |  22 +
> >  4 files changed, 516 insertions(+), 74 deletions(-)  create mode 100644
> > include/dt-bindings/power/imx8mm-power.h
> > 
> > --
> > 2.20.1

Re: [PATCH 00/11] i.MX8MM power domain support

[Adam Ford]

On Fri, Oct 9, 2020 at 6:16 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi Jacky,
>
> On Fr, 2020-10-09 at 03:00 +0000, Jacky Bai wrote:
> > > -----Original Message-----
> > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > Sent: Wednesday, September 30, 2020 11:50 PM
> > > To: Shawn Guo <shawnguo@kernel.org>; Rob Herring <robh+dt@kernel.org>
> > > Cc: dl-linux-imx <linux-imx@nxp.com>; Fabio Estevam
> > > <festevam@gmail.com>; Frieder Schrempf <frieder.schrempf@kontron.de>;
> > > Marek Vasut <marex@denx.de>; linux-arm-kernel@lists.infradead.org;
> > > devicetree@vger.kernel.org; kernel@pengutronix.de;
> > > patchwork-lst@pengutronix.de
> > > Subject: [PATCH 00/11] i.MX8MM power domain support
> > >
> > > Hi all,
> > >
> > > this adds power domain support for the i.MX8MM to the existing GPCv2 driver.
> > > It is not complete yet, as it is still missing the VPU and display power domains,
> > > as those require support for the BLK_CTL regions of the VPUMIX and
> > > DISPLAYMIX domains. A Linux driver for those regions on the i.MX8MP is
> > > currently under development and we plan to use this as a template for the
> > > i.MX8MM when the dust has settled. The changes in this series have been
> > > made with this in mind, so once the BLK_CTL driver exists it should be a
> > > matter of hooking things together via DT, with no further changes required on
> > > the GPCv2 driver side (famous last words).
> > >
> > > Special thanks to Marek Vasut who helped with testing and debugging of early
> > > versions of this code.
> > >
> >
> > Lucas,
> >
> > thanks for working on this, but I think current support for 8MM can NOT 100% work due to HW limitation.
> > Maybe, we need further discussion before moving forward, otherwise, we will meet awkward situation when NXP
> > doing LTS upgrade. Below are some info shared.
> >
> > 1. The GPU & VPU related power domains need to do special handling due to HW limitation, can refer to the power domain sequence
> >   In NXP release.
>
> For the GPU this driver already does the same thing as the TF-A based
> implementation by driving the GPU2D and GPU3D domains together and
> triggering the SRC reset.
>
> For the VPU I expect that we can do all the necessary syncing with a
> proper VPU BLK_CTL driver.
>
> > 2. another reason that we do power domain control in TF-A in NXP release is that MAIN NOC power domain can only be controlled by
> >   TF-A, and before MAIN NOC power domain, we need to check other MIXs' power status. If other power domain is controlled by linux side,
> >   It is not easy to cross world status sync.
>
> This is a valid concern and I want to learn more about this. When do
> you turn off MAIN NOC power in the TF-A? Is it just system suspend? If
> so I think it's a valid requirement for the kernel driver to shut down
> all the peripheral power domains before entering system suspend.
>
> > 3. either 8MM, 8MN, or 8MP, the power domain design is different, I am not sure if it is the good to add hundreds line of code in GPCv2 each time
> >   a new SOC is added.
>
> I don't buy into this argument. We have lots of drivers in the Linux
> kernel that require some changes for new SoC generations, that's what
> Linux drivers are for. The complexity of the hardware doesn't disappear
> just because you push some of the driver bits into TF-A, you just
> handle the complexity at a different palce and IMHO that the wrong
> place. The power domains have complex interactions with other drivers
> in the Linux system, so debugging and deplyong fixes is much easier
> when the power domain handling is fully done by a kernel driver.

In an effort to keep the code size manageable, what if we were to
propose a gpc-core configured to be a generic function common to all
SoC's, and move the tables for each unique SoC into separate files.
Making each SoC's GPC a Kconfig option could give people the ability
to disable the various options that don't apply to their specific
application, and the setup and configuration of the tables should be
easier to read.  I know of at least one touchscreen driver that does
this (tsc200x).

adam
>
> Regards,
> Lucas
>
> > BR
> > Jacky Bai
> >
> > > Regards,
> > > Lucas
> > >
> > > Lucas Stach (11):
> > >   soc: imx: gpcv2: move to more ideomatic error handling in probe
> > >   soc: imx: gpcv2: move domain mapping to domain driver probe
> > >   soc: imx: gpcv2: split power up and power down sequence control
> > >   soc: imx: gpcv2: wait for ADB400 handshake
> > >   soc: imx: gpcv2: add runtime PM support for power-domains
> > >   soc: imx: gpcv2: allow domains without power-sequence control
> > >   soc: imx: gpcv2: add support for optional resets
> > >   dt-bindings: add defines for i.MX8MM power domains
> > >   soc: imx: gpcv2: add support for i.MX8MM power domains
> > >   arm64: dts: imx8mm: add GPC node and power domains
> > >   arm64: dts: imx8mm: put USB controllers into power-domains
> > >
> > >  .../bindings/power/fsl,imx-gpcv2.yaml         |   8 +
> > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  59 +++
> > >  drivers/soc/imx/gpcv2.c                       | 501
> > > +++++++++++++++---
> > >  include/dt-bindings/power/imx8mm-power.h      |  22 +
> > >  4 files changed, 516 insertions(+), 74 deletions(-)  create mode 100644
> > > include/dt-bindings/power/imx8mm-power.h
> > >
> > > --
> > > 2.20.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 00/11] i.MX8MM power domain support

[Jacky Bai]

> -----Original Message-----
> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Friday, October 9, 2020 7:12 PM
> To: Jacky Bai <ping.bai@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Rob
> Herring <robh+dt@kernel.org>
> Cc: dl-linux-imx <linux-imx@nxp.com>; Fabio Estevam
> <festevam@gmail.com>; Frieder Schrempf <frieder.schrempf@kontron.de>;
> Marek Vasut <marex@denx.de>; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; kernel@pengutronix.de;
> patchwork-lst@pengutronix.de
> Subject: Re: [PATCH 00/11] i.MX8MM power domain support
> 
> Hi Jacky,
> 
> On Fr, 2020-10-09 at 03:00 +0000, Jacky Bai wrote:
> > > -----Original Message-----
> > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > Sent: Wednesday, September 30, 2020 11:50 PM
> > > To: Shawn Guo <shawnguo@kernel.org>; Rob Herring
> > > <robh+dt@kernel.org>
> > > Cc: dl-linux-imx <linux-imx@nxp.com>; Fabio Estevam
> > > <festevam@gmail.com>; Frieder Schrempf
> > > <frieder.schrempf@kontron.de>; Marek Vasut <marex@denx.de>;
> > > linux-arm-kernel@lists.infradead.org;
> > > devicetree@vger.kernel.org; kernel@pengutronix.de;
> > > patchwork-lst@pengutronix.de
> > > Subject: [PATCH 00/11] i.MX8MM power domain support
> > >
> > > Hi all,
> > >
> > > this adds power domain support for the i.MX8MM to the existing GPCv2
> driver.
> > > It is not complete yet, as it is still missing the VPU and display
> > > power domains, as those require support for the BLK_CTL regions of
> > > the VPUMIX and DISPLAYMIX domains. A Linux driver for those regions
> > > on the i.MX8MP is currently under development and we plan to use
> > > this as a template for the i.MX8MM when the dust has settled. The
> > > changes in this series have been made with this in mind, so once the
> > > BLK_CTL driver exists it should be a matter of hooking things
> > > together via DT, with no further changes required on the GPCv2 driver side
> (famous last words).
> > >
> > > Special thanks to Marek Vasut who helped with testing and debugging
> > > of early versions of this code.
> > >
> >
> > Lucas,
> >
> > thanks for working on this, but I think current support for 8MM can NOT 100%
> work due to HW limitation.
> > Maybe, we need further discussion before moving forward, otherwise, we
> > will meet awkward situation when NXP doing LTS upgrade. Below are some
> info shared.
> >
> > 1. The GPU & VPU related power domains need to do special handling due
> to HW limitation, can refer to the power domain sequence
> >   In NXP release.
> 
> For the GPU this driver already does the same thing as the TF-A based
> implementation by driving the GPU2D and GPU3D domains together and
> triggering the SRC reset.
> 
> For the VPU I expect that we can do all the necessary syncing with a proper
> VPU BLK_CTL driver.
> 

Ok, thanks. I saw the reset handling in this patchset.

> > 2. another reason that we do power domain control in TF-A in NXP release is
> that MAIN NOC power domain can only be controlled by
> >   TF-A, and before MAIN NOC power domain, we need to check other
> MIXs' power status. If other power domain is controlled by linux side,
> >   It is not easy to cross world status sync.
> 
> This is a valid concern and I want to learn more about this. When do you turn
> off MAIN NOC power in the TF-A? Is it just system suspend? If so I think it's a
> valid requirement for the kernel driver to shut down all the peripheral power
> domains before entering system suspend.
> 

The main NOC will be off just in system suspend case. Main NoC on/off is controlled by
GPC HW slot method. As all the MIXs with ADB400 bridge are connected on the main NoC,
we must make sure that all these ADB400 port in power down status when main NoC power off
in system suspend, otherwise system will hang when resume. Previously, all the MIX power domain
is controlled by TF-A, then TF-A knows the status of each MIX, if any MIX is on, we will skip the NOC
power down setting.

> > 3. either 8MM, 8MN, or 8MP, the power domain design is different, I am not
> sure if it is the good to add hundreds line of code in GPCv2 each time
> >   a new SOC is added.
> 
> I don't buy into this argument. We have lots of drivers in the Linux kernel that
> require some changes for new SoC generations, that's what Linux drivers are
> for. The complexity of the hardware doesn't disappear just because you push
> some of the driver bits into TF-A, you just handle the complexity at a different
> palce and IMHO that the wrong place. The power domains have complex
> interactions with other drivers in the Linux system, so debugging and
> deplyong fixes is much easier when the power domain handling is fully done
> by a kernel driver.

Actually, due to the security requirement from other system solution provider,
for example, Microsoft Azure Sphere, it has strict requirement for power domain
to be controlled by secure subsystem(either TF-A, TEE or dedicated secure domain controller).
Same requirement for reset control, and system critical clock control.

For NXP i.MX8M family, it is ok to implement in linux kernel, just a tradeoff to find out a place
to hide the complexity ^_^.

BTW, for virtualization support, it is better to put the power domain in a central place to simplify
the VM implementation.

BR
Jacky Bai

> 
> Regards,
> Lucas
> 
> > BR
> > Jacky Bai
> >
> > > Regards,
> > > Lucas
> > >
> > > Lucas Stach (11):
> > >   soc: imx: gpcv2: move to more ideomatic error handling in probe
> > >   soc: imx: gpcv2: move domain mapping to domain driver probe
> > >   soc: imx: gpcv2: split power up and power down sequence control
> > >   soc: imx: gpcv2: wait for ADB400 handshake
> > >   soc: imx: gpcv2: add runtime PM support for power-domains
> > >   soc: imx: gpcv2: allow domains without power-sequence control
> > >   soc: imx: gpcv2: add support for optional resets
> > >   dt-bindings: add defines for i.MX8MM power domains
> > >   soc: imx: gpcv2: add support for i.MX8MM power domains
> > >   arm64: dts: imx8mm: add GPC node and power domains
> > >   arm64: dts: imx8mm: put USB controllers into power-domains
> > >
> > >  .../bindings/power/fsl,imx-gpcv2.yaml         |   8 +
> > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  59 +++
> > >  drivers/soc/imx/gpcv2.c                       | 501
> > > +++++++++++++++---
> > >  include/dt-bindings/power/imx8mm-power.h      |  22 +
> > >  4 files changed, 516 insertions(+), 74 deletions(-)  create mode
> > > 100644 include/dt-bindings/power/imx8mm-power.h
> > >
> > > --
> > > 2.20.1

Re: [PATCH 00/11] i.MX8MM power domain support

[Lucas Stach]

On Sa, 2020-10-10 at 02:16 +0000, Jacky Bai wrote:
> > -----Original Message-----
> > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > Sent: Friday, October 9, 2020 7:12 PM
> > To: Jacky Bai <ping.bai@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Rob
> > Herring <robh+dt@kernel.org>
> > Cc: dl-linux-imx <linux-imx@nxp.com>; Fabio Estevam
> > <festevam@gmail.com>; Frieder Schrempf <frieder.schrempf@kontron.de>;
> > Marek Vasut <marex@denx.de>; linux-arm-kernel@lists.infradead.org;
> > devicetree@vger.kernel.org; kernel@pengutronix.de;
> > patchwork-lst@pengutronix.de
> > Subject: Re: [PATCH 00/11] i.MX8MM power domain support
> > 
> > Hi Jacky,
> > 
> > On Fr, 2020-10-09 at 03:00 +0000, Jacky Bai wrote:
> > > > -----Original Message-----
> > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > > Sent: Wednesday, September 30, 2020 11:50 PM
> > > > To: Shawn Guo <shawnguo@kernel.org>; Rob Herring
> > > > <robh+dt@kernel.org>
> > > > Cc: dl-linux-imx <linux-imx@nxp.com>; Fabio Estevam
> > > > <festevam@gmail.com>; Frieder Schrempf
> > > > <frieder.schrempf@kontron.de>; Marek Vasut <marex@denx.de>;
> > > > linux-arm-kernel@lists.infradead.org;
> > > > devicetree@vger.kernel.org; kernel@pengutronix.de;
> > > > patchwork-lst@pengutronix.de
> > > > Subject: [PATCH 00/11] i.MX8MM power domain support
> > > > 
> > > > Hi all,
> > > > 
> > > > this adds power domain support for the i.MX8MM to the existing GPCv2
> > driver.
> > > > It is not complete yet, as it is still missing the VPU and display
> > > > power domains, as those require support for the BLK_CTL regions of
> > > > the VPUMIX and DISPLAYMIX domains. A Linux driver for those regions
> > > > on the i.MX8MP is currently under development and we plan to use
> > > > this as a template for the i.MX8MM when the dust has settled. The
> > > > changes in this series have been made with this in mind, so once the
> > > > BLK_CTL driver exists it should be a matter of hooking things
> > > > together via DT, with no further changes required on the GPCv2 driver side
> > (famous last words).
> > > > Special thanks to Marek Vasut who helped with testing and debugging
> > > > of early versions of this code.
> > > > 
> > > 
> > > Lucas,
> > > 
> > > thanks for working on this, but I think current support for 8MM can NOT 100%
> > work due to HW limitation.
> > > Maybe, we need further discussion before moving forward, otherwise, we
> > > will meet awkward situation when NXP doing LTS upgrade. Below are some
> > info shared.
> > > 1. The GPU & VPU related power domains need to do special handling due
> > to HW limitation, can refer to the power domain sequence
> > >   In NXP release.
> > 
> > For the GPU this driver already does the same thing as the TF-A based
> > implementation by driving the GPU2D and GPU3D domains together and
> > triggering the SRC reset.
> > 
> > For the VPU I expect that we can do all the necessary syncing with a proper
> > VPU BLK_CTL driver.
> > 
> 
> Ok, thanks. I saw the reset handling in this patchset.
> 
> > > 2. another reason that we do power domain control in TF-A in NXP release is
> > that MAIN NOC power domain can only be controlled by
> > >   TF-A, and before MAIN NOC power domain, we need to check other
> > MIXs' power status. If other power domain is controlled by linux side,
> > >   It is not easy to cross world status sync.
> > 
> > This is a valid concern and I want to learn more about this. When do you turn
> > off MAIN NOC power in the TF-A? Is it just system suspend? If so I think it's a
> > valid requirement for the kernel driver to shut down all the peripheral power
> > domains before entering system suspend.
> > 
> 
> The main NOC will be off just in system suspend case. Main NoC on/off is controlled by
> GPC HW slot method. As all the MIXs with ADB400 bridge are connected on the main NoC,
> we must make sure that all these ADB400 port in power down status when main NoC power off
> in system suspend, otherwise system will hang when resume. Previously, all the MIX power domain
> is controlled by TF-A, then TF-A knows the status of each MIX, if any MIX is on, we will skip the NOC
> power down setting.

And that just means the SoC doesn't enter the desired low-power state
when going into suspend, right? I think it would be much easier and
less error prone to just mandate that all peripheral power domains must
be powered down before trying to enter system suspend.

Is there any use-case where you would legitimately want to enter a high
power suspend state with some of ther peripheral power domains still
active?

> > > 3. either 8MM, 8MN, or 8MP, the power domain design is different, I am not
> > sure if it is the good to add hundreds line of code in GPCv2 each time
> > >   a new SOC is added.
> > 
> > I don't buy into this argument. We have lots of drivers in the Linux kernel that
> > require some changes for new SoC generations, that's what Linux drivers are
> > for. The complexity of the hardware doesn't disappear just because you push
> > some of the driver bits into TF-A, you just handle the complexity at a different
> > palce and IMHO that the wrong place. The power domains have complex
> > interactions with other drivers in the Linux system, so debugging and
> > deplyong fixes is much easier when the power domain handling is fully done
> > by a kernel driver.
> 
> Actually, due to the security requirement from other system solution provider,
> for example, Microsoft Azure Sphere, it has strict requirement for power domain
> to be controlled by secure subsystem(either TF-A, TEE or dedicated secure domain controller).
> Same requirement for reset control, and system critical clock control.

Yes, I'm aware of those requirements, but to satisfy those you need a
full implementation of all those parts in the secure subsystem. Doing
it just for the power domains adds complexity for no gain, as you still
won't be able to meet all the requirements and frankly I don't think
this is a realistic goal to achieve with the current i.MX8M family of
SoCs. 

Meeting those requirements needs a fully system approach where the
secure subsystem parts are made sufficiently independent from the non-
secure parts on a hardware level, which I don't see on the i.MX8M SoC
and hardware design guide.

> For NXP i.MX8M family, it is ok to implement in linux kernel, just a tradeoff to find out a place
> to hide the complexity ^_^.
> 
> BTW, for virtualization support, it is better to put the power domain in a central place to simplify
> the VM implementation.

Same as above. If you can make all the relevant bits (clock, reset,
power-domain, regulator) available via a virtualization friendly API,
then I would see a point in adding complexity for this abstraction. As
long as this added abstraction only solves a very tiny bit of the
overall picture, I just don't see the point in the added complexity and
(from a Linux PoV) obfuscation.

Regards,
Lucas

> BR
> Jacky Bai
> 
> > Regards,
> > Lucas
> > 
> > > BR
> > > Jacky Bai
> > > 
> > > > Regards,
> > > > Lucas
> > > > 
> > > > Lucas Stach (11):
> > > >   soc: imx: gpcv2: move to more ideomatic error handling in probe
> > > >   soc: imx: gpcv2: move domain mapping to domain driver probe
> > > >   soc: imx: gpcv2: split power up and power down sequence control
> > > >   soc: imx: gpcv2: wait for ADB400 handshake
> > > >   soc: imx: gpcv2: add runtime PM support for power-domains
> > > >   soc: imx: gpcv2: allow domains without power-sequence control
> > > >   soc: imx: gpcv2: add support for optional resets
> > > >   dt-bindings: add defines for i.MX8MM power domains
> > > >   soc: imx: gpcv2: add support for i.MX8MM power domains
> > > >   arm64: dts: imx8mm: add GPC node and power domains
> > > >   arm64: dts: imx8mm: put USB controllers into power-domains
> > > > 
> > > >  .../bindings/power/fsl,imx-gpcv2.yaml         |   8 +
> > > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  59 +++
> > > >  drivers/soc/imx/gpcv2.c                       | 501
> > > > +++++++++++++++---
> > > >  include/dt-bindings/power/imx8mm-power.h      |  22 +
> > > >  4 files changed, 516 insertions(+), 74 deletions(-)  create mode
> > > > 100644 include/dt-bindings/power/imx8mm-power.h
> > > > 
> > > > --
> > > > 2.20.1

RE: [PATCH 00/11] i.MX8MM power domain support

[Peng Fan]

> Subject: Re: [PATCH 00/11] i.MX8MM power domain support
> 
> On Sa, 2020-10-10 at 02:16 +0000, Jacky Bai wrote:
> > > -----Original Message-----
> > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > Sent: Friday, October 9, 2020 7:12 PM
> > > To: Jacky Bai <ping.bai@nxp.com>; Shawn Guo <shawnguo@kernel.org>;
> > > Rob Herring <robh+dt@kernel.org>
> > > Cc: dl-linux-imx <linux-imx@nxp.com>; Fabio Estevam
> > > <festevam@gmail.com>; Frieder Schrempf
> > > <frieder.schrempf@kontron.de>; Marek Vasut <marex@denx.de>;
> > > linux-arm-kernel@lists.infradead.org;
> > > devicetree@vger.kernel.org; kernel@pengutronix.de;
> > > patchwork-lst@pengutronix.de
> > > Subject: Re: [PATCH 00/11] i.MX8MM power domain support
> > >
> > > Hi Jacky,
> > >
> > > On Fr, 2020-10-09 at 03:00 +0000, Jacky Bai wrote:
> > > > > -----Original Message-----
> > > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > > > Sent: Wednesday, September 30, 2020 11:50 PM
> > > > > To: Shawn Guo <shawnguo@kernel.org>; Rob Herring
> > > > > <robh+dt@kernel.org>
> > > > > Cc: dl-linux-imx <linux-imx@nxp.com>; Fabio Estevam
> > > > > <festevam@gmail.com>; Frieder Schrempf
> > > > > <frieder.schrempf@kontron.de>; Marek Vasut <marex@denx.de>;
> > > > > linux-arm-kernel@lists.infradead.org;
> > > > > devicetree@vger.kernel.org; kernel@pengutronix.de;
> > > > > patchwork-lst@pengutronix.de
> > > > > Subject: [PATCH 00/11] i.MX8MM power domain support
> > > > >
> > > > > Hi all,
> > > > >
> > > > > this adds power domain support for the i.MX8MM to the existing
> > > > > GPCv2
> > > driver.
> > > > > It is not complete yet, as it is still missing the VPU and
> > > > > display power domains, as those require support for the BLK_CTL
> > > > > regions of the VPUMIX and DISPLAYMIX domains. A Linux driver for
> > > > > those regions on the i.MX8MP is currently under development and
> > > > > we plan to use this as a template for the i.MX8MM when the dust
> > > > > has settled. The changes in this series have been made with this
> > > > > in mind, so once the BLK_CTL driver exists it should be a matter
> > > > > of hooking things together via DT, with no further changes
> > > > > required on the GPCv2 driver side
> > > (famous last words).
> > > > > Special thanks to Marek Vasut who helped with testing and
> > > > > debugging of early versions of this code.
> > > > >
> > > >
> > > > Lucas,
> > > >
> > > > thanks for working on this, but I think current support for 8MM
> > > > can NOT 100%
> > > work due to HW limitation.
> > > > Maybe, we need further discussion before moving forward,
> > > > otherwise, we will meet awkward situation when NXP doing LTS
> > > > upgrade. Below are some
> > > info shared.
> > > > 1. The GPU & VPU related power domains need to do special handling
> > > > due
> > > to HW limitation, can refer to the power domain sequence
> > > >   In NXP release.
> > >
> > > For the GPU this driver already does the same thing as the TF-A
> > > based implementation by driving the GPU2D and GPU3D domains
> together
> > > and triggering the SRC reset.
> > >
> > > For the VPU I expect that we can do all the necessary syncing with a
> > > proper VPU BLK_CTL driver.
> > >
> >
> > Ok, thanks. I saw the reset handling in this patchset.
> >
> > > > 2. another reason that we do power domain control in TF-A in NXP
> > > > release is
> > > that MAIN NOC power domain can only be controlled by
> > > >   TF-A, and before MAIN NOC power domain, we need to check other
> > > MIXs' power status. If other power domain is controlled by linux
> > > side,
> > > >   It is not easy to cross world status sync.
> > >
> > > This is a valid concern and I want to learn more about this. When do
> > > you turn off MAIN NOC power in the TF-A? Is it just system suspend?
> > > If so I think it's a valid requirement for the kernel driver to shut
> > > down all the peripheral power domains before entering system suspend.
> > >
> >
> > The main NOC will be off just in system suspend case. Main NoC on/off
> > is controlled by GPC HW slot method. As all the MIXs with ADB400
> > bridge are connected on the main NoC, we must make sure that all these
> > ADB400 port in power down status when main NoC power off in system
> > suspend, otherwise system will hang when resume. Previously, all the
> > MIX power domain is controlled by TF-A, then TF-A knows the status of each
> MIX, if any MIX is on, we will skip the NOC power down setting.
> 
> And that just means the SoC doesn't enter the desired low-power state when
> going into suspend, right? I think it would be much easier and less error prone
> to just mandate that all peripheral power domains must be powered down
> before trying to enter system suspend.
> 
> Is there any use-case where you would legitimately want to enter a high
> power suspend state with some of ther peripheral power domains still active?
> 
> > > > 3. either 8MM, 8MN, or 8MP, the power domain design is different,
> > > > I am not
> > > sure if it is the good to add hundreds line of code in GPCv2 each
> > > time
> > > >   a new SOC is added.
> > >
> > > I don't buy into this argument. We have lots of drivers in the Linux
> > > kernel that require some changes for new SoC generations, that's
> > > what Linux drivers are for. The complexity of the hardware doesn't
> > > disappear just because you push some of the driver bits into TF-A,
> > > you just handle the complexity at a different palce and IMHO that
> > > the wrong place. The power domains have complex interactions with
> > > other drivers in the Linux system, so debugging and deplyong fixes
> > > is much easier when the power domain handling is fully done by a kernel
> driver.
> >
> > Actually, due to the security requirement from other system solution
> > provider, for example, Microsoft Azure Sphere, it has strict
> > requirement for power domain to be controlled by secure subsystem(either
> TF-A, TEE or dedicated secure domain controller).
> > Same requirement for reset control, and system critical clock control.
> 
> Yes, I'm aware of those requirements, but to satisfy those you need a full
> implementation of all those parts in the secure subsystem. Doing it just for the
> power domains adds complexity for no gain, as you still won't be able to meet
> all the requirements and frankly I don't think this is a realistic goal to achieve
> with the current i.MX8M family of SoCs.

At least we are moving to that direction.

> 
> Meeting those requirements needs a fully system approach where the secure
> subsystem parts are made sufficiently independent from the non- secure
> parts on a hardware level, which I don't see on the i.MX8M SoC and hardware
> design guide.

CSU could restrict the access permission.

> 
> > For NXP i.MX8M family, it is ok to implement in linux kernel, just a
> > tradeoff to find out a place to hide the complexity ^_^.
> >
> > BTW, for virtualization support, it is better to put the power domain
> > in a central place to simplify the VM implementation.
> 
> Same as above. If you can make all the relevant bits (clock, reset,
> power-domain, regulator) available via a virtualization friendly API, then I
> would see a point in adding complexity for this abstraction. As long as this
> added abstraction only solves a very tiny bit of the overall picture, I just don't
> see the point in the added complexity and (from a Linux PoV) obfuscation.

Could we use SCMI for power domain, system critical clocks, smc watchdog
and etc?

Or we support two approaches, one is let Linux control everything, the other
is using SCMI.

Thoughts?

Thanks,
Peng.

> 
> Regards,
> Lucas
> 
> > BR
> > Jacky Bai
> >
> > > Regards,
> > > Lucas
> > >
> > > > BR
> > > > Jacky Bai
> > > >
> > > > > Regards,
> > > > > Lucas
> > > > >
> > > > > Lucas Stach (11):
> > > > >   soc: imx: gpcv2: move to more ideomatic error handling in probe
> > > > >   soc: imx: gpcv2: move domain mapping to domain driver probe
> > > > >   soc: imx: gpcv2: split power up and power down sequence control
> > > > >   soc: imx: gpcv2: wait for ADB400 handshake
> > > > >   soc: imx: gpcv2: add runtime PM support for power-domains
> > > > >   soc: imx: gpcv2: allow domains without power-sequence control
> > > > >   soc: imx: gpcv2: add support for optional resets
> > > > >   dt-bindings: add defines for i.MX8MM power domains
> > > > >   soc: imx: gpcv2: add support for i.MX8MM power domains
> > > > >   arm64: dts: imx8mm: add GPC node and power domains
> > > > >   arm64: dts: imx8mm: put USB controllers into power-domains
> > > > >
> > > > >  .../bindings/power/fsl,imx-gpcv2.yaml         |   8 +
> > > > >  arch/arm64/boot/dts/freescale/imx8mm.dtsi     |  59 +++
> > > > >  drivers/soc/imx/gpcv2.c                       | 501
> > > > > +++++++++++++++---
> > > > >  include/dt-bindings/power/imx8mm-power.h      |  22 +
> > > > >  4 files changed, 516 insertions(+), 74 deletions(-)  create
> > > > > mode
> > > > > 100644 include/dt-bindings/power/imx8mm-power.h
> > > > >
> > > > > --
> > > > > 2.20.1

Re: [PATCH 00/11] i.MX8MM power domain support

[Lucas Stach]

Hi Peng,

On Mi, 2020-10-14 at 01:23 +0000, Peng Fan wrote:
[...]
> > > > > 3. either 8MM, 8MN, or 8MP, the power domain design is different,
> > > > > I am not
> > > > sure if it is the good to add hundreds line of code in GPCv2 each
> > > > time
> > > > >   a new SOC is added.
> > > > 
> > > > I don't buy into this argument. We have lots of drivers in the Linux
> > > > kernel that require some changes for new SoC generations, that's
> > > > what Linux drivers are for. The complexity of the hardware doesn't
> > > > disappear just because you push some of the driver bits into TF-A,
> > > > you just handle the complexity at a different palce and IMHO that
> > > > the wrong place. The power domains have complex interactions with
> > > > other drivers in the Linux system, so debugging and deplyong fixes
> > > > is much easier when the power domain handling is fully done by a kernel
> > driver.
> > > Actually, due to the security requirement from other system solution
> > > provider, for example, Microsoft Azure Sphere, it has strict
> > > requirement for power domain to be controlled by secure subsystem(either
> > TF-A, TEE or dedicated secure domain controller).
> > > Same requirement for reset control, and system critical clock control.
> > 
> > Yes, I'm aware of those requirements, but to satisfy those you need a full
> > implementation of all those parts in the secure subsystem. Doing it just for the
> > power domains adds complexity for no gain, as you still won't be able to meet
> > all the requirements and frankly I don't think this is a realistic goal to achieve
> > with the current i.MX8M family of SoCs.
> 
> At least we are moving to that direction.

To me it seems like the current way (custom TF-A interface and
implementation) is one step in the right direction, but two steps
backwards in terms of complexity.

> > Meeting those requirements needs a fully system approach where the secure
> > subsystem parts are made sufficiently independent from the non- secure
> > parts on a hardware level, which I don't see on the i.MX8M SoC and hardware
> > design guide.
> 
> CSU could restrict the access permission.

While this is true, my argument is much broader and not only focused on
on-SoC peripherals. For example some of the power domains need
different voltages for specific performance states, which means you
need to communicate with a external PMIC or other voltage regulator,
which in turn means you need to set aside the necessary i2c bus and/or
GPIO banks required for this communication at system design time, so it
isn't shared between TF-A and the rich OS. I don't see this in any of
the i.MX8M designs.

> > > For NXP i.MX8M family, it is ok to implement in linux kernel, just a
> > > tradeoff to find out a place to hide the complexity ^_^.
> > > 
> > > BTW, for virtualization support, it is better to put the power domain
> > > in a central place to simplify the VM implementation.
> > 
> > Same as above. If you can make all the relevant bits (clock, reset,
> > power-domain, regulator) available via a virtualization friendly API, then I
> > would see a point in adding complexity for this abstraction. As long as this
> > added abstraction only solves a very tiny bit of the overall picture, I just don't
> > see the point in the added complexity and (from a Linux PoV) obfuscation.
> 
> Could we use SCMI for power domain, system critical clocks, smc watchdog
> and etc?

If you could demonstrate a working solution with all those pieces
hidden behind a standard SCMI interface, this would make for a much
more compelling story supporting the secure subsystem argument.

> Or we support two approaches, one is let Linux control everything, the other
> is using SCMI.
> 
> Thoughts?

I wouldn't be opposed to such a solution. If you can put all this
behind a standard SCMI interface, I guess we wouldn't need two
different SoC specific drivers for the same purpose, so we could easily
have a Linux full-control solution (i.e. this patchset) coexist with a
SCMI based implementation, possibly with just a slightly different base
SoC DT with all the power domains, clocks and other system level
control stuff behind SCMI.

What I'm strongly opposed to is having a custom TF-A interface and all
the added complexity for little to no gain in actual system security.

Regards,
Lucas

Re: [PATCH 00/11] i.MX8MM power domain support

[Fabio Estevam]

On Thu, Oct 22, 2020 at 5:24 AM Lucas Stach <l.stach@pengutronix.de> wrote:

> What I'm strongly opposed to is having a custom TF-A interface and all
> the added complexity for little to no gain in actual system security.

I agree with Lucas.

I would also prefer this series to be applied so that i.MX8MM upstream
progress does not get stalled any longer.

Thanks

RE: [PATCH 00/11] i.MX8MM power domain support

[Peng Fan]

Hi Lucas,

> Subject: Re: [PATCH 00/11] i.MX8MM power domain support
> 
> Hi Peng,
> 
> On Mi, 2020-10-14 at 01:23 +0000, Peng Fan wrote:
> [...]
> > > > > > 3. either 8MM, 8MN, or 8MP, the power domain design is
> > > > > > different, I am not
> > > > > sure if it is the good to add hundreds line of code in GPCv2
> > > > > each time
> > > > > >   a new SOC is added.
> > > > >
> > > > > I don't buy into this argument. We have lots of drivers in the
> > > > > Linux kernel that require some changes for new SoC generations,
> > > > > that's what Linux drivers are for. The complexity of the
> > > > > hardware doesn't disappear just because you push some of the
> > > > > driver bits into TF-A, you just handle the complexity at a
> > > > > different palce and IMHO that the wrong place. The power domains
> > > > > have complex interactions with other drivers in the Linux
> > > > > system, so debugging and deplyong fixes is much easier when the
> > > > > power domain handling is fully done by a kernel
> > > driver.
> > > > Actually, due to the security requirement from other system
> > > > solution provider, for example, Microsoft Azure Sphere, it has
> > > > strict requirement for power domain to be controlled by secure
> > > > subsystem(either
> > > TF-A, TEE or dedicated secure domain controller).
> > > > Same requirement for reset control, and system critical clock control.
> > >
> > > Yes, I'm aware of those requirements, but to satisfy those you need
> > > a full implementation of all those parts in the secure subsystem.
> > > Doing it just for the power domains adds complexity for no gain, as
> > > you still won't be able to meet all the requirements and frankly I
> > > don't think this is a realistic goal to achieve with the current i.MX8M
> family of SoCs.
> >
> > At least we are moving to that direction.
> 
> To me it seems like the current way (custom TF-A interface and
> implementation) is one step in the right direction, but two steps backwards in
> terms of complexity.
> 
> > > Meeting those requirements needs a fully system approach where the
> > > secure subsystem parts are made sufficiently independent from the
> > > non- secure parts on a hardware level, which I don't see on the
> > > i.MX8M SoC and hardware design guide.
> >
> > CSU could restrict the access permission.
> 
> While this is true, my argument is much broader and not only focused on
> on-SoC peripherals. For example some of the power domains need different
> voltages for specific performance states, which means you need to
> communicate with a external PMIC or other voltage regulator, which in turn
> means you need to set aside the necessary i2c bus and/or GPIO banks
> required for this communication at system design time, so it isn't shared
> between TF-A and the rich OS. I don't see this in any of the i.MX8M designs.
> 
> > > > For NXP i.MX8M family, it is ok to implement in linux kernel, just
> > > > a tradeoff to find out a place to hide the complexity ^_^.
> > > >
> > > > BTW, for virtualization support, it is better to put the power
> > > > domain in a central place to simplify the VM implementation.
> > >
> > > Same as above. If you can make all the relevant bits (clock, reset,
> > > power-domain, regulator) available via a virtualization friendly
> > > API, then I would see a point in adding complexity for this
> > > abstraction. As long as this added abstraction only solves a very
> > > tiny bit of the overall picture, I just don't see the point in the added
> complexity and (from a Linux PoV) obfuscation.
> >
> > Could we use SCMI for power domain, system critical clocks, smc
> > watchdog and etc?
> 
> If you could demonstrate a working solution with all those pieces hidden
> behind a standard SCMI interface, this would make for a much more
> compelling story supporting the secure subsystem argument.
> 
> > Or we support two approaches, one is let Linux control everything, the
> > other is using SCMI.
> >
> > Thoughts?
> 
> I wouldn't be opposed to such a solution. If you can put all this behind a
> standard SCMI interface, I guess we wouldn't need two different SoC specific
> drivers for the same purpose, so we could easily have a Linux full-control
> solution (i.e. this patchset) coexist with a SCMI based implementation,
> possibly with just a slightly different base SoC DT with all the power domains,
> clocks and other system level control stuff behind SCMI.
> 
> What I'm strongly opposed to is having a custom TF-A interface and all the
> added complexity for little to no gain in actual system security.

Understand. There are truly the SoC design might not fit well to protect
all the stuff.

It is good that you did this patchset. Vote for you to add more support
on i.MX.

Your patchset not conflict with SCMI, as you said, this is true.

Please continue your effort.

Thanks,
Peng.

> 
> Regards,
> Lucas

Re: [PATCH 00/11] i.MX8MM power domain support

[Adam Ford]

On Wed, Oct 28, 2020 at 8:51 AM Peng Fan <peng.fan@nxp.com> wrote:
>
> Hi Lucas,
>
> > Subject: Re: [PATCH 00/11] i.MX8MM power domain support
> >
> > Hi Peng,
> >
> > On Mi, 2020-10-14 at 01:23 +0000, Peng Fan wrote:
> > [...]
> > > > > > > 3. either 8MM, 8MN, or 8MP, the power domain design is
> > > > > > > different, I am not
> > > > > > sure if it is the good to add hundreds line of code in GPCv2
> > > > > > each time
> > > > > > >   a new SOC is added.
> > > > > >
> > > > > > I don't buy into this argument. We have lots of drivers in the
> > > > > > Linux kernel that require some changes for new SoC generations,
> > > > > > that's what Linux drivers are for. The complexity of the
> > > > > > hardware doesn't disappear just because you push some of the
> > > > > > driver bits into TF-A, you just handle the complexity at a
> > > > > > different palce and IMHO that the wrong place. The power domains
> > > > > > have complex interactions with other drivers in the Linux
> > > > > > system, so debugging and deplyong fixes is much easier when the
> > > > > > power domain handling is fully done by a kernel
> > > > driver.
> > > > > Actually, due to the security requirement from other system
> > > > > solution provider, for example, Microsoft Azure Sphere, it has
> > > > > strict requirement for power domain to be controlled by secure
> > > > > subsystem(either
> > > > TF-A, TEE or dedicated secure domain controller).
> > > > > Same requirement for reset control, and system critical clock control.
> > > >
> > > > Yes, I'm aware of those requirements, but to satisfy those you need
> > > > a full implementation of all those parts in the secure subsystem.
> > > > Doing it just for the power domains adds complexity for no gain, as
> > > > you still won't be able to meet all the requirements and frankly I
> > > > don't think this is a realistic goal to achieve with the current i.MX8M
> > family of SoCs.
> > >
> > > At least we are moving to that direction.
> >
> > To me it seems like the current way (custom TF-A interface and
> > implementation) is one step in the right direction, but two steps backwards in
> > terms of complexity.
> >
> > > > Meeting those requirements needs a fully system approach where the
> > > > secure subsystem parts are made sufficiently independent from the
> > > > non- secure parts on a hardware level, which I don't see on the
> > > > i.MX8M SoC and hardware design guide.
> > >
> > > CSU could restrict the access permission.
> >
> > While this is true, my argument is much broader and not only focused on
> > on-SoC peripherals. For example some of the power domains need different
> > voltages for specific performance states, which means you need to
> > communicate with a external PMIC or other voltage regulator, which in turn
> > means you need to set aside the necessary i2c bus and/or GPIO banks
> > required for this communication at system design time, so it isn't shared
> > between TF-A and the rich OS. I don't see this in any of the i.MX8M designs.
> >
> > > > > For NXP i.MX8M family, it is ok to implement in linux kernel, just
> > > > > a tradeoff to find out a place to hide the complexity ^_^.
> > > > >
> > > > > BTW, for virtualization support, it is better to put the power
> > > > > domain in a central place to simplify the VM implementation.
> > > >
> > > > Same as above. If you can make all the relevant bits (clock, reset,
> > > > power-domain, regulator) available via a virtualization friendly
> > > > API, then I would see a point in adding complexity for this
> > > > abstraction. As long as this added abstraction only solves a very
> > > > tiny bit of the overall picture, I just don't see the point in the added
> > complexity and (from a Linux PoV) obfuscation.
> > >
> > > Could we use SCMI for power domain, system critical clocks, smc
> > > watchdog and etc?
> >
> > If you could demonstrate a working solution with all those pieces hidden
> > behind a standard SCMI interface, this would make for a much more
> > compelling story supporting the secure subsystem argument.
> >
> > > Or we support two approaches, one is let Linux control everything, the
> > > other is using SCMI.
> > >
> > > Thoughts?
> >
> > I wouldn't be opposed to such a solution. If you can put all this behind a
> > standard SCMI interface, I guess we wouldn't need two different SoC specific
> > drivers for the same purpose, so we could easily have a Linux full-control
> > solution (i.e. this patchset) coexist with a SCMI based implementation,
> > possibly with just a slightly different base SoC DT with all the power domains,
> > clocks and other system level control stuff behind SCMI.
> >
> > What I'm strongly opposed to is having a custom TF-A interface and all the
> > added complexity for little to no gain in actual system security.
>
> Understand. There are truly the SoC design might not fit well to protect
> all the stuff.
>
> It is good that you did this patchset. Vote for you to add more support
> on i.MX.
>
> Your patchset not conflict with SCMI, as you said, this is true.
>
> Please continue your effort.

Despite ongoing efforts to get the displaymix (and probably vpumix)
operational, it would be nice to have operational USB which needs two
of the power domains.  Could we apply the initial framework and only
the DT nodes for the functional power domains (hsiomix and pgc_otg1) .

While it's not a perfect solution, I think incremental improvements
are better than going stale while we wait for the perfect solution.
Having them pulled into a staging branch also helps coordinate
efforts.

adam
>
> Thanks,
> Peng.
>
> >
> > Regards,
> > Lucas
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel