Feature/use one core folder for all

[Masmiseim36]

This pull request has come out of the discussion with @JonatanAntoni in #12.
This pull request is not yet fully perfected and is mainly intended to clarify and discuss the concept of a shared core folder.

[JonatanAntoni]

Hey Markus,

This looks really great at the first glance! Thank you very much for your time contributing this. I think we can hardly assure that we don't break anything; even if the Core Validation tests are still working. But I guess this is the best we can do.

Let us carefully review the changes and decide if we want to take the remaining risk in favour of the better maintainability.

Thanks again,
Jonatan

[JonatanAntoni]

Just my 50ct regarding file naming. Any specific reason for going away from short cmsis_compiler.h for the top-level entry? If we'd keep this as is, it might easy updating existing projects.

And, as a proper detection of target architecture might be compiler specific, why not keep the "generic compiler headers" named like cmsis_<compiler>.h and branch into architecture specifics from there? E.g., cmsis_<compiler>_<arch>.h.

[Masmiseim36]

Hello @JonatanAntoni,
The file names are still a leftover from the pull request with dedicated folders for each architecture. But you are right, that is no longer necessary here and rather confusing. I have therefore renamed the files according to your suggestion.

[JonatanAntoni]

Generating the pack failed during packchk validation. Could you please try to run the gen_pack.sh and fix all the issues?

[Masmiseim36]

I will take a look, but I will need some more time for this.

[Masmiseim36]

@JonatanAntoni It works on my machine now:

Pack ARM.CMSIS.6.0.0-dev33+gc3472d4.pack generated successfully!

But I think there is an additional issue. In the moment there are two doxygen configuration. One for the core folder and another one for the core_A folder. I haven’t checked, if the second configuration can be removed or if further adaptions need to be done.
I take a look.

[Masmiseim36]

I removed some doxygen warnings. The documentation seems to be okay as far as I can tell.

[ghost]

Hello @Masmiseim36,
I just started to take a look at the changes which are introduced with this PR.
It is not only 'one core folder for all' but it is also the introduction of additional Cortex-A cores and basic support for Cortex-R cores.

At the first check I only have some minor complaints like:

• different function doxygen description style is used between cmsis_corea.h and cmsis_corem.h files.
  using the same stile eases a compare.
• tiarmclang support is different compared to armclang support.
  tiarmclang should also provide the file cmsis_tiarmclang.h regardless if only Cortex-M is supported.

[Masmiseim36]

Hello @GuentherMartin .

Thanks for the feedback.

• I added the additional Cortex-A and Cortex-R cores to get a better idea how the file structure should look like. If we agree on the structure, I can also change it to two pull requests, one for the structural change and a second for the additional cores. Which would you prefer?

• I'm not quite sure what differences in the doxygen comments you are referring to. I only see one additional line break. Which of the two is the preferred?

• Correct tiarmclang I would still need to do an equivalent restructuring. But the shared part looks identical to armclang. Would it be ok in cmsis_tiarmclang_corem.h to include the file cmsis_armclang.h? Or should I create the file cmsis_tiarmclang.h and include cmsis_armclang.h in it?

Regards
Markus

[JonatanAntoni]

• Correct tiarmclang I would still need to do an equivalent restructuring. But the shared part looks identical to armclang. Would it be ok in cmsis_tiarmclang_corem.h to include the file cmsis_armclang.h? Or should I create the file cmsis_tiarmclang.h and include cmsis_armclang.h in it?

I'd not mix different compilers. Yes, most of the clang-based compilers will have shared stuff. If we figure out that we can reuse major parts, I'd go for another cmsis_llvm.h for instance to cover the stuff that comes from upstream, but not being a full compiler header on its own.

[Masmiseim36]

I have split tiarmclang into a generic and a cortex-m specific part. As expected, the file cmsis_armclang.h is almost identical to cmsis_tiarmclang.h.

@JonatanAntoni , I saw that you are working on an implementation for clang on branch llvm. Do you want to use this code to abstract the generic parts of all clang variants?

[JonatanAntoni]

@Masmiseim36, yes, I am adding a cmsis_clang.h for ARM-software/LLVM-embedded-toolchain-for-Arm. Let me merge that patch and then we can check about the commonalities between Arm Clang, TI Clang and "Vanilla" Clang. Common parts may go into cmsis_llvm.h we include into all three. Does this make sense to you?

[Masmiseim36]

That makes sense.

[EdmundPlayer]

Hi everyone,

Thanks for raising this request @Masmiseim36. I really like the idea of tidying up the CMSIS repository so that it's more future-proof for newer cores and CMSIS v6 seems like a good opportunity to make such changes, if there's enough time/resources.

I have no problem with having a single Core folder for:

• all existing processor core headers, e.g., core_cm3.h
• new processor core headers, including those associated with R-profile and A-profile, if that's something useful for device end users working with A-profile/R-profile devices in the MCU/Embedded/IoT field, or even the smaller set of users working on things like validation/verification for new A-profile/R-profile devices.

I think that we need to be careful about file naming because there are some architectural features like the PMU that span different architectures. Another example is the MPU that can be used in both M-profile and R-profile systems. At the moment we have files liked pmu_v8.h for M-profile. I would like to see less code in the core_cmx.h headers and more files like pmu_v8.h in the future, i.e., creating more architectural header files, in particular for architectural features that are optional, but also to avoid duplicating code across processor core headers.

We could rename such files to something more relevant, e.g., pmu_v8.h could become pmu_v8.1-m.h. However, I would imagine that doing this might potentially break documentation and might cause build failures in existing projects that upgrade to the newer version of CMSIS.

Perhaps a better option would be to create three separate folders within the Core directory, for example:

Core
     core_cm<x>.h
     :
     core_cr<x>.h
     :
     core_ca<x>.h
     :
     A-profile (new folder)
     R-profile (new folder)
     M-profile (new folder)
           cache1_armv7.h
           pmu_v8.h
           mpu_v8.h
           :

Each of these folders we would store architecture specific files like mpu_v8.h, and so on. The M-profile folder name relates to the official names for the architecture profiles, e.g., https://developer.arm.com/Architectures/M-Profile%20Architecture. Therefore, it is clear to users that the top-level contains processor core files, and the sub-folders contain architectural information that the processor implementations inherit.

Potentially these folders could be used for any compiler-specific headers for each architecture too. A top-level compiler file could perhaps still exist in the Core folder.

Each processor core header would need to be updated to include the new folder path to the architectural headers.

This is just one option and there are probably many different opinions on the exact structure and naming that should be used. I think that the main thing I want to get across here is that having some folders for architecture specific information means that we're not crossing the streams too much (apologies for Ghostbuster quote).

Best regards,

Ed

P.S. In the longer term, probably after CMSIS v6 has been released, I would like to see less information in the processor core headers and just information that is implementation-specific to the Arm processor. A lot of the current code/information is architectural-specific and is duplicated across different core headers, e.g., NVIC registers and functions. Even the existing mpu_v7.h and mpu_v8.h headers do not include all of the MPU code. For instance, in theory we could move the MPU structs from the core headers into the relevant MPU headers too. The same goes for the PMU. Anyway, this is something we can raise via a separate pull request and do bit-by-bit.

[Masmiseim36]

Hello @EdmundPlayer

thanks for your detailed input. Appreciate that.

“We could rename such files to something more relevant, e.g., pmu_v8.h could become pmu_v8.1-m.h. However, I would imagine that doing this might potentially break documentation and might cause build failures in existing projects that upgrade to the newer version of CMSIS.”

I do not quite understand how such a renaming (pmu_v8.h --> pmu_v8.1-m.h) should lead to a problem. In a CMSIS compliant usage, a <device>.h file specific to a controller is provided by the silicon vendor. The generic core_<cpu>.h file from the CMSIS library should be included in this file. See also: https://arm-software.github.io/CMSIS_5/Core/html/templates_pg.html
No direct access to other files is allowed. There should be no problem if files included in core_<cpu>.h are renamed. The only access allowed is via the files in the CMSIS Core Processor Files table (--> https://arm-software.github.io/CMSIS_5/Core/html/templates_pg.html#CMSIS_Processor_files).

„Perhaps a better option would be to create three separate folders within the Core directory(…). Each of these folders we would store architecture specific files like mpu_v8.h, and so on.”

Where would a feature be stored that is used by several architectures? For example, the GIC (A and R) or MPU (M and R).
May I make an alternative suggestion for the folder structure? To tidy up the core folder a bit, the feature-specific files and the compiler-specific files could be moved to separate subfolders. Something like this:

Core
     core_cm<x>.h
     :
     core_cr<x>.h
     :
     core_ca<x>.h
     :
     Compiler
           cmsis_compiler.h
           cmsis_gcc.h
           cmsis_clang.h
           :
     Feature
           cache1_armv7.h
           pmu_v8.1-m.h
           mpu_v8.h
           gic_v20.h
           :

What would be the next steps for such a transformation? How can I provide support?

Best Regards
Markus

[JonatanAntoni]

Hi Markus et al,

I think it might be beneficial to have a online discussion about this topic rather than lengthen this thread. Would you be available for an online meeting throughout the the next couple of days? I should be able to host a session using Arm's Zoom infrastructure.

If you let me know your availability via mail to [email protected] I'll send the invite.

Cheers,
Jonatan

[EdmundPlayer]

That's a good idea Jonatan.

Just to quickly answer some of your questions, Markus...

The MPU in M-profile is not the same as the MPU in R-profile, so we'd need separate files for these profiles. Therefore, we might need to rename the mpu_v8.h header to something more specific, e.g., mpu_v8-m.h, or go with separate folders for different profiles, as I mentioned previously.

You raise a good point about A-profile and R-profile as there is more commonality, e.g., the GIC. We could potentially create a shared folder for A-profile and R-profile for any common features, or create filenames that clearly show that the file is suitable for A-profile and R-profile.

In terms of using the code, renaming the files might not be a problem, but we'd need to be 100% sure that this is the case. In terms of documentation though, some references to filenames might no longer be correct unless we update the documentation, e.g., the following document is just one example that refers to a specific file name (pmu_v8.h.)
https://developer.arm.com/documentation/arm051-799564642-251/latest/

I think the Compiler folder idea is a possibility and something we could discuss.

I am thinking as well that we need to consider what would make things easier for the customer/user and customers creating their own packs.

[EdmundPlayer]

Hi Markus and Jonatan,

It was really useful to chat on the phone today about these improvements. Please find attached the sketch Jonatan drew during the meeting. Also, please find below a short summary of what we agreed on:

• Retire the Core_A folder and use a single Core folder for all processor core headers.
• Separate folders for different architecture profiles (A-profile, R-profile and M-profile) as discussed previously.
• Top-level compiler headers will exist in the Core folder (no separate Compiler folder is needed).
  • The cmsis_compiler.h header will continue to figure out which compiler toolchain is being used.
  • The compiler toolchain specific headers in the Core folder will contain code common to all architecture profiles.
  • Compiler headers specific to each architecture profiles can reside within the new architecture profile folders.
• For any features that are common to more than one architecture profile, e.g, the GIC is common to A-profile and R-profile, we can include the GIC header in each architectural folder, but only one of these files contains the code to avoid duplication. The file with the same name that doesn't include the code will simply include the code from the other folder- this should hopefully be clear to developers.

I hope this is accurate, but feel free to comment on anything I've missed or if anything needs amending.

Also, perhaps the second-level files like cmsis_gcc.h could potentially be named cmsis_gcc_a.h, cmsis_gcc_r.h and cmsis_gcc_m.h instead, just so we don't have multiple files with the same name - this is just something another colleague thought of after our call, so I thought I should share their idea too.

Best regards,

Ed

[JonatanAntoni]

Hi Markus, Thanks for your contribution. Your feedback from a real-world project is highly valuable to me. When you restructure the pull-request, may I kindly ask you to break the contribution up into “baby steps”? I.e., in a first PR we probably should only include the file renames to achieve the desired folder structure. I’d prefer no functional changes so that we have the chance to have the CoreValidation tests to continue running without changes, ideally. That means, we move the compiler headers into the profile folders but keep the duplicated (shared) code. In a second PR we then move the common parts one folder level up. Does this make sense to you?

[Masmiseim36]

Hello @JonatanAntoni ,
I created a dedicated PR for this: #38
Unfortunately, git did not recognise that the compiler header were moved and new files with the same name were created. Now it looks like they have been completely recreated

[github-actions]

Test Results

0 tests   0 ✔️  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 9e9fad7.

[JonatanAntoni]

This PR looks stale, can it be closed?

[Masmiseim36]

I had left it open for reference. I have submitted separate PullRequests for the next steps. You could possibly take a look at them.

• - Add initial support for ARMv8A, ARMV7R and ARMV8R #45
• Split up the compiler files to a generic part and a profile specific part #44