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

[Jean-Jacques Hiblot]

2017-01-11 9:15 GMT+01:00  <Wenyou.Yang@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.

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@xxxxxxxxx>
>> >> ---
>> >>
>> >>  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@xxxxxxxxxxxxxxxxxxx
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel