Add support Flash QSPI on S32Z270 #78073
Conversation
zephyrbot
added
area: MEMC
area: Settings
zephyrbot
requested review from
bperseghetti,
carlescufi,
Dat-NguyenDuy,
dcpleung,
de-nordic,
jakub-uC,
kartben,
Laczen,
nashif,
nordic-krch and
PetervdPerk-NXP
|
The following west manifest projects have been modified in this Pull Request:
Note: This message is automatically posted and updated by the Manifest GitHub Action. |
zephyrbot
added
manifest
manifest-hal_nxp
DNM
congnguyenhuu
changed the title
congnguyenhuu
force-pushed
the
nxp-s32ze-support-flash-qspi
branch
from
eb86fa0
to
a517164
Compare
gmarull
dismissed
their stale review
looks better than before, so will let others finish the review.
congnguyenhuu
dismissed stale reviews from manuargue and Dat-NguyenDuy
via
59c912d
congnguyenhuu
force-pushed
the
nxp-s32ze-support-flash-qspi
branch
from
87aa3a7
to
59c912d
Compare
|
I have rebased to fix conflict the west manifest. Could you please revisit PR? @nordicjm @manuargue @Dat-NguyenDuy |
|
Hello @de-nordic, could you visit this PR? thanks. |
congnguyenhuu
force-pushed
the
nxp-s32ze-support-flash-qspi
branch
from
59c912d
to
9776ca8
Compare
| start-address: | ||
| type: int | ||
| required: true | ||
| description: | | ||
| Specifies the specific flash memory region start address. | ||
| end-address: | ||
| type: int | ||
| required: true | ||
| description: | | ||
| Specifies the specific flash memory region end address. |
There was a problem hiding this comment.
why is this not using reg?
There was a problem hiding this comment.
There is a problem prevent me, the QSPI node has multiple child nodes with different compatible bindings (device flash nodes). In some cases, they all use reg but it is possible dupliacte, which triggers a duplicate reg warning. To avoid this, I decided to use the reg property only for the device flash node.
Do you have any suggestions on how to handle this?@nordicjm
There was a problem hiding this comment.
I don't know what you mean? You can have a reg inside of a reg, that's how flash partitions work
There was a problem hiding this comment.
use-case:
&qspi0 {
sfp_frad_0: sfp_frad@0 {
compatible = "nxp,s32-qspi-sfp-frad";
reg = <0x0 DT_SIZE_M(512)>;
};
s26hs512t: s26hs512t@0 {
compatible = "nxp,s32-qspi-hyperflash";
reg = <0>;
status = "okay";
partitions {
compatible = "fixed-partitions";
#address-cells = <1>;
#size-cells = <1>;
storage_partition: partition@0 {
label = "storage";
reg = <0x0 0x10000>;
};
};
};
the log warning:
There was a problem hiding this comment.
I found solution for this, I updated to move sfp_frad_0 into a parent node, look like as:
&qspi0 {
sfp {
compatible = "secure-flash-protection";
#address-cells = <1>;
#size-cells = <1>;
frad_0: frad@0 {
compatible = "nxp,s32-qspi-sfp-frad";
reg = <0x0 DT_SIZE_M(512)>;
};
};
s26hs512t: s26hs512t@0 {
compatible = "nxp,s32-qspi-hyperflash";
reg = <0>;
status = "okay";
partitions {
compatible = "fixed-partitions";
#address-cells = <1>;
#size-cells = <1>;
storage_partition: partition@0 {
label = "storage";
reg = <0x0 0x10000>;
};
};
};
| program-page-size: | ||
| type: int | ||
| required: true | ||
| description: | | ||
| The programming page size of the flash memory device, | ||
| as specified in the flash memory device datasheet. |
There was a problem hiding this comment.
why can this not use write-block-size which already exists?
There was a problem hiding this comment.
this property differs with write-block-size, this defines maximum of programming page buffer size configuration for hardware device flash. To avoid confusing I renamed this property to max-program-buffer-size.
As specified configuration in the flash memory device datasheet.
There was a problem hiding this comment.
Where is the write block size set then, that is required for flash, and erase block size?
There was a problem hiding this comment.
write block size is defined by https://github.com/zephyrproject-rtos/zephyr/pull/78073/files#diff-aa5c7e912c23e4bbc1cabe0abb767637bcdaf3f9892233d71bd0841fdbec5c93R13
ease block size is defined by https://github.com/zephyrproject-rtos/zephyr/pull/78073/files#diff-626f2f951a3779218f24c5da9f599d4a6bde9f3fca7e6656cee9531ac1fc1277L46
There was a problem hiding this comment.
write block size being a define in code is useless and wrong, there is a dts property, likewise erase block size should be in dts, it's a hardware property (the page size is different, that is what size you want to erase, it is not the erase size)
There was a problem hiding this comment.
I see that write-block-size and erase-block-size, defined in https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/mtd/soc-nv-flash.yaml which uses for internal flash.
I refer the other support external flash, I see most them not use these property.
eg. a supported driver that not use these. the write block size is fixed.
https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/flash/nrf_qspi_nor.c#L1172
There was a problem hiding this comment.
Hello @nordicjm, I’ve addressed all your comments. Could you please revisit the PR? I hope you happy with this and can approve my changes so we can merge it soon. Thanks.
| @@ -344,6 +344,7 @@ | |||
| readoc = "1-4-4"; | |||
| writeoc = "1-4-4"; | |||
| has-32k-erase; | |||
| program-page-size = <256>; | |||
There was a problem hiding this comment.
and to go with the above comment, this would not be 256, unless you mean you can't write anything less than 256 bytes, e.g. 4 byte write would fail, which I doubt is the case
There was a problem hiding this comment.
pls see my above comments #78073 (comment)
congnguyenhuu
force-pushed
the
nxp-s32ze-support-flash-qspi
branch
3 times, most recently
from
202af1f
to
29a5fdd
Compare
drivers/flash/flash_nxp_s32_qspi.h
Outdated
| #include "jesd216.h" | ||
|
|
||
| #define QSPI_ERASE_VALUE 0xff | ||
| #define QSPI_WRITE_BLOCK_SIZE 1U |
There was a problem hiding this comment.
This needs to come from dts (the write block size). Does your driver switch the commands it sends depending upon what some uses? E.g. if I use flash_write() with 129 bytes it will send 32 commands to write 4 bytes followed by 1 command to write 1 byte or will it just fail at the last part? Likewise what if there is a flash memory used with this that does not support 1 byte writes and is configures for 4 byte writes only? As an aside, without the coming from dts with the correct name, things like MCUboot will assume default values that, like in the example above, could be wrong
There was a problem hiding this comment.
This driver will use memory-alignment dt properties (https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/mtd/nxp%2Cs32-qspi-device.yaml#L17) to pad (by adding 0xFF) at the unaligned data parts. E.g with memory-alignment=2 use flash_write() with 129 bytes it will send 130 bytes (last byte is padded if offset is even number). From application pov, flash_write() can be used with any size between 1 and the value specified in max-program-buffer-size dt properties.
This platform does not support MCUboot now so I'm not sure this.
There was a problem hiding this comment.
But that is wrong? That's fine if you are using a flash chip, but you get non-flash SPI/QSPI devices (e.g. FRAM) and writing 0xff will write 0xff over data you do not want to touch
There was a problem hiding this comment.
This driver supports hyperflash chip (using s26hs512 flash chip on board), writing 0xff not affect the existed data.
There was a problem hiding this comment.
And will completely fail with e.g. https://www.infineon.com/cms/en/product/memories/f-ram-ferroelectric-ram/excelon-f-ram/cy15v104qsn-108sxit/
There was a problem hiding this comment.
People that will write code for subsystem to work on any device have to align offset and size to write-block-size anyway, so by not requiring that. So if somebody delivers something for your device with assumption that data will be padded, will have to fix that when testing code against other device.
There was a problem hiding this comment.
@de-nordic @nordicjm i believe your concern for this topic has been addressed (by updating the PR to use write-block-size), could both of you help me to take another look.
There was a problem hiding this comment.
@de-nordic @nordicjm i believe your concern for this topic has been addressed (by updating the PR to use
write-block-size), could both of you help me to take another look.
You have added a write block size of 1, so does that mean then when I call this function, your driver writes exactly one byte with no padding or 0xff's what so ever?
There was a problem hiding this comment.
you are saying this write block size. this is write block size for a serial NOR flash device. it allows to write exactly 1 byte without padding.
There was a problem hiding this comment.
I don't mean for a serial NOR flash device, I mean the actual command that comes out the chip, does it send a command to the other device consisting of writing 1 byte and 1 byte only without any padding or 0xff's or does it send a command with additional data/padding?
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. Update to use 'program-page-size' instead of using 'CONFIG_FLASH_NXP_S32_QSPI_LAYOUT_PAGE_SIZE' for setting memory pageSize because this is the page size to write flash, is not the erase sector size. 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
force-pushed
the
nxp-s32ze-support-flash-qspi
branch
from
29a5fdd
to
72a718a
Compare
|
Hello @nordicjm, after some consideration, I added the |
| 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 |
There was a problem hiding this comment.
Still mentioning padding.
| { | ||
| Qspi_Ip_MemoryConfigType *memory_cfg = get_memory_config(dev); | ||
|
|
||
| return ((offset >= 0) && (offset < memory_cfg->memSize) && |
There was a problem hiding this comment.
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)
| } | ||
|
|
||
| if (!area_is_subregion(dev, offset, size)) { | ||
| return -ENODEV; |
There was a problem hiding this comment.
I am quite sure other drivers return -EINVAL here.
There was a problem hiding this comment.
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.
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: