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

cpu: Rename CPU_ARCH to CPU_CORE #14246

Merged
merged 4 commits into from
Jun 16, 2020

Conversation

leandrolanzieri
Copy link
Contributor

Contribution description

Currently cortexm and mips CPUs are using the variable CPU_ARCH to refer to the CPU core identifier. Even the documentation of this variable indicates that:

export CPU_ARCH # The specific identifier of the core present in the CPU, used currently only for ARM CPU's. Needed for depency resolution.

A naming discussion started in #14176 regarding our classification system for our CPUs. It makes sense to structure the names using the correct terms. This PR introduces CPU_CORE as a replacement for CPU_ARCH, and assigns the correct architecture value to cortexm and mips CPUs.

Testing procedure

  • Use make dependency-debug to check that the correct CPU_CORE and CPU_ARCH is assign to the boards

Issues/PRs references

Somehow related to #14176

@leandrolanzieri leandrolanzieri added Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: cpu Area: CPU/MCU ports labels Jun 10, 2020
cpu/cortexm_common/Makefile.features Outdated Show resolved Hide resolved
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Tested with for board in $(make info-boards --no-print-directory); do printf "%s=" ${board}; BOARD=${board} make --no-print-directory info-debug-variable-CPU_CORE -C examples/hello-world/; done
and for board in $(make info-boards --no-print-directory); do printf "%s=" ${board}; BOARD=${board} make --no-print-directory info-debug-variable-CPU_ARCH -C examples/hello-world/; done.

Assuming murdock passes I think this would be OK. I will ACK! @kaspar030 has your comment been addressed and @fjmolinas any last words?

@MrKevinWeiss MrKevinWeiss added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 11, 2020
@fjmolinas
Copy link
Contributor

I haven't taken a detailed look, but +1 for the general idea.

@kaspar030
Copy link
Contributor

This PR introduces CPU_CORE as a replacement for CPU_ARCH, and assigns the correct architecture value to cortexm and mips CPUs.

Could you sum up the state we have and will have? I'm honestly confused.

@leandrolanzieri
Copy link
Contributor Author

Could you sum up the state we have and will have? I'm honestly confused.

Currently CPU_ARCH variable is being used only in cortexm and mips CPUs to indicate the core of the CPU (instead of the architecture as the name suggests).

The PR moves these values to (the newly introduced variable) CPU_CORE, to use the proper terminology. CPU_ARCH is kept for the CPUs that already define it, but the correct architecture identifier is assigned to it.

For instance, a CPU that currently has CPU_ARCH=cortex-m0plus, with this PR will have CPU_CORE=cortex-m0plus and CPU_ARCH=armv6m.

@leandrolanzieri leandrolanzieri added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 11, 2020
@MrKevinWeiss
Copy link
Contributor

@kaspar030 It makes sense to me, is there anything else that you can think of that this clarification of naming might effect (ie. anything with murdock or if something is used outside RIOT)?

@leandrolanzieri leandrolanzieri added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 15, 2020
@fjmolinas
Copy link
Contributor

@leandrolanzieri this one needs a rebase, other than that can you uncrustify core/include/panic.h?

@fjmolinas
Copy link
Contributor

You can squash afterwards as well @leandrolanzieri!

@kaspar030 kaspar030 dismissed their stale review June 16, 2020 08:38

comment addressed

@fjmolinas fjmolinas added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jun 16, 2020
@fjmolinas
Copy link
Contributor

Added the testing lable, since @MrKevinWeiss tested!

@aabadie
Copy link
Contributor

aabadie commented Jun 16, 2020

This needs a rebase now.

@fjmolinas fjmolinas added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines and removed Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Jun 16, 2020
@fjmolinas fjmolinas added the Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines label Jun 16, 2020
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

2nd ACK

@fjmolinas fjmolinas merged commit 315b979 into RIOT-OS:master Jun 16, 2020
@miri64 miri64 added this to the Release 2020.07 milestone Jun 24, 2020
@leandrolanzieri leandrolanzieri deleted the pr/cpu_arch_to_cpu_core branch August 7, 2020 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants