From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Goldschmidt Date: Tue, 19 Feb 2019 20:29:58 +0100 Subject: [U-Boot] [PATCH] ARM: socfpga: Clear PL310 early in SPL In-Reply-To: <682aa3e5-92e4-ccf0-0e87-aeb79b1303a7@denx.de> References: <20190219125240.2833-1-marex@denx.de> <0ed007f6-c31b-1e58-d1f5-e2ef798e2ecb@denx.de> <60481271-afb7-fe89-45ff-595ec2a372a6@denx.de> <2cc446bd-67e1-e69d-44cf-d7d7d0594c01@gmail.com> <682aa3e5-92e4-ccf0-0e87-aeb79b1303a7@denx.de> Message-ID: <00e96206-1334-ae09-32b4-46f0ad3ebc5f@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de Am 19.02.2019 um 17:39 schrieb 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 >>>>> > geschrieben: >>>>>> >>>>>>       On 2/19/19 2:28 PM, Simon Goldschmidt wrote: >>>>>>       > On Tue, Feb 19, 2019 at 1:53 PM Marek Vasut >>>>>       > 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 >>>>> > >>>>>>       >> Cc: Dalon Westergreen >>>>>       > >>>>>>       >> Cc: Dinh Nguyen >>>>> > >>>>>>       > >>>>>>       > We've tested this and it seems to fix the issue, so: >>>>>>       > >>>>>>       > Tested-by: Simon Goldschmidt >>>>>       > >>>> >>>> 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 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. > > :) Sorry, I phrased that wrong. I *do* (still) not understand why this is necessary before accessing DRAM. Sorry if I'm annoying you with that, but I'd like to understand. Does the DWMMC DMA really trigger PL310 operations? I thought DMA would only affect cache if it goes through ACP/SCU? Regards, Simon > >>>> 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 >