[PATCH 0/3] ARM: at91: fix cpuidle crash on SAMA5D4 Xplained board

[PATCH 0/3] ARM: at91: fix cpuidle crash on SAMA5D4 Xplained board

[Wenyou Yang]

Fix cpuidle crash on SAMA5D4 Xplained board when enable
CONFIG_ARM_AT91_CPUIDLE. Because some SoCs have the L2 cache,
we should flush the L2 cache before entering the cpu idle.


Wenyou Yang (3):
  ARM: at91: flush the L2 cache before entering cpu idle
  doc: binding: add new compatible for SDRAM/DDR Controller
  ARM: dts: at91: use "atmel,sama5d4-ddramc" for ramc

 Documentation/devicetree/bindings/arm/atmel-at91.txt |  1 +
 arch/arm/boot/dts/sama5d2.dtsi                       |  2 +-
 arch/arm/boot/dts/sama5d4.dtsi                       |  2 +-
 arch/arm/mach-at91/pm.c                              | 19 +++++++++++++++++++
 drivers/memory/atmel-sdramc.c                        |  1 +
 5 files changed, 23 insertions(+), 2 deletions(-)

-- 
2.11.0

[PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Wenyou Yang]

For the SoCs such as SAMA5D2 and SAMA5D4 which have L2 cache,
flush the L2 cache first before entering the cpu idle.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---

 arch/arm/mach-at91/pm.c       | 19 +++++++++++++++++++
 drivers/memory/atmel-sdramc.c |  1 +
 2 files changed, 20 insertions(+)

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index b4332b727e9c..1a60dede1a01 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -289,6 +289,24 @@ static void at91_ddr_standby(void)
 		at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
 }
 
+static void at91_ddr_cache_standby(void)
+{
+	u32 saved_lpr;
+
+	flush_cache_all();
+	outer_disable();
+
+	saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
+	at91_ramc_write(0, AT91_DDRSDRC_LPR, (saved_lpr &
+			(~AT91_DDRSDRC_LPCB)) | AT91_DDRSDRC_LPCB_SELF_REFRESH);
+
+	cpu_do_idle();
+
+	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
+
+	outer_resume();
+}
+
 /* We manage both DDRAM/SDRAM controllers, we need more than one value to
  * remember.
  */
@@ -324,6 +342,7 @@ static const struct of_device_id const ramc_ids[] __initconst = {
 	{ .compatible = "atmel,at91sam9260-sdramc", .data = at91sam9_sdram_standby },
 	{ .compatible = "atmel,at91sam9g45-ddramc", .data = at91_ddr_standby },
 	{ .compatible = "atmel,sama5d3-ddramc", .data = at91_ddr_standby },
+	{ .compatible = "atmel,sama5d4-ddramc", .data = at91_ddr_cache_standby },
 	{ /*sentinel*/ }
 };
 
diff --git a/drivers/memory/atmel-sdramc.c b/drivers/memory/atmel-sdramc.c
index b418b39af180..7e5c5c6c1348 100644
--- a/drivers/memory/atmel-sdramc.c
+++ b/drivers/memory/atmel-sdramc.c
@@ -48,6 +48,7 @@ static const struct of_device_id atmel_ramc_of_match[] = {
 	{ .compatible = "atmel,at91sam9260-sdramc", .data = &at91rm9200_caps, },
 	{ .compatible = "atmel,at91sam9g45-ddramc", .data = &at91sam9g45_caps, },
 	{ .compatible = "atmel,sama5d3-ddramc", .data = &sama5d3_caps, },
+	{ .compatible = "atmel,sama5d4-ddramc", .data = &sama5d3_caps, },
 	{},
 };
 
-- 
2.11.0

[PATCH 2/3] doc: binding: add new compatible for SDRAM/DDR Controller

[Wenyou Yang]

Add the new compatible "atmel,sama5d4-ddramc" for the SDRAM/DDR
Controller.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---

 Documentation/devicetree/bindings/arm/atmel-at91.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
index 29737b9b616e..9b5de6397666 100644
--- a/Documentation/devicetree/bindings/arm/atmel-at91.txt
+++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
@@ -111,6 +111,7 @@ RAMC SDRAM/DDR Controller required properties:
 			"atmel,at91sam9260-sdramc",
 			"atmel,at91sam9g45-ddramc",
 			"atmel,sama5d3-ddramc",
+			"atmel,sama5d4-ddramc",
 - reg: Should contain registers location and length
 
 Examples:
-- 
2.11.0

[PATCH 3/3] ARM: dts: at91: use "atmel,sama5d4-ddramc" for ramc

[Wenyou Yang]

Use the new compatible "atmel,sama5d4-ddramc" for the ramc of
SAMA5D2 and SAMA5D4.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---

 arch/arm/boot/dts/sama5d2.dtsi | 2 +-
 arch/arm/boot/dts/sama5d4.dtsi | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index ceb9783ff7e1..b5259d85737d 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -380,7 +380,7 @@
 			};
 
 			ramc0: ramc@f000c000 {
-				compatible = "atmel,sama5d3-ddramc";
+				compatible = "atmel,sama5d4-ddramc";
 				reg = <0xf000c000 0x200>;
 				clocks = <&ddrck>, <&mpddr_clk>;
 				clock-names = "ddrck", "mpddr";
diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index 4f60c1b7b137..603ba986858c 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -370,7 +370,7 @@
 			};
 
 			ramc0: ramc@f0010000 {
-				compatible = "atmel,sama5d3-ddramc";
+				compatible = "atmel,sama5d4-ddramc";
 				reg = <0xf0010000 0x200>;
 				clocks = <&ddrck>, <&mpddr_clk>;
 				clock-names = "ddrck", "mpddr";
-- 
2.11.0

Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Alexandre Belloni]

Hi,

On 06/01/2017 at 14:59:45 +0800, Wenyou Yang wrote :
> For the SoCs such as SAMA5D2 and SAMA5D4 which have L2 cache,
> flush the L2 cache first before entering the cpu idle.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
> 
>  arch/arm/mach-at91/pm.c       | 19 +++++++++++++++++++
>  drivers/memory/atmel-sdramc.c |  1 +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index b4332b727e9c..1a60dede1a01 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -289,6 +289,24 @@ static void at91_ddr_standby(void)
>  		at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
>  }
>  
> +static void at91_ddr_cache_standby(void)
> +{
> +	u32 saved_lpr;
> +
> +	flush_cache_all();
> +	outer_disable();
> +
> +	saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
> +	at91_ramc_write(0, AT91_DDRSDRC_LPR, (saved_lpr &
> +			(~AT91_DDRSDRC_LPCB)) | AT91_DDRSDRC_LPCB_SELF_REFRESH);
> +
> +	cpu_do_idle();
> +
> +	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
> +
> +	outer_resume();
> +}
> +

Seems good to me. Did you measure the added latency on sama5d3 if you
add the cache operations in at91_ddr_standby instead of having a new
function?


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

RE: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Wenyou.Yang]

Hi Alexandre,

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
> Sent: 2017年1月6日 17:05
> To: Wenyou Yang - A41535 <Wenyou.Yang@microchip.com>
> Cc: Russell King <linux@arm.linux.org.uk>; Nicolas Ferre
> <nicolas.ferre@atmel.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
> <mark.rutland@arm.com>; linux-kernel@vger.kernel.org; Wenyou Yang - A41535
> <Wenyou.Yang@microchip.com>; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
> 
> Hi,
> 
> On 06/01/2017 at 14:59:45 +0800, Wenyou Yang wrote :
> > For the SoCs such as SAMA5D2 and SAMA5D4 which have L2 cache, flush
> > the L2 cache first before entering the cpu idle.
> >
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >
> >  arch/arm/mach-at91/pm.c       | 19 +++++++++++++++++++
> >  drivers/memory/atmel-sdramc.c |  1 +
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> > b4332b727e9c..1a60dede1a01 100644
> > --- a/arch/arm/mach-at91/pm.c
> > +++ b/arch/arm/mach-at91/pm.c
> > @@ -289,6 +289,24 @@ static void at91_ddr_standby(void)
> >  		at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);  }
> >
> > +static void at91_ddr_cache_standby(void) {
> > +	u32 saved_lpr;
> > +
> > +	flush_cache_all();
> > +	outer_disable();
> > +
> > +	saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
> > +	at91_ramc_write(0, AT91_DDRSDRC_LPR, (saved_lpr &
> > +			(~AT91_DDRSDRC_LPCB)) |
> AT91_DDRSDRC_LPCB_SELF_REFRESH);
> > +
> > +	cpu_do_idle();
> > +
> > +	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
> > +
> > +	outer_resume();
> > +}
> > +
> 
> Seems good to me. Did you measure the added latency on sama5d3 if you add the
> cache operations in at91_ddr_standby instead of having a new function?

No, I didn't. How to measure it?


Best Regards,
Wenyou Yang

Re: [PATCH 2/3] doc: binding: add new compatible for SDRAM/DDR Controller

[Rob Herring]

On Fri, Jan 06, 2017 at 02:59:46PM +0800, Wenyou Yang wrote:
> Add the new compatible "atmel,sama5d4-ddramc" for the SDRAM/DDR
> Controller.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
> 
>  Documentation/devicetree/bindings/arm/atmel-at91.txt | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Alexandre Belloni]

I though a bit more about it, and I don't really like the new compatible
string. I don't feel this should be necessary.

What about the following:

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index b4332b727e9c..0333aca63e44 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);
 static struct {
 	unsigned long uhp_udp_mask;
 	int memctrl;
+	bool has_l2_cache;
 } at91_pm_data;
 
 void __iomem *at91_ramc_base[2];
@@ -267,6 +268,11 @@ static void at91_ddr_standby(void)
 	u32 lpr0, lpr1 = 0;
 	u32 saved_lpr0, saved_lpr1 = 0;
 
+	if (at91_pm_data.has_l2_cache) {
+		flush_cache_all();
+		outer_disable();
+	}
+
 	if (at91_ramc_base[1]) {
 		saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
 		lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB;
@@ -287,6 +293,9 @@ static void at91_ddr_standby(void)
 	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
 	if (at91_ramc_base[1])
 		at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
+
+	if (at91_pm_data.has_l2_cache)
+		outer_resume();
 }
 
 /* We manage both DDRAM/SDRAM controllers, we need more than one value
 * to
@@ -353,6 +362,11 @@ static __init void at91_dt_ramc(void)
 		return;
 	}
 
+	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
+	if (np)
+		at91_pm_data.has_l2_cache = true;
+	of_node_put(np);
+
 	at91_pm_set_standby(standby);
 }


This has the following benefits:
 - everybody will have the fix, regardless of whether the dtb is updated
 - has_l2_cache can be used later in at91_pm_suspend instead of calling
   it unconditionnaly (I'll send a patch)


On 06/01/2017 at 14:59:45 +0800, Wenyou Yang wrote :
> For the SoCs such as SAMA5D2 and SAMA5D4 which have L2 cache,
> flush the L2 cache first before entering the cpu idle.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
> 
>  arch/arm/mach-at91/pm.c       | 19 +++++++++++++++++++
>  drivers/memory/atmel-sdramc.c |  1 +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index b4332b727e9c..1a60dede1a01 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -289,6 +289,24 @@ static void at91_ddr_standby(void)
>  		at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
>  }
>  
> +static void at91_ddr_cache_standby(void)
> +{
> +	u32 saved_lpr;
> +
> +	flush_cache_all();
> +	outer_disable();
> +
> +	saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
> +	at91_ramc_write(0, AT91_DDRSDRC_LPR, (saved_lpr &
> +			(~AT91_DDRSDRC_LPCB)) | AT91_DDRSDRC_LPCB_SELF_REFRESH);
> +
> +	cpu_do_idle();
> +
> +	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
> +
> +	outer_resume();
> +}
> +
>  /* We manage both DDRAM/SDRAM controllers, we need more than one value to
>   * remember.
>   */
> @@ -324,6 +342,7 @@ static const struct of_device_id const ramc_ids[] __initconst = {
>  	{ .compatible = "atmel,at91sam9260-sdramc", .data = at91sam9_sdram_standby },
>  	{ .compatible = "atmel,at91sam9g45-ddramc", .data = at91_ddr_standby },
>  	{ .compatible = "atmel,sama5d3-ddramc", .data = at91_ddr_standby },
> +	{ .compatible = "atmel,sama5d4-ddramc", .data = at91_ddr_cache_standby },
>  	{ /*sentinel*/ }
>  };
>  
> diff --git a/drivers/memory/atmel-sdramc.c b/drivers/memory/atmel-sdramc.c
> index b418b39af180..7e5c5c6c1348 100644
> --- a/drivers/memory/atmel-sdramc.c
> +++ b/drivers/memory/atmel-sdramc.c
> @@ -48,6 +48,7 @@ static const struct of_device_id atmel_ramc_of_match[] = {
>  	{ .compatible = "atmel,at91sam9260-sdramc", .data = &at91rm9200_caps, },
>  	{ .compatible = "atmel,at91sam9g45-ddramc", .data = &at91sam9g45_caps, },
>  	{ .compatible = "atmel,sama5d3-ddramc", .data = &sama5d3_caps, },
> +	{ .compatible = "atmel,sama5d4-ddramc", .data = &sama5d3_caps, },
>  	{},
>  };
>  
> -- 
> 2.11.0
> 

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

Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Alexandre Belloni]

On 10/01/2017 at 17:18:21 +0100, Alexandre Belloni wrote :
> I though a bit more about it, and I don't really like the new compatible
> string. I don't feel this should be necessary.
> 
> What about the following:
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index b4332b727e9c..0333aca63e44 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);
>  static struct {
>  	unsigned long uhp_udp_mask;
>  	int memctrl;
> +	bool has_l2_cache;
>  } at91_pm_data;
>  
>  void __iomem *at91_ramc_base[2];
> @@ -267,6 +268,11 @@ static void at91_ddr_standby(void)
>  	u32 lpr0, lpr1 = 0;
>  	u32 saved_lpr0, saved_lpr1 = 0;
>  
> +	if (at91_pm_data.has_l2_cache) {
> +		flush_cache_all();
> +		outer_disable();
> +	}
> +
>  	if (at91_ramc_base[1]) {
>  		saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
>  		lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB;
> @@ -287,6 +293,9 @@ static void at91_ddr_standby(void)
>  	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
>  	if (at91_ramc_base[1])
>  		at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
> +
> +	if (at91_pm_data.has_l2_cache)
> +		outer_resume();
>  }
>  
>  /* We manage both DDRAM/SDRAM controllers, we need more than one value
>  * to
> @@ -353,6 +362,11 @@ static __init void at91_dt_ramc(void)
>  		return;
>  	}
>  
> +	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> +	if (np)
> +		at91_pm_data.has_l2_cache = true;
> +	of_node_put(np);
> +
>  	at91_pm_set_standby(standby);
>  }
> 
> 
> This has the following benefits:
>  - everybody will have the fix, regardless of whether the dtb is updated
>  - has_l2_cache can be used later in at91_pm_suspend instead of calling
>    it unconditionnaly (I'll send a patch)
> 

I forgot to add that the added latency on at91sam9 and sama5d3 is exactly 5 instructions.

> 
> On 06/01/2017 at 14:59:45 +0800, Wenyou Yang wrote :
> > For the SoCs such as SAMA5D2 and SAMA5D4 which have L2 cache,
> > flush the L2 cache first before entering the cpu idle.
> > 
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> > 
> >  arch/arm/mach-at91/pm.c       | 19 +++++++++++++++++++
> >  drivers/memory/atmel-sdramc.c |  1 +
> >  2 files changed, 20 insertions(+)
> > 
> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> > index b4332b727e9c..1a60dede1a01 100644
> > --- a/arch/arm/mach-at91/pm.c
> > +++ b/arch/arm/mach-at91/pm.c
> > @@ -289,6 +289,24 @@ static void at91_ddr_standby(void)
> >  		at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
> >  }
> >  
> > +static void at91_ddr_cache_standby(void)
> > +{
> > +	u32 saved_lpr;
> > +
> > +	flush_cache_all();
> > +	outer_disable();
> > +
> > +	saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
> > +	at91_ramc_write(0, AT91_DDRSDRC_LPR, (saved_lpr &
> > +			(~AT91_DDRSDRC_LPCB)) | AT91_DDRSDRC_LPCB_SELF_REFRESH);
> > +
> > +	cpu_do_idle();
> > +
> > +	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
> > +
> > +	outer_resume();
> > +}
> > +
> >  /* We manage both DDRAM/SDRAM controllers, we need more than one value to
> >   * remember.
> >   */
> > @@ -324,6 +342,7 @@ static const struct of_device_id const ramc_ids[] __initconst = {
> >  	{ .compatible = "atmel,at91sam9260-sdramc", .data = at91sam9_sdram_standby },
> >  	{ .compatible = "atmel,at91sam9g45-ddramc", .data = at91_ddr_standby },
> >  	{ .compatible = "atmel,sama5d3-ddramc", .data = at91_ddr_standby },
> > +	{ .compatible = "atmel,sama5d4-ddramc", .data = at91_ddr_cache_standby },
> >  	{ /*sentinel*/ }
> >  };
> >  
> > diff --git a/drivers/memory/atmel-sdramc.c b/drivers/memory/atmel-sdramc.c
> > index b418b39af180..7e5c5c6c1348 100644
> > --- a/drivers/memory/atmel-sdramc.c
> > +++ b/drivers/memory/atmel-sdramc.c
> > @@ -48,6 +48,7 @@ static const struct of_device_id atmel_ramc_of_match[] = {
> >  	{ .compatible = "atmel,at91sam9260-sdramc", .data = &at91rm9200_caps, },
> >  	{ .compatible = "atmel,at91sam9g45-ddramc", .data = &at91sam9g45_caps, },
> >  	{ .compatible = "atmel,sama5d3-ddramc", .data = &sama5d3_caps, },
> > +	{ .compatible = "atmel,sama5d4-ddramc", .data = &sama5d3_caps, },
> >  	{},
> >  };
> >  
> > -- 
> > 2.11.0
> > 
> 
> -- 
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

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

Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Jean-Jacques Hiblot]

2017-01-10 17:18 GMT+01:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> I though a bit more about it, and I don't really like the new compatible
> string. I don't feel this should be necessary.
>
> What about the following:
>
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index b4332b727e9c..0333aca63e44 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);
>  static struct {
>         unsigned long uhp_udp_mask;
>         int memctrl;
> +       bool has_l2_cache;
>  } at91_pm_data;
>
>  void __iomem *at91_ramc_base[2];
> @@ -267,6 +268,11 @@ static void at91_ddr_standby(void)
>         u32 lpr0, lpr1 = 0;
>         u32 saved_lpr0, saved_lpr1 = 0;
>

> +       if (at91_pm_data.has_l2_cache) {
> +               flush_cache_all();
what is the point of calling flush_cache_all() here ? Do we really
care that dirty data in L1 is written to DDR ? I may be missing
something but to me it's just extra latency.
> +               outer_disable();
It seems to me that if there's no L2 cache, then outer_disable()  is a
no-op. It could be called unconditionally.
> +       }
> +
>         if (at91_ramc_base[1]) {
>                 saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
>                 lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB;
> @@ -287,6 +293,9 @@ static void at91_ddr_standby(void)
>         at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
>         if (at91_ramc_base[1])
>                 at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
> +
> +       if (at91_pm_data.has_l2_cache)
> +               outer_resume();

same remark as for outer_disable()

Jean-Jacques

>  }
>
>  /* We manage both DDRAM/SDRAM controllers, we need more than one value
>  * to
> @@ -353,6 +362,11 @@ static __init void at91_dt_ramc(void)
>                 return;
>         }
>
> +       np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> +       if (np)
> +               at91_pm_data.has_l2_cache = true;
> +       of_node_put(np);
> +
>         at91_pm_set_standby(standby);
>  }
>
>
> This has the following benefits:
>  - everybody will have the fix, regardless of whether the dtb is updated
>  - has_l2_cache can be used later in at91_pm_suspend instead of calling
>    it unconditionnaly (I'll send a patch)
>
>
> On 06/01/2017 at 14:59:45 +0800, Wenyou Yang wrote :
>> For the SoCs such as SAMA5D2 and SAMA5D4 which have L2 cache,
>> flush the L2 cache first before entering the cpu idle.
>>
>> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
>> ---
>>
>>  arch/arm/mach-at91/pm.c       | 19 +++++++++++++++++++
>>  drivers/memory/atmel-sdramc.c |  1 +
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
>> index b4332b727e9c..1a60dede1a01 100644
>> --- a/arch/arm/mach-at91/pm.c
>> +++ b/arch/arm/mach-at91/pm.c
>> @@ -289,6 +289,24 @@ static void at91_ddr_standby(void)
>>               at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
>>  }
>>
>> +static void at91_ddr_cache_standby(void)
>> +{
>> +     u32 saved_lpr;
>> +
>> +     flush_cache_all();
>> +     outer_disable();
>> +
>> +     saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
>> +     at91_ramc_write(0, AT91_DDRSDRC_LPR, (saved_lpr &
>> +                     (~AT91_DDRSDRC_LPCB)) | AT91_DDRSDRC_LPCB_SELF_REFRESH);
>> +
>> +     cpu_do_idle();
>> +
>> +     at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
>> +
>> +     outer_resume();
>> +}
>> +
>>  /* We manage both DDRAM/SDRAM controllers, we need more than one value to
>>   * remember.
>>   */
>> @@ -324,6 +342,7 @@ static const struct of_device_id const ramc_ids[] __initconst = {
>>       { .compatible = "atmel,at91sam9260-sdramc", .data = at91sam9_sdram_standby },
>>       { .compatible = "atmel,at91sam9g45-ddramc", .data = at91_ddr_standby },
>>       { .compatible = "atmel,sama5d3-ddramc", .data = at91_ddr_standby },
>> +     { .compatible = "atmel,sama5d4-ddramc", .data = at91_ddr_cache_standby },
>>       { /*sentinel*/ }
>>  };
>>
>> diff --git a/drivers/memory/atmel-sdramc.c b/drivers/memory/atmel-sdramc.c
>> index b418b39af180..7e5c5c6c1348 100644
>> --- a/drivers/memory/atmel-sdramc.c
>> +++ b/drivers/memory/atmel-sdramc.c
>> @@ -48,6 +48,7 @@ static const struct of_device_id atmel_ramc_of_match[] = {
>>       { .compatible = "atmel,at91sam9260-sdramc", .data = &at91rm9200_caps, },
>>       { .compatible = "atmel,at91sam9g45-ddramc", .data = &at91sam9g45_caps, },
>>       { .compatible = "atmel,sama5d3-ddramc", .data = &sama5d3_caps, },
>> +     { .compatible = "atmel,sama5d4-ddramc", .data = &sama5d3_caps, },
>>       {},
>>  };
>>
>> --
>> 2.11.0
>>
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Alexandre Belloni]

On 10/01/2017 at 17:50:58 +0100, Jean-Jacques Hiblot wrote :
> 2017-01-10 17:18 GMT+01:00 Alexandre Belloni
> <alexandre.belloni@free-electrons.com>:
> > I though a bit more about it, and I don't really like the new compatible
> > string. I don't feel this should be necessary.
> >
> > What about the following:
> >
> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> > index b4332b727e9c..0333aca63e44 100644
> > --- a/arch/arm/mach-at91/pm.c
> > +++ b/arch/arm/mach-at91/pm.c
> > @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);
> >  static struct {
> >         unsigned long uhp_udp_mask;
> >         int memctrl;
> > +       bool has_l2_cache;
> >  } at91_pm_data;
> >
> >  void __iomem *at91_ramc_base[2];
> > @@ -267,6 +268,11 @@ static void at91_ddr_standby(void)
> >         u32 lpr0, lpr1 = 0;
> >         u32 saved_lpr0, saved_lpr1 = 0;
> >
> 
> > +       if (at91_pm_data.has_l2_cache) {
> > +               flush_cache_all();
> what is the point of calling flush_cache_all() here ? Do we really
> care that dirty data in L1 is written to DDR ? I may be missing
> something but to me it's just extra latency.

I agree that this one is the main problem.

> > +               outer_disable();
> It seems to me that if there's no L2 cache, then outer_disable()  is a
> no-op. It could be called unconditionally.

It is not on sama5, it will jump to outer_disable which will at least
save the context and restore it

> > +       }
> > +
> >         if (at91_ramc_base[1]) {
> >                 saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
> >                 lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB;
> > @@ -287,6 +293,9 @@ static void at91_ddr_standby(void)
> >         at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
> >         if (at91_ramc_base[1])
> >                 at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
> > +
> > +       if (at91_pm_data.has_l2_cache)
> > +               outer_resume();
> 
> same remark as for outer_disable()

It is not either but this is a macro and I admit testing has_l2_cache is
superfluous.


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

RE: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Wenyou.Yang]

Hi Jean-Jacques,

> -----Original Message-----
> From: Jean-Jacques Hiblot [mailto:jjhiblot@gmail.com]
> Sent: 2017年1月11日 0:51
> To: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> Cc: Wenyou Yang - A41535 <Wenyou.Yang@microchip.com>; Mark Rutland
> <mark.rutland@arm.com>; devicetree <devicetree@vger.kernel.org>; Russell
> King <linux@arm.linux.org.uk>; Wenyou Yang - A41535
> <Wenyou.Yang@microchip.com>; Nicolas Ferre <nicolas.ferre@atmel.com>;
> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Rob Herring
> <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
> 
> 2017-01-10 17:18 GMT+01:00 Alexandre Belloni
> <alexandre.belloni@free-electrons.com>:
> > I though a bit more about it, and I don't really like the new
> > compatible string. I don't feel this should be necessary.
> >
> > What about the following:
> >
> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> > b4332b727e9c..0333aca63e44 100644
> > --- a/arch/arm/mach-at91/pm.c
> > +++ b/arch/arm/mach-at91/pm.c
> > @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);  static
> > struct {
> >         unsigned long uhp_udp_mask;
> >         int memctrl;
> > +       bool has_l2_cache;
> >  } at91_pm_data;
> >
> >  void __iomem *at91_ramc_base[2];
> > @@ -267,6 +268,11 @@ static void at91_ddr_standby(void)
> >         u32 lpr0, lpr1 = 0;
> >         u32 saved_lpr0, saved_lpr1 = 0;
> >
> 
> > +       if (at91_pm_data.has_l2_cache) {
> > +               flush_cache_all();
> what is the point of calling flush_cache_all() here ? Do we really care that dirty
> data in L1 is written to DDR ? I may be missing something but to me it's just extra
> latency.

Are you mean use outer_flush_all() to flush all cache lines in the outer cache only?

> > +               outer_disable();
> It seems to me that if there's no L2 cache, then outer_disable()  is a no-op. It
> could be called unconditionally.
> > +       }
> > +
> >         if (at91_ramc_base[1]) {
> >                 saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
> >                 lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB; @@ -287,6
> > +293,9 @@ static void at91_ddr_standby(void)
> >         at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
> >         if (at91_ramc_base[1])
> >                 at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
> > +
> > +       if (at91_pm_data.has_l2_cache)
> > +               outer_resume();
> 
> same remark as for outer_disable()
> 
> Jean-Jacques
> 
> >  }
> >
> >  /* We manage both DDRAM/SDRAM controllers, we need more than one
> > value
> >  * to
> > @@ -353,6 +362,11 @@ static __init void at91_dt_ramc(void)
> >                 return;
> >         }
> >
> > +       np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> > +       if (np)
> > +               at91_pm_data.has_l2_cache = true;
> > +       of_node_put(np);
> > +
> >         at91_pm_set_standby(standby);
> >  }
> >
> >
> > This has the following benefits:
> >  - everybody will have the fix, regardless of whether the dtb is
> > updated
> >  - has_l2_cache can be used later in at91_pm_suspend instead of calling
> >    it unconditionnaly (I'll send a patch)
> >
> >
> > On 06/01/2017 at 14:59:45 +0800, Wenyou Yang wrote :
> >> For the SoCs such as SAMA5D2 and SAMA5D4 which have L2 cache, flush
> >> the L2 cache first before entering the cpu idle.
> >>
> >> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> >> ---
> >>
> >>  arch/arm/mach-at91/pm.c       | 19 +++++++++++++++++++
> >>  drivers/memory/atmel-sdramc.c |  1 +
> >>  2 files changed, 20 insertions(+)
> >>
> >> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> >> b4332b727e9c..1a60dede1a01 100644
> >> --- a/arch/arm/mach-at91/pm.c
> >> +++ b/arch/arm/mach-at91/pm.c
> >> @@ -289,6 +289,24 @@ static void at91_ddr_standby(void)
> >>               at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);  }
> >>
> >> +static void at91_ddr_cache_standby(void) {
> >> +     u32 saved_lpr;
> >> +
> >> +     flush_cache_all();
> >> +     outer_disable();
> >> +
> >> +     saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
> >> +     at91_ramc_write(0, AT91_DDRSDRC_LPR, (saved_lpr &
> >> +                     (~AT91_DDRSDRC_LPCB)) |
> >> + AT91_DDRSDRC_LPCB_SELF_REFRESH);
> >> +
> >> +     cpu_do_idle();
> >> +
> >> +     at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
> >> +
> >> +     outer_resume();
> >> +}
> >> +
> >>  /* We manage both DDRAM/SDRAM controllers, we need more than one
> value to
> >>   * remember.
> >>   */
> >> @@ -324,6 +342,7 @@ static const struct of_device_id const ramc_ids[]
> __initconst = {
> >>       { .compatible = "atmel,at91sam9260-sdramc", .data =
> at91sam9_sdram_standby },
> >>       { .compatible = "atmel,at91sam9g45-ddramc", .data = at91_ddr_standby },
> >>       { .compatible = "atmel,sama5d3-ddramc", .data =
> >> at91_ddr_standby },
> >> +     { .compatible = "atmel,sama5d4-ddramc", .data =
> >> + at91_ddr_cache_standby },
> >>       { /*sentinel*/ }
> >>  };
> >>
> >> diff --git a/drivers/memory/atmel-sdramc.c
> >> b/drivers/memory/atmel-sdramc.c index b418b39af180..7e5c5c6c1348
> >> 100644
> >> --- a/drivers/memory/atmel-sdramc.c
> >> +++ b/drivers/memory/atmel-sdramc.c
> >> @@ -48,6 +48,7 @@ static const struct of_device_id atmel_ramc_of_match[]
> = {
> >>       { .compatible = "atmel,at91sam9260-sdramc", .data =
> &at91rm9200_caps, },
> >>       { .compatible = "atmel,at91sam9g45-ddramc", .data =
> &at91sam9g45_caps, },
> >>       { .compatible = "atmel,sama5d3-ddramc", .data = &sama5d3_caps,
> >> },
> >> +     { .compatible = "atmel,sama5d4-ddramc", .data = &sama5d3_caps,
> >> + },
> >>       {},
> >>  };
> >>
> >> --
> >> 2.11.0
> >>
> >
> > --
> > Alexandre Belloni, Free Electrons
> > Embedded Linux and Kernel engineering
> > http://free-electrons.com
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Jean-Jacques Hiblot]

2017-01-11 9:15 GMT+01:00  <Wenyou.Yang@microchip.com>:
> Hi Jean-Jacques,
>
>> -----Original Message-----
>> From: Jean-Jacques Hiblot [mailto:jjhiblot@gmail.com]
>> Sent: 2017年1月11日 0:51
>> To: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>> Cc: Wenyou Yang - A41535 <Wenyou.Yang@microchip.com>; Mark Rutland
>> <mark.rutland@arm.com>; devicetree <devicetree@vger.kernel.org>; Russell
>> King <linux@arm.linux.org.uk>; Wenyou Yang - A41535
>> <Wenyou.Yang@microchip.com>; Nicolas Ferre <nicolas.ferre@atmel.com>;
>> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Rob Herring
>> <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org
>> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
>>
>> 2017-01-10 17:18 GMT+01:00 Alexandre Belloni
>> <alexandre.belloni@free-electrons.com>:
>> > I though a bit more about it, and I don't really like the new
>> > compatible string. I don't feel this should be necessary.
>> >
>> > What about the following:
>> >
>> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
>> > b4332b727e9c..0333aca63e44 100644
>> > --- a/arch/arm/mach-at91/pm.c
>> > +++ b/arch/arm/mach-at91/pm.c
>> > @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);  static
>> > struct {
>> >         unsigned long uhp_udp_mask;
>> >         int memctrl;
>> > +       bool has_l2_cache;
>> >  } at91_pm_data;
>> >
>> >  void __iomem *at91_ramc_base[2];
>> > @@ -267,6 +268,11 @@ static void at91_ddr_standby(void)
>> >         u32 lpr0, lpr1 = 0;
>> >         u32 saved_lpr0, saved_lpr1 = 0;
>> >
>>
>> > +       if (at91_pm_data.has_l2_cache) {
>> > +               flush_cache_all();
>> what is the point of calling flush_cache_all() here ? Do we really care that dirty
>> data in L1 is written to DDR ? I may be missing something but to me it's just extra
>> latency.
>
> Are you mean use outer_flush_all() to flush all cache lines in the outer cache only?

Yes that's what I meant. You see, you don't flush the cache for
sama5d3 so it shouldn't be required either for sam5d4. You should be
able to test it quickly and see if L1 flush is indeed required by
replacing  flush_cache_all() with outer_flush_all(). BTW is highly
probable that L2 cache flush is done in outer_disable() so calling
outer_flush_all() is probably no required.

However the more I think on it, the more I wonder about the reason why
L2 flushing is required or to put it differently: is flusing the L2
cache the correct thing to do or just a workaround ?
Could it be that L2 is doing some maintenance operation when DDR
enters self refresh? In that case maybe a simple cache sync could be
used.

>
>> > +               outer_disable();
>> It seems to me that if there's no L2 cache, then outer_disable()  is a no-op. It
>> could be called unconditionally.
>> > +       }
>> > +
>> >         if (at91_ramc_base[1]) {
>> >                 saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
>> >                 lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB; @@ -287,6
>> > +293,9 @@ static void at91_ddr_standby(void)
>> >         at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
>> >         if (at91_ramc_base[1])
>> >                 at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
>> > +
>> > +       if (at91_pm_data.has_l2_cache)
>> > +               outer_resume();
>>
>> same remark as for outer_disable()
>>
>> Jean-Jacques
>>
>> >  }
>> >
>> >  /* We manage both DDRAM/SDRAM controllers, we need more than one
>> > value
>> >  * to
>> > @@ -353,6 +362,11 @@ static __init void at91_dt_ramc(void)
>> >                 return;
>> >         }
>> >
>> > +       np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
>> > +       if (np)
>> > +               at91_pm_data.has_l2_cache = true;
>> > +       of_node_put(np);
>> > +
>> >         at91_pm_set_standby(standby);
>> >  }
>> >
>> >
>> > This has the following benefits:
>> >  - everybody will have the fix, regardless of whether the dtb is
>> > updated
>> >  - has_l2_cache can be used later in at91_pm_suspend instead of calling
>> >    it unconditionnaly (I'll send a patch)
>> >
>> >
>> > On 06/01/2017 at 14:59:45 +0800, Wenyou Yang wrote :
>> >> For the SoCs such as SAMA5D2 and SAMA5D4 which have L2 cache, flush
>> >> the L2 cache first before entering the cpu idle.
>> >>
>> >> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
>> >> ---
>> >>
>> >>  arch/arm/mach-at91/pm.c       | 19 +++++++++++++++++++
>> >>  drivers/memory/atmel-sdramc.c |  1 +
>> >>  2 files changed, 20 insertions(+)
>> >>
>> >> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
>> >> b4332b727e9c..1a60dede1a01 100644
>> >> --- a/arch/arm/mach-at91/pm.c
>> >> +++ b/arch/arm/mach-at91/pm.c
>> >> @@ -289,6 +289,24 @@ static void at91_ddr_standby(void)
>> >>               at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);  }
>> >>
>> >> +static void at91_ddr_cache_standby(void) {
>> >> +     u32 saved_lpr;
>> >> +
>> >> +     flush_cache_all();
>> >> +     outer_disable();
>> >> +
>> >> +     saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
>> >> +     at91_ramc_write(0, AT91_DDRSDRC_LPR, (saved_lpr &
>> >> +                     (~AT91_DDRSDRC_LPCB)) |
>> >> + AT91_DDRSDRC_LPCB_SELF_REFRESH);
>> >> +
>> >> +     cpu_do_idle();
>> >> +
>> >> +     at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
>> >> +
>> >> +     outer_resume();
>> >> +}
>> >> +
>> >>  /* We manage both DDRAM/SDRAM controllers, we need more than one
>> value to
>> >>   * remember.
>> >>   */
>> >> @@ -324,6 +342,7 @@ static const struct of_device_id const ramc_ids[]
>> __initconst = {
>> >>       { .compatible = "atmel,at91sam9260-sdramc", .data =
>> at91sam9_sdram_standby },
>> >>       { .compatible = "atmel,at91sam9g45-ddramc", .data = at91_ddr_standby },
>> >>       { .compatible = "atmel,sama5d3-ddramc", .data =
>> >> at91_ddr_standby },
>> >> +     { .compatible = "atmel,sama5d4-ddramc", .data =
>> >> + at91_ddr_cache_standby },
>> >>       { /*sentinel*/ }
>> >>  };
>> >>
>> >> diff --git a/drivers/memory/atmel-sdramc.c
>> >> b/drivers/memory/atmel-sdramc.c index b418b39af180..7e5c5c6c1348
>> >> 100644
>> >> --- a/drivers/memory/atmel-sdramc.c
>> >> +++ b/drivers/memory/atmel-sdramc.c
>> >> @@ -48,6 +48,7 @@ static const struct of_device_id atmel_ramc_of_match[]
>> = {
>> >>       { .compatible = "atmel,at91sam9260-sdramc", .data =
>> &at91rm9200_caps, },
>> >>       { .compatible = "atmel,at91sam9g45-ddramc", .data =
>> &at91sam9g45_caps, },
>> >>       { .compatible = "atmel,sama5d3-ddramc", .data = &sama5d3_caps,
>> >> },
>> >> +     { .compatible = "atmel,sama5d4-ddramc", .data = &sama5d3_caps,
>> >> + },
>> >>       {},
>> >>  };
>> >>
>> >> --
>> >> 2.11.0
>> >>
>> >
>> > --
>> > Alexandre Belloni, Free Electrons
>> > Embedded Linux and Kernel engineering
>> > http://free-electrons.com
>> >
>> > _______________________________________________
>> > linux-arm-kernel mailing list
>> > linux-arm-kernel@lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Russell King - ARM Linux]

On Wed, Jan 11, 2017 at 12:05:05PM +0100, Jean-Jacques Hiblot wrote:
> 2017-01-11 9:15 GMT+01:00  <Wenyou.Yang@microchip.com>:
> > Hi Jean-Jacques,
> >
> >> -----Original Message-----
> >> From: Jean-Jacques Hiblot [mailto:jjhiblot@gmail.com]
> >> Sent: 2017年1月11日 0:51
> >> To: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> >> Cc: Wenyou Yang - A41535 <Wenyou.Yang@microchip.com>; Mark Rutland
> >> <mark.rutland@arm.com>; devicetree <devicetree@vger.kernel.org>; Russell
> >> King <linux@arm.linux.org.uk>; Wenyou Yang - A41535
> >> <Wenyou.Yang@microchip.com>; Nicolas Ferre <nicolas.ferre@atmel.com>;
> >> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Rob Herring
> >> <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org
> >> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
> >>
> >> 2017-01-10 17:18 GMT+01:00 Alexandre Belloni
> >> <alexandre.belloni@free-electrons.com>:
> >> > I though a bit more about it, and I don't really like the new
> >> > compatible string. I don't feel this should be necessary.
> >> >
> >> > What about the following:
> >> >
> >> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> >> > b4332b727e9c..0333aca63e44 100644
> >> > --- a/arch/arm/mach-at91/pm.c
> >> > +++ b/arch/arm/mach-at91/pm.c
> >> > @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);  static
> >> > struct {
> >> >         unsigned long uhp_udp_mask;
> >> >         int memctrl;
> >> > +       bool has_l2_cache;
> >> >  } at91_pm_data;
> >> >
> >> >  void __iomem *at91_ramc_base[2];
> >> > @@ -267,6 +268,11 @@ static void at91_ddr_standby(void)
> >> >         u32 lpr0, lpr1 = 0;
> >> >         u32 saved_lpr0, saved_lpr1 = 0;
> >> >
> >>
> >> > +       if (at91_pm_data.has_l2_cache) {
> >> > +               flush_cache_all();
> >> what is the point of calling flush_cache_all() here ? Do we really care that dirty
> >> data in L1 is written to DDR ? I may be missing something but to me it's just extra
> >> latency.
> >
> > Are you mean use outer_flush_all() to flush all cache lines in the outer cache only?
> 
> Yes that's what I meant. You see, you don't flush the cache for
> sama5d3 so it shouldn't be required either for sam5d4. You should be
> able to test it quickly and see if L1 flush is indeed required by
> replacing  flush_cache_all() with outer_flush_all(). BTW is highly
> probable that L2 cache flush is done in outer_disable() so calling
> outer_flush_all() is probably no required.

Please don't.  Read the comments in the code, and understand the APIs
that you're suggesting people use _before_ making the suggestion:

/**
 * outer_flush_all - clean and invalidate all cache lines in the outer cache
 *
 * Note: depending on implementation, this may not be atomic - it must
 * only be called with interrupts disabled and no other active outer
 * cache masters.
 *
 * It is intended that this function is only used by implementations
 * needing to override the outer_cache.disable() method due to security.
 * (Some implementations perform this as a clean followed by an invalidate.)
 */

So, outer_flush_all() should not be called except from L2 cache code
implementing the outer_disable() function - it's not intended for
platforms to use.

There are, however, sadly three users of outer_flush_all() which have
crept in through arm-soc, that should be outer_disable() instead.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Jean-Jacques Hiblot]

2017-01-11 12:18 GMT+01:00 Russell King - ARM Linux <linux@armlinux.org.uk>:
> On Wed, Jan 11, 2017 at 12:05:05PM +0100, Jean-Jacques Hiblot wrote:
>> 2017-01-11 9:15 GMT+01:00  <Wenyou.Yang@microchip.com>:
>> > Hi Jean-Jacques,
>> >
>> >> -----Original Message-----
>> >> From: Jean-Jacques Hiblot [mailto:jjhiblot@gmail.com]
>> >> Sent: 2017年1月11日 0:51
>> >> To: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>> >> Cc: Wenyou Yang - A41535 <Wenyou.Yang@microchip.com>; Mark Rutland
>> >> <mark.rutland@arm.com>; devicetree <devicetree@vger.kernel.org>; Russell
>> >> King <linux@arm.linux.org.uk>; Wenyou Yang - A41535
>> >> <Wenyou.Yang@microchip.com>; Nicolas Ferre <nicolas.ferre@atmel.com>;
>> >> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Rob Herring
>> >> <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org
>> >> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
>> >>
>> >> 2017-01-10 17:18 GMT+01:00 Alexandre Belloni
>> >> <alexandre.belloni@free-electrons.com>:
>> >> > I though a bit more about it, and I don't really like the new
>> >> > compatible string. I don't feel this should be necessary.
>> >> >
>> >> > What about the following:
>> >> >
>> >> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
>> >> > b4332b727e9c..0333aca63e44 100644
>> >> > --- a/arch/arm/mach-at91/pm.c
>> >> > +++ b/arch/arm/mach-at91/pm.c
>> >> > @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);  static
>> >> > struct {
>> >> >         unsigned long uhp_udp_mask;
>> >> >         int memctrl;
>> >> > +       bool has_l2_cache;
>> >> >  } at91_pm_data;
>> >> >
>> >> >  void __iomem *at91_ramc_base[2];
>> >> > @@ -267,6 +268,11 @@ static void at91_ddr_standby(void)
>> >> >         u32 lpr0, lpr1 = 0;
>> >> >         u32 saved_lpr0, saved_lpr1 = 0;
>> >> >
>> >>
>> >> > +       if (at91_pm_data.has_l2_cache) {
>> >> > +               flush_cache_all();
>> >> what is the point of calling flush_cache_all() here ? Do we really care that dirty
>> >> data in L1 is written to DDR ? I may be missing something but to me it's just extra
>> >> latency.
>> >
>> > Are you mean use outer_flush_all() to flush all cache lines in the outer cache only?
>>
>> Yes that's what I meant. You see, you don't flush the cache for
>> sama5d3 so it shouldn't be required either for sam5d4. You should be
>> able to test it quickly and see if L1 flush is indeed required by
>> replacing  flush_cache_all() with outer_flush_all(). BTW is highly
>> probable that L2 cache flush is done in outer_disable() so calling
>> outer_flush_all() is probably no required.
>
> Please don't.  Read the comments in the code, and understand the APIs
> that you're suggesting people use _before_ making the suggestion:
>
> /**
>  * outer_flush_all - clean and invalidate all cache lines in the outer cache
>  *
>  * Note: depending on implementation, this may not be atomic - it must
>  * only be called with interrupts disabled and no other active outer
>  * cache masters.
>  *
>  * It is intended that this function is only used by implementations
>  * needing to override the outer_cache.disable() method due to security.
>  * (Some implementations perform this as a clean followed by an invalidate.)
>  */
>
> So, outer_flush_all() should not be called except from L2 cache code
> implementing the outer_disable() function - it's not intended for
> platforms to use.

OK. My bad. I didn't understand the comments.

>
> There are, however, sadly three users of outer_flush_all() which have
> crept in through arm-soc, that should be outer_disable() instead.
>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

RE: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Wenyou.Yang]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@armlinux.org.uk]
> Sent: 2017年1月11日 19:18
> To: Jean-Jacques Hiblot <jjhiblot@gmail.com>
> Cc: Wenyou Yang - A41535 <Wenyou.Yang@microchip.com>; Alexandre Belloni
> <alexandre.belloni@free-electrons.com>; Mark Rutland <mark.rutland@arm.com>;
> devicetree <devicetree@vger.kernel.org>; Nicolas Ferre
> <nicolas.ferre@atmel.com>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; robh+dt <robh+dt@kernel.org>; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
> 
> On Wed, Jan 11, 2017 at 12:05:05PM +0100, Jean-Jacques Hiblot wrote:
> > 2017-01-11 9:15 GMT+01:00  <Wenyou.Yang@microchip.com>:
> > > Hi Jean-Jacques,
> > >
> > >> -----Original Message-----
> > >> From: Jean-Jacques Hiblot [mailto:jjhiblot@gmail.com]
> > >> Sent: 2017年1月11日 0:51
> > >> To: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > >> Cc: Wenyou Yang - A41535 <Wenyou.Yang@microchip.com>; Mark Rutland
> > >> <mark.rutland@arm.com>; devicetree <devicetree@vger.kernel.org>;
> > >> Russell King <linux@arm.linux.org.uk>; Wenyou Yang - A41535
> > >> <Wenyou.Yang@microchip.com>; Nicolas Ferre
> > >> <nicolas.ferre@atmel.com>; Linux Kernel Mailing List
> > >> <linux-kernel@vger.kernel.org>; Rob Herring <robh+dt@kernel.org>;
> > >> linux-arm-kernel@lists.infradead.org
> > >> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before
> > >> entering cpu idle
> > >>
> > >> 2017-01-10 17:18 GMT+01:00 Alexandre Belloni
> > >> <alexandre.belloni@free-electrons.com>:
> > >> > I though a bit more about it, and I don't really like the new
> > >> > compatible string. I don't feel this should be necessary.
> > >> >
> > >> > What about the following:
> > >> >
> > >> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> > >> > index
> > >> > b4332b727e9c..0333aca63e44 100644
> > >> > --- a/arch/arm/mach-at91/pm.c
> > >> > +++ b/arch/arm/mach-at91/pm.c
> > >> > @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);
> > >> > static struct {
> > >> >         unsigned long uhp_udp_mask;
> > >> >         int memctrl;
> > >> > +       bool has_l2_cache;
> > >> >  } at91_pm_data;
> > >> >
> > >> >  void __iomem *at91_ramc_base[2]; @@ -267,6 +268,11 @@ static
> > >> > void at91_ddr_standby(void)
> > >> >         u32 lpr0, lpr1 = 0;
> > >> >         u32 saved_lpr0, saved_lpr1 = 0;
> > >> >
> > >>
> > >> > +       if (at91_pm_data.has_l2_cache) {
> > >> > +               flush_cache_all();
> > >> what is the point of calling flush_cache_all() here ? Do we really
> > >> care that dirty data in L1 is written to DDR ? I may be missing
> > >> something but to me it's just extra latency.
> > >
> > > Are you mean use outer_flush_all() to flush all cache lines in the outer cache
> only?
> >
> > Yes that's what I meant. You see, you don't flush the cache for
> > sama5d3 so it shouldn't be required either for sam5d4. You should be
> > able to test it quickly and see if L1 flush is indeed required by
> > replacing  flush_cache_all() with outer_flush_all(). BTW is highly
> > probable that L2 cache flush is done in outer_disable() so calling
> > outer_flush_all() is probably no required.
> 
> Please don't.  Read the comments in the code, and understand the APIs that
> you're suggesting people use _before_ making the suggestion:
> 
> /**
>  * outer_flush_all - clean and invalidate all cache lines in the outer cache
>  *
>  * Note: depending on implementation, this may not be atomic - it must
>  * only be called with interrupts disabled and no other active outer
>  * cache masters.
>  *
>  * It is intended that this function is only used by implementations
>  * needing to override the outer_cache.disable() method due to security.
>  * (Some implementations perform this as a clean followed by an invalidate.)  */
> 
> So, outer_flush_all() should not be called except from L2 cache code
> implementing the outer_disable() function - it's not intended for platforms to use.
> 
> There are, however, sadly three users of outer_flush_all() which have crept in
> through arm-soc, that should be outer_disable() instead.

Here, outer_flush_all() should not be called, calling outer_disable() is enough. Is it right?

In the implementation of l2c_disable(void) of in mm/cache-l2x0.c, the outer_cache.flush_all() is called.

> 
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.


Best Regards,
Wenyou Yang