-
Notifications
You must be signed in to change notification settings - Fork 7.3k
drivers: flash: mspi_nor: support MODE_SINGLE and MODE_QUAD_1_4_4 #85520
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
Conversation
fe6106a
to
917071b
Compare
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.
Which 1-4-4 flash device are we initially targeting now?
Does this device not require switching between MSPI_IO_MODE_SINGLE and MSPI_IO_MODE_QUAD_1_4_4 when accessing register and accessing memory?
917071b
to
de3c1d0
Compare
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
de3c1d0
to
06b44a0
Compare
No, it doesn't have to be in this PR... Just mark it as TODO then. It will have to be moved from the generic core one way or another. |
drivers/flash/flash_mspi_nor.c
Outdated
const struct flash_mspi_nor_config *dev_config = dev->config; | ||
|
||
int rc = mspi_dev_config(dev_config->bus, &dev_config->mspi_id, | ||
dev_config->mspi_nor_cfg_mask, cfg); |
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.
mspi_nor_cfg_mask
is MSPI_DEVICE_CONFIG_NONE
when there's no software-mulitperipheral
property set in the controller and then the above call will not perform reconfiguration.
drivers/flash/flash_mspi_nor.c
Outdated
.cmd = SPI_NOR_OCMD_CE, | ||
.cmd_length = 2, | ||
}, | ||
.sfdp = { |
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 command should be also defined for the single and 1-4-4 modes.
drivers/flash/spi_nor.h
Outdated
@@ -14,6 +14,7 @@ | |||
/* Status register bits */ | |||
#define SPI_NOR_WIP_BIT BIT(0) /* Write in progress */ | |||
#define SPI_NOR_WEL_BIT BIT(1) /* Write enable latch */ | |||
#define SPI_NOR_QE_BIT BIT(6) /* Quad enable */ |
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.
Location of this may be different in various devices, enabling of the Quad mode should be handled like in the spi_nor.c
driver.
9e2bdbe
to
bd62322
Compare
Extend driver to support single lane and 1-4-4 IO modes. Move flash chip quirks to a separate file. Signed-off-by: Marcin Szymczyk <[email protected]>
Separate compatible is required for Flash MSPI quirks. Signed-off-by: Marcin Szymczyk <[email protected]>
drivers/flash/flash_mspi_nor.h
Outdated
.cmd = SPI_NOR_OCMD_RD, | ||
.cmd_length = 2, | ||
.addr_length = 4, | ||
.rx_dummy = 20, |
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.
So what should we do about these hardcoded values? These can change based on SCK frequency.
Maybe create vendor.c and add API to query the necessary info like dummy and address length?
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.
The flash_mspi_nor_quirks.h looks more like a mx25r.c rather than a common include...
Suggest to add vendor.c and move mx25r specifics to it.
Move MSPI NOR commands to rodata. Replace array with empty padding (~1kB) with macro-based assignments. Ref: NCSDK-32779 Signed-off-by: Tomasz Chyrowicz <[email protected]>
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.
Looks pretty good!! Thanks @masz-nordic.
DT_INST_STRING_TOKEN(inst, quad_enable_requirements)) | ||
#define FLASH_DW15_QER(inst) COND_CODE_1(DT_INST_NODE_HAS_PROP(inst, quad_enable_requirements), \ | ||
(FLASH_DW15_QER_VAL(inst)), (JESD216_DW15_QER_VAL_NONE)) | ||
|
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.
Could you align the slashes? It looks way better below than what is above....
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 generally like the direction of this change, but wondering if it makes sense to define a table of struct flash_mspi_nor_cmds
by JEDEC ID rather than attempting to be generic. I think that values like the rx_dummy
cycles will end up being device specific.
One example of a way we could do this is how openocd defines SPI flashes, in a table with JEDEC IDs as a key: https://github.com/openocd-org/openocd/blob/master/src/flash/nor/spi.c#L41
Not saying we need to make this change in this PR, but I wonder if this is a reasonable direction to take for this driver longer term
.cmd = SPI_NOR_CMD_READ_FAST, | ||
.cmd_length = 1, | ||
.addr_length = 3, | ||
.rx_dummy = 8, |
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.
Why are we using rx_dummy = 8
here? To keep compatibility with the existing flash devices this driver supports? The same question applies to the sfdp
field
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.
Well most cmds are the same though, the ones that we need to deal with are the ones that are not common(or not in the jedec spec) which should falls into quirks.
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.
For the dummy cycles, I've suggested add additional API to query in vendor.c with ID and frequency. I get the idea basically from what linux spi is doing which is to catagorize the flash devices under each flash manufacturer(vendor).c files and have the jedec spi core to call fixup(quirk) API to conduct vendor specific operations. For dummy cycles, it can be more general but still vendor specific.
Since it can be overwhelming to do it in the same PR, so I didn't request it explicitly here.
And of course, sfdp would be able to solve this, but IMO we shouldn't force everyone to use it.
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.
Sure, I don't think this PR should handle the full generic case- I'm just wondering if that is the best way to handle NOR flash long term. I also am not sure SFDP is the solution- in the flash_mcux_flexspi_nor.c
SFDP implementation I ran into a lot of flash chips that didn't follow SFDP well for quad/octal mode.
IMO long term we should have a table of flash configurations, with the JEDEC ID as the key- the driver can then select the right flash setting based on the flash's JEDEC ID.
.cmd = JESD216_CMD_READ_SFDP, | ||
.cmd_length = 1, | ||
.addr_length = 3, | ||
.rx_dummy = 8, |
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.
Looking at this closer, rx_dummy
can't be 8 here- the MSPI driver does not support a nonzero rx_dummy
value when using SINGLE mode, right?
Line 983 in e1a0350
(req->rx_dummy != 0 || req->tx_dummy != 0)) { |
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.
In jedec spec, that‘s the sfdp default in serial mode, if a controller does not support it, it is their problem.... And it should not affect how we write the generic jedec driver.
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.
Sure, I'm aware that is how the command is defined in the spec. I'll log an issue against the mspi
driver, since this is the driver that will have this issue:
Line 983 in e9fd4b1
(req->rx_dummy != 0 || req->tx_dummy != 0)) { |
dismissing until rx_dummy
question is resolved
Import flash_mspi_nor driver from upstream, based on PR: zephyrproject-rtos/zephyr#85520 This driver has been heavily patched to enable runtime JEDEC ID probing. Once the PR merges, these changes will be sent upstream. Signed-off-by: Daniel DeGrasse <[email protected]>
Import flash_mspi_nor driver from upstream, based on PR: zephyrproject-rtos/zephyr#85520 This driver has been heavily patched to enable runtime JEDEC ID probing. Once the PR merges, these changes will be sent upstream. Signed-off-by: Daniel DeGrasse <[email protected]>
Depends on #80042