Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

soc: xtensa: nxp: invoke west sign only for SOF #59603

Conversation

iuliana-prodan
Copy link
Collaborator

west sign is used only for Sound Open Firmware (SOF) sample.
So, add this only if CONFIG_SOF is enabled.
Otherwise, when compiling other samples they're not building because of the rimage dependency.

Fixes: 5ae7bd8 ("soc: xtensa: nxp: invoke west sign at west build time")

west sign is used only for Sound Open Firmware (SOF)
sample.
So, add this only if CONFIG_SOF is enabled.
Otherwise, when compiling other samples they're not
building because of the rimage dependency.

Fixes: 5ae7bd8 ("soc: xtensa: nxp: invoke west sign at west build time")

Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
@marc-hb
Copy link
Collaborator

marc-hb commented Jun 22, 2023

Why not but I don't understand what problem this solves because I implemented --if-tool-available to make sure rimage is optional always - for instance for build-only twister tests that may or may not have rimage installed.

Otherwise, when compiling other samples they're not building because of the rimage dependency.

That should not happen. Or did you mean: rimage is found while not wanted and it fails in some cases?

@marc-hb

This comment was marked as off-topic.

@iuliana-prodan
Copy link
Collaborator Author

iuliana-prodan commented Jun 22, 2023

Why not but I don't understand what problem this solves because I implemented --if-tool-available to make sure rimage is optional always - for instance for build-only twister tests that may or may not have rimage installed.

Otherwise, when compiling other samples they're not building because of the rimage dependency.

That should not happen. Or did you mean: rimage is found while not wanted and it fails in some cases?

It fails, while building other samples, because it cannot find zephyr.elf, needed by zephyr.ri.
Here's the error I got when compiling samples/subsys/ipc/openamp_rsc_table/:

nxa06898@lsv15040:~/data/zephyrproject/zephyr$ west build -p always -b nxp_adsp_imx8m samples/subsys/ipc/openamp_rsc_table/
-- west build: generating a build system
Loading Zephyr default modules (Zephyr base).
-- Application: /home/nxa06898/data/zephyrproject/zephyr/samples/subsys/ipc/openamp_rsc_table
-- CMake version: 3.21.1
-- Found Python3: /usr/bin/python3.8 (found suitable exact version "3.8.0") found components: Interpreter 
...
-- Found GnuLd: /opt/zephyr-sdk-0.16.0/xtensa-nxp_imx8m_adsp_zephyr-elf/bin/../lib/gcc/xtensa-nxp_imx8m_adsp_zephyr-elf/12.2.0/../../../../xtensa-nxp_imx8m_adsp_zephyr-elf/bin/ld.bfd (found version "2.38") 
-- The C compiler identification is GNU 12.2.0
-- The CXX compiler identification is GNU 12.2.0
-- The ASM compiler identification is GNU
-- Found assembler: /opt/zephyr-sdk-0.16.0/xtensa-nxp_imx8m_adsp_zephyr-elf/bin/xtensa-nxp_imx8m_adsp_zephyr-elf-gcc
...
-- Generating done
-- Build files have been written to: /home/nxa06898/data/zephyrproject/zephyr/build
-- west build: building application
ninja: error: 'zephyr/zephyr.elf', needed by 'zephyr/zephyr.ri', missing and no known rule to make it
FATAL ERROR: command exited with status 1: /usr/bin/cmake --build /opt/samba/nxa06898/zephyrproject/zephyr/build
nxa06898@lsv15040:~/data/zephyrproject/zephyr$

TBH is weird to me too, because, I don't get any error when building synchronization or hello_world samples.

@iuliana-prodan
Copy link
Collaborator Author

Why not but I don't understand what problem this solves because I implemented --if-tool-available to make sure rimage is optional always - for instance for build-only twister tests that may or may not have rimage installed.

Otherwise, when compiling other samples they're not building because of the rimage dependency.

That should not happen. Or did you mean: rimage is found while not wanted and it fails in some cases?

It fails, while building other samples, because it cannot find zephyr.elf, needed by zephyr.ri. Here's the error I got when compiling samples/subsys/ipc/openamp_rsc_table/:

