-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add support for QSPI to Cypress Boards #10435
Conversation
@morser499, thank you for your changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise looks fine to me
|
||
|
||
|
||
// #define FAST_MODE_ENABLE() \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this code is commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the commented code. this is not supported by the PSoC 6 device
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the test "features-storage-tests-blockdevice-general_block_device" successful?
@@ -0,0 +1,259 @@ | |||
/* mbed Microcontroller Library | |||
* Copyright (c) 2018-2018 ARM Limited | |||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add SPDX identifiers to new files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Please also fix travis astyle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We would like to produce 5.12.2 release candidate today prior holidays, please let us know when the update can be done. |
}, | ||
"CY8CPROTO_062_4343W": { | ||
"QSPI_FREQ": "50000000", | ||
"QSPI_MIN_PROG_SIZE": "512" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that SFDP standard requires minimum Program-Granularity of 1byte.
The standard and our code fully support 512 Bytes Page Size without the need for this parameter.
The parameter was added for supporting devices that couldn't support the standard, and has an impact on Filesystem heap.
If the flash chips that you're adding support SFDP standard and so does the Cypress target, then this parameter is not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the prog_size setting as both the PSoC 6 and memory chip are compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@morser499 - Thanks for the update! I'm not sure whether the test "features-storage-tests-blockdevice-general_block_device" runs as default - it would be a good test to verify the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most important is running "features-storage-tests-blockdevice-general_block_device" after you've removed the prog_size. And making sure in the test report that it ran on QSPIF blockdevice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests were run after the removal of prog_size. How do I verify it ran on the QSPIF blockdevice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just uploaded the details for the mbed-os-features-storage-tests-blockdevice-general_block_device (below) which seems to indicate that it was using QSPIF
This needs rebase, latest fixes were merged early today for 5.12.2. Please update |
|
937c4bd
to
bf5ec6a
Compare
Fixed |
@offirko could you please re-review the changes ? Thanks |
I believe all the test issues are now addressed |
@offirko could you please review the test fixes ? In the meantime, kicking off the ci. |
CI started |
Test run: FAILEDSummary: 2 of 7 test jobs failed Failed test jobs:
|
I overlooked the Sequana CM0 target. That has now been fixed too. |
CI restarted |
Test run: FAILEDSummary: 1 of 11 test jobs failed Failed test jobs:
|
Exporters restarted (one failure that was success but failed as well) Update: not yet rerun, having issue with CI |
@ARMmbed/mbed-os-test review ^^ exporters rerun |
Restarted the failed exporter (uvision6) / target (K66F) combinations. |
This PR affects other boards. Looking at the commit, as it is squashed it is very hard to find a reason why @ARMmbed/mbed-os-maintainers we need to request changes for commits like this, blame on me here as well. Not reverting the entire PR (one commit), I can revert only changes to the common test code. The main question is: why these changes are needed and how they affect targets? |
Remove some buggy code introduced to hal_qspi_test by PR ARMmbed#10435 Added support for QSPI to Cypress Boards a8570ff
Description
Added support for QSPI to Cypress targets
Pull request type