Skip to content

Conversation

@serhiy-katsyuba-intel
Copy link
Contributor

Initial commit with set of cmake and linker scripts to build loadable modules binary.

Things yet TODO:
add all necessary Dev Kit header files to include dir,
replace dummy example with somewhat useful module example.

@serhiy-katsyuba-intel
Copy link
Contributor Author

I will be on leave till April 10, so no response until then.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review, needs to documentation to move forward.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GCC should also work here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this toolchain file is a difficult subject.
In my opinion we have 2 choices here:

  1. we can try to re-use Zephyr toolchain files.
  2. we can define new toolchain file like you did here for Xtensa

From my perspective, if you choose option 2), I would like to consider few things:

  • create separate toolchain files for Xtensa and for GCC
  • please add "toolchain" word to the filename, eg. xtensa-toolchain.cmake
  • please make it work using cmake --toolchain flag

Zephyr project does not use toolchain files like they should be used (atomically and without cmake include command).
We have a change to improve this here.
Basically, toolchain file should work as such:
cmake -S . -B build -G Ninja --toolchain cmake/xtensa-toolchain.cmake
And should not be used as such
CMakeLists.txt file content:

if(XTENSA_TOOLCHAIN)
    include(cmake/xtensa-toolchain.cmake)
elif(GCC_TOOLCHAIN)
    include(cmake/gcc-toolchain.cmake)
endif()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed toolchain file to xtensa-toolchain.cmake. The file is not included but used as

set(CMAKE_TOOLCHAIN_FILE "${CMAKE_CURRENT_LIST_DIR}/../../cmake/xtensa-toolchain.cmake")

in dummy library CMakeLists.txt. So if that line is commented out, the toolchain file may be used with cmake --tollchain= option.

Copy link
Contributor

@aborisovich aborisovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some things that we need to address, but overall it looks quite good.
My biggest concern is a editing manually config.cmake file. I assume this was just done for testing to show us how the draft works. See comments below

@sys-pt1s
Copy link

sys-pt1s commented Apr 6, 2023

Can one of the admins verify this patch?

@serhiy-katsyuba-intel serhiy-katsyuba-intel force-pushed the lmdk_dummy_module branch 3 times, most recently from 52cefd0 to f5a782b Compare April 13, 2023 16:55
@serhiy-katsyuba-intel serhiy-katsyuba-intel marked this pull request as ready for review April 13, 2023 17:17
@pjdobrowolski
Copy link
Contributor

Building modules required fresh rimage tool, is it a good idea to use west tool for checking freshness of repositories and build?

@kv2019i
Copy link
Collaborator

kv2019i commented Apr 24, 2023

@pjdobrowolski wrote:

Building modules required fresh rimage tool, is it a good idea to use west tool for checking freshness of repositories and build?

SOF rimage is fixed to version in sof/west.yml. So if you need a newer rimage, the rimage version needs to be upgraded first in SOF main, and then you can merge your PR and trust the rimage is always new enough.

@lgirdwood
Copy link
Member

@serhiy-katsyuba-intel can we track the rimage & west dependencies here so we can merge after deps are merged.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be really nice if this common part of the toml could be shared and included. We now need the "base toml" for each platform in multiple places -- for base FW build of SOF, to build Zephyr OS test cases and now also here.

Not a blocker though, something we can do incrementally as well.

@serhiy-katsyuba-intel
Copy link
Contributor Author

@aborisovich , I've addressed the changes you requested, could you redo the review ("changes requested" status blocks merging)?

@aborisovich
Copy link
Contributor

@aborisovich , I've addressed the changes you requested, could you redo the review ("changes requested" status blocks merging)?

I'll check those in a moment and let you know

@serhiy-katsyuba-intel
Copy link
Contributor Author

@serhiy-katsyuba-intel can we track the rimage & west dependencies here so we can merge after deps are merged.

@lgirdwood , not sure if I understand correctly. There is no dependency on zephyr or west. As for rimage, I just verified: all required rimage modification to support loadable modules are already merged to rimage repo and that repo sha is updated in our west yaml.

Copy link
Contributor

@aborisovich aborisovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During testing, some issues found. I know it is dummy but still we should make sure it works in some expected way right?
Moreover, this does not build on Windows - fails on call to rimage during the build:

FAILED: CMakeFiles/dummy_target C:/Workspace/my_sof_workspace/sof/lmdk/libraries/dummy/build/CMakeFiles/dummy_target
cmd.exe /C "cd /D C:\Workspace\my_sof_workspace\sof\lmdk\libraries\dummy\build && ......\rimage -k ......\keys\otc_private_key.pem -f 2.0.0 -b 1 -o dummy_noextmft -c C:/Workspace/my_sof_workspace/sof/lmdk/libraries/dummy/dummy_mtl.toml -e dummy && "C:\Program Files\CMake\bin\cmake.exe" -E cat dummy_noextmft.xman dummy_noextmft > dummy.bin"
'......\rimage' is not recognized as an internal or external command,
operable program or batch file.
ninja: build stopped: subcommand failed.

Initial commit with set of cmake and linker scripts to build loadable
modules binary.

Things yet TODO:
  add all necessary Dev Kit header files to include dir,
  replace dummy example with somewhat useful module example.

Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>
@serhiy-katsyuba-intel
Copy link
Contributor Author

Changes in the updated version:

  • Proper error message is displayed if XTENSA_TOOLCHAIN_PATH environment or CMake variable not defined.
  • Now if required toolchain not found, CMake will NOT try to find some other toolchain to use.
  • Indentation in .cmake files replaced to use tabs.

@serhiy-katsyuba-intel
Copy link
Contributor Author

Just in case, the error reported by checkpatch

ERROR: trailing whitespace
#68: FILE: lmdk/README.md:10:

is kind of false positive: there intentionally has to be a spaces in README.md.

3 warnings in checkpatch

Prefer __section(".buildinfo") over __attribute__((section(".buildinfo")))

not worth to be fixed: currently dummy loadable module does not include any header files. In future, proper headers should be added for loadable modules and these could have such macro defined. Also dummy loadable module example in future will be replaced with more practical example.

Copy link
Contributor

@aborisovich aborisovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All my questions had been answered, looks good now.

Copy link
Contributor

@aborisovich aborisovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, found the reproduction where it still does not work (rimage issue). My previous request changes was not addressed. I've prepared a fix if you like - just cherry pick aborisovich@eafbaa2 or if you don't fix it differently. Either way I think passing a relative path to rimage should work.
image

Copy link
Contributor

@aborisovich aborisovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agreed to merge this as is without relative path to rimage working and I will provide this fixup in separate PR.

@kv2019i
Copy link
Collaborator

kv2019i commented May 11, 2023

Ready to go, need a clean CI run still.

@mwasko mwasko merged commit c00354b into thesofproject:main May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants