Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755331AbcJLNEh (ORCPT ); Wed, 12 Oct 2016 09:04:37 -0400 Received: from mo1.mail-out.ovh.net ([178.32.228.1]:50572 "EHLO mo1.mail-out.ovh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932939AbcJLNEZ (ORCPT ); Wed, 12 Oct 2016 09:04:25 -0400 X-Greylist: delayed 910 seconds by postgrey-1.27 at vger.kernel.org; Wed, 12 Oct 2016 09:04:25 EDT MIME-Version: 1.0 In-Reply-To: <20161007163427.11454-3-alexandre.belloni@free-electrons.com> References: <20161007163427.11454-1-alexandre.belloni@free-electrons.com> <20161007163427.11454-3-alexandre.belloni@free-electrons.com> From: Jean-Jacques Hiblot Date: Wed, 12 Oct 2016 14:48:27 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 2/2] power/reset: at91-poweroff: timely shitdown LPDDR memories To: Alexandre Belloni Cc: Sebastian Reichel , Dmitry Eremin-Solenikov , Nicolas Ferre , Linux Kernel Mailing List , "linux-arm-kernel@lists.infradead.org" , linux-pm@vger.kernel.org Content-Type: text/plain; charset=UTF-8 X-Ovh-Tracer-Id: 3550525358364055575 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrfeelvddrgedvgdehhecutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjpdevjffgvefmvefgnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8027 Lines: 229 2016-10-07 18:34 GMT+02:00 Alexandre Belloni : > LPDDR memories can only handle up to 400 uncontrolled power off. Ensure the > proper power off sequence is used before shutting down the platform. > > Signed-off-by: Alexandre Belloni > --- > drivers/power/reset/at91-poweroff.c | 52 +++++++++++++++++++++++++++++++- > drivers/power/reset/at91-sama5d2_shdwc.c | 48 ++++++++++++++++++++++++++++- > 2 files changed, 98 insertions(+), 2 deletions(-) > > diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c > index e9e24df35f26..bf97390e6cd7 100644 > --- a/drivers/power/reset/at91-poweroff.c > +++ b/drivers/power/reset/at91-poweroff.c > @@ -14,9 +14,12 @@ > #include > #include > #include > +#include > #include > #include > > +#include > + > #define AT91_SHDW_CR 0x00 /* Shut Down Control Register */ > #define AT91_SHDW_SHDW BIT(0) /* Shut Down command */ > #define AT91_SHDW_KEY (0xa5 << 24) /* KEY Password */ > @@ -50,6 +53,7 @@ static const char *shdwc_wakeup_modes[] = { > > static void __iomem *at91_shdwc_base; > static struct clk *sclk; > +static void __iomem *mpddrc_base; > > static void __init at91_wakeup_status(void) > { > @@ -73,6 +77,28 @@ static void at91_poweroff(void) > writel(AT91_SHDW_KEY | AT91_SHDW_SHDW, at91_shdwc_base + AT91_SHDW_CR); > } > > +static void at91_lpddr_poweroff(void) > +{ > + asm volatile( > + /* Align to cache lines */ > + ".balign 32\n\t" > + > + " ldr r6, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t" At first sight, it looks useless. I assume it's used to preload the TLB before the LPDDR is turned off. A comment to explain why this line is useful would prevent its removal. > + > + /* Power down SDRAM0 */ > + " str %1, [%0, #" __stringify(AT91_DDRSDRC_LPR) "]\n\t" > + /* Shutdown CPU */ > + " str %3, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t" > + > + " b .\n\t" > + : > + : "r" (mpddrc_base), > + "r" cpu_to_le32(AT91_DDRSDRC_LPDDR2_PWOFF), > + "r" (at91_shdwc_base), > + "r" cpu_to_le32(AT91_SHDW_KEY | AT91_SHDW_SHDW) > + : "r0"); > +} > + > static int at91_poweroff_get_wakeup_mode(struct device_node *np) > { > const char *pm; > @@ -124,6 +150,8 @@ static void at91_poweroff_dt_set_wakeup_mode(struct platform_device *pdev) > static int __init at91_poweroff_probe(struct platform_device *pdev) > { > struct resource *res; > + struct device_node *np; > + u32 ddr_type; > int ret; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > @@ -150,12 +178,29 @@ static int __init at91_poweroff_probe(struct platform_device *pdev) > > pm_power_off = at91_poweroff; > > + np = of_find_compatible_node(NULL, NULL, "atmel,sama5d3-ddramc"); > + if (!np) > + return 0; > + > + mpddrc_base = of_iomap(np, 0); > + of_node_put(np); > + > + if (!mpddrc_base) > + return 0; > + > + ddr_type = readl(mpddrc_base + AT91_DDRSDRC_MDR) & AT91_DDRSDRC_MD; > + if ((ddr_type == AT91_DDRSDRC_MD_LPDDR2) || > + (ddr_type == AT91_DDRSDRC_MD_LPDDR3)) Souldn't there be something like "pm_power_off = at91_lpddr_poweroff;" here ? Jean-Jacques > + else > + iounmap(mpddrc_base); > + > return 0; > } > > static int __exit at91_poweroff_remove(struct platform_device *pdev) > { > - if (pm_power_off == at91_poweroff) > + if (pm_power_off == at91_poweroff || > + pm_power_off == at91_lpddr_poweroff) > pm_power_off = NULL; > > clk_disable_unprepare(sclk); > @@ -163,6 +208,11 @@ static int __exit at91_poweroff_remove(struct platform_device *pdev) > return 0; > } > > +static const struct of_device_id at91_ramc_of_match[] = { > + { .compatible = "atmel,sama5d3-ddramc", }, > + { /* sentinel */ } > +}; > + > static const struct of_device_id at91_poweroff_of_match[] = { > { .compatible = "atmel,at91sam9260-shdwc", }, > { .compatible = "atmel,at91sam9rl-shdwc", }, > diff --git a/drivers/power/reset/at91-sama5d2_shdwc.c b/drivers/power/reset/at91-sama5d2_shdwc.c > index 8a5ac9706c9c..5736f360b374 100644 > --- a/drivers/power/reset/at91-sama5d2_shdwc.c > +++ b/drivers/power/reset/at91-sama5d2_shdwc.c > @@ -22,9 +22,12 @@ > #include > #include > #include > +#include > #include > #include > > +#include > + > #define SLOW_CLOCK_FREQ 32768 > > #define AT91_SHDW_CR 0x00 /* Shut Down Control Register */ > @@ -75,6 +78,7 @@ struct shdwc { > */ > static struct shdwc *at91_shdwc; > static struct clk *sclk; > +static void __iomem *mpddrc_base; > > static const unsigned long long sdwc_dbc_period[] = { > 0, 3, 32, 512, 4096, 32768, > @@ -108,6 +112,28 @@ static void at91_poweroff(void) > at91_shdwc->at91_shdwc_base + AT91_SHDW_CR); > } > > +static void at91_lpddr_poweroff(void) > +{ > + asm volatile( > + /* Align to cache lines */ > + ".balign 32\n\t" > + > + " ldr r6, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t" > + > + /* Power down SDRAM0 */ > + " str %1, [%0, #" __stringify(AT91_DDRSDRC_LPR) "]\n\t" > + /* Shutdown CPU */ > + " str %3, [%2, #" __stringify(AT91_SHDW_CR) "]\n\t" > + > + " b .\n\t" > + : > + : "r" (mpddrc_base), > + "r" cpu_to_le32(AT91_DDRSDRC_LPDDR2_PWOFF), > + "r" (at91_shdwc->at91_shdwc_base), > + "r" cpu_to_le32(AT91_SHDW_KEY | AT91_SHDW_SHDW) > + : "r0"); > +} > + > static u32 at91_shdwc_debouncer_value(struct platform_device *pdev, > u32 in_period_us) > { > @@ -212,6 +238,8 @@ static int __init at91_shdwc_probe(struct platform_device *pdev) > { > struct resource *res; > const struct of_device_id *match; > + struct device_node *np; > + u32 ddr_type; > int ret; > > if (!pdev->dev.of_node) > @@ -249,6 +277,23 @@ static int __init at91_shdwc_probe(struct platform_device *pdev) > > pm_power_off = at91_poweroff; > > + np = of_find_compatible_node(NULL, NULL, "atmel,sama5d3-ddramc"); > + if (!np) > + return 0; > + > + mpddrc_base = of_iomap(np, 0); > + of_node_put(np); > + > + if (!mpddrc_base) > + return 0; > + > + ddr_type = readl(mpddrc_base + AT91_DDRSDRC_MDR) & AT91_DDRSDRC_MD; > + if ((ddr_type == AT91_DDRSDRC_MD_LPDDR2) || > + (ddr_type == AT91_DDRSDRC_MD_LPDDR3)) > + pm_power_off = at91_lpddr_poweroff; > + else > + iounmap(mpddrc_base); > + > return 0; > } > > @@ -256,7 +301,8 @@ static int __exit at91_shdwc_remove(struct platform_device *pdev) > { > struct shdwc *shdw = platform_get_drvdata(pdev); > > - if (pm_power_off == at91_poweroff) > + if (pm_power_off == at91_poweroff || > + pm_power_off == at91_lpddr_poweroff) > pm_power_off = NULL; > > /* Reset values to disable wake-up features */ > -- > 2.9.3 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel