Hi Andre,
I have found the issue. Please see inline.
On 09/10/2020 18:37, Biju Das wrote:
Hi Biju,
> Subject: Re: commit 75fab6496e5fce9a11 ("libc: memset: improve > performance by avoiding single byte writes") causing BL31 boot > failure on Renesas RZ/G2 platforms. > > On 08/10/2020 20:05, Biju Das wrote: >> Then on target, found that BL31 is failed to boot[3], which is >> fixed by reverting the commit 75fab6496e5fce9a11 >> ("libc: memset: improve performance by avoiding single byte >> writes"), see > the logs [3]. > > Mmmh, interesting. Can you build with "DEBUG=1" to get more > output from BL31?
Sure Will do and provide feedback.
> I see some calls to memset() from code in drivers/renesas/rcar. > Can you add some debug prints at the top of memset() to dump the > parameters on each call? To see what breaks it?
OK.
I believe commenting [1] fixed the issue which corresponds to memset
parameters[2].
Looks like calling rcar_log_init-->memset from [3] is causing the issue.
Many thanks for debugging this, that's helpful!
But without your patch it is working fine. Only thing is, you have added extra code to
improve the speed. I am not sure how it is impacting here. Currently I don't
have access to Lauterbach to debug further. Any thoughts ???
So this RCAR_BL31_LOG_BASE address is mapped as device memory,
what
kind of device is this? SRAM? Or something non-volatile, even? Even if it's SRAM, sometimes the controller managing this does not support all AMBA accesses (hence it's a device, not memory).
It is DRAM only, with Security protected attribute.
Mmh, interesting. Why is it then mapped as device?
#define MAP_ATFW_LOG MAP_REGION_FLAT(RCAR_BL31_LOG_BASE, \ RCAR_BL31_LOG_SIZE, \ MT_DEVICE | MT_RW | MT_SECURE)
You could try using MT_NORMAL instead. Or ...
In general letting the compiler access device memory directly is asking for trouble, every access should go through MMIO accessors. Linux has memset_io() for those kind of problems.
Can you try the attached patch? That should downgrade the stores to 32-bit ones. Not a solution, but would help to isolate the problem.
Patch didn't fix the issue.
I see, bummer. Does it work if you apply this on top?
diff --git a/lib/libc/memset.c b/lib/libc/memset.c index 5ad6ede51..7478871e3 100644 --- a/lib/libc/memset.c +++ b/lib/libc/memset.c @@ -7,6 +7,7 @@ #include <stddef.h> #include <string.h> #include <stdint.h> +#include <lib/mmio.h>
void *memset(void *dst, int val, size_t count) { @@ -34,7 +35,8 @@ void *memset(void *dst, int val, size_t count) /* Use 32-bit writes for as long as possible. */ ptr32 = (void *)ptr; for (; count >= 4; count -= 4) { -*ptr32++ = fill; +mmio_write_32((uintptr_t)ptr32, fill); +ptr32++; }
/* Handle the remaining part byte-per-byte. */
However I copied the same code emmc_memset to rcar_memset() and
with that it works fine.
Looks like byte access works OK. Any thoughts??
That's weird, I just see "str w1, [x3, x5]" in the disassembly, and since everything is nicely aligned, it should actually work. That is still with the note that letting the compiler access device memory is wrong, but the generated code is actually very similar in this special case here.
Issue is with your original commit[1], it is entering infinite loop. I commented that while loop and it is fine.
[1]https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/lib/libc/me...
Regards, Biju
Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647