Conversation
leon6002
requested review from
DavidMCerdeira,
danielRep,
josecm,
miguelafsilva5 and
sandro2pinto
as code owners
There was a problem hiding this comment.
The PR is already in a good state for approval. There is, however, an issue with ` e3650_enable_imp_def: these type of core implementation-defined configurations should be done in a different manner. We discussed a possible solution for these cases in our weekly meeting. We'll try to introduce the necessary infrastructure for this in the following days. I'll tag this PR as soon as that PR is introduced.
Please solve also the issues with gitlint (commit sign-off) and the code needs to be format using make format
src/platform/e3650/e3650.c
Outdated
|
|
||
| /* Enable Prefetch */ | ||
| /* mrrc p15, 0, r0, r1, c15 : read AxCACHE PREFETCH CONTROL REGISTER */ | ||
| __asm__ volatile("mrrc p15, 0, %0, %1, c15" : "=r"(lo), "=r"(hi)); |
There was a problem hiding this comment.
We use the macros SYSREG_GEN_ACCESSORS* to generate the appropriate accessors for system registers. Please modify accordingly throughout e3650_enable_imp_def
There was a problem hiding this comment.
I have refactored the system register access using the SYSREG_GEN_ACCESSORS macros as suggested. I've also verified the changes on the actual hardware, and the platform behaves as expected. Please let me know if the current implementation aligns with the project's standards.
src/platform/e3650/e3650.c
Outdated
| hi &= ~3U; /* Clear bit 0,1 */ | ||
| lo &= ~(7U << 13); | ||
| /* CONFIG_DATA_PREFETCH_OT = 5 */ | ||
| lo |= (1U << 11) | (5U << 13); |
There was a problem hiding this comment.
Please avoid using magical numbers and switch for macros.
There was a problem hiding this comment.
I have replaced the magic numbers with macros as suggested.
src/platform/e3650/e3650.c
Outdated
| /* Enable Branch Prediction */ | ||
| val = 0; | ||
| __asm__ volatile("mcr p15, 1, %0, c9, c1, 1" :: "r"(val)); | ||
| __asm__ volatile("isb"); |
There was a problem hiding this comment.
| __asm__ volatile("isb"); | |
| ISB(); |
src/platform/e3650/e3650.c
Outdated
| val = (ICACHE_FLASH_2_AXIM_2 | DCACHE_FLASH_2_AXIM_2); | ||
| __asm__ volatile("mcr p15, 1, %0, c9, c1, 0" :: "r"(val)); | ||
|
|
||
| __asm__ volatile("isb"); |
There was a problem hiding this comment.
| __asm__ volatile("isb"); | |
| ISB(); |
src/platform/e3650/objects.mk
Outdated
| ## Copyright (c) Bao Project and Contributors. All rights reserved. | ||
|
|
||
| boards-objs-y+=e3650_desc.o | ||
| boards-objs-y+=e3650.o |
There was a problem hiding this comment.
e3650.c should be named platform.c to keep consistency. We're using that naming for the source file containing the definition of void platform_default_init(void)
There was a problem hiding this comment.
Renamed e3650.c to platform.c as suggested. Thanks!
There was a problem hiding this comment.
I have also formatted the code using make format as suggested.
Signed-off-by: gulong <uomleon@163.com>
2 participants
This PR added support for Semidrive's E3650 platform.
The related PR:
bao-project/bao-demos#75