-
Notifications
You must be signed in to change notification settings - Fork 8.2k
Support QSPI Flash driver for Renesas RA6 devices #78959
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
Support QSPI Flash driver for Renesas RA6 devices #78959
Conversation
|
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. |
d793e21 to
96a9f25
Compare
96a9f25 to
92e6654
Compare
92e6654 to
8187848
Compare
de-nordic
left a comment
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.
Issue with EX_OP and mostly nits regarding other areas.
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.
Spi->SPI
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.
We fixed it. We changed to use macros in https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/flash/spi_nor.h
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.
Qpi->QSPI
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 isn't typo. We mentioned QPI mode (4-4-4) here.
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.
Most of those defines are actually provided here https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/flash/spi_nor.h, I know the naming is SPI_NOR, but adding additional defines only brings new names for functionally the same things.
You could just use the SPI_NOR defines and define these that are not provided by the header.
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.
We changed to use macros in https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/flash/spi_nor.h
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.
There is already system wide id for reset
FLASH_EX_OP_RESET = 0
zephyr/include/zephyr/drivers/flash.h
Line 640 in 03959a2
| FLASH_EX_OP_RESET = 0, |
and if the meaning is the same, the system wide one should be used.
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.
We fixed it as your advice.
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 define is in the same range as system wide definition for non-vendor space, and as such is not allowed.
Please refer to
zephyr/include/zephyr/drivers/flash.h
Lines 617 to 631 in 03959a2
| /* | |
| * Extended operation interface provides flexible way for supporting flash | |
| * controller features. Code space is divided equally into Zephyr codes | |
| * (MSb == 0) and vendor codes (MSb == 1). This way we can easily add extended | |
| * operations to the drivers without cluttering the API or problems with API | |
| * incompatibility. Extended operation can be promoted from vendor codes to | |
| * Zephyr codes if the feature is available in most flash controllers and | |
| * can be represented in the same way. | |
| * | |
| * It's not forbidden to have operation in Zephyr codes and vendor codes for | |
| * the same functionality. In this case, vendor operation could provide more | |
| * specific access when abstraction in Zephyr counterpart is insufficient. | |
| */ | |
| #define FLASH_EX_OP_VENDOR_BASE 0x8000 | |
| #define FLASH_EX_OP_IS_VENDOR(c) ((c) & FLASH_EX_OP_VENDOR_BASE) |
for instruction on how to define vendor specific operation
and to https://github.com/zephyrproject-rtos/zephyr/blob/03959a20f7834ffe50a51df2b2e45ef87d1b610b/include/zephyr/drivers/flash/stm32_flash_api_extensions.h for an example of such definition.
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 think that there is also typo and QPI should be QSPI?
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.
We updated for "QSPI_FLASH_EX_OP_EXIT_QPI" as your advice.
And "QPI" isn't typo. We mentioned QPI mode (4-4-4).
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.
You probably want to have some logging in at least one of the ifs, otherwise you have the same error returned to user with no way to distinguish problems except for debugging.
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.
We fixed it. We added log to distinguish for errors.
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.
You lack here userspace processing, is that desired?
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.
Yes, it is designed without userspace processing.
It is supported to reset QPI mode and memory.
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.
Here you fail operation of -EINVAL, even if len == 0 would cause nothing happening at all,
on the other side in qspi_flash_ra_write you first check len == 0 and bypass any more checks as there is no more work to.
Consider having one flow everywhere, for example checking the len == 0 and just exit as there is no work to do and actually no possibility to failure
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.
We fixed it. We unified one flow for both qspi_flash_ra_write() and qspi_flash_ra_read(): check "len == 0" first and just exit.
|
There is a lot of QPI in the code, shouldn't that be QSPI? |
8187848 to
f462ce1
Compare
@de-nordic : Thank you so much for your review. We fixed issues which you pointed out. For "QPI" in the code, it isn't typo of "QSPI", this is support for QPI mode (4-4-4). Could you please help us to recheck for the update? Thanks again. |
| /* Reset Flash device (at QPI(4-4-4) mode) */ | ||
| QSPI_FLASH_EX_OP_EXIT_QPI, |
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 needs proper documentation, see FLASH_RA_EX_OP_WRITE_PROTECT doc in main -- even though I guess there's no in / out to document, there should still be proper doxygen documentation
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 have updated it to follow proper Doxygen documentation standards, thank you
| #define PAGE_SIZE_BYTE SPI_NOR_PAGE_SIZE | ||
| /* sector size of QSPI flash device */ | ||
| #define BLOCK_SIZE_4K (4096U) | ||
| #define BLOCK_SIZE_32K (32678U) |
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.
Indeed. Please fix
| uint8_t buffer[size]; | ||
|
|
||
| acquire_device(dev); | ||
| memset(&buffer[0], 0, sizeof(buffer)); | ||
| buffer[0] = QSPI_QPI_CMD_RDSFDP; | ||
| buffer[1] = addr; | ||
| buffer[2] = addr >> 8; | ||
| buffer[3] = addr >> 16; | ||
|
|
||
| err = R_QSPI_DirectWrite(&qspi_data->qspi_ctrl, &buffer[0], FOUR_BYTE, true); | ||
| if (err != FSP_SUCCESS) { | ||
| LOG_ERR("Direct write for READ SFDP failed"); | ||
| err = -EIO; | ||
| goto out; | ||
| } | ||
|
|
||
| err = R_QSPI_DirectRead(&qspi_data->qspi_ctrl, &buffer[0], size); |
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.
| #define QSPI_FLASH_ADDRESS(page_no) \ | ||
| ((uint8_t *)(QSPI_DEVICE_START_ADDRESS + ((page_no) * SPI_NOR_PAGE_SIZE))) |
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.
unused?
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.
Yes, I have removed it. Many thanks
|
@khoa-nguyen-18 @thaoluonguw ping in case you missed my comments |
a406f3e
012bb7f to
a406f3e
Compare
Add QSPI Flash driver supports for Renesas RA6. Signed-off-by: Tri Nguyen <[email protected]> Signed-off-by: Thao Luong <[email protected]> Signed-off-by: Khoa Nguyen <[email protected]>
Add qspi node on Renesas RA6 devicetree to support QSPI flash driver Signed-off-by: Quy Tran <[email protected]>
Add support for QSPI flash driver on EK-RA6E2, EK-RA6M3, EK-RA6M4 and EK-RA6M5 Signed-off-by: Quy Tran <[email protected]> Signed-off-by: Khoa Nguyen <[email protected]>
Add support to test flash/common for qspi_nor on Renesas ek_ra6m5, ek_ra6m4, ek_ra6m3, ek_ra6e2 Signed-off-by: Khoa Nguyen <[email protected]>
Add support for qspi Renesas RA to run the sample spi_flash Signed-off-by: Khoa Nguyen <[email protected]>
Add support for qspi_nor for the sample jesd216 to run sample on Renesas RA6 boards Signed-off-by: Khoa Nguyen <[email protected]>
a406f3e to
04e7722
Compare
|



Add support for QSPI Flash driver for RA6 devices