Skip to content
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

Add support Flash QSPI on S32Z270 #78073

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

congnguyenhuu
Copy link
Contributor

@congnguyenhuu congnguyenhuu commented Sep 6, 2024

Introduce support HyperFlash memory devices on a NXP S32 QSPI bus. This driver uses a fixed LUT configuration that defined in HAL RTD HyperFlash driver and allows to read, write, and erase HyperFlash devices.
Add support QSPI secure flash protection (SFP) for memory control NXP S32 QSPI.
Enable support Flash QSPI on S32Z2XX, the on-board S26HS512T 512M-bit HyperFlash memory is connected to the QSPI controller port A1. This board configuration selects it as the default flash controller.

The test result:

west twister  --disable-warnings-as-errors --device-testing --device-serial=/dev/ttyUSB0 -v --runtime-artifact-cleanup --west-runner trace32 --west-flash="--config=./config.t32" -p s32z2xxdc2/s32z270/rtu1  -T samples/subsys/fs/littlefs -T samples/subsys/settings -T samples/subsys/shell/fs -T tests/drivers/flash -T tests/subsys/fs -T tests/subsys/settings -T tests/subsys/storage/flash_map -T tests/subsys/logging/log_backend_fs
INFO    - Using Ninja..
INFO    - Zephyr version: 8c53d91026a8
INFO    - Using 'zephyr' toolchain.
INFO    - Building initial testsuite list...

Device testing on:

| Platform                | ID   | Serial device   |
|-------------------------|------|-----------------|
| s32z2xxdc2/s32z270/rtu1 |      | /dev/ttyUSB0 |

INFO    - JOBS: 6
INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue
INFO    - 58/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/storage/flash_map/storage.flash_map   PASSED (device 80.480s)
INFO    - 59/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/logging/log_backend_fs/logging.backend.fs.automounted PASSED (device 38.796s)
INFO    - 60/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/settings/functional/file/settings.file PASSED (device 28.017s)
INFO    - 61/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/logging/log_backend_fs/logging.backend.fs.manualmounted PASSED (device 40.086s)
INFO    - 62/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/settings/functional/nvs/settings.functional.nvs.chosen PASSED (device 27.685s)
INFO    - 63/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/settings/functional/fcb/settings.functional.fcb.chosen PASSED (device 27.686s)
INFO    - 64/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/settings/functional/fcb/settings.functional.fcb PASSED (device 27.692s)
INFO    - 65/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/settings/functional/nvs/settings.functional.nvs.dk PASSED (device 27.624s)
INFO    - 66/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/settings/functional/nvs/settings.functional.nvs PASSED (device 27.641s)
INFO    - 67/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/fs/fat_fs_api/filesystem.fat.api.sdmmc SKIPPED (runtime filter)
INFO    - 68/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/fs/fat_fs_api/filesystem.fat.api.mmc  SKIPPED (runtime filter)
INFO    - 69/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/storage/flash_map/storage.flash_map.mbedtls PASSED (device 80.335s)
INFO    - 70/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/storage/flash_map/storage.flash_map.psa PASSED (device 80.570s)
INFO    - 72/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/settings/file/settings.file.raw       PASSED (device 30.420s)
INFO    - 73/77 s32z2xxdc2/s32z270/rtu1   samples/subsys/settings/sample.subsys.settings     PASSED (device 27.357s)
INFO    - 74/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/fs/fs_api/filesystem.api              PASSED (device 26.865s)
INFO    - 75/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/settings/fcb/settings.fcb.raw         PASSED (device 47.201s)
INFO    - 76/77 s32z2xxdc2/s32z270/rtu1   tests/drivers/flash/common/drivers.flash.common.default PASSED (device 26.663s)
INFO    - 77/77 s32z2xxdc2/s32z270/rtu1   tests/subsys/fs/littlefs/filesystem.littlefs.default PASSED (device 39.218s)

INFO    - 77 test scenarios (77 test instances) selected, 59 configurations skipped (57 by static filter, 2 at runtime).
INFO    - 18 of 77 test configurations passed (100.00%), 0 failed, 0 errored, 59 skipped with 0 warnings in 808.67 seconds
INFO    - In total 134 test cases were executed, 355 skipped on 1 out of total 4 platforms (25.00%)
INFO    - 18 test configurations executed on platforms, 0 test configurations were only built.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Sep 6, 2024

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
hal_nxp zephyrproject-rtos/hal_nxp@74a7735 (master) zephyrproject-rtos/hal_nxp#434 zephyrproject-rtos/hal_nxp#434/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Sep 6, 2024
@congnguyenhuu congnguyenhuu changed the title Add support flash qspi on S32Z270 Add support Flash QSPI on S32Z270 Sep 6, 2024
@Laczen Laczen removed their request for review September 6, 2024 11:05
dts/bindings/mtd/nxp,s32-qspi-hyperflash.yaml Outdated Show resolved Hide resolved
drivers/flash/flash_nxp_s32_qspi_hyperflash.c Outdated Show resolved Hide resolved
boards/nxp/s32z2xxdc2/s32z2xxdc2_s32z270_pinctrl.dtsi Outdated Show resolved Hide resolved
dts/bindings/qspi/nxp,s32-qspi-sfp-frad.yaml Outdated Show resolved Hide resolved
dts/bindings/qspi/nxp,s32-qspi-sfp-frad.yaml Show resolved Hide resolved
dts/bindings/qspi/nxp,s32-qspi-sfp-mdad.yaml Show resolved Hide resolved
dts/bindings/mtd/nxp,s32-qspi-device.yaml Outdated Show resolved Hide resolved
boards/nxp/mr_canhubk3/mr_canhubk3.dts Outdated Show resolved Hide resolved
@congnguyenhuu congnguyenhuu force-pushed the nxp-s32ze-support-flash-qspi branch 3 times, most recently from 202af1f to 29a5fdd Compare October 21, 2024 02:11
@congnguyenhuu
Copy link
Contributor Author

