linux-kernel - Re: [PATCH] clk: at91: Add sama5d2 suspend/resume

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Mon, 24 Apr 2017 18:25:31 +0200
From:   Nicolas Ferre <nicolas.ferre@...rochip.com>
To:     Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Stephen Boyd <sboyd@...eaurora.org>
CC:     <linux-clk@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] clk: at91: Add sama5d2 suspend/resume

Le 14/04/2017 à 23:56, Alexandre Belloni a écrit :
> On sama5d2, VDD core maybe be cut while in suspend. This means registers
> will be lost. Ensure they are saved and restored properly.

I'm pretty sure we can't just restore them like you do. But the
bootloader must have restored the registers already, so maybe an update
of the regmap cache should be sufficient...

> Signed-off-by: Alexandre Belloni <alexandre.belloni@...e-electrons.com>
> ---
>  drivers/clk/at91/pmc.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 108 insertions(+)
> 
> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
> index 526df5ba042d..5cd1f0a2ea72 100644
> --- a/drivers/clk/at91/pmc.c
> +++ b/drivers/clk/at91/pmc.c
> @@ -13,7 +13,9 @@
>  #include <linux/clk/at91_pmc.h>
>  #include <linux/of.h>
>  #include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
>  #include <linux/regmap.h>
> +#include <linux/syscore_ops.h>
>  
>  #include <asm/proc-fns.h>
>  
> @@ -41,3 +43,109 @@ int of_at91_get_clk_range(struct device_node *np, const char *propname,
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(of_at91_get_clk_range);
> +
> +#ifdef CONFIG_PM
> +static struct regmap *pmcreg;
> +
> +static struct
> +{
> +	u32 scsr;
> +	u32 pcsr0;
> +	u32 uckr;
> +	u32 mor;
> +	u32 mcfr;
> +	u32 pllar;
> +	u32 mckr;
> +	u32 usb;
> +	u32 imr;
> +	u32 pcsr1;
> +	u32 pcr[64];
> +} pmc_cache;
> +
> +static int pmc_suspend(void)
> +{
> +	int i;
> +
> +	regmap_read(pmcreg, AT91_PMC_IMR, &pmc_cache.scsr);
> +	regmap_read(pmcreg, AT91_PMC_PCSR, &pmc_cache.pcsr0);
> +	regmap_read(pmcreg, AT91_CKGR_UCKR, &pmc_cache.uckr);
> +	regmap_read(pmcreg, AT91_CKGR_MOR, &pmc_cache.mor);
> +	regmap_read(pmcreg, AT91_CKGR_MCFR, &pmc_cache.mcfr);
> +	regmap_read(pmcreg, AT91_CKGR_PLLAR, &pmc_cache.pllar);
> +	regmap_read(pmcreg, AT91_PMC_MCKR, &pmc_cache.mckr);
> +	regmap_read(pmcreg, AT91_PMC_USB, &pmc_cache.usb);
> +	regmap_read(pmcreg, AT91_PMC_IMR, &pmc_cache.imr);
> +	regmap_read(pmcreg, AT91_PMC_PCSR1, &pmc_cache.pcsr1);
> +
> +	for (i = 2; i < 64; i++) {
> +		regmap_write(pmcreg, AT91_PMC_PCR, (i & AT91_PMC_PCR_PID_MASK));
> +		regmap_read(pmcreg, AT91_PMC_PCR, &pmc_cache.pcr[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +static bool pmc_ready(unsigned int mask)
> +{
> +	unsigned int status;
> +
> +	regmap_read(pmcreg, AT91_PMC_SR, &status);
> +
> +	return ((status & mask) == mask) ? 1 : 0;
> +}
> +
> +static void pmc_resume(void)
> +{
> +	int i;
> +	unsigned int mask = AT91_PMC_MCKRDY | AT91_PMC_LOCKA;

...Moreover, some of these registers needs a particular sequencing to
behave properly while writing to it. The rationale behind that (if we
can speak about rationale) is that you should only change *one* field at
a time and for some fields wait for the proper "ready bit" to show up.

A proven good sequence is available in AT91Bootstrap:

> +	regmap_write(pmcreg, AT91_PMC_IMR, pmc_cache.scsr);
> +	regmap_write(pmcreg, AT91_PMC_PCER, pmc_cache.pcsr0);
> +	regmap_write(pmcreg, AT91_CKGR_UCKR, pmc_cache.uckr);
> +	regmap_write(pmcreg, AT91_CKGR_MOR, pmc_cache.mor);
> +	regmap_write(pmcreg, AT91_CKGR_MCFR, pmc_cache.mcfr);

for this one:
> +	regmap_write(pmcreg, AT91_CKGR_PLLAR, pmc_cache.pllar);
https://github.com/linux4sam/at91bootstrap/blob/master/driver/pmc.c#L156

and this one at least:
> +	regmap_write(pmcreg, AT91_PMC_MCKR, pmc_cache.mckr);
https://github.com/linux4sam/at91bootstrap/blob/master/driver/pmc.c#L168

> +	regmap_write(pmcreg, AT91_PMC_USB, pmc_cache.usb);
> +	regmap_write(pmcreg, AT91_PMC_IMR, pmc_cache.imr);
> +	regmap_write(pmcreg, AT91_PMC_PCER1, pmc_cache.pcsr1);
> +
> +	for (i = 2; i < 64; i++) {
> +		regmap_write(pmcreg, AT91_PMC_PCR, (i & AT91_PMC_PCR_PID_MASK));
> +		regmap_write(pmcreg, AT91_PMC_PCR, pmc_cache.pcr[i]);
> +	}
> +
> +	if (pmc_cache.uckr & AT91_PMC_UPLLEN)
> +		mask |= AT91_PMC_LOCKU;
> +
> +	while (!pmc_ready(mask))
> +		cpu_relax();
> +}
> +
> +static struct syscore_ops pmc_syscore_ops = {
> +	.suspend = pmc_suspend,
> +	.resume = pmc_resume,
> +};
> +
> +static const struct of_device_id sama5d2_pmc_dt_ids[] = {
> +	{ .compatible = "atmel,sama5d2-pmc" },
> +	{ /* sentinel */ }
> +};
> +
> +static int __init pmc_register_ops(void)
> +{
> +	struct device_node *np;
> +
> +	np = of_find_matching_node(NULL, sama5d2_pmc_dt_ids);
> +
> +	pmcreg = syscon_node_to_regmap(np);
> +	if (IS_ERR(pmcreg))
> +		return PTR_ERR(pmcreg);
> +
> +	register_syscore_ops(&pmc_syscore_ops);
> +
> +	return 0;
> +}
> +/* This has to happen before arch_initcall becaues of the tcb_clksrc driver */
> +postcore_initcall(pmc_register_ops);
> +#endif
> 


-- 
Nicolas Ferre