-
Notifications
You must be signed in to change notification settings - Fork 1.5k
drivers/mtd: add support for direct MTD access to raw flash #16624
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
Conversation
|
The example configuration for the test on samv71-xult and s25fl1 is in #16625 |
e5f7744 to
d570be2
Compare
| * | ||
| ****************************************************************************/ | ||
|
|
||
| static off_t mtd_seek(FAR struct file *filep, off_t offset, int whence) |
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.
can we reuse bch code in this driver?
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 mean call the same function? There is a different lock and different calculation for SEEK_END in bch driver, it has to calculate from sectors. Otherwise the code for seek is basically just a copy paste.
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.
Normally, the flash erase size is very larger than the flash program size, so it's possible to reuse BCH but skip FTL which could avoid the erase buffer at FTL layer, but program buffer at BCH layer still exist.
d570be2 to
0f93167
Compare
|
@michallenc please include a Documentation/ about this feature and include this test code as an usage example. |
This commit introduces the new option MTD_PARTITION_REGISTER that allows to register MTD partition with mtd_partition_register call as a standard device driver with support for open/read/write/seek calls. This brings to the possibility to directly access raw flash without the need for FTL layer (that causes unnecessary block erases) and BCH layer (usually used with buffers of erase block size, large memory consumption). The MTD access avoids any buffering (except for write page allocation if byte read/write is not available in the flash driver), but keeps the responsibility of flash erases on the user application. Therefore, write call only writes the given number of bytes, but doesn't check if erase is needed, wear leveling, false blocks and so on. Signed-off-by: Michal Lenc <michallenc@seznam.cz>
0f93167 to
996d04a
Compare
@acassis I have added documentation entry, not quite sure where to put the test code. It is just a quick test sample, nothing interesting as it just calls standard POSIX interface, the same as BCH layer. More interesting is how to register it, which will be in #16625 |
Signed-off-by: Michal Lenc <michallenc@seznam.cz>
996d04a to
e343b8e
Compare
Agree! Maybe we could include it latter as a mtd partition test inside apps/testing/ |
@michallenc This approach has one major limitation: user can't access flash indirectly through BCH/FTL or directly at the same time. Another approach is skipping the buffering at runtime if user pass some special flag(e.g. O_DIRECT) to open, so the ordinary application still work as before, but the special designed application could save the memory.
Actually, we found the similar issue internally, and decide to take the second approach to reduce the memory consumption and improve the compatibility at the same time. |
| buf = kmm_zalloc(nblocks * dev->blksize); | ||
| if (buf == NULL) | ||
| { | ||
| nxmutex_unlock(&dev->lock); |
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.
If there is no MTD_WRITE interface but only the MTD_BWRITE interface, your modification applies for the space of page size to complete the page size aligned write (of course, write page size is very small), I believe this is repetitive with the bch logic. We should be able to reuse the logic of bch directly.
I recently optimized the BCH, BCH more interface is exposed, externally to make a better, I think the new driver layer can be directly call BCH library of the read/write/open/close/seek to achieve, because the code is very similar, we don't need to write it again. I will submit this change tomorrow for your reference |
|
@michallenc @xiaoxiang781216 What might require further consideration is:
|
|
|
||
| startblock = filep->f_pos / dev->blksize; | ||
| nblocks = (len / dev->blksize) + 1; | ||
| buf = kmm_zalloc(nblocks * dev->blksize); |
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.
Just a question: why do we need zero allocated memory?
|
|
||
| dev->size = geo.erasesize * geo.neraseblocks; | ||
| dev->blksize = geo.blocksize; | ||
| dev->readonly = readonly; |
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.
could we let the open flag decide this driver is readonly or writeonly ?
open(/dev/mtd_partition_dev, O_RDOK); or open(/dev/mtd_partition_dev, O_WRONLY);
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 reused this from bchdev_register. It has an advantage that you can limit the access from board level. You basically say "this partition is read only" to be sure an application won't overwrite your data. It has a big hole though, application can still erase the entire partition...
| /* Byte read not supported, use block read */ | ||
|
|
||
| startblock = filep->f_pos / dev->blksize; | ||
| nblocks = (len / dev->blksize) + 1; |
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.
off_t end_pos = (filep->f_pos + len) / dev->blksize;
nblocks = endblock - startblock + 1;
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.
A large amount of memory might be allocated here :
buf = kmm_zalloc(nblocks * dev->blksize);
but in fact, we only need to dynamically apply for one blksize:
- Possible misalignment handling of the starting position
- Addresses that are aligned in the middle do not require memory processing of this blksize size
- Possible misalignment at the end
This is exactly the job of bch
| if (buf) | ||
| { | ||
| offset = filep->f_pos - (startblock * dev->blksize); | ||
| ret = MTD_BREAD(dev->mtd, startblock, nblocks, |
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.
MTD_BREAD(dev->mtd, startblock, nblocks, (FAR uint8_t *)buffer) should changed to :
MTD_BREAD(dev->mtd, startblock, nblocks, (FAR uint8_t *)buf) ?
| #endif | ||
| { | ||
| startblock = filep->f_pos / dev->blksize; | ||
| nblocks = (len / dev->blksize) + 1; |
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.
ditto
| /* Byte read not supported, use block read */ | ||
|
|
||
| startblock = filep->f_pos / dev->blksize; | ||
| nblocks = (len / dev->blksize) + 1; |
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.
A large amount of memory might be allocated here :
buf = kmm_zalloc(nblocks * dev->blksize);
but in fact, we only need to dynamically apply for one blksize:
- Possible misalignment handling of the starting position
- Addresses that are aligned in the middle do not require memory processing of this blksize size
- Possible misalignment at the end
This is exactly the job of bch
|
@xiaoxiang781216 @jingfei195887 Thanks for the feedback. I have converted this to draft for now, will wait for your changes in BCH layer. Regarding some comments.
This seems like a good option if we manage to use BCH layer for this. It would solve the need to initialize separate drivers. But this would require some changes in FTL layer as well, because even with write buffer disabled, it still reads the entire erase page, erase it, and then write. If we want to skip it, FTL would basically just had to call NOR flash specific write/read function and skip all of its own logic. Also, and I haven't seen your changes yet, BCH layer is sector oriented, it doesn't utilize byte access now, so we have to read/write the entire page. If I would just call BCH functions from this new MTD driver, then I could call them only if byte access is not possible. But that would mean a separate driver.
I am not against the option to perform erases from BCH layer, if that's what you mean, but wouldn't do it automatically for two reasons. It takes some time to erase a page and sometimes I need to write the data fast because of power cut off. It is possible to ensure your board has enough reserve to write one write page, but not if it does erase before. And it might not be always needed, I actually have a logging library with sequence logging, but still erase one block in advance, because it is a rotation log, so I use it to detect start/end of the log.
Is it actually possible to use Nand Flash with BCH/FTL now? I thought the implementation is not suitable even now. |
|
@michallenc @xiaoxiang781216 |
Thanks, let's align our effort to come a best solution.
Yes, you are right. BCH/FTL need some modification to achieve this goal.
@jingfei195887 will provide the related change in the next couple day.
we can use open flag or ioctl to control the behavior.
we use BCH/FTL for NAND flash too. Of course, ECC need be handled by lowerhalf MTD driver. |
|
@xiaoxiang781216 @jingfei195887 Looking at the code and your modifications, I basically see two options how to use BCH in direct access to MTD without buffering. Separate driver that uses BCH functions:
But better option seems the changes @xiaoxiang781216 suggested: change the behavior of BCH/FTL layer to support non buffered access without page erase before write if some specific flag is passed during open operation. This should be pretty straightforward, just some if statements. The bigger issue is how to support byte access as FTL (or any other block based driver) write function expects to get blocks to write, not bytes. |
|
@michallenc @xiaoxiang781216 I believe that the byte access flash of FTL you mentioned is what I am currently working to implement. Please revisit issue #16642, as some new patches related to FTL have been introduced. Let us compare the advantages and disadvantages of the two implementations:
|
I think this is going in a right direction. Looking at the recent changes (just a short look, I am on a children summer camp this week and don't have much time), the only reservation I have is this part in if (offset == 0)
{
ret = ftl_mtd_erase(dev, starteraseblock);
if (ret < 0)
{
return ret;
}
}While this is true for some (maybe most) use cases, it might not be for others. And this basically requires the sequential access inside an erase page (you can't write first write page after the others, otherwise the changes are lost). This is a reasonable requirement for application access anyway, but maybe I would keep it configurable, use some sort of ioctl that would toggle it? Just to clarify the major reason of this PR and separate MTD driver, I was trying to limit RAM memory usage. Our design has quite large number of partition on NOR flash (several for logging, some for firmware update, etc) and the buffers alone consumed about 60 kBs, almost 20 % of available RAM space. So the main reason was to avoid buffering and limit unnecessary flash erases if possible. I think your BCH/FTL changes will do that once finished, so I am completely fine with it. And it is even better if it is (at least partially) ready for NAND flashes. |
|
@michallenc #16642 is ready for review, could you look at whether the implementation match your case? |
|
Closed as obsolete with #16642 merged |
Summary
This commit introduces the new option
MTD_PARTITION_REGISTERthat allows to register MTD partition withmtd_partition_registercall as a standard device driver with support foropen/read/write/seekcalls. This brings to the possibility to directly access raw flash without the need for FTL layer (that causes unnecessary block erases) and BCH layer (usually used with buffers of erase block size, large memory consumption).The MTD access avoids any buffering (except for write page allocation if byte read/write is not available in the flash driver), but keeps the responsibility of flash erases on the user application. Therefore, write call only writes the given number of bytes, but doesn't check if erase is needed, wear leveling, false blocks and so on.
Impact
None on current implementation. The newly added code is compiled only if
MTD_PARTITION_REGISTERis configured.The goal is to provide the direct access to raw flash. If the application needs to write directly to flash (usually NOR), it can utilize FTL and BCH layer. But these two have some disadvantages. FTL layer performs erase before every write, thus it has to read the entire erase page, erase it and write it with changes. This means the entire erase page has to be buffered (can be several kilo bytes of RAM memory) and that flash wear level is much higher than it has to be. The FTL layer can be used with BCH layer with write buffering enabled. This limits flash wear as erase/write is performed only when different erase page is accessed (or during flush), but brings another erase page size large buffer, thus yet again increases memory consumption. Last but not least, the write operation performed by FTL takes significant amount of time because of page erase, meaning the data may be lost during power cut off.
FTL and BCH combination is a good option if you have a lot of RAM and want simple application that doesn't care about whether it has to erase the page before writing to it. The newly introduced option brings the possibility of less memory consumption and potentially faster writes, but imposes further requirements on the application using it.
The access is similar to the one provided by Linux with MTD layer.
Testing
Tested with SAMv7 custom board on w25q NOR flash and SAMV71-Xult evaluation kit on s25fl1 NOR flash. The behavior was tested with a custom logging application that reads/writes/erases the partitions and the following code. The board registers the partition to
/dev/mtddevpath and performs write and seek operations. The partition was erased prior to writes withflash_eraseall /dev/mtdThe subsequent hexdump output is as expected: