nRF52840_DK: "qspi_api.c" check read/write WORD alignment, set clock frequency divider #8832
Conversation
|
Results from mbed greentea "tests-mbed_hal-qspi": mbedgt: greentea test automation tool ver. 1.4.0 |
|
|
||
| // Convert hz to closest NRF frequency divider | ||
| if (hz > 16000000) | ||
| freq = NRF_QSPI_FREQ_32MDIV1; // 32 MHz |
There was a problem hiding this comment.
It would be safer to round down instead of up.
|
|
||
| config.pins.sck_pin = (uint32_t)sclk; | ||
| config.pins.sck_pin = (uint32_t)sclk; |
There was a problem hiding this comment.
Please do not introduce new spaces (if possible set your editor to do this for you on lines you edit)
|
|
||
| // use sync version, no handler | ||
| (void)(obj); | ||
| if (hz > MBED_HAL_QSPI_MAX_FREQ) |
There was a problem hiding this comment.
Use alignment that is already in the code
j3hill
force-pushed
the
nRF52840_QSPI
branch
from
5f8e7d5 to
abc14df
Compare
| @@ -247,6 +251,11 @@ qspi_status_t qspi_frequency(qspi_t *obj, int hz) | |||
|
|
|||
| qspi_status_t qspi_write(qspi_t *obj, const qspi_command_t *command, const void *data, size_t *length) | |||
| { | |||
| // length needs to be rounded up to the next WORD (4 bytes) | |||
| if ((*length & WORD_MASK) > 0) { | |||
There was a problem hiding this comment.
Why does the transfer size need to be 4 byte aligned? I don't see this as a requirement in the documentation qspi_api.h.
There was a problem hiding this comment.
Turns out this is a limitation of the NRF52. Thanks for updating the documentation @j3hill
There was a problem hiding this comment.
Yes. This is a Nordic requirement. In the nRF52840 product specification document (https://infocenter.nordicsemi.com/pdf/nRF52840_OPS_v0.5.pdf ) it states that for qspi read and write operations: "The length must be a multiple of 4 bytes." To help make this clear going forward, I've updated the length comments in "qspi_api.h" to note that support for non 4-byte (WORD) aligned reads/writes is implementation defined.
There was a problem hiding this comment.
@j3hill - I do not understand the comment: "support for non 4-byte (WORD) aligned reads/writes is implementation defined". 4-bytes alignment is Nordic limitation. Other targets might have other limitations.
Currently qspi_hal has no limitation on read/write granularity - so minimal size is 1byte.
The way I understand it the remark should be something like: "support for any specific target alignment requirement is - target implementation defined"
There was a problem hiding this comment.
I dont not understand neither rereading this thread. Similar limitations we have in flashing. How do we cover it there? via get_program_page_size() ?
There was a problem hiding this comment.
@donatieng , @0xc0170 - if you're referring to FalshIAP, it is covered by a hal API flash_get_page_size() which is implemented in each target, and surface its requirement to the driver FlashIAP. The FlashIAP driver then handles the program address alignment and granularity requirements with its own internal buffer.
Driver:
_uint32_t FlashIAP::get_page_size() const
{
return flash_get_page_size(&flash);
}
HAL:
_/** Get page size
*
There was a problem hiding this comment.
@offirko you are referring to the internal flash. The external QSPI flash blockdevice is defined here: https://github.com/ARMmbed/mbed-os/blob/master/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp#L468-L472
There was a problem hiding this comment.
@marcuschangarm - I know.
I was simply showing @0xc0170 and @donatieng that in FlashIAP the driver handles alignment and write-granularity using its own internal buffer, and based on page_size upstreamed from hal .
My original suggestion was to do the same for QSPI
j3hill
force-pushed
the
nRF52840_QSPI
branch
2 times, most recently
from
d378141 to
c7df6c7
Compare
There was a problem hiding this comment.
@maciejbocianski Can you review additions to the qspi_api.h ?
|
Generally looks OK, but found this in QSPIBlockDevice mbed-os/components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.cpp Lines 462 to 471 in 78401ed @offirko can you check whether this 4B read/program size alignment should be taken into account in QSPIFBlockDevice layer |
There was a problem hiding this comment.
@maciejbocianski - FileSystem is relying on 'QSPIFBlockDevice::get_program_size()' to align or buffer its data if required - so indeed it should be taken into account in QSPIFBlockDevice.
The problem is that QSPIFBlockDevice is an SFDP standard BlockDevice and SFDP standard defines write granularity 1byte. In order for specific targets to expose their special non-standard requirement, the information should surface from HAL to QSPI Driver and to QSPIF BD. We need to add such an API both to qspi_hal and to qspi driver.
| @@ -247,6 +251,11 @@ qspi_status_t qspi_frequency(qspi_t *obj, int hz) | |||
|
|
|||
| qspi_status_t qspi_write(qspi_t *obj, const qspi_command_t *command, const void *data, size_t *length) | |||
| { | |||
| // length needs to be rounded up to the next WORD (4 bytes) | |||
| if ((*length & WORD_MASK) > 0) { | |||
There was a problem hiding this comment.
@j3hill - I do not understand the comment: "support for non 4-byte (WORD) aligned reads/writes is implementation defined". 4-bytes alignment is Nordic limitation. Other targets might have other limitations.
Currently qspi_hal has no limitation on read/write granularity - so minimal size is 1byte.
The way I understand it the remark should be something like: "support for any specific target alignment requirement is - target implementation defined"
|
Maybe my understanding is extremely naïve, but instead of defining a new HAL API or limitations to address the quirks of one platform can we just pad any unaligned accesses on this platform with |
|
But this approach will require data replication to internal aligned buffer what is not good idea. |
|
@donatieng - This solution can be a valid one, considering the extra replication that @maciejbocianski mentioned. If you're referring to a solution in higher level (driver/blockdevice) then we still need the hal API to know if the quirk of the platform is 4-bytes alignment or more.. |
j3hill
force-pushed
the
nRF52840_QSPI
branch
from
c7df6c7 to
fd86f36
Compare
0xc0170
dismissed
their stale review
Not convinced about the note in the docs (non 4 byte alignment )
|
We'll always find platforms with some specific quirks. Question is: how many other platforms have this specific requirement? I've just checked the datasheet + driver and memory for reads and writes must be aligned in the QSPI Flash and RAM. So in addition to padding we also need to potentially move the memory in RAM to make it aligned. I'm uncomfortable with "upstreaming" that as a requirement for the HAL. |
|
I want to jump in here and try to get this back on track. This PR is just about getting the QSPI working on the Nordic platform. This Nordic platform does not support transfers that are not 32-bit aligned and sized. So in this case, the new functionality returns the appropriate error. I don't want to dismiss the effort to understand what the implications around the block driver and what this does to that subsystem, but that is not the intent of the PR. I think the architectural discussions around the QSPI block device work SHOULD happen, but I don't think we should hold these fixed up. |
j3hill
force-pushed
the
nRF52840_QSPI
branch
from
fd86f36 to
eacaca5
Compare
|
I have updated the "hal/qspi_api.h" comments as requested. This PR is now ready for approval. |
|
Hey @j3hill I don't see the workarounds for alignment in targets\TARGET_NORDIC\TARGET_NRF5x\qspi_hal.c`? |
j3hill
dismissed
donatieng’s stale review
We are not implementing alignment fixes in this PR, we are just returning an error if reads/writes are less than 4 bytes.
clock frequency divider These changes are to enable QSPI functioanlity for the NRF52840DK.
j3hill
force-pushed
the
nRF52840_QSPI
branch
from
eacaca5 to
2c4a3d5
Compare
|
@donatieng The changes to the comments in "hal\qspi_api.h" have been removed, as requested. |
|
@donatieng Happy with the current state here? |
|
CI started |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
Description
In "targets\TARGET_NORDIC\TARGET_NRF5x\qspi_hal.c", add logic to make sure that Read/Write commands are WORD (4 bytes) aligned. If they are not aligned, return QSPI_STATUS_INVALID_PARAMETER. Remove incorrect MBED_HAL_QSPI_HZ_TO_CONFIG macro, and replace with a new private helper function that sets the closest available nRF clock frequency divider for the requested QSPI clock frequency (up to 32 MHz).
Pull request type