linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jean-Jacques Hiblot <jjhiblot@gmail.com>
To: Russell King - ARM Linux <linux@armlinux.org.uk>
Cc: Wenyou Yang <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" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle
Date: Wed, 11 Jan 2017 14:18:48 +0100	[thread overview]
Message-ID: <CACh+v5OEKZzLePwoo6wSf3ccA4e_wwV-FX0v+=vS2JTAK8ih5g@mail.gmail.com> (raw)
In-Reply-To: <20170111111814.GJ14217@n2100.armlinux.org.uk>

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.

  reply	other threads:[~2017-01-11 13:20 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-06  6:59 [PATCH 0/3] ARM: at91: fix cpuidle crash on SAMA5D4 Xplained board Wenyou Yang
2017-01-06  6:59 ` [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle Wenyou Yang
2017-01-06  9:05   ` Alexandre Belloni
2017-01-09  1:47     ` Wenyou.Yang
2017-01-10 16:18   ` Alexandre Belloni
2017-01-10 16:30     ` Alexandre Belloni
2017-01-10 16:50     ` Jean-Jacques Hiblot
2017-01-10 17:22       ` Alexandre Belloni
2017-01-11  8:15       ` Wenyou.Yang
2017-01-11 11:05         ` Jean-Jacques Hiblot
2017-01-11 11:18           ` Russell King - ARM Linux
2017-01-11 13:18             ` Jean-Jacques Hiblot [this message]
2017-01-12  1:25             ` Wenyou.Yang
2017-01-06  6:59 ` [PATCH 2/3] doc: binding: add new compatible for SDRAM/DDR Controller Wenyou Yang
2017-01-09 18:45   ` Rob Herring
2017-01-06  6:59 ` [PATCH 3/3] ARM: dts: at91: use "atmel,sama5d4-ddramc" for ramc Wenyou Yang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CACh+v5OEKZzLePwoo6wSf3ccA4e_wwV-FX0v+=vS2JTAK8ih5g@mail.gmail.com' \
    --to=jjhiblot@gmail.com \
    --cc=Wenyou.Yang@microchip.com \
    --cc=alexandre.belloni@free-electrons.com \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=mark.rutland@arm.com \
    --cc=nicolas.ferre@atmel.com \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).