mips: Import of mips32r2_common module by neiljay · Pull Request #6060 · RIOT-OS/RIOT

neiljay · 2016-11-04T13:10:19Z

Add support for the mips32r2 architecture to RIOT.

cpu/mips32r2_common adds base architecture support for mips32r2 cores it can be
built in it own right as a 'CPU', but is dependant on a bootloader (like u-boot)
to have bootstrapped the system, this has been tested on a 'malta' FPGA system
(BOARD=mips-malta) with various mips32r2 compliant cores (interAptiv, P5600,
etc).

NOTE: This port requires the use of the GCC based MIPS Codescape SDK toolchain
available here:

https://community.imgtec.com/developers/mips/tools/codescape-mips-sdk/download-codescape-mips-sdk-essentials/

This has been tested against the following examples:
hello-world
default
ipc_pingpong
timer_periodic_wakeup
riot_and_cpp

Note this is a re-work of #5885 with review comments addressed but now broken down into 3 separate PR's of which this is the first.

miri64 · 2016-11-04T13:25:49Z

Took the freedom to rename the PR :-)

neiljay · 2016-11-04T14:12:58Z

@miri64 No worries, much nicer ;-)

haukepetersen · 2016-12-12T15:46:25Z

boards/mips-malta/Makefile.features

@@ -0,0 +1,3 @@
+FEATURES_PROVIDED += periph_timer
+FEATURES_PROVIDED += periph_uart
+FEATURES_PROVIDED += cpp


Please use some existing Makefile.features as template for structuring this file:

# Put defined MCU peripherals here (in alphabetical order) FEATURES_PROVIDED += periph_timer FEATURES_PROVIDED += periph_uart # Various other features (if any) FEATURES_PROVIDED += cpp # The board MPU family (used for grouping by the CI system) FEATURES_MCU_GROUP = mips32

haukepetersen · 2016-12-12T15:49:21Z

boards/mips-malta/include/board.h

+ * @file
+ * @brief       Board specific definitions for the MIPS Malta FPGA System.
+ *
+ * @author      Imagination Technologies.


The author should AFAIK be person(s)

does it matter, I'm not vain ;-)

Actually, yes. We want to know who to ask when something breaks. ;)

haukepetersen · 2016-12-12T15:49:33Z

boards/mips-malta/include/board.h

+#endif
+
+/**
+ * @brief Set how many increments of the count register per uS


indention...

haukepetersen · 2016-12-12T15:52:22Z

boards/mips-malta/include/board.h

+ * @brief Set how many increments of the count register per uS
+ *        needed by timer code
+ */
+#define TICKS_PER_US (15)


If this is configuring the timer, I would expect it to be placed in the periph_conf.h

The timer uses it, its the number of CPU core ticks per uS, so its not necessarily peripheral specific, peripherals could be (and often are) running from a different clock.

I see, so never mind

haukepetersen · 2016-12-12T15:53:44Z

boards/mips-malta/include/periph_conf.h

