[PATCH 0/2] ARM: at91: properly handle LPDDR poweroff

[PATCH 0/2] ARM: at91: properly handle LPDDR poweroff

[Alexandre Belloni]

Hi,

This patch set improves LPDDR support on SoCs using the Atmel MPDDR controller.

LPDDR memoris can only handle up to 400 uncontrolled power offs in their
life. The proper power off sequence has to be applied before shutting down the SoC.

I'm not too happy with the code duplication but this is a design choice
that has been made before because both shitdown controler are really
different appart from the shutdown itself.

I guess it is still better than slowly killing the LPDDR.

Alexandre Belloni (2):
  ARM: at91: define LPDDR types
  power/reset: at91-poweroff: timely shitdown LPDDR memories

 drivers/power/reset/at91-poweroff.c      | 52 +++++++++++++++++++++++++++++++-
 drivers/power/reset/at91-sama5d2_shdwc.c | 48 ++++++++++++++++++++++++++++-
 include/soc/at91/at91sam9_ddrsdr.h       |  3 ++
 3 files changed, 101 insertions(+), 2 deletions(-)

-- 
2.9.3

[PATCH 1/2] ARM: at91: define LPDDR types

[Alexandre Belloni]

The Atmel MPDDR controller support LPDDR2 and LPDDR3 memories, add their
types.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 include/soc/at91/at91sam9_ddrsdr.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/soc/at91/at91sam9_ddrsdr.h b/include/soc/at91/at91sam9_ddrsdr.h
index dc10c52e0e91..393362bdb860 100644
--- a/include/soc/at91/at91sam9_ddrsdr.h
+++ b/include/soc/at91/at91sam9_ddrsdr.h
@@ -81,6 +81,7 @@
 #define			AT91_DDRSDRC_LPCB_POWER_DOWN		2
 #define			AT91_DDRSDRC_LPCB_DEEP_POWER_DOWN	3
 #define		AT91_DDRSDRC_CLKFR	(1 << 2)	/* Clock Frozen */
+#define		AT91_DDRSDRC_LPDDR2_PWOFF	(1 << 3)	/* LPDDR Power Off */
 #define		AT91_DDRSDRC_PASR	(7 << 4)	/* Partial Array Self Refresh */
 #define		AT91_DDRSDRC_TCSR	(3 << 8)	/* Temperature Compensated Self Refresh */
 #define		AT91_DDRSDRC_DS		(3 << 10)	/* Drive Strength */
@@ -96,7 +97,9 @@
 #define			AT91_DDRSDRC_MD_SDR		0
 #define			AT91_DDRSDRC_MD_LOW_POWER_SDR	1
 #define			AT91_DDRSDRC_MD_LOW_POWER_DDR	3
+#define			AT91_DDRSDRC_MD_LPDDR3		5
 #define			AT91_DDRSDRC_MD_DDR2		6	/* [SAM9 Only] */
+#define			AT91_DDRSDRC_MD_LPDDR2		7
 #define		AT91_DDRSDRC_DBW	(1 << 4)		/* Data Bus Width */
 #define			AT91_DDRSDRC_DBW_32BITS		(0 <<  4)
 #define			AT91_DDRSDRC_DBW_16BITS		(1 <<  4)
-- 
2.9.3

[PATCH 2/2] power/reset: at91-poweroff: timely shitdown LPDDR memories

[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 <alexandre.belloni@free-electrons.com>
---
 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 <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/printk.h>
 
+#include <soc/at91/at91sam9_ddrsdr.h>
+
 #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"
+
+		/* 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))
+	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 <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/printk.h>
 
+#include <soc/at91/at91sam9_ddrsdr.h>
+
 #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

Re: [PATCH 0/2] ARM: at91: properly handle LPDDR poweroff

[Alexander Stein]

Hi Alexandre,

On Friday 07 October 2016 18:34:25, Alexandre Belloni wrote:
> Hi,
> 
> This patch set improves LPDDR support on SoCs using the Atmel MPDDR
> controller.
> 
> LPDDR memoris can only handle up to 400 uncontrolled power offs in their
> life. The proper power off sequence has to be applied before shutting down
> the SoC.
> 
> I'm not too happy with the code duplication but this is a design choice
> that has been made before because both shitdown controler are really
                                         ^^^^^^^^

I guess you mean shutdown? :) Same for the suject of 2nd patch.

Best regards,
Alexander

Re: [PATCH 0/2] ARM: at91: properly handle LPDDR poweroff

[Alexandre Belloni]

On 10/10/2016 at 08:00:30 +0200, Alexander Stein wrote :
> Hi Alexandre,
> 
> On Friday 07 October 2016 18:34:25, Alexandre Belloni wrote:
> > Hi,
> > 
> > This patch set improves LPDDR support on SoCs using the Atmel MPDDR
> > controller.
> > 
> > LPDDR memoris can only handle up to 400 uncontrolled power offs in their
> > life. The proper power off sequence has to be applied before shutting down
> > the SoC.
> > 
> > I'm not too happy with the code duplication but this is a design choice
> > that has been made before because both shitdown controler are really
>                                          ^^^^^^^^
> 
> I guess you mean shutdown? :) Same for the suject of 2nd patch.
> 

I'm planning to send a v2, hopefully fixing my typos ;)

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/2] power/reset: at91-poweroff: timely shitdown LPDDR memories

[Jean-Jacques Hiblot]

2016-10-07 18:34 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> 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 <alexandre.belloni@free-electrons.com>
> ---
>  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 <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/platform_device.h>
>  #include <linux/printk.h>
>
> +#include <soc/at91/at91sam9_ddrsdr.h>
> +
>  #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 <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/platform_device.h>
>  #include <linux/printk.h>
>
> +#include <soc/at91/at91sam9_ddrsdr.h>
> +
>  #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

Re: [PATCH 2/2] power/reset: at91-poweroff: timely shitdown LPDDR memories

[Alexandre Belloni]

On 12/10/2016 at 14:48:27 +0200, Jean-Jacques Hiblot wrote :
> > +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.

Yes, this is the case. I can add a comment.

Anyway, I would prefer the whole thing to run from SRAM, as a PIE
instead of relying on the cache.

> > +       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 ?
> 

Indeed


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/2] power/reset: at91-poweroff: timely shitdown LPDDR memories

[Jean-Jacques Hiblot]

2016-10-13 13:03 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 12/10/2016 at 14:48:27 +0200, Jean-Jacques Hiblot wrote :
>> > +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.
>
> Yes, this is the case. I can add a comment.
>
> Anyway, I would prefer the whole thing to run from SRAM, as a PIE
> instead of relying on the cache.

Instead of copying into the SRAM, you can make the cache reliable by
preloading it, much like the TLB.
LDI is probably not available for most of atmel's SOC, so the only way
I can think of, is to execute code from the targeted area. here is an
example:
+               /*
+                * Jump to the end of the sequence to preload instruction cache
+                * It only works because the sequence is short enough not to
+                * sit accross more than 2 cache lines
+                */
+               "       b end_of_sequence\n\t"
+               "start_of_sequence:\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"
+
+               /*
+                * we're now 100% sure that the code to shutdown the LPDDR and
+                * the CPU is in cache, go back to do the actual job
+                */
+               "end_of_sequence:\n\t"
+               "       b start_of_sequence\n\t"
                :


>
>> > +       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 ?
>>
>
> Indeed
>
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH 2/2] power/reset: at91-poweroff: timely shitdown LPDDR memories

[Richard Genoud]

2016-10-13 14:27 GMT+02:00 Jean-Jacques Hiblot <jjhiblot@traphandler.com>:
> 2016-10-13 13:03 GMT+02:00 Alexandre Belloni
> <alexandre.belloni@free-electrons.com>:
>> On 12/10/2016 at 14:48:27 +0200, Jean-Jacques Hiblot wrote :
>>> > +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.
>>
>> Yes, this is the case. I can add a comment.
>>
>> Anyway, I would prefer the whole thing to run from SRAM, as a PIE
>> instead of relying on the cache.
>
> Instead of copying into the SRAM, you can make the cache reliable by
> preloading it, much like the TLB.
> LDI is probably not available for most of atmel's SOC, so the only way
> I can think of, is to execute code from the targeted area. here is an
> example:
> +               /*
> +                * Jump to the end of the sequence to preload instruction cache
> +                * It only works because the sequence is short enough not to
> +                * sit accross more than 2 cache lines
> +                */
> +               "       b end_of_sequence\n\t"
> +               "start_of_sequence:\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"
> +
> +               /*
> +                * we're now 100% sure that the code to shutdown the LPDDR and
> +                * the CPU is in cache, go back to do the actual job
> +                */
> +               "end_of_sequence:\n\t"
> +               "       b start_of_sequence\n\t"
>                 :

My 2c: I think you may want to change your subject :)

Richard.

Re: [PATCH 2/2] power/reset: at91-poweroff: timely shitdown LPDDR memories

[Alexandre Belloni]

On 13/10/2016 at 14:27:15 +0200, Jean-Jacques Hiblot wrote :
> 2016-10-13 13:03 GMT+02:00 Alexandre Belloni
> <alexandre.belloni@free-electrons.com>:
> > On 12/10/2016 at 14:48:27 +0200, Jean-Jacques Hiblot wrote :
> >> > +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.
> >
> > Yes, this is the case. I can add a comment.
> >
> > Anyway, I would prefer the whole thing to run from SRAM, as a PIE
> > instead of relying on the cache.
> 
> Instead of copying into the SRAM, you can make the cache reliable by
> preloading it, much like the TLB.
> LDI is probably not available for most of atmel's SOC, so the only way
> I can think of, is to execute code from the targeted area. here is an
> example:
> +               /*
> +                * Jump to the end of the sequence to preload instruction cache
> +                * It only works because the sequence is short enough not to
> +                * sit accross more than 2 cache lines
> +                */
> +               "       b end_of_sequence\n\t"
> +               "start_of_sequence:\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"
> +
> +               /*
> +                * we're now 100% sure that the code to shutdown the LPDDR and
> +                * the CPU is in cache, go back to do the actual job
> +                */
> +               "end_of_sequence:\n\t"
> +               "       b start_of_sequence\n\t"
>                 :
> 

I don't think this is necessary. By aligning the instructions properly,
we are already sure the whole code is loaded into the cache.

My plan is to get rid of the assembly and use PIE so it is written in C
and we can properly separate the RAM stuff in the ddrc driver.

The mpddrc driver could load its shutdown function in SRAM. The reset
controller driver would load the reset function in SRAM and the shutdown
controller would load the poweroff function in SRAM. It would e quite
cleaner than what we have here.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/2] power/reset: at91-poweroff: timely shitdown LPDDR memories

[Jean-Jacques Hiblot]

2016-10-13 15:47 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 13/10/2016 at 14:27:15 +0200, Jean-Jacques Hiblot wrote :
>> 2016-10-13 13:03 GMT+02:00 Alexandre Belloni
>> <alexandre.belloni@free-electrons.com>:
>> > On 12/10/2016 at 14:48:27 +0200, Jean-Jacques Hiblot wrote :
>> >> > +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.
>> >
>> > Yes, this is the case. I can add a comment.
>> >
>> > Anyway, I would prefer the whole thing to run from SRAM, as a PIE
>> > instead of relying on the cache.
>>
>> Instead of copying into the SRAM, you can make the cache reliable by
>> preloading it, much like the TLB.
>> LDI is probably not available for most of atmel's SOC, so the only way
>> I can think of, is to execute code from the targeted area. here is an
>> example:
>> +               /*
>> +                * Jump to the end of the sequence to preload instruction cache
>> +                * It only works because the sequence is short enough not to
>> +                * sit accross more than 2 cache lines
>> +                */
>> +               "       b end_of_sequence\n\t"
>> +               "start_of_sequence:\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"
>> +
>> +               /*
>> +                * we're now 100% sure that the code to shutdown the LPDDR and
>> +                * the CPU is in cache, go back to do the actual job
>> +                */
>> +               "end_of_sequence:\n\t"
>> +               "       b start_of_sequence\n\t"
>>                 :
>>
>
> I don't think this is necessary. By aligning the instructions properly,
> we are already sure the whole code is loaded into the cache.
right I didn't see the align directive.
>
> My plan is to get rid of the assembly and use PIE so it is written in C
> and we can properly separate the RAM stuff in the ddrc driver.
>
> The mpddrc driver could load its shutdown function in SRAM. The reset
> controller driver would load the reset function in SRAM and the shutdown
> controller would load the poweroff function in SRAM. It would e quite
> cleaner than what we have here.
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com