nxa06898@lsv15040:~/data/zephyrproject/zephyr$ west build -p always -b nxp_adsp_imx8m samples/subsys/ipc/openamp_rsc_table/
-- west build: generating a build system
Loading Zephyr default modules (Zephyr base).
-- Application: /home/nxa06898/data/zephyrproject/zephyr/samples/subsys/ipc/openamp_rsc_table
-- CMake version: 3.21.1
-- Found Python3: /usr/bin/python3.8 (found suitable exact version "3.8.0") found components: Interpreter 
...
-- Generating done
-- Build files have been written to: /home/nxa06898/data/zephyrproject/zephyr/build
-- west build: building application
ninja: error: 'zephyr/zephyr.elf', needed by 'zephyr/zephyr.ri', missing and no known rule to make it
FATAL ERROR: command exited with status 1: /usr/bin/cmake --build /opt/samba/nxa06898/zephyrproject/zephyr/build
nxa06898@lsv15040:~/data/zephyrproject/zephyr$

TBH is weird to me too, because, I don't get any error when building synchronization or hello_world samples.

@marc-hb or it might be because for openamp_rsc_table sample it takes longer to build and get the .elf, compared with synchronization?
And, since the west build and west sign are done in parallel, the west sign starts before the .elf is done.

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 22, 2023

And, since the west build and west sign are done in parallel, the west sign starts before the .elf is done.

No that's not done in parallel, it would be really bad! west sign has an explicit dependency on zephyr.elf as shown in this error message that you shared:

'zephyr/zephyr.elf', needed by 'zephyr/zephyr.ri', missing and no known rule to make it

My first impression is that some examples simply produce no zephyr.elf file for some unknown reason.

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 22, 2023

samples/subsys/ipc/openamp_rsc_table/ fails to build much earlier in a clean tree: error: Aborting due to Kconfig warnings. Can you please clone everything from scratch and try to reproduce?

cd zproj/zephyr
git checkout 75dfdb14a1ec
west update
west compare # all clean

rm -rf build/ # pristine works only most of the time
west build -p always -b nxp_adsp_imx8m samples/subsys/ipc/openamp_rsc_table/

-- west build: making build dir zproj/zephyr/build pristine
-- west build: generating a build system
Loading Zephyr default modules (Zephyr base).
-- Application: zproj/zephyr/samples/subsys/ipc/openamp_rsc_table
-- CMake version: 3.26.3
-- Found Python3: /usr/bin/python3.11 (found suitable exact version "3.11.3") found components: Interpreter 
-- Cache files will be written to: .cache/zephyr
-- Zephyr version: 3.4.99 (zproj/zephyr)
-- Found west (found suitable version "1.0.99", minimum required is "0.14.0")
-- Board: nxp_adsp_imx8m
-- ZEPHYR_TOOLCHAIN_VARIANT not set, trying to locate Zephyr SDK
-- Found host-tools: zephyr 0.16.1 (zephyr-sdk-0.16.1)
-- Found toolchain: zephyr 0.16.1 (zephyr-sdk-0.16.1)
-- Found Dtc: /usr/bin/dtc (found suitable version "1.7.0", minimum required is "1.4.6") 
-- Found BOARD.dts: zproj/zephyr/boards/xtensa/nxp_adsp_imx8m/nxp_adsp_imx8m.dts
-- Generated zephyr.dts: zproj/zephyr/build/zephyr/zephyr.dts
-- Generated devicetree_generated.h: zproj/zephyr/build/zephyr/include/generated/devicetree_generated.h
-- Including generated dts.cmake file: zproj/zephyr/build/zephyr/dts.cmake

warning: PRINTK (defined at subsys/debug/Kconfig:203) was assigned the value 'n' but got the value
'y'. See http://docs.zephyrproject.org/latest/kconfig.html#CONFIG_PRINTK and/or look up PRINTK in
the menuconfig/guiconfig interface. The Application Development Primer, Setting Configuration
Values, and Kconfig - Tips and Best Practices sections of the manual might be helpful too.


zproj/zephyr/samples/subsys/ipc/openamp_rsc_table/prj.conf:4: warning: attempt to assign the value 'n' to the undefined symbol PLATFORM_SPECIFIC_INIT
Parsing zproj/zephyr/Kconfig
Loaded configuration 'zproj/zephyr/boards/xtensa/nxp_adsp_imx8m/nxp_adsp_imx8m_defconfig'
Merged configuration 'zproj/zephyr/samples/subsys/ipc/openamp_rsc_table/prj.conf'

error: Aborting due to Kconfig warnings

CMake Error at zproj/zephyr/cmake/modules/kconfig.cmake:343 (message):
  command failed with return code: 1
Call Stack (most recent call first):
  zproj/zephyr/cmake/modules/zephyr_default.cmake:115 (include)
  zproj/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:66 (include)
  zproj/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:92 (include_boilerplate)
  CMakeLists.txt:7 (find_package)


-- Configuring incomplete, errors occurred!
FATAL ERROR: command exited with status 1: /usr/bin/cmake -DWEST_PYTHON=/usr/bin/python -Bzproj/zephyr/build -GNinja -DBOARD=nxp_adsp_imx8m -Szproj/zephyr/samples/subsys/ipc/openamp_rsc_table

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Let's first make sure this fixes a real issue that can be reproduced in a clean tree.

@iuliana-prodan
Copy link
Collaborator Author

And, since the west build and west sign are done in parallel, the west sign starts before the .elf is done.

No that's not done in parallel, it would be really bad! west sign has an explicit dependency on zephyr.elf as shown in this error message that you shared:

'zephyr/zephyr.elf', needed by 'zephyr/zephyr.ri', missing and no known rule to make it

My first impression is that some examples simply produce no zephyr.elf file for some unknown reason.

The openamp_rsc_table sample has an elf but is called openamp_rsc_table.elf - see https://github.com/zephyrproject-rtos/zephyr/blob/main/samples/subsys/ipc/openamp_rsc_table/prj.conf#L1, not zephyr.elf, as the other samples

@iuliana-prodan
Copy link
Collaborator Author

iuliana-prodan commented Jun 23, 2023

samples/subsys/ipc/openamp_rsc_table/ fails to build much earlier in a clean tree: error: Aborting due to Kconfig warnings. Can you please clone everything from scratch and try to reproduce?

cd zproj/zephyr
git checkout 75dfdb14a1ec
west update
west compare # all clean

rm -rf build/ # pristine works only most of the time
west build -p always -b nxp_adsp_imx8m samples/subsys/ipc/openamp_rsc_table/

-- west build: making build dir zproj/zephyr/build pristine
-- west build: generating a build system
Loading Zephyr default modules (Zephyr base).
-- Application: zproj/zephyr/samples/subsys/ipc/openamp_rsc_table
-- CMake version: 3.26.3
-- Found Python3: /usr/bin/python3.11 (found suitable exact version "3.11.3") found components: Interpreter 
-- Cache files will be written to: .cache/zephyr
-- Zephyr version: 3.4.99 (zproj/zephyr)
-- Found west (found suitable version "1.0.99", minimum required is "0.14.0")
-- Board: nxp_adsp_imx8m
-- ZEPHYR_TOOLCHAIN_VARIANT not set, trying to locate Zephyr SDK
-- Found host-tools: zephyr 0.16.1 (zephyr-sdk-0.16.1)
-- Found toolchain: zephyr 0.16.1 (zephyr-sdk-0.16.1)
-- Found Dtc: /usr/bin/dtc (found suitable version "1.7.0", minimum required is "1.4.6") 
-- Found BOARD.dts: zproj/zephyr/boards/xtensa/nxp_adsp_imx8m/nxp_adsp_imx8m.dts
-- Generated zephyr.dts: zproj/zephyr/build/zephyr/zephyr.dts
-- Generated devicetree_generated.h: zproj/zephyr/build/zephyr/include/generated/devicetree_generated.h
-- Including generated dts.cmake file: zproj/zephyr/build/zephyr/dts.cmake

warning: PRINTK (defined at subsys/debug/Kconfig:203) was assigned the value 'n' but got the value
'y'. See http://docs.zephyrproject.org/latest/kconfig.html#CONFIG_PRINTK and/or look up PRINTK in
the menuconfig/guiconfig interface. The Application Development Primer, Setting Configuration
Values, and Kconfig - Tips and Best Practices sections of the manual might be helpful too.


zproj/zephyr/samples/subsys/ipc/openamp_rsc_table/prj.conf:4: warning: attempt to assign the value 'n' to the undefined symbol PLATFORM_SPECIFIC_INIT
Parsing zproj/zephyr/Kconfig
Loaded configuration 'zproj/zephyr/boards/xtensa/nxp_adsp_imx8m/nxp_adsp_imx8m_defconfig'
Merged configuration 'zproj/zephyr/samples/subsys/ipc/openamp_rsc_table/prj.conf'

error: Aborting due to Kconfig warnings

CMake Error at zproj/zephyr/cmake/modules/kconfig.cmake:343 (message):
  command failed with return code: 1