+/*
+ * Copyright 2016, Imagination Technologies Limited and/or its
+ *                 affiliated group companies.
+ * This file is subject to the terms and conditions of the GNU Lesser


newline missing

haukepetersen · 2016-12-12T15:54:35Z

boards/mips-malta/malta.c

+#define MIPS_MALTA_ADDR    (0xbf000500)
+#define MIPS_MALTA_VAL_RST (0x42)
+
+static void malta_reset(void)


these addresses are CPU dependent, right? So they should go into the CPU code?!

No, they are specific to the Malta FPGA platform only.

haukepetersen · 2016-12-12T16:01:20Z

cpu/mips32r2_common/include/cpu.h

+ */
+static inline void cpu_init_early(void)
+{
+    /* This function must exist else RIOT won't compile */


Is this function mandatory by some parts of the MIPS toolchain or similar? RIOT itself does not care about it...

Nope, looks like that one can go.

haukepetersen · 2016-12-12T16:02:59Z

boards/mips-malta/Makefile.include

@@ -0,0 +1,3 @@
+export CPU = mips32r2_common
+export INCLUDES += -I$(RIOTBOARD)/$(BOARD)/include/
+export CFLAGS += -mhard-float -mdsp


How about a simple variable like USE_HARD_FLOAT and then checking that in the mips_common/Makefile.include and set the CFLAGS there?

haukepetersen · 2016-12-12T16:06:23Z

cpu/mips32r2_common/thread_arch.c

+    p--;
+    *p-- = STACK_END_PAINT;
+
+


?? where ? Do you want the double newline removing #74 or one adding between #71 and #72 ?

sorry, I meant to remove the double newline l74

haukepetersen · 2016-12-12T16:11:04Z

Sorry for the late review and about being so picky about formatting and such...

IMHO the PR looks quite good to me, mainly minor things that I noticed.

Now when trying to compile it, I get and error resulting from MIPS_ELF_ROOT not being set on my system. I simply downloaded the mips-mti-elf- MTI Bare Metal binaries tarball and used it right away. Seems like there is some more configuration effort needed here?!

neiljay · 2016-12-12T17:16:09Z

@haukepetersen

Thanks for the review.

Now when trying to compile it, I get and error resulting from MIPS_ELF_ROOT not being set on my system.

There are two toolchains for MIPS pre r6 (mips-mti-elf) and r6 onwards (mips-img-elf), MIPS_ELF_ROOT thus needs setting appropriately, I did add default paths to the makefile but Oleg asked me to remove them:

cpu/mips_pic32mx/Makefile
@@ -0,0 +1,15 @@
+ifeq ("$(OS)","Windows_NT")
+MIPS_ELF_ROOT?=C:\cross-mips-mti-elf
+else
+MIPS_ELF_ROOT?=/opt/imgtec/Toolchains/mips-mti-elf/2016.05-03
+endif

@OlegHahm on Sep 29 Member

I don't think we want to have absolute paths here. 😉

@neiljay on Sep 30

They are '?=' ie defaults if nothing else is set and are the default locations the toolchain installs too, but I can remove them if that's what you prefer.

@OlegHahm on Oct 13 Member

Yes, I would prefer to remove the absolute paths.

OlegHahm · 2016-12-12T17:24:20Z

Hm, okay, maybe I misjudged. Not sure. On the one hand, I still don't like to have these absolute paths in the build system, on the other hand that's probably the most convenient to do for most of the users.

@haukepetersen, do you have a preference?

haukepetersen · 2016-12-13T11:55:21Z

Hmm, I think not setting a default value sounds better to me, but how about some descriptive error message in case the variable is not set?!

haukepetersen · 2016-12-13T12:01:35Z

Before I forget, there is one more important problem we have to solve before we can merge this PR: we need to integrate the MIPS toolchain into our CI system.

@kaspar030: is it easy to integrate the toolchain into Murdock?!

neiljay · 2016-12-16T12:05:47Z

Hmm, I think not setting a default value sounds better to me, but how about some descriptive error
message in case the variable is not set?!

done, along with the other changes you requested.

Neill

haukepetersen · 2016-12-16T15:34:20Z

Cool, thanks. I already triggered @kaspar030 offline, to include the MIPS toolchain into the CI system, so we can put Murdock onto this PR

kaspar030 · 2017-01-11T15:09:45Z

Cool, thanks. I already triggered @kaspar030 offline, to include the MIPS toolchain into the CI system, so we can put Murdock onto this PR

I'm on int. Are you guys using the installer SDK version, or one of the binary tarballs?

neiljay · 2017-01-11T15:26:33Z

Just the tarballs, you'll want the 'MTI Bare Metal' ones, not 'IMG' ones (or 'GNU Linux' ones).

…

On 01/11/17 15:09, Kaspar Schleiser wrote: > Cool, thanks. I already triggered @kaspar030 offline, to include the MIPS toolchain into the CI system, so we can put Murdock onto this PR I'm on int. Are you guys using the installer SDK version, or one of the binary tarballs? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6060 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AUEU6pmSXVoilK7MfbBXPiXHG4ufN2A2ks5rRPC8gaJpZM4KphNc>.

kaspar030 · 2017-01-11T16:28:50Z

Thanks @neiljay.

The Docker image update is PR'ed here: RIOT-OS/riotdocker#24

Once that is merged, I'll update the image Murdock is using, and we should be able to test this.

kaspar030 · 2017-01-13T20:48:49Z

Murdock should be ready, let's see!

OlegHahm · 2017-01-15T15:15:26Z

The CI found a couple of problems.

PeterKietzmann · 2017-01-15T15:46:33Z

@neiljay would you please rebase to latest master?

neiljay · 2017-01-16T18:31:12Z

The rebase was clean, so I pushed it without a compile check first :-s
But it doesn't build.
It appears at some point, someone has turned on C99 in your build system: -std=c99
This breaks in-line asm code (as this is just a GCC'ism).
I assume you have hit this elsewhere ? What is your preferred solution ?

OlegHahm · 2017-01-16T18:42:19Z

See #5824 for the change. __asm__ should work.

cpu/mips32r2_common adds base architecture support for mips32r2 cores it can be built in it own right as a 'CPU', but is dependant on a bootloader (like u-boot) to have bootstrapped the system, this has been tested on a 'malta' FPGA system (BOARD=mips-malta) with various mips32r2 compliant cores (interAptiv, P5600, etc).

Add basic support for the MIPS malta FPGA platform.

No UART is available on the mips-malta board so blacklist this board.

kaspar030 · 2017-02-09T12:50:07Z

tests/leds/main.c

 #include "board.h"
 #include "periph_conf.h"

+


this one wasn't here before...

hmm how did that get in, I'll fix.

It has no UART.

neiljay · 2017-02-09T15:16:08Z

@haukepetersen Can you please revert your review request so we can finally get this merged!

haukepetersen on Dec 12, 2016 Member

these addresses are CPU dependent, right? So they should go into the CPU code?!
@neiljay
neiljay on Dec 12, 2016 Member

No, they are specific to the Malta FPGA platform only.
@haukepetersen
haukepetersen on Dec 13, 2016 Member

ah ok.

jnohlgard · 2017-02-09T15:31:04Z

Please update dist/tools/ci/print_toolchain_versions.sh to include a version line for the compiler and libc. This will let you see the toolchain version in the output from the static tests on Jenkins and Murdock, makes it easier to track down problems.

neiljay · 2017-02-09T15:44:27Z

@gebart In a separate PR ?

jnohlgard · 2017-02-09T15:45:56Z

@neiljay I'm fine with having the info script update in a separate PR.

neiljay · 2017-02-09T15:57:15Z

@gebart see #6585

kaspar030

There's minor stuff, but I think we should get this in and improve later. ACK.

kaspar030 · 2017-02-09T16:59:49Z

@haukepetersen I'll dismiss your review, reading your comment it seems your comment has been addressed.

@haukepetersen

All comments addressed, last one @haukepetersen said "ahok".

kaspar030 · 2017-02-09T17:01:07Z

Congrats @neiljay for adding the first new architecture in years! Thanks for all the work!

neiljay · 2017-02-09T17:05:18Z

Thanks all !

ghost · 2017-02-09T17:07:20Z

Thank you very much everyone for your great work !

miri64 · 2017-02-09T17:42:53Z

At last! Congratz and thanks for your big contribution and your patience @neiljay :party: ✨

emmanuelsearch · 2017-02-09T18:11:38Z

yay! Congrats!

kYc0o · 2017-02-10T14:13:29Z

Wow congrats! I'm looking forward to test it! Thanks to all people here.

jnohlgard · 2017-02-10T14:55:33Z

Congratulations on getting this large PR merged!
@neiljay I did run into some trouble trying to build this on my machine, as well as when testing #5616 on Jenkins. I have create a tracking issue for the issues in #6588. Please don't take this as a lack of appreciation, I think it's great that we are getting new hardware supported, I only want to improve on it.

miri64 assigned thomaseichinger and haukepetersen Nov 4, 2016

miri64 added this to the Release 2017.01 milestone Nov 4, 2016

miri64 changed the title ~~Pr/add mips32r2 common v2~~ mips: Import of mips32r2_common module Nov 4, 2016

This was referenced Nov 7, 2016

cpu: Add PIC32MZ support #6066

Merged

cpu: Add PIC32MX Support #6092

Merged

haukepetersen previously requested changes Dec 12, 2016

View reviewed changes

kaspar030 mentioned this pull request Jan 11, 2017

add mips toolchain RIOT-OS/riotdocker#24

Merged

kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 13, 2017

Neil Jones and others added 7 commits February 9, 2017 12:16

cpu: mips32r2_common: Add make infrastructure for mips.

eb99851

boards: mips-malta: Add basic malta FPGA support.

d09fd77

Add basic support for the MIPS malta FPGA platform.

make: export MIPS_ELF_ROOT in Makefile.buildtest subtargets

64c12da

gnrc_boarder_router: blacklist mips-malta board

bc57f42

No UART is available on the mips-malta board so blacklist this board.

sys: fmt: include stdio.h for mips.

e3c8386

sys: od: Fix build on MIPS gcc.

4d7b7a8

neiljay force-pushed the pr/add_mips32r2_common_v2 branch from d8a66c4 to 00f2ae3 Compare February 9, 2017 12:45

kaspar030 reviewed Feb 9, 2017

View reviewed changes

kaspar030 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 9, 2017

Neil Jones added 2 commits February 9, 2017 13:05

tests: Add mssing includes for leds and xtimer test

fd9ce7a

test: slip: blacklist mips-malta board

8434f9c

It has no UART.

neiljay force-pushed the pr/add_mips32r2_common_v2 branch from 00f2ae3 to 8434f9c Compare February 9, 2017 14:20

kaspar030 approved these changes Feb 9, 2017

View reviewed changes

kaspar030 merged commit ef958cc into RIOT-OS:master Feb 9, 2017

neiljay deleted the pr/add_mips32r2_common_v2 branch February 9, 2017 17:05

Conversation

neiljay commented Nov 4, 2016

Uh oh!

miri64 commented Nov 4, 2016

Uh oh!

neiljay commented Nov 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Dec 12, 2016

Uh oh!

neiljay commented Dec 12, 2016

Uh oh!

OlegHahm commented Dec 12, 2016

Uh oh!

haukepetersen commented Dec 13, 2016

Uh oh!

haukepetersen commented Dec 13, 2016

Uh oh!

neiljay commented Dec 16, 2016

Uh oh!

haukepetersen commented Dec 16, 2016

Uh oh!

kaspar030 commented Jan 11, 2017 via email

Uh oh!

neiljay commented Jan 11, 2017 via email

Uh oh!

kaspar030 commented Jan 11, 2017

Uh oh!

kaspar030 commented Jan 13, 2017

Uh oh!

OlegHahm commented Jan 15, 2017

Uh oh!

PeterKietzmann commented Jan 15, 2017

Uh oh!

neiljay commented Jan 16, 2017

Uh oh!

OlegHahm commented Jan 16, 2017

Uh oh!

Choose a reason for hiding this comment