QSPI SFDP Flash Block Device #8352
Conversation
|
@bulislaw , @donatieng , @ARMmbed/mbed-os-maintainers , @dannybenor , @davidsaada . |
|
I didn't made any code review, but here is new test result:
Here is console log for teh failed test : |
|
@maciejbocianski - Maciej, I'd appreciate your help. |
|
@offirko |
|
DISCO-L475VG-IOT01A removed for now from tests (known limitation) |
|
DISCO-L475VG-IOT01A returned to tests |
|
/morph build |
Build : SUCCESSBuild number : 3351 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 2984 |
Test : SUCCESSBuild number : 3156 |
There was a problem hiding this comment.
Looks good to me from a high-level
Has anything changed between this and https://github.com/ARMmbed/qspif-blockdevice?
| ### Debugging | ||
| Set `MBED_CONF_MBED_TRACE_ENABLE` as true(1) to enable logs. | ||
|
|
||
| ### Example |
There was a problem hiding this comment.
@offirko, Would you be able to add info on QSPI block device to the block device example once this is merged into mbed-os?
https://github.com/ARMmbed/mbed-os-example-blockdevice/blob/master/README.md#changing-the-block-device
This will probably also need a reference page in the docs, but you're probably already aware of that. CC @AnotherButler FYI.
There was a problem hiding this comment.
@geky - sure thing, that is the plan
| // _devices_mutex is used to lock csel list - only one QSPIFBlockDevice instance per csel is allowed | ||
| SingletonPtr<PlatformMutex> QSPIFBlockDevice::_devices_mutex; | ||
| int QSPIFBlockDevice::_number_of_active_qspif_flash_csel = 0; | ||
| PinName *QSPIFBlockDevice::_active_qspif_flash_csel_arr = generate_initialized_active_qspif_csel_arr(); |
There was a problem hiding this comment.
This should be a SingletonPtr (or function returning a static value). These can be garbage collected by the compiler if the QSPIFBlockDevice is unused. As is, all applications will pay a RAM cost for this array, even if they don't use QSPIFBlockDevice.
This is low priority, since we're currently half-protected by the component mechanism, but this is a defect we should fix at some point.
There was a problem hiding this comment.
@geky - interesting, thanks! (with your permission I'll open an issue and update later on?)
| case 1: | ||
| case 2: | ||
| tr_debug("_setQPIEnabled - send command 38h"); | ||
| _qspi_send_general_command(0x38, QSPI_NO_ADDRESS_COMMAND, NULL, 0, NULL, 0); |
There was a problem hiding this comment.
Will like to see some error handling for calls in _sfdp_set_qpi_enabled like others in this file. Not blocking request though
There was a problem hiding this comment.
@deepikabhavnani - added error logging, note that even if QPI (4-4-4) fails we do not fail init and use 1-4-4 or 1-1-4 mode
| tr_debug("_setQPIEnabled - set config bit 6 and send command 71h"); | ||
| _qspi_send_general_command(0x65, 0x800003, NULL, 0, (char *)config_reg, 1); | ||
| config_reg[0] |= 0x40; //Set Bit 6 | ||
| _qspi_send_general_command(0x71, 0x800003, NULL, 0, (char *)config_reg, 1); |
There was a problem hiding this comment.
Additionally, not sure why we have so many magic numbers here?
There was a problem hiding this comment.
SFDP standard finds it hard to cope with the variety of chip instructions .
I only used magic numbers where the standard used magic numbers without any "rational" reasoning.
|
/morph build |
Build : SUCCESSBuild number : 3417 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 3041 |
Test : SUCCESSBuild number : 3211 |
NirSonnenschein
added
ready for merge
needs: CI
and removed
needs: CI
ready for merge
labels
|
@ARMmbed/mbed-os-maintainers this PR passed CI and technical review but doesn't have an official Gatekeeper review. Anyone have time to take a look? |
|
Will look at and probably merge today. |
| /********************************/ | ||
| /* Different Device Csel Mgmt */ | ||
| /********************************/ | ||
| static PinName *generate_initialized_active_qspif_csel_arr() |
There was a problem hiding this comment.
Is the idea with this function to deallocate a pin so that it can be used by this class?
There was a problem hiding this comment.
The idea is to allow only a single QSPIFBlockDevice instance per flash chip (you can still have several instances for a target with several QSPI Flash chips). This function allocates a static array of all the QSPI Flash chips (csel-pins) that are currently use a QSPIFBlockDevice instance.
| int status = 0; | ||
| _devices_mutex->lock(); | ||
| if (_number_of_active_qspif_flash_csel >= QSPIF_MAX_ACTIVE_FLASH_DEVICES ) { | ||
| status = -2; |
There was a problem hiding this comment.
Does this exit status correspond with some sort of enum?
There was a problem hiding this comment.
It does not. It's not exposed and only used internally.
There was a problem hiding this comment.
@offirko Thanks for the info. Y'all might want to consider them at some point in the future., but if that's how y'all are doing things now, then that's fine.
| #define TEST_ERROR_MASK 16 | ||
| #define QSPIF_TEST_NUM_OF_THREADS 5 | ||
|
|
||
| const struct { |
There was a problem hiding this comment.
This is a pretty neat structure.
There was a problem hiding this comment.
I agree (-: , I inherited it from the original SPIFBlockDevice test code.
I believe the credit goes to @geky
There was a problem hiding this comment.
@cmonr - please see my answers above - thanks
|
Thanks for the replies @offirko! |
This function writes a "config" register to ensure that the flash part is in high performance mode, not low-power mode. This is required at by at least MX25R6435F in order to operate at frequencies > 33MHz (for reference, DISCO_L475VG_IOT01A runs the QSPI interface at 80 MHz). The config register that this writes does not appear to be covered by the SFDP spec (JESD216D.01) so this remains the status quo of unconditional execution, as has been done on master since ARMmbed#8352.
This function writes a "config" register to ensure that the flash part is in high performance mode, not low-power mode. This is required at by at least MX25R6435F in order to operate at frequencies > 33MHz (for reference, DISCO_L475VG_IOT01A runs the QSPI interface at 80 MHz). The config register that this writes does not appear to be covered by the SFDP spec (JESD216D.01) so this remains the status quo of unconditional execution, as has been done on master since ARMmbed#8352.
This function writes a "config" register to ensure that the flash part is in high performance mode, not low-power mode. This is required at by at least MX25R6435F in order to operate at frequencies > 33MHz (for reference, DISCO_L475VG_IOT01A runs the QSPI interface at 80 MHz). The config register that this writes does not appear to be covered by the SFDP spec (JESD216D.01) so this remains the status quo of unconditional execution, as has been done on master since ARMmbed#8352.
This function writes a "config" register to ensure that the flash part is in high performance mode, not low-power mode. This is required at by at least MX25R6435F in order to operate at frequencies > 33MHz (for reference, DISCO_L475VG_IOT01A runs the QSPI interface at 80 MHz). The config register that this writes does not appear to be covered by the SFDP spec (JESD216D.01) so this remains the status quo of unconditional execution, as has been done on master since ARMmbed#8352.
This function writes a "config" register to ensure that the flash part is in high performance mode, not low-power mode. This is required at by at least MX25R6435F in order to operate at frequencies > 33MHz (for reference, DISCO_L475VG_IOT01A runs the QSPI interface at 80 MHz). The config register that this writes does not appear to be covered by the SFDP spec (JESD216D.01) so this remains the status quo of unconditional execution, as has been done on master since #8352.
Description
Quad SPI Block Device for Flash supporting SFDP standard
Pull request type