On Thursday, December 10, 2015 at 03:27:08 AM, Tom Rini wrote: > On Wed, Dec 09, 2015 at 03:09:13PM +0100, Marek Vasut wrote: > > On Wednesday, December 09, 2015 at 02:02:03 PM, Stefan Roese wrote: > > > Hi All! > > > > > > On 09.12.2015 09:09, Albert ARIBAUD wrote: > > > > On Tue, 8 Dec 2015 14:43:42 +0100, Marek Vasut <ma...@denx.de> wrote: > > > >> The arch/arm/lib/cache-cp15.c checks for CONFIG_ARMV7 and if this > > > >> macro is set, it configures TTBR0 register. This register must be > > > >> configured for the cache on ARMv7 to operate correctly. > > > >> > > > >> The problem is that noone actually sets the CONFIG_ARMV7 macro and > > > >> thus the TTBR0 is not configured at all. On SoCFPGA, this produces > > > >> all sorts of minor issues which are hard to replicate, for example > > > >> certain USB sticks are not detected or QSPI NOR sometimes fails to > > > >> write pages completely. > > > >> > > > >> The solution is to replace CONFIG_ARMV7 test with CONFIG_CPU_V7 one. > > > >> This is correct because the code which added the test(s) for > > > >> CONFIG_ARMV7 was added shortly after CONFIG_ARMV7 was replaced by > > > >> CONFIG_CPU_V7 and this code was not adjusted correctly to reflect > > > >> that change. > > > > > > > > Note: > > > > > > > > As discussed with Marek on IRC, this patch enables what is supposed > > > > to be the correct MMU settings for ARMv7, which causes a sharp > > > > Ethernet performance drop (40%) but also a strong general memory > > > > access performance hit (a copy of 4 MB is almost instantaneous > > > > without the patch and takes 2-3 seconds with it). > > > > > > I've tested Marek's patch on my current Armada XP platform (also > > > ARMv7). And also noticed a performance drop by about 30-40%. > > > The dhrystone command can be used for this testing as well > > > (CONFIG_CMD_DHRYSTONE). > > > > > > So this is definitely a NAK from me to this current patch. > > > > This is a wrong approach, this patch just fixes another patch which was > > broken from the getgo and the TTBR0 reg was not programmed correctly due > > to that. This patch must be applied, but what we need to decide is > > whether we need _another_ patch which removes the S-bit from the > > pagetable or how to deal with that part. > > No, we don't have to apply this patch. The original patch here > (97840b5) was likely not run-time tested when submitted as it was using > the then-defunct CONFIG_ARMV7 symbol, and was likely a bugfix in an > internal tree that was pushed upstream (which is on the whole, good).
I find it surprising that such a patch, which modifies common code even, was not scrutinized enough. > So in sum U-Boot has always been as broken in some specific regard > before that patch as it is after that patch. So we have time to see > what we need to do about enabling this feature correctly and even > ensuring that we don't happen to say break things once Linux is up too. In that case, I am looking forward to better suggestions. I still disagree that this patch should not be applied, it corrects code which was broken. The slowdown caused by the original patch is a separate issue and should be corrected by a separate patch. If these two patches get applied at once, that is fine. Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
