SFDP: Add support for multiple configurations and sector maps #14989
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@LDong-Arm, thank you for your changes. |
LDong-Arm
force-pushed
the
sfdp_sector_maps_multi
branch
from
bb6866a
to
92deb48
Compare
Patater
reviewed
Aug 10, 2021
storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp
Outdated
Show resolved
Hide resolved
Patater
reviewed
Aug 10, 2021
storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp
Outdated
Show resolved
Hide resolved
|
@LDong-Arm, thank you for your changes. |
LDong-Arm
force-pushed
the
sfdp_sector_maps_multi
branch
from
92deb48
to
9ce29eb
Compare
|
Fixed astyle and |
|
@ARMmbed/team-cypress @miteshdedhia7 @amartsan |
rwalton-arm
reviewed
Aug 10, 2021
storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp
Outdated
Show resolved
Hide resolved
storage/blockdevice/COMPONENT_QSPIF/source/QSPIFBlockDevice.cpp
Outdated
Show resolved
Hide resolved
LDong-Arm
force-pushed
the
sfdp_sector_maps_multi
branch
2 times, most recently
from
56ca3c9
to
c60d286
Compare
LDong-Arm
force-pushed
the
sfdp_sector_maps_multi
branch
from
c60d286
to
68829bc
Compare
|
The unit test for the CR3NV[1] quirk previously segfaulted in GitHub Actions, but the latest run passes and it passes every time on my PC. There must be a real problem, e.g. something like referencing a variable that's gone out of scope but hasn't been overwritten in RAM yet. Looking into it. |
|
@LDong-Arm I have executed positive tests on this as follows: So that is good. However, I want to execute a negative test where I ask the block device example to write to a different sector than the one at offset 0x0. This should determine if the fix will support multiple sectors. Ian |
|
@ifyall Thanks for trying it out. Good to hear the block device example is working at offset 0! I'll come up with a modified version of the example sometime later, or maybe a system test (greentea test) if it's feasible, to test different offsets/regions of the board. |
When passing an allocation size directly to `std::make_unique`, `std::nothrow` is unavailable, so any failed allocation results in an exception which we cannot catch because Mbed OS is compiled with `-fno-exceptions`. To fix this, chain `std::make_unique` with `new (std::nothrow)`, and the allocated `unique_ptr` retains the `nullptr` property.
The SFDP functions parse SFDP data which is fetched by a callback
called `sfdp_reader` provided by {SPIF,QSPIF,OSPIF}BlockDevice.
Currently, this callback interface only takes a read address and an RX
buffer to store output data. This has been enough, because other SPI
parameters are always the same when fetching the SFDP table only -
they are just hardcoded in each reader.
But in the future we will add support for flash devices with multiple
configurations (in a subsequent commit), and to detect which
configuration is enabled, we will need to send detection commands
which require device-dependent SPI parameters:
* address size
* instruction
* dummy cycles
This commit
* turns the above SPI parameters from predefined/hardcoded values
into parameters of the callback
* lets the SFDP functions pass the above parameters to the callback
(Note: To read the SFDP table itself, those values are constants
defined by the standard, not tied to any particular device, so they
can be known to the SFDP functions)
* updates the callbacks implemented by {SPIF,QSPIF,OSPIF}BlockDevice
* updates the mock callback for unit tests and expectations
Some test data in test_sfdp.cpp is used by one test case only, so we turn it into a local variable.
The second byte of the sector map descriptor is the configuration ID. On a device with non-configurable layout, the only available map descriptor's configuration ID must be 0x00 as required by the JESD216D standard. This value is important, because we will check each descriptor's configuration ID when we support multiple configurations. Note: The test data is fake - when we modified real data of a configurable device to become non-configurable for test purpose, we forgot to change this field.
This allows the entire QSPI class to be mocked/faked for unit testing purpose, without dependencies from the real implementation such as `qspi_free()` from the HAL.
In Mbed OS, each library has an `include/<library>/` subdirectory containing headers. The recommended way to include a header is `#include "<library>/<header>.h"` to avoid potential conflicts with any external modules that have same names of headers. This is not enforced yet, and both include/ and include/<component>/ are in a library's include paths, to avoid breaking preexisting Mbed projects that don't follow the recommendation. But code within Mbed OS should follow it at least.
Cypress S25FS512S's SFDP table suggests that bit-1 of the register CR3NV on 25FS512S should equal 1. But it is a known issue that the value is actually 0 on hardware. So if we query the value of CR3NV during configuration detection, we can't find a configuration that matches the SFDP data. This issue has been discussed in the Linux MTD (Memory Technology Devices) mailing list: https://linux-mtd.infradead.narkive.com/ylHK6CyT/spi-nor-fs512s-incorrect-cr3nv-1-value This commit adds a quirk to report bit-1 of CR3NV as 1. Note: In Mbed OS, vendor-specific quirks can only be handled in block devices. The SFDP functions assume data from hardware to be correct. So this quirk is in the block device.
The entire flash chip S25FS512S consists of uniform 256KB sectors. Additionally, it has three configurations: * 0x01: Eight 4KB sectors (32KB in total) overlaying the start of the first 256KB sector * 0x03: Eight 4KB sectors (32KB in total) overlaying the end of the last 256KB sector * 0x05: No overlaying sectors The active configuration is determined from bit fields of two registers, CR1NV and CR3NV. Mbed OS does not currently support partially overlaying sectors, meaning that with eight 4KB sectors overlay a 256KB sectors, the remaining 224KB (== 256KB - 8 * 4KB) of the big sector can't be correctly handled. Supporting such scenario would involve a large amount of rewriting in Mbed OS's BlockDevice, SFDP and their tests, and may increase the code size. So, this commit applies a quirk to always report configuration 0x05 (no overlaying sectors). Even if 0x01 or 0x03 is the real configuration, they are compatible with the 0x05 configuration because 256KB sectors exist in all cases. Note: This quirk does *not* change actual configurations on hardware, because registers CR1NV and CR3NV are one-time configurable (OTP) - each bit field has a factory value of 0 and can be changed to 1 by the user but not back to 0. So QSPIFBlockDevice avoids changing them.
Add a test case to verify that the quirk on `CR1NV` and `CR3NV` registers get applied by `QSPIFBlockDevice` when the flash device is S25FS512S. `QSPIFBlockDevice` depends on the SFDP functions and the `QSPI` class, but we can't use gMock on them because: * SFDP functions are global functions, whereas gMock only supports mocking class member functions. * A mocked class is injected into the test subject, but passing a preexisting `QSPI` instance into `QSPIFBlockDevice` is not possible (unless we resort to pointers). For details, see comments in test_QSPIFBlockDevice.cpp. So fakes of the `QSPI` class and any SFDP functions involved are defined in test_QSPIFBlockDevice.cpp.
A Sector Map Parameter Table contains a sequence of the following descriptors: * (Optional) configuration detection command descriptors, one for each command to run to determine the current configuration. This exists only if the flash layout is configurable. * Sector map descriptors, one for each possible configuration. On a flash device with a non-configurable layout, there is only one such descriptor. Previously we only supported the non-configurable case with a single descriptor. This commit adds support for multiple configurations.
QSPIF was disabled on CYW9P62S1_43012EVB_01 because its S25FS512S flash chip has multiple configurations but our SFDP parser did not support this scenario. Now having added support for this, we can enable QSPIF (the component label for QSPIFBlockDevice) and QSPI (the label for Quad-SPI communication).
LDong-Arm
force-pushed
the
sfdp_sector_maps_multi
branch
from
d9c4d5e
to
9ade440
Compare
mergify
bot
dismissed
Patater’s stale review
Pull request has been modified.
|
@Patater @rwalton-arm Ready for review again, thanks! |
|
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mbedmain
added
release-version: 6.15.0
Release-pending
and removed
release-type: feature
Release-pending
labels
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
Fixes #12574
This PR adds to following changes, to support CYW9P62S1_43012EVB_01's QSPI flash (S25FS512S):
A Sector Map Parameter Table contains a sequence of the following descriptors:
Previously we only supported the non-configurable case with a single descriptor. This PR adds support for multiple configurations and provides a positive unit test case.
To be able to run configuration detection commands, a callback provided by each block device and used by the SFDP class needs to take additional parameters: address size, instruction and dummy cycles. Their values come from sector map descriptors in the SFDP table. This PR changes the callback interface, and updates callbacks implemented by SPI, QSPI and OSPI block devices to the extended interface.
A quirk is provided for Cypress S25FS512S flash device's inconsistent register value: This device's SFDP table suggests checking CR3NV[1], which needs to be 1 in order for the config detection algorithm to work. But the actual hardware has value 0 instead. This has been discussed before on the Internet (e.g. on a Linux mailing list) - it's not possible to change a value in hardware that has been in production for years.
Two of the three configurations of S25FS512S have eight 4KB sectors (32KB in total) overlaying a 256KB sector. This type of partial overlay is not current supported by Mbed OS's BlockDevice and SFDP parser. A quirk is provided to ignore the 4KB sectors and only use the 256KB sectors.
Fixes of a few preexisting issues we found.
More details in commit messages.
Impact of changes
Migration actions required
Documentation
To check.
Pull request type
Test results
Reviewers