Add dedicated CMakeLists.txt to target HALs and util#25
Add dedicated CMakeLists.txt to target HALs and util#25
Conversation
lukamac
force-pushed
the
dedicated-hal-cmake
branch
2 times, most recently
from
4aa96fb to
b93e159
Compare
lukamac
force-pushed
the
dedicated-hal-cmake
branch
from
b93e159 to
2075ddd
Compare
Scheremo
left a comment
Scheremo
left a comment
There was a problem hiding this comment.
Changes overall look reasonable. The way PICOLIBC and COMPILER_RT are set look like they are not generic / portable, so maybe a warning sign or slight refactor could help. I also spotted a hardcoded base address define, where I'm not sure if it's a generic thing or implementation-specific.
| set(PICOLIBC $ENV{TOOLCHAIN_LLVM_INSTALL_DIR}/picolibc/riscv) | ||
| set(COMPILER_RT $ENV{TOOLCHAIN_LLVM_INSTALL_DIR}/lib/clang/15.0.0/lib/baremetal/rv32imc/) |
There was a problem hiding this comment.
These conventions are a bit idiosyncratic. It might be worth it to wrap in a check if they are externally defined and to document that they are adjustable.
There was a problem hiding this comment.
This might not be related to this PR though, but opening an issue might be a good idea.
There was a problem hiding this comment.
Added two new environment variables to remove the idiosyncrasies. Commit 650eb73
| @@ -0,0 +1,75 @@ | |||
| if (NOT DEFINED ENV{PULP_SDK_HOME}) | |||
There was a problem hiding this comment.
Might be nice to have this file in the PULP-SDK proper.
There was a problem hiding this comment.
It would be nice. Until they aren't, they stay.
| set(COMPILER_RT $ENV{TOOLCHAIN_LLVM_INSTALL_DIR}/lib/clang/15.0.0/lib/baremetal/rv32imc/) | ||
|
|
There was a problem hiding this comment.
The toolchain files seem duplicated. Might be a good idea to keep them in one place.
There was a problem hiding this comment.
Moved them to a single cmake folder in test. Commit 14a0aff
| // Minimal copy from neureka/bsp/neureka_siracusa_bsp.c | ||
| #define NEUREKA_SIRACUSA_BASE_ADDR (0x00201000) | ||
| static const neureka_dev_t nnx_dev = { | ||
| .hwpe_dev = (struct hwpe_dev_t){ | ||
| .base_addr = (volatile uint32_t *)NEUREKA_SIRACUSA_BASE_ADDR}}; |
There was a problem hiding this comment.
Is this base address generic or implementation specific? If it is implementation-specific, you might want to consider wrapping it, so it cna be overwritten without editing a source file.
There was a problem hiding this comment.
Do you mean just wrapped with #ifndef?
There was a problem hiding this comment.
Wrapped them with #ifndef. Commit 06e66bf
|
Unfortunately, this does not work for a "non-BSP" build, as CMake always tries to build the whole Also, see https://github.com/Scheremo/pulp-nnx/tree/dedicated-hal-cmake for some small fixes to eliminate compiler warnings and make Neureka architecture parameters overridable with |
|
@da-gazzi yeah, my idea was for you to directly use the CMake file in the hal folder, not the top-level one, but as discussed with Moritz, we will continue with your initial PR, so disregard this one |
|
closing the PR in favor of #26 |
3 participants
Added dedicated CMakeLists.txt to each target HAL. This required also modularizing the pulp-nnx utilities in its own library. I decided to make the HALs static libraries and the utilities into object libraries so that they can get inlined.