Hello, Please see in-lined. On Tue, Aug 18, 2015 at 2:52 AM, Siarhei Siamashka <[email protected]> wrote: > On Tue, 18 Aug 2015 01:53:23 +0800 > Vishnu Patekar <[email protected]> wrote: > >> On allwinner SOCs A33,H3 MMU is not enabled by BROM, so don't exit >> if MMU is not enabled by BROM and return NULL. >> It was reported on A33 tablet and Orange Pi H3 Board. >> I've tested it on A33 tablet. >> >> Signed-off-by: VishnuPatekar <[email protected]> > > Thanks. If some of the SoC variants don't have MMU enabled by the BROM > in the first place, then there is no need for us to temporarily disable > it during the SPL execution. > > Later we might want to try enabling MMU on A33 for performance reasons > though. The USB driver in the BROM is apparently doing a memcpy loop as > part of the data transfer. And tweaking the cacheability attributes of > the memory sections via MMU can improve the transfer speed quite > significantly (up to 3x speed boost on Allwinner A20): > > https://github.com/linux-sunxi/sunxi-tools/commit/e4b3da2b17ee9d7c5cab9b80e708b3a309fc4c96 > > You can benchmark the transfer speed to DRAM on A33 by running something > like "fel -v write 0x42000000 uImage". benchmark result is: Written 2944.6 KB in 15.3 sec (speed: 191.9 KB/s) > >> --- >> fel.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/fel.c b/fel.c >> index 41b19c9..c53be60 100644 >> --- a/fel.c >> +++ b/fel.c >> @@ -502,7 +502,7 @@ uint32_t aw_get_sctlr(libusb_device_handle *usb) >> >> uint32_t *aw_backup_and_disable_mmu(libusb_device_handle *usb) >> { >> - uint32_t *tt = malloc(16 * 1024); >> + uint32_t *tt = NULL; >> uint32_t ttbr0 = aw_get_ttbr0(usb); >> uint32_t sctlr = aw_get_sctlr(usb); >> uint32_t i; >> @@ -520,19 +520,20 @@ uint32_t >> *aw_backup_and_disable_mmu(libusb_device_handle *usb) >> >> if (!(sctlr & 1)) { >> fprintf(stderr, "MMU is not enabled by BROM\n"); > > If this is in fact not an error on some SoC variants (A33), then > probably we should downgrade it from "fprintf(stderr, ...)" to > "pr_info(...)" in order not to scare/confuse the users. Okie. I'll change it to pr_info. > >> - exit(1); >> + goto exit; > > Maybe just "return NULL" here would be cleaner without adding goto > statements and labels? Okie. > >> } >> >> if ((sctlr >> 28) & 1) { >> fprintf(stderr, "TEX remap is enabled!\n"); >> - exit(1); >> + goto exit; >> } >> >> if (ttbr0 & 0x3FFF) { >> fprintf(stderr, "Unexpected TTBR0 (%08X)\n", ttbr0); >> - exit(1); >> + goto exit; >> } > > I would not touch these two extra safety checks unless really necessary. > They can only fail if you have MMU enabled, but configured in an > unexpected way. Okie. > >> + tt = malloc(16 * 1024); >> pr_info("Reading the MMU translation table from 0x%08X\n", ttbr0); >> aw_fel_read(usb, ttbr0, tt, 16 * 1024); >> for (i = 0; i < 4096; i++) >> @@ -555,6 +556,7 @@ uint32_t *aw_backup_and_disable_mmu(libusb_device_handle >> *usb) >> aw_fel_execute(usb, FEL_EXEC_SCRATCH_AREA); >> pr_info(" done.\n"); >> >> +exit: >> return tt; >> } >> >> @@ -735,7 +737,9 @@ void aw_fel_write_and_execute_spl(libusb_device_handle >> *usb, >> exit(1); >> } >> >> - aw_restore_and_enable_mmu(usb, tt); >> + /* Disable the MMU if it was enabled */ > > Here we are in fact re-enabling the MMU again. So the comment text > is not particularly accurate. > >> + if(tt != NULL) >> + aw_restore_and_enable_mmu(usb, tt); >> } >> >> /* Constants taken from ${U-BOOT}/include/image.h */ > > > > -- > Best regards, > Siarhei Siamashka
-- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
