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

arch: arm: Add Cortex-R floating point configuration support. #20469

Closed

Conversation

stephanosio
Copy link
Member

  1. Allow CPU_HAS_FPU_DOUBLE_PRECISION on Cortex-R4 and Cortex-R5, as
    the optional VFPv3-D16 is, by default, a double precision unit.

  2. Add GCC compiler support for Cortex-R4F and Cortex-R5F floating
    point unit. The FPU type on these cores is fixed to VFPv3-D16,
    which supports double precision by default. While later revisions
    of Cortex-R5F can have single precision VFPv3-D16, GCC does not
    have a separate type for it.

Signed-off-by: Stephanos Ioannidis root@stephanos.io

NOTE: Separated out from #19698 for timely review and merging.

soc/arm/Kconfig Outdated Show resolved Hide resolved
@@ -34,7 +34,6 @@ config CPU_HAS_NRF_IDAU

config CPU_HAS_FPU_DOUBLE_PRECISION
bool
depends on CPU_CORTEX_M7
Copy link
Member

Choose a reason for hiding this comment

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

I think this should still depend on the arch variants, no?

Copy link
Collaborator

@SebastianBoe SebastianBoe Nov 8, 2019

Choose a reason for hiding this comment

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

I don't see any reason for it to do so, and omitting this line means easier out-of-tree support.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should still depend on the arch variants, no?

@ioannisg Technically, that is right if we want to double-check arch support, but this list is only going to grow as more ARM variants are added. Also as @SebastianBoe pointed out, removing this line will allow the use of out-of-tree CPUs.

Copy link
Member Author

@stephanosio stephanosio Nov 8, 2019

Choose a reason for hiding this comment

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

For instance, we do not double-check if a CPU supports MPU or not using depends on. It is really up to the SoC to select appropriate feature symbols.

zephyr/soc/arm/Kconfig

Lines 6 to 11 in c704495

config CPU_HAS_ARM_MPU
bool
select CPU_HAS_MPU
help
This option is enabled when the CPU has a Memory Protection Unit (MPU)
in ARM flavor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This double-checking is not worth the extra maintenance and out-of-tree issues IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

All CPUs support the ARM MPU (except cortex-m0, but m0+ does)
Double-precision FPU is Cortex-M7 only, so I was thinking that's a protection against accidental enabling

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's no longer M7 only, so this protection is causing more trouble than it's worth.

Copy link
Member

Choose a reason for hiding this comment

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

..yeah, from Cortex-m side, it's M7, but sure i am ok if we wanna get rid of it
Checking with @galak as well :)

Copy link
Member Author

@stephanosio stephanosio Nov 8, 2019

Choose a reason for hiding this comment

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

Just to elaborate, it might become something like this in the future:
depends on (CPU_CORTEX_M7 || CPU_CORTEX_R4 || CPU_CORTEX_R5 || CPU_CORTEX_R7 || CPU_CORTEX_R8 || CPU_CORTEX_R52 || CPU_CORTEX_A8 || CPU_CORTEX_A9 || CPU_CORTEX_A15 || CPU_CORTEX_A53 || ...)

Copy link
Member

@ioannisg ioannisg Nov 8, 2019

Choose a reason for hiding this comment

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

Well, it could be CPU_CORTEX_R || CPU_CORTEX_A, exactly as we have CPU_CORTEX_M.
But, that's fine, I am convinced :) Waiting for @galak to comment :p

Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

I have a major concern with all cortex-r related changes, we are not testing ANY of those changes, because tests for qemu_cortex_r are disabled, so before doing all of that, we should fix this and remove all exclusions from:

boards/arm/qemu_cortex_r5/qemu_cortex_r5.yaml

If this PR fixes any of the excluded tags, the tag should be removed so we can verify this with qemu.

@stephanosio
Copy link
Member Author

stephanosio commented Nov 8, 2019

If this PR fixes any of the excluded tags, the tag should be removed so we can verify this with qemu.

@nashif This exact problem is still being addressed in #20217.

To summarise, the kernel tests run and pass if you install the Zephyr SDK 0.11.0-alpha-4 or above on your dev machine and apply the patches in #20298 and #20267; so, manual testing and verification with qemu_cortex_r5 is possible (detailed steps available here: #19698 (comment))

The main problem, even if we merge #20298 and #20267, is that the CI image does not use Zephyr SDK 0.11.0, which comes with Xilinx fork of QEMU that is required to run qemu_cortex_r5 tests.

I am trying to get these changes essential changes in for 2.1 because I am working on TI Hercules (Cortex-R) support at the moment and want to make sure it can run on the 2.1 release (at this time, it simply can't because the Cortex-R port contains many bugs).

@stephanosio stephanosio mentioned this pull request Nov 12, 2019
25 tasks
@stephanosio stephanosio added this to the v2.2.0 milestone Dec 9, 2019
1. Allow CPU_HAS_FPU_DOUBLE_PRECISION on Cortex-R4 and Cortex-R5, as
   the optional VFPv3-D16 is, by default, a double precision unit.

2. Add GCC compiler support for Cortex-R4F and Cortex-R5F floating
   point unit. The FPU type on these cores is fixed to VFPv3-D16,
   which supports double precision by default. While later revisions
   of Cortex-R5F can have single precision VFPv3-D16, GCC does not
   have a separate type for it.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
@stephanosio
Copy link
Member Author

Rebased

@stephanosio stephanosio requested a review from nashif February 12, 2020 06:52
@stephanosio stephanosio modified the milestones: v2.2.0, v2.3.0 Feb 12, 2020
@stephanosio
Copy link
Member Author

Superseded by #23904

@stephanosio stephanosio deleted the cortex_r_fp_config branch April 23, 2020 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants