[U-Boot] [PATCH] ARM: socfpga: Clear PL310 early in SPL

[Marek Vasut]

On 2/19/19 5:31 PM, Simon Goldschmidt wrote:
> Am 19.02.2019 um 17:00 schrieb Marek Vasut:
>> On 2/19/19 4:55 PM, Simon Goldschmidt wrote:
>>> 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 at denx.de
>>>>> <mailto:marex at 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 at denx.de
>>>>>       <mailto:marex at 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 at denx.de>>
>>>>>       >> Cc: Dalon Westergreen <dwesterg at gmail.com
>>>>>       <mailto:dwesterg at gmail.com>>
>>>>>       >> Cc: Dinh Nguyen <dinguyen at kernel.org
>>>>> <mailto:dinguyen at kernel.org>>
>>>>>       >
>>>>>       > We've tested this and it seems to fix the issue, so:
>>>>>       >
>>>>>       > Tested-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com
>>>>>       <mailto:simon.k.r.goldschmidt at 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 :-(
>>
>> Did you ever have this hang happen on SoCrates ?
> 
> I think so, yes.
> 
> What I did was running some lmb tests loading from mmc, removing the
> sdcard to write an updated version of U-Boot, replacing it and then
> typing 'reset'. It would then sometimes hang in cache initialization - I
> added debug prints throughout cache initialization and was debug prints
> from one function and then nothing more after a simple function call...
> 
> That would very much fit to your explanation. However, I cannot
> reproduce this currently :-(

So basically a board which uses DMA (like the DWMMC DMA) is likely to be
affected. Surely, the DMA does trigger PL310 operations.

>>> 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?
>>
>> It's required in SPL as soon as possible, to avoid accessing the DRAM,
>> see above. The current location is close to the rest of PL310 init in
>> SPL.
> 
> OK, I just did not understand why this is necessary before accessing DRAM.

:)

>>> 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?
>> Actually, I-Cache is enabled in SPL:
>>
>> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv7/start.S;h=0cb6dd39ccfc0584f4e6f01523fef9405c314763;hb=HEAD#l158
>>
> 
> Oh, right. Unless disabled via config option.

Yes

-- 
Best regards,
Marek Vasut