[PATCH 0/7] AT91 pm improvements for 3.20

[PATCH 0/7] AT91 pm improvements for 3.20

[Wenyou Yang]

Hi Nicolas,

This patch set is add pm support for ARMv7 SoCs.
 - Add WFI support for ARMv7.
 - Disable the L1 D-cache and L2 cache before suspending.
 - Get the memory type from the dts file config.
 - Disable the mpddr controller's clock and DDR clock during the suspending.

It is based on the following patch set,
	[PATCH v2 00/12] AT91 pm cleanup for 3.20

Patrice Vilchez (1):
  pm: at91: pm_suspend: MOR register KEY was missing

Wenyou Yang (6):
  pm: at91: achieve the memory controller's type from the dts file.
  pm: at91: pm_suspend: add the WFI support for ARMv7
  ARM: at91: enable the L2 Cache controller
  pm: at91: add disable/enable the L1/L2 cache while suspend/resume
  pm: at91: add achieve the mpddrc peripheral ID and the DDR clock ID
    support
  pm: at91: add disable/enable the mpddrc's clock and DDR clock support

 arch/arm/mach-at91/board-dt-sama5.c |   53 ++++++++
 arch/arm/mach-at91/generic.h        |    7 ++
 arch/arm/mach-at91/pm.c             |   22 ++--
 arch/arm/mach-at91/pm.h             |    8 ++
 arch/arm/mach-at91/pm_suspend.S     |  227 ++++++++++++++++++++++++++++++++++-
 arch/arm/mach-at91/setup.c          |   52 +++++++-
 6 files changed, 356 insertions(+), 13 deletions(-)

-- 
1.7.9.5

[PATCH 0/7] AT91 pm improvements for 3.20

[Wenyou Yang]

Hi Nicolas,

This patch set is add pm support for ARMv7 SoCs.
 - Add WFI support for ARMv7.
 - Disable the L1 D-cache and L2 cache before suspending.
 - Get the memory type from the dts file config.
 - Disable the mpddr controller's clock and DDR clock during the suspending.

It is based on the following patch set,
	[PATCH v2 00/12] AT91 pm cleanup for 3.20

Patrice Vilchez (1):
  pm: at91: pm_suspend: MOR register KEY was missing

Wenyou Yang (6):
  pm: at91: achieve the memory controller's type from the dts file.
  pm: at91: pm_suspend: add the WFI support for ARMv7
  ARM: at91: enable the L2 Cache controller
  pm: at91: add disable/enable the L1/L2 cache while suspend/resume
  pm: at91: add achieve the mpddrc peripheral ID and the DDR clock ID
    support
  pm: at91: add disable/enable the mpddrc's clock and DDR clock support

 arch/arm/mach-at91/board-dt-sama5.c |   53 ++++++++
 arch/arm/mach-at91/generic.h        |    7 ++
 arch/arm/mach-at91/pm.c             |   22 ++--
 arch/arm/mach-at91/pm.h             |    8 ++
 arch/arm/mach-at91/pm_suspend.S     |  227 ++++++++++++++++++++++++++++++++++-
 arch/arm/mach-at91/setup.c          |   52 +++++++-
 6 files changed, 356 insertions(+), 13 deletions(-)

-- 
1.7.9.5

[PATCH 1/7] pm: at91: achieve the memory controller's type from the dts file.

[Wenyou Yang]

Instead of achieve the ram controller's tpye through the SoC,
through the sram controller configuration, it is more sensible.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 arch/arm/mach-at91/generic.h |    5 +++++
 arch/arm/mach-at91/pm.c      |    9 +--------
 arch/arm/mach-at91/setup.c   |   28 ++++++++++++++++++++++++----
 3 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-at91/generic.h b/arch/arm/mach-at91/generic.h
index a8ee83e..41796bf 100644
--- a/arch/arm/mach-at91/generic.h
+++ b/arch/arm/mach-at91/generic.h
@@ -44,4 +44,9 @@ void __init at91_sam9g45_pm_init(void) { }
 void __init at91_sam9x5_pm_init(void) { }
 #endif
 
+struct at91_pm_struct {
+	unsigned long uhp_udp_mask;
+	int memctrl;
+};
+
 #endif /* _AT91_GENERIC_H */
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 7473978..f75dc32 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -35,10 +35,7 @@
 #include "generic.h"
 #include "pm.h"
 
-static struct {
-	unsigned long uhp_udp_mask;
-	int memctrl;
-} at91_pm_data;
+struct at91_pm_struct at91_pm_data;
 
 static int at91_pm_valid_state(suspend_state_t state)
 {
@@ -287,14 +284,12 @@ void __init at91_rm9200_pm_init(void)
 	at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
 
 	at91_pm_data.uhp_udp_mask = AT91RM9200_PMC_UHP | AT91RM9200_PMC_UDP;
-	at91_pm_data.memctrl = AT91_MEMCTRL_MC;
 
 	at91_pm_init();
 }
 
 void __init at91_sam9260_pm_init(void)
 {
-	at91_pm_data.memctrl = AT91_MEMCTRL_SDRAMC;
 	at91_pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP;
 	return at91_pm_init();
 }
@@ -302,13 +297,11 @@ void __init at91_sam9260_pm_init(void)
 void __init at91_sam9g45_pm_init(void)
 {
 	at91_pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP;
-	at91_pm_data.memctrl = AT91_MEMCTRL_DDRSDR;
 	return at91_pm_init();
 }
 
 void __init at91_sam9x5_pm_init(void)
 {
 	at91_pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP;
-	at91_pm_data.memctrl = AT91_MEMCTRL_DDRSDR;
 	return at91_pm_init();
 }
diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c
index b8dba9f..7924663 100644
--- a/arch/arm/mach-at91/setup.c
+++ b/arch/arm/mach-at91/setup.c
@@ -26,6 +26,8 @@
 #include "generic.h"
 #include "pm.h"
 
+extern struct at91_pm_struct	at91_pm_data;
+
 struct at91_init_soc __initdata at91_boot_soc;
 
 struct at91_socinfo at91_soc_initdata;
@@ -361,11 +363,25 @@ void __init at91_ioremap_matrix(u32 base_addr)
 		panic(pr_fmt("Impossible to ioremap at91_matrix_base\n"));
 }
 
+struct at91_ramc_of_data {
+	u8 ramc_type;
+};
+
+static const struct at91_ramc_of_data at91rm9200_ramc_of_data = {
+	.ramc_type = AT91_MEMCTRL_MC,
+};
+static const struct at91_ramc_of_data at91sam9260_ramc_of_data = {
+	.ramc_type = AT91_MEMCTRL_SDRAMC,
+};
+static const struct at91_ramc_of_data at91sam9g45_ramc_of_data = {
+	.ramc_type = AT91_MEMCTRL_DDRSDR,
+};
+
 static struct of_device_id ramc_ids[] = {
-	{ .compatible = "atmel,at91rm9200-sdramc" },
-	{ .compatible = "atmel,at91sam9260-sdramc" },
-	{ .compatible = "atmel,at91sam9g45-ddramc" },
-	{ .compatible = "atmel,sama5d3-ddramc" },
+	{ .compatible = "atmel,at91rm9200-sdramc", .data = &at91rm9200_ramc_of_data },
+	{ .compatible = "atmel,at91sam9260-sdramc", .data = &at91sam9260_ramc_of_data },
+	{ .compatible = "atmel,at91sam9g45-ddramc", .data = &at91sam9g45_ramc_of_data },
+	{ .compatible = "atmel,sama5d3-ddramc", .data = &at91sam9g45_ramc_of_data },
 	{ /*sentinel*/ }
 };
 
@@ -374,12 +390,16 @@ static void at91_dt_ramc(void)
 	struct device_node *np;
 	const struct of_device_id *of_id;
 	int idx = 0;
+	const struct at91_ramc_of_data *of_data;
 
 	for_each_matching_node_and_match(np, ramc_ids, &of_id) {
 		at91_ramc_base[idx] = of_iomap(np, 0);
 		if (!at91_ramc_base[idx])
 			panic(pr_fmt("unable to map ramc[%d] cpu registers\n"), idx);
 
+		of_data = of_id->data;
+		at91_pm_data.memctrl = of_data->ramc_type;
+
 		idx++;
 	}
 
-- 
1.7.9.5

[PATCH 1/7] pm: at91: achieve the memory controller's type from the dts file.

[Wenyou Yang]

Instead of achieve the ram controller's tpye through the SoC,
through the sram controller configuration, it is more sensible.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 arch/arm/mach-at91/generic.h |    5 +++++
 arch/arm/mach-at91/pm.c      |    9 +--------
 arch/arm/mach-at91/setup.c   |   28 ++++++++++++++++++++++++----
 3 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-at91/generic.h b/arch/arm/mach-at91/generic.h
index a8ee83e..41796bf 100644
--- a/arch/arm/mach-at91/generic.h
+++ b/arch/arm/mach-at91/generic.h
@@ -44,4 +44,9 @@ void __init at91_sam9g45_pm_init(void) { }
 void __init at91_sam9x5_pm_init(void) { }
 #endif
 
+struct at91_pm_struct {
+	unsigned long uhp_udp_mask;
+	int memctrl;
+};
+
 #endif /* _AT91_GENERIC_H */
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 7473978..f75dc32 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -35,10 +35,7 @@
 #include "generic.h"
 #include "pm.h"
 
-static struct {
-	unsigned long uhp_udp_mask;
-	int memctrl;
-} at91_pm_data;
+struct at91_pm_struct at91_pm_data;
 
 static int at91_pm_valid_state(suspend_state_t state)
 {
@@ -287,14 +284,12 @@ void __init at91_rm9200_pm_init(void)
 	at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
 
 	at91_pm_data.uhp_udp_mask = AT91RM9200_PMC_UHP | AT91RM9200_PMC_UDP;
-	at91_pm_data.memctrl = AT91_MEMCTRL_MC;
 
 	at91_pm_init();
 }
 
 void __init at91_sam9260_pm_init(void)
 {
-	at91_pm_data.memctrl = AT91_MEMCTRL_SDRAMC;
 	at91_pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP;
 	return at91_pm_init();
 }
@@ -302,13 +297,11 @@ void __init at91_sam9260_pm_init(void)
 void __init at91_sam9g45_pm_init(void)
 {
 	at91_pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP;
-	at91_pm_data.memctrl = AT91_MEMCTRL_DDRSDR;
 	return at91_pm_init();
 }
 
 void __init at91_sam9x5_pm_init(void)
 {
 	at91_pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP | AT91SAM926x_PMC_UDP;
-	at91_pm_data.memctrl = AT91_MEMCTRL_DDRSDR;
 	return at91_pm_init();
 }
diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c
index b8dba9f..7924663 100644
--- a/arch/arm/mach-at91/setup.c
+++ b/arch/arm/mach-at91/setup.c
@@ -26,6 +26,8 @@
 #include "generic.h"
 #include "pm.h"
 
+extern struct at91_pm_struct	at91_pm_data;
+
 struct at91_init_soc __initdata at91_boot_soc;
 
 struct at91_socinfo at91_soc_initdata;
@@ -361,11 +363,25 @@ void __init at91_ioremap_matrix(u32 base_addr)
 		panic(pr_fmt("Impossible to ioremap at91_matrix_base\n"));
 }
 
+struct at91_ramc_of_data {
+	u8 ramc_type;
+};
+
+static const struct at91_ramc_of_data at91rm9200_ramc_of_data = {
+	.ramc_type = AT91_MEMCTRL_MC,
+};
+static const struct at91_ramc_of_data at91sam9260_ramc_of_data = {
+	.ramc_type = AT91_MEMCTRL_SDRAMC,
+};
+static const struct at91_ramc_of_data at91sam9g45_ramc_of_data = {
+	.ramc_type = AT91_MEMCTRL_DDRSDR,
+};
+
 static struct of_device_id ramc_ids[] = {
-	{ .compatible = "atmel,at91rm9200-sdramc" },
-	{ .compatible = "atmel,at91sam9260-sdramc" },
-	{ .compatible = "atmel,at91sam9g45-ddramc" },
-	{ .compatible = "atmel,sama5d3-ddramc" },
+	{ .compatible = "atmel,at91rm9200-sdramc", .data = &at91rm9200_ramc_of_data },
+	{ .compatible = "atmel,at91sam9260-sdramc", .data = &at91sam9260_ramc_of_data },
+	{ .compatible = "atmel,at91sam9g45-ddramc", .data = &at91sam9g45_ramc_of_data },
+	{ .compatible = "atmel,sama5d3-ddramc", .data = &at91sam9g45_ramc_of_data },
 	{ /*sentinel*/ }
 };
 
@@ -374,12 +390,16 @@ static void at91_dt_ramc(void)
 	struct device_node *np;
 	const struct of_device_id *of_id;
 	int idx = 0;
+	const struct at91_ramc_of_data *of_data;
 
 	for_each_matching_node_and_match(np, ramc_ids, &of_id) {
 		at91_ramc_base[idx] = of_iomap(np, 0);
 		if (!at91_ramc_base[idx])
 			panic(pr_fmt("unable to map ramc[%d] cpu registers\n"), idx);
 
+		of_data = of_id->data;
+		at91_pm_data.memctrl = of_data->ramc_type;
+
 		idx++;
 	}
 
-- 
1.7.9.5

[PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Wenyou Yang]

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 arch/arm/mach-at91/pm_suspend.S |   54 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
index 122a3f1..e796722 100644
--- a/arch/arm/mach-at91/pm_suspend.S
+++ b/arch/arm/mach-at91/pm_suspend.S
@@ -53,6 +53,58 @@ mode	.req	r6
 	beq	1b
 	.endm
 
+/*
+ * Put the processor to enter the WFI state
+ */
+	.macro _do_wfi
+
+#if defined(CONFIG_CPU_V7)
+	/*
+	 * Execute an ISB instruction to flush the pipeline to ensure
+	 * that all of operations have beem completed.
+	 */
+	isb
+
+	/*
+	 * Execute an ISB instruction to ensure that all of the
+	 * CP15 register changes have been committed.
+	 */
+	dsb
+	dmb
+
+	/* Disable the processor's clock */
+	mov	tmp1, #AT91_PMC_PCK
+	str	tmp1, [pmc, #AT91_PMC_SCDR]
+
+	/* Execute a WFI instruction */
+	wfi	@ Wait For Interrupt
+
+	/*
+	 * CPU can specualatively prefetch the instructions
+	 * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline.
+	 */
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+#else
+	mcr	p15, 0, tmp1, c7, c0, 4
+#endif
+
+	.endm
+
 	.text
 
 /*
@@ -181,7 +233,7 @@ sdr_sr_done:
 
 skip_disable_main_clock:
 	/* Wait for interrupt */
-	mcr	p15, 0, tmp1, c7, c0, 4
+	_do_wfi
 
 	tst	mode, #AT91_PM_SLOW_CLOCK
 	beq	skip_enable_main_clock
-- 
1.7.9.5

[PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Wenyou Yang]

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 arch/arm/mach-at91/pm_suspend.S |   54 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
index 122a3f1..e796722 100644
--- a/arch/arm/mach-at91/pm_suspend.S
+++ b/arch/arm/mach-at91/pm_suspend.S
@@ -53,6 +53,58 @@ mode	.req	r6
 	beq	1b
 	.endm
 
+/*
+ * Put the processor to enter the WFI state
+ */
+	.macro _do_wfi
+
+#if defined(CONFIG_CPU_V7)
+	/*
+	 * Execute an ISB instruction to flush the pipeline to ensure
+	 * that all of operations have beem completed.
+	 */
+	isb
+
+	/*
+	 * Execute an ISB instruction to ensure that all of the
+	 * CP15 register changes have been committed.
+	 */
+	dsb
+	dmb
+
+	/* Disable the processor's clock */
+	mov	tmp1, #AT91_PMC_PCK
+	str	tmp1, [pmc, #AT91_PMC_SCDR]
+
+	/* Execute a WFI instruction */
+	wfi	@ Wait For Interrupt
+
+	/*
+	 * CPU can specualatively prefetch the instructions
+	 * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline.
+	 */
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+#else
+	mcr	p15, 0, tmp1, c7, c0, 4
+#endif
+
+	.endm
+
 	.text
 
 /*
@@ -181,7 +233,7 @@ sdr_sr_done:
 
 skip_disable_main_clock:
 	/* Wait for interrupt */
-	mcr	p15, 0, tmp1, c7, c0, 4
+	_do_wfi
 
 	tst	mode, #AT91_PM_SLOW_CLOCK
 	beq	skip_enable_main_clock
-- 
1.7.9.5

[PATCH 3/7] pm: at91: pm_suspend: MOR register KEY was missing

[Wenyou Yang]

From: Patrice Vilchez <patrice.vilchez@atmel.com>

Signed-off-by: Patrice Vilchez <patrice.vilchez@atmel.com>
---
 arch/arm/mach-at91/pm_suspend.S |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
index e796722..88cf228 100644
--- a/arch/arm/mach-at91/pm_suspend.S
+++ b/arch/arm/mach-at91/pm_suspend.S
@@ -229,6 +229,7 @@ sdr_sr_done:
 	/* Turn off the main oscillator */
 	ldr	tmp1, [pmc, #AT91_CKGR_MOR]
 	bic	tmp1, tmp1, #AT91_PMC_MOSCEN
+	orr	tmp1, tmp1, #AT91_PMC_KEY
 	str	tmp1, [pmc, #AT91_CKGR_MOR]
 
 skip_disable_main_clock:
@@ -241,6 +242,7 @@ skip_disable_main_clock:
 	/* Turn on the main oscillator */
 	ldr	tmp1, [pmc, #AT91_CKGR_MOR]
 	orr	tmp1, tmp1, #AT91_PMC_MOSCEN
+	orr	tmp1, tmp1, #AT91_PMC_KEY
 	str	tmp1, [pmc, #AT91_CKGR_MOR]
 
 	wait_moscrdy
-- 
1.7.9.5

[PATCH 3/7] pm: at91: pm_suspend: MOR register KEY was missing

[Wenyou Yang]

From: Patrice Vilchez <patrice.vilchez@atmel.com>

Signed-off-by: Patrice Vilchez <patrice.vilchez@atmel.com>
---
 arch/arm/mach-at91/pm_suspend.S |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
index e796722..88cf228 100644
--- a/arch/arm/mach-at91/pm_suspend.S
+++ b/arch/arm/mach-at91/pm_suspend.S
@@ -229,6 +229,7 @@ sdr_sr_done:
 	/* Turn off the main oscillator */
 	ldr	tmp1, [pmc, #AT91_CKGR_MOR]
 	bic	tmp1, tmp1, #AT91_PMC_MOSCEN
+	orr	tmp1, tmp1, #AT91_PMC_KEY
 	str	tmp1, [pmc, #AT91_CKGR_MOR]
 
 skip_disable_main_clock:
@@ -241,6 +242,7 @@ skip_disable_main_clock:
 	/* Turn on the main oscillator */
 	ldr	tmp1, [pmc, #AT91_CKGR_MOR]
 	orr	tmp1, tmp1, #AT91_PMC_MOSCEN
+	orr	tmp1, tmp1, #AT91_PMC_KEY
 	str	tmp1, [pmc, #AT91_CKGR_MOR]
 
 	wait_moscrdy
-- 
1.7.9.5

[PATCH 4/7] ARM: at91: enable the L2 Cache controller

[Wenyou Yang]

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 arch/arm/mach-at91/board-dt-sama5.c |   53 +++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
index 86cffcd..ed6db28 100644
--- a/arch/arm/mach-at91/board-dt-sama5.c
+++ b/arch/arm/mach-at91/board-dt-sama5.c
@@ -17,17 +17,70 @@
 #include <linux/of_platform.h>
 #include <linux/phy.h>
 #include <linux/clk-provider.h>
+#include <linux/of_address.h>
 
 #include <asm/setup.h>
 #include <asm/irq.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
 #include <asm/mach/irq.h>
+#include <asm/hardware/cache-l2x0.h>
 
 #include "generic.h"
 
+void __iomem *at91_l2cc_base;
+EXPORT_SYMBOL_GPL(at91_l2cc_base);
+
+#ifdef CONFIG_CACHE_L2X0
+static void __init at91_init_l2cache(void)
+{
+	struct device_node *np;
+	u32 reg;
+
+	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
+	if (!np)
+		return;
+
+	at91_l2cc_base = of_iomap(np, 0);
+	if (!at91_l2cc_base)
+		panic("unable to map l2cc cpu registers\n");
+
+	of_node_put(np);
+
+	/* Disable cache if it hasn't been done yet */
+	if (readl_relaxed(at91_l2cc_base + L2X0_CTRL) & L2X0_CTRL_EN)
+		writel_relaxed(~L2X0_CTRL_EN, at91_l2cc_base + L2X0_CTRL);
+
+	/* Prefetch Control */
+	reg = readl_relaxed(at91_l2cc_base + L310_PREFETCH_CTRL);
+	reg &= ~L310_PREFETCH_CTRL_OFFSET_MASK;
+	reg |= 0x01;
+	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
+	reg |= L310_PREFETCH_CTRL_PREFETCH_DROP;
+	reg |= L310_PREFETCH_CTRL_DATA_PREFETCH;
+	reg |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
+	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL;
+	writel_relaxed(reg, at91_l2cc_base + L310_PREFETCH_CTRL);
+
+	/* Power Control */
+	reg = readl_relaxed(at91_l2cc_base + L310_POWER_CTRL);
+	reg |= L310_STNDBY_MODE_EN;
+	reg |= L310_DYNAMIC_CLK_GATING_EN;
+	writel_relaxed(reg, at91_l2cc_base + L310_POWER_CTRL);
+
+	/* Disable interrupts */
+	writel_relaxed(0x00, at91_l2cc_base + L2X0_INTR_MASK);
+	writel_relaxed(0x01ff, at91_l2cc_base + L2X0_INTR_CLEAR);
+	l2x0_of_init(0, ~0UL);
+}
+#else
+static inline void at91_init_l2cache(void) {}
+#endif
+
 static void __init sama5_dt_device_init(void)
 {
+	at91_init_l2cache();
+
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 	at91_sam9x5_pm_init();
 }
-- 
1.7.9.5

[PATCH 4/7] ARM: at91: enable the L2 Cache controller

[Wenyou Yang]

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 arch/arm/mach-at91/board-dt-sama5.c |   53 +++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
index 86cffcd..ed6db28 100644
--- a/arch/arm/mach-at91/board-dt-sama5.c
+++ b/arch/arm/mach-at91/board-dt-sama5.c
@@ -17,17 +17,70 @@
 #include <linux/of_platform.h>
 #include <linux/phy.h>
 #include <linux/clk-provider.h>
+#include <linux/of_address.h>
 
 #include <asm/setup.h>
 #include <asm/irq.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
 #include <asm/mach/irq.h>
+#include <asm/hardware/cache-l2x0.h>
 
 #include "generic.h"
 
+void __iomem *at91_l2cc_base;
+EXPORT_SYMBOL_GPL(at91_l2cc_base);
+
+#ifdef CONFIG_CACHE_L2X0
+static void __init at91_init_l2cache(void)
+{
+	struct device_node *np;
+	u32 reg;
+
+	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
+	if (!np)
+		return;
+
+	at91_l2cc_base = of_iomap(np, 0);
+	if (!at91_l2cc_base)
+		panic("unable to map l2cc cpu registers\n");
+
+	of_node_put(np);
+
+	/* Disable cache if it hasn't been done yet */
+	if (readl_relaxed(at91_l2cc_base + L2X0_CTRL) & L2X0_CTRL_EN)
+		writel_relaxed(~L2X0_CTRL_EN, at91_l2cc_base + L2X0_CTRL);
+
+	/* Prefetch Control */
+	reg = readl_relaxed(at91_l2cc_base + L310_PREFETCH_CTRL);
+	reg &= ~L310_PREFETCH_CTRL_OFFSET_MASK;
+	reg |= 0x01;
+	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
+	reg |= L310_PREFETCH_CTRL_PREFETCH_DROP;
+	reg |= L310_PREFETCH_CTRL_DATA_PREFETCH;
+	reg |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
+	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL;
+	writel_relaxed(reg, at91_l2cc_base + L310_PREFETCH_CTRL);
+
+	/* Power Control */
+	reg = readl_relaxed(at91_l2cc_base + L310_POWER_CTRL);
+	reg |= L310_STNDBY_MODE_EN;
+	reg |= L310_DYNAMIC_CLK_GATING_EN;
+	writel_relaxed(reg, at91_l2cc_base + L310_POWER_CTRL);
+
+	/* Disable interrupts */
+	writel_relaxed(0x00, at91_l2cc_base + L2X0_INTR_MASK);
+	writel_relaxed(0x01ff, at91_l2cc_base + L2X0_INTR_CLEAR);
+	l2x0_of_init(0, ~0UL);
+}
+#else
+static inline void at91_init_l2cache(void) {}
+#endif
+
 static void __init sama5_dt_device_init(void)
 {
+	at91_init_l2cache();
+
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 	at91_sam9x5_pm_init();
 }
-- 
1.7.9.5

[PATCH 5/7] pm: at91: add disable/enable the L1/L2 cache while suspend/resume

[Wenyou Yang]

For the sama5, disable L1 D-cache and L2 cache before the cpu go to wfi,
after wakeing up, enable L1 D-cache and L2 cache.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 arch/arm/mach-at91/pm.c         |    9 ++++
 arch/arm/mach-at91/pm_suspend.S |  107 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+)

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index f75dc32..50cde92 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -35,6 +35,9 @@
 #include "generic.h"
 #include "pm.h"
 
+void __weak at91_disable_l1_l2_cache(void) {}
+void __weak at91_enable_l1_l2_cache(void) {}
+
 struct at91_pm_struct at91_pm_data;
 
 static int at91_pm_valid_state(suspend_state_t state)
@@ -142,8 +145,14 @@ static void at91_pm_suspend(suspend_state_t state)
 	pm_data |= (state == PM_SUSPEND_MEM) ?
 				AT91_PM_MODE(AT91_PM_SLOW_CLOCK) : 0;
 
+	/* Disable L1 D-cache and L2 cache */
+	at91_disable_l1_l2_cache();
+
 	at91_suspend_sram_fn(at91_pmc_base, at91_ramc_base[0],
 				at91_ramc_base[1], pm_data);
+
+	/* Enable L1 D-cache and L2 cache */
+	at91_enable_l1_l2_cache();
 }
 
 static int at91_pm_enter(suspend_state_t state)
diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
index 88cf228..e6e7f7a 100644
--- a/arch/arm/mach-at91/pm_suspend.S
+++ b/arch/arm/mach-at91/pm_suspend.S
@@ -15,6 +15,7 @@
 #include <linux/clk/at91_pmc.h>
 #include <mach/hardware.h>
 #include <mach/at91_ramc.h>
+#include <asm/hardware/cache-l2x0.h>
 
 #include "pm.h"
 
@@ -329,3 +330,109 @@ ram_restored:
 
 ENTRY(at91_pm_suspend_in_sram_sz)
 	.word .-at91_pm_suspend_in_sram
+
+/*---------------------------------------*/
+
+#if defined(CONFIG_CPU_V7)
+
+/*
+ * void at91_disable_l1_l2_cache(void)
+ *
+ * This function code disables, cleans & invalidates the L1 D-cache
+ * and cleans, invalidates & disable the L2 cache.
+ */
+ENTRY(at91_disable_l1_l2_cache)
+	stmfd	sp!, {r4 - r12, lr}
+
+	/*
+	 * Flush all data from the L1 D-cache before disabling
+	 * SCTLR.C bit.
+	 */
+	bl	v7_flush_dcache_all
+
+	/*
+	 * Clear the SCTLR.C bit to prevent further data cache
+	 * allocation. Clearing SCTLR.C would make all the data accesses
+	 * strongly ordered and would not hit the cache.
+	 */
+	mrc	p15, 0, r0, c1, c0, 0
+	bic	r0, r0, #(1 << 2)	@ Disable the C bit
+	mcr	p15, 0, r0, c1, c0, 0
+	isb
+
+	/*
+	 * Invalidate L1 D-cache. Even though only invalidate is
+	 * necessary exported flush API is used here. Doing clean
+	 * on already clean cache would be almost NOP.
+	 */
+	bl	v7_flush_dcache_all
+
+	/*
+	 * Clean and invalidate the L2 cache.
+	 * Common cache-l2x0.c functions can't be used here since it
+	 * uses spinlocks. We are out of coherency here with data cache
+	 * disabled. The spinlock implementation uses exclusive load/store
+	 * instruction which can fail without data cache being enabled.
+	 * Because of this, CPU can lead to deadlock.
+	 */
+	ldr	r1, at91_l2cc_base_addr
+	ldr	r2, [r1]
+	cmp	r2, #0
+	beq	skip_l2disable
+	mov	r0, #0xff
+	str	r0, [r2, #L2X0_CLEAN_INV_WAY]
+wait:
+	ldr	r0, [r2, #L2X0_CLEAN_INV_WAY]
+	mov	r1, #0xff
+	ands	r0, r0, r1
+	bne	wait
+
+	mov	r0, #0
+	str	r0, [r2, #L2X0_CTRL]
+
+l2x_sync:
+	ldr	r0, [r2, #L2X0_CACHE_SYNC]
+	bic	r0, r0, #0x1
+	str	r0, [r2, #L2X0_CACHE_SYNC]
+sync:
+	ldr	r0, [r2, #L2X0_CACHE_SYNC]
+	ands	r0, r0, #0x1
+	bne	sync
+
+skip_l2disable:
+	ldmfd	sp!, {r4 - r12, pc}
+ENDPROC(at91_disable_l1_l2_cache)
+
+/*
+ * void at91_enable_l1_l2_cache(void)
+ *
+ * This function code enables the L1 D-cache and the L2 cache.
+ */
+ENTRY(at91_enable_l1_l2_cache)
+	stmfd	sp!, {r4 - r12, lr}
+
+	/* Enable the L2 cache */
+	ldr	r1, at91_l2cc_base_addr
+	ldr	r2, [r1]
+	cmp	r2, #0
+	beq	skip_l2en
+	ldr	r0, [r2, #L2X0_CTRL]
+	ands	r0, r0, #L2X0_CTRL_EN
+	bne	skip_l2en	@ Skip if already enabled
+	mov	r0, #L2X0_CTRL_EN
+	str	r0, [r2, #L2X0_CTRL]
+skip_l2en:
+
+	/* Enable the L1 D-cache */
+	mrc	p15, 0, r0, c1, c0, 0
+	tst	r0, #(1 << 2)		@ Check C bit enabled?
+	orreq	r0, r0, #(1 << 2)	@ Enable the C bit
+	mcreq	p15, 0, r0, c1, c0, 0
+	isb
+
+	ldmfd	sp!, {r4 - r12, pc}
+ENDPROC(at91_enable_l1_l2_cache)
+
+at91_l2cc_base_addr:
+	.word	at91_l2cc_base
+#endif
-- 
1.7.9.5

[PATCH 5/7] pm: at91: add disable/enable the L1/L2 cache while suspend/resume

[Wenyou Yang]

For the sama5, disable L1 D-cache and L2 cache before the cpu go to wfi,
after wakeing up, enable L1 D-cache and L2 cache.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 arch/arm/mach-at91/pm.c         |    9 ++++
 arch/arm/mach-at91/pm_suspend.S |  107 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+)

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index f75dc32..50cde92 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -35,6 +35,9 @@
 #include "generic.h"
 #include "pm.h"
 
+void __weak at91_disable_l1_l2_cache(void) {}
+void __weak at91_enable_l1_l2_cache(void) {}
+
 struct at91_pm_struct at91_pm_data;
 
 static int at91_pm_valid_state(suspend_state_t state)
@@ -142,8 +145,14 @@ static void at91_pm_suspend(suspend_state_t state)
 	pm_data |= (state == PM_SUSPEND_MEM) ?
 				AT91_PM_MODE(AT91_PM_SLOW_CLOCK) : 0;
 
+	/* Disable L1 D-cache and L2 cache */
+	at91_disable_l1_l2_cache();
+
 	at91_suspend_sram_fn(at91_pmc_base, at91_ramc_base[0],
 				at91_ramc_base[1], pm_data);
+
+	/* Enable L1 D-cache and L2 cache */
+	at91_enable_l1_l2_cache();
 }
 
 static int at91_pm_enter(suspend_state_t state)
diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
index 88cf228..e6e7f7a 100644
--- a/arch/arm/mach-at91/pm_suspend.S
+++ b/arch/arm/mach-at91/pm_suspend.S
@@ -15,6 +15,7 @@
 #include <linux/clk/at91_pmc.h>
 #include <mach/hardware.h>
 #include <mach/at91_ramc.h>
+#include <asm/hardware/cache-l2x0.h>
 
 #include "pm.h"
 
@@ -329,3 +330,109 @@ ram_restored:
 
 ENTRY(at91_pm_suspend_in_sram_sz)
 	.word .-at91_pm_suspend_in_sram
+
+/*---------------------------------------*/
+
+#if defined(CONFIG_CPU_V7)
+
+/*
+ * void at91_disable_l1_l2_cache(void)
+ *
+ * This function code disables, cleans & invalidates the L1 D-cache
+ * and cleans, invalidates & disable the L2 cache.
+ */
+ENTRY(at91_disable_l1_l2_cache)
+	stmfd	sp!, {r4 - r12, lr}
+
+	/*
+	 * Flush all data from the L1 D-cache before disabling
+	 * SCTLR.C bit.
+	 */
+	bl	v7_flush_dcache_all
+
+	/*
+	 * Clear the SCTLR.C bit to prevent further data cache
+	 * allocation. Clearing SCTLR.C would make all the data accesses
+	 * strongly ordered and would not hit the cache.
+	 */
+	mrc	p15, 0, r0, c1, c0, 0
+	bic	r0, r0, #(1 << 2)	@ Disable the C bit
+	mcr	p15, 0, r0, c1, c0, 0
+	isb
+
+	/*
+	 * Invalidate L1 D-cache. Even though only invalidate is
+	 * necessary exported flush API is used here. Doing clean
+	 * on already clean cache would be almost NOP.
+	 */
+	bl	v7_flush_dcache_all
+
+	/*
+	 * Clean and invalidate the L2 cache.
+	 * Common cache-l2x0.c functions can't be used here since it
+	 * uses spinlocks. We are out of coherency here with data cache
+	 * disabled. The spinlock implementation uses exclusive load/store
+	 * instruction which can fail without data cache being enabled.
+	 * Because of this, CPU can lead to deadlock.
+	 */
+	ldr	r1, at91_l2cc_base_addr
+	ldr	r2, [r1]
+	cmp	r2, #0
+	beq	skip_l2disable
+	mov	r0, #0xff
+	str	r0, [r2, #L2X0_CLEAN_INV_WAY]
+wait:
+	ldr	r0, [r2, #L2X0_CLEAN_INV_WAY]
+	mov	r1, #0xff
+	ands	r0, r0, r1
+	bne	wait
+
+	mov	r0, #0
+	str	r0, [r2, #L2X0_CTRL]
+
+l2x_sync:
+	ldr	r0, [r2, #L2X0_CACHE_SYNC]
+	bic	r0, r0, #0x1
+	str	r0, [r2, #L2X0_CACHE_SYNC]
+sync:
+	ldr	r0, [r2, #L2X0_CACHE_SYNC]
+	ands	r0, r0, #0x1
+	bne	sync
+
+skip_l2disable:
+	ldmfd	sp!, {r4 - r12, pc}
+ENDPROC(at91_disable_l1_l2_cache)
+
+/*
+ * void at91_enable_l1_l2_cache(void)
+ *
+ * This function code enables the L1 D-cache and the L2 cache.
+ */
+ENTRY(at91_enable_l1_l2_cache)
+	stmfd	sp!, {r4 - r12, lr}
+
+	/* Enable the L2 cache */
+	ldr	r1, at91_l2cc_base_addr
+	ldr	r2, [r1]
+	cmp	r2, #0
+	beq	skip_l2en
+	ldr	r0, [r2, #L2X0_CTRL]
+	ands	r0, r0, #L2X0_CTRL_EN
+	bne	skip_l2en	@ Skip if already enabled
+	mov	r0, #L2X0_CTRL_EN
+	str	r0, [r2, #L2X0_CTRL]
+skip_l2en:
+
+	/* Enable the L1 D-cache */
+	mrc	p15, 0, r0, c1, c0, 0
+	tst	r0, #(1 << 2)		@ Check C bit enabled?
+	orreq	r0, r0, #(1 << 2)	@ Enable the C bit
+	mcreq	p15, 0, r0, c1, c0, 0
+	isb
+
+	ldmfd	sp!, {r4 - r12, pc}
+ENDPROC(at91_enable_l1_l2_cache)
+
+at91_l2cc_base_addr:
+	.word	at91_l2cc_base
+#endif
-- 
1.7.9.5

[PATCH 6/7] pm: at91: add achieve the mpddrc peripheral ID and the DDR clock ID support

[Wenyou Yang]

The patch achieves the mpddr controller peripheral ID
and the DDR clock ID from the dts file.

They will be used in the future to disable the mpddr controller'c clock
the and DDR clock to decrease the power consumption during suspending.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 arch/arm/mach-at91/generic.h |    2 ++
 arch/arm/mach-at91/setup.c   |   24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/arch/arm/mach-at91/generic.h b/arch/arm/mach-at91/generic.h
index 41796bf..3c72a3e 100644
--- a/arch/arm/mach-at91/generic.h
+++ b/arch/arm/mach-at91/generic.h
@@ -47,6 +47,8 @@ void __init at91_sam9x5_pm_init(void) { }
 struct at91_pm_struct {
 	unsigned long uhp_udp_mask;
 	int memctrl;
+	u32 mpddrc_id[2];
+	u32 ddrck_id;
 };
 
 #endif /* _AT91_GENERIC_H */
diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c
index 7924663..a306f95 100644
--- a/arch/arm/mach-at91/setup.c
+++ b/arch/arm/mach-at91/setup.c
@@ -363,6 +363,27 @@ void __init at91_ioremap_matrix(u32 base_addr)
 		panic(pr_fmt("Impossible to ioremap at91_matrix_base\n"));
 }
 
+static u32 at91_of_get_ddr_id(struct device_node *np, char *name)
+{
+	struct of_phandle_args clkspec;
+	u32 id;
+	int index;
+	int rc;
+
+	index = of_property_match_string(np, "clock-names", name);
+	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index, &clkspec);
+	if (rc)
+		return 0;
+
+	rc = of_property_read_u32(clkspec.np, "reg", &id);
+	if (rc)
+		return 0;
+
+	of_node_put(clkspec.np);
+
+	return id;
+}
+
 struct at91_ramc_of_data {
 	u8 ramc_type;
 };
@@ -400,6 +421,9 @@ static void at91_dt_ramc(void)
 		of_data = of_id->data;
 		at91_pm_data.memctrl = of_data->ramc_type;
 
+		at91_pm_data.mpddrc_id[idx] = at91_of_get_ddr_id(np, "mpddr");
+		at91_pm_data.ddrck_id = at91_of_get_ddr_id(np, "ddrck");
+
 		idx++;
 	}
 
-- 
1.7.9.5

[PATCH 6/7] pm: at91: add achieve the mpddrc peripheral ID and the DDR clock ID support

[Wenyou Yang]

The patch achieves the mpddr controller peripheral ID
and the DDR clock ID from the dts file.

They will be used in the future to disable the mpddr controller'c clock
the and DDR clock to decrease the power consumption during suspending.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 arch/arm/mach-at91/generic.h |    2 ++
 arch/arm/mach-at91/setup.c   |   24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/arch/arm/mach-at91/generic.h b/arch/arm/mach-at91/generic.h
index 41796bf..3c72a3e 100644
--- a/arch/arm/mach-at91/generic.h
+++ b/arch/arm/mach-at91/generic.h
@@ -47,6 +47,8 @@ void __init at91_sam9x5_pm_init(void) { }
 struct at91_pm_struct {
 	unsigned long uhp_udp_mask;
 	int memctrl;
+	u32 mpddrc_id[2];
+	u32 ddrck_id;
 };
 
 #endif /* _AT91_GENERIC_H */
diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c
index 7924663..a306f95 100644
--- a/arch/arm/mach-at91/setup.c
+++ b/arch/arm/mach-at91/setup.c
@@ -363,6 +363,27 @@ void __init at91_ioremap_matrix(u32 base_addr)
 		panic(pr_fmt("Impossible to ioremap at91_matrix_base\n"));
 }
 
+static u32 at91_of_get_ddr_id(struct device_node *np, char *name)
+{
+	struct of_phandle_args clkspec;
+	u32 id;
+	int index;
+	int rc;
+
+	index = of_property_match_string(np, "clock-names", name);
+	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index, &clkspec);
+	if (rc)
+		return 0;
+
+	rc = of_property_read_u32(clkspec.np, "reg", &id);
+	if (rc)
+		return 0;
+
+	of_node_put(clkspec.np);
+
+	return id;
+}
+
 struct at91_ramc_of_data {
 	u8 ramc_type;
 };
@@ -400,6 +421,9 @@ static void at91_dt_ramc(void)
 		of_data = of_id->data;
 		at91_pm_data.memctrl = of_data->ramc_type;
 
+		at91_pm_data.mpddrc_id[idx] = at91_of_get_ddr_id(np, "mpddr");
+		at91_pm_data.ddrck_id = at91_of_get_ddr_id(np, "ddrck");
+
 		idx++;
 	}
 
-- 
1.7.9.5

[PATCH 7/7] pm: at91: add disable/enable the mpddrc's clock and DDR clock support

[Wenyou Yang]

In order to decrease the power consumption, when go to suspend,
disable the mpddr controller peripheral clock and the DDR clock
after the DDR enters the self-refresh mode.

Due the mpddr controller's issue, postpone the disable clocks operation,
instead of the DDR enters self-fresh mode immediately.

Enable the clocks after resume and before the DDR exits the self-refresh mode.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 arch/arm/mach-at91/pm.c         |    4 +++
 arch/arm/mach-at91/pm.h         |    8 +++++
 arch/arm/mach-at91/pm_suspend.S |   64 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+)

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 50cde92..6bd2a6b 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -145,6 +145,10 @@ static void at91_pm_suspend(suspend_state_t state)
 	pm_data |= (state == PM_SUSPEND_MEM) ?
 				AT91_PM_MODE(AT91_PM_SLOW_CLOCK) : 0;
 
+	pm_data |= AT91_PM_DDRCK(at91_pm_data.ddrck_id);
+
+	pm_data |= AT91_PM_DDRC_ID(at91_pm_data.mpddrc_id[0]);
+
 	/* Disable L1 D-cache and L2 cache */
 	at91_disable_l1_l2_cache();
 
diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
index 158575e..fd4c5fc 100644
--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -19,6 +19,14 @@
 #define	AT91_PM_MODE_MASK	0x0f
 #define	AT91_PM_MODE(x)		(((x) & AT91_PM_MODE_MASK) << AT91_PM_MODE_OFFSET)
 
+#define	AT91_PM_DDRCK_OFFSET	8
+#define	AT91_PM_DDRCK_MASK	0xff
+#define	AT91_PM_DDRCK(x)	(((x) & AT91_PM_DDRCK_MASK) << AT91_PM_DDRCK_OFFSET)
+
+#define	AT91_PM_DDRC_ID_OFFSET	16
+#define	AT91_PM_DDRC_ID_MASK	0xff
+#define	AT91_PM_DDRC_ID(x)	(((x) & AT91_PM_DDRC_ID_MASK) << AT91_PM_DDRC_ID_OFFSET)
+
 #define	AT91_PM_SLOW_CLOCK	0x01
 
 #endif
diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
index e6e7f7a..8fb276c 100644
--- a/arch/arm/mach-at91/pm_suspend.S
+++ b/arch/arm/mach-at91/pm_suspend.S
@@ -26,6 +26,8 @@ memctrl	.req	r3
 tmp1	.req	r4
 tmp2	.req	r5
 mode	.req	r6
+ddrc_id	.req	r7
+ddrck	.req	r8
 
 /*
  * Wait until master clock is ready (after switching master clock source)
@@ -130,6 +132,16 @@ ENTRY(at91_pm_suspend_in_sram)
 	and	mode, tmp2, #AT91_PM_MODE_MASK
 
 	mov	tmp1, memctrl
+	mov	tmp2, tmp1, lsr#AT91_PM_DDRCK_OFFSET
+	mov	tmp1, #0x01
+	and	tmp2, tmp2, #AT91_PM_DDRCK_MASK
+	mov	ddrck, tmp1, lsl tmp2
+
+	mov	tmp1, memctrl
+	mov	tmp2, tmp1, lsr#AT91_PM_DDRC_ID_OFFSET
+	and	ddrc_id, tmp2, #AT91_PM_DDRC_ID_MASK
+
+	mov	tmp1, memctrl
 	and	memctrl, tmp1, #AT91_PM_MEMCTRL_MASK
 
 	cmp	memctrl, #AT91_MEMCTRL_MC
@@ -234,6 +246,32 @@ sdr_sr_done:
 	str	tmp1, [pmc, #AT91_CKGR_MOR]
 
 skip_disable_main_clock:
+
+	/*
+	 * Disable the SDRAM controller clock
+	 */
+	cmp	ddrc_id, #0
+	beq	skip_disable_ddrc_clock
+	bic	tmp2, ddrc_id, #0xe0 /* fetch least 5 bits */
+	mov	tmp1, #0x01
+	mov	tmp1, tmp1, lsl tmp2
+
+	tst	ddrc_id, #0xe0	/* > 32 ? */
+	beq	set_pmc_pcdr
+	str	tmp1, [pmc, #AT91_PMC_PCDR1]
+	b	skip_disable_ddrc_clock
+set_pmc_pcdr:
+	str	tmp1, [pmc, #AT91_PMC_PCDR]
+skip_disable_ddrc_clock:
+
+	/*
+	 * Disable SDRAM system clock
+	 */
+	cmp	ddrck, #0
+	beq	skip_disable_ddrck
+	str	ddrck, [pmc, #AT91_PMC_SCDR]
+skip_disable_ddrck:
+
 	/* Wait for interrupt */
 	_do_wfi
 
@@ -269,6 +307,32 @@ skip_disable_main_clock:
 	wait_mckrdy
 
 skip_enable_main_clock:
+
+	/*
+	 * Enable the SDRAM controller clock
+	 */
+	cmp	ddrc_id, #0
+	beq	skip_enable_ddrc_clock
+	bic	tmp2, ddrc_id, #0xe0 /* fetch lowest 5 bits */
+	mov	tmp1, #0x01
+	mov	tmp1, tmp1, lsl tmp2
+
+	tst	ddrc_id, #0xe0	/* > 32 ? */
+	beq	set_pmc_pcer
+	str	tmp1, [pmc, #AT91_PMC_PCER1]
+	b	skip_enable_ddrc_clock
+set_pmc_pcer:
+	str	tmp1, [pmc, #AT91_PMC_PCER]
+skip_enable_ddrc_clock:
+
+	/*
+	 * Enable SDRAM system clock
+	 */
+	cmp	ddrck, #0
+	beq	skip_enable_ddrck
+	str	ddrck, [pmc, #AT91_PMC_SCER]
+skip_enable_ddrck:
+
 	/*
 	 * at91rm9200 Memory controller
 	 * Do nothing - self-refresh is automatically disabled.
-- 
1.7.9.5

[PATCH 7/7] pm: at91: add disable/enable the mpddrc's clock and DDR clock support

[Wenyou Yang]

In order to decrease the power consumption, when go to suspend,
disable the mpddr controller peripheral clock and the DDR clock
after the DDR enters the self-refresh mode.

Due the mpddr controller's issue, postpone the disable clocks operation,
instead of the DDR enters self-fresh mode immediately.

Enable the clocks after resume and before the DDR exits the self-refresh mode.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 arch/arm/mach-at91/pm.c         |    4 +++
 arch/arm/mach-at91/pm.h         |    8 +++++
 arch/arm/mach-at91/pm_suspend.S |   64 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+)

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 50cde92..6bd2a6b 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -145,6 +145,10 @@ static void at91_pm_suspend(suspend_state_t state)
 	pm_data |= (state == PM_SUSPEND_MEM) ?
 				AT91_PM_MODE(AT91_PM_SLOW_CLOCK) : 0;
 
+	pm_data |= AT91_PM_DDRCK(at91_pm_data.ddrck_id);
+
+	pm_data |= AT91_PM_DDRC_ID(at91_pm_data.mpddrc_id[0]);
+
 	/* Disable L1 D-cache and L2 cache */
 	at91_disable_l1_l2_cache();
 
diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
index 158575e..fd4c5fc 100644
--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -19,6 +19,14 @@
 #define	AT91_PM_MODE_MASK	0x0f
 #define	AT91_PM_MODE(x)		(((x) & AT91_PM_MODE_MASK) << AT91_PM_MODE_OFFSET)
 
+#define	AT91_PM_DDRCK_OFFSET	8
+#define	AT91_PM_DDRCK_MASK	0xff
+#define	AT91_PM_DDRCK(x)	(((x) & AT91_PM_DDRCK_MASK) << AT91_PM_DDRCK_OFFSET)
+
+#define	AT91_PM_DDRC_ID_OFFSET	16
+#define	AT91_PM_DDRC_ID_MASK	0xff
+#define	AT91_PM_DDRC_ID(x)	(((x) & AT91_PM_DDRC_ID_MASK) << AT91_PM_DDRC_ID_OFFSET)
+
 #define	AT91_PM_SLOW_CLOCK	0x01
 
 #endif
diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
index e6e7f7a..8fb276c 100644
--- a/arch/arm/mach-at91/pm_suspend.S
+++ b/arch/arm/mach-at91/pm_suspend.S
@@ -26,6 +26,8 @@ memctrl	.req	r3
 tmp1	.req	r4
 tmp2	.req	r5
 mode	.req	r6
+ddrc_id	.req	r7
+ddrck	.req	r8
 
 /*
  * Wait until master clock is ready (after switching master clock source)
@@ -130,6 +132,16 @@ ENTRY(at91_pm_suspend_in_sram)
 	and	mode, tmp2, #AT91_PM_MODE_MASK
 
 	mov	tmp1, memctrl
+	mov	tmp2, tmp1, lsr#AT91_PM_DDRCK_OFFSET
+	mov	tmp1, #0x01
+	and	tmp2, tmp2, #AT91_PM_DDRCK_MASK
+	mov	ddrck, tmp1, lsl tmp2
+
+	mov	tmp1, memctrl
+	mov	tmp2, tmp1, lsr#AT91_PM_DDRC_ID_OFFSET
+	and	ddrc_id, tmp2, #AT91_PM_DDRC_ID_MASK
+
+	mov	tmp1, memctrl
 	and	memctrl, tmp1, #AT91_PM_MEMCTRL_MASK
 
 	cmp	memctrl, #AT91_MEMCTRL_MC
@@ -234,6 +246,32 @@ sdr_sr_done:
 	str	tmp1, [pmc, #AT91_CKGR_MOR]
 
 skip_disable_main_clock:
+
+	/*
+	 * Disable the SDRAM controller clock
+	 */
+	cmp	ddrc_id, #0
+	beq	skip_disable_ddrc_clock
+	bic	tmp2, ddrc_id, #0xe0 /* fetch least 5 bits */
+	mov	tmp1, #0x01
+	mov	tmp1, tmp1, lsl tmp2
+
+	tst	ddrc_id, #0xe0	/* > 32 ? */
+	beq	set_pmc_pcdr
+	str	tmp1, [pmc, #AT91_PMC_PCDR1]
+	b	skip_disable_ddrc_clock
+set_pmc_pcdr:
+	str	tmp1, [pmc, #AT91_PMC_PCDR]
+skip_disable_ddrc_clock:
+
+	/*
+	 * Disable SDRAM system clock
+	 */
+	cmp	ddrck, #0
+	beq	skip_disable_ddrck
+	str	ddrck, [pmc, #AT91_PMC_SCDR]
+skip_disable_ddrck:
+
 	/* Wait for interrupt */
 	_do_wfi
 
@@ -269,6 +307,32 @@ skip_disable_main_clock:
 	wait_mckrdy
 
 skip_enable_main_clock:
+
+	/*
+	 * Enable the SDRAM controller clock
+	 */
+	cmp	ddrc_id, #0
+	beq	skip_enable_ddrc_clock
+	bic	tmp2, ddrc_id, #0xe0 /* fetch lowest 5 bits */
+	mov	tmp1, #0x01
+	mov	tmp1, tmp1, lsl tmp2
+
+	tst	ddrc_id, #0xe0	/* > 32 ? */
+	beq	set_pmc_pcer
+	str	tmp1, [pmc, #AT91_PMC_PCER1]
+	b	skip_enable_ddrc_clock
+set_pmc_pcer:
+	str	tmp1, [pmc, #AT91_PMC_PCER]
+skip_enable_ddrc_clock:
+
+	/*
+	 * Enable SDRAM system clock
+	 */
+	cmp	ddrck, #0
+	beq	skip_enable_ddrck
+	str	ddrck, [pmc, #AT91_PMC_SCER]
+skip_enable_ddrck:
+
 	/*
 	 * at91rm9200 Memory controller
 	 * Do nothing - self-refresh is automatically disabled.
-- 
1.7.9.5

Re: [PATCH 4/7] ARM: at91: enable the L2 Cache controller

[Mark Rutland]

On Mon, Jan 26, 2015 at 10:07:16AM +0000, Wenyou Yang wrote:
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>  arch/arm/mach-at91/board-dt-sama5.c |   53 +++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
> index 86cffcd..ed6db28 100644
> --- a/arch/arm/mach-at91/board-dt-sama5.c
> +++ b/arch/arm/mach-at91/board-dt-sama5.c
> @@ -17,17 +17,70 @@
>  #include <linux/of_platform.h>
>  #include <linux/phy.h>
>  #include <linux/clk-provider.h>
> +#include <linux/of_address.h>
>  
>  #include <asm/setup.h>
>  #include <asm/irq.h>
>  #include <asm/mach/arch.h>
>  #include <asm/mach/map.h>
>  #include <asm/mach/irq.h>
> +#include <asm/hardware/cache-l2x0.h>
>  
>  #include "generic.h"
>  
> +void __iomem *at91_l2cc_base;
> +EXPORT_SYMBOL_GPL(at91_l2cc_base);
> +
> +#ifdef CONFIG_CACHE_L2X0
> +static void __init at91_init_l2cache(void)
> +{
> +	struct device_node *np;
> +	u32 reg;
> +
> +	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> +	if (!np)
> +		return;
> +
> +	at91_l2cc_base = of_iomap(np, 0);
> +	if (!at91_l2cc_base)
> +		panic("unable to map l2cc cpu registers\n");
> +
> +	of_node_put(np);
> +
> +	/* Disable cache if it hasn't been done yet */
> +	if (readl_relaxed(at91_l2cc_base + L2X0_CTRL) & L2X0_CTRL_EN)
> +		writel_relaxed(~L2X0_CTRL_EN, at91_l2cc_base + L2X0_CTRL);
> +
> +	/* Prefetch Control */
> +	reg = readl_relaxed(at91_l2cc_base + L310_PREFETCH_CTRL);
> +	reg &= ~L310_PREFETCH_CTRL_OFFSET_MASK;
> +	reg |= 0x01;
> +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
> +	reg |= L310_PREFETCH_CTRL_PREFETCH_DROP;
> +	reg |= L310_PREFETCH_CTRL_DATA_PREFETCH;
> +	reg |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
> +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL;
> +	writel_relaxed(reg, at91_l2cc_base + L310_PREFETCH_CTRL);
> +
> +	/* Power Control */
> +	reg = readl_relaxed(at91_l2cc_base + L310_POWER_CTRL);
> +	reg |= L310_STNDBY_MODE_EN;
> +	reg |= L310_DYNAMIC_CLK_GATING_EN;
> +	writel_relaxed(reg, at91_l2cc_base + L310_POWER_CTRL);
> +
> +	/* Disable interrupts */
> +	writel_relaxed(0x00, at91_l2cc_base + L2X0_INTR_MASK);
> +	writel_relaxed(0x01ff, at91_l2cc_base + L2X0_INTR_CLEAR);
> +	l2x0_of_init(0, ~0UL);
> +}
> +#else
> +static inline void at91_init_l2cache(void) {}
> +#endif

This poking of the PL310 should live in the cache_l2x0 driver.

Mark.

[PATCH 4/7] ARM: at91: enable the L2 Cache controller

[Mark Rutland]

On Mon, Jan 26, 2015 at 10:07:16AM +0000, Wenyou Yang wrote:
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>  arch/arm/mach-at91/board-dt-sama5.c |   53 +++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
> index 86cffcd..ed6db28 100644
> --- a/arch/arm/mach-at91/board-dt-sama5.c
> +++ b/arch/arm/mach-at91/board-dt-sama5.c
> @@ -17,17 +17,70 @@
>  #include <linux/of_platform.h>
>  #include <linux/phy.h>
>  #include <linux/clk-provider.h>
> +#include <linux/of_address.h>
>  
>  #include <asm/setup.h>
>  #include <asm/irq.h>
>  #include <asm/mach/arch.h>
>  #include <asm/mach/map.h>
>  #include <asm/mach/irq.h>
> +#include <asm/hardware/cache-l2x0.h>
>  
>  #include "generic.h"
>  
> +void __iomem *at91_l2cc_base;
> +EXPORT_SYMBOL_GPL(at91_l2cc_base);
> +
> +#ifdef CONFIG_CACHE_L2X0
> +static void __init at91_init_l2cache(void)
> +{
> +	struct device_node *np;
> +	u32 reg;
> +
> +	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> +	if (!np)
> +		return;
> +
> +	at91_l2cc_base = of_iomap(np, 0);
> +	if (!at91_l2cc_base)
> +		panic("unable to map l2cc cpu registers\n");
> +
> +	of_node_put(np);
> +
> +	/* Disable cache if it hasn't been done yet */
> +	if (readl_relaxed(at91_l2cc_base + L2X0_CTRL) & L2X0_CTRL_EN)
> +		writel_relaxed(~L2X0_CTRL_EN, at91_l2cc_base + L2X0_CTRL);
> +
> +	/* Prefetch Control */
> +	reg = readl_relaxed(at91_l2cc_base + L310_PREFETCH_CTRL);
> +	reg &= ~L310_PREFETCH_CTRL_OFFSET_MASK;
> +	reg |= 0x01;
> +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
> +	reg |= L310_PREFETCH_CTRL_PREFETCH_DROP;
> +	reg |= L310_PREFETCH_CTRL_DATA_PREFETCH;
> +	reg |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
> +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL;
> +	writel_relaxed(reg, at91_l2cc_base + L310_PREFETCH_CTRL);
> +
> +	/* Power Control */
> +	reg = readl_relaxed(at91_l2cc_base + L310_POWER_CTRL);
> +	reg |= L310_STNDBY_MODE_EN;
> +	reg |= L310_DYNAMIC_CLK_GATING_EN;
> +	writel_relaxed(reg, at91_l2cc_base + L310_POWER_CTRL);
> +
> +	/* Disable interrupts */
> +	writel_relaxed(0x00, at91_l2cc_base + L2X0_INTR_MASK);
> +	writel_relaxed(0x01ff, at91_l2cc_base + L2X0_INTR_CLEAR);
> +	l2x0_of_init(0, ~0UL);
> +}
> +#else
> +static inline void at91_init_l2cache(void) {}
> +#endif

This poking of the PL310 should live in the cache_l2x0 driver.

Mark.

Re: [PATCH 6/7] pm: at91: add achieve the mpddrc peripheral ID and the DDR clock ID support

[Mark Rutland]

On Mon, Jan 26, 2015 at 10:08:16AM +0000, Wenyou Yang wrote:
> The patch achieves the mpddr controller peripheral ID
> and the DDR clock ID from the dts file.
> 
> They will be used in the future to disable the mpddr controller'c clock
> the and DDR clock to decrease the power consumption during suspending.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>  arch/arm/mach-at91/generic.h |    2 ++
>  arch/arm/mach-at91/setup.c   |   24 ++++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/arch/arm/mach-at91/generic.h b/arch/arm/mach-at91/generic.h
> index 41796bf..3c72a3e 100644
> --- a/arch/arm/mach-at91/generic.h
> +++ b/arch/arm/mach-at91/generic.h
> @@ -47,6 +47,8 @@ void __init at91_sam9x5_pm_init(void) { }
>  struct at91_pm_struct {
>  	unsigned long uhp_udp_mask;
>  	int memctrl;
> +	u32 mpddrc_id[2];
> +	u32 ddrck_id;
>  };
>  
>  #endif /* _AT91_GENERIC_H */
> diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c
> index 7924663..a306f95 100644
> --- a/arch/arm/mach-at91/setup.c
> +++ b/arch/arm/mach-at91/setup.c
> @@ -363,6 +363,27 @@ void __init at91_ioremap_matrix(u32 base_addr)
>  		panic(pr_fmt("Impossible to ioremap at91_matrix_base\n"));
>  }
>  
> +static u32 at91_of_get_ddr_id(struct device_node *np, char *name)
> +{
> +	struct of_phandle_args clkspec;
> +	u32 id;
> +	int index;
> +	int rc;
> +
> +	index = of_property_match_string(np, "clock-names", name);
> +	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index, &clkspec);
> +	if (rc)
> +		return 0;
> +
> +	rc = of_property_read_u32(clkspec.np, "reg", &id);
> +	if (rc)
> +		return 0;
> +
> +	of_node_put(clkspec.np);
> +
> +	return id;
> +}

This doesn't look right to me. This assumes the format of the clock
provider node, which invalidates the point of having the abstraction in
the first place.

> +
>  struct at91_ramc_of_data {
>  	u8 ramc_type;
>  };
> @@ -400,6 +421,9 @@ static void at91_dt_ramc(void)
>  		of_data = of_id->data;
>  		at91_pm_data.memctrl = of_data->ramc_type;
>  
> +		at91_pm_data.mpddrc_id[idx] = at91_of_get_ddr_id(np, "mpddr");
> +		at91_pm_data.ddrck_id = at91_of_get_ddr_id(np, "ddrck");
> +

Why do you need these here?

Surely the logic for poking any clocks should live in the relevant
clock controller drivers?

Mark.

[PATCH 6/7] pm: at91: add achieve the mpddrc peripheral ID and the DDR clock ID support

[Mark Rutland]

On Mon, Jan 26, 2015 at 10:08:16AM +0000, Wenyou Yang wrote:
> The patch achieves the mpddr controller peripheral ID
> and the DDR clock ID from the dts file.
> 
> They will be used in the future to disable the mpddr controller'c clock
> the and DDR clock to decrease the power consumption during suspending.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>  arch/arm/mach-at91/generic.h |    2 ++
>  arch/arm/mach-at91/setup.c   |   24 ++++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/arch/arm/mach-at91/generic.h b/arch/arm/mach-at91/generic.h
> index 41796bf..3c72a3e 100644
> --- a/arch/arm/mach-at91/generic.h
> +++ b/arch/arm/mach-at91/generic.h
> @@ -47,6 +47,8 @@ void __init at91_sam9x5_pm_init(void) { }
>  struct at91_pm_struct {
>  	unsigned long uhp_udp_mask;
>  	int memctrl;
> +	u32 mpddrc_id[2];
> +	u32 ddrck_id;
>  };
>  
>  #endif /* _AT91_GENERIC_H */
> diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c
> index 7924663..a306f95 100644
> --- a/arch/arm/mach-at91/setup.c
> +++ b/arch/arm/mach-at91/setup.c
> @@ -363,6 +363,27 @@ void __init at91_ioremap_matrix(u32 base_addr)
>  		panic(pr_fmt("Impossible to ioremap at91_matrix_base\n"));
>  }
>  
> +static u32 at91_of_get_ddr_id(struct device_node *np, char *name)
> +{
> +	struct of_phandle_args clkspec;
> +	u32 id;
> +	int index;
> +	int rc;
> +
> +	index = of_property_match_string(np, "clock-names", name);
> +	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index, &clkspec);
> +	if (rc)
> +		return 0;
> +
> +	rc = of_property_read_u32(clkspec.np, "reg", &id);
> +	if (rc)
> +		return 0;
> +
> +	of_node_put(clkspec.np);
> +
> +	return id;
> +}

This doesn't look right to me. This assumes the format of the clock
provider node, which invalidates the point of having the abstraction in
the first place.

> +
>  struct at91_ramc_of_data {
>  	u8 ramc_type;
>  };
> @@ -400,6 +421,9 @@ static void at91_dt_ramc(void)
>  		of_data = of_id->data;
>  		at91_pm_data.memctrl = of_data->ramc_type;
>  
> +		at91_pm_data.mpddrc_id[idx] = at91_of_get_ddr_id(np, "mpddr");
> +		at91_pm_data.ddrck_id = at91_of_get_ddr_id(np, "ddrck");
> +

Why do you need these here?

Surely the logic for poking any clocks should live in the relevant
clock controller drivers?

Mark.

Re: [PATCH 4/7] ARM: at91: enable the L2 Cache controller

[Russell King - ARM Linux]

On Mon, Jan 26, 2015 at 06:07:16PM +0800, Wenyou Yang wrote:
> +#ifdef CONFIG_CACHE_L2X0
> +static void __init at91_init_l2cache(void)
> +{
> +	struct device_node *np;
> +	u32 reg;
> +
> +	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> +	if (!np)
> +		return;
> +
> +	at91_l2cc_base = of_iomap(np, 0);
> +	if (!at91_l2cc_base)
> +		panic("unable to map l2cc cpu registers\n");
> +
> +	of_node_put(np);
> +
> +	/* Disable cache if it hasn't been done yet */
> +	if (readl_relaxed(at91_l2cc_base + L2X0_CTRL) & L2X0_CTRL_EN)
> +		writel_relaxed(~L2X0_CTRL_EN, at91_l2cc_base + L2X0_CTRL);
> +
> +	/* Prefetch Control */
> +	reg = readl_relaxed(at91_l2cc_base + L310_PREFETCH_CTRL);
> +	reg &= ~L310_PREFETCH_CTRL_OFFSET_MASK;
> +	reg |= 0x01;
> +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
> +	reg |= L310_PREFETCH_CTRL_PREFETCH_DROP;
> +	reg |= L310_PREFETCH_CTRL_DATA_PREFETCH;
> +	reg |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
> +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL;
> +	writel_relaxed(reg, at91_l2cc_base + L310_PREFETCH_CTRL);
> +
> +	/* Power Control */
> +	reg = readl_relaxed(at91_l2cc_base + L310_POWER_CTRL);
> +	reg |= L310_STNDBY_MODE_EN;
> +	reg |= L310_DYNAMIC_CLK_GATING_EN;
> +	writel_relaxed(reg, at91_l2cc_base + L310_POWER_CTRL);
> +
> +	/* Disable interrupts */
> +	writel_relaxed(0x00, at91_l2cc_base + L2X0_INTR_MASK);
> +	writel_relaxed(0x01ff, at91_l2cc_base + L2X0_INTR_CLEAR);

Stop hacking around the core L2x0 code.  None of the above should be
necessary, and is in fact potentially dangerous if the cache was already
previously enabled.  Disabling an already enabled cache is a potential
data corrupting event.

Any of the above configuration should be performed by your boot loader
or board firmware, and if not, then we need DT properties for it.

The last thing we need is platforms buggering around in this way, so
consider this a firm NAK against this approach.

> +	l2x0_of_init(0, ~0UL);
> +}
> +#else
> +static inline void at91_init_l2cache(void) {}
> +#endif
> +
>  static void __init sama5_dt_device_init(void)
>  {
> +	at91_init_l2cache();
> +
>  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>  	at91_sam9x5_pm_init();
>  }
> -- 
> 1.7.9.5

In fact, none of this code is necessary.  If you set l2c_aux_mask and
l2c_aux_val (preferably to ~0 and 0 respectively) then the generic ARM
code will initialise the L2 cache in the appropriate way for you.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 4/7] ARM: at91: enable the L2 Cache controller

[Russell King - ARM Linux]

On Mon, Jan 26, 2015 at 06:07:16PM +0800, Wenyou Yang wrote:
> +#ifdef CONFIG_CACHE_L2X0
> +static void __init at91_init_l2cache(void)
> +{
> +	struct device_node *np;
> +	u32 reg;
> +
> +	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> +	if (!np)
> +		return;
> +
> +	at91_l2cc_base = of_iomap(np, 0);
> +	if (!at91_l2cc_base)
> +		panic("unable to map l2cc cpu registers\n");
> +
> +	of_node_put(np);
> +
> +	/* Disable cache if it hasn't been done yet */
> +	if (readl_relaxed(at91_l2cc_base + L2X0_CTRL) & L2X0_CTRL_EN)
> +		writel_relaxed(~L2X0_CTRL_EN, at91_l2cc_base + L2X0_CTRL);
> +
> +	/* Prefetch Control */
> +	reg = readl_relaxed(at91_l2cc_base + L310_PREFETCH_CTRL);
> +	reg &= ~L310_PREFETCH_CTRL_OFFSET_MASK;
> +	reg |= 0x01;
> +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
> +	reg |= L310_PREFETCH_CTRL_PREFETCH_DROP;
> +	reg |= L310_PREFETCH_CTRL_DATA_PREFETCH;
> +	reg |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
> +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL;
> +	writel_relaxed(reg, at91_l2cc_base + L310_PREFETCH_CTRL);
> +
> +	/* Power Control */
> +	reg = readl_relaxed(at91_l2cc_base + L310_POWER_CTRL);
> +	reg |= L310_STNDBY_MODE_EN;
> +	reg |= L310_DYNAMIC_CLK_GATING_EN;
> +	writel_relaxed(reg, at91_l2cc_base + L310_POWER_CTRL);
> +
> +	/* Disable interrupts */
> +	writel_relaxed(0x00, at91_l2cc_base + L2X0_INTR_MASK);
> +	writel_relaxed(0x01ff, at91_l2cc_base + L2X0_INTR_CLEAR);

Stop hacking around the core L2x0 code.  None of the above should be
necessary, and is in fact potentially dangerous if the cache was already
previously enabled.  Disabling an already enabled cache is a potential
data corrupting event.

Any of the above configuration should be performed by your boot loader
or board firmware, and if not, then we need DT properties for it.

The last thing we need is platforms buggering around in this way, so
consider this a firm NAK against this approach.

> +	l2x0_of_init(0, ~0UL);
> +}
> +#else
> +static inline void at91_init_l2cache(void) {}
> +#endif
> +
>  static void __init sama5_dt_device_init(void)
>  {
> +	at91_init_l2cache();
> +
>  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>  	at91_sam9x5_pm_init();
>  }
> -- 
> 1.7.9.5

In fact, none of this code is necessary.  If you set l2c_aux_mask and
l2c_aux_val (preferably to ~0 and 0 respectively) then the generic ARM
code will initialise the L2 cache in the appropriate way for you.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Sergei Shtylyov]

Hello.

On 1/26/2015 1:06 PM, Wenyou Yang wrote:

> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>   arch/arm/mach-at91/pm_suspend.S |   54 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 53 insertions(+), 1 deletion(-)

> diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
> index 122a3f1..e796722 100644
> --- a/arch/arm/mach-at91/pm_suspend.S
> +++ b/arch/arm/mach-at91/pm_suspend.S
> @@ -53,6 +53,58 @@ mode	.req	r6
>   	beq	1b
>   	.endm
>
> +/*
> + * Put the processor to enter the WFI state
> + */
> +	.macro _do_wfi
> +
> +#if defined(CONFIG_CPU_V7)
> +	/*
> +	 * Execute an ISB instruction to flush the pipeline to ensure
> +	 * that all of operations have beem completed.

    Been.

> +	 */
> +	isb
> +
> +	/*
> +	 * Execute an ISB instruction to ensure that all of the

    ISB again, while you're executing DSB/DMB?

> +	 * CP15 register changes have been committed.
> +	 */
> +	dsb
> +	dmb
> +
> +	/* Disable the processor's clock */
> +	mov	tmp1, #AT91_PMC_PCK

    What's 'tmp1'? Is that a register name?

> +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> +
> +	/* Execute a WFI instruction */

    Self-obvious comment, I'd say...

> +	wfi	@ Wait For Interrupt
> +
> +	/*
> +	 * CPU can specualatively prefetch the instructions

    Speculatively.

[...]

WBR, Sergei

[PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Sergei Shtylyov]

Hello.

On 1/26/2015 1:06 PM, Wenyou Yang wrote:

> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>   arch/arm/mach-at91/pm_suspend.S |   54 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 53 insertions(+), 1 deletion(-)

> diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
> index 122a3f1..e796722 100644
> --- a/arch/arm/mach-at91/pm_suspend.S
> +++ b/arch/arm/mach-at91/pm_suspend.S
> @@ -53,6 +53,58 @@ mode	.req	r6
>   	beq	1b
>   	.endm
>
> +/*
> + * Put the processor to enter the WFI state
> + */
> +	.macro _do_wfi
> +
> +#if defined(CONFIG_CPU_V7)
> +	/*
> +	 * Execute an ISB instruction to flush the pipeline to ensure
> +	 * that all of operations have beem completed.

    Been.

> +	 */
> +	isb
> +
> +	/*
> +	 * Execute an ISB instruction to ensure that all of the

    ISB again, while you're executing DSB/DMB?

> +	 * CP15 register changes have been committed.
> +	 */
> +	dsb
> +	dmb
> +
> +	/* Disable the processor's clock */
> +	mov	tmp1, #AT91_PMC_PCK

    What's 'tmp1'? Is that a register name?

> +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> +
> +	/* Execute a WFI instruction */

    Self-obvious comment, I'd say...

> +	wfi	@ Wait For Interrupt
> +
> +	/*
> +	 * CPU can specualatively prefetch the instructions

    Speculatively.

[...]

WBR, Sergei

Re: [PATCH 4/7] ARM: at91: enable the L2 Cache controller

[Alexandre Belloni]

Hi Wenyou,

This patch is not necessary, the only thing missing is the prefetch
configuration and I will submit the correct DT snippet for 3.21.

On 26/01/2015 at 18:07:16 +0800, Wenyou Yang wrote :
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>  arch/arm/mach-at91/board-dt-sama5.c |   53 +++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
> index 86cffcd..ed6db28 100644
> --- a/arch/arm/mach-at91/board-dt-sama5.c
> +++ b/arch/arm/mach-at91/board-dt-sama5.c
> @@ -17,17 +17,70 @@
>  #include <linux/of_platform.h>
>  #include <linux/phy.h>
>  #include <linux/clk-provider.h>
> +#include <linux/of_address.h>
>  
>  #include <asm/setup.h>
>  #include <asm/irq.h>
>  #include <asm/mach/arch.h>
>  #include <asm/mach/map.h>
>  #include <asm/mach/irq.h>
> +#include <asm/hardware/cache-l2x0.h>
>  
>  #include "generic.h"
>  
> +void __iomem *at91_l2cc_base;
> +EXPORT_SYMBOL_GPL(at91_l2cc_base);
> +
> +#ifdef CONFIG_CACHE_L2X0
> +static void __init at91_init_l2cache(void)
> +{
> +	struct device_node *np;
> +	u32 reg;
> +
> +	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> +	if (!np)
> +		return;
> +
> +	at91_l2cc_base = of_iomap(np, 0);
> +	if (!at91_l2cc_base)
> +		panic("unable to map l2cc cpu registers\n");
> +
> +	of_node_put(np);
> +
> +	/* Disable cache if it hasn't been done yet */
> +	if (readl_relaxed(at91_l2cc_base + L2X0_CTRL) & L2X0_CTRL_EN)
> +		writel_relaxed(~L2X0_CTRL_EN, at91_l2cc_base + L2X0_CTRL);
> +
> +	/* Prefetch Control */
> +	reg = readl_relaxed(at91_l2cc_base + L310_PREFETCH_CTRL);
> +	reg &= ~L310_PREFETCH_CTRL_OFFSET_MASK;
> +	reg |= 0x01;
> +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
> +	reg |= L310_PREFETCH_CTRL_PREFETCH_DROP;
> +	reg |= L310_PREFETCH_CTRL_DATA_PREFETCH;
> +	reg |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
> +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL;
> +	writel_relaxed(reg, at91_l2cc_base + L310_PREFETCH_CTRL);
> +
> +	/* Power Control */
> +	reg = readl_relaxed(at91_l2cc_base + L310_POWER_CTRL);
> +	reg |= L310_STNDBY_MODE_EN;
> +	reg |= L310_DYNAMIC_CLK_GATING_EN;
> +	writel_relaxed(reg, at91_l2cc_base + L310_POWER_CTRL);
> +
> +	/* Disable interrupts */
> +	writel_relaxed(0x00, at91_l2cc_base + L2X0_INTR_MASK);
> +	writel_relaxed(0x01ff, at91_l2cc_base + L2X0_INTR_CLEAR);
> +	l2x0_of_init(0, ~0UL);
> +}
> +#else
> +static inline void at91_init_l2cache(void) {}
> +#endif
> +
>  static void __init sama5_dt_device_init(void)
>  {
> +	at91_init_l2cache();
> +
>  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>  	at91_sam9x5_pm_init();
>  }
> -- 
> 1.7.9.5
> 

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

[PATCH 4/7] ARM: at91: enable the L2 Cache controller

[Alexandre Belloni]

Hi Wenyou,

This patch is not necessary, the only thing missing is the prefetch
configuration and I will submit the correct DT snippet for 3.21.

On 26/01/2015 at 18:07:16 +0800, Wenyou Yang wrote :
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>  arch/arm/mach-at91/board-dt-sama5.c |   53 +++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
> index 86cffcd..ed6db28 100644
> --- a/arch/arm/mach-at91/board-dt-sama5.c
> +++ b/arch/arm/mach-at91/board-dt-sama5.c
> @@ -17,17 +17,70 @@
>  #include <linux/of_platform.h>
>  #include <linux/phy.h>
>  #include <linux/clk-provider.h>
> +#include <linux/of_address.h>
>  
>  #include <asm/setup.h>
>  #include <asm/irq.h>
>  #include <asm/mach/arch.h>
>  #include <asm/mach/map.h>
>  #include <asm/mach/irq.h>
> +#include <asm/hardware/cache-l2x0.h>
>  
>  #include "generic.h"
>  
> +void __iomem *at91_l2cc_base;
> +EXPORT_SYMBOL_GPL(at91_l2cc_base);
> +
> +#ifdef CONFIG_CACHE_L2X0
> +static void __init at91_init_l2cache(void)
> +{
> +	struct device_node *np;
> +	u32 reg;
> +
> +	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> +	if (!np)
> +		return;
> +
> +	at91_l2cc_base = of_iomap(np, 0);
> +	if (!at91_l2cc_base)
> +		panic("unable to map l2cc cpu registers\n");
> +
> +	of_node_put(np);
> +
> +	/* Disable cache if it hasn't been done yet */
> +	if (readl_relaxed(at91_l2cc_base + L2X0_CTRL) & L2X0_CTRL_EN)
> +		writel_relaxed(~L2X0_CTRL_EN, at91_l2cc_base + L2X0_CTRL);
> +
> +	/* Prefetch Control */
> +	reg = readl_relaxed(at91_l2cc_base + L310_PREFETCH_CTRL);
> +	reg &= ~L310_PREFETCH_CTRL_OFFSET_MASK;
> +	reg |= 0x01;
> +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
> +	reg |= L310_PREFETCH_CTRL_PREFETCH_DROP;
> +	reg |= L310_PREFETCH_CTRL_DATA_PREFETCH;
> +	reg |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
> +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL;
> +	writel_relaxed(reg, at91_l2cc_base + L310_PREFETCH_CTRL);
> +
> +	/* Power Control */
> +	reg = readl_relaxed(at91_l2cc_base + L310_POWER_CTRL);
> +	reg |= L310_STNDBY_MODE_EN;
> +	reg |= L310_DYNAMIC_CLK_GATING_EN;
> +	writel_relaxed(reg, at91_l2cc_base + L310_POWER_CTRL);
> +
> +	/* Disable interrupts */
> +	writel_relaxed(0x00, at91_l2cc_base + L2X0_INTR_MASK);
> +	writel_relaxed(0x01ff, at91_l2cc_base + L2X0_INTR_CLEAR);
> +	l2x0_of_init(0, ~0UL);
> +}
> +#else
> +static inline void at91_init_l2cache(void) {}
> +#endif
> +
>  static void __init sama5_dt_device_init(void)
>  {
> +	at91_init_l2cache();
> +
>  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>  	at91_sam9x5_pm_init();
>  }
> -- 
> 1.7.9.5
> 

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

RE: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Yang, Wenyou]

Hi Sergei,

Thank you for your review.

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com]
> Sent: Monday, January 26, 2015 9:05 PM
> To: Yang, Wenyou; Ferre, Nicolas; linux@arm.linux.org.uk
> Cc: sylvain.rochet@finsecur.com; Vilchez, Patrice; linux-kernel@vger.kernel.org;
> alexandre.belloni@free-electrons.com; peda@axentia.se; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7
> 
> Hello.
> 
> On 1/26/2015 1:06 PM, Wenyou Yang wrote:
> 
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >   arch/arm/mach-at91/pm_suspend.S |   54
> ++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 53 insertions(+), 1 deletion(-)
> 
> > diff --git a/arch/arm/mach-at91/pm_suspend.S
> > b/arch/arm/mach-at91/pm_suspend.S index 122a3f1..e796722 100644
> > --- a/arch/arm/mach-at91/pm_suspend.S
> > +++ b/arch/arm/mach-at91/pm_suspend.S
> > @@ -53,6 +53,58 @@ mode	.req	r6
> >   	beq	1b
> >   	.endm
> >
> > +/*
> > + * Put the processor to enter the WFI state  */
> > +	.macro _do_wfi
> > +
> > +#if defined(CONFIG_CPU_V7)
> > +	/*
> > +	 * Execute an ISB instruction to flush the pipeline to ensure
> > +	 * that all of operations have beem completed.
> 
>     Been.
> 
> > +	 */
> > +	isb
> > +
> > +	/*
> > +	 * Execute an ISB instruction to ensure that all of the
> 
>     ISB again, while you're executing DSB/DMB?
>
Thank you for your pointing.
 
> > +	 * CP15 register changes have been committed.
> > +	 */
> > +	dsb
> > +	dmb
> > +
> > +	/* Disable the processor's clock */
> > +	mov	tmp1, #AT91_PMC_PCK
> 
>     What's 'tmp1'? Is that a register name?
Yes, a register name defined at the head of file.

> 
> > +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> > +
> > +	/* Execute a WFI instruction */
> 
>     Self-obvious comment, I'd say...
> 
> > +	wfi	@ Wait For Interrupt
> > +
> > +	/*
> > +	 * CPU can specualatively prefetch the instructions
> 
>     Speculatively.
Thanks.
> 
> [...]
> 
> WBR, Sergei


Best Regards,
Wenyou Yang

[PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Yang, Wenyou]

Hi Sergei,

Thank you for your review.

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sergei.shtylyov at cogentembedded.com]
> Sent: Monday, January 26, 2015 9:05 PM
> To: Yang, Wenyou; Ferre, Nicolas; linux at arm.linux.org.uk
> Cc: sylvain.rochet at finsecur.com; Vilchez, Patrice; linux-kernel at vger.kernel.org;
> alexandre.belloni at free-electrons.com; peda at axentia.se; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7
> 
> Hello.
> 
> On 1/26/2015 1:06 PM, Wenyou Yang wrote:
> 
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >   arch/arm/mach-at91/pm_suspend.S |   54
> ++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 53 insertions(+), 1 deletion(-)
> 
> > diff --git a/arch/arm/mach-at91/pm_suspend.S
> > b/arch/arm/mach-at91/pm_suspend.S index 122a3f1..e796722 100644
> > --- a/arch/arm/mach-at91/pm_suspend.S
> > +++ b/arch/arm/mach-at91/pm_suspend.S
> > @@ -53,6 +53,58 @@ mode	.req	r6
> >   	beq	1b
> >   	.endm
> >
> > +/*
> > + * Put the processor to enter the WFI state  */
> > +	.macro _do_wfi
> > +
> > +#if defined(CONFIG_CPU_V7)
> > +	/*
> > +	 * Execute an ISB instruction to flush the pipeline to ensure
> > +	 * that all of operations have beem completed.
> 
>     Been.
> 
> > +	 */
> > +	isb
> > +
> > +	/*
> > +	 * Execute an ISB instruction to ensure that all of the
> 
>     ISB again, while you're executing DSB/DMB?
>
Thank you for your pointing.
 
> > +	 * CP15 register changes have been committed.
> > +	 */
> > +	dsb
> > +	dmb
> > +
> > +	/* Disable the processor's clock */
> > +	mov	tmp1, #AT91_PMC_PCK
> 
>     What's 'tmp1'? Is that a register name?
Yes, a register name defined at the head of file.

> 
> > +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> > +
> > +	/* Execute a WFI instruction */
> 
>     Self-obvious comment, I'd say...
> 
> > +	wfi	@ Wait For Interrupt
> > +
> > +	/*
> > +	 * CPU can specualatively prefetch the instructions
> 
>     Speculatively.
Thanks.
> 
> [...]
> 
> WBR, Sergei


Best Regards,
Wenyou Yang

RE: [PATCH 4/7] ARM: at91: enable the L2 Cache controller

[Yang, Wenyou]

Hi Alexandre,

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
> Sent: Tuesday, January 27, 2015 6:37 AM
> To: Yang, Wenyou
> Cc: Ferre, Nicolas; linux@arm.linux.org.uk; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; sylvain.rochet@finsecur.com; peda@axentia.se;
> Vilchez, Patrice
> Subject: Re: [PATCH 4/7] ARM: at91: enable the L2 Cache controller
> 
> Hi Wenyou,
> 
> This patch is not necessary, the only thing missing is the prefetch configuration
> and I will submit the correct DT snippet for 3.21.
I will drop this patch.

> 
> On 26/01/2015 at 18:07:16 +0800, Wenyou Yang wrote :
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >  arch/arm/mach-at91/board-dt-sama5.c |   53
> +++++++++++++++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> >
> > diff --git a/arch/arm/mach-at91/board-dt-sama5.c
> > b/arch/arm/mach-at91/board-dt-sama5.c
> > index 86cffcd..ed6db28 100644
> > --- a/arch/arm/mach-at91/board-dt-sama5.c
> > +++ b/arch/arm/mach-at91/board-dt-sama5.c
> > @@ -17,17 +17,70 @@
> >  #include <linux/of_platform.h>
> >  #include <linux/phy.h>
> >  #include <linux/clk-provider.h>
> > +#include <linux/of_address.h>
> >
> >  #include <asm/setup.h>
> >  #include <asm/irq.h>
> >  #include <asm/mach/arch.h>
> >  #include <asm/mach/map.h>
> >  #include <asm/mach/irq.h>
> > +#include <asm/hardware/cache-l2x0.h>
> >
> >  #include "generic.h"
> >
> > +void __iomem *at91_l2cc_base;
> > +EXPORT_SYMBOL_GPL(at91_l2cc_base);
> > +
> > +#ifdef CONFIG_CACHE_L2X0
> > +static void __init at91_init_l2cache(void) {
> > +	struct device_node *np;
> > +	u32 reg;
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> > +	if (!np)
> > +		return;
> > +
> > +	at91_l2cc_base = of_iomap(np, 0);
> > +	if (!at91_l2cc_base)
> > +		panic("unable to map l2cc cpu registers\n");
> > +
> > +	of_node_put(np);
> > +
> > +	/* Disable cache if it hasn't been done yet */
> > +	if (readl_relaxed(at91_l2cc_base + L2X0_CTRL) & L2X0_CTRL_EN)
> > +		writel_relaxed(~L2X0_CTRL_EN, at91_l2cc_base + L2X0_CTRL);
> > +
> > +	/* Prefetch Control */
> > +	reg = readl_relaxed(at91_l2cc_base + L310_PREFETCH_CTRL);
> > +	reg &= ~L310_PREFETCH_CTRL_OFFSET_MASK;
> > +	reg |= 0x01;
> > +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
> > +	reg |= L310_PREFETCH_CTRL_PREFETCH_DROP;
> > +	reg |= L310_PREFETCH_CTRL_DATA_PREFETCH;
> > +	reg |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
> > +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL;
> > +	writel_relaxed(reg, at91_l2cc_base + L310_PREFETCH_CTRL);
> > +
> > +	/* Power Control */
> > +	reg = readl_relaxed(at91_l2cc_base + L310_POWER_CTRL);
> > +	reg |= L310_STNDBY_MODE_EN;
> > +	reg |= L310_DYNAMIC_CLK_GATING_EN;
> > +	writel_relaxed(reg, at91_l2cc_base + L310_POWER_CTRL);
> > +
> > +	/* Disable interrupts */
> > +	writel_relaxed(0x00, at91_l2cc_base + L2X0_INTR_MASK);
> > +	writel_relaxed(0x01ff, at91_l2cc_base + L2X0_INTR_CLEAR);
> > +	l2x0_of_init(0, ~0UL);
> > +}
> > +#else
> > +static inline void at91_init_l2cache(void) {} #endif
> > +
> >  static void __init sama5_dt_device_init(void)  {
> > +	at91_init_l2cache();
> > +
> >  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> >  	at91_sam9x5_pm_init();
> >  }
> > --
> > 1.7.9.5
> >
> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com


Best Regards,
Wenyou Yang

[PATCH 4/7] ARM: at91: enable the L2 Cache controller

[Yang, Wenyou]

Hi Alexandre,

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni at free-electrons.com]
> Sent: Tuesday, January 27, 2015 6:37 AM
> To: Yang, Wenyou
> Cc: Ferre, Nicolas; linux at arm.linux.org.uk; linux-arm-kernel at lists.infradead.org;
> linux-kernel at vger.kernel.org; sylvain.rochet at finsecur.com; peda at axentia.se;
> Vilchez, Patrice
> Subject: Re: [PATCH 4/7] ARM: at91: enable the L2 Cache controller
> 
> Hi Wenyou,
> 
> This patch is not necessary, the only thing missing is the prefetch configuration
> and I will submit the correct DT snippet for 3.21.
I will drop this patch.

> 
> On 26/01/2015 at 18:07:16 +0800, Wenyou Yang wrote :
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >  arch/arm/mach-at91/board-dt-sama5.c |   53
> +++++++++++++++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> >
> > diff --git a/arch/arm/mach-at91/board-dt-sama5.c
> > b/arch/arm/mach-at91/board-dt-sama5.c
> > index 86cffcd..ed6db28 100644
> > --- a/arch/arm/mach-at91/board-dt-sama5.c
> > +++ b/arch/arm/mach-at91/board-dt-sama5.c
> > @@ -17,17 +17,70 @@
> >  #include <linux/of_platform.h>
> >  #include <linux/phy.h>
> >  #include <linux/clk-provider.h>
> > +#include <linux/of_address.h>
> >
> >  #include <asm/setup.h>
> >  #include <asm/irq.h>
> >  #include <asm/mach/arch.h>
> >  #include <asm/mach/map.h>
> >  #include <asm/mach/irq.h>
> > +#include <asm/hardware/cache-l2x0.h>
> >
> >  #include "generic.h"
> >
> > +void __iomem *at91_l2cc_base;
> > +EXPORT_SYMBOL_GPL(at91_l2cc_base);
> > +
> > +#ifdef CONFIG_CACHE_L2X0
> > +static void __init at91_init_l2cache(void) {
> > +	struct device_node *np;
> > +	u32 reg;
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> > +	if (!np)
> > +		return;
> > +
> > +	at91_l2cc_base = of_iomap(np, 0);
> > +	if (!at91_l2cc_base)
> > +		panic("unable to map l2cc cpu registers\n");
> > +
> > +	of_node_put(np);
> > +
> > +	/* Disable cache if it hasn't been done yet */
> > +	if (readl_relaxed(at91_l2cc_base + L2X0_CTRL) & L2X0_CTRL_EN)
> > +		writel_relaxed(~L2X0_CTRL_EN, at91_l2cc_base + L2X0_CTRL);
> > +
> > +	/* Prefetch Control */
> > +	reg = readl_relaxed(at91_l2cc_base + L310_PREFETCH_CTRL);
> > +	reg &= ~L310_PREFETCH_CTRL_OFFSET_MASK;
> > +	reg |= 0x01;
> > +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
> > +	reg |= L310_PREFETCH_CTRL_PREFETCH_DROP;
> > +	reg |= L310_PREFETCH_CTRL_DATA_PREFETCH;
> > +	reg |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
> > +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL;
> > +	writel_relaxed(reg, at91_l2cc_base + L310_PREFETCH_CTRL);
> > +
> > +	/* Power Control */
> > +	reg = readl_relaxed(at91_l2cc_base + L310_POWER_CTRL);
> > +	reg |= L310_STNDBY_MODE_EN;
> > +	reg |= L310_DYNAMIC_CLK_GATING_EN;
> > +	writel_relaxed(reg, at91_l2cc_base + L310_POWER_CTRL);
> > +
> > +	/* Disable interrupts */
> > +	writel_relaxed(0x00, at91_l2cc_base + L2X0_INTR_MASK);
> > +	writel_relaxed(0x01ff, at91_l2cc_base + L2X0_INTR_CLEAR);
> > +	l2x0_of_init(0, ~0UL);
> > +}
> > +#else
> > +static inline void at91_init_l2cache(void) {} #endif
> > +
> >  static void __init sama5_dt_device_init(void)  {
> > +	at91_init_l2cache();
> > +
> >  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> >  	at91_sam9x5_pm_init();
> >  }
> > --
> > 1.7.9.5
> >
> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com


Best Regards,
Wenyou Yang

RE: [PATCH 6/7] pm: at91: add achieve the mpddrc peripheral ID and the DDR clock ID support

[Yang, Wenyou]

Hi Mark,

Thank you for your review.

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@arm.com]
> Sent: Monday, January 26, 2015 7:50 PM
> To: Yang, Wenyou
> Cc: Ferre, Nicolas; linux@arm.linux.org.uk; sylvain.rochet@finsecur.com; Vilchez,
> Patrice; linux-kernel@vger.kernel.org; alexandre.belloni@free-electrons.com;
> peda@axentia.se; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 6/7] pm: at91: add achieve the mpddrc peripheral ID and the
> DDR clock ID support
> 
> On Mon, Jan 26, 2015 at 10:08:16AM +0000, Wenyou Yang wrote:
> > The patch achieves the mpddr controller peripheral ID and the DDR
> > clock ID from the dts file.
> >
> > They will be used in the future to disable the mpddr controller'c
> > clock the and DDR clock to decrease the power consumption during suspending.
> >
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >  arch/arm/mach-at91/generic.h |    2 ++
> >  arch/arm/mach-at91/setup.c   |   24 ++++++++++++++++++++++++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/arch/arm/mach-at91/generic.h
> > b/arch/arm/mach-at91/generic.h index 41796bf..3c72a3e 100644
> > --- a/arch/arm/mach-at91/generic.h
> > +++ b/arch/arm/mach-at91/generic.h
> > @@ -47,6 +47,8 @@ void __init at91_sam9x5_pm_init(void) { }  struct
> > at91_pm_struct {
> >  	unsigned long uhp_udp_mask;
> >  	int memctrl;
> > +	u32 mpddrc_id[2];
> > +	u32 ddrck_id;
> >  };
> >
> >  #endif /* _AT91_GENERIC_H */
> > diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c
> > index 7924663..a306f95 100644
> > --- a/arch/arm/mach-at91/setup.c
> > +++ b/arch/arm/mach-at91/setup.c
> > @@ -363,6 +363,27 @@ void __init at91_ioremap_matrix(u32 base_addr)
> >  		panic(pr_fmt("Impossible to ioremap at91_matrix_base\n"));  }
> >
> > +static u32 at91_of_get_ddr_id(struct device_node *np, char *name) {
> > +	struct of_phandle_args clkspec;
> > +	u32 id;
> > +	int index;
> > +	int rc;
> > +
> > +	index = of_property_match_string(np, "clock-names", name);
> > +	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
> &clkspec);
> > +	if (rc)
> > +		return 0;
> > +
> > +	rc = of_property_read_u32(clkspec.np, "reg", &id);
> > +	if (rc)
> > +		return 0;
> > +
> > +	of_node_put(clkspec.np);
> > +
> > +	return id;
> > +}
> 
> This doesn't look right to me. This assumes the format of the clock provider node,
> which invalidates the point of having the abstraction in the first place.
> 
> > +
> >  struct at91_ramc_of_data {
> >  	u8 ramc_type;
> >  };
> > @@ -400,6 +421,9 @@ static void at91_dt_ramc(void)
> >  		of_data = of_id->data;
> >  		at91_pm_data.memctrl = of_data->ramc_type;
> >
> > +		at91_pm_data.mpddrc_id[idx] = at91_of_get_ddr_id(np, "mpddr");
> > +		at91_pm_data.ddrck_id = at91_of_get_ddr_id(np, "ddrck");
> > +
> 
> Why do you need these here?
> 
> Surely the logic for poking any clocks should live in the relevant clock controller
> drivers?
Thank for your suggestion.

I thought that it is reasonable to get the DDR controller's peripheral id from the DDR device node.

Anyway, let me think over how to do it.

> 
> Mark.

Best Regards,
Wenyou Yang

[PATCH 6/7] pm: at91: add achieve the mpddrc peripheral ID and the DDR clock ID support

[Yang, Wenyou]

Hi Mark,

Thank you for your review.

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland at arm.com]
> Sent: Monday, January 26, 2015 7:50 PM
> To: Yang, Wenyou
> Cc: Ferre, Nicolas; linux at arm.linux.org.uk; sylvain.rochet at finsecur.com; Vilchez,
> Patrice; linux-kernel at vger.kernel.org; alexandre.belloni at free-electrons.com;
> peda at axentia.se; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 6/7] pm: at91: add achieve the mpddrc peripheral ID and the
> DDR clock ID support
> 
> On Mon, Jan 26, 2015 at 10:08:16AM +0000, Wenyou Yang wrote:
> > The patch achieves the mpddr controller peripheral ID and the DDR
> > clock ID from the dts file.
> >
> > They will be used in the future to disable the mpddr controller'c
> > clock the and DDR clock to decrease the power consumption during suspending.
> >
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >  arch/arm/mach-at91/generic.h |    2 ++
> >  arch/arm/mach-at91/setup.c   |   24 ++++++++++++++++++++++++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/arch/arm/mach-at91/generic.h
> > b/arch/arm/mach-at91/generic.h index 41796bf..3c72a3e 100644
> > --- a/arch/arm/mach-at91/generic.h
> > +++ b/arch/arm/mach-at91/generic.h
> > @@ -47,6 +47,8 @@ void __init at91_sam9x5_pm_init(void) { }  struct
> > at91_pm_struct {
> >  	unsigned long uhp_udp_mask;
> >  	int memctrl;
> > +	u32 mpddrc_id[2];
> > +	u32 ddrck_id;
> >  };
> >
> >  #endif /* _AT91_GENERIC_H */
> > diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c
> > index 7924663..a306f95 100644
> > --- a/arch/arm/mach-at91/setup.c
> > +++ b/arch/arm/mach-at91/setup.c
> > @@ -363,6 +363,27 @@ void __init at91_ioremap_matrix(u32 base_addr)
> >  		panic(pr_fmt("Impossible to ioremap at91_matrix_base\n"));  }
> >
> > +static u32 at91_of_get_ddr_id(struct device_node *np, char *name) {
> > +	struct of_phandle_args clkspec;
> > +	u32 id;
> > +	int index;
> > +	int rc;
> > +
> > +	index = of_property_match_string(np, "clock-names", name);
> > +	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
> &clkspec);
> > +	if (rc)
> > +		return 0;
> > +
> > +	rc = of_property_read_u32(clkspec.np, "reg", &id);
> > +	if (rc)
> > +		return 0;
> > +
> > +	of_node_put(clkspec.np);
> > +
> > +	return id;
> > +}
> 
> This doesn't look right to me. This assumes the format of the clock provider node,
> which invalidates the point of having the abstraction in the first place.
> 
> > +
> >  struct at91_ramc_of_data {
> >  	u8 ramc_type;
> >  };
> > @@ -400,6 +421,9 @@ static void at91_dt_ramc(void)
> >  		of_data = of_id->data;
> >  		at91_pm_data.memctrl = of_data->ramc_type;
> >
> > +		at91_pm_data.mpddrc_id[idx] = at91_of_get_ddr_id(np, "mpddr");
> > +		at91_pm_data.ddrck_id = at91_of_get_ddr_id(np, "ddrck");
> > +
> 
> Why do you need these here?
> 
> Surely the logic for poking any clocks should live in the relevant clock controller
> drivers?
Thank for your suggestion.

I thought that it is reasonable to get the DDR controller's peripheral id from the DDR device node.

Anyway, let me think over how to do it.

> 
> Mark.

Best Regards,
Wenyou Yang

Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Lorenzo Pieralisi]

On Mon, Jan 26, 2015 at 10:06:01AM +0000, Wenyou Yang wrote:

Commit log please.

> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>  arch/arm/mach-at91/pm_suspend.S |   54 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
> index 122a3f1..e796722 100644
> --- a/arch/arm/mach-at91/pm_suspend.S
> +++ b/arch/arm/mach-at91/pm_suspend.S
> @@ -53,6 +53,58 @@ mode	.req	r6
>  	beq	1b
>  	.endm
>  
> +/*
> + * Put the processor to enter the WFI state
> + */
> +	.macro _do_wfi

You will have to explain why you need this, really.

> +
> +#if defined(CONFIG_CPU_V7)
> +	/*
> +	 * Execute an ISB instruction to flush the pipeline to ensure
> +	 * that all of operations have beem completed.

s/beem/been

> +	 */
> +	isb
> +
> +	/*
> +	 * Execute an ISB instruction to ensure that all of the
> +	 * CP15 register changes have been committed.
> +	 */
> +	dsb

This is a dsb not an isb.

> +	dmb

You have to explain why you need every single one of these barriers,
otherwise I am NAKing this patch.

> +
> +	/* Disable the processor's clock */
> +	mov	tmp1, #AT91_PMC_PCK
> +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> +
> +	/* Execute a WFI instruction */
> +	wfi	@ Wait For Interrupt

This one looks ok :)

> +
> +	/*
> +	 * CPU can specualatively prefetch the instructions
> +	 * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline.

So what ? I suspect your issue is related to wfi completion on pending
IRQ. I would like to know the details that describe the issue you are
trying to solve here please.

> +	 */
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +#else
> +	mcr	p15, 0, tmp1, c7, c0, 4
> +#endif

Tell us what's the problem you have to solve, first, then we will see how
to fix it.

Thanks,
Lorenzo

> +
> +	.endm
> +
>  	.text
>  
>  /*
> @@ -181,7 +233,7 @@ sdr_sr_done:
>  
>  skip_disable_main_clock:
>  	/* Wait for interrupt */
> -	mcr	p15, 0, tmp1, c7, c0, 4
> +	_do_wfi
>  
>  	tst	mode, #AT91_PM_SLOW_CLOCK
>  	beq	skip_enable_main_clock
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

[PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Lorenzo Pieralisi]

On Mon, Jan 26, 2015 at 10:06:01AM +0000, Wenyou Yang wrote:

Commit log please.

> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>  arch/arm/mach-at91/pm_suspend.S |   54 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
> index 122a3f1..e796722 100644
> --- a/arch/arm/mach-at91/pm_suspend.S
> +++ b/arch/arm/mach-at91/pm_suspend.S
> @@ -53,6 +53,58 @@ mode	.req	r6
>  	beq	1b
>  	.endm
>  
> +/*
> + * Put the processor to enter the WFI state
> + */
> +	.macro _do_wfi

You will have to explain why you need this, really.

> +
> +#if defined(CONFIG_CPU_V7)
> +	/*
> +	 * Execute an ISB instruction to flush the pipeline to ensure
> +	 * that all of operations have beem completed.

s/beem/been

> +	 */
> +	isb
> +
> +	/*
> +	 * Execute an ISB instruction to ensure that all of the
> +	 * CP15 register changes have been committed.
> +	 */
> +	dsb

This is a dsb not an isb.

> +	dmb

You have to explain why you need every single one of these barriers,
otherwise I am NAKing this patch.

> +
> +	/* Disable the processor's clock */
> +	mov	tmp1, #AT91_PMC_PCK
> +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> +
> +	/* Execute a WFI instruction */
> +	wfi	@ Wait For Interrupt

This one looks ok :)

> +
> +	/*
> +	 * CPU can specualatively prefetch the instructions
> +	 * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline.

So what ? I suspect your issue is related to wfi completion on pending
IRQ. I would like to know the details that describe the issue you are
trying to solve here please.

> +	 */
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +#else
> +	mcr	p15, 0, tmp1, c7, c0, 4
> +#endif

Tell us what's the problem you have to solve, first, then we will see how
to fix it.

Thanks,
Lorenzo

> +
> +	.endm
> +
>  	.text
>  
>  /*
> @@ -181,7 +233,7 @@ sdr_sr_done:
>  
>  skip_disable_main_clock:
>  	/* Wait for interrupt */
> -	mcr	p15, 0, tmp1, c7, c0, 4
> +	_do_wfi
>  
>  	tst	mode, #AT91_PM_SLOW_CLOCK
>  	beq	skip_enable_main_clock
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

RE: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Yang, Wenyou]

Hi Lorenzo,

Thank you for your review.

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> Sent: Wednesday, January 28, 2015 7:26 PM
> To: Yang, Wenyou
> Cc: Ferre, Nicolas; linux@arm.linux.org.uk; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; alexandre.belloni@free-electrons.com;
> sylvain.rochet@finsecur.com; peda@axentia.se; Vilchez, Patrice
> Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7
> 
> On Mon, Jan 26, 2015 at 10:06:01AM +0000, Wenyou Yang wrote:
> 
> Commit log please.
Added in the v2.0

> 
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >  arch/arm/mach-at91/pm_suspend.S |   54
> ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/mach-at91/pm_suspend.S
> > b/arch/arm/mach-at91/pm_suspend.S index 122a3f1..e796722 100644
> > --- a/arch/arm/mach-at91/pm_suspend.S
> > +++ b/arch/arm/mach-at91/pm_suspend.S
> > @@ -53,6 +53,58 @@ mode	.req	r6
> >  	beq	1b
> >  	.endm
> >
> > +/*
> > + * Put the processor to enter the WFI state  */
> > +	.macro _do_wfi
> 
> You will have to explain why you need this, really.
I don't understand your meaning.

> 
> > +
> > +#if defined(CONFIG_CPU_V7)
> > +	/*
> > +	 * Execute an ISB instruction to flush the pipeline to ensure
> > +	 * that all of operations have beem completed.
> 
> s/beem/been
Thanks.

> 
> > +	 */
> > +	isb
> > +
> > +	/*
> > +	 * Execute an ISB instruction to ensure that all of the
> > +	 * CP15 register changes have been committed.
> > +	 */
> > +	dsb
> 
> This is a dsb not an isb.
Changed in the v2.0

> 
> > +	dmb
> 
> You have to explain why you need every single one of these barriers, otherwise I
> am NAKing this patch.
No need this one?

> 
> > +
> > +	/* Disable the processor's clock */
> > +	mov	tmp1, #AT91_PMC_PCK
> > +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> > +
> > +	/* Execute a WFI instruction */
> > +	wfi	@ Wait For Interrupt
> 
> This one looks ok :)
> 
> > +
> > +	/*
> > +	 * CPU can specualatively prefetch the instructions
> > +	 * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline.
> 
> So what ? I suspect your issue is related to wfi completion on pending IRQ. I
> would like to know the details that describe the issue you are trying to solve here
> please.
Honestly, I referred to others, I will dig more, and test it.

> 
> > +	 */
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +#else
> > +	mcr	p15, 0, tmp1, c7, c0, 4
> > +#endif
> 
> Tell us what's the problem you have to solve, first, then we will see how to fix it.
> 
> Thanks,
> Lorenzo
> 
> > +
> > +	.endm
> > +
> >  	.text
> >
> >  /*
> > @@ -181,7 +233,7 @@ sdr_sr_done:
> >
> >  skip_disable_main_clock:
> >  	/* Wait for interrupt */
> > -	mcr	p15, 0, tmp1, c7, c0, 4
> > +	_do_wfi
> >
> >  	tst	mode, #AT91_PM_SLOW_CLOCK
> >  	beq	skip_enable_main_clock
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-kernel" in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >

Best Regards,
Wenyou Yang

[PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Yang, Wenyou]

Hi Lorenzo,

Thank you for your review.

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> Sent: Wednesday, January 28, 2015 7:26 PM
> To: Yang, Wenyou
> Cc: Ferre, Nicolas; linux at arm.linux.org.uk; linux-arm-kernel at lists.infradead.org;
> linux-kernel at vger.kernel.org; alexandre.belloni at free-electrons.com;
> sylvain.rochet at finsecur.com; peda at axentia.se; Vilchez, Patrice
> Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7
> 
> On Mon, Jan 26, 2015 at 10:06:01AM +0000, Wenyou Yang wrote:
> 
> Commit log please.
Added in the v2.0

> 
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >  arch/arm/mach-at91/pm_suspend.S |   54
> ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/mach-at91/pm_suspend.S
> > b/arch/arm/mach-at91/pm_suspend.S index 122a3f1..e796722 100644
> > --- a/arch/arm/mach-at91/pm_suspend.S
> > +++ b/arch/arm/mach-at91/pm_suspend.S
> > @@ -53,6 +53,58 @@ mode	.req	r6
> >  	beq	1b
> >  	.endm
> >
> > +/*
> > + * Put the processor to enter the WFI state  */
> > +	.macro _do_wfi
> 
> You will have to explain why you need this, really.
I don't understand your meaning.

> 
> > +
> > +#if defined(CONFIG_CPU_V7)
> > +	/*
> > +	 * Execute an ISB instruction to flush the pipeline to ensure
> > +	 * that all of operations have beem completed.
> 
> s/beem/been
Thanks.

> 
> > +	 */
> > +	isb
> > +
> > +	/*
> > +	 * Execute an ISB instruction to ensure that all of the
> > +	 * CP15 register changes have been committed.
> > +	 */
> > +	dsb
> 
> This is a dsb not an isb.
Changed in the v2.0

> 
> > +	dmb
> 
> You have to explain why you need every single one of these barriers, otherwise I
> am NAKing this patch.
No need this one?

> 
> > +
> > +	/* Disable the processor's clock */
> > +	mov	tmp1, #AT91_PMC_PCK
> > +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> > +
> > +	/* Execute a WFI instruction */
> > +	wfi	@ Wait For Interrupt
> 
> This one looks ok :)
> 
> > +
> > +	/*
> > +	 * CPU can specualatively prefetch the instructions
> > +	 * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline.
> 
> So what ? I suspect your issue is related to wfi completion on pending IRQ. I
> would like to know the details that describe the issue you are trying to solve here
> please.
Honestly, I referred to others, I will dig more, and test it.

> 
> > +	 */
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +#else
> > +	mcr	p15, 0, tmp1, c7, c0, 4
> > +#endif
> 
> Tell us what's the problem you have to solve, first, then we will see how to fix it.
> 
> Thanks,
> Lorenzo
> 
> > +
> > +	.endm
> > +
> >  	.text
> >
> >  /*
> > @@ -181,7 +233,7 @@ sdr_sr_done:
> >
> >  skip_disable_main_clock:
> >  	/* Wait for interrupt */
> > -	mcr	p15, 0, tmp1, c7, c0, 4
> > +	_do_wfi
> >
> >  	tst	mode, #AT91_PM_SLOW_CLOCK
> >  	beq	skip_enable_main_clock
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-kernel" in the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >

Best Regards,
Wenyou Yang

Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Lorenzo Pieralisi]

On Thu, Jan 29, 2015 at 02:36:01AM +0000, Yang, Wenyou wrote:
> Hi Lorenzo,
> 
> Thank you for your review.
> 
> > -----Original Message-----
> > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> > Sent: Wednesday, January 28, 2015 7:26 PM
> > To: Yang, Wenyou
> > Cc: Ferre, Nicolas; linux@arm.linux.org.uk; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; alexandre.belloni@free-electrons.com;
> > sylvain.rochet@finsecur.com; peda@axentia.se; Vilchez, Patrice
> > Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7
> > 
> > On Mon, Jan 26, 2015 at 10:06:01AM +0000, Wenyou Yang wrote:
> > 
> > Commit log please.
> Added in the v2.0
> 
> > 
> > > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > > ---
> > >  arch/arm/mach-at91/pm_suspend.S |   54
> > ++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 53 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm/mach-at91/pm_suspend.S
> > > b/arch/arm/mach-at91/pm_suspend.S index 122a3f1..e796722 100644
> > > --- a/arch/arm/mach-at91/pm_suspend.S
> > > +++ b/arch/arm/mach-at91/pm_suspend.S
> > > @@ -53,6 +53,58 @@ mode	.req	r6
> > >  	beq	1b
> > >  	.endm
> > >
> > > +/*
> > > + * Put the processor to enter the WFI state  */
> > > +	.macro _do_wfi
> > 
> > You will have to explain why you need this, really.
> I don't understand your meaning.

I want to understand why this assembly snippet (that can be rewritten
in C BTW):

/* Disable the processor's clock */
mov	tmp1, #AT91_PMC_PCK
str	tmp1, [pmc, #AT91_PMC_SCDR]

+

cpu_do_idle()

is not sufficient for you, or put it differently, why do you need
this macro.

> 
> > 
> > > +
> > > +#if defined(CONFIG_CPU_V7)
> > > +	/*
> > > +	 * Execute an ISB instruction to flush the pipeline to ensure
> > > +	 * that all of operations have beem completed.
> > 
> > s/beem/been
> Thanks.
> 
> > 
> > > +	 */
> > > +	isb

This isb should not be there, unless you know a reason why it should
and you explain it to me.

> > > +
> > > +	/*
> > > +	 * Execute an ISB instruction to ensure that all of the
> > > +	 * CP15 register changes have been committed.
> > > +	 */
> > > +	dsb
> > 
> > This is a dsb not an isb.
> Changed in the v2.0

Yes, but I still do not understand why you want to execute it before
disabling the clocks (I really hope that by "disabling the clocks" you
mean "set the power controller to a state when, on wfi execution, the
clocks are gated").

> 
> > 
> > > +	dmb
> > 
> > You have to explain why you need every single one of these barriers, otherwise I
> > am NAKing this patch.
> No need this one?

No, remove it.

> 
> > 
> > > +
> > > +	/* Disable the processor's clock */
> > > +	mov	tmp1, #AT91_PMC_PCK
> > > +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> > > +
> > > +	/* Execute a WFI instruction */
> > > +	wfi	@ Wait For Interrupt
> > 
> > This one looks ok :)
> > 
> > > +
> > > +	/*
> > > +	 * CPU can specualatively prefetch the instructions
> > > +	 * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline.
> > 
> > So what ? I suspect your issue is related to wfi completion on pending IRQ. I
> > would like to know the details that describe the issue you are trying to solve here
> > please.
> Honestly, I referred to others, I will dig more, and test it.

You should not copy and paste code, because:

1) it might be broken
2) and/or unoptimized
3) and/or does not apply to your platform

See my suggestion above, if it does not work for you, you will report the
issue and we will take it from there.

Thanks,
Lorenzo

> 
> > 
> > > +	 */
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +#else
> > > +	mcr	p15, 0, tmp1, c7, c0, 4
> > > +#endif
> > 
> > Tell us what's the problem you have to solve, first, then we will see how to fix it.
> > 
> > Thanks,
> > Lorenzo
> > 
> > > +
> > > +	.endm
> > > +
> > >  	.text
> > >
> > >  /*
> > > @@ -181,7 +233,7 @@ sdr_sr_done:
> > >
> > >  skip_disable_main_clock:
> > >  	/* Wait for interrupt */
> > > -	mcr	p15, 0, tmp1, c7, c0, 4
> > > +	_do_wfi
> > >
> > >  	tst	mode, #AT91_PM_SLOW_CLOCK
> > >  	beq	skip_enable_main_clock
> > > --
> > > 1.7.9.5
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe
> > > linux-kernel" in the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at  http://www.tux.org/lkml/
> > >
> 
> Best Regards,
> Wenyou Yang
> 

[PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Lorenzo Pieralisi]

On Thu, Jan 29, 2015 at 02:36:01AM +0000, Yang, Wenyou wrote:
> Hi Lorenzo,
> 
> Thank you for your review.
> 
> > -----Original Message-----
> > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> > Sent: Wednesday, January 28, 2015 7:26 PM
> > To: Yang, Wenyou
> > Cc: Ferre, Nicolas; linux at arm.linux.org.uk; linux-arm-kernel at lists.infradead.org;
> > linux-kernel at vger.kernel.org; alexandre.belloni at free-electrons.com;
> > sylvain.rochet at finsecur.com; peda at axentia.se; Vilchez, Patrice
> > Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7
> > 
> > On Mon, Jan 26, 2015 at 10:06:01AM +0000, Wenyou Yang wrote:
> > 
> > Commit log please.
> Added in the v2.0
> 
> > 
> > > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > > ---
> > >  arch/arm/mach-at91/pm_suspend.S |   54
> > ++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 53 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm/mach-at91/pm_suspend.S
> > > b/arch/arm/mach-at91/pm_suspend.S index 122a3f1..e796722 100644
> > > --- a/arch/arm/mach-at91/pm_suspend.S
> > > +++ b/arch/arm/mach-at91/pm_suspend.S
> > > @@ -53,6 +53,58 @@ mode	.req	r6
> > >  	beq	1b
> > >  	.endm
> > >
> > > +/*
> > > + * Put the processor to enter the WFI state  */
> > > +	.macro _do_wfi
> > 
> > You will have to explain why you need this, really.
> I don't understand your meaning.

I want to understand why this assembly snippet (that can be rewritten
in C BTW):

/* Disable the processor's clock */
mov	tmp1, #AT91_PMC_PCK
str	tmp1, [pmc, #AT91_PMC_SCDR]

+

cpu_do_idle()

is not sufficient for you, or put it differently, why do you need
this macro.

> 
> > 
> > > +
> > > +#if defined(CONFIG_CPU_V7)
> > > +	/*
> > > +	 * Execute an ISB instruction to flush the pipeline to ensure
> > > +	 * that all of operations have beem completed.
> > 
> > s/beem/been
> Thanks.
> 
> > 
> > > +	 */
> > > +	isb

This isb should not be there, unless you know a reason why it should
and you explain it to me.

> > > +
> > > +	/*
> > > +	 * Execute an ISB instruction to ensure that all of the
> > > +	 * CP15 register changes have been committed.
> > > +	 */
> > > +	dsb
> > 
> > This is a dsb not an isb.
> Changed in the v2.0

Yes, but I still do not understand why you want to execute it before
disabling the clocks (I really hope that by "disabling the clocks" you
mean "set the power controller to a state when, on wfi execution, the
clocks are gated").

> 
> > 
> > > +	dmb
> > 
> > You have to explain why you need every single one of these barriers, otherwise I
> > am NAKing this patch.
> No need this one?

No, remove it.

> 
> > 
> > > +
> > > +	/* Disable the processor's clock */
> > > +	mov	tmp1, #AT91_PMC_PCK
> > > +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> > > +
> > > +	/* Execute a WFI instruction */
> > > +	wfi	@ Wait For Interrupt
> > 
> > This one looks ok :)
> > 
> > > +
> > > +	/*
> > > +	 * CPU can specualatively prefetch the instructions
> > > +	 * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline.
> > 
> > So what ? I suspect your issue is related to wfi completion on pending IRQ. I
> > would like to know the details that describe the issue you are trying to solve here
> > please.
> Honestly, I referred to others, I will dig more, and test it.

You should not copy and paste code, because:

1) it might be broken
2) and/or unoptimized
3) and/or does not apply to your platform

See my suggestion above, if it does not work for you, you will report the
issue and we will take it from there.

Thanks,
Lorenzo

> 
> > 
> > > +	 */
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +	nop
> > > +#else
> > > +	mcr	p15, 0, tmp1, c7, c0, 4
> > > +#endif
> > 
> > Tell us what's the problem you have to solve, first, then we will see how to fix it.
> > 
> > Thanks,
> > Lorenzo
> > 
> > > +
> > > +	.endm
> > > +
> > >  	.text
> > >
> > >  /*
> > > @@ -181,7 +233,7 @@ sdr_sr_done:
> > >
> > >  skip_disable_main_clock:
> > >  	/* Wait for interrupt */
> > > -	mcr	p15, 0, tmp1, c7, c0, 4
> > > +	_do_wfi
> > >
> > >  	tst	mode, #AT91_PM_SLOW_CLOCK
> > >  	beq	skip_enable_main_clock
> > > --
> > > 1.7.9.5
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe
> > > linux-kernel" in the body of a message to majordomo at vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at  http://www.tux.org/lkml/
> > >
> 
> Best Regards,
> Wenyou Yang
> 

RE: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Yang, Wenyou]

Hi Lorenzo,

Thanks a lot.

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> Sent: Thursday, January 29, 2015 8:22 PM
> To: Yang, Wenyou
> Cc: Ferre, Nicolas; linux@arm.linux.org.uk; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; alexandre.belloni@free-electrons.com;
> sylvain.rochet@finsecur.com; peda@axentia.se; Vilchez, Patrice
> Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7
> 
> On Thu, Jan 29, 2015 at 02:36:01AM +0000, Yang, Wenyou wrote:
> > Hi Lorenzo,
> >
> > Thank you for your review.
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> > > Sent: Wednesday, January 28, 2015 7:26 PM
> > > To: Yang, Wenyou
> > > Cc: Ferre, Nicolas; linux@arm.linux.org.uk;
> > > linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; alexandre.belloni@free-electrons.com;
> > > sylvain.rochet@finsecur.com; peda@axentia.se; Vilchez, Patrice
> > > Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support
> > > for ARMv7
> > >
> > > On Mon, Jan 26, 2015 at 10:06:01AM +0000, Wenyou Yang wrote:
> > >
> > > Commit log please.
> > Added in the v2.0
> >
> > >
> > > > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > > > ---
> > > >  arch/arm/mach-at91/pm_suspend.S |   54
> > > ++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 53 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm/mach-at91/pm_suspend.S
> > > > b/arch/arm/mach-at91/pm_suspend.S index 122a3f1..e796722 100644
> > > > --- a/arch/arm/mach-at91/pm_suspend.S
> > > > +++ b/arch/arm/mach-at91/pm_suspend.S
> > > > @@ -53,6 +53,58 @@ mode	.req	r6
> > > >  	beq	1b
> > > >  	.endm
> > > >
> > > > +/*
> > > > + * Put the processor to enter the WFI state  */
> > > > +	.macro _do_wfi
> > >
> > > You will have to explain why you need this, really.
> > I don't understand your meaning.
> 
> I want to understand why this assembly snippet (that can be rewritten in C BTW):
> 
> /* Disable the processor's clock */
> mov	tmp1, #AT91_PMC_PCK
> str	tmp1, [pmc, #AT91_PMC_SCDR]
> 
> +
> 
> cpu_do_idle()
> 
> is not sufficient for you, or put it differently, why do you need this macro.
This assembly snippet will be copied and run in the SRAM, in the period the DDR is in the self-refresh state.
So, it can't invoke other functions generally. 

> 
> >
> > >
> > > > +
> > > > +#if defined(CONFIG_CPU_V7)
> > > > +	/*
> > > > +	 * Execute an ISB instruction to flush the pipeline to ensure
> > > > +	 * that all of operations have beem completed.
> > >
> > > s/beem/been
> > Thanks.
> >
> > >
> > > > +	 */
> > > > +	isb
> 
> This isb should not be there, unless you know a reason why it should and you
> explain it to me.
I encountered system lock during verifying the pm function. 
Anyway, I will tested again whether it works after removing it. 

> 
> > > > +
> > > > +	/*
> > > > +	 * Execute an ISB instruction to ensure that all of the
> > > > +	 * CP15 register changes have been committed.
> > > > +	 */
> > > > +	dsb
> > >
> > > This is a dsb not an isb.
> > Changed in the v2.0
> 
> Yes, but I still do not understand why you want to execute it before disabling the
> clocks (I really hope that by "disabling the clocks" you mean "set the power
> controller to a state when, on wfi execution, the clocks are gated").
Are you meaning to execute dsb and wfi after disabling the clocks?

> 
> >
> > >
> > > > +	dmb
> > >
> > > You have to explain why you need every single one of these barriers,
> > > otherwise I am NAKing this patch.
> > No need this one?
> 
> No, remove it.
OK, thanks

> 
> >
> > >
> > > > +
> > > > +	/* Disable the processor's clock */
> > > > +	mov	tmp1, #AT91_PMC_PCK
> > > > +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> > > > +
> > > > +	/* Execute a WFI instruction */
> > > > +	wfi	@ Wait For Interrupt
> > >
> > > This one looks ok :)
> > >
> > > > +
> > > > +	/*
> > > > +	 * CPU can specualatively prefetch the instructions
> > > > +	 * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline.
> > >
> > > So what ? I suspect your issue is related to wfi completion on
> > > pending IRQ. I would like to know the details that describe the
> > > issue you are trying to solve here please.
> > Honestly, I referred to others, I will dig more, and test it.
> 
> You should not copy and paste code, because:
> 
> 1) it might be broken
> 2) and/or unoptimized
> 3) and/or does not apply to your platform
> 
> See my suggestion above, if it does not work for you, you will report the issue and
> we will take it from there.
OK, thanks.

> 
> Thanks,
> Lorenzo
> 
> >
> > >
> > > > +	 */
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +#else
> > > > +	mcr	p15, 0, tmp1, c7, c0, 4
> > > > +#endif
> > >
> > > Tell us what's the problem you have to solve, first, then we will see how to fix it.
> > >
> > > Thanks,
> > > Lorenzo
> > >
> > > > +
> > > > +	.endm
> > > > +
> > > >  	.text
> > > >
> > > >  /*
> > > > @@ -181,7 +233,7 @@ sdr_sr_done:
> > > >
> > > >  skip_disable_main_clock:
> > > >  	/* Wait for interrupt */
> > > > -	mcr	p15, 0, tmp1, c7, c0, 4
> > > > +	_do_wfi
> > > >
> > > >  	tst	mode, #AT91_PM_SLOW_CLOCK
> > > >  	beq	skip_enable_main_clock
> > > > --
> > > > 1.7.9.5
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe
> > > > linux-kernel" in the body of a message to
> > > > majordomo@vger.kernel.org More majordomo info at
> > > > http://vger.kernel.org/majordomo-info.html
> > > > Please read the FAQ at  http://www.tux.org/lkml/
> > > >
> >
> > Best Regards,
> > Wenyou Yang
> >

Best Regards,
Wenyou Yang

[PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Yang, Wenyou]

Hi Lorenzo,

Thanks a lot.

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> Sent: Thursday, January 29, 2015 8:22 PM
> To: Yang, Wenyou
> Cc: Ferre, Nicolas; linux at arm.linux.org.uk; linux-arm-kernel at lists.infradead.org;
> linux-kernel at vger.kernel.org; alexandre.belloni at free-electrons.com;
> sylvain.rochet at finsecur.com; peda at axentia.se; Vilchez, Patrice
> Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7
> 
> On Thu, Jan 29, 2015 at 02:36:01AM +0000, Yang, Wenyou wrote:
> > Hi Lorenzo,
> >
> > Thank you for your review.
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> > > Sent: Wednesday, January 28, 2015 7:26 PM
> > > To: Yang, Wenyou
> > > Cc: Ferre, Nicolas; linux at arm.linux.org.uk;
> > > linux-arm-kernel at lists.infradead.org;
> > > linux-kernel at vger.kernel.org; alexandre.belloni at free-electrons.com;
> > > sylvain.rochet at finsecur.com; peda at axentia.se; Vilchez, Patrice
> > > Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support
> > > for ARMv7
> > >
> > > On Mon, Jan 26, 2015 at 10:06:01AM +0000, Wenyou Yang wrote:
> > >
> > > Commit log please.
> > Added in the v2.0
> >
> > >
> > > > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > > > ---
> > > >  arch/arm/mach-at91/pm_suspend.S |   54
> > > ++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 53 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm/mach-at91/pm_suspend.S
> > > > b/arch/arm/mach-at91/pm_suspend.S index 122a3f1..e796722 100644
> > > > --- a/arch/arm/mach-at91/pm_suspend.S
> > > > +++ b/arch/arm/mach-at91/pm_suspend.S
> > > > @@ -53,6 +53,58 @@ mode	.req	r6
> > > >  	beq	1b
> > > >  	.endm
> > > >
> > > > +/*
> > > > + * Put the processor to enter the WFI state  */
> > > > +	.macro _do_wfi
> > >
> > > You will have to explain why you need this, really.
> > I don't understand your meaning.
> 
> I want to understand why this assembly snippet (that can be rewritten in C BTW):
> 
> /* Disable the processor's clock */
> mov	tmp1, #AT91_PMC_PCK
> str	tmp1, [pmc, #AT91_PMC_SCDR]
> 
> +
> 
> cpu_do_idle()
> 
> is not sufficient for you, or put it differently, why do you need this macro.
This assembly snippet will be copied and run in the SRAM, in the period the DDR is in the self-refresh state.
So, it can't invoke other functions generally. 

> 
> >
> > >
> > > > +
> > > > +#if defined(CONFIG_CPU_V7)
> > > > +	/*
> > > > +	 * Execute an ISB instruction to flush the pipeline to ensure
> > > > +	 * that all of operations have beem completed.
> > >
> > > s/beem/been
> > Thanks.
> >
> > >
> > > > +	 */
> > > > +	isb
> 
> This isb should not be there, unless you know a reason why it should and you
> explain it to me.
I encountered system lock during verifying the pm function. 
Anyway, I will tested again whether it works after removing it. 

> 
> > > > +
> > > > +	/*
> > > > +	 * Execute an ISB instruction to ensure that all of the
> > > > +	 * CP15 register changes have been committed.
> > > > +	 */
> > > > +	dsb
> > >
> > > This is a dsb not an isb.
> > Changed in the v2.0
> 
> Yes, but I still do not understand why you want to execute it before disabling the
> clocks (I really hope that by "disabling the clocks" you mean "set the power
> controller to a state when, on wfi execution, the clocks are gated").
Are you meaning to execute dsb and wfi after disabling the clocks?

> 
> >
> > >
> > > > +	dmb
> > >
> > > You have to explain why you need every single one of these barriers,
> > > otherwise I am NAKing this patch.
> > No need this one?
> 
> No, remove it.
OK, thanks

> 
> >
> > >
> > > > +
> > > > +	/* Disable the processor's clock */
> > > > +	mov	tmp1, #AT91_PMC_PCK
> > > > +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> > > > +
> > > > +	/* Execute a WFI instruction */
> > > > +	wfi	@ Wait For Interrupt
> > >
> > > This one looks ok :)
> > >
> > > > +
> > > > +	/*
> > > > +	 * CPU can specualatively prefetch the instructions
> > > > +	 * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline.
> > >
> > > So what ? I suspect your issue is related to wfi completion on
> > > pending IRQ. I would like to know the details that describe the
> > > issue you are trying to solve here please.
> > Honestly, I referred to others, I will dig more, and test it.
> 
> You should not copy and paste code, because:
> 
> 1) it might be broken
> 2) and/or unoptimized
> 3) and/or does not apply to your platform
> 
> See my suggestion above, if it does not work for you, you will report the issue and
> we will take it from there.
OK, thanks.

> 
> Thanks,
> Lorenzo
> 
> >
> > >
> > > > +	 */
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +	nop
> > > > +#else
> > > > +	mcr	p15, 0, tmp1, c7, c0, 4
> > > > +#endif
> > >
> > > Tell us what's the problem you have to solve, first, then we will see how to fix it.
> > >
> > > Thanks,
> > > Lorenzo
> > >
> > > > +
> > > > +	.endm
> > > > +
> > > >  	.text
> > > >
> > > >  /*
> > > > @@ -181,7 +233,7 @@ sdr_sr_done:
> > > >
> > > >  skip_disable_main_clock:
> > > >  	/* Wait for interrupt */
> > > > -	mcr	p15, 0, tmp1, c7, c0, 4
> > > > +	_do_wfi
> > > >
> > > >  	tst	mode, #AT91_PM_SLOW_CLOCK
> > > >  	beq	skip_enable_main_clock
> > > > --
> > > > 1.7.9.5
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe
> > > > linux-kernel" in the body of a message to
> > > > majordomo at vger.kernel.org More majordomo info at
> > > > http://vger.kernel.org/majordomo-info.html
> > > > Please read the FAQ at  http://www.tux.org/lkml/
> > > >
> >
> > Best Regards,
> > Wenyou Yang
> >

Best Regards,
Wenyou Yang

Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Lorenzo Pieralisi]

On Fri, Jan 30, 2015 at 07:23:21AM +0000, Yang, Wenyou wrote:

[...]

> > > > > + * Put the processor to enter the WFI state  */
> > > > > +	.macro _do_wfi
> > > >
> > > > You will have to explain why you need this, really.
> > > I don't understand your meaning.
> > 
> > I want to understand why this assembly snippet (that can be rewritten in C BTW):
> > 
> > /* Disable the processor's clock */
> > mov	tmp1, #AT91_PMC_PCK
> > str	tmp1, [pmc, #AT91_PMC_SCDR]
> > 
> > +
> > 
> > cpu_do_idle()
> > 
> > is not sufficient for you, or put it differently, why do you need this macro.
> This assembly snippet will be copied and run in the SRAM, in the period the DDR is in the self-refresh state.
> So, it can't invoke other functions generally. 

Ok, thanks for explaining, I will have a look again to check where you
use the macro to verify the code flow.

> 
> > 
> > >
> > > >
> > > > > +
> > > > > +#if defined(CONFIG_CPU_V7)
> > > > > +	/*
> > > > > +	 * Execute an ISB instruction to flush the pipeline to ensure
> > > > > +	 * that all of operations have beem completed.
> > > >
> > > > s/beem/been
> > > Thanks.
> > >
> > > >
> > > > > +	 */
> > > > > +	isb
> > 
> > This isb should not be there, unless you know a reason why it should and you
> > explain it to me.
> I encountered system lock during verifying the pm function. 
> Anyway, I will tested again whether it works after removing it. 

See above, I have to check when you switch to SRAM execution to see
if an isb is appropriate there, it is not self-explanatory.

Thank you,
Lorenzo

> 
> > 
> > > > > +
> > > > > +	/*
> > > > > +	 * Execute an ISB instruction to ensure that all of the
> > > > > +	 * CP15 register changes have been committed.
> > > > > +	 */
> > > > > +	dsb
> > > >
> > > > This is a dsb not an isb.
> > > Changed in the v2.0
> > 
> > Yes, but I still do not understand why you want to execute it before disabling the
> > clocks (I really hope that by "disabling the clocks" you mean "set the power
> > controller to a state when, on wfi execution, the clocks are gated").
> Are you meaning to execute dsb and wfi after disabling the clocks?
> 
> > 
> > >
> > > >
> > > > > +	dmb
> > > >
> > > > You have to explain why you need every single one of these barriers,
> > > > otherwise I am NAKing this patch.
> > > No need this one?
> > 
> > No, remove it.
> OK, thanks
> 
> > 
> > >
> > > >
> > > > > +
> > > > > +	/* Disable the processor's clock */
> > > > > +	mov	tmp1, #AT91_PMC_PCK
> > > > > +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> > > > > +
> > > > > +	/* Execute a WFI instruction */
> > > > > +	wfi	@ Wait For Interrupt
> > > >
> > > > This one looks ok :)
> > > >
> > > > > +
> > > > > +	/*
> > > > > +	 * CPU can specualatively prefetch the instructions
> > > > > +	 * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline.
> > > >
> > > > So what ? I suspect your issue is related to wfi completion on
> > > > pending IRQ. I would like to know the details that describe the
> > > > issue you are trying to solve here please.
> > > Honestly, I referred to others, I will dig more, and test it.
> > 
> > You should not copy and paste code, because:
> > 
> > 1) it might be broken
> > 2) and/or unoptimized
> > 3) and/or does not apply to your platform
> > 
> > See my suggestion above, if it does not work for you, you will report the issue and
> > we will take it from there.
> OK, thanks.
> 
> > 
> > Thanks,
> > Lorenzo
> > 
> > >
> > > >
> > > > > +	 */
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +#else
> > > > > +	mcr	p15, 0, tmp1, c7, c0, 4
> > > > > +#endif
> > > >
> > > > Tell us what's the problem you have to solve, first, then we will see how to fix it.
> > > >
> > > > Thanks,
> > > > Lorenzo
> > > >
> > > > > +
> > > > > +	.endm
> > > > > +
> > > > >  	.text
> > > > >
> > > > >  /*
> > > > > @@ -181,7 +233,7 @@ sdr_sr_done:
> > > > >
> > > > >  skip_disable_main_clock:
> > > > >  	/* Wait for interrupt */
> > > > > -	mcr	p15, 0, tmp1, c7, c0, 4
> > > > > +	_do_wfi
> > > > >
> > > > >  	tst	mode, #AT91_PM_SLOW_CLOCK
> > > > >  	beq	skip_enable_main_clock
> > > > > --
> > > > > 1.7.9.5
> > > > >
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe
> > > > > linux-kernel" in the body of a message to
> > > > > majordomo@vger.kernel.org More majordomo info at
> > > > > http://vger.kernel.org/majordomo-info.html
> > > > > Please read the FAQ at  http://www.tux.org/lkml/
> > > > >
> > >
> > > Best Regards,
> > > Wenyou Yang
> > >
> 
> Best Regards,
> Wenyou Yang
> 

[PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Lorenzo Pieralisi]

On Fri, Jan 30, 2015 at 07:23:21AM +0000, Yang, Wenyou wrote:

[...]

> > > > > + * Put the processor to enter the WFI state  */
> > > > > +	.macro _do_wfi
> > > >
> > > > You will have to explain why you need this, really.
> > > I don't understand your meaning.
> > 
> > I want to understand why this assembly snippet (that can be rewritten in C BTW):
> > 
> > /* Disable the processor's clock */
> > mov	tmp1, #AT91_PMC_PCK
> > str	tmp1, [pmc, #AT91_PMC_SCDR]
> > 
> > +
> > 
> > cpu_do_idle()
> > 
> > is not sufficient for you, or put it differently, why do you need this macro.
> This assembly snippet will be copied and run in the SRAM, in the period the DDR is in the self-refresh state.
> So, it can't invoke other functions generally. 

Ok, thanks for explaining, I will have a look again to check where you
use the macro to verify the code flow.

> 
> > 
> > >
> > > >
> > > > > +
> > > > > +#if defined(CONFIG_CPU_V7)
> > > > > +	/*
> > > > > +	 * Execute an ISB instruction to flush the pipeline to ensure
> > > > > +	 * that all of operations have beem completed.
> > > >
> > > > s/beem/been
> > > Thanks.
> > >
> > > >
> > > > > +	 */
> > > > > +	isb
> > 
> > This isb should not be there, unless you know a reason why it should and you
> > explain it to me.
> I encountered system lock during verifying the pm function. 
> Anyway, I will tested again whether it works after removing it. 

See above, I have to check when you switch to SRAM execution to see
if an isb is appropriate there, it is not self-explanatory.

Thank you,
Lorenzo

> 
> > 
> > > > > +
> > > > > +	/*
> > > > > +	 * Execute an ISB instruction to ensure that all of the
> > > > > +	 * CP15 register changes have been committed.
> > > > > +	 */
> > > > > +	dsb
> > > >
> > > > This is a dsb not an isb.
> > > Changed in the v2.0
> > 
> > Yes, but I still do not understand why you want to execute it before disabling the
> > clocks (I really hope that by "disabling the clocks" you mean "set the power
> > controller to a state when, on wfi execution, the clocks are gated").
> Are you meaning to execute dsb and wfi after disabling the clocks?
> 
> > 
> > >
> > > >
> > > > > +	dmb
> > > >
> > > > You have to explain why you need every single one of these barriers,
> > > > otherwise I am NAKing this patch.
> > > No need this one?
> > 
> > No, remove it.
> OK, thanks
> 
> > 
> > >
> > > >
> > > > > +
> > > > > +	/* Disable the processor's clock */
> > > > > +	mov	tmp1, #AT91_PMC_PCK
> > > > > +	str	tmp1, [pmc, #AT91_PMC_SCDR]
> > > > > +
> > > > > +	/* Execute a WFI instruction */
> > > > > +	wfi	@ Wait For Interrupt
> > > >
> > > > This one looks ok :)
> > > >
> > > > > +
> > > > > +	/*
> > > > > +	 * CPU can specualatively prefetch the instructions
> > > > > +	 * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline.
> > > >
> > > > So what ? I suspect your issue is related to wfi completion on
> > > > pending IRQ. I would like to know the details that describe the
> > > > issue you are trying to solve here please.
> > > Honestly, I referred to others, I will dig more, and test it.
> > 
> > You should not copy and paste code, because:
> > 
> > 1) it might be broken
> > 2) and/or unoptimized
> > 3) and/or does not apply to your platform
> > 
> > See my suggestion above, if it does not work for you, you will report the issue and
> > we will take it from there.
> OK, thanks.
> 
> > 
> > Thanks,
> > Lorenzo
> > 
> > >
> > > >
> > > > > +	 */
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +	nop
> > > > > +#else
> > > > > +	mcr	p15, 0, tmp1, c7, c0, 4
> > > > > +#endif
> > > >
> > > > Tell us what's the problem you have to solve, first, then we will see how to fix it.
> > > >
> > > > Thanks,
> > > > Lorenzo
> > > >
> > > > > +
> > > > > +	.endm
> > > > > +
> > > > >  	.text
> > > > >
> > > > >  /*
> > > > > @@ -181,7 +233,7 @@ sdr_sr_done:
> > > > >
> > > > >  skip_disable_main_clock:
> > > > >  	/* Wait for interrupt */
> > > > > -	mcr	p15, 0, tmp1, c7, c0, 4
> > > > > +	_do_wfi
> > > > >
> > > > >  	tst	mode, #AT91_PM_SLOW_CLOCK
> > > > >  	beq	skip_enable_main_clock
> > > > > --
> > > > > 1.7.9.5
> > > > >
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe
> > > > > linux-kernel" in the body of a message to
> > > > > majordomo at vger.kernel.org More majordomo info at
> > > > > http://vger.kernel.org/majordomo-info.html
> > > > > Please read the FAQ at  http://www.tux.org/lkml/
> > > > >
> > >
> > > Best Regards,
> > > Wenyou Yang
> > >
> 
> Best Regards,
> Wenyou Yang
> 

Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Lorenzo Pieralisi]

On Fri, Jan 30, 2015 at 07:23:21AM +0000, Yang, Wenyou wrote:

[...]

> > > > > +	 */
> > > > > +	isb
> > 
> > This isb should not be there, unless you know a reason why it should and you
> > explain it to me.
> I encountered system lock during verifying the pm function. 
> Anyway, I will tested again whether it works after removing it. 

Re-checked the code flow, I see no reason why this isb should be there.
Adding barriers changes code execution, this might paper over some
issues instead of solving them, so every single barrier should be
commented and added with a specific purpose that I do not see here.

Lorenzo

[PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7

[Lorenzo Pieralisi]

On Fri, Jan 30, 2015 at 07:23:21AM +0000, Yang, Wenyou wrote:

[...]

> > > > > +	 */
> > > > > +	isb
> > 
> > This isb should not be there, unless you know a reason why it should and you
> > explain it to me.
> I encountered system lock during verifying the pm function. 
> Anyway, I will tested again whether it works after removing it. 

Re-checked the code flow, I see no reason why this isb should be there.
Adding barriers changes code execution, this might paper over some
issues instead of solving them, so every single barrier should be
commented and added with a specific purpose that I do not see here.

Lorenzo