[v3] soc: imx: gpc: Turn PU domain on/off in sleep on 6qp
diff mbox series

Message ID ab0c7997bdb0caf6fe7e842315bb9565c2cb059d.1530895248.git.leonard.crestez@nxp.com
State Rejected
Headers show
Series
  • [v3] soc: imx: gpc: Turn PU domain on/off in sleep on 6qp
Related show

Commit Message

Leonard Crestez July 6, 2018, 4:46 p.m. UTC
On imx6qp power gating on the PU domain is disabled because of errata
ERR009619. However power gating during suspend/resume can still be
performed.

Enable this by implementing SLEEP_PM_OPS in imx_pgc_power_domain_driver.

In order to ensure correct ordering add device links from devices inside
the PU domain to the pgc platform_device.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

---
 drivers/soc/imx/gpc.c | 88 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 73 insertions(+), 15 deletions(-)

Link to v2: https://lkml.org/lkml/2018/7/5/564

Changes since v2: Add device links on attach

The NXP kernel has been turning the PU domain off in sleep on 6qp for
many releases so this behavior went through multiple QA cycles and is
known to work. Further small improvements in power usage could be
obtained by also shutting off the PU LDO regulator but that's not as
well-validated.

Comments

Ulf Hansson July 9, 2018, 10:27 p.m. UTC | #1
On 6 July 2018 at 18:46, Leonard Crestez <leonard.crestez@nxp.com> wrote:
> On imx6qp power gating on the PU domain is disabled because of errata
> ERR009619. However power gating during suspend/resume can still be
> performed.

Sounds a bit hand wavy. Exactly why can it be done during system suspend/resume?

>
> Enable this by implementing SLEEP_PM_OPS in imx_pgc_power_domain_driver.
>
> In order to ensure correct ordering add device links from devices inside
> the PU domain to the pgc platform_device.

In genpd, the PM domain gets powered off after all device's
corresponding ->suspend_noirq() callback has been invoked - and it
also considers if there are some wakeup settings enabled, possibly
avoiding to do the power off.

It sound like the imx's power domain driver ->power_off|on() callback
should deal with this, to make get a consistent behavior. I understand
that you currently can't tell under what circumstances the
->power_off|on() callbacks are being invoked, that may need some
changes in genpd.

Currently we have a flag, GENPD_FLAG_ALWAYS_ON which makes genpd to
always prevent power off. Perhaps we should add a new flag
"GENPD_FLAG_RUNTIME_ON", which makes genpd to call the
->power_on|off() callbacks, but only during system wide PM.

Thoughts?

[...]

Kind regards
Uffe
Lucas Stach July 10, 2018, 8:05 a.m. UTC | #2
Am Dienstag, den 10.07.2018, 00:27 +0200 schrieb Ulf Hansson:
> > On 6 July 2018 at 18:46, Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > On imx6qp power gating on the PU domain is disabled because of errata
> > ERR009619. However power gating during suspend/resume can still be
> > performed.
> 
> Sounds a bit hand wavy. Exactly why can it be done during system suspend/resume?

Power gating this domain in normal operation may introduce a glitch in
unrelated modules, potentially disturbing a running display scanout.
This is not a concern when the whole system enters suspend state, so
it's safe to do in this case.

Regards,
Lucas

> > 
> > Enable this by implementing SLEEP_PM_OPS in imx_pgc_power_domain_driver.
> > 
> > In order to ensure correct ordering add device links from devices inside
> > the PU domain to the pgc platform_device.
> 
> In genpd, the PM domain gets powered off after all device's
> corresponding ->suspend_noirq() callback has been invoked - and it
> also considers if there are some wakeup settings enabled, possibly
> avoiding to do the power off.
> 
> It sound like the imx's power domain driver ->power_off|on() callback
> should deal with this, to make get a consistent behavior. I understand
> that you currently can't tell under what circumstances the
> ->power_off|on() callbacks are being invoked, that may need some
> changes in genpd.
> 
> Currently we have a flag, GENPD_FLAG_ALWAYS_ON which makes genpd to
> always prevent power off. Perhaps we should add a new flag
> "GENPD_FLAG_RUNTIME_ON", which makes genpd to call the
> ->power_on|off() callbacks, but only during system wide PM.
> 
> Thoughts?

Yes, this might work for this case.

