2017-01-11 12:18 GMT+01:00 Russell King - ARM Linux <linux@xxxxxxxxxxxxxxx>: > On Wed, Jan 11, 2017 at 12:05:05PM +0100, Jean-Jacques Hiblot wrote: >> 2017-01-11 9:15 GMT+01:00 <Wenyou.Yang@xxxxxxxxxxxxx>: >> > Hi Jean-Jacques, >> > >> >> -----Original Message----- >> >> From: Jean-Jacques Hiblot [mailto:jjhiblot@xxxxxxxxx] >> >> Sent: 2017年1月11日 0:51 >> >> To: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxxxxxxxxx> >> >> Cc: Wenyou Yang - A41535 <Wenyou.Yang@xxxxxxxxxxxxx>; Mark Rutland >> >> <mark.rutland@xxxxxxx>; devicetree <devicetree@xxxxxxxxxxxxxxx>; Russell >> >> King <linux@xxxxxxxxxxxxxxxx>; Wenyou Yang - A41535 >> >> <Wenyou.Yang@xxxxxxxxxxxxx>; Nicolas Ferre <nicolas.ferre@xxxxxxxxx>; >> >> Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; Rob Herring >> >> <robh+dt@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx >> >> 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@xxxxxxxxxxxxxxxxxx>: >> >> > 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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
