| Message ID | 20170110161821.vp673jyfqx6s76pg@piout.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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
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.
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.
> -----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
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