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.

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

Silicon Labs QSPI HAL implementation #7825

Merged
merged 3 commits into from Aug 27, 2018

Conversation

Projects
None yet
5 participants
@stevew817
Contributor

stevew817 commented Aug 18, 2018

Description

Implemented the QSPI HAL on EFM32GG11, since that is our target which has QSPI available today. The port was verified working on the STK3701 and its on-board QSPI flash, using the supplied hal-qspi test.

Pull request type

[ ] Fix
[ ] Refactor
[X] Target update
[ ] Functionality change
[ ] Breaking change
@stevew817

This comment has been minimized.

Contributor

stevew817 commented Aug 18, 2018

Can be redirected to master once #7783 lands.

@0xc0170 0xc0170 requested a review from maciejbocianski Aug 20, 2018

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Aug 20, 2018

@stevew817 Did you run mbed_hal-qspi test on it?

@stevew817

This comment has been minimized.

Contributor

stevew817 commented Aug 20, 2018

@maciejbocianski Yes, I did. It passed all tests (43 of them).

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 22, 2018

@OPpuolitaival @ARMmbed/mbed-os-test How can jenkins-ci/cloud_client_smoke_test be restarted?

@stevew817 stevew817 changed the base branch from feature-qspi to master Aug 24, 2018

@stevew817 stevew817 force-pushed the SiliconLabs:siliconlabs-qspi branch from fcf2c1e to 58b14ac Aug 24, 2018

@stevew817

This comment has been minimized.

Contributor

stevew817 commented Aug 24, 2018

@maciejbocianski Can you formally approve this PR if you're happy with it?
@cmonr @0xc0170 Rebased on master now that #7783 has been merged. I'd appreciate it if this came into the same release as #7783.

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Aug 24, 2018

@stevew817
It could require another rebase, and pin names modification if #7817 will go in first

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 25, 2018

@maciejbocianski It looks like it doesn't need a rebase this time around.

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 25, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 25, 2018

@stevew817 Looks like I spoke too soon, but on the plus side, the fixes look simple.

@cmonr cmonr added needs: work and removed needs: CI labels Aug 25, 2018

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Aug 25, 2018

@stevew817 pin names should be changed to QSPI_FLASH1_XXX

stevew817 added some commits Aug 18, 2018

Initial commit of Silicon Labs QSPI HAL implementation
* For EFM32GG11, since that is the only Silicon Labs target with QSPI per today
* Verified working using the on-board flash and tests-mbed_hal-qspi
Allow unaligned input/output for QSPI
The code is written such that access to the data input/output happens word-by-word, and that means unaligned access is fine (though with a performance loss) on Cortex-M3/M4 devices.
Apply changes corresponding to #7817
QSPI standard pin names were changed after the QSPI feature PR.

@stevew817 stevew817 force-pushed the SiliconLabs:siliconlabs-qspi branch from 58b14ac to 55c6dad Aug 27, 2018

@stevew817

This comment has been minimized.

Contributor

stevew817 commented Aug 27, 2018

@cmonr @0xc0170 Rebased and updated pinnames, should now be according to spec.

@maciejbocianski Feedback post-mortem: It would've been nice to only have one pull request to have to look at when implementing a new feature PR. What was the reason for adding a change PR when the original feature PR wasn't even merged yet?

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Aug 27, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 27, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 27, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Aug 27, 2018

@cmonr cmonr merged commit 9135418 into ARMmbed:master Aug 27, 2018

15 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(-0.09%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job was successful
Details
travis-ci/astyle Passed, 555 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9103 cycles (-1524 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Aug 29, 2018

Feedback post-mortem: It would've been nice to only have one pull request to have to look at when implementing a new feature PR. What was the reason for adding a change PR when the original feature PR wasn't even merged yet?

@stevew817 you are right this should go this way, (the reason was to busy schedule)

@cmonr cmonr removed the risk: G label Sep 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment