Copilot commented on code in PR #17866:
URL: https://github.com/apache/nuttx/pull/17866#discussion_r2686735377
##########
drivers/mtd/Kconfig:
##########
@@ -1503,4 +1503,11 @@ config MTD_CFI
---help---
Support CFI(common flash interface) NOR FLASH.
+config MTD_CFI_PAGE_SIZE
+ int "Page Size of MTD CFI NOR FLASH (bytes)"
+ default 16
+ depends on MTD_CFI
+ ---help---
+ Configure write page size of CFI Flash.
Review Comment:
The page size should ideally be a multiple of the bank width to ensure
optimal write operations. The code at line 1100 uses ALIGN_DOWN(nbytes,
cfi->bankwidth), which suggests that writes must be aligned to bankwidth.
Consider documenting in the Kconfig help text that the page size should be a
multiple of the bank width, or add validation to ensure this constraint is met.
```suggestion
Configure write page size of CFI Flash.
The configured page size should be a multiple of the device
bank width to match the alignment requirements of the CFI
driver and avoid having writes rounded down to the nearest
bank-width boundary.
```
##########
drivers/mtd/Kconfig:
##########
@@ -1503,4 +1503,11 @@ config MTD_CFI
---help---
Support CFI(common flash interface) NOR FLASH.
+config MTD_CFI_PAGE_SIZE
+ int "Page Size of MTD CFI NOR FLASH (bytes)"
+ default 16
Review Comment:
The configuration option lacks validation to ensure CONFIG_MTD_CFI_PAGE_SIZE
is positive. A page size of zero would cause division by zero errors in
cfi_mtd_bread (line 113, 116) and cfi_mtd_bwrite (line 132, 135) in mtd_cfi.c,
as well as a modulo by zero error in cfi_write (line 1096) in cfi.c. Consider
adding a "range 1 65536" constraint to prevent invalid values.
```suggestion
default 16
range 1 65536
```
##########
drivers/mtd/Kconfig:
##########
@@ -1503,4 +1503,11 @@ config MTD_CFI
---help---
Support CFI(common flash interface) NOR FLASH.
+config MTD_CFI_PAGE_SIZE
+ int "Page Size of MTD CFI NOR FLASH (bytes)"
+ default 16
+ depends on MTD_CFI
+ ---help---
+ Configure write page size of CFI Flash.
+
Review Comment:
The help text for this configuration option should be more detailed. It
should explain: (1) the valid range of values (must be at least 1 and should
not exceed the hardware-supported maximum write buffer size), (2) the
relationship to the CFI flash's max_write_bytes_num parameter, and (3) the
performance implications of choosing different page sizes. Users need this
context to make informed configuration decisions.
```suggestion
Configure the write page size (in bytes) used by the CFI NOR
Flash
driver.
The value must be at least 1 and should not exceed the maximum
write buffer size supported by the CFI device. In the driver,
this capability is typically exposed as the device's
"max_write_bytes_num" parameter; this configuration value should
be chosen such that it is less than or equal to
max_write_bytes_num for the target flash.
Larger page sizes (up to the hardware limit) usually improve
write throughput by allowing more data to be written per program
operation, reducing command overhead. Smaller page sizes may be
more conservative and can reduce the latency of individual
writes, but they increase the number of write operations needed
for the same amount of data and can therefore reduce overall
performance.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