congnguyenhuu commented Oct 22, 2024

Hello @nordicjm, after some consideration, I added the write-block-size property to the DTS. I noticed it relates to the existing memory-alignment property. Therefore, I decided to remove memory-alignment and can use write-block-size instead of.

Memory alignment in bytes, used to calculate padding when performing
unaligned accesses.
If not provided, 1 byte alignment will be selected.
The number of bytes used in write operations, it also used to calculate padding when
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still mentioning padding.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated

{
Qspi_Ip_MemoryConfigType *memory_cfg = get_memory_config(dev);

return ((offset >= 0) && (offset < memory_cfg->memSize) &&
Copy link
Collaborator

@de-nordic de-nordic Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if size and offset get large enough you can get into situation where they overflow and you and up having true in line 22, even though parameters are out of range.
More proper would be to do:

return (offset >= 0 && offset < memory_cfg->memSize && ((memory_cfg->mem_size - offset) >= size)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated

}

if (!area_is_subregion(dev, offset, size)) {
return -ENODEV;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am quite sure other drivers return -EINVAL here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no check of offset nor size for alignment to write-block-size set in DTS, I know that device permits 1, but if you set it to something else and let to be ignored, this will allow different subsystems write/read with different alignment and produce data that they may not be able mutually read from the same device.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed comment. could you please revisit? @de-nordic

@congnguyenhuu congnguyenhuu force-pushed the nxp-s32ze-support-flash-qspi branch 3 times, most recently from 3adf998 to c973a5d Compare October 23, 2024 04:26
@nordicjm nordicjm dismissed their stale review October 23, 2024 06:20

Issues addressed

Dat-NguyenDuy
Dat-NguyenDuy previously approved these changes Oct 24, 2024
@congnguyenhuu
Copy link
Contributor Author

Hello @de-nordic, could you revisit this PR? thanks.

Following the commit f98fde0, DT_REG_ADDR now expands with a 'U'
suffix as an unsigned value. However, for compatibility with IS_EQ,
a raw value without any suffix is required. Therefore, this update is
necessary.

Signed-off-by: Cong Nguyen Huu <[email protected]>
Add support QSPI secure flash protection (SFP)

Signed-off-by: Cong Nguyen Huu <[email protected]>
Create common source code to use for supporting HyperFlash.

Rename 'FLASH_NXP_S32_QSPI_NOR_SFDP_RUNTIME' to
'FLASH_NXP_S32_QSPI_SFDP_RUNTIME' as a common kconfig.

Add the 'max-program-buffer-size' property to use for
setting memory pageSize, instead of using
'CONFIG_FLASH_NXP_S32_QSPI_LAYOUT_PAGE_SIZE' for setting.

Add the 'write-block-size' propertyto use for setting
the number of bytes used in write operations, it also
uses to instead of the 'memory-alignment' property.

Signed-off-by: Cong Nguyen Huu <[email protected]>
Add support HyperFlash memory devices on a NXP S32 QSPI bus.
This driver uses a fixed LUT configuration that defined in HAL RTD
HyperFlash driver.
Driver allows to read, write and erase HyperFlash devices.

Signed-off-by: Cong Nguyen Huu <[email protected]>
The on-board S26HS512T 512M-bit HyperFlash memory is connected to
the QSPI controller port A1.
This board configuration selects it as the default flash controller.

Signed-off-by: Cong Nguyen Huu <[email protected]>
Enable flash samples for s32z board

Signed-off-by: Cong Nguyen Huu <[email protected]>
Enable flash tests for s32z board

Signed-off-by: Cong Nguyen Huu <[email protected]>
@congnguyenhuu
Copy link
Contributor Author

I rebased to fix conflict file west manifest

@congnguyenhuu
Copy link
Contributor Author

congnguyenhuu commented Oct 25, 2024

Hello @dleach02, @manuargue and @de-nordic. Is there any chance that this PR can be merged before the freeze? Thanks.

Copy link
Collaborator

@de-nordic de-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments left.

Configuration for storage samples, look ok.

if (status != STATUS_QSPI_IP_SUCCESS) {
LOG_ERR("Failed to read memory status (%d)", status);
ret = -EIO;
} else if (timeout <= 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timeout will never be negative, it is uint32_t

Comment on lines +40 to +43
do {
status = Qspi_Ip_GetMemoryStatus(data->instance);
timeout--;
} while ((status == STATUS_QSPI_IP_BUSY) && (timeout > 0));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You really want to busy loop here? Is there any idling in Qspi_Ip_GetMemoryStatus ?

size_t max_write = (size_t)MIN(QSPI_IP_MAX_WRITE_SIZE, memory_cfg->pageSize);
size_t len;
int ret = 0;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is size is 0 there is a lot happening, without write, that could be just bypassed.

Qspi_Ip_StatusType status;
size_t erase_size;
int ret = 0;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, why not just exit when size is 0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: File System area: Flash area: Logging area: MEMC area: Samples Samples area: Settings Settings subsystem area: Shell Shell subsystem area: Storage Storage subsystem DNM This PR should not be merged (Do Not Merge) manifest manifest-hal_nxp platform: NXP S32 NXP Semiconductors, S32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants