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

ARM: Move CMSIS out of main tree #23373

Closed
ioannisg opened this issue Mar 10, 2020 · 24 comments · Fixed by #23610
Closed

ARM: Move CMSIS out of main tree #23373

ioannisg opened this issue Mar 10, 2020 · 24 comments · Fixed by #23610
Labels
area: ARM ARM (32-bit) Architecture Feature A planned feature with a milestone
Milestone

Comments

@ioannisg
Copy link
Member

Introduction

This is a proposal to move the CMSIS out of the main Zephyr repository.

Problem description

CMSIS is one of the two "hal" layers that are still part of the main Zephyr tree, residing inside the /ext folder.

Proposed change

We would like to remove the ARM CMSIS "hal" layer from the /ext directory of the Zephyr main tree and instead, place it in a "hal" repository in the Zephyrproject organization, which, ideally, will mirror the ARM CMSIS repository.

Detailed RFC

The repository content could be fetched using the west tool, as we do for all other HAL layers.

Dependencies

Not applicable.

Alternatives

Keep having the CMSIS as part of the main zephyr repository.

@ioannisg ioannisg added area: ARM ARM (32-bit) Architecture RFC Request For Comments: want input from the community labels Mar 10, 2020
@ioannisg
Copy link
Member Author

FYI @galak @stephanosio @MaureenHelm @carlescufi @erwango

Is this something we should try to agree upon and do for v2.3 release?

@carlescufi
Copy link
Member

FYI @galak @stephanosio @MaureenHelm @carlescufi @erwango

Is this something we should try to agree upon and do for v2.3 release?

There was a "requirement" a while back that one should be able to build for qemu without any additional modules/repos. This is the reason we keep CMSIS in the tree. Perhaps @nashif can comment since I recall him being the one that first introduced this concept (which kind of makes sense to me as well).

@ioannisg
Copy link
Member Author

It is not documented, anywhere that we need to build QEMU without additional repositories (to the best of my knowledge, of course)

Anyway, this does not even hold, today, at least for qemu_cortex_m0 (Nordic) and qemu_cortex_m3 (TI) that, I believe, require vendor HALs.

@carlescufi
Copy link
Member

Anyway, this does not even hold, today, at least for qemu_cortex_m0 (Nordic) and qemu_cortex_m3 (TI) that, I believe, require vendor HALs.

Not for qemu_cortex_m3. The UART driver is in-tree.

@ioannisg
Copy link
Member Author

Ah, OK. Are you sure this Soc/Board does not need TI HALs at all?

In any case the argument holds for the M0 Qemu

@stephanosio
Copy link
Member

Unless there is something wrong with my current understanding of how things work, HALs (including CMSIS) need not reside in the main repository.

The first thing that CI does is to west update and this means that everything (including the to-be-modular-ised CMSIS) will get pulled before the CI builds tests and samples.

@carlescufi
Copy link
Member

Unless there is something wrong with my current understanding of how things work, HALs (including CMSIS) need not reside in the main repository.

The first thing that CI does is to west update and this means that everything (including the to-be-modular-ised CMSIS) will get pulled before the CI builds tests and samples.

Yes, but the idea here was that downstream distros of zephyr that do not necessarily include those HALs can still run the qemu basic kernel tests.

@carlescufi
Copy link
Member

Ah, OK. Are you sure this Soc/Board does not need TI HALs at all?

Almost sure.

In any case the argument holds for the M0 Qemu

Right, but this is new.

@stephanosio
Copy link
Member

stephanosio commented Mar 10, 2020

Re: CMSIS

  • CMSIS consists of many different components (e.g. Core, DSP, NN), and the current Zephyr EXT CMSIS only includes the CMSIS-Core component.
  • CMSIS codebase, including all the applicable components, is quite large and converting it into a module is a reasonable approach to take.
  • There is already a pending PR to add the CMSIS-DSP component (Add CMSIS-DSP #21600) and it has been requested the CMSIS be moved out into a separate repo/module for this.
  • Simply mirroring and patching the upstream CMSIS repository may not suffice for modularisation due to the following reasons:
    • There will be many unnecessary components included (e.g. CoreValidation, DAP, Driver, RTOS, RTOS2).
    • The CMSIS-DSP (digital signal processing) and CMSIS-NN (neural network), which are primarily of interest, use CMake-based build system and this will be directly in conflict with the Zephyr build system (unless we nuke them all in the patches, but this will not be crucial to maintain in the long term).

@ioannisg
Copy link
Member Author

@carlescufi FYI there will be one more QEMU platform that will probably require external HAL (STM32) support; the Cortex-M4F: #23185.

@carlescufi
Copy link
Member

@stephanosio @ioannisg I have no objections to going forward with this. We should create a new repo for CMSIS that contains all the other applicable components as well (DSP in particular is of interest to many). I would however like to hear from @MaureenHelm, @galak and @nashif before we go ahead.

@carlescufi
Copy link
Member

@stephanosio note that for CMSIS-DSP we will need to settle this first:

#7516

A discussion is ongoing at the board level regarding this.

@stephanosio
Copy link
Member

stephanosio commented Mar 19, 2020

@stephanosio note that for CMSIS-DSP we will need to settle this first:

#7516

A discussion is ongoing at the board level regarding this.

@carlescufi In #21600, I have already integrated CMSIS-DSP to the Zephyr build system (with full Kconfig support for customisation) in the source code form and linking binary blobs will not be necessary; I suppose #7516 won't be relevant in that case?

In terms of licensing, all CMSIS components (including the CMSIS-DSP) are licensed Apache 2.0.

p.s. of course, users will be able to simply disable the Kconfig option for this and use the binary blobs provided by the SoC vendor if they wish.

@galak
Copy link
Collaborator

galak commented Mar 19, 2020

I'd ask if we need the qemu_cortex_m3 platform at all. Is mps2_an385 sufficient?

@stephanosio
Copy link
Member

I'd ask if we need the qemu_cortex_m3 platform at all. Is mps2_an385 sufficient?

@galak #22870

@carlescufi
Copy link
Member

@carlescufi In #21600, I have already integrated CMSIS-DSP to the Zephyr build system (with full Kconfig support for customisation) in the source code form and linking binary blobs will not be necessary; I suppose #7516 won't be relevant in that case?

@stephanosio maybe I'm missing something, so CMSIS-DSP is offered both in source code and library form in the CMSIS repo? I thought it was only available as libraries.

@carlescufi
Copy link
Member

I'd ask if we need the qemu_cortex_m3 platform at all. Is mps2_an385 sufficient?

Agreed with @stephanosio. Once Cortex-M4 is supported in qemu, m3 is not required at all.

@stephanosio
Copy link
Member

@stephanosio maybe I'm missing something, so CMSIS-DSP is offered both in source code and library form in the CMSIS repo? I thought it was only available as libraries.

Yep, the upstream CMSIS repository contains both the source code and pre-built libs for the CMSIS-DSP.

The source code can be found here:
https://github.com/ARM-software/CMSIS_5/tree/develop/CMSIS/DSP/Source

@nashif
Copy link
Member

nashif commented Mar 19, 2020

I'd ask if we need the qemu_cortex_m3 platform at all. Is mps2_an385 sufficient?

We tried to remove it when we enabled mps2_an385 but there was some resistance due to some drivers and network tests depending on it if I remember correctly. IMO we should consolidate this and cleanup many of the qemu duplicates we have now, also for x86.

@nashif
Copy link
Member

nashif commented Mar 19, 2020

if we are going to remove cmsis and put it in a module, we should also remove the altera hal at the same time and probably other items in ext and complete abolish "ext/"

@ioannisg
Copy link
Member Author

For ARMv7-M things are pretty clear; we don't need qemu_cortex_m3, we have mps2_an385 for a Cortex-M3 and we can make it run without MPU. We want to have Cortex-M4F - that's what #23185 will do.

@ioannisg
Copy link
Member Author

if we are going to remove cmsis and put it in a module, we should also remove the altera hal at the same time and probably other items in ext and complete abolish "ext/"

Any particular reason these things need to happen "at the same time"? CMSIS and altera-hal are totally unrelated. Agree with abolishing /ext if possible - just that I am more keen of doing things in steps

@galak
Copy link
Collaborator

galak commented Mar 19, 2020

We need to sync whatever the CMSIS HAL repo would be w/TFM's CMSIS usage as well.

@carlescufi
Copy link
Member

@stephanosio @ioannisg I will move forward with this. I think it should be done in steps:

  1. Create the repo
  2. Populate it with CMSIS-Core
  3. Move CMSIS-Core from ext/ to the repo
  4. Populate the repo with CMSIS-DSP
  5. Add CMSIS-DSP support in Zephyr

@ioannisg ioannisg added the Feature A planned feature with a milestone label Mar 19, 2020
@ioannisg ioannisg added this to the v2.3.0 milestone Mar 19, 2020
@ioannisg ioannisg removed the RFC Request For Comments: want input from the community label Mar 19, 2020
carlescufi added a commit to carlescufi/zephyr that referenced this issue Mar 19, 2020
Use an external CMSIS repo instead of keeping a copy in ext/hal.

Closes zephyrproject-rtos#23373

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
carlescufi added a commit that referenced this issue Mar 19, 2020
Use an external CMSIS repo instead of keeping a copy in ext/hal.

Closes #23373

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
hakehuang pushed a commit to hakehuang/zephyr that referenced this issue Apr 30, 2020
Use an external CMSIS repo instead of keeping a copy in ext/hal.

Closes zephyrproject-rtos#23373

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ARM ARM (32-bit) Architecture Feature A planned feature with a milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants