Re: [PATCH v2] soc: imx: gpc: Turn PU domain on/off in sleep on 6qp

[Lucas Stach]

Am Donnerstag, den 05.07.2018, 18:12 +0300 schrieb Leonard Crestez:
> 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.
> 
> > Signed-off-by: Leonard Crestez <leonard.crestez@xxxxxxx>

This looks much better than the previous version.

> ---
> Âdrivers/soc/imx/gpc.c | 72 ++++++++++++++++++++++++++++++++++---------
> Â1 file changed, 57 insertions(+), 15 deletions(-)
> 
> Changes since v1: Implement SLEEP_PM_OPS instead of calling from
> platform-level suspend code, as suggested by Lucas.
> 
> I'm not sure if doing interesting things in the pm_ops for PM providers
> is recommended. Is correct ordering guaranteed in such a case?ÂÂAny
> VPU/GPU suspend code must execute before imx_pgc_suspend or it might try
> to access registers in a region which is powered off.
> 
> Perhaps ordering needs to be guaranteed by implementing device_attach
> and adding explicit device links to pgc platform_device?

Yes, I think we need device links between the users of the power
domains and the PGC devices. Since this is one of the use-cases for
which device links were conceived I would guess that implementing this
should be fairly straight-forward.

> Link to v1: https://lkml.org/lkml/2018/7/2/370
> 
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index b4acdfd3cffd..96d9e882d583 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);

If possible the regulator should also be disabled in the system suspend
path, as this might reduce power consumption some more. But then this
might have issues with suspend ordering of the regulator.

As I don't want to send you down the rabbit hole too much with adding
ordering to this stuff, I won't object to this going in as-is. If this
is in fact due to a ordering limitation I would like to see this
documented in a code comment.

Regards,
Lucas

> Â
> > Â	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,27 @@ 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 imx_pgc_get_clocks(struct device *dev, struct imx_pm_domain *domain)
> @@ -227,18 +242,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,
> Â};