[PATCH 1/2] soc: imx: gpcv2: rename keep_clocks to bus_clocks

[PATCH 1/2] soc: imx: gpcv2: rename keep_clocks to bus_clocks

[Lucas Stach]

Some top-level PGC domains have bus clocks that feed the logic
inside the domain like the ADB and the blk-ctrl, which must not
be gated as long as the domain is powered up. Until now we have
marked those domains with the keep_clocks property. Rename this
to bus_clocks as there are some more behavioral differences for
the bus clocks needed that should be keyed off this property.

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

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 4b3300b090a8..706f852e5d87 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -293,7 +293,7 @@ struct imx_pgc_domain {
 	} bits;
 
 	const int voltage;
-	const bool keep_clocks;
+	const bool bus_clocks;
 	struct device *dev;
 
 	unsigned int pgc_sw_pup_reg;
@@ -396,7 +396,7 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
 	}
 
 	/* Disable reset clocks for all devices in the domain */
-	if (!domain->keep_clocks)
+	if (!domain->bus_clocks)
 		clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
 
 	return 0;
@@ -419,7 +419,7 @@ static int imx_pgc_power_down(struct generic_pm_domain *genpd)
 	int ret;
 
 	/* Enable reset clocks for all devices in the domain */
-	if (!domain->keep_clocks) {
+	if (!domain->bus_clocks) {
 		ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
 		if (ret) {
 			dev_err(domain->dev, "failed to enable reset clocks\n");
@@ -484,7 +484,7 @@ static int imx_pgc_power_down(struct generic_pm_domain *genpd)
 	return 0;
 
 out_clk_disable:
-	if (!domain->keep_clocks)
+	if (!domain->bus_clocks)
 		clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
 
 	return ret;
@@ -638,7 +638,7 @@ static const struct imx_pgc_domain imx8m_pgc_domains[] = {
 			.hskack = IMX8M_VPU_HSK_PWRDNACKN,
 		},
 		.pgc   = BIT(IMX8M_PGC_VPU),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8M_POWER_DOMAIN_DISP] = {
@@ -738,7 +738,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
 			.hskreq = IMX8MM_HSIO_HSK_PWRDNREQN,
 			.hskack = IMX8MM_HSIO_HSK_PWRDNACKN,
 		},
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MM_POWER_DOMAIN_PCIE] = {
@@ -787,7 +787,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
 			.hskack = IMX8MM_GPUMIX_HSK_PWRDNACKN,
 		},
 		.pgc   = BIT(IMX8MM_PGC_GPUMIX),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MM_POWER_DOMAIN_GPU] = {
@@ -814,7 +814,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
 			.hskack = IMX8MM_VPUMIX_HSK_PWRDNACKN,
 		},
 		.pgc   = BIT(IMX8MM_PGC_VPUMIX),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MM_POWER_DOMAIN_VPUG1] = {
@@ -848,7 +848,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
 			.map = IMX8MM_VPUH1_A53_DOMAIN,
 		},
 		.pgc   = BIT(IMX8MM_PGC_VPUH1),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MM_POWER_DOMAIN_DISPMIX] = {
@@ -862,7 +862,7 @@ static const struct imx_pgc_domain imx8mm_pgc_domains[] = {
 			.hskack = IMX8MM_DISPMIX_HSK_PWRDNACKN,
 		},
 		.pgc   = BIT(IMX8MM_PGC_DISPMIX),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MM_POWER_DOMAIN_MIPI] = {
@@ -976,7 +976,7 @@ static const struct imx_pgc_domain imx8mp_pgc_domains[] = {
 			.hskack = IMX8MP_MLMIX_PWRDNACKN,
 		},
 		.pgc = BIT(IMX8MP_PGC_MLMIX),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MP_POWER_DOMAIN_AUDIOMIX] = {
@@ -990,7 +990,7 @@ static const struct imx_pgc_domain imx8mp_pgc_domains[] = {
 			.hskack = IMX8MP_AUDIOMIX_PWRDNACKN,
 		},
 		.pgc = BIT(IMX8MP_PGC_AUDIOMIX),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MP_POWER_DOMAIN_GPU2D] = {
@@ -1015,7 +1015,7 @@ static const struct imx_pgc_domain imx8mp_pgc_domains[] = {
 			.hskack = IMX8MP_GPUMIX_PWRDNACKN,
 		},
 		.pgc = BIT(IMX8MP_PGC_GPUMIX),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MP_POWER_DOMAIN_VPUMIX] = {
@@ -1029,7 +1029,7 @@ static const struct imx_pgc_domain imx8mp_pgc_domains[] = {
 			.hskack = IMX8MP_VPUMIX_PWRDNACKN,
 		},
 		.pgc = BIT(IMX8MP_PGC_VPUMIX),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MP_POWER_DOMAIN_GPU3D] = {
@@ -1054,7 +1054,7 @@ static const struct imx_pgc_domain imx8mp_pgc_domains[] = {
 			.hskack = IMX8MP_MEDIAMIX_PWRDNACKN,
 		},
 		.pgc = BIT(IMX8MP_PGC_MEDIAMIX),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MP_POWER_DOMAIN_VPU_G1] = {
@@ -1101,7 +1101,7 @@ static const struct imx_pgc_domain imx8mp_pgc_domains[] = {
 			.hskack = IMX8MP_HDMIMIX_PWRDNACKN,
 		},
 		.pgc = BIT(IMX8MP_PGC_HDMIMIX),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MP_POWER_DOMAIN_HDMI_PHY] = {
@@ -1137,7 +1137,7 @@ static const struct imx_pgc_domain imx8mp_pgc_domains[] = {
 			.hskack = IMX8MP_HSIOMIX_PWRDNACKN,
 		},
 		.pgc = BIT(IMX8MP_PGC_HSIOMIX),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MP_POWER_DOMAIN_MEDIAMIX_ISPDWP] = {
@@ -1228,7 +1228,7 @@ static const struct imx_pgc_domain imx8mn_pgc_domains[] = {
 			.hskreq = IMX8MN_HSIO_HSK_PWRDNREQN,
 			.hskack = IMX8MN_HSIO_HSK_PWRDNACKN,
 		},
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MN_POWER_DOMAIN_OTG1] = {
@@ -1254,7 +1254,7 @@ static const struct imx_pgc_domain imx8mn_pgc_domains[] = {
 			.hskack = IMX8MN_GPUMIX_HSK_PWRDNACKN,
 		},
 		.pgc   = BIT(IMX8MN_PGC_GPUMIX),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MN_POWER_DOMAIN_DISPMIX] = {
@@ -1268,7 +1268,7 @@ static const struct imx_pgc_domain imx8mn_pgc_domains[] = {
 			.hskack = IMX8MN_DISPMIX_HSK_PWRDNACKN,
 		},
 		.pgc   = BIT(IMX8MN_PGC_DISPMIX),
-		.keep_clocks = true,
+		.bus_clocks = true,
 	},
 
 	[IMX8MN_POWER_DOMAIN_MIPI] = {
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early

[Lucas Stach]

Prepare the bus clocks during PGC domain driver probe. This avoids a
potential deadlock when there a clock providers inside the domain,
as this might end up trying to take the CCF prepare_lock from two
different contexts, when runtime PM is trying to resume the PGC domain
for the clock provider. By keeping the bus clocks prepared as long as
there is a PGC domain driver attached, we don't need to take the
prepare_lock in the domain power up/down paths.

We don't want to do this for regular reset clocks, as this might lead
to some PLLs being kept prepared that could otherwise be shut down.
For the bus clocks this isn't a concern, as all the bus clocks are
derived from always-on system PLLs.

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

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 706f852e5d87..428e2fd82f26 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -337,10 +337,14 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
 
 	reset_control_assert(domain->reset);
 
-	/* Enable reset clocks for all devices in the domain */
-	ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
+	if (!domain->bus_clocks)
+		/* Enable reset clocks for all devices in the domain */
+		ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
+	else
+		/* Enable bus clocks for this domain */
+		ret = clk_bulk_enable(domain->num_clks, domain->clks);
 	if (ret) {
-		dev_err(domain->dev, "failed to enable reset clocks\n");
+		dev_err(domain->dev, "failed to enable clocks\n");
 		goto out_regulator_disable;
 	}
 
@@ -402,7 +406,10 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
 	return 0;
 
 out_clk_disable:
-	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
+	if (!domain->bus_clocks)
+		clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
+	else
+		clk_bulk_disable(domain->num_clks, domain->clks);
 out_regulator_disable:
 	if (!IS_ERR(domain->regulator))
 		regulator_disable(domain->regulator);
@@ -466,8 +473,11 @@ static int imx_pgc_power_down(struct generic_pm_domain *genpd)
 		}
 	}
 
-	/* Disable reset clocks for all devices in the domain */
-	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
+	/* Disable bus or reset clocks for all devices in the domain */
+	if (!domain->bus_clocks)
+		clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
+	else
+		clk_bulk_disable(domain->num_clks, domain->clks);
 
 	if (!IS_ERR(domain->regulator)) {
 		ret = regulator_disable(domain->regulator);
@@ -486,6 +496,8 @@ static int imx_pgc_power_down(struct generic_pm_domain *genpd)
 out_clk_disable:
 	if (!domain->bus_clocks)
 		clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
+	else
+		clk_bulk_disable(domain->num_clks, domain->clks);
 
 	return ret;
 }
@@ -1343,10 +1355,19 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
 		regmap_update_bits(domain->regmap, domain->regs->map,
 				   domain->bits.map, domain->bits.map);
 
+	if (domain->bus_clocks) {
+		ret = clk_bulk_prepare(domain->num_clks, domain->clks);
+		if (ret) {
+			dev_err(domain->dev,
+				"Failed to prepare domain's clocks\n");
+			goto out_domain_unmap;
+		}
+	}
+
 	ret = pm_genpd_init(&domain->genpd, NULL, true);
 	if (ret) {
 		dev_err(domain->dev, "Failed to init power domain\n");
-		goto out_domain_unmap;
+		goto out_disable_clocks;
 	}
 
 	if (IS_ENABLED(CONFIG_LOCKDEP) &&
@@ -1364,6 +1385,9 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
 
 out_genpd_remove:
 	pm_genpd_remove(&domain->genpd);
+out_disable_clocks:
+	if (domain->bus_clocks)
+		clk_bulk_unprepare(domain->num_clks, domain->clks);
 out_domain_unmap:
 	if (domain->bits.map)
 		regmap_update_bits(domain->regmap, domain->regs->map,
@@ -1380,6 +1404,9 @@ static int imx_pgc_domain_remove(struct platform_device *pdev)
 	of_genpd_del_provider(domain->dev->of_node);
 	pm_genpd_remove(&domain->genpd);
 
+	if (domain->bus_clocks)
+		clk_bulk_unprepare(domain->num_clks, domain->clks);
+
 	if (domain->bits.map)
 		regmap_update_bits(domain->regmap, domain->regs->map,
 				   domain->bits.map, 0);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early

[Marek Vasut]

On 6/2/23 20:54, Lucas Stach wrote:
> Prepare the bus clocks during PGC domain driver probe. This avoids a
> potential deadlock when there a clock providers inside the domain,
> as this might end up trying to take the CCF prepare_lock from two
> different contexts, when runtime PM is trying to resume the PGC domain
> for the clock provider. By keeping the bus clocks prepared as long as
> there is a PGC domain driver attached, we don't need to take the
> prepare_lock in the domain power up/down paths.
> 
> We don't want to do this for regular reset clocks, as this might lead
> to some PLLs being kept prepared that could otherwise be shut down.
> For the bus clocks this isn't a concern, as all the bus clocks are
> derived from always-on system PLLs.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>   drivers/soc/imx/gpcv2.c | 41 ++++++++++++++++++++++++++++++++++-------
>   1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 706f852e5d87..428e2fd82f26 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -337,10 +337,14 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
>   
>   	reset_control_assert(domain->reset);
>   
> -	/* Enable reset clocks for all devices in the domain */
> -	ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
> +	if (!domain->bus_clocks)
> +		/* Enable reset clocks for all devices in the domain */
> +		ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
> +	else
> +		/* Enable bus clocks for this domain */
> +		ret = clk_bulk_enable(domain->num_clks, domain->clks);
>   	if (ret) {
> -		dev_err(domain->dev, "failed to enable reset clocks\n");
> +		dev_err(domain->dev, "failed to enable clocks\n");
>   		goto out_regulator_disable;
>   	}
>   
> @@ -402,7 +406,10 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
>   	return 0;
>   
>   out_clk_disable:
> -	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
> +	if (!domain->bus_clocks)
> +		clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
> +	else
> +		clk_bulk_disable(domain->num_clks, domain->clks);
>   out_regulator_disable:
>   	if (!IS_ERR(domain->regulator))
>   		regulator_disable(domain->regulator);
> @@ -466,8 +473,11 @@ static int imx_pgc_power_down(struct generic_pm_domain *genpd)
>   		}
>   	}
>   
> -	/* Disable reset clocks for all devices in the domain */
> -	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
> +	/* Disable bus or reset clocks for all devices in the domain */
> +	if (!domain->bus_clocks)
> +		clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
> +	else
> +		clk_bulk_disable(domain->num_clks, domain->clks);
>   
>   	if (!IS_ERR(domain->regulator)) {
>   		ret = regulator_disable(domain->regulator);
> @@ -486,6 +496,8 @@ static int imx_pgc_power_down(struct generic_pm_domain *genpd)
>   out_clk_disable:
>   	if (!domain->bus_clocks)
>   		clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
> +	else
> +		clk_bulk_disable(domain->num_clks, domain->clks);
>   
>   	return ret;
>   }
> @@ -1343,10 +1355,19 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
>   		regmap_update_bits(domain->regmap, domain->regs->map,
>   				   domain->bits.map, domain->bits.map);
>   
> +	if (domain->bus_clocks) {
> +		ret = clk_bulk_prepare(domain->num_clks, domain->clks);
> +		if (ret) {
> +			dev_err(domain->dev,
> +				"Failed to prepare domain's clocks\n");
> +			goto out_domain_unmap;
> +		}
> +	}
> +
>   	ret = pm_genpd_init(&domain->genpd, NULL, true);
>   	if (ret) {
>   		dev_err(domain->dev, "Failed to init power domain\n");
> -		goto out_domain_unmap;
> +		goto out_disable_clocks;
>   	}
>   
>   	if (IS_ENABLED(CONFIG_LOCKDEP) &&
> @@ -1364,6 +1385,9 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
>   
>   out_genpd_remove:
>   	pm_genpd_remove(&domain->genpd);
> +out_disable_clocks:
> +	if (domain->bus_clocks)
> +		clk_bulk_unprepare(domain->num_clks, domain->clks);
>   out_domain_unmap:
>   	if (domain->bits.map)
>   		regmap_update_bits(domain->regmap, domain->regs->map,
> @@ -1380,6 +1404,9 @@ static int imx_pgc_domain_remove(struct platform_device *pdev)
>   	of_genpd_del_provider(domain->dev->of_node);
>   	pm_genpd_remove(&domain->genpd);
>   
> +	if (domain->bus_clocks)
> +		clk_bulk_unprepare(domain->num_clks, domain->clks);
> +
>   	if (domain->bits.map)
>   		regmap_update_bits(domain->regmap, domain->regs->map,
>   				   domain->bits.map, 0);

Isn't this similar approach to

[PATCH] [RFC] soc: imx: gpcv2: Split clock prepare from clock enable in 
the domain

where Laurent (now on CC) reported that this still causes issues ?

If not, then please ignore my comment here.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early

[Lucas Stach]

Hi Marek,

Am Freitag, dem 02.06.2023 um 21:12 +0200 schrieb Marek Vasut:
> On 6/2/23 20:54, Lucas Stach wrote:
> > Prepare the bus clocks during PGC domain driver probe. This avoids a
> > potential deadlock when there a clock providers inside the domain,
> > as this might end up trying to take the CCF prepare_lock from two
> > different contexts, when runtime PM is trying to resume the PGC domain
> > for the clock provider. By keeping the bus clocks prepared as long as
> > there is a PGC domain driver attached, we don't need to take the
> > prepare_lock in the domain power up/down paths.
> > 
> > We don't want to do this for regular reset clocks, as this might lead
> > to some PLLs being kept prepared that could otherwise be shut down.
> > For the bus clocks this isn't a concern, as all the bus clocks are
> > derived from always-on system PLLs.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> 
[...]
> Isn't this similar approach to
> 
> [PATCH] [RFC] soc: imx: gpcv2: Split clock prepare from clock enable in 
> the domain
> 
> where Laurent (now on CC) reported that this still causes issues ?
> 
> If not, then please ignore my comment here.

Yes, that's right. It doesn't solve the clock provider (HDMI PHY) is
inside a power domain itself issue.
However it resolves the simple cases where we would deadlock due to the
needed bus clocks, which is what happens when audiomix adds runtime PM
support. So I still think it's a good idea in general, even if it
doesn't solve all the problems. 

Regards,
Lucas

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early

[Marco Felsch]

Hi Lucas,

thanks for posting.

On 23-06-02, Lucas Stach wrote:
> Prepare the bus clocks during PGC domain driver probe. This avoids a
> potential deadlock when there a clock providers inside the domain,
> as this might end up trying to take the CCF prepare_lock from two
> different contexts, when runtime PM is trying to resume the PGC domain
> for the clock provider. By keeping the bus clocks prepared as long as
> there is a PGC domain driver attached, we don't need to take the
> prepare_lock in the domain power up/down paths.
> 
> We don't want to do this for regular reset clocks, as this might lead
> to some PLLs being kept prepared that could otherwise be shut down.
> For the bus clocks this isn't a concern, as all the bus clocks are
> derived from always-on system PLLs.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Feel free to add my rb for both patches.

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

> ---
>  drivers/soc/imx/gpcv2.c | 41 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 706f852e5d87..428e2fd82f26 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -337,10 +337,14 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
>  
>  	reset_control_assert(domain->reset);
>  
> -	/* Enable reset clocks for all devices in the domain */
> -	ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
> +	if (!domain->bus_clocks)
> +		/* Enable reset clocks for all devices in the domain */
> +		ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
> +	else
> +		/* Enable bus clocks for this domain */
> +		ret = clk_bulk_enable(domain->num_clks, domain->clks);
>  	if (ret) {
> -		dev_err(domain->dev, "failed to enable reset clocks\n");
> +		dev_err(domain->dev, "failed to enable clocks\n");
>  		goto out_regulator_disable;
>  	}
>  
> @@ -402,7 +406,10 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd)
>  	return 0;
>  
>  out_clk_disable:
> -	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
> +	if (!domain->bus_clocks)
> +		clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
> +	else
> +		clk_bulk_disable(domain->num_clks, domain->clks);
>  out_regulator_disable:
>  	if (!IS_ERR(domain->regulator))
>  		regulator_disable(domain->regulator);
> @@ -466,8 +473,11 @@ static int imx_pgc_power_down(struct generic_pm_domain *genpd)
>  		}
>  	}
>  
> -	/* Disable reset clocks for all devices in the domain */
> -	clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
> +	/* Disable bus or reset clocks for all devices in the domain */
> +	if (!domain->bus_clocks)
> +		clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
> +	else
> +		clk_bulk_disable(domain->num_clks, domain->clks);
>  
>  	if (!IS_ERR(domain->regulator)) {
>  		ret = regulator_disable(domain->regulator);
> @@ -486,6 +496,8 @@ static int imx_pgc_power_down(struct generic_pm_domain *genpd)
>  out_clk_disable:
>  	if (!domain->bus_clocks)
>  		clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
> +	else
> +		clk_bulk_disable(domain->num_clks, domain->clks);
>  
>  	return ret;
>  }
> @@ -1343,10 +1355,19 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
>  		regmap_update_bits(domain->regmap, domain->regs->map,
>  				   domain->bits.map, domain->bits.map);
>  
> +	if (domain->bus_clocks) {
> +		ret = clk_bulk_prepare(domain->num_clks, domain->clks);
> +		if (ret) {
> +			dev_err(domain->dev,
> +				"Failed to prepare domain's clocks\n");
> +			goto out_domain_unmap;
> +		}
> +	}
> +
>  	ret = pm_genpd_init(&domain->genpd, NULL, true);
>  	if (ret) {
>  		dev_err(domain->dev, "Failed to init power domain\n");
> -		goto out_domain_unmap;
> +		goto out_disable_clocks;
>  	}
>  
>  	if (IS_ENABLED(CONFIG_LOCKDEP) &&
> @@ -1364,6 +1385,9 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
>  
>  out_genpd_remove:
>  	pm_genpd_remove(&domain->genpd);
> +out_disable_clocks:
> +	if (domain->bus_clocks)
> +		clk_bulk_unprepare(domain->num_clks, domain->clks);
>  out_domain_unmap:
>  	if (domain->bits.map)
>  		regmap_update_bits(domain->regmap, domain->regs->map,
> @@ -1380,6 +1404,9 @@ static int imx_pgc_domain_remove(struct platform_device *pdev)
>  	of_genpd_del_provider(domain->dev->of_node);
>  	pm_genpd_remove(&domain->genpd);
>  
> +	if (domain->bus_clocks)
> +		clk_bulk_unprepare(domain->num_clks, domain->clks);
> +
>  	if (domain->bits.map)
>  		regmap_update_bits(domain->regmap, domain->regs->map,
>  				   domain->bits.map, 0);
> -- 
> 2.39.2
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early

[Luca Ceresoli]

Hi Lucas,

On Fri,  2 Jun 2023 20:54:17 +0200
Lucas Stach <l.stach@pengutronix.de> wrote:

> Prepare the bus clocks during PGC domain driver probe. This avoids a
> potential deadlock when there a clock providers inside the domain,
> as this might end up trying to take the CCF prepare_lock from two
> different contexts, when runtime PM is trying to resume the PGC domain
> for the clock provider. By keeping the bus clocks prepared as long as
> there is a PGC domain driver attached, we don't need to take the
> prepare_lock in the domain power up/down paths.
> 
> We don't want to do this for regular reset clocks, as this might lead
> to some PLLs being kept prepared that could otherwise be shut down.
> For the bus clocks this isn't a concern, as all the bus clocks are
> derived from always-on system PLLs.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

...

> @@ -1343,10 +1355,19 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
>  		regmap_update_bits(domain->regmap, domain->regs->map,
>  				   domain->bits.map, domain->bits.map);
>  
> +	if (domain->bus_clocks) {
> +		ret = clk_bulk_prepare(domain->num_clks, domain->clks);
> +		if (ret) {
> +			dev_err(domain->dev,
> +				"Failed to prepare domain's clocks\n");
> +			goto out_domain_unmap;
> +		}
> +	}
> +
>  	ret = pm_genpd_init(&domain->genpd, NULL, true);
>  	if (ret) {
>  		dev_err(domain->dev, "Failed to init power domain\n");
> -		goto out_domain_unmap;
> +		goto out_disable_clocks;
>  	}
>  
>  	if (IS_ENABLED(CONFIG_LOCKDEP) &&
> @@ -1364,6 +1385,9 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
>  
>  out_genpd_remove:
>  	pm_genpd_remove(&domain->genpd);
> +out_disable_clocks:
> +	if (domain->bus_clocks)
> +		clk_bulk_unprepare(domain->num_clks, domain->clks);

Shouldn't the label be "out_unprepare_clocks"?

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel