* [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-10 16:18 ` Alexandre Belloni
0 siblings, 0 replies; 47+ messages in thread
From: Alexandre Belloni @ 2017-01-10 16:18 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-10 16:18 ` Alexandre Belloni
0 siblings, 0 replies; 47+ messages in thread
From: Alexandre Belloni @ 2017-01-10 16:18 UTC (permalink / raw)
To: Wenyou Yang
Cc: Russell King, Nicolas Ferre, Rob Herring, Mark Rutland,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Wenyou Yang,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
2017-01-10 16:18 ` Alexandre Belloni
(?)
@ 2017-01-10 16:30 ` Alexandre Belloni
-1 siblings, 0 replies; 47+ messages in thread
From: Alexandre Belloni @ 2017-01-10 16:30 UTC (permalink / raw)
To: Wenyou Yang
Cc: Russell King, Nicolas Ferre, Rob Herring, Mark Rutland,
linux-kernel, Wenyou Yang, devicetree, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 47+ messages in thread* [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-10 16:30 ` Alexandre Belloni
0 siblings, 0 replies; 47+ messages in thread
From: Alexandre Belloni @ 2017-01-10 16:30 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-10 16:30 ` Alexandre Belloni
0 siblings, 0 replies; 47+ messages in thread
From: Alexandre Belloni @ 2017-01-10 16:30 UTC (permalink / raw)
To: Wenyou Yang
Cc: Mark Rutland, devicetree, Russell King, Wenyou Yang,
Nicolas Ferre, linux-kernel, Rob Herring, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-10 16:50 ` Jean-Jacques Hiblot
0 siblings, 0 replies; 47+ messages in thread
From: Jean-Jacques Hiblot @ 2017-01-10 16:50 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Wenyou Yang, Mark Rutland, devicetree, Russell King, Wenyou Yang,
Nicolas Ferre, Linux Kernel Mailing List, Rob Herring,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 47+ messages in thread* [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-10 16:50 ` Jean-Jacques Hiblot
0 siblings, 0 replies; 47+ messages in thread
From: Jean-Jacques Hiblot @ 2017-01-10 16:50 UTC (permalink / raw)
To: linux-arm-kernel
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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-10 16:50 ` Jean-Jacques Hiblot
0 siblings, 0 replies; 47+ messages in thread
From: Jean-Jacques Hiblot @ 2017-01-10 16:50 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Wenyou Yang, Mark Rutland, devicetree, Russell King, Wenyou Yang,
Nicolas Ferre, Linux Kernel Mailing List, Rob Herring,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
2017-01-10 17:18 GMT+01:00 Alexandre Belloni
<alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> 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-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> ---
>>
>> 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-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
2017-01-10 16:50 ` Jean-Jacques Hiblot
(?)
@ 2017-01-10 17:22 ` Alexandre Belloni
-1 siblings, 0 replies; 47+ messages in thread
From: Alexandre Belloni @ 2017-01-10 17:22 UTC (permalink / raw)
To: Jean-Jacques Hiblot
Cc: Wenyou Yang, Mark Rutland, devicetree, Russell King, Wenyou Yang,
Nicolas Ferre, Linux Kernel Mailing List, Rob Herring,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 47+ messages in thread* [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-10 17:22 ` Alexandre Belloni
0 siblings, 0 replies; 47+ messages in thread
From: Alexandre Belloni @ 2017-01-10 17:22 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-10 17:22 ` Alexandre Belloni
0 siblings, 0 replies; 47+ messages in thread
From: Alexandre Belloni @ 2017-01-10 17:22 UTC (permalink / raw)
To: Jean-Jacques Hiblot
Cc: Wenyou Yang, Mark Rutland, devicetree, Russell King, Wenyou Yang,
Nicolas Ferre, Linux Kernel Mailing List, Rob Herring,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
2017-01-10 16:50 ` Jean-Jacques Hiblot
(?)
@ 2017-01-11 8:15 ` Wenyou.Yang
-1 siblings, 0 replies; 47+ messages in thread
From: Wenyou.Yang @ 2017-01-11 8:15 UTC (permalink / raw)
To: jjhiblot, alexandre.belloni
Cc: mark.rutland, devicetree, linux, nicolas.ferre, linux-kernel,
robh+dt, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 47+ messages in thread* [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-11 8:15 ` Wenyou.Yang
0 siblings, 0 replies; 47+ messages in thread
From: Wenyou.Yang at microchip.com @ 2017-01-11 8:15 UTC (permalink / raw)
To: linux-arm-kernel
Hi Jean-Jacques,
> -----Original Message-----
> From: Jean-Jacques Hiblot [mailto:jjhiblot at 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 at 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 at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-11 8:15 ` Wenyou.Yang
0 siblings, 0 replies; 47+ messages in thread
From: Wenyou.Yang @ 2017-01-11 8:15 UTC (permalink / raw)
To: jjhiblot, alexandre.belloni
Cc: mark.rutland, devicetree, linux, nicolas.ferre, linux-kernel,
robh+dt, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
2017-01-11 8:15 ` Wenyou.Yang
(?)
@ 2017-01-11 11:05 ` Jean-Jacques Hiblot
-1 siblings, 0 replies; 47+ messages in thread
From: Jean-Jacques Hiblot @ 2017-01-11 11:05 UTC (permalink / raw)
To: Wenyou Yang
Cc: Alexandre Belloni, Mark Rutland, devicetree,
Russell King - ARM Linux, Nicolas Ferre,
Linux Kernel Mailing List, robh+dt, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 47+ messages in thread* [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-11 11:05 ` Jean-Jacques Hiblot
0 siblings, 0 replies; 47+ messages in thread
From: Jean-Jacques Hiblot @ 2017-01-11 11:05 UTC (permalink / raw)
To: linux-arm-kernel
2017-01-11 9:15 GMT+01:00 <Wenyou.Yang@microchip.com>:
> Hi Jean-Jacques,
>
>> -----Original Message-----
>> From: Jean-Jacques Hiblot [mailto:jjhiblot at 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 at 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 at lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-11 11:05 ` Jean-Jacques Hiblot
0 siblings, 0 replies; 47+ messages in thread
From: Jean-Jacques Hiblot @ 2017-01-11 11:05 UTC (permalink / raw)
To: Wenyou Yang
Cc: Alexandre Belloni, Mark Rutland, devicetree,
Russell King - ARM Linux, Nicolas Ferre,
Linux Kernel Mailing List, robh+dt, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
2017-01-11 11:05 ` Jean-Jacques Hiblot
(?)
@ 2017-01-11 11:18 ` Russell King - ARM Linux
-1 siblings, 0 replies; 47+ messages in thread
From: Russell King - ARM Linux @ 2017-01-11 11:18 UTC (permalink / raw)
To: Jean-Jacques Hiblot
Cc: Wenyou Yang, Alexandre Belloni, Mark Rutland, devicetree,
Nicolas Ferre, Linux Kernel Mailing List, robh+dt,
linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 47+ messages in thread* [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-11 11:18 ` Russell King - ARM Linux
0 siblings, 0 replies; 47+ messages in thread
From: Russell King - ARM Linux @ 2017-01-11 11:18 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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 at 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.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-11 11:18 ` Russell King - ARM Linux
0 siblings, 0 replies; 47+ messages in thread
From: Russell King - ARM Linux @ 2017-01-11 11:18 UTC (permalink / raw)
To: Jean-Jacques Hiblot
Cc: Wenyou Yang, Alexandre Belloni, Mark Rutland, devicetree,
Nicolas Ferre, Linux Kernel Mailing List, robh+dt,
linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
2017-01-11 11:18 ` Russell King - ARM Linux
(?)
@ 2017-01-11 13:18 ` Jean-Jacques Hiblot
-1 siblings, 0 replies; 47+ messages in thread
From: Jean-Jacques Hiblot @ 2017-01-11 13:18 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Wenyou Yang, Alexandre Belloni, Mark Rutland, devicetree,
Nicolas Ferre, Linux Kernel Mailing List, robh+dt,
linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 47+ messages in thread* [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-11 13:18 ` Jean-Jacques Hiblot
0 siblings, 0 replies; 47+ messages in thread
From: Jean-Jacques Hiblot @ 2017-01-11 13:18 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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 at 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.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-11 13:18 ` Jean-Jacques Hiblot
0 siblings, 0 replies; 47+ messages in thread
From: Jean-Jacques Hiblot @ 2017-01-11 13:18 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Mark Rutland, devicetree, Wenyou Yang, Nicolas Ferre,
Linux Kernel Mailing List, robh+dt, Alexandre Belloni,
linux-arm-kernel
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-12 1:25 ` Wenyou.Yang-UWL1GkI3JZL3oGB3hsPCZA
0 siblings, 0 replies; 47+ messages in thread
From: Wenyou.Yang @ 2017-01-12 1:25 UTC (permalink / raw)
To: linux, jjhiblot
Cc: alexandre.belloni, mark.rutland, devicetree, nicolas.ferre,
linux-kernel, robh+dt, linux-arm-kernel
> -----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
^ permalink raw reply [flat|nested] 47+ messages in thread* [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-12 1:25 ` Wenyou.Yang-UWL1GkI3JZL3oGB3hsPCZA
0 siblings, 0 replies; 47+ messages in thread
From: Wenyou.Yang at microchip.com @ 2017-01-12 1:25 UTC (permalink / raw)
To: linux-arm-kernel
> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at 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 at vger.kernel.org>; robh+dt <robh+dt@kernel.org>; linux-arm-
> kernel at 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 at 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 at 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
^ permalink raw reply [flat|nested] 47+ messages in thread* RE: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
@ 2017-01-12 1:25 ` Wenyou.Yang-UWL1GkI3JZL3oGB3hsPCZA
0 siblings, 0 replies; 47+ messages in thread
From: Wenyou.Yang-UWL1GkI3JZL3oGB3hsPCZA @ 2017-01-12 1:25 UTC (permalink / raw)
To: linux-I+IVW8TIWO2tmTQ+vhA3Yw, jjhiblot-Re5JQEeQqe8AvxtiuMwx3w
Cc: alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA,
nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4856 bytes --]
> -----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
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i
^ permalink raw reply [flat|nested] 47+ messages in thread