| Message ID | 20230602185417.4098937-2-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] soc: imx: gpcv2: rename keep_clocks to bus_clocks | expand |
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.
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
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 > > >
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
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);
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(-)