driver: flash: npcx: introduce npcx flash driver #60024
Conversation
MulinChao
requested review from
nashif,
carlescufi,
galak,
MaureenHelm,
ChiHuaL,
ssekar15,
tbursztyka,
gmarull and
de-nordic
as code owners
zephyrbot
added
platform: Nuvoton NPCX
MulinChao
changed the title
MulinChao
force-pushed
the
npcx_qspi_fiu
branch
4 times, most recently
from
eb524bd
to
d944b96
Compare
drivers/flash/flash_npcx_fiu_nor.c
Outdated
| /* Don't write more than a page. */ | ||
| if (sz_write >= SPI_NOR_PAGE_SIZE) { | ||
| sz_write = SPI_NOR_PAGE_SIZE; | ||
| } | ||
|
|
||
| /* Don't write across a page boundary */ | ||
| if (((addr + sz_write - 1U) / SPI_NOR_PAGE_SIZE) | ||
| != (addr / SPI_NOR_PAGE_SIZE)) { | ||
| sz_write = SPI_NOR_PAGE_SIZE - (addr % SPI_NOR_PAGE_SIZE); | ||
| } |
There was a problem hiding this comment.
All this code is only meaningful for the first loop iteration, which basically means that it can be executed prior to entering the loop.
There was a problem hiding this comment.
This is based on the requirement of Page Program command. First, the write size cannot exceed the page size. Second, if the start address is not aligned with the page size, the write range cannot be across a page boundary. In the other flash drivers, such as spi_nor.c, flas_stm32_qspi.c, also have the same checks.
MulinChao
dismissed stale reviews from keith-zephyr and fabiobaltieri
via
fb9986c
|
Hi @de-nordic, any further comment? |
drivers/flash/flash_npcx_fiu_nor.c
Outdated
| while (size > 0) { | ||
| size_t sz_write = size; | ||
|
|
||
| /* Don't write more than a page. */ | ||
| if (sz_write >= SPI_NOR_PAGE_SIZE) { | ||
| sz_write = SPI_NOR_PAGE_SIZE; | ||
| } | ||
|
|
||
| /* Don't write across a page boundary */ | ||
| if (((addr + sz_write - 1U) / SPI_NOR_PAGE_SIZE) | ||
| != (addr / SPI_NOR_PAGE_SIZE)) { | ||
| sz_write = SPI_NOR_PAGE_SIZE - (addr % SPI_NOR_PAGE_SIZE); | ||
| } |
There was a problem hiding this comment.
Because your MCU does not have source alignment you can just make this:
size_t sz_write;
/* This is max that the first, or any, write can take, which is SP_NOR_PAGE_SIZE */
if (size >= SPI_NOR_PAGE_SIZE) {
sz_write = SPI_NOR_PAGE_SIZE;
}
/* Correct the first write to not go through page boundary */
size_t overflow = (addr + sz_write) & (SPI_NOR_PAGE_SIZE - 1);
sz_write -= overflow;
/* Now sz_write is reminder needed for addr to align to page boundary */
/* This is still valid test, as nothing has been written yet */
while (size > 0) {
/* Write something */
flash_npcx_uma_cmd_only(dev, SPI_NOR_CMD_WREN);
ret = flash_npcx_uma_write_by_addr(dev, SPI_NOR_CMD_PP, tx_buf, sz_write, addr);
if (ret != 0) {
break;
}
ret = flash_npcx_nor_wait_until_ready(dev);
if (ret != 0) {
break;
}
size -= sz_write;
addr += sz_write;
tx_buf += sz_write;
/* On the first run the sz_write was set to max bytes that would not cross the page boundary,
but not more than page size and no more than size; for each next run the addr is now aligned to page boundary,
so try to either write full page or what has left of size */
if (size > SPI_NOR_PAGE_SIZE) {
sz_write = SPI_NOR_PAGE_SIZE;
} else {
sz_write = size;
}
}
Correct me if I have missed something, and the change is up to you, but I think that this significantly simplifies the loop and reduces branching within it.
There was a problem hiding this comment.
Thanks for the detailed description. I got your point and will update a new version for it. Regarding overflow variable, we might need to check whether the addr plus sz_write cross the page boundary. Or the size of the first write might be zero.
Add a new pinctrl type to control peripheral modules' specific IO characteristics such as tri-state, the power supply type selection (3.3V or 1.8V), and so on. In NPCX series, the corresponding registers/fields are irregular. This CL wraps these definitions to dt nodes and put them in pinctrl property if needed. Signed-off-by: Mulin Chao <mlchao@nuvoton.com>
|
Seems all |
|
That
That's fixed, sorry about it, retried the check, it should pass now. |
This CL attempts to implement npcx's flash driver instead of the original one (npcx spi driver plus spi_nor flash driver). Signed-off-by: Mulin Chao <mlchao@nuvoton.com>
|
@MulinChao Can you make another PR with release notes update documenting the addition of the driver? Thanks. |
Sure. Please refer to PR 60660 |
This PR attempts to implement npcx's flash driver instead of the original one (npcx spi driver plus spi_nor flash driver). There are two reasons to do this refactor. First, we'd like to support two or more spi nor flashes via a single Flash Interface Unit (FIU) module. Although the Zephyr spi driver uses different GPIOs to achieve this feature generically, npcx ec switches different interfaces and chip-select pins via several modules as ECTS in FIU and DEVCNT/DEV_CTLx in SCFG (System Configuration) instead.
Case 1: GPIOs for different CS pins
Second, npcx FIU supports Direct Read Access (DRA) mode for faster flash reading and User Mode Access (UMA) mode for customized spi transactions. In the previous implementation, we only adopt the UMA mode. In this PR, the flash driver uses DRA mode directly to implement flash_read() API for better performance, especially when Quad/4-byte address mode enables. After applying this PR, the nor flash dt nodes looks like
Case 2: NPCX qspi-flash dt nodes
The users can access the different nor flashes on qspi bus module by Zephyr flash APIs. We needn't care about pin-muxing, chip selection, and additional QE/4-byte settings between accessing the different flash interfaces. It looks like this:
In order to support
cros_ecflash driver, this PR also supports extended flash operations APIs (CONFIG_FLASH_EX_OP_ENABLED) which includes UMA operation for status register read/write and specific operations for write protection. With CL4663912, it passed the following tests.software syncflag enabled.(We upload the image with this PR before testing it every time by the script.)