From mboxrd@z Thu Jan  1 00:00:00 1970
From: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
Date: Tue, 19 Feb 2019 16:55:15 +0100
Subject: [U-Boot] [PATCH] ARM: socfpga: Clear PL310 early in SPL
In-Reply-To: <60481271-afb7-fe89-45ff-595ec2a372a6@denx.de>
References: <20190219125240.2833-1-marex@denx.de>
 <CAAh8qsyWQhycVFZYx5K7MEMgju9RcvOgMYXe_9jSGQc2TvwkSQ@mail.gmail.com>
 <0ed007f6-c31b-1e58-d1f5-e2ef798e2ecb@denx.de>
 <CAAh8qsxBWTiq-LgL9X5LkMqPsXB1n5LCafGYReQ9R1uCQ9ENtg@mail.gmail.com>
 <60481271-afb7-fe89-45ff-595ec2a372a6@denx.de>
Message-ID: <2cc446bd-67e1-e69d-44cf-d7d7d0594c01@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Am 19.02.2019 um 15:00 schrieb Marek Vasut:
> On 2/19/19 2:58 PM, Simon Goldschmidt wrote:
>>
>>
>> Am Di., 19. Feb. 2019, 14:45 hat Marek Vasut <marex@denx.de
>> <mailto:marex@denx.de>> geschrieben:
>>
>>      On 2/19/19 2:28 PM, Simon Goldschmidt wrote:
>>      > On Tue, Feb 19, 2019 at 1:53 PM Marek Vasut <marex@denx.de
>>      <mailto:marex@denx.de>> wrote:
>>      >>
>>      >> On SoCFPGA Gen5 systems, it can rarely happen that a reboot from
>>      Linux
>>      >> will result in stale data in PL310 L2 cache controller. Even if
>>      the L2
>>      >> cache controller is disabled via the CTRL register CTRL_EN bit, those
>>      >> data can interfere with operation of devices using DMA, like e.g. the
>>      >> DWMMC controller. This can in turn cause e.g. SPL to fail reading
>>      data
>>      >> from SD/MMC.
>>      >>
>>      >> The obvious solution here would be to fully reset the L2 cache
>>      controller
>>      >> via the reset manager MPUMODRST L2 bit, however this causes bus
>>      hang even
>>      >> if executed entirely from L1 I-cache to avoid generating any bus
>>      traffic
>>      >> through the L2 cache controller.
>>      >>
>>      >> This patch thus configures and enables the L2 cache controller
>>      very early
>>      >> in the SPL boot process, clears the L2 cache and disables the L2
>>      cache
>>      >> controller again.
>>      >>
>>      >> The reason for doing it in SPL is because we need to avoid
>>      accessing any
>>      >> of the potentially stale data in the L2 cache, and we are certain
>>      any of
>>      >> the stale data will be below the OCRAM address range. To further
>>      reduce
>>      >> bus traffic during the L2 cache invalidation, we enable L1
>>      I-cache and
>>      >> run the invalidation code entirely out of the L1 I-cache.
>>      >>
>>      >> Signed-off-by: Marek Vasut <marex at denx.de <mailto:marex@denx.de>>
>>      >> Cc: Dalon Westergreen <dwesterg@gmail.com
>>      <mailto:dwesterg@gmail.com>>
>>      >> Cc: Dinh Nguyen <dinguyen at kernel.org <mailto:dinguyen@kernel.org>>
>>      >
>>      > We've tested this and it seems to fix the issue, so:
>>      >
>>      > Tested-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com
>>      <mailto:simon.k.r.goldschmidt@gmail.com>>

Seems like I was a little fast there: the patch does not apply cleanly 
to master. It did apply to our tree, obviously.

It's missing an include to <asm/pl310.h> and 'pl310' is undefined. 
Probably a result of rebasing it...

>>      >
>>      > However, I don't really get why clearing the L2 cache later in U-Boot
>>      > isn't good enough.
>>
>>      Because when U-Boot is running, it is already running from RAM, which is
>>      on different PL310 master than OCRAM, and you're likely to hit the
>>      corrupted cache lines on the DRAM master which is primed by prior Linux
>>      operation. Such cacheline can be hit between the code enabling the PL310
>>      and the code invalidating it, which is a C code, using stack and calling
>>      functions, thus accessing memory which would likely reside in different
>>      PL310 cachelines. If one of those cachelines contains invalid/corrupted
>>      data, they can be provided to the CPU before the cacheline is
>>      invalidated.
>>
>>      To further reduce the likelihood of hitting any such cache line, the
>>      code which enables the PL310 and invalidates it runs from primed L1
>>      icache lines, thus generating no bus traffic at all.
>>
>>
>> Ah, right. I somehow didn't realize that invalidating is done after
>> enabling:-)
> 
> Right. If it could be done before (like e.g. by whacking the L2CC
> reset), that'd be fantastic.

I haven't yet found a way to cleanly reproduce this on my socrates, so 
unfortunately, I currently cannot test any suggestions, right now :-(

Wouldn't it be better to place this function in some central position 
near the cache initialization code instead of running it from SPL? Or is 
it required to run it in SPL?

Also, it seems this patch enables the ICACHE in SPL, which wasn't 
enabled before. Not a bad thing, but might this have side effects?

Regards,
Simon

> 
>>      > Also, wouldn't this affect other platforms as well?
>>
>>      I had the same concern, however I suspect it might have to do with the
>>      reset implementation on SoCFPGA, which doesn't clear the L2CC, while
>>      reset implementations on other SoCs likely do clear the L2CC. I am
>>      however inquiring with Altera/Intel about this.
>>
>>
>> Ok, thanks for the explanation!
> 
> Sure
>