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

os/board/rtl8730e&rtl8721csm: Support CMSIS #6196

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

ZhenBei-Sin
Copy link
Contributor

@ZhenBei-Sin ZhenBei-Sin commented May 24, 2024

external/cmsis: Add CMSIS in the external
Change Notes:

  • Port CMSIS-DSP for rtl8721csm https://github.com/ARM-software/CMSIS-DSP/tree/v1.15.0
    os/arch/arm/include/amebad/cmsis_dsp/*

  • Add the CMSIS-DSP and CMSIS-NN lib to external folder
    external/cmsis_dsp/*
    external/cmsis_nn/*

  • Delete the previous located CMSIS lib from the os folder
    os/arch/arm/src//Make.defs
    os/arch/arm/src/
    /Kconfig
    os/board//src/libs/

apps/examples/board_specific_demo: Add CMSIS example
Change Notes:

  • Add CMSIS-DSP example for rtl8721csm
    apps/examples/board_specific_demo/ameba_cmsis_dsp_demo/*

  • Standard Code Formatting and clean up the naming
    apps/examples/board_specific_demo/ameba_cmsis_nn_demo/example_cmsis_nn.c
    apps/examples/board_specific_demo/ameba_cmsis_nn_demo/Makefile

build/configs/rtl8730e&rtl8721csm: Adjust defconfig for CMSIS
rtl8730e: Support CMSIS-NN
rtl8721csm: Support CMSIS-NN and CMSIS-DSP

Change Notes:

  • Modify the config for the CMSIS macro
    CONFIG_AMEBAx_CMSIS_NN -> CONFIG_EXTERNAL_CMSIS_NN
    CONFIG_EXTERNAL_CMSIS_DSP

Copy link
Contributor

@sunghan-chang sunghan-chang 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 split the commit as 3 commits

  1. add cmsis in the external (including removing from os)
  2. add example in the apps
  3. adjust defconfig for cmsis

build/configs/rtl8730e/flat_dev/defconfig Outdated Show resolved Hide resolved
os/FlatLibs.mk Outdated Show resolved Hide resolved
@ZhenBei-Sin ZhenBei-Sin force-pushed the TP1x_CMSIS_DSP branch 3 times, most recently from ece7f17 to 7691e2c Compare May 30, 2024 05:06
@ZhenBei-Sin ZhenBei-Sin changed the title os/board/rtl8730e&rtl8721csm: Support CMSIS [Under Verification]os/board/rtl8730e&rtl8721csm: Support CMSIS May 30, 2024
@Taejun-Kwon
Copy link
Contributor

In our side, we checked performance became better. How about your side?

@Dane-Kang
Copy link

In our side, we checked performance became better. How about your side?

Hi @Taejun-Kwon , May i know if you tested on loadable app config also ?
In our side, There are link issue like over during build.
#6196 (comment)

So, Zhenbei asked how to resolve it .

@vibhor-m
Copy link
Contributor

vibhor-m commented Jun 4, 2024

May i know if you tested on loadable app config also ?

We tested loadable configuration with cmsis_nn, it was working fine. Now I see @sunghan-chang has suggested not to change os/*.mk files. So we will check the compilation again and share result with you.

@aadotverma
Copy link
Contributor

In our side, we checked performance became better. How about your side?

Hi @Taejun-Kwon , May i know if you tested on loadable app config also ? In our side, There are link issue like over during build. #6196 (comment)

So, Zhenbei asked how to resolve it .

Hello @ZhenBei-Sin,
Please refer the following commit for resolving cmsis library linking issue.
aadotverma@5e52390
Compilation is success for cmsis_nn for both rtl8721csm/hello & rtl8721csm/loadable_apps configuration.
but for cmsis_dsp, multiple definition error is coming for rtl8721csm/loadable_apps.
I checked, some symbols are defined two times in cmsis_dsp library.

@ZhenBei-Sin
Copy link
Contributor Author

In our side, we checked performance became better. How about your side?

Hi @Taejun-Kwon , May i know if you tested on loadable app config also ? In our side, There are link issue like over during build. #6196 (comment)
So, Zhenbei asked how to resolve it .

Hello @ZhenBei-Sin, Please refer the following commit for resolving cmsis library linking issue. aadotverma@5e52390 Compilation is success for cmsis_nn for both rtl8721csm/hello & rtl8721csm/loadable_apps configuration. but for cmsis_dsp, multiple definition error is coming for rtl8721csm/loadable_apps. I checked, some symbols are defined two times in cmsis_dsp library.

Hi @aadotverma

Thank you for assist to archive cmsis lib into libexternal.a for loadable_apps.

I updated the latest lib_cmsis_dsp.a, which solved the multiple definition error.
Root Cause: Include the .c multiple time cause multiple definition (example: ComplexMathFunctions.c)
https://github.com/ARM-software/CMSIS-DSP/blob/v1.15.0/Source/ComplexMathFunctions/ComplexMathFunctions.c

@ZhenBei-Sin
Copy link
Contributor Author

Let's split the commit as 3 commits

  1. add cmsis in the external (including removing from os)
  2. add example in the apps
  3. adjust defconfig for cmsis

Solved the link lib for loadable_apps.
Will proceed to resolve the conflicts and separate to 3 commits.

Change Notes:
- Port CMSIS-DSP for rtl8721csm https://github.com/ARM-software/CMSIS-DSP/tree/v1.15.0
  os/arch/arm/include/amebad/cmsis_dsp/*

- Add the CMSIS-DSP and CMSIS-NN lib to external folder
  external/cmsis_dsp/*
  external/cmsis_nn/*

- Delete the previous located CMSIS lib from the os folder
  os/arch/arm/src/*/Make.defs
  os/arch/arm/src/*/Kconfig
  os/board/*/src/libs/*
Change Notes:
- Add CMSIS-DSP example for rtl8721csm
  apps/examples/board_specific_demo/ameba_cmsis_dsp_demo/*

- Standard Code Formatting and clean up the naming
  apps/examples/board_specific_demo/ameba_cmsis_nn_demo/example_cmsis_nn.c
  apps/examples/board_specific_demo/ameba_cmsis_nn_demo/Makefile
rtl8730e: Support CMSIS-NN
rtl8721csm: Support CMSIS-NN and CMSIS-DSP

Change Notes:
- Modify the config for the CMSIS macro
 CONFIG_AMEBAx_CMSIS_NN -> CONFIG_EXTERNAL_CMSIS_NN
 CONFIG_EXTERNAL_CMSIS_DSP
@ZhenBei-Sin ZhenBei-Sin changed the title [Under Verification]os/board/rtl8730e&rtl8721csm: Support CMSIS os/board/rtl8730e&rtl8721csm: Support CMSIS Jun 10, 2024
@ZhenBei-Sin
Copy link
Contributor Author

Hi @sunghan-chang , @Taejun-Kwon , @vibhor-m , @aadotverma

Please help to review the final changes.
Thank You.

@Taejun-Kwon Taejun-Kwon merged commit 258b593 into Samsung:master Jun 12, 2024
11 checks passed
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.

7 participants