Enable CM3DS_MPS2 target #4414
Merged
Enable CM3DS_MPS2 target #4414
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
greentea logs attached below: Can we get this merged for v5.5 if all OK? @0xc0170 , @sg- , @screamerbg , @paldwort , @abhishek-pandit |
|
Hi @0xc0170 , @sg- , @screamerbg , @paldwort , @abhishek-pandit Can you review the code so we can get this merged? Thanks, |
|
/morph test |
|
sorry, cant take the pr pipeline right now. |
Result: FAILUREYour command has finished executing! Here's what you wrote!
OutputBuild Prep failed! |
|
Thanks @tkaman for adding GCC and IAR support. I can confirm that I have tested "blinky" on the target with all 3 compilers and all of them are good to go. Attaching BUILD directory here for quick access. |
|
my bad @sg-, thanks for stopping it |
|
/morph test |
bridadan
suggested changes
Jun 5, 2017
| MOSI_SPI = 300, | ||
| MISO_SPI = 301, | ||
| SCLK_SPI = 302, | ||
| SSEL_SPI = 303, |
There was a problem hiding this comment.
We don't have a hard standard for this, however if you look through the code base, the majority of targets use the naming convention SPI_X instead of X_SPI. For example, SPI_MOSI instead of MOSI_SPI. This is not required, however you will be compatible with more example programs in the mbed ecosystem if you follow this convention.
| }else if (pin == 310){ | ||
| pin_value = pin-305; | ||
| }else if (pin == 311){ | ||
| pin_value = pin-305; |
There was a problem hiding this comment.
All these pin_values are being assigned pin-305. Is this correct? Or is this just a copy-paste error?
There was a problem hiding this comment.
These pin_values are calculated to match the bits in in the FPGAIO->MISC register
__IO uint32_t MISC; // Offset: 0x04C (R/W) Misc control */
// [31:10] : Reserved
// [9] : SHIELD_1_SPI_nCS
// [8] : SHIELD_0_SPI_nCS
// [7] : ADC_SPI_nCS
// [6] : CLCD_BL_CTRL
// [5] : CLCD_RD
// [4] : CLCD_RS
// [3] : CLCD_RESET
// [2] : RESERVED
// [1] : SPI_nSS
// [0] : CLCD_CS
|
|
||
|
|
||
|
|
||
| void i2c_frequency(i2c_t *obj, int hz) { |
There was a problem hiding this comment.
Why is this function empty? Does the target support setting the I2C frequency?
@0xc0170 If the target doesn't support setting an I2C frequency, should it throw a runtime error/assert?
There was a problem hiding this comment.
it needs to support it ! otherwise I2C should not be enabled
| } | ||
|
|
||
| int i2c_byte_read(i2c_t *obj, int last) { | ||
| return 0; |
There was a problem hiding this comment.
Why is this function empty?
There was a problem hiding this comment.
@bridadan This will be fixed and pushed it in a separate PR.
| } | ||
|
|
||
| int i2c_byte_write(i2c_t *obj, int data) { | ||
| return 0; |
There was a problem hiding this comment.
Why is this function empty?
There was a problem hiding this comment.
@bridadan This will be fixed and pushed it in a separate PR.
|
|
||
| void pin_function(PinName pin, int function) { | ||
| MBED_ASSERT(pin != (PinName)NC); | ||
|
|
There was a problem hiding this comment.
Why is this function empty?
There was a problem hiding this comment.
@bridadan This will be fixed and pushed it in a separate PR.
| } | ||
|
|
||
| void pin_mode(PinName pin, PinMode mode) { | ||
| MBED_ASSERT(pin != (PinName)NC); |
There was a problem hiding this comment.
Why is this function empty?
There was a problem hiding this comment.
@bridadan Pin mode is not supported
| */ | ||
| void rtc_free(void) | ||
| { | ||
| /* Not supported */ |
There was a problem hiding this comment.
@0xc0170 Any cause for concern here?
There was a problem hiding this comment.
@abhishek-pandit I understand the target may not support it, however I'm curious if this will affect expected behavior of the OS.
There was a problem hiding this comment.
It won't at the moment. free function does not specify the requirements. As soon as we have them, we provide tests and this will need to conform. It is fine as it is.
Plus this function is not used directly so far.
|
|
||
| } | ||
|
|
||
| void serial_format(serial_t *obj, int data_bits, SerialParity parity, int stop_bits) { |
There was a problem hiding this comment.
Why is this function empty?
There was a problem hiding this comment.
@bridadan The CMSDK APB UART is a simple design that supports 8-bit communication without parity, and is fixed at one stop bit per configuration. Ref. DDI0479C_cortex_m_system_design_kit_r1p0_trm.pdf. However, one comment and mbed error will be added in this function.
| void serial_break_clear(serial_t *obj) { | ||
| } | ||
| void serial_set_flow_control(serial_t *obj, FlowControl type, PinName rxflow, PinName txflow) { | ||
| } |
There was a problem hiding this comment.
@0xc0170 Not sure how supported these features are across the code base, any cause for concern? Specifically serial_break_set, serial_break_clear, serial_set_flow_control.
There was a problem hiding this comment.
@bridadan The CMSDK APB UART doesn't support serial break and flow control. Ref. DDI0479C_cortex_m_system_design_kit_r1p0_trm.pdf. However, one comment and mbed error will be added in this function.
0xc0170
requested changes
Jun 5, 2017
targets/targets.json
Outdated
| "supported_toolchains": ["ARM", "GCC_ARM", "IAR"], | ||
| "extra_labels": ["ARM_SSG", "CM3DS_MPS2"], | ||
| "macros": ["CMSDK_CM3DS"], | ||
| "device_has": ["AACI", "ANALOGIN", "CLCD", "ETHERNET", "I2C", "INTERRUPTIN", "PORTIN", "PORTINOUT", "PORTOUT", "SERIAL", "SERIAL_FC", "SPI", "SPISLAVE", "TSC", "RTC"], |
There was a problem hiding this comment.
where is this CLCD and TSC coming from ? I would suggest to use config. These are not valid device_has macros
There was a problem hiding this comment.
@0xc0170 This is due to copy from previous platform, I guess we can simply remove those two.
There was a problem hiding this comment.
@0xc0170 CLCD and TSC will be removed from device_has macros.
| US_TICKER_TIMER1->TimerControl |= 0x80; //enable timer | ||
| } | ||
|
|
||
| void us_ticker_disable_interrupt(void) { |
There was a problem hiding this comment.
please fix the style . { on the new line
There was a problem hiding this comment.
@0xc0170 I agree. It will be fixed
| delta = (int)(timestamp - us_ticker_read()); | ||
| if (delta <= 0) { | ||
| // This event was in the past: | ||
| us_ticker_irq_handler(); |
There was a problem hiding this comment.
should not call the IRQ directly (nested calls might result in the stack overflow). Can you please set pending interrupt here instead?
There was a problem hiding this comment.
@0xc0170 I agree. The new PR update contains the fix.
| void us_ticker_set_interrupt(timestamp_t timestamp) { | ||
| int delta = 0; | ||
|
|
||
| if (!us_ticker_inited) |
There was a problem hiding this comment.
Should be surrounded by { + }
There was a problem hiding this comment.
@0xc0170 I agree. It will be fixed
| break; | ||
| } | ||
|
|
||
| if (enable) |
There was a problem hiding this comment.
https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/ - should be if () {
There was a problem hiding this comment.
@0xc0170 I agree. It will be fixed
| } | ||
| } | ||
|
|
||
| void uart0_irq() {uart_irq(CMSDK_UART1->INTSTATUS & 0x3, 0, (CMSDK_UART_TypeDef*)CMSDK_UART1);} |
There was a problem hiding this comment.
one statement per line please
There was a problem hiding this comment.
@0xc0170 I agree. It will be fixed
| baudrate_div = SystemCoreClock / baudrate; | ||
| if(baudrate >= 16){ | ||
| switch ((int)obj->uart) { | ||
| case UART_0: CMSDK_UART1->BAUDDIV = baudrate_div; break; |
There was a problem hiding this comment.
one statement per line please
There was a problem hiding this comment.
@0xc0170 I agree. It will be fixed
| int baudrate_div = 0; | ||
| baudrate_div = SystemCoreClock / baudrate; | ||
| if(baudrate >= 16){ | ||
| switch ((int)obj->uart) { |
There was a problem hiding this comment.
@0xc0170 I agree. It will be fixed
|
Sorry for the delay there were few other Pr that came earlier. This can get into 5.5.1 earliest, if that is ok? |
Result: FAILUREYour command has finished executing! Here's what you wrote!
OutputBuild failed! |
|
To further clarify things from a release perspective, to make it into 5.5.1 , this has to have all review comments fixed and accepted, pass all the ci checks and be merged into master prior to the code freeze for 5.5.1 on Thu 15th June... |
|
@tkaman Please address the review comments |
|
As far as I can tell, this port was developed pre-RTX 5 upgrade. If that's the case, this port may need some additional work to bring it up to date. @abhishek-pandit Can you please address the build failures? As you may not be familiar with our CI system, I've placed a direct link to each build failure below. Be aware that our CI system always merges the PR branch into the latest master branch of mbed OS before doing any builds/tests, which is why the build may be working on your machine but not in CI. You will need to carry out this rebase on your machine as well to reproduce the errors
This target can't come into a release until it builds. |
|
Please make sure you rebase when updating your mbed-os rather than merging. Merge commits cause issues with our automated patch creation systems. Thanks |
|
@abhishek-pandit @mattot01 I know your team is under a tight deadline. If you have questions or concerns please be sure to voice them early so we can assist you in a timely manner. There are still quite a few unaddressed comments. |
|
@bridadan The build issues will be fixed in the next update of this PR. |
|
Test results running MBED GT tests on the target: |
bridadan
reviewed
Jun 7, 2017
| { | ||
| MBED_ASSERT(pin != (PinName)NC); | ||
|
|
||
| /* TODO */ |
There was a problem hiding this comment.
I'm pretty sure this is a crucial function of the mbed HAL.
This function is called by pinmap_pinout (https://github.com/ARMmbed/mbed-os/blob/master/hal/mbed_pinmap_common.c#L19)
And pinmap_pinout is called by your own port. So it seems like no pin mappings would actually work?
There was a problem hiding this comment.
It is called but function might be ignored if it is not required by a target (no need to set up specific function).
| { | ||
| MBED_ASSERT(pin != (PinName)NC); | ||
|
|
||
| /* Pin modes configuration is not supported */ |
There was a problem hiding this comment.
Same thing here, I'm pretty sure this is a crucial function of the mbed HAL.
There was a problem hiding this comment.
As the comment says, the target does not support changing the mode. Thus is the only one. There are couple of targets that have the same features - unsupported pin modes
| * without parity, and is fixed at one stop bit per configuration. | ||
| * Ref. DDI0479C_cortex_m_system_design_kit_r1p0_trm.pdf | ||
| */ | ||
| error("serial format function not supported"); |
There was a problem hiding this comment.
+1 for failing loudly instead of silently 😄
| */ | ||
| void rtc_free(void) | ||
| { | ||
| /* Not supported */ |
0xc0170
reviewed
Jun 8, 2017
| } | ||
| } | ||
|
|
||
| void us_ticker_disable_interrupt(void) { |
There was a problem hiding this comment.
please fix the formatting for disable and clear interrupt
|
|
||
| void spi_free(spi_t *obj) | ||
| { | ||
| error("SPI format error"); |
| (altfunction[3]<<CMSDK_GPIO_ALTFUNC_ADC_CS_SPI_SET); | ||
| break; | ||
| case (int)SPI_3: /* Shield 0 SPI */ | ||
| GPIO_MAP[CMSDK_GPIO_SH0_MOSI_SPI_GPIO_NUM]->ALTFUNCSET |= |
There was a problem hiding this comment.
curiosity - pin_mode not supported but this has some altfunction - isn't this pin mode or it is about something else?
There was a problem hiding this comment.
@0xc0170 This functionalty belongs to pin_function and not to pin_mode. pin_mode is not supported by the target.
| } | ||
| } | ||
|
|
||
| void port_dir(port_t *obj, PinDirection dir) { |
There was a problem hiding this comment.
{ new line for function body, in this file please
| { | ||
| MBED_ASSERT(pin != (PinName)NC); | ||
|
|
||
| /* TODO */ |
There was a problem hiding this comment.
It is called but function might be ignored if it is not required by a target (no need to set up specific function).
| } else { | ||
| CMSDK_GPIO_0->INTPOLCLR |= ch_bit; | ||
| if (enable) { | ||
| CMSDK_GPIO_0->INTENSET |= ch_bit; |
There was a problem hiding this comment.
fix the formatting in this file, 4 spaces, if () { etc, one statement per line (not case: do-sth(); break;)
| // * The interrupt was already served | ||
| // * There is no user handler | ||
| // * It is a level interrupt, not an edge interrupt | ||
| if (ch_bit <16){ |
| struct ethernetif { | ||
| const struct eth_addr *ethaddr; | ||
| int is_enabled; | ||
| sys_mutex_t tx_mutex; |
There was a problem hiding this comment.
4 spaces (2 are in this file all over?)
There was a problem hiding this comment.
@0xc0170 I agree. This file needs to be formatted to 4.
|
|
||
| uint32_t gpio_set(PinName pin) | ||
| { | ||
| /* TODO */ |
There was a problem hiding this comment.
not supported because target does not need to set the pin to gpio or not yet implemented?
There was a problem hiding this comment.
@0xc0170 not yet implemented. It will be implemented as the same time the pin_function is implemented and push it in a separate PR .
|
Test results running MBED GT tests on the target: |
0xc0170
requested changes
Jun 9, 2017
| define symbol __ICFEDIT_region_RAM_end__ = 0x2001FFFF; | ||
| /*-Sizes-*/ | ||
| /* Heap and Stack size */ | ||
| define symbol __ICFEDIT_size_cstack__ = 0x4000; |
There was a problem hiding this comment.
why cstack is much more bigger than heap? For mbed OS 5 this is value used for isr stack, this value is way too big. and heap too small then
There was a problem hiding this comment.
This is seems to be a mistake. Will be fixed in the next update of this PR.
- Creates new target in targets.json - Creates device specific files under ARM_SSG/CM3DS_MPS2 directory - Driver layer files under CM3DS_MPS2 are based on Beid target - Device specific files under CM3DS_MPS2/device are based on CMSIS_5 and Beetle Change-Id: I29ea7a7f42b11cf25b516cce4b9255ab828ca019 Signed-off-by: Tamas Kaman <[email protected]> Signed-off-by: Marc Moreno <[email protected]>
Refactor SMSC9220 Ethernet controller driver Change-Id: I75c3c42d5675441de1292100a54c50d990070c6f Signed-off-by: Gabor Kertesz <[email protected]>
Based on lwip_ethernetif.c skeleton file, use init, receive and transfer functionality of SMSC9220 Ethernet driver for the lightweight IP stack. Receive mechanism is interrupt driven. HW buffer sizes: Tx = 4608 bytes (MTU) Rx = 10560 bytes lwIP fine tuning: mbed-os/features/FEATURE_LWIP/lwip-interface/lwip/src/include/lwip/opt.h Change-Id: I0ea95650c65fb32cafb5c2d3dde11420c61dba66 Signed-off-by: Gabor Kertesz <[email protected]>
- Modify CMSDK_CM3DS.h: add register interface - Modify targets.json: add RTC as available device to CM3DS - Create rtc_api.c: implement mandatory API functions Change-Id: I14bc1074a9ac0d5e4cbada46d3c90ca82c1e28b0 Signed-off-by: Tamas Ban <[email protected]>
1. Add startup code and linker script for IAR and GCC_ARM compilers. 2. Enable IAR and GCC_ARM compilers in targets.json. Change-Id: I742a89ae73a4e5ede980a8af0821c3f0e5a461ef Signed-off-by: Mate Toth-Pal <[email protected]>
|
Test results running MBED GT tests on the target: |
0xc0170
approved these changes
Jun 12, 2017
bridadan
approved these changes
Jun 12, 2017
|
/morph test |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Notes:
Description
A few sentences describing the overall goals of the pull request's commits.
This pull request is to add support to CM3DS_MPS2 target in MBED-OS.
The changes are based on Beid, Beetle and CMSIS_5.
Status
READY/IN DEVELOPMENT/HOLD
Migrations
If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.
YES | NO
Related PRs
List related PRs against other branches:
Todos
Deploy notes
Notes regarding the deployment of this PR. These should note any
required changes in the build environment, tools, compilers, etc.
Name of the target is ARM_CM3DS_MPS2
Steps to test or reproduce
Outline the steps to test or reproduce the PR here.
Test results running MBED GT tests on the target:
mbed_gt_test_run_cm3ds.txt