RFQ: progmem.h: Modify interface to avoid terms like page, sector, and block, as these terms are used interchangeably by different silicon vendors. #3834
Conversation
antmerlino
changed the title
antmerlino
requested review from
xiaoxiang781216,
acassis,
btashton,
davids5 and
patacongo
…ck, as these terms are used interchangeably by different silicon vendors.
include/nuttx/progmem.h
Outdated
|
|
||
| /**************************************************************************** | ||
| * Name: up_progmem_ispageerased | ||
| * Name: up_progmem_issectionerased |
There was a problem hiding this comment.
up_progmem_iserased
|
@jlaitine Please have a look. |
include/nuttx/progmem.h
Outdated
| * | ||
| * Input Parameters: | ||
| * addr - Address with or without flash offset | ||
| * (absolute or aligned to page0) | ||
| * (absolute or aligned to sector0) |
There was a problem hiding this comment.
I find this confusing.
If it is an address the caller has to know the base address of the memory, so the arch data has to be exposed to the caller. Would could add a function that returns the the base(s) as any counted array.
If it is BASE address less it is an offset.
There was a problem hiding this comment.
@davids5 I believe I have addressed all of your comments except this one. The up_progmem_getaddress() interface could be used to get the base addresses of memory. Is this not sufficient? Or perhaps I don't understand your concern fully.
There was a problem hiding this comment.
IIRC the H7 has 2 banks of FLASH. How would the user get the second one or know it is the second own without including arch specific code. Maybe we should talk this one through.
include/nuttx/progmem.h
Outdated
| * | ||
| * Input Parameters: | ||
| * addr - Address with or without flash offset | ||
| * (absolute or aligned to page0) | ||
| * (absolute or aligned to sector0) |
There was a problem hiding this comment.
| * (absolute or aligned to sector0) | |
| * (absolute or aligned to the zeroth unit of memory) |
include/nuttx/progmem.h
Outdated
| * | ||
| * Input Parameters: | ||
| * addr - Address with or without flash offset | ||
| * (absolute or aligned to page0) | ||
| * (absolute or aligned to sector0) |
There was a problem hiding this comment.
| * (absolute or aligned to sector0) | |
| * (absolute or aligned to the zeroth unit of memory) |
| * The following errors are reported (errno is not set!): | ||
| * | ||
| * -EFAULT: On invalid address | ||
| * | ||
| ****************************************************************************/ | ||
|
|
||
| ssize_t up_progmem_getpage(size_t addr); | ||
| ssize_t up_progmem_getindex(size_t addr); |
There was a problem hiding this comment.
In the original naming, the word "block" referred to smallest erasable unit of flash, and "page" referred to the smallest writable unit. This separation in naming should be kept consistent; On this line the function name uses the word "index" referring to original "block" (smallest erasable unit). This should really return the "write granule" and not the eraseblock!
So the function name here would be wrong.
There was a problem hiding this comment.
No.. this is literally the problem. You interpreted the getpage call as the smallest writable block. But if you look at the majority of the existing drivers, this returned the index of the erasable unit.
There was a problem hiding this comment.
If this is the intention, the function should be "up_progmem_getblock", so it would be wrong originally.
| * | ||
| ****************************************************************************/ | ||
|
|
||
| size_t up_progmem_getaddress(size_t page); |
There was a problem hiding this comment.
Same comment as above; word "index" is confusing, it is not obvious whether this returns the address of the smalles erasable unit, or address of the smalles writable unit
| * | ||
| ****************************************************************************/ | ||
|
|
||
| ssize_t up_progmem_ispageerased(size_t page); | ||
| ssize_t up_progmem_iserased(size_t index); |
There was a problem hiding this comment.
I find this change even more confusing; there should be a clear separation for items which are "smallest erasable unit" and "smallest writable unit".
Previously these were cleary defined to be "block" for smallest erasable unit and "page" for smallest writable unit.
Even though different HW vendors mix up the terms sector, page, block, eraseblock, we'd need to have a consistent naming for the two different things in this interface - this is extremely important when dealing with NAND flash memories (such as the one in stm32h7).
I am not against change of naming, but I suggest for finding descriptive names for these units first, and not using words like "index".
I'd suggest "eraseblock" for "block". Something else for "page"... writegranule? I don't know, sounds funny. Maybe it is enough to just change the other one?
One idea would be, instead of changing the naming, just add big bold comments describing the "NuttX" standard for "block" and "page"
I think that mapping the "nuttx specified" naming should be done in the implementing code. For example, in stm32h7 stm_flash.c ( which @davids5 mentioned ), the mapping seems to be done correctly. STM32H7 name "SECTOR" really means smallest erasable unit (block) and "PAGE" means the write-granule (page). I guess there are drivers which map these differently (wrong) still; so these should also be fixed in order not to get burned.
|
"I think that mapping the "nuttx specified" naming should be done in the implementing code. For example, in stm32h7 stm_flash.c ( which @davids5 mentioned ), the mapping seems to be done correctly. STM32H7 name "SECTOR" really means smallest erasable unit (block) and "PAGE" means the write-granule (page). I guess there are drivers which map these differently (wrong) still; so these should also be fixed in order not to get burned." Well, to be fair. It is really the STM32H7 that did not follow what all the other drivers did. It is the one that really introduced the disparity. In all other drivers, the getaddress and getpage calls returned what erasable unit you were in. On the H7 though, you got back the index for what writable unit you were in. And the claim that one is right and one is wrong is EXACTLY why I don't want to use sector or page terminology. I hear what you are saying that we may need to expand the interface to include functions for both erasable and writable, but I refuse to accept that reintroducing the term page or sector as a good idea, because it's how the drivers got out of line in the first place. |
|
@jlaitine As I mentioned, the H7 was the only platform that interpreted getpage as the index of writable units instead of erasable units. Can you explain why this was necessary? It stands out to me that no one needed this information before and makes me wonder what the use-case is for this info is. |
I don't think the above is true; you need to look into other devices which use the progmem interface on NAND flash (with ECC correction). I am not saying that everything in stm32h7 flash driver is good :) But it is extremely important that we maintain clear distinction between the eraseblocks and write/read unit. Otherwise it is impossible to use the interface with NAND flash devices, where the smallest read/write unit actually matters. On NOR flash devices it pretty much makes no difference. On some NAND flash devices the smallest erasable unit is so small that you can actually mix these up by also reading/writing the same size. On stm32h7 there is no room for compromise, since it has got a 128KB (!) smallest erase size and 32 byte read/write size. However, I have seen it mapped to both PX4 flashparams and also nuttx littlefs (latter turned out to make no sense, but worked in principle). So the it can't be all broken... It is also that this distinction is there on purpose in progmem.h; see the topmost commit touching it: Also check out the commit history of "drivers/mtd/mtd_progmem.c" I don't object naming changes as such, if there is a common view that it is important. I just 1) see the need of distinction between the "eraseblock" and "write/read unit" for NAND flash support and 2) personally liked the nuttx-way of naming these "block" and "page". |
|
|
||
| /**************************************************************************** | ||
| * Name: up_progmem_isuniform | ||
| * | ||
| * Description: | ||
| * Is program memory uniform or erase page and read/write page size differs? | ||
| * Are all erasable units of memory the same size? |
There was a problem hiding this comment.
| * Are all erasable units of memory the same size? | |
| * Check whether all erasable units of memory are equally sized. |
Uniformize alignment of description text.
Also, I think a description text with a question does not seem too fit for documentation.
Here is a suggestion for a direct statement.
| * up_progmem_write is an even multiple of the writegranularity. Padding | ||
| * may be required. |
There was a problem hiding this comment.
| * up_progmem_write is an even multiple of the writegranularity. Padding | |
| * may be required. | |
| * up_progmem_write is an even multiple of the up_progmem_writegranularity. | |
| * Padding may be required. |
Reference to the full name of the function.
| * Return the erase size for a specified index. Will always be a multiple | ||
| * of the write granularity. |
There was a problem hiding this comment.
| * Return the erase size for a specified index. Will always be a multiple | |
| * of the write granularity. | |
| * Return the erase size for a specified index. Will always be a multiple | |
| * of the write granularity indicated by up_progmem_writegranularity. |
It may sound repetitive, but it is helpful for making the link between those functions explicit.
Summary
Modify the progmem interface to avoid terms like page, sector, and block, as these terms are used interchangeably by different silicon vendors.
I have a bootloader that uses the progmem interface to program FLASH. I expected, given the common interface, that it would be possible to use the exact same code across multiple platforms. However, due to the interpretations of terms like page, sector, and block, the underlying drivers are inconsistent in their implementations, and therefore this currently does not work.
Some drivers refer to the smallest erasable unit of memory as a page, while others refer to it as a sector. Prior to this change, the up_progmem_pagesize function was used to get the smallest writable unit of memory - or, at least on some platforms that was the case. Clearly, the term page has different meanings, to different people.
This change attempts to reconcile this issue by modifying the common progmem interface and removing all references to page, sector, and block, since these terms can't be used consistently across vendors. Instead, descriptive terms are attempted to be used.
Additionally, some platforms were exposing additional chip specific functions such as stm32_flash_unlock, or stm32_flash_writeprotect that were not exposed by the common interface. A generic version of these functions has been added to the progmem interface.
I should mention that this is not the first time I've run into issues because of this inconsistency - and I am not alone. @davids5 and the PX4 community has worked around these issues as well.
I am looking for feedback on the naming and once we are happy with the interface change, I will go through each platform and adjust the drivers so that things are consistent.
Impact
Testing