[RTEMS Project] #4981: Overwork Flashdev

[RTEMS trac]

#4981: Overwork Flashdev
-----------------------------+--------------------
  Reporter:  Bernd Moessner  |      Owner:  (none)
      Type:  defect          |     Status:  new
  Priority:  normal          |  Milestone:
 Component:  admin           |    Version:  6
  Severity:  normal          |   Keywords:
Blocked By:                  |   Blocking:
-----------------------------+--------------------
 Hi, Aaron Nyholm has added a nice flashdev API this year. I wanted to put
 LittleFS on top of this API and encountered some difficulties / found some
 bugs / have some open questions / would like to make some breaking API
 changes. Therefore, I hope that Aaron can comment.


 1. C++ include guards are missing in flashdev.h

 2. The switch case in flashdev.c => rtems_flashdev_ioctl is missing a
 default case

 3. Is there a guideline for naming IOCTLs? There are several defines and
 functions which, I feel, could have a more descriptive name. For example,
 there is the define RTEMS_FLASHDEV_IOCTL_PAGE_COUNT which is used to get
 the number of pages within the flash. Could we have it called
 RTEMS_FLASHDEV_IOCTL_GET_PAGE_COUNT so that it is immediately clear what
 is good for? Basically, I would like to add "_GET" / "_get" at all
 appropriate places (i.e. refactor the corresponding function names, too).
 I guess there will be an issue with the 80 char per line limit, but I'd
 provide a proposal for handling them at a final stage when all other
 issues have been addressed.

 4. I need to be able to create more than one flashdev. Therefore, the
 mutex initializer in rtems_flashdev_do_init is a bit of a roadblocker as
 it would use the same name for the mutex inside each flashdev instance.
 From my point of view, the easiest way to deal with this is to have the
 mutex name as an argument to the initializer. However, I'd prefer to
   a. shorten the name - like FDEV_MTX_${Number}
   b. loop over ${Number} until we can create the mutex
   Is there an rtems_get_id_by_name? I've only found a function to get the
 name of an id.

 5. Some thoughts on the "regions". Regions behave much like paritions,
 except that not every region / partition gets its own file handle, right?
   a. Why not calling the regions partitions?
   b. Do we really need to set them up "dynamically", or could we pass a
 partition table to the flashdev initialisation?
   c. There are the IOCTLs RTEMS_FLASHDEV_IOCTL_REGION_SET and
 RTEMS_FLASHDEV_IOCTL_REGION_UNSET. Looking at the implementation I feel
 they do more than I'd expect. For example, RTEMS_FLASHDEV_IOCTL_REGION_SET
 creates the region, or even redefines a region if one has not called
 unset, and then it activates the region. I'd like to refactor this into
 four IOCTLs:
 - RTEMS_FLASHDEV_IOCTL_ REGION or PARTITION _CREATE
 - RTEMS_FLASHDEV_IOCTL_ REGION or PARTITION _DELETE
 - RTEMS_FLASHDEV_IOCTL_ REGION or PARTITION _ACTIVATE
 - RTEMS_FLASHDEV_IOCTL_ REGION or PARTITION _DEACTIVATE

 For my use case, setting up a static partition table would be good enough.
 We could spare the _CREATE, _DELETE, check the partitions more thoroughly
 during initalisation (for example that they do not overlap), and would not
 have to provide a mechanism to be able to resize them during runtime.



 Flashdev shall work for NAND flashes, too. I am not an expert when it
 comes to NAND flashes. Therefore, I would like to outline my basic
 understanding and hope someone tells me if I am completely off.

 Some parts I find often in designs:

 NOR - MT25Q
 https://www.micron.com/-/media/client/global/documents/products/data-sheet
 /nor-flash/serial-nor/mt25q/die-rev-b/mt25q_qlkt_u_01g_bbb_0.pdf

 NOR - S25FL512
 https://www.infineon.com/dgdl/Infineon-S25FL512S_512_Mb_(64_MB)_3
 .0_V_SPI_Flash_Memory-DataSheet-
 v20_00-EN.pdf?fileId=8ac78c8c7d0d8da4017d0ed046ae4b53

 NAND - W25N01
 https://www.mouser.de/datasheet/2/949/w25n01gv_revl_050918_unsecured-1489588.pdf


 **Memory layout**: bytes form pages, pages form sectors and sectors form
 blocks. There is always some confusion as some vendors omit the term
 sector

 **Read operations**: can start at every address and have arbitrary length

 **Write / Program operations**: work on single pages. Therefore, at max.
 PAGE_SIZE Bytes can be written in one go. There is a wrap around in case
 the configured address was not byte 0 in the current page. Usually, the
 minimum write size is 1 byte. However, setting a minimum write size to a
 different value might allow using additional features (such as ECC for the
 s25fl when using 16 Byte writes to 16 byte aligned addresses).

 **Erase operations**: work on sectors and they always erase the complete
 sector. If an address is passed which points right in the middle of the
 sector, also the sector memory before the address is erased.


 6. Wrt. the write operations. I think
 RTEMS_FLASHDEV_IOCTL_WRITE_BLOCK_SIZE is misleading as the term "block" is
 already defined in the context of flash memories. The implementation /
 comments / unit tests show that we are looking at the minimum write size.
   a. Could we refactor this to RTEMS_FLASHDEV_IOCTL_GET_MIN_WRITE_SIZE?
   b. Especially regarding the ECC features of some flashes. The addresses
 need to be aligned to RTEMS_FLASHDEV_IOCTL_GET_MIN_WRITE_SIZE. I think we
 should test the adress alignment on writes.

 7. Wrt. the erase operations. The erase operations work on sectors!
   a. I need an API extension to get the erase size
   b. We should test that the address passed to the erase function is
 aligned to the erase size.
   c. The erase size puts constraints on the regions / partions. Their
 addresses need to be aligned with the erase size and their length must be
 a multiple of the erase size

 8. Some unit tests (testsuite flashdev01) fail as expected, but not due to
 expected reason. Take for example line 148 in
 testsuites/libtests/flashdev01/init.c and change

   read_data = fgets(buff, 2048, file);

   to

   read_data = fgets(buff, 1, file);

   Actually, the test case is expected to fail as the sector size is only
 512 bytes and we try to read 2048 bytes. Unfortunatelly, reading a single
 byte doesnt work either. This is due to the fact that by default buffered
 IO is used. On my side, each fgets translates to one or more read
 operations of 1024 bytes length. I.e. this testcase will always succeed as
 we expect the test to fail.
   a. I like to propose that we initialise flashdev to operate in buffered
 mode using RTEMS_FLASHDEV_IOCTL_GET_MIN_WRITE_SIZE.
   b. I think we should add a raw_read and raw_write to be able to transfer
 larger junks of data.

 I can work on this and start providing patches for review.

--
Ticket URL: <http://devel.rtems.org/ticket/4981>
RTEMS Project <http://www.rtems.org/>
RTEMS Project