Enable OpenAMP Demo configuration via CMake #365
Conversation
bentheredonethat
force-pushed
the
app_configuration
branch
from
bd1434e
to
e917fcb
Compare
Allow symbols for IPI and shared memory to be set in CMake configuration Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
bentheredonethat
force-pushed
the
app_configuration
branch
from
e917fcb
to
54b5817
Compare
Allow symbols for IPI and shared memory to be set in CMake configuration Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
bentheredonethat
force-pushed
the
app_configuration
branch
from
54b5817
to
e2bf071
Compare
apps/CMakeLists.txt
Outdated
| if(NOT DEFINED ${RSC_TABLE}) | ||
| set(RSC_TABLE 0x3ed20000 CACHE STRING "") | ||
| endif(NOT DEFINED ${RSC_TABLE}) | ||
|
|
There was a problem hiding this comment.
This part is related to the machine, the resource table is optional
There was a problem hiding this comment.
updated so that now this logic is only in the machine
bentheredonethat
force-pushed
the
app_configuration
branch
from
e2bf071
to
5f04543
Compare
apps/examples/echo/CMakeLists.txt
Outdated
| @@ -38,7 +38,7 @@ foreach (_app rpmsg-echo-ping rpmsg-echo) | |||
| add_executable (${_app}.out ${_sources}) | |||
| set_source_files_properties(${_sources} PROPERTIES COMPILE_FLAGS "${_cflags}") | |||
|
|
|||
| target_link_libraries(${_app}.out -Wl,-Map=${_app}.map -Wl,--gc-sections ${_linker_opt} -Wl,--start-group ${OPENAMP_LIB}-static ${_deps} -Wl,--end-group) | |||
| target_link_libraries(${_app}.out -Wl,-Map=${_app}.map -Wl,--defsym,_rsc_table=${RSC_TABLE} -Wl,--gc-sections ${_linker_opt} -Wl,--start-group ${OPENAMP_LIB}-static ${_deps} -Wl,--end-group) | |||
There was a problem hiding this comment.
My apologize seems that my comment on my first review has not been saved...
Here it is the same the "--defsym,_rsc_table=${RSC_TABLE}" should come from the machine.
what's happen if ${RSC_TABLE} is not set?
here is an alternative:
https://github.com/OpenAMP/open-amp/blob/main/apps/system/generic/machine/zynqmp_r5/CMakeLists.txt#L6
There was a problem hiding this comment.
updated so that it is in this file
| # If not provided, add default value for resource table | ||
| if(NOT DEFINED ${RSC_TABLE}) | ||
| set(RSC_TABLE 0x3ed20000 CACHE STRING "") | ||
| endif(NOT DEFINED ${RSC_TABLE}) |
There was a problem hiding this comment.
There was a problem hiding this comment.
moved to the file you suggestd
bentheredonethat
force-pushed
the
app_configuration
branch
from
5f04543
to
a1f6ef3
Compare
|
@arnopo updated so that only linker scripts and the 2 CMakeLists.txt files are updated. thanks |
Currently the resource table location in the firmware is hard-coded. Preserve the current functionality by adding a default value. Allow user to overwrite the default resource table location by adding the CMake property: set(RSC_TABLE <address> CACHE STRING "") Signed-off-by: Ben Levinsky <ben.levinsky@xilinx.com>
bentheredonethat
force-pushed
the
app_configuration
branch
from
a1f6ef3
to
f80d919
Compare
There was a problem hiding this comment.
I wonder if the application folder should not be in a separate repo as code is for demo and test and doesn't request same level of maturity.
@edmooring what is your feeling on this?
That's said for time being, LGTM with just a comment/question
| #define SHM_DEV_NAME "3ed20000.shm" /* shared device name */ | ||
|
|
||
| #endif /* SHM_DEV_NAME */ |
There was a problem hiding this comment.
Do you really need to give a name that depend on the address?
Same for POLL_DEV_NAME and IPI_DEV_NAME
There was a problem hiding this comment.
for linux a name is needed as the device is accessed via UIO and device tree node name
I think having the apps in a separate repository will make it harder for newcomers to work with the code. Their lives are hard enough already:) |
Currently the demos' interrupt, shared memory and IPC structure values are hard-coded.
Allow these to configured via CMake configure by
(a) wrapping C symbols in #ifdef / #ifndef
(b) enabling linker script defined resource table location to be set in CMake.