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

boards: Add support for the NXP MIMXRT595 DSP core #61356

Merged
merged 2 commits into from
Oct 25, 2023

Conversation

dluke62
Copy link
Contributor

@dluke62 dluke62 commented Aug 10, 2023

Add board and soc files for the audio DSP in NXP MIMXRT595.

Copy link
Contributor

@DerekSnell DerekSnell left a comment

Choose a reason for hiding this comment

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

Hi @dluke62 ,
Thank you for contributing this! Can you fix the compliance issues? I also have some quick feedback of the SOC series naming convention we need to change. I'm sure we will have more feedback to come. Thank you

@dbaluta
Copy link
Collaborator

dbaluta commented Aug 10, 2023

Also, I guess you will need to add at least some compile time tests. @iuliana-prodan care to provide some guidance here on how did you do this for imx8m_adsp?

soc/xtensa/nxp_adsp/Kconfig Outdated Show resolved Hide resolved
soc/xtensa/nxp_adsp/imx8m/Kconfig.defconfig.series Outdated Show resolved Hide resolved
@iuliana-prodan
Copy link
Collaborator

Also, I guess you will need to add at least some compile time tests. @iuliana-prodan care to provide some guidance here on how did you do this for imx8m_adsp?

You can have a simple compilation test by adding a yaml file in the boards folder for your target: see https://docs.zephyrproject.org/latest/develop/test/twister.html and an example: https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/xtensa/nxp_adsp_imx8m/nxp_adsp_imx8m.yaml

@dluke62
Copy link
Contributor Author

dluke62 commented Aug 10, 2023

@iuliana-prodan , @dbaluta if I understand correctly, we can't have any compile time tests because we can't compile for RT595 DSP without downloading Xtensa toolchain separately (unlike imx8, which has toolchain in Zephyr SDK). So the CI won't be able to compile Zephyr for RT595 DSP. Is that correct?

@dluke62 dluke62 force-pushed the dluke62/nxp_rt595_adsp_board branch from 7fcc0f3 to e7079a1 Compare August 10, 2023 18:51
@dleach02
Copy link
Member

@stephanosio how would one submit an enablement to Zephyr upstream if there isn't an open toolchain in our CI?

@dbaluta
Copy link
Collaborator

dbaluta commented Aug 11, 2023

@iuliana-prodan , @dbaluta if I understand correctly, we can't have any compile time tests because we can't compile for RT595 DSP without downloading Xtensa toolchain separately (unlike imx8, which has toolchain in Zephyr SDK). So the CI won't be able to compile Zephyr for RT595 DSP. Is that correct?

Technically, once you have the Xtensa toolchain configuration you can use that to create the corresponding open toolchain and integrate it in Zephyr SDK.

For example, here is how imx8 toolchain was added:

https://github.com/zephyrproject-rtos/sdk-ng/pull/187/commits

Another clean PR for adding a toolchain for a new board is here:

zephyrproject-rtos/sdk-ng#694

@dluke62
Copy link
Contributor Author

dluke62 commented Aug 11, 2023

@iuliana-prodan , @dbaluta if I understand correctly, we can't have any compile time tests because we can't compile for RT595 DSP without downloading Xtensa toolchain separately (unlike imx8, which has toolchain in Zephyr SDK). So the CI won't be able to compile Zephyr for RT595 DSP. Is that correct?

Technically, once you have the Xtensa toolchain configuration you can use that to create the corresponding toolchain and integrate it in Zephyr SDK.

For example, here is how imx8 toolchain was added:

https://github.com/zephyrproject-rtos/sdk-ng/pull/187/commits

Another clean PR for adding a toolchain for a new board is here:

zephyrproject-rtos/sdk-ng#694

Thank you! As far as I understand the toolchain/core configuration is not freely available - one has to create an account with Cadence to get it. I don't know if we can simply check it in here - I'm assuming it comes with its own license, user agreement, etc. @DerekSnell could you please chime in?

@dbaluta
Copy link
Collaborator

dbaluta commented Aug 11, 2023

Thank you! As far as I understand the toolchain/core configuration is not freely available - one has to create an account with Cadence to get it. I don't know if we can simply check it in here - I'm assuming it comes with its own license, user agreement, etc. @DerekSnell could you please chime in?

Toolchain is not freely available but you don't need that.

You just need the configuration files that are created by hardware integration team and I think Cadence understoond the value of making that configuration open.

So, for example here is the license of one of the headers in the configuration files:

image

zephyrproject-rtos/sdk-ng@e6d3435#diff-293f51bf819c4d20c9fc914c7c5b1be732cb1238523bd1702e68b3e1fa9aed14

If you can send me your toolchain archive I can try to make a PR to integrate the configuration in Zephyr SDK so that we can build an open toolchain.

Zephyr uses crosstool-ng to build the toolchain based on the configuration files you provide.

@dluke62 How do you actually compile the code now?

Copy link
Contributor

@DerekSnell DerekSnell left a comment

Choose a reason for hiding this comment

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

Hi @dluke62 ,
Sorry, yesterday I requested these changes when I submitted my review before you pushed an update. But it appears GitHub did not capture them (probably my fault).

We need to change the naming of the SOC series. RT595 is the SOC derivative in the i.MX RT500 series, which in Zephyr is named RT5xx. NXP may offer other RT5xx part numbers in this series using the same F1 DSP core, which would share this code. So my changes keep the RT595 name when referencing the SOC, but use RT5xx when referencing the series.

Thanks again for your contribution!

boards/arm/mimxrt595_evk/doc/index.rst Outdated Show resolved Hide resolved
boards/arm/mimxrt595_evk/doc/index.rst Outdated Show resolved Hide resolved
@@ -0,0 +1,46 @@
# Copyright (c) 2023 Google LLC.
Copy link
Contributor

Choose a reason for hiding this comment

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

This folder name rt595 should be changed to rt5xx for the SOC series using this F1 DSP core, to enable other RT500 part numbers using the same DSP core.

soc/xtensa/nxp_adsp/rt595/Kconfig.defconfig.series Outdated Show resolved Hide resolved
soc/xtensa/nxp_adsp/rt595/Kconfig.defconfig.series Outdated Show resolved Hide resolved
soc/xtensa/nxp_adsp/rt595/Kconfig.soc Outdated Show resolved Hide resolved
boards/xtensa/nxp_adsp_rt595/Kconfig.board Outdated Show resolved Hide resolved
boards/xtensa/nxp_adsp_rt595/nxp_adsp_rt595_defconfig Outdated Show resolved Hide resolved
soc/xtensa/nxp_adsp/CMakeLists.txt Outdated Show resolved Hide resolved
soc/xtensa/nxp_adsp/rt595/Kconfig.series Outdated Show resolved Hide resolved
@dbaluta
Copy link
Collaborator

dbaluta commented Aug 16, 2023

@dluke62 some questions for you when you get back to this:

  • how do you load the code and start the FW?
  • what is your plan on building a toolchain configuration inside Zephyr SDK. My suggestion is to start with the configuration you have for your internal Xtensa build and open that up as described here xtensa: Add mt8195_adsp toolchain sdk-ng#694. Or just send me the configuration and will take care about this.

@dluke62 dluke62 force-pushed the dluke62/nxp_rt595_adsp_board branch from 414d0b5 to f681aae Compare August 18, 2023 00:21
@hakehuang
Copy link
Collaborator

@hakehuang can you please try again.

#63095

is merged.

@dbaluta , you still need update the west.yml to direct to the right hal_nxp commit.

@iuliana-prodan
Copy link
Collaborator

iuliana-prodan commented Sep 27, 2023

@hakehuang can you please try again.
#63095
is merged.

@dbaluta , you still need update the west.yml to direct to the right hal_nxp commit.

@hakehuang the hal_nxp from west.yml has all the needed support for this PR. So, no changes there.
Can you please try to run CI on this PR?

@dluke62 can you rebase or amend this PR in order to trigger the Zephyr CI.

@hakehuang
Copy link
Collaborator

hakehuang commented Sep 27, 2023

@hakehuang the hal_nxp from west.yml has all the needed support for this PR. So, no changes there.
Can you please try to run CI on this PR?

@iuliana-prodan in this case, you need rebase this PR, I try to rebase for you, but I just not get permission.

besides, as you do not use sysbuild to make M7 to boot rt595 core, I have no way to run your case, so I can only do build checking. can you double check the run at your side

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 27, 2023

I try to rebase for you, but I just not get permission.

Indeed you cannot change someone else's clone.

The way to solve this problem (for instance: when someone is out of office), is to fetch their branch (git fetch origin pull/61356/head) and submit a new PR. This preserves their name on the commit messages.

@iuliana-prodan
Copy link
Collaborator

I try to rebase for you, but I just not get permission.

Indeed you cannot change someone else's clone.

The way to solve this problem (for instance: when someone is out of office), is to fetch their branch (git fetch origin pull/61356) and submit a new PR. This preserves their name.

@hakehuang this is not my PR.
Let's wait a bit for @dluke62, maybe he can update this (/his) PR to trigger the CI.
All I can do is what @marc-hb suggested above, but I would like to wait for @dluke62 :)
I don't want to duplicate this PR.

@dluke62
Copy link
Contributor Author

dluke62 commented Sep 27, 2023

I try to rebase for you, but I just not get permission.

Indeed you cannot change someone else's clone.
The way to solve this problem (for instance: when someone is out of office), is to fetch their branch (git fetch origin pull/61356) and submit a new PR. This preserves their name.

@hakehuang this is not my PR. Let's wait a bit for @dluke62, maybe he can update this (/his) PR to trigger the CI. All I can do is what @marc-hb suggested above, but I would like to wait for @dluke62 :) I don't want to duplicate this PR.

thank you guys! I am rebasing the PR, will update it soon.

In preparation for RT500 ADSP enablement, consolidate common Xtensa
configuration parameters in top level Kconfig.defconfig.

Signed-off-by: Dmitry Lukyantsev <[email protected]>
@marc-hb marc-hb dismissed their stale review September 27, 2023 16:06

very out of date review, apologies

dbaluta
dbaluta previously approved these changes Sep 27, 2023
@dluke62 dluke62 requested a review from nashif September 27, 2023 17:54
iuliana-prodan
iuliana-prodan previously approved these changes Sep 27, 2023
@hakehuang
Copy link
Collaborator

hakehuang commented Sep 28, 2023

a build warning:

-- Including generated dts.cmake file: /home/shared/disk/zephyr_project/zephyr_test/zephyr/samples/hello_world/build/zephyr/dts.cmake
/home/shared/disk/zephyr_project/zephyr_test/zephyr/samples/hello_world/build/zephyr/zephyr.dts:18.8-26.5: Warning (simple_bus_reg): /soc/cpus: missing or empty reg/ranges property
Parsing /home/shared/disk/zephyr_project/zephyr_test/zephyr/Kconfig

others are ok

scripts/twister -p nxp_adsp_rt595 -T tests/kernel/ --build-only
Renaming output directory to /home/shared/disk/zephyr_project/zephyr_test/zephyr/twister-out.1
INFO    - Using Ninja..
INFO    - Zephyr version: zephyr-v3.4.0-4456-ge936ecbb074d
INFO    - Using 'zephyr' toolchain.
INFO    - Building initial testsuite list...
INFO    - Writing JSON report /home/shared/disk/zephyr_project/zephyr_test/zephyr/twister-out/testplan.json
INFO    - JOBS: 16
INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue
INFO    - Total complete:  177/ 177  100%  skipped:   90, failed:    0, error:    0
INFO    - 177 test scenarios (177 test instances) selected, 90 configurations skipped (59 by static filter, 31 at runtime).
INFO    - 87 of 177 test configurations passed (100.00%), 0 failed, 0 errored, 90 skipped with 0 warnings in 380.11 seconds
INFO    - 0 test configurations executed on platforms, 87 test configurations were only built.
INFO    - Saving reports...
INFO    - Writing JSON report /home/shared/disk/zephyr_project/zephyr_test/zephyr/twister-out/twister.json
INFO    - Writing xunit report /home/shared/disk/zephyr_project/zephyr_test/zephyr/twister-out/twister.xml...
INFO    - Writing xunit report /home/shared/disk/zephyr_project/zephyr_test/zephyr/twister-out/twister_report.xml...
INFO    - Run completed

Copy link
Collaborator

@LaurentiuM1234 LaurentiuM1234 left a comment

Choose a reason for hiding this comment

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

A few nitpicks/suggestions

@dbaluta
Copy link
Collaborator

dbaluta commented Oct 3, 2023

@nashif I think we are ready to merge this. @LaurentiuM1234 we can fix the nitpicks in the future PRs.

@dluke62 dluke62 dismissed stale reviews from iuliana-prodan and dbaluta via b3f4a75 October 5, 2023 23:33
@dluke62 dluke62 force-pushed the dluke62/nxp_rt595_adsp_board branch from e936ecb to b3f4a75 Compare October 5, 2023 23:33
@dluke62
Copy link
Contributor Author

dluke62 commented Oct 5, 2023

a build warning:

-- Including generated dts.cmake file: /home/shared/disk/zephyr_project/zephyr_test/zephyr/samples/hello_world/build/zephyr/dts.cmake
/home/shared/disk/zephyr_project/zephyr_test/zephyr/samples/hello_world/build/zephyr/zephyr.dts:18.8-26.5: Warning (simple_bus_reg): /soc/cpus: missing or empty reg/ranges property
Parsing /home/shared/disk/zephyr_project/zephyr_test/zephyr/Kconfig

others are ok

scripts/twister -p nxp_adsp_rt595 -T tests/kernel/ --build-only
Renaming output directory to /home/shared/disk/zephyr_project/zephyr_test/zephyr/twister-out.1
INFO    - Using Ninja..
INFO    - Zephyr version: zephyr-v3.4.0-4456-ge936ecbb074d
INFO    - Using 'zephyr' toolchain.
INFO    - Building initial testsuite list...
INFO    - Writing JSON report /home/shared/disk/zephyr_project/zephyr_test/zephyr/twister-out/testplan.json
INFO    - JOBS: 16
INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue
INFO    - Total complete:  177/ 177  100%  skipped:   90, failed:    0, error:    0
INFO    - 177 test scenarios (177 test instances) selected, 90 configurations skipped (59 by static filter, 31 at runtime).
INFO    - 87 of 177 test configurations passed (100.00%), 0 failed, 0 errored, 90 skipped with 0 warnings in 380.11 seconds
INFO    - 0 test configurations executed on platforms, 87 test configurations were only built.
INFO    - Saving reports...
INFO    - Writing JSON report /home/shared/disk/zephyr_project/zephyr_test/zephyr/twister-out/twister.json
INFO    - Writing xunit report /home/shared/disk/zephyr_project/zephyr_test/zephyr/twister-out/twister.xml...
INFO    - Writing xunit report /home/shared/disk/zephyr_project/zephyr_test/zephyr/twister-out/twister_report.xml...
INFO    - Run completed

Thank you, fixed the warning.

Add board and soc files for the NXP MIMXRT595 DSP core.

Signed-off-by: Dmitry Lukyantsev <[email protected]>
@dluke62 dluke62 force-pushed the dluke62/nxp_rt595_adsp_board branch from b3f4a75 to e3815af Compare October 5, 2023 23:41
@dluke62
Copy link
Contributor Author

dluke62 commented Oct 9, 2023

@nashif would you mind taking another look? CI is now green.

@dluke62
Copy link
Contributor Author

dluke62 commented Oct 23, 2023

Hi @dbaluta! Do you know if there is a way to move forward with this PR? Are we blocked by Anas's change request?

@dbaluta
Copy link
Collaborator

dbaluta commented Oct 24, 2023

@dluke62 I talked with @nashif on discord and he will pick this up once the 3.5 release is out. Will ping him again.

@dluke62
Copy link
Contributor Author

dluke62 commented Oct 24, 2023

@dluke62 I talked with @nashif on discord and he will pick this up once the 3.5 release is out. Will ping him again.

thank you!!

@nashif nashif merged commit 357d6ce into zephyrproject-rtos:main Oct 25, 2023
18 checks passed
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.