Skip to content

Fastmodels support: add FVP_MPS2 targets to mbed os #6862

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
May 23, 2018
Merged

Fastmodels support: add FVP_MPS2 targets to mbed os #6862

merged 11 commits into from
May 23, 2018

Conversation

jamesbeyond
Copy link
Contributor

Description

  • add TARGET_ARM_FM top level folder in targets

  • add TARGET_FVP_MPS2 platform category

  • add following targets to the FVP_MPS2 category

    • FVP_MPS2_M0
    • FVP_MPS2_M0P
    • FVP_MPS2_M3
    • FVP_MPS2_M4
    • FVP_MPS2_M7

  • add all necessary drivers for FVP_MPS2 platfrom and each target

  • add toolchian scripts for ARM, GCC_ARM and IAR

  • modify target.json enable mbed for fastmodel FVP_MPS2 targets

Pull request type

[ ] Fix
[ ] Refactor
[x] New target
[ ] Feature
[ ] Breaking change

@jamesbeyond
Copy link
Contributor Author

@bulislaw Could you review this PR, or add the people who can review this RP. Seems I don't have permission to raise review request

@bulislaw
Copy link
Member

@ARMmbed/mbed-os-maintainers can we add @jamesbeyond to the ArmMbed org please?

@bulislaw
Copy link
Member

@jamesbeyond some checks are failing. Also where did you get all this code from? What about tools changes are they coming in separate PR?

@jamesbeyond
Copy link
Contributor Author

@bulislaw Travis failing seem because of Travis server issue. I am not sure how I can re-trigger Travis,

This PR only about add the Fastmodel targets to mbed os.

Most of the code I got from the hardware ARM_MPS2_Mx target drivers, I had some modification to suit the models where necessary

The changes for the tools will come in separate PRs

@adbridge
Copy link
Contributor

@bulislaw Qinghao is already a member of mbed-staff..Only a select few people can add reviewers :)

bulislaw
bulislaw previously approved these changes May 11, 2018
View reviewed changes
Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've skimmed the files, it looks good.

OPpuolitaival
OPpuolitaival previously approved these changes May 11, 2018
View reviewed changes
theotherjimmy
theotherjimmy suggested changes May 11, 2018
View reviewed changes
targets/targets.json Outdated
"FVP_MPS2_M0": {
"inherits": ["ARM_FM_Target"],
"core": "Cortex-M0",
"supported_toolchains": ["GCC_ARM", "ARM", "IAR"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to ARM_FM_Target

targets/targets.json Outdated
@@ -4008,5 +4008,60 @@
"detect_code": ["7013"],
"release_versions": ["5"],
"bootloader_supported": true
},
"ARM_FM_Target": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the redundant Target

targets/targets.json Outdated
"inherits": ["ARM_FM_Target"],
"core": "Cortex-M0",
"supported_toolchains": ["GCC_ARM", "ARM", "IAR"],
"extra_labels": ["ARM_FM", "FVP_MPS2", "FVP_MPS2_M0"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move "ARM_FM" (should be the parent name) and "FVP_MPS2" to the parent, and change this to an extra_labels_add

targets/targets.json Outdated
"core": "Cortex-M0",
"supported_toolchains": ["GCC_ARM", "ARM", "IAR"],
"extra_labels": ["ARM_FM", "FVP_MPS2", "FVP_MPS2_M0"],
"OUTPUT_EXT": "elf",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to parent.

targets/targets.json Outdated
"extra_labels": ["ARM_FM", "FVP_MPS2", "FVP_MPS2_M0"],
"OUTPUT_EXT": "elf",
"macros": ["CMSDK_CM0","CMSIS_VECTAB_VIRTUAL", "CMSIS_VECTAB_VIRTUAL_HEADER_FILE=\"cmsis_nvic.h\""],
"device_has": ["AACI", "ANALOGIN", "CLCD", "I2C", "INTERRUPTIN", "PORTIN", "PORTINOUT", "PORTOUT", "SERIAL", "SERIAL_FC", "SPI", "SPISLAVE", "TSC"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't duplicate the devices_has of the parent. If anything is added here, add it with extra_labels_add.

Qinghao Shi added 7 commits May 14, 2018 11:30
FastModels: add a new parent platform ARM_FM
2c88a83 
create a new platform folder TARGET_ARM_FM
add general drivers files for FVP_MPS2
FastModels: add FVP_MPS2_M3 target support
2e4a023 
add cmsis drivers and toolchain scripts for FVP_MPS2_M3
FastModels: add FVP_MPS2_M4 target support
ce2c21a 
add cmsis drivers and toolchain scripts for FVP_MPS2_M4
FastModels: add FVP_MPS2_M0 target support
67abec3 
add cmsis drivers and toolchain scripts for FVP_MPS2_M0
FastModels: add FVP_MPS2_M0P target support
1bf59cd 
add cmsis drivers and toolchain scripts for FVP_MPS2_M0P
FastModels: add FVP_MPS2_M7 target support
76dc715 
add cmsis drivers and toolchain scripts for FVP_MPS2_M7
FastModels: enable FVP_MPS2 targets for mbed os
953a735 
add ARM_FM top level category
add second level parent FVP_MPS2 inherits ARM_FM
add each target ( M0 M0P M3 M4 M7 ) inherits FVP_MPS2
adbridge
adbridge requested changes May 17, 2018
View reviewed changes
Copy link
Contributor

@adbridge adbridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The level of formatting in the majority of these files does not meet the coding guidelines, they all need to be tidied up.

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/PeripheralNames.h
} ADCName;

typedef enum {
SPI_0 = (int)MPS2_SSP1_BASE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indentation levels

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/PinNames.h Outdated
#endif

typedef enum {
PIN_INPUT,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be 4 spaces

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/PinNames.h Outdated

typedef enum {
// MPS2 EXP Pin Names
EXP0 = 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be 4 spaces

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/SDK/ETH_MPS2.c Outdated

error = 0;
val = SMSC9220->MAC_CSR_CMD;
if(!(val & ((unsigned int)1 << 31))) { // Make sure there's no pending operation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the guidelines to see if there is supposed to be a space after 'if'

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/SDK/ETH_MPS2.c Outdated
if(!timedout) {
error = 1;
}
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be -

if(!timedout) {
error = 1;
} else {
...
}

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/ethernet_api.c Outdated

if(smsc9220_reset_phy()) {
error = TRUE;
return error;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this error return immediately but the ones above don't? Also if returning immediately why is the error variable even used ? Why not just return TRUE ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Through this code was not originally from me, I guess that is for readability.
let people know this returns an error value, rather than miss leading people by TRUE implicates successful.

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/ethernet_api.c Outdated

int ethernet_write(const char *data, int size)
{
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/ethernet_api.c Outdated

int ethernet_send()
{
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/gpio_api.c
@@ -0,0 +1,140 @@
/* mbed Microcontroller Library
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting in this file is horrible, please conform to the coding guidelines

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/gpio_irq_api.c
@@ -0,0 +1,241 @@
/* mbed Microcontroller Library
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File needs reformatting to meet coding standards

@adbridge
Copy link
Contributor

@theotherjimmy can you re-review the changes to the target stuff on Monday when you are back please

@jamesbeyond
Copy link
Contributor Author

Hi @adbridge, all of your comments about formatting been addressed, Could you review again?

theotherjimmy
theotherjimmy suggested changes May 21, 2018
View reviewed changes
targets/targets.json Outdated
"inherits": ["ARM_FM"],
"public": false,
"supported_toolchains": ["GCC_ARM", "ARM", "IAR"],
"extra_labels_add": ["FVP_MPS2"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant. This already happens because of the target name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't realize that was automatically added. Now got all of these redundant "extra_label_add" removed. can you review pls @theotherjimmy

targets/targets.json Outdated
"FVP_MPS2_M0": {
"inherits": ["FVP_MPS2"],
"core": "Cortex-M0",
"extra_labels_add": ["FVP_MPS2_M0"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant. This already happens because of the target name.

targets/targets.json Outdated
"FVP_MPS2_M0P": {
"inherits": ["FVP_MPS2"],
"core": "Cortex-M0+",
"extra_labels_add": ["FVP_MPS2_M0P"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant. This already happens because of the target name.

targets/targets.json Outdated
"FVP_MPS2_M3": {
"inherits": ["FVP_MPS2"],
"core": "Cortex-M3",
"extra_labels_add": ["FVP_MPS2_M3"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant. This already happens because of the target name.

targets/targets.json Outdated
"FVP_MPS2_M4": {
"inherits": ["FVP_MPS2"],
"core": "Cortex-M4",
"extra_labels_add": ["FVP_MPS2_M4"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant. This already happens because of the target name.

targets/targets.json Outdated
"FVP_MPS2_M7": {
"inherits": ["FVP_MPS2"],
"core": "Cortex-M7",
"extra_labels_add": ["FVP_MPS2_M7"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant. This already happens because of the target name.

theotherjimmy
theotherjimmy previously approved these changes May 21, 2018
View reviewed changes
Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@jamesbeyond jamesbeyond dismissed stale reviews from theotherjimmy and OPpuolitaival via 304b584 May 22, 2018 10:26
@jamesbeyond
Copy link
Contributor Author

@adbridge could you review again pls

adbridge
adbridge approved these changes May 22, 2018
View reviewed changes
targets/TARGET_ARM_FM/TARGET_FVP_MPS2/SDK/ETH_MPS2.c Outdated
error = 1;
}
else
} else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ } needed around single statements

@adbridge
Copy link
Contributor

/morph build

0xc0170
0xc0170 approved these changes May 22, 2018
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question regarding empty functions in HAL.

targets/TARGET_ARM_FM/TARGET_FVP_MPS2/serial_api.c
{
}
void serial_set_flow_control(serial_t *obj, FlowControl type, PinName rxflow, PinName txflow)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why flow control is emtpy? is it enabled? will it be supported?

Similar is to pinmap functionality (assume that target does not have any pinmap functionality, neither break set/clear) thus they are emptY?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fastmodels and the hardware it modeling after don't support for flow control.
The details can be found in bellow reference guide:

@mbed-ci
Copy link

mbed-ci commented May 22, 2018

Build : FAILURE

Build number : 2099
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6862/

@0xc0170
Copy link
Contributor

0xc0170 commented May 22, 2018

@studavekar Please review the latest build ([Errno 28] No space left on device error)

@jamesbeyond
Copy link
Contributor Author

Hi @studavekar, any update on this build error?

@cmonr
Copy link
Contributor

cmonr commented May 22, 2018

Should be resolved now.
/morph build

@mbed-ci
Copy link

mbed-ci commented May 23, 2018

Build : SUCCESS

Build number : 2113
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6862/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented May 23, 2018

Test : SUCCESS

Build number : 1914
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6862/1914

@mbed-ci
Copy link

mbed-ci commented May 23, 2018

Exporter Build : SUCCESS

Build number : 1748
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6862/

@cmonr cmonr merged commit a404a93 into ARMmbed:master May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OPpuolitaival OPpuolitaival OPpuolitaival approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@adbridge adbridge adbridge approved these changes

@bulislaw bulislaw bulislaw left review comments

@theotherjimmy theotherjimmy theotherjimmy left review comments

@c1728p9 c1728p9 Awaiting requested review from c1728p9

Assignees
No one assigned
Labels
release-version: 5.9.0-rc1 scope: new-target
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jamesbeyond @bulislaw @adbridge @mbed-ci @0xc0170 @cmonr @theotherjimmy @OPpuolitaival