Skip to content

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.

Sign up for GitHub

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

Open
TaliPerry wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Flash: Handle 4B only flashes and flash reset. #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/flash/spi_flash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ static int spi_flash_configure_device (const struct spi_flash *flash, bool wake_

if (reset_device) {
status = spi_flash_reset_device (flash);
if (status != 0) {
if ((status != 0) && (status != SPI_FLASH_RESET_NOT_SUPPORTED)) {
Copy link
Contributor

@chweimer chweimer Jan 13, 2023

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.

Copy link
Author

@TaliPerry TaliPerry Jan 15, 2023

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:

  1. spi_flash_init : read_id + SFDP.
  2. spi_flash_config: fast_read\quad\drive strength etc.
  3. 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!

goto exit;
}
}
Expand Down
8 changes: 7 additions & 1 deletion core/flash/spi_flash_sfdp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,13 @@ int spi_flash_sfdp_get_4byte_mode_switch (const struct spi_flash_sfdp_basic_tabl

params = (struct spi_flash_sfdp_basic_parameter_table_1_5*) table->data;
if (table->sfdp->sfdp_header.parameter0.minor_revision >= 5) {
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) ==
Copy link
Contributor

@chweimer chweimer Jan 13, 2023

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.

Copy link
Author

@TaliPerry TaliPerry Jan 15, 2023

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!

SPI_FLASH_SFDP_4BYTE_ONLY) {
*addr_4byte = SPI_FLASH_SFDP_4BYTE_MODE_FIXED;
}
else if ((params->enter_4b & 0x7f) == 0) {
*addr_4byte = SPI_FLASH_SFDP_4BYTE_MODE_UNSUPPORTED;
}
else if ((params->enter_4b & SPI_FLASH_SFDP_4B_ENTER_B7) &&
Expand Down