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

Initial support for ARM Cortex-R #9316

Merged
merged 12 commits into from Aug 9, 2019

Conversation

@bbolen
Copy link

commented Aug 6, 2018

There have been a couple of requests for the addition of ARM Cortex-R support, e.g. #6545 and #5540.

I have started work for Cortex-R4 support for a project my company is doing. This PR represents work that gets the "shell" sample working on an R4. Basic context switching and interrupts using an R4 connected to a GIC400 are working.

The work and testing were done on a custom SoC so the PR does not include the SoC code since it won't be very useful to anyone.

Closes #6545
Closes #5540

Signed-off-by: Bradley Bolen bbolen@lexmark.com

@codecov-io

This comment has been minimized.

Copy link

commented Aug 6, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@be2c937). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #9316   +/-   ##
=========================================
  Coverage          ?   52.92%           
=========================================
  Files             ?      309           
  Lines             ?    45268           
  Branches          ?    10451           
=========================================
  Hits              ?    23956           
  Misses            ?    16544           
  Partials          ?     4768

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be2c937...5396c68. Read the comment docs.

@ulfalizer
Copy link
Collaborator

left a comment

Just some Kconfig nits.

arch/arm/core/Kconfig Outdated Show resolved Hide resolved
arch/arm/core/cortex_r/Kconfig Outdated Show resolved Hide resolved
arch/arm/core/cortex_r/Kconfig Outdated Show resolved Hide resolved
arch/arm/core/cortex_r/Kconfig Outdated Show resolved Hide resolved
arch/arm/core/cortex_r/Kconfig Outdated Show resolved Hide resolved
arch/arm/core/Kconfig Outdated Show resolved Hide resolved
config ISA_THUMB2
bool
# Omit prompt to signify "hidden" option
default n

This comment has been minimized.

Copy link
@ulfalizer

ulfalizer Aug 6, 2018

Collaborator

Redundant default n.

arch/arm/core/cortex_r/Kconfig Outdated Show resolved Hide resolved
arch/arm/core/cortex_r/Kconfig Outdated Show resolved Hide resolved
arch/arm/core/cortex_r/Kconfig Show resolved Hide resolved
@galak

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

@bbolen have you tried to see if you can port qemu and the xlnx-zynqmp platform?

@bbolen bbolen force-pushed the bbolen:cortex_r branch from a87639e to 11d1081 Aug 7, 2018

@bbolen

This comment has been minimized.

Copy link
Author

commented Aug 7, 2018

@ulfalizer Thanks for the comments. Those should all be fixed now.

@bbolen

This comment has been minimized.

Copy link
Author

commented Aug 7, 2018

@galak I saw that yesterday trying to find a way for others to replicate my work. I will keep looking at it, but I can't guarantee when I may have something useful with respect to qemu.

@ulfalizer
Copy link
Collaborator

left a comment

Kconfig parts look good to me.

#define GIC_INT_WD 30
#define GIC_INT_IRQ 31

#define IRQ_TYPE_NONE 0x00000000

This comment has been minimized.

Copy link
@pizi-nordic

pizi-nordic Aug 8, 2018

Collaborator

I think that IRQ type should be set by DTS. This means that:

  • IRQ type definitions have to be exposed as DTS bindings.
  • YAML files should be updated accordingly.
  • Driver should set GICD_ICFGn basing on the flags specified in interrupt property.
#define NO_GIC_INT_PENDING 1023

/* CPU Interrupt numbers */
#define GIC_INT_GTMR 27

This comment has been minimized.

Copy link
@pizi-nordic

pizi-nordic Aug 8, 2018

Collaborator
  • Could we use here names which will be self-descriptive or easy to find in docs? (like GIC_INT_LEGACY_FIQ)
  • According to the CoreLink™ GIC-400 Generic Interrupt Controller Technical Reference Manual there is no watchdog interrupt at ID 30 (GIC_INT_WD suggests watchdog). Is watchdog on this line specific to your SoC?
  • IMHO these defines should be exposed to the DTS or some platform code which will use it (like timer driver).
* Set priority on PPI and SGI interrupts
*/
for (i = 0; i < 32; i += 4)
sys_write32(0xa0a0a0a0, GICD_IPRIORITYRn + i);

This comment has been minimized.

Copy link
@pizi-nordic

pizi-nordic Aug 8, 2018

Collaborator

IMHO priority should be taken from DTS.

This comment has been minimized.

Copy link
@bbolen

bbolen Aug 9, 2018

Author

I see two possible paths to get the flags and priority from gic.c. 1) add an intr_set_priority() callback to the irq_next_level_api struct and call it from arch/arm/core/irq_manage.c:_irq_priority_set() OR 2) redefine _ARCH_IRQ_CONNECT for Cortex-R to pass flags_p instead of 0 to _ISR_DECLARE and get to the flags through the isr_sw_table.

Do you have a preference between these two, or another option?

@@ -19,7 +19,6 @@ config ARC

config ARM

This comment has been minimized.

Copy link
@pizi-nordic

pizi-nordic Aug 8, 2018

Collaborator

IMHO subject of the commit does not reflect actual change.
What about "arch: arm: Move thread_abort.c to cortex_m specific directory"?

@@ -4,7 +4,6 @@ zephyr_library_sources(
exc_exit.S
swap.c
swap_helper.S
fault.c

This comment has been minimized.

Copy link
@pizi-nordic

pizi-nordic Aug 8, 2018

Collaborator

Here also you are moving file specific to cortex-m, but subject of the commit suggests cortex-r.

* Cortex-R only has one IRQ line so the main handler will be at
* offset 0 of the table.
*/
mov r0, #0

This comment has been minimized.

Copy link
@pizi-nordic

pizi-nordic Aug 8, 2018

Collaborator

Are you going to dispatch FIQ and IRQ using the same IRQ table?

@@ -140,7 +151,11 @@ SECTION_FUNC(TEXT, __pendsv)
str r3, [r4]
#else
ldr r0, [r2, #_thread_offset_to_basepri]
#ifdef CONFIG_CPU_CORTEX_M
movs.n r3, #0

This comment has been minimized.

Copy link
@pizi-nordic

pizi-nordic Aug 8, 2018

Collaborator

IMHO we just could use "movs r3, #0" on both Cortex-M and Cortex-R

mov r4, lr

cps #MODE_SYS
stmdb sp!, {r0-r4}

This comment has been minimized.

Copy link
@pizi-nordic

pizi-nordic Aug 8, 2018

Collaborator

What about R12?

arch/arm/core/swap_helper.S Show resolved Hide resolved
@@ -0,0 +1,158 @@
/* ARM Cortex-M GCC specific public inline assembler functions and macros */

This comment has been minimized.

Copy link
@pizi-nordic

pizi-nordic Aug 8, 2018

Collaborator

IMHO with some modifications in _arch_irq_lock()/_arch_irq_unlock() this file could be common for Cortex-M and R.

@ioannisg
Copy link
Member

left a comment

Went through most of the /arch code; added a few comments about re-using code and about Kconfig options.

config CPU_CORTEX_R4
bool
select CPU_CORTEX_R
select ARMV7_R_ARMV8_R_MAINLINE

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 9, 2018

Member

Is this the right thing to do?
I mean: for Cortex-M, ARMv8-M_Mainline is a "super-set" of and backwards-compatible with ARMv7-M.
Is this true for ARMv7-R? Do ARMv7-R, ARMv8-R have the Baseline, Mainline versions?

If not, I would say, it is better to name the architecture as ARMV7_R


if CPU_CORTEX_R

config ISA_THUMB2

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 9, 2018

Member

I agree. There are several options that might be not-specific to ARM-R (instead, shared with ARM-M).
Could we move those options to arm/core/Kconfig, instead?

select ATOMIC_OPERATIONS_BUILTIN
select ISA_ARM
help
This option signifies the use of an ARMv7-R processor

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 9, 2018

Member

Coming back to this one: is this accurate for ARMv8-R? Are there Baseline and Mainline variations?

@@ -0,0 +1,37 @@
/*

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 9, 2018

Member

Is this file just copy-pasted from the cortex-m version of it?

arch/arm/core/cortex_r/fault.c Show resolved Hide resolved
@@ -0,0 +1,71 @@
/*

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 9, 2018

Member

I had a quick look, and it seems to me that the only difference from Cortex-M is that we do not have the code block for the case of CONFIG_CPU_CORTEX_M_HAS_VTOR=y.

SYS_Stack:
.skip 1024 * 4

#define STACK_MARGIN 4

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 9, 2018

Member

...And use them, directly in the linker script, perhaps, @pizi-nordic ?

*
* SPDX-License-Identifier: Apache-2.0
*/

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 9, 2018

Member

Could we re-use the Cortex-M scb.c file, instead?
And add this dummy implementation, if ARMv7_R is defined (instead of ARMv-M). Note, that this would move the scb.c one level up.

#include <arch/arm/cortex_m/cmsis.h>
#else

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 9, 2018

Member

Perhaps, it is better to have CONFIG_CPU_CORTEX_3 explicitly given, here.

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 9, 2018

Member

Same applies to more cases, below.

This comment has been minimized.

Copy link
@bbolen

bbolen Aug 9, 2018

Author

I'm sorry, I think I'm missing something here. I was assuming that this code applied to all Cortex-M's. Are you saying that it should only apply to the M3?

/* set pending bit to make sure we will take a PendSV exception */
SCB->ICSR |= SCB_ICSR_PENDSVSET_Msk;
#endif

/* clear mask or enable all irqs to take a pendsv */
irq_unlock(0);

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 9, 2018

Member

Is this needed for Cortex-R?

@nashif nashif closed this Aug 11, 2018

@nashif nashif reopened this Aug 11, 2018

@bbolen bbolen force-pushed the bbolen:cortex_r branch 2 times, most recently from a97a908 to 85a8ecf Aug 14, 2018

@bbolen

This comment has been minimized.

Copy link
Author

commented Aug 14, 2018

@pizi-nordic @ioannisg I've updated this PR with changes to address your comments.

@SebastianBoe SebastianBoe removed their request for review Aug 16, 2018

@ioannisg
Copy link
Member

left a comment

Some more suggestions and issues/questions.

One general thing: Please, use #if defined (), instead of #ifdef in the arm/core and include/ sub-trees

help
This option signifies the use of an ARMv7-R processor
implementation, or the use of an ARMv8-R processor
implementation supporting the Main Extension.

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 17, 2018

Member

Please, clarify: does the -R- architecture have Base and Mainline versions for v8? If not, the help text needs update

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 17, 2018

Member

Btw, I searched a bit and I didn't see clear indication of Baseline, Mainline terminology outside the -M- architecture...

referred to as a Mainline Implementation.
- ARMv7-R compatibility requires the Main Extension.

From https://developer.arm.com/products/architecture/m-profile:

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 17, 2018

Member

Link points to -M- documentation

depends on ARMV7_R
help
This option signifies the use of an ARMv7-R processor
implementation, or the use of an ARMv8-R processor

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 17, 2018

Member

ARMv8 is not reflected in the symbol name.

bool
select CPU_CORTEX_R
select ARMV7_R
select ARMV7_M_ARMV8_M_FP if CPU_HAS_FPU

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 17, 2018

Member

ARMV7_M_ARMV8_M_FP is for Cortex-M CPUs

This comment has been minimized.

Copy link
@bbolen

bbolen Aug 17, 2018

Author

I'm sorry. Most of this is just copy-paste crud that I'll get rid of.

@@ -120,7 +124,8 @@ SECTION_FUNC(TEXT, k_cpu_idle)
mov lr, r0
#endif

#if defined(CONFIG_ARMV6_M_ARMV8_M_BASELINE)
#if defined(CONFIG_ARMV6_M_ARMV8_M_BASELINE) \

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 17, 2018

Member

This, here (and right below on line 180/185) look a bit weird; ARMV7_R is "grouped" together with ARMV6_M, instead of ARMV7_M.

If this is indeed correct, and needs to be as is, it will confuse the reader.
Any suggestion how to address this, @galak ?


#ifdef CONFIG_PREEMPT_ENABLED
ldr r0, =_kernel

ldr r1, [r0, #_kernel_offset_to_current]

ldr r0, [r0, _kernel_offset_to_ready_q_cache]
ldr r0, [r0, #_kernel_offset_to_ready_q_cache]

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 17, 2018

Member

What was the reason for this?

This comment has been minimized.

Copy link
@bbolen

bbolen Aug 17, 2018

Author

Using the zephyr-sdk, I get a compile error
src/zephyr/arch/arm/core/exc_exit.S: Assembler messages:
src/zephyr/arch/arm/core/exc_exit.S:82: Error: immediate expression requires a # prefix -- `ldr r0,[r0,(0x1c+0x0)]' since _kernel_offset_to_ready_q_cache is an immediate value. I can't really say why the M boards do not see this.

arch/arm/core/swap_helper.S Outdated Show resolved Hide resolved
@@ -140,7 +151,7 @@ SECTION_FUNC(TEXT, __pendsv)
str r3, [r4]
#else
ldr r0, [r2, #_thread_offset_to_basepri]
movs.n r3, #0
movs r3, #0

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 17, 2018

Member

what is the reason for this change?

This comment has been minimized.

Copy link
@bbolen

bbolen Aug 17, 2018

Author

Again, compile error when using the SDK for Cortex-R
src/zephyr/arch/arm/core/swap_helper.S: Assembler messages:
src/zephyr/arch/arm/core/swap_helper.S:158: Error: unexpected character n' in type specifier src/zephyr/arch/arm/core/swap_helper.S:158: Error: bad instruction movs.n r3,#0'

This comment has been minimized.

Copy link
@ioannisg

@bbolen bbolen force-pushed the bbolen:cortex_r branch from 85a8ecf to f4d02d8 Aug 21, 2018

flash: 256
testing:
default: false
ignore_tags:

This comment has been minimized.

Copy link
@ioannisg

ioannisg Aug 9, 2019

Member

Not sure why these tags are all ignored

This comment has been minimized.

Copy link
@bbolen

bbolen Aug 9, 2019

Author

Currently, the cortex R5 on the xlnx-zynmp qemu machine is not hooked up to an interrupt controller, so only the "hello world" test case will run right now.

drivers/serial/Kconfig.xlnx Outdated Show resolved Hide resolved
@ioannisg
Copy link
Member

left a comment

Went through the PR, looks OK to me. I still left several comments to address, but, just to try to push this for 2.0, I give a +1

@ioannisg

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@bbolen I suppose if you rebase once more you can run CI with the new SDK, Qemu.

@ioannisg ioannisg requested review from galak, wjliang and andrewboie Aug 9, 2019

@galak galak referenced this pull request Aug 9, 2019
46 of 48 tasks complete
@ioannisg

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@andrewboie @galak could you have another look?
@bbolen could you rebase to latest master?

Bradley Bolen and others added 12 commits Apr 18, 2018
Bradley Bolen
interrupt_controller: gic: Add support for the GIC400
The GIC400 is a common interrupt controller that can be used with the
Cortex A and R series processors.  This patch adds basic interrupt
handling for the GIC, but does not handle multiple routing or
priorities.

Signed-off-by: Bradley Bolen <bbolen@lexmark.com>
Bradley Bolen
arch: arm: Add Cortex-R support
This adds initial Cortex-R support for interrupts and context switching.

Signed-off-by: Bradley Bolen <bbolen@lexmark.com>
Bradley Bolen
drivers: interrupts: Add a set_priority callback
Provide a path for irq controller drivers to change properties of an
individual irq using priority and flags fields that come from the device
tree.

Signed-off-by: Bradley Bolen <bbolen@lexmark.com>
Bradley Bolen
arch: arm: Add Cortex-R5 support
Pass the correct -mcpu flags to the compiler when building for the
Cortex-R5.

Signed-off-by: Bradley Bolen <bbolen@lexmark.com>
Wendy Liang Bradley Bolen
timer: Add Xilinx ZynqMP PS ttc timer
Add Xilinx PS ttc timer for Xilinx ZynqMP platform.

Signed-off-by: Wendy Liang <wendy.liang@xilinx.com>
Wendy Liang Bradley Bolen
serial: Add Xilinx ZynqMP PS uart driver
Add ZynqMP PS uart driver for Xilinx ZynqMP platform

Signed-off-by: Wendy Liang <wendy.liang@xilinx.com>
Bradley Bolen
Revert "toolchain: gcc: Remove ref. to undefined sym. CONFIG_ISA_ARM"
This reverts commit e20fd5f.

This is needed for Cortex-R support.

Signed-off-by: Bradley Bolen <bbolen@lexmark.com>
Bradley Bolen
arch: arm: cortex_r: Add memory barriers for register accesses
Cortex R has a write buffer that can cause reordering problems when
accessing memory mapped registers.  Use memory barries to make sure that
these accesses are performed in the desired order.

Signed-off-by: Bradley Bolen <bbolen@lexmark.com>
Bradley Bolen
soc: arm: xilinx_zynqmp: Add qemu based SoC
This commit adds support for the Zynq UltraScale+ MPSoC as a qemu based
platform for Cortex-R based testing.  This SoC only supports an
interrupt controller and serial port for limited testing.

Signed-off-by: Bradley Bolen <bbolen@lexmark.com>
Bradley Bolen
arch: arm: Add Cortex-R4 support
Pass the correct -march and -mcpu flags to the compiler when building
for the Cortex-R4.

Signed-off-by: Bradley Bolen <bbolen@lexmark.com>
Bradley Bolen
boards: qemu_cortex_r5: Add qemu test board for the Cortex-R series
This adds a qemu test board using the Xilinx ZynqMP SoC.

Signed-off-by: Bradley Bolen <bbolen@lexmark.com>
Bradley Bolen
cmake: emu: Allow overriding the qemu binary
The xlnx-zcu102 qemu machine is the only one that supports a Cortex-R
processor.  However, its main CPUs are Cortex A53s which requires the
aarch64 qemu binary to run.

Signed-off-by: Bradley Bolen <bbolen@lexmark.com>

@bbolen bbolen force-pushed the bbolen:cortex_r branch from 30670c6 to 8401923 Aug 9, 2019

@ioannisg ioannisg merged commit b602e38 into zephyrproject-rtos:master Aug 9, 2019

8 checks passed

Codeowners Checks passed
Details
Documentation Checks passed
Details
Gitlint Checks passed
Details
Identity/Emails Checks passed
Details
Kconfig Checks passed
Details
License Checks passed
Details
Shippable Run 48120 status is SUCCESS.
Details
checkpatch Checks passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.