Update QSPI porting guide #676
Conversation
| @@ -53,4 +75,8 @@ To enable the QSPI HAL, define `QSPI` in the targets.json file inside `device_ha | |||
|
|
|||
| ### Testing | |||
|
|
|||
| To be implemented | |||
| The Mbed OS HAL provides a set of conformance tests for the QSPI interface (**Note: QSPI HAL tests require onboard QSPI flash memory**). You can use these tests to validate the correctness of your implementation. To run the QSPI HAL tests, use the following command: | |||
There was a problem hiding this comment.
Note: QSPI HAL tests require onboard QSPI flash memory
I would say it's still a valid use case to support QSPI even if the board doesn't have on board qspi flash. Could we only run the test requiring onboard flash if the QSPI_FLASH1_xxx are defined? @donatieng @jamesbeyond
There was a problem hiding this comment.
@bulislaw Actually QSPI test is run only on targets defining DEVICE_QSPI.
Currently DEVICE_QSPI means device with onboard flash, so we don't recognize QSPI capable devices without onboard QSPI flash. I think that it would be good to have something like: DEVICE_QSPI and DEVICE_QSPI_FLASH to distinguish them.
There was a problem hiding this comment.
Why do we only consider devices with onboard flash? Not every SPI device has SPI memory attached to it...
There was a problem hiding this comment.
Not every.
e.g. Nucleo-L496ZG has no QSPI flash but it has QSPI signals routed out on external connector according circuit diagram (waiting for STM confirmation on it in ARMmbed/mbed-os#7817)
There was a problem hiding this comment.
I would say it's still a valid use case to support QSPI even if the board doesn't have on board qspi flash. Could we only run the test requiring onboard flash if the QSPI_FLASH1_xxx are defined? @donatieng @jamesbeyond
👍 Wording would be "requires QSPI FLASH pins to be defined" ? Targets with onboard flash would have it by default, others would need a config to add them.
| @@ -29,6 +29,28 @@ To make sure your platform is ready for the upcoming changes, you need to implem | |||
|
|
|||
There was a problem hiding this comment.
Query: Can we delete or change the warning in line 7?
There was a problem hiding this comment.
The code was merged it looks like, should be good to remove that warning,
There was a problem hiding this comment.
@AnotherButler warning was removed
maciejbocianski
force-pushed
the
update_qspi_docs
branch
from
b57f104 to
27140aa
Compare
Make minor formatting changes.
|
Have all comments been addressed? |
Fix phrasing as requested in comments for accuracy.
|
@donatieng @0xc0170 Is this OK to merge? |
|
LGTM 😄! |
Align docs with changes introduced in:
ARMmbed/mbed-os#7817
ARMmbed/mbed-os#7783