-
Notifications
You must be signed in to change notification settings - Fork 26
Flash: Handle 4B only flashes and flash reset. #11
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
base: master
Are you sure you want to change the base?
Conversation
Flash 4B address mode only. Add support for flashes that are in 4B address mode only
there is no command to switch 4B mode on and off.
Flash reset: Some flashes don't support reset command.
If the app calls spi_flash_initialize_device it
does not know it advance and it masy ask to reset the device. If reset is not supported by the flash
spi_flash_initialize_device should still continue in this case and not fail the flash init.
|
@TaliPerry please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
|
@TaliPerry the command you issued was incorrect. Please try again. Examples are: and |
1 similar comment
|
@TaliPerry the command you issued was incorrect. Please try again. Examples are: and |
|
@microsoft-github-policy-service agree company="Nuvoton" |
| if ((params->enter_4b & 0x7f) == 0) { | ||
|
|
||
| // 4 bytes only flash, no switch command available: | ||
| if ((params->table_1_0.dspi_qspi & SPI_FLASH_SFDP_ADDRESS_BYTES) == |
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.
Is this situation not already covered by the check for enter_4b flags to be 0? Do you have an example of a flash device that advertises support for enter_4b through SFDP but does in reality does not? If so, adding that example flash device to the suite of SFDP tests would be useful.
It also seems like this scenario would get handled naturally in the spi_flash layer when it checks for address mode compatibility before actually executing the mode switch.
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.
This is not a real flash, it's an OT device, a standalone chip that is on the data path to the flash itself.
I don't have DS, I only have a sample of the SFDP table that this device provides.
Cerberus sees the OT as a regular flash. This device supports only 4 byte address mode. sending any command to change the address mode to 3 or 4 bytes is ignored. The SFDP protocol allows having such a device, though there is no real commercial flash in fixed mode for 4 bytes, AFAIK.
Since those commands are ignored, the device SFDP table doesn't declare them to be part of the command set. It just declare that it's a fixed 4 byte device, so no need for ENTER4B\EXIT4B commands.
spi_flash_sfdp.h already has such an option:
/**
- Supported methods for entering and exiting 4-byte addressing mode.
*/
enum spi_flash_sfdp_4byte_addressing {
SPI_FLASH_SFDP_4BYTE_MODE_UNSUPPORTED, /< 4-byte addressing is not supported. */
SPI_FLASH_SFDP_4BYTE_MODE_COMMAND, /< Use a command to switch the mode. */
SPI_FLASH_SFDP_4BYTE_MODE_COMMAND_WRITE_ENABLE, /< Issue write enable before mode switch. */
SPI_FLASH_SFDP_4BYTE_MODE_FIXED, /< Device is permanently in 4-byte mode. */
};
but the last value is not used anywhere.
if spi_flash_sfdp_get_4byte_mode_switch encounter such a device that doesn't support EXIT\ENTER4B but is fixed 4bytes mode, then it shouldn't return SPI_FLASH_SFDP_4BYTE_MODE_UNSUPPORTED, because it does support 4B.
I will add such a unit test and resubmit the patch.
Thanks, Chris, for the review!
| if (reset_device) { | ||
| status = spi_flash_reset_device (flash); | ||
| if (status != 0) { | ||
| if ((status != 0) && (status != SPI_FLASH_RESET_NOT_SUPPORTED)) { |
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.
This is an interesting scenario. On the one hand, if you are saying you want the flash device reset, that request has failed if the device doesn't support the operation. On the other hand, I could see the use-case where you want the reset to be a "best effort", meaning reset if you can. I wonder if we need separate initialize_device APIs to cover each case.
Also, there need to be unit tests submitted with code 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.
I think the best option is to split this long function to sub functions:
- spi_flash_init : read_id + SFDP.
- spi_flash_config: fast_read\quad\drive strength etc.
- spi_flash_reset: only reset.
otherwise for a flash that doesn't support reset the function will not do all the steps that happen after the reset.
API is a drastic change, so I tried to keep it minimal.
I will try to add a new unit test for this and resubmit this patch.
thanks, Chris!
|
I just pushed some updates that included a couple commits that should address these scenarios, with associated unit tests. Take a look at the current code state and see if this PR still applies. |
Flash 4B address mode only. Add support for flashes that are in 4B address mode only
there is no command to switch 4B mode on and off.
Flash reset: Some flashes don't support reset command.
If the app calls spi_flash_initialize_device it
does not know it in advance and it may ask to reset the device. If reset is not supported by the flash
spi_flash_initialize_device should still continue in this case and not fail the flash init.