cpu/mips: Integrate better with RIOT newlib layer by jnohlgard · Pull Request #6639 · RIOT-OS/RIOT

jnohlgard · 2017-02-22T17:43:28Z

Depends on #4699 ~~#6066~~ ~~#6092~~ #6670 ~~#6672~~ ~~#6673~~

Copied the relevant parts of mipshal.mk from the toolchain
Cleaned up CFLAGS and more in Makefile.include
Added empty syscalls implementation which can be used to interface with the UHI system, in reference to mips: Build system is not aligned with other platforms #6588 (comment)
Added a note on why -std=gnu99 is necessary for this CPU presently, should be removed once the toolchain is updated to a more recent newlib version.

The reason for this PR is that we need VFS (#5616) to be able to tell when newlib is used in order to provide the correct headers for use in statvfs etc.

jnohlgard · 2017-02-22T17:45:40Z

cpu/mips32r2_common/Makefile.include

+# A bit of makefile magic:
+# foreach symbol in overridable ld-symbols :
+#   If symbol has a value, produce a linker argument for that symbol.
+MIPS_HAL_LDFLAGS = $(foreach a,$(priv_symbols),$(if $($a),-Wl$(comma)--defsym$(comma)__$(call lc,$(a))=$($a)))


The above block is basically the only thing that had any effect for our use case in mipshal.mk, all other CFLAGS and tool paths were already copied to this file in the initial MIPS PR.

We probably want to add this too:

ifdef ROMABLE
MIPS_HAL_LDFLAGS += -T bootcode.ld
endif

As we use the toolchain bootcode for PIC32.

jnohlgard · 2017-02-22T17:47:24Z

references #6588

kaspar030 · 2017-02-24T15:03:56Z

cpu/mips32r2_common/Makefile.include

 export APP_START=0x80000000

-include $(MIPS_ELF_ROOT)/share/mips/rules/mipshal.mk
+# Portable 'lowercase' func.


While this looks definitely beautiful, we're using $(shell echo X | tr 'A-Z' 'a-z') in many other places. Mental note to myself: analyze what is faster and then unify.

kaspar030 · 2017-02-24T15:05:41Z

The changes look fine and very necessary. Thanks @gebart!

@neiljay Could you test this?

neiljay · 2017-02-24T15:18:57Z

Yes will do. This will conflict with #6066 and #6092 which re-factors out common makefile code to a new file (cpu/Makefile.include.mips_common) It would be nice if we could get these merged first, then apply this change to the new common file. Neil

…

On 02/24/17 15:05, Kaspar Schleiser wrote: The changes look fine and very necessary. Thanks @gebart <https://github.com/gebart>! @neiljay <https://github.com/neiljay> Could you test this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6639 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AUEU6p8vMK3jsqZtmQ_Sj7sGJdiWIgC9ks5rfvHIgaJpZM4MI90U>.

jnohlgard · 2017-02-24T16:31:33Z

I found an issue. The Makefile.include for sys/newlib does not seem to be included with this patch. Need to investigate

jnohlgard · 2017-02-24T17:33:51Z

Sure, I will try to rebase onto those PRs

…

On Feb 24, 2017 4:18 PM, "neiljay" ***@***.***> wrote: Yes will do. This will conflict with #6066 and #6092 which re-factors out common makefile code to a new file (cpu/Makefile.include.mips_common) It would be nice if we could get these merged first, then apply this change to the new common file. Neil On 02/24/17 15:05, Kaspar Schleiser wrote: > The changes look fine and very necessary. Thanks @gebart > <https://github.com/gebart>! > > @neiljay <https://github.com/neiljay> Could you test this? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#6639 (comment)>, or > mute the thread > <https://github.com/notifications/unsubscribe-auth/AUEU6p8vMK3jsqZtmQ_ Sj7sGJdiWIgC9ks5rfvHIgaJpZM4MI90U>. > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6639 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATYQt_DS4mlYblZM3eyZNmYVYqIpEXQks5rfvTigaJpZM4MI90U> .

neiljay · 2017-02-27T13:26:27Z

cpu/mips32r2_common/Makefile.include

-export CFLAGS_DBG = -O0 -g2
-export CFLAGS_OPT = -Os -g2
+export CFLAGS_DBG   = -g3
+export CFLAGS_OPT   = -Os


It can be useful to have debug symbols on a release build too, the elf is typically converted to some binary format for 'real world' targets anyway.

@neiljay you have misinterpreted the intent of the CFLAGS_DBG and CFLAGS_OPT variables in the other Makefiles, they are simply groupings for making the file easier to read:

CFLAGS_DBG contains flags which affect the generation of debug symbols

CFLAGS_OPT contains flags which affect the code optimization
If you check the other platforms you'll see that they are both unconditionally added to CFLAGS.
We don't have any concept of Release build vs. Debug build in the current RIOT build system.
I forgot to remove the comment on CFLAGS_DBG below, though.

neiljay · 2017-02-27T13:32:52Z

@gebart @kaspar030

I've tried this change with a few of the examples and it hasn't broken anything, I had a couple of minor review points, but otherwise I'm happy with this as long as we can merge #6066 and #6092 first then rebase this atop.

Cheers.

Neil.

neiljay · 2017-03-01T11:01:17Z

@gebart Ah ok I see, it would be nice to have a standard mechanism to do debug vs release builds, GUI debuggers can get confused with optimised code. Neil

…

On 03/01/17 10:53, Joakim Nohlgård wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In cpu/mips32r2_common/Makefile.include <#6639 (comment)>: > export CFLAGS_LINK = -ffunction-sections -fdata-sections -fno-builtin -fshort-enums -export CFLAGS_DBG = -O0 -g2 -export CFLAGS_OPT = -Os -g2 +export CFLAGS_DBG = -g3 +export CFLAGS_OPT = -Os @neiljay <https://github.com/neiljay> you have misinterpreted the intent of the CFLAGS_DBG and CFLAGS_OPT variables in the other Makefiles, they are simply groupings for making the file easier to read: * CFLAGS_DBG contains flags which affect the generation of debug symbols * CFLAGS_OPT contains flags which affect the code optimization If you check the other platforms you'll see that they are both unconditionally added to CFLAGS. We don't have any concept of Release build vs. Debug build in the current RIOT build system. I forgot to remove the comment on CFLAGS_DBG below, though. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6639 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AUEU6vyaVTEo8ATxeYJPnxo3uema3ZfCks5rhU4UgaJpZM4MI90U>.

jnohlgard · 2017-03-01T13:59:22Z

rebased on top of #6092

neiljay · 2017-03-10T13:54:48Z

@gebart

I have checked this works on the pic32-clicker and pic32-wifire boards.

I guess this needs re-basing now the VFS PR got merged.

jnohlgard · 2017-03-10T14:04:32Z

I will try to rebase this some time during the evening or weekend

jnohlgard · 2017-03-11T04:46:11Z

rebased

- Copied the relevant parts of mipshal.mk from the toolchain - Cleaned up CFLAGS and more in Makefile.include - Added empty syscalls implementation which can be used to interface with the UHI system - Added a note on why -std=gnu99 is necessary for this CPU presently

Some headers in the tool chain cause compilation errors with Clang in assembly mode.

Causes compilation error on Clang

jnohlgard · 2017-03-30T07:29:45Z

rebased on latest #4699, #6670

neiljay · 2017-03-30T09:20:25Z

@gebart, Is it possible for me to commit directly to the PR, or should I create my own based off this ?
I have a tracking branch setup tracking gebart/pr/mips-newlib can I just push to that ?

jnohlgard · 2017-03-30T10:46:43Z

@neiljay AFAIK it's not possible for you to update my PR branch, but if you would like to take over this PR you can open a new PR with your own branch and I'll close this one.

jnohlgard · 2017-04-11T18:46:59Z

Closing in favour of #6892

jnohlgard added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: build system Area: Build system Platform: MIPS Platform: This PR/issue effects MIPS-based platforms labels Feb 22, 2017

jnohlgard added this to the Release 2017.04 milestone Feb 22, 2017

jnohlgard assigned haukepetersen Feb 22, 2017

jnohlgard requested review from kaspar030 and neiljay February 22, 2017 17:43

jnohlgard commented Feb 22, 2017

View reviewed changes

kaspar030 reviewed Feb 24, 2017

View reviewed changes

neiljay reviewed Feb 27, 2017

View reviewed changes

jnohlgard force-pushed the pr/mips-newlib branch from afc4a9c to 23975a0 Compare March 1, 2017 13:57

jnohlgard added the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 1, 2017

jnohlgard mentioned this pull request Mar 6, 2017

vfs: file system abstraction #5616

Merged

55 tasks

neiljay mentioned this pull request Mar 10, 2017

cpu: Add PIC32MZ support #6066

Merged

jnohlgard force-pushed the pr/mips-newlib branch from f88e513 to 9ca8437 Compare March 11, 2017 04:39

neiljay mentioned this pull request Mar 17, 2017

cpu: Add PIC32MX Support #6092

Merged

Joakim Nohlgård added 4 commits March 30, 2017 09:16

Makefiles: move to new directory /makefiles

b921559

squash rename .mk -> .inc.mk

f3cac86

squash update include names

a6945c2

squash move Makefile.include.mips_common -> arch/mips.inc.mk

0a6f879

jnohlgard force-pushed the pr/mips-newlib branch from 9aabfa5 to 3c4f80a Compare March 30, 2017 07:22

Joakim Nohlgård added 7 commits March 30, 2017 09:23

newlib: Split newlib into one generic part and one default syscalls part

e4e90b2

squash move Makefile.newlib -> newlib.mk

0ddd2ef

cpu/mips32r2_common: Use GCC for assembler files if TOOLCHAIN=llvm

4790bac

Some headers in the tool chain cause compilation errors with Clang in assembly mode.

cpu/mips32r2_common: ifdef __attribute__((optimize))

375637e

Causes compilation error on Clang

squash add if ROMABLE

5b961e0

squash remove POSIX headers workaround for mips-malta

6716925

jnohlgard force-pushed the pr/mips-newlib branch from 3c4f80a to 6716925 Compare March 30, 2017 07:24

neiljay mentioned this pull request Apr 11, 2017

cpu/mips: Integrate better with RIOT newlib layer (v2) #6892

Merged

jnohlgard closed this Apr 11, 2017

miri64 added the CI: needs squashing label Oct 5, 2018

jnohlgard deleted the pr/mips-newlib branch October 25, 2018 16:06

Conversation

jnohlgard commented Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnohlgard Feb 22, 2017

Choose a reason for hiding this comment

Uh oh!

neiljay Feb 27, 2017

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Feb 22, 2017

Uh oh!

kaspar030 Feb 24, 2017

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Feb 24, 2017

Uh oh!

neiljay commented Feb 24, 2017 via email

Uh oh!

jnohlgard commented Feb 24, 2017

Uh oh!

jnohlgard commented Feb 24, 2017 via email

Uh oh!

neiljay Feb 27, 2017

Choose a reason for hiding this comment

Uh oh!

jnohlgard Mar 1, 2017

Choose a reason for hiding this comment

Uh oh!

neiljay commented Feb 27, 2017

Uh oh!

neiljay commented Mar 1, 2017 via email

Uh oh!

jnohlgard commented Mar 1, 2017

Uh oh!

neiljay commented Mar 10, 2017

Uh oh!

jnohlgard commented Mar 10, 2017

Uh oh!

jnohlgard commented Mar 11, 2017

Uh oh!

jnohlgard commented Mar 30, 2017

Uh oh!

neiljay commented Mar 30, 2017

Uh oh!

jnohlgard commented Mar 30, 2017

Uh oh!

jnohlgard commented Apr 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jnohlgard commented Feb 22, 2017 •

edited

Loading