-
Notifications
You must be signed in to change notification settings - Fork 16
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: add armv8m #342
base: master
Are you sure you want to change the base?
arch: add armv8m #342
Conversation
34e5bca
to
1e61bb4
Compare
I have a small comment. I suggest adding a |
Hmm, but those files are intended for various armv8 cpus, like Cortex-M23 and so on (I know that now they are not provided, but maybe they will be available in the future) |
General remark - modify copyright headers. You are an author of all these files, of some (like Makefile) you are the only author |
.syntax unified | ||
|
||
.text | ||
.align 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alignment might cause some performance penalty. IMHO better to set it to 4, to make it predictable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to leave 2 for other arm architectures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other arm archs aren't in the scope of this PR. Imho it's best to change alignment to 4 everywhere eventually, as @lukileczo found out it's faster indeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see that it has been changed recently
|
||
.globl sigsetjmp | ||
.type sigsetjmp, %function | ||
.align 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant
.align 2 |
|
||
void __attribute__((naked)) __aeabi_read_tp(void) | ||
{ | ||
__asm__ volatile( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__asm__ volatile( | |
__asm__ volatile ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we leave it without a space to be compliant with clang-format? Like for other architectures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not, clang-format is confused in this case, as it thinks it's a function - it's not. It would be best to use clang-format off on every inline asm instead
1e61bb4
to
7c1d513
Compare
JIRA: RTOS-770
JIRA: RTOS-770
7c1d513
to
0faa598
Compare
With the merge of #347, I've created conflicts for your port again. I've made a good chunk of code previously split into |
Description
arch: add armv8m
Motivation and Context
Finalizing the nrf9160 basic port
Types of changes
How Has This Been Tested?
Checklist:
Special treatment