Regards,
Lucas
Ulf Hansson July 10, 2018, 9:34 a.m. UTC | #3
On 10 July 2018 at 10:05, Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Dienstag, den 10.07.2018, 00:27 +0200 schrieb Ulf Hansson:
>> > On 6 July 2018 at 18:46, Leonard Crestez <leonard.crestez@nxp.com> wrote:
>> > On imx6qp power gating on the PU domain is disabled because of errata
>> > ERR009619. However power gating during suspend/resume can still be
>> > performed.
>>
>> Sounds a bit hand wavy. Exactly why can it be done during system suspend/resume?
>
> Power gating this domain in normal operation may introduce a glitch in
> unrelated modules, potentially disturbing a running display scanout.
> This is not a concern when the whole system enters suspend state, so
> it's safe to do in this case.

Makes sense. Could we please fold this information into the commit
message to make this clear!?

>
> Regards,
> Lucas
>
>> >
>> > Enable this by implementing SLEEP_PM_OPS in imx_pgc_power_domain_driver.
>> >
>> > In order to ensure correct ordering add device links from devices inside
>> > the PU domain to the pgc platform_device.
>>
>> In genpd, the PM domain gets powered off after all device's
>> corresponding ->suspend_noirq() callback has been invoked - and it
>> also considers if there are some wakeup settings enabled, possibly
>> avoiding to do the power off.
>>
>> It sound like the imx's power domain driver ->power_off|on() callback
>> should deal with this, to make get a consistent behavior. I understand
>> that you currently can't tell under what circumstances the
>> ->power_off|on() callbacks are being invoked, that may need some
>> changes in genpd.
>>
>> Currently we have a flag, GENPD_FLAG_ALWAYS_ON which makes genpd to
>> always prevent power off. Perhaps we should add a new flag
>> "GENPD_FLAG_RUNTIME_ON", which makes genpd to call the
>> ->power_on|off() callbacks, but only during system wide PM.
>>
>> Thoughts?
>
> Yes, this might work for this case.

Alright. I can help to implement, if you want. Just tell.

Kind regards
Uffe

Patch
diff mbox series

diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
index 32f0748fd067..b96d8dc9626d 100644
--- a/drivers/soc/imx/gpc.c
+++ b/drivers/soc/imx/gpc.c
@@ -54,19 +54,16 @@  static inline struct imx_pm_domain *
 to_imx_pm_domain(struct generic_pm_domain *genpd)
 {
 	return container_of(genpd, struct imx_pm_domain, base);
 }
 
-static int imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
+static void _imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
 {
 	struct imx_pm_domain *pd = to_imx_pm_domain(genpd);
 	int iso, iso2sw;
 	u32 val;
 
-	if (pd->flags & PGC_DOMAIN_FLAG_NO_PD)
-		return -EBUSY;
-
 	/* Read ISO and ISO2SW power down delays */
 	regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val);
 	iso = val & 0x3f;
 	iso2sw = (val >> 8) & 0x3f;
 
@@ -78,32 +75,33 @@  static int imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
 	val = BIT(pd->cntr_pdn_bit);
 	regmap_update_bits(pd->regmap, GPC_CNTR, val, val);
 
 	/* Wait ISO + ISO2SW IPG clock cycles */
 	udelay(DIV_ROUND_UP(iso + iso2sw, pd->ipg_rate_mhz));
+}
+
+static int imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
+{
+	struct imx_pm_domain *pd = to_imx_pm_domain(genpd);
+
+	if (pd->flags & PGC_DOMAIN_FLAG_NO_PD)
+		return -EBUSY;
+
+	_imx6_pm_domain_power_off(genpd);
 
 	if (pd->supply)
 		regulator_disable(pd->supply);
 
 	return 0;
 }
 
-static int imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
+static void _imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
 {
 	struct imx_pm_domain *pd = to_imx_pm_domain(genpd);
-	int i, ret, sw, sw2iso;
+	int i, sw, sw2iso;
 	u32 val;
 
-	if (pd->supply) {
-		ret = regulator_enable(pd->supply);
-		if (ret) {
-			pr_err("%s: failed to enable regulator: %d\n",
-			       __func__, ret);
-			return ret;
-		}
-	}
-
 	/* Enable reset clocks for all devices in the domain */
 	for (i = 0; i < pd->num_clks; i++)
 		clk_prepare_enable(pd->clk[i]);
 
 	/* Gate off domain when powered down */
@@ -123,10 +121,39 @@  static int imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
 	udelay(DIV_ROUND_UP(sw + sw2iso, pd->ipg_rate_mhz));
 
 	/* Disable reset clocks for all devices in the domain */
 	for (i = 0; i < pd->num_clks; i++)
 		clk_disable_unprepare(pd->clk[i]);
+}
+
+static int imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
+{
+	struct imx_pm_domain *pd = to_imx_pm_domain(genpd);
+	int ret;
+
+	if (pd->supply) {
+		ret = regulator_enable(pd->supply);
+		if (ret) {
+			pr_err("%s: failed to enable regulator: %d\n",
+			       __func__, ret);
+			return ret;
+		}
+	}
+
+	_imx6_pm_domain_power_on(genpd);
+
+	return 0;
+}
+
+static int imx6_pm_domain_attach_dev(
+               struct generic_pm_domain *genpd,
+               struct device *dev)
+{
+	struct imx_pm_domain *pd = to_imx_pm_domain(genpd);
+
+	if (pd->flags & PGC_DOMAIN_FLAG_NO_PD)
+		device_link_add(dev, genpd->dev.parent, DL_FLAG_AUTOREMOVE);
 
 	return 0;
 }
 
 static int imx_pgc_get_clocks(struct device *dev, struct imx_pm_domain *domain)
@@ -198,10 +225,11 @@  static int imx_pgc_power_domain_probe(struct platform_device *pdev)
 	if (IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)) {
 		pm_genpd_init(&domain->base, NULL, false);
 		ret = of_genpd_add_provider_simple(dev->of_node, &domain->base);
 		if (ret)
 			goto genpd_err;
+		domain->base.dev.parent = dev;
 	}
 
 	device_link_add(dev, dev->parent, DL_FLAG_AUTOREMOVE);
 
 	return 0;
@@ -224,18 +252,45 @@  static int imx_pgc_power_domain_remove(struct platform_device *pdev)
 	}
 
 	return 0;
 }
 
+static int imx_pgc_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct imx_pm_domain *pd = pdev->dev.platform_data;
+
+	if (pd->flags & PGC_DOMAIN_FLAG_NO_PD)
+		_imx6_pm_domain_power_off(&pd->base);
+
+	return 0;
+}
+
+static int imx_pgc_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct imx_pm_domain *pd = pdev->dev.platform_data;
+
+	if (pd->flags & PGC_DOMAIN_FLAG_NO_PD)
+		_imx6_pm_domain_power_on(&pd->base);
+
+	return 0;
+}
+
+static const struct dev_pm_ops imx_pgc_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(imx_pgc_suspend, imx_pgc_resume)
+};
+
 static const struct platform_device_id imx_pgc_power_domain_id[] = {
 	{ "imx-pgc-power-domain"},
 	{ },
 };
 
 static struct platform_driver imx_pgc_power_domain_driver = {
 	.driver = {
 		.name = "imx-pgc-pd",
+		.pm = &imx_pgc_pm_ops,
 	},
 	.probe = imx_pgc_power_domain_probe,
 	.remove = imx_pgc_power_domain_remove,
 	.id_table = imx_pgc_power_domain_id,
 };
@@ -259,28 +314,31 @@  static struct imx_pm_domain imx_gpc_domains[] = {
 	}, {
 		.base = {
 			.name = "PU",
 			.power_off = imx6_pm_domain_power_off,
 			.power_on = imx6_pm_domain_power_on,
+			.attach_dev = imx6_pm_domain_attach_dev,
 			.states = &imx6_pm_domain_pu_state,
 			.state_count = 1,
 		},
 		.reg_offs = 0x260,
 		.cntr_pdn_bit = 0,
 	}, {
 		.base = {
 			.name = "DISPLAY",
 			.power_off = imx6_pm_domain_power_off,
 			.power_on = imx6_pm_domain_power_on,
+			.attach_dev = imx6_pm_domain_attach_dev,
 		},
 		.reg_offs = 0x240,
 		.cntr_pdn_bit = 4,
 	}, {
 		.base = {
 			.name = "PCI",
 			.power_off = imx6_pm_domain_power_off,
 			.power_on = imx6_pm_domain_power_on,
+			.attach_dev = imx6_pm_domain_attach_dev,
 		},
 		.reg_offs = 0x200,
 		.cntr_pdn_bit = 6,
 	},
 };