ports: mips: Add Support for mips32r2 (v2)#5885
ports: mips: Add Support for mips32r2 (v2)#5885neiljay wants to merge 33 commits intoRIOT-OS:masterfrom
Conversation
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.
cpu/mips_pic32mz adds basic support for Microchip PIC32MZ devices, including bootstrapping the device, the build system produces a hex image loadable via MPLAB-IPE. It has been tested on a Digilent Wifire board.
cpu/mips_pic32mx adds basic support for Microchip PIC32MX devices, including bootstrapping the device, the build system produces a hex image loadable via MPLAB-IPE. It has been tested on a MikroElectronic Clicker board.
Add basic support for the Digilent Wifire board which features a PIC32MZ device.
Add basic support for the MicroElectonika Clicker board, which features a pic32mz device.
miri64
added
Type: enhancement
|
I think this PR would benefit from some uncrustifying. |
|
Could you advice a good hardware platform to purchase to test this PR? |
|
On 09/28/16 23:21, Oleg Hahm wrote:
I ran it through checkpatch.pl from the linux kernel, the only errors
|
|
On 09/28/16 23:22, Oleg Hahm wrote:
I would recommend the Digilent Wifire: and The MicroElectronica Clicker board, unfortunately MicroE are having Or if cost is an issue, this is pretty cheap (but has a big/fast pic32): I could potentially arrange donation of hardware to the RIOT project,
|
Having ran through uncrustify the only change appear to be swapping tabs for spaces.
|
Files now uncrustified. |
|
Thanks for your fast response!
Such a test system is under development and we should definitely integrate a MIPS platform as soon as it is ready and this PR is merged. |
|
|
||
| /* Note this file must exist for xtimer code to build */ | ||
| #define TIMER_NUMOF (1U) | ||
| #define TIMER_0_CHANNELS 3 |
There was a problem hiding this comment.
I would prefer to have parens around macro definitions in a consistent style, i.e. either always put them (I would prefer that) or never put them.
cpu/mips32r2_common/irq_arch.c
Outdated
| { | ||
| if (state & SR_IE) { | ||
| mips32_bs_c0(C0_STATUS, SR_IE); | ||
| } else { |
cpu/mips32r2_common/lpm_arch.c
Outdated
|
|
||
| void lpm_arch_init(void) | ||
| { | ||
|
|
There was a problem hiding this comment.
Could you please add according TODO comments here and everywhere else where applicable?
There was a problem hiding this comment.
I have implemented all that is possible for power management in the base mips32r2 architecture (ie execute a 'wait' instruction), anything else would be SoC specific.
I will delete these unimplemented functions.
cpu/mips_pic32mx/Makefile
Outdated
| MIPS_ELF_ROOT?=C:\cross-mips-mti-elf | ||
| else | ||
| MIPS_ELF_ROOT?=/opt/imgtec/Toolchains/mips-mti-elf/2016.05-03 | ||
| endif |
There was a problem hiding this comment.
I don't think we want to have absolute paths here. 😉
There was a problem hiding this comment.
They are '?=' ie defaults if nothing else is set and are the default locations the toolchain install too, but can remove them it thats what you prefer.
There was a problem hiding this comment.
Yes, I would prefer to remove the absolute paths.
| /* Pass all other exceptions through to the toolchain handler */ | ||
| __exception_handle(ctx, exception); | ||
| } | ||
|
|
There was a problem hiding this comment.
Haven't checked for all the other files, but at least this one has tabs instead of spaces.
There was a problem hiding this comment.
Are you sure you pulled the latest version, uncrustify changed all the tabs to spaces ?
cpu/mips_pic32mx/include/cpu_conf.h
Outdated
| #define THREAD_EXTRA_STACKSIZE_PRINTF (2048) | ||
| #endif | ||
| #ifndef THREAD_STACKSIZE_DEFAULT | ||
| #define THREAD_STACKSIZE_DEFAULT (4096) |
There was a problem hiding this comment.
Out of curiosity: are stacks really required to be that big on MIPS or is this just a very conservative guess for now?
There was a problem hiding this comment.
Debug builds possibly, we have no interrupt stack currently and I'm yet to push FPU + DSP + MSA context saving.
On the subject of debug builds is there a RIOT standard way of doing debug vs release builds and a define we can use, so we can have different sizes for stacks for debug vs release ?
There was a problem hiding this comment.
Are there other space requirements accept for printf you need for debug? If not, than just add THREAD_EXTRA_STACKSIZE_PRINTF to the stacksize if debug is enabled.
There was a problem hiding this comment.
Well code built at -O0 is very stupid and always uses the stack, so stack usage is always higher in debug builds than release.
What symbol(s) do you use to differentiate between a debug vs release build in RIOT ?
Refactor out common code into cpu/mips_pic32_common. Move configuration settings to board files as this is a more natural fit, different boards with the same CPU may require diferent config bit settings. Add CPU register definition files and clean-up some literal usage.
|
I have re-factored the pic32 support to remove common code, restructured it somewhat and fixed various style issues. |
Clock out is enabled when the setting is 0 not 1. Clock out is not connected up on the Wifire board so turn it off.
boards/mips-malta/Makefile.include
Outdated
| @@ -0,0 +1 @@ | |||
| # This file must exist for RIOT to build. | |||
There was a problem hiding this comment.
This file should at least add the include directory to RIOT's include path, so that one can include the "board.h".
| @@ -0,0 +1 @@ | |||
| /* This file must exist to get timer code to build */ | |||
There was a problem hiding this comment.
This file should go into the cpu directory and contain the MCU's peripheral configuration
boards/mips-malta/malta.c
Outdated
|
|
||
| void board_init(void) | ||
| { | ||
|
|
There was a problem hiding this comment.
Who's performing the board's and CPU's initialization then?
There was a problem hiding this comment.
Malta is our FPGA evaluation platform it has a bootloader pre-installed, typically u-boot.
There was a problem hiding this comment.
Then maybe you can add a comment explaining this here.
| @@ -0,0 +1 @@ | |||
| #This file must exist for RIOT to build. | |||
cpu/mips32r2_common/periph/timer.c
Outdated
| unsigned int timer_read(tim_t dev) | ||
| { | ||
| if (dev != 0) { | ||
| return -1; |
There was a problem hiding this comment.
Maybe we should rather for an assert here anyway.
cpu/mips32r2_common/periph/timer.c
Outdated
| compares[i] = 0; | ||
| } | ||
|
|
||
| counter = 1 << TIMER_ACCURACY_SHIFT; |
cpu/mips32r2_common/periph/timer.c
Outdated
| /* Clear down soft counters */ | ||
| for (i = 0; i < CHANNELS; i++) { | ||
| compares[i] = 0; | ||
| } |
cpu/mips32r2_common/periph/timer.c
Outdated
|
|
||
| if (channel >= CHANNELS) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
I would prefer asserts here, too.
cpu/mips_pic32mx/Makefile
Outdated
| MIPS_ELF_ROOT?=C:\cross-mips-mti-elf | ||
| else | ||
| MIPS_ELF_ROOT?=/opt/imgtec/Toolchains/mips-mti-elf/2016.05-03 | ||
| endif |
There was a problem hiding this comment.
Yes, I would prefer to remove the absolute paths.
cpu/mips_pic32mx/include/cpu_conf.h
Outdated
| #define THREAD_EXTRA_STACKSIZE_PRINTF (2048) | ||
| #endif | ||
| #ifndef THREAD_STACKSIZE_DEFAULT | ||
| #define THREAD_STACKSIZE_DEFAULT (4096) |
There was a problem hiding this comment.
Are there other space requirements accept for printf you need for debug? If not, than just add THREAD_EXTRA_STACKSIZE_PRINTF to the stacksize if debug is enabled.
|
Sorry, that it took a while. |
|
@OlegHahm, No worries, I was feeling a little unappreciated last week. |
|
How do you build the doxygen docs ? ~/src/RIOT/doc/doxygen] |
|
I just tried this on master (I usually build from root with |
$ doxygen --version ( I'm using a pretty old version of CentOS ) |
|
Mhhh can you maybe try it with our vagrant image if you have the time? In the end the CI server will check this, too but it would make things quicker if we can reduce the load on that. |
|
I've installed 1.8.12 which seems to work, albeit lots of warnings / errors. |
|
That's normal :D we try to reduce those ;-). |
|
I just realized that the |
|
Ah ok, so you can just do: make BOARD='pic32-wifire' and not have to explicitly set the CPU it uses ? That makes sense. |
|
Exactly. |
|
Okay, I succeeded to build the hello-world example for the pic32-clicker, but how am I supposed to flash the binary onto the board? |
Both of them, but I tried first with the clicker. |
|
So, do I get this right: there is no programming device or serial bootloader available on the boards (clicker and wifire) that would make it possible to just flash the device using a USB cable? I.e., we first need to purchase some mips JTAG device or similar to flash it, right? |
|
Cool, The wifire is probably better as an initial development platform as it has JTAG pins exposed + it has more flash + ram. But the radio is Wifi and documentation on it isn't great. The pic32 clicker doesn't have JTAG bonded out and is a smaller slower PIC32 device but it does have a more interesting 6LowPan radio, and will probably be the first MIPS board to get radio support added for that reason. details on how to program the clicker are here: |
Yes, unless you have the very latest revD wifire which now includes the FTDI JTAG device on the board. Well actually bootloaders are available for both board I think (certainly wifire) which support programming via USB - some AVR standard? But this obviously takes up flash space. The flash images produced for RIOT contain custom boot + app code without any reflash support, so once you flash a RIOT image you would loose your existing bootcode, also these (3rd party) bootloaders don't allow making use of advanced features like microMIPS code compression and from what I have seen are not 'production' ready. |
| * Note Microchip number the UARTS 1->4. | ||
| * @{ | ||
| */ | ||
| #define UART_NUMOF (6) |
There was a problem hiding this comment.
Seems like there is a logical layer missing here: RIOT makes use of 'logical' periph devices (e.g. UART_DEV(x)), which should be freely mappable to the actual hardware devices (e.g. UART1-6 or similar). This enables us to to be completely free when mapping peripheral devices and their pins. So would expect this mapping to be made here...
There was a problem hiding this comment.
Can you point me to an example to follow ?
boards/pic32-wifire/include/board.h
Outdated
| * @brief The peripheral clock is required for the UART Baud rate calculation | ||
| * It is configured by the 'config' registers (see pic32_config_settings.c) | ||
| */ | ||
| #define PERIPHERAL_CLOCK (100000000) /* Hz */ |
There was a problem hiding this comment.
For consistency over all boards, I would expect this kind of define in the periph_conf.h file...
| @@ -0,0 +1 @@ | |||
| export INCLUDES += -I$(RIOTBOARD)/$(BOARD)/include/ | |||
There was a problem hiding this comment.
Where do you define the CPU and the CPU_MODEL defines?
There was a problem hiding this comment.
Oleg just pointed this out and is now fixed.
There was a problem hiding this comment.
( I was always specifying CPU and BOARD at the prompt when building)
| #define PORTFCLR 0xBF860524 | ||
| #define TRISFCLR 0xBF860514 | ||
| #define TRISFSET 0xBF860518 | ||
| #define ODCFCLR 0xBF860544 |
There was a problem hiding this comment.
Are these really board specific?
There was a problem hiding this comment.
No, this is fixed in my WIP branch (https://github.com/neiljay/RIOT/tree/wip) there is another vendor supplied file with all these definitions in. I'll submit another PR for this, this one is already too big.
| @@ -0,0 +1,403 @@ | |||
| /* | |||
There was a problem hiding this comment.
I am not really familiar with the MIPS architecture: looking at this file, it seems like it contains many configuration options that I would expect to be done by their connected modules/periph drivers. Do all of these options need to be set during compile time? And are they read only later on? In the current form these options seem to be very hard to maintain/to configure for others...
There was a problem hiding this comment.
This has nothing to do with MIPS architecture. This is a weird quirk of the PIC32 implementation from Microchip (and there odd programming tools). These value map onto registers which overlay the flash, when a reflash occurs these register get 'erased' and must be programmed when a new firmware image is loaded and thus these values must exist at the correct addresses in the resultant elf file / binary image.
I agree its horrible but a necessity for PIC32 devices, Microchip in there own tools (XC32) have implemented a #pragma for these values which make it a bit nicer:
https://people.ece.cornell.edu/land/courses/ece4760/PIC32/PLIB_examples/plib_examples/uart/uart_basic/source/main.c
We don't have that ability in generic tools.
There was a problem hiding this comment.
(So yes they are effectively read-only once programmed)
| * Setup pin mux for UART4 this is the one connected | ||
| * to the ftdi chip (usb<->uart) | ||
| */ | ||
| w32(U4RXR, 0xb); /* connect pin RPF2 to UART 4 RX */ |
There was a problem hiding this comment.
Pin configuration should be part of the periph_conf.h and then be handled by the actual peripheral driver module (possibly by using the GPIO driver).
There was a problem hiding this comment.
I agree a whole pin-mux driver needs implemented and we have started work on one.
peripheral support is a bit hacky at the moment, we just wanted to get the UART going so we can show that the 'core' architecture port works. get that merged then improve pic32 device support so it can be used for actual products.
| @@ -0,0 +1,3 @@ | |||
| /* This file must exist to get timer code to build */ | |||
There was a problem hiding this comment.
hm, as there are periph_cpu.h files in the specific cpu folders, there should be no need for this file here? Seems like something is not quite in order if this file is really needed...
There was a problem hiding this comment.
mips32r2_common can be selected as a CPU on its own for testing on FPGA systems with minimal peripherals, this is what BOARD=mips-malta uses.
| #endif | ||
|
|
||
| #define ISR_STACKSIZE (0) | ||
| /** @} */ |
There was a problem hiding this comment.
These values are equal for all mips32r2 cpus, right? So I would suggest to move them into cpu/mips32r2_common/include/cpu.h as done for the cortexm_common cpus.
There was a problem hiding this comment.
Currently yes. But they may change in the future.
pic32mz supports FPU, pic32mx does not for example.
| /* pic uarts are numbered 1 to 6 */ | ||
| PIC32_UART_T pic_uart[UART_NUMOF + 1]; | ||
|
|
||
| int uart_init(uart_t uart, uint32_t baudrate, uart_rx_cb_t rx_cb, void *arg) |
There was a problem hiding this comment.
are incoming chars handled anywhere?
There was a problem hiding this comment.
No not yet.
As said previously we just want to get something going to prove the core architecture port works on real hardware.
It will be coming of course.
| #define UxTXREG(U) (U.regs[0x20/4]) | ||
| #define UxRXREG(U) (U.regs[0x30/4]) | ||
| #define UxBRG(U) (U.regs[0x40/4]) | ||
| #define REGS_SPACING (0x200) |
There was a problem hiding this comment.
I think it would make sense to put these into a header file?! And are the UART peripherals the same for each mips32r2 based CPU? Or is this like with other MCU families, where each vendor uses their own UART peripheral?
There was a problem hiding this comment.
This is in 'pic32_common' not 'mips32r2_common' this UART is common across all PIC32 implementations, obviously not across all mips32r2 implementations, I'm happy to move these to a header file though.
Remove unused defines Remove duplicate defines available in cpu part files. Make uart data static.
|
Hi All, After discussion with Oleg and Martine, I'll split this up into 3 separate PRs. Thanks for the reviews so far. |
|
@neiljay I didn't check in detail. Can we close this in favour of the above mentioned three PRs? If yes, please do so. |
7 participants
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).
cpu/mips_pic32mz adds basic support for Microchip PIC32MZ devices, including
bootstrapping the device, the build system produces a hex image loadable via
MPLAB-IPE. It has been tested on a Digilent Wifire board.
cpu/mips_pic32mx adds basic support for Microchip PIC32MX devices, including
bootstrapping the device, the build system produces a hex image loadable via
MPLAB-IPE. It has been tested on a MikroElectronic Clicker board.
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/
You cannot use Microchip's MPLAB / Harmony tools to build this port.
All 3 'CPU' types have been tested against the following examples:
hello-world
ipc_pingpong
timer_periodic_wakeup
riot_and_cpp
The following CPU / BOARD combinations are valid:
CPU=mips32r2_common BOARD=mips-malta
CPU=mips_pic32mz BOARD=pic32-wifire
CPU=mips_pic32mx BOARD=pic32-clicker
Changes in v2:
Split into multiple logical commits.
Add LGPL license boilerplate.
Removed some literal usage in pic32 device setup.