Call Stack (most recent call first):
  zproj/zephyr/cmake/modules/zephyr_default.cmake:115 (include)
  zproj/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:66 (include)
  zproj/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:92 (include_boilerplate)
  CMakeLists.txt:7 (find_package)


-- Configuring incomplete, errors occurred!
FATAL ERROR: command exited with status 1: /usr/bin/cmake -DWEST_PYTHON=/usr/bin/python -Bzproj/zephyr/build -GNinja -DBOARD=nxp_adsp_imx8m -Szproj/zephyr/samples/subsys/ipc/openamp_rsc_table

With a clean tree, the openamp_rsc_table sample is not compiling since there is no support for nxp_adsp_imx8m.
This sample is supported by STM32 - see https://github.com/zephyrproject-rtos/zephyr/tree/main/samples/subsys/ipc/openamp_rsc_table/boards.

I'm currently working on adding support for nxp_adsp_imx8m. Actually, I'm have all fixed, I'm at the part of code clean-up to put everything, for review, in upstream.
I've updated my repo to latest zephyr and I'm facing the compile error mentioned here

The first problem when compiling openamp_rsc_table sample for nxp_adsp_imx8m is the PLATFORM_SPECIFIC_INIT config, which is ARM specific (see here) and we are compiling the sample for Xtensa HiFi4.
So, there are multiple issues to fix, in order to have this sample working on nxp_adsp_imx8m.

I'll send everything for review, soon.

@iuliana-prodan
Copy link
Collaborator Author

Let's first make sure this fixes a real issue that can be reproduced in a clean tree.

As mentioned here, the openamp_rsc_table will not compile on a clean tree, for nxp_adsp_imx8m.

The fix proposed here, IMO is valid, since the west sign (or any other rimage commands) we want to use only for SOF.
Us, at NXP, we want to run other samples on HiFi4 DSP, not just SOF - https://eoss2023.sched.com/event/1LcNN
That's why I don't want to link all samples that will be compiled for HiFi with rimage (or a dependency for zephyr.ri).

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 23, 2023

The openamp_rsc_table sample has an elf but is called openamp_rsc_table.elf

Thanks! Now I can reproduce with

west build -p always -b nxp_adsp_imx8m samples/hello_world/ -- \
  -DCONFIG_KERNEL_BIN_NAME='"fubar"'

So you can forget about the more complex openamp_rsc_table example in this PR.

Unfortunately sign.py is hardcoded to zephyr.elf but I may have some solution, I should know tomorrow. This needs to be fixed anyway; someone may want to use rimage with a different CONFIG_KERNEL_BIN_NAME.

Us, at NXP, we want to run other samples on HiFi4 DSP, not just SOF - https://eoss2023.sched.com/event/1LcNN
That's why I don't want to link all samples that will be compiled for HiFi with rimage

I understand you don't always want to run rimage but it seems much more flexible to turn rimage on/off with west config rather than a hardcoded and somewhat artificial dependency on CONFIG_SOF.

Also, it would be good to keep the divergence with soc/xtensa/intel_adsp/common/CMakeLists.txt to a minimum so they can be de-duplicated in the future (and rimage is definitely needed to run samples on Intel)

@iuliana-prodan
Copy link
Collaborator Author

I understand you don't always want to run rimage but it seems much more flexible to turn rimage on/off with west config rather than a hardcoded and somewhat artificial dependency on CONFIG_SOF.

I don't know that well west config, so I'm open to suggestions.
Thank you!

Also, it would be good to keep the divergence with soc/xtensa/intel_adsp/common/CMakeLists.txt to a minimum so they can be de-duplicated in the future (and rimage is definitely needed to run samples on Intel)

I agree!

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 23, 2023

I don't know that well west config, so I'm open to suggestions. Thank you!

  • Do NOT "install" rimage in your PATH
  • When you want to use it: west config rimage.path /path/to/build-rimage/rimage
  • When you want to stop using it: west config -d rimage.path.

I recently updated the rimage documentation in commit dd09b04

marc-hb added a commit to marc-hb/zephyr that referenced this pull request Jun 23, 2023
This fixes the build error below reported in zephyrproject-rtos#59603 when
CONFIG_KERNEL_BIN_NAME is used.

```
zephyr/zephyr.elf', needed by 'zephyr/zephyr.ri', missing and no known
rule to make it
```

