* [PATCH 1/2] soc: imx: gpcv2: rename keep_clocks to bus_clocks
@ 2023-06-02 18:54 Lucas Stach
2023-06-02 18:54 ` [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early Lucas Stach
0 siblings, 1 reply; 6+ messages in thread
From: Lucas Stach @ 2023-06-02 18:54 UTC (permalink / raw)
To: Shawn Guo
Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
Marek Vasut, patchwork-lst, linux-arm-kernel, Luca Ceresoli
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early
2023-06-02 18:54 [PATCH 1/2] soc: imx: gpcv2: rename keep_clocks to bus_clocks Lucas Stach
@ 2023-06-02 18:54 ` Lucas Stach
2023-06-02 19:12 ` Marek Vasut
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Lucas Stach @ 2023-06-02 18:54 UTC (permalink / raw)
To: Shawn Guo
Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
Marek Vasut, patchwork-lst, linux-arm-kernel, Luca Ceresoli
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early
2023-06-02 18:54 ` [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early Lucas Stach
@ 2023-06-02 19:12 ` Marek Vasut
2023-06-02 20:06 ` Lucas Stach
2023-06-05 7:15 ` Marco Felsch
2023-06-05 8:06 ` Luca Ceresoli
2 siblings, 1 reply; 6+ messages in thread
From: Marek Vasut @ 2023-06-02 19:12 UTC (permalink / raw)
To: Lucas Stach, Shawn Guo
Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
patchwork-lst, linux-arm-kernel, Luca Ceresoli, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early
2023-06-02 19:12 ` Marek Vasut
@ 2023-06-02 20:06 ` Lucas Stach
0 siblings, 0 replies; 6+ messages in thread
From: Lucas Stach @ 2023-06-02 20:06 UTC (permalink / raw)
To: Marek Vasut, Shawn Guo
Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
patchwork-lst, linux-arm-kernel, Luca Ceresoli, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early
2023-06-02 18:54 ` [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early Lucas Stach
2023-06-02 19:12 ` Marek Vasut
@ 2023-06-05 7:15 ` Marco Felsch
2023-06-05 8:06 ` Luca Ceresoli
2 siblings, 0 replies; 6+ messages in thread
From: Marco Felsch @ 2023-06-05 7:15 UTC (permalink / raw)
To: Lucas Stach
Cc: Shawn Guo, Marek Vasut, Luca Ceresoli, patchwork-lst,
NXP Linux Team, Pengutronix Kernel Team, Fabio Estevam,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early
2023-06-02 18:54 ` [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early Lucas Stach
2023-06-02 19:12 ` Marek Vasut
2023-06-05 7:15 ` Marco Felsch
@ 2023-06-05 8:06 ` Luca Ceresoli
2 siblings, 0 replies; 6+ messages in thread
From: Luca Ceresoli @ 2023-06-05 8:06 UTC (permalink / raw)
To: Lucas Stach
Cc: Shawn Guo, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Marek Vasut, patchwork-lst, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-06-05 8:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-02 18:54 [PATCH 1/2] soc: imx: gpcv2: rename keep_clocks to bus_clocks Lucas Stach
2023-06-02 18:54 ` [PATCH 2/2] soc: imx: gpcv2: prepare bus clocks early Lucas Stach
2023-06-02 19:12 ` Marek Vasut
2023-06-02 20:06 ` Lucas Stach
2023-06-05 7:15 ` Marco Felsch
2023-06-05 8:06 ` Luca Ceresoli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).