Note `rimage` is _still_ optional. As before, to avoid using rimage:
- make sure it's not in your PATH
- west config -d rimage.path

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
Copy link
Collaborator

marc-hb commented Jun 23, 2023

Unfortunately sign.py is hardcoded to zephyr.elf but I may have some solution, I should know tomorrow.

CONFIG_KERNEL_BIN_NAME fix submitted:

I understand you don't always want to run rimage but it seems much more flexible to turn rimage on/off with west config rather than a hardcoded and somewhat artificial dependency on CONFIG_SOF.

You later found that the root cause is CONFIG_KERNEL_BIN_NAME and that's unrelated to CONFIG_SOF.

west config offers the flexibility to turn rimage ON/OFF with zero code change. Tying rimage and CONFIG_SOF as this PR does would lose that flexibility: it would not be possible at all to use rimage on any other example/app - whatever its CONFIG_KERNEL_BIN_NAME is. I think that's way too restrictive.

This PR would also make CONFIG_KERNEL_BIN_NAME incompatible with CONFIG_SOF:

west build -p always -b nxp_adsp_imx8 ../modules/audio/sof/app/ --   -DCONFIG_KERNEL_BIN_NAME='"my_bin_name"'
 => build still fails the same even with this PR.

Not like I want to use CONFIG_KERNEL_BIN_NAME (I think it's a bad idea) but this example demonstrates that this PR is not aimed at the root cause.

So a totally different alternative would be:

if(CONFIG_KERNEL_BIN_NAME STREQUAL "zephyr")
  add_custom_command(
    west sign --if-tool-available ...

In other words: invoke west sign only for the CONFIG_urations it supports currently. How about it?

@iuliana-prodan
Copy link
Collaborator Author

Unfortunately sign.py is hardcoded to zephyr.elf but I may have some solution, I should know tomorrow.

CONFIG_KERNEL_BIN_NAME fix submitted:

I understand you don't always want to run rimage but it seems much more flexible to turn rimage on/off with west config rather than a hardcoded and somewhat artificial dependency on CONFIG_SOF.

You later found that the root cause is CONFIG_KERNEL_BIN_NAME and that's unrelated to CONFIG_SOF.

west config offers the flexibility to turn rimage ON/OFF with zero code change. Tying rimage and CONFIG_SOF as this PR does would lose that flexibility: it would not be possible at all to use rimage on any other example/app - whatever its CONFIG_KERNEL_BIN_NAME is. I think that's way too restrictive.

This PR would also make CONFIG_KERNEL_BIN_NAME incompatible with CONFIG_SOF:

west build -p always -b nxp_adsp_imx8 ../modules/audio/sof/app/ --   -DCONFIG_KERNEL_BIN_NAME='"my_bin_name"'
 => build still fails the same even with this PR.

Not like I want to use CONFIG_KERNEL_BIN_NAME (I think it's a bad idea) but this example demonstrates that this PR is not aimed at the root cause.

So a totally different alternative would be:

if(CONFIG_KERNEL_BIN_NAME STREQUAL "zephyr")
  add_custom_command(
    west sign --if-tool-available ...

In other words: invoke west sign only for the CONFIG_urations it supports currently. How about it?

@marc-hb I've tested #59721 on my samples for nxp_adsp_imx8m and is working perfectly.
Thanks for the fix!

@iuliana-prodan
Copy link
Collaborator Author

Closing this since a better fix is in review #59721

marc-hb added a commit to marc-hb/zephyr that referenced this pull request Jun 23, 2023
sign.py, sof/scripts/xtensa-build-zephyr.py and other SOF infrastructure
do not support the (rarely used) CONFIG_KERNEL_BIN_NAME so stop
pretending they do.

This fixes the build error below reported in zephyrproject-rtos#59603 when
CONFIG_KERNEL_BIN_NAME is used:
```
zephyr/zephyr.elf', needed by 'zephyr/zephyr.ri', missing and no known
rule to make it
```

Note `rimage` is _still_ optional, even when CONFIG_KERNEL_BIN_NAME is
not used. As before, to avoid using rimage:
- make sure it's not in your PATH
- west config -d rimage.path

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
Copy link
Collaborator

marc-hb commented Jun 23, 2023

So a totally different alternative would be:
if(CONFIG_KERNEL_BIN_NAME STREQUAL "zephyr")
add_custom_command(
west sign --if-tool-available ...
In other words: invoke west sign only for the CONFIG_urations it supports currently. How about it?

Submitted in:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants