[RFC,v3 5/5] mtd: spinand: skip set/get oob data bytes when interleaved case

Fri Nov 12 00:33:34 PST 2021

Hi Miquel,

Firstly, I would like to introduce Mediatek HW ECC engine how it
organize the whole nand page data.

Take page size 2KB and OOB size 64B for example,

nand page standard data format:
+---------------------------+------------+
|                           |            |
|        main area          |  oob area  |
|                           |            |
+---------------------------+------------+

Mediatek HW ECC pipelined data format(sector size 1KB):
+------------+-------+-----------+-------+
|            |       |           |       |
|  sector(0) | oob(0)|  sector(1)| oob(1)|
|            |       |           |       |
+------------+-------+-----------+-------+

Mediatek HW ECC engine organize data as sector in unit.
The sector size can be 512 or 1024 bytes.
The OOB free and ecc bytes are stored right after the sector(n)
main data.

oob(n) layout:
+-------+---------+-----------+
|       |         |           |
| part1 |  part2  |   part3   |
|       |         |           |
+-------+---------+-----------+
part1: OOB bytes will be protected by ecc engine which called FDM ECC
part2: OOB bytes not be protected by ecc engine
part3: OOB bytes to store ecc parity for (sector data + FDM ECC bytes)

part1 and part2 called FDM(flash disk manage data) which can be used to
store BBM or filesystem manage data(like jffs2).

The FDM and FDM ECC can be configurable,
FDM number of bytes: 0 ~ 8 bytes
FDM ECC number of byte: 0 ~ FDM size 

Therefore, the mtk ecc driver need to handle BBM swap and OOB shift
operation before/after the write/read operation in
ecc_finish/prepare_io_req.

1. Why need BBM swap operation in mtk ecc driver?

Ensure the BBM poisition consistent with nand specification.

           main area           oob area
+---------------------------+------------+
|                           |
            |
|                           |*           |
|               
            |            |
+---------------------------+------------+

   sector(0)   oob(0)   sector(1)   oob(1)
+------------+-------+-----------+-------+
|            |       |           |       |
|            |       |       #   |       |
|            |       |           |       |
+------------+-------+-----------+-------+

(*): stand for the BBM
(#): stand for one byte main data

For the 2KB + 64B page case, the standard BBM position will be main
data in Mediatek HW ECC data format. Therefore, the BBM swap between
the BBM with one bye main data in sector(1).

The data struct when BBM swap:

   sector(0)   oob(0)   sector(1)   oob(1)
+------------+-------+-----------+-------+
|            |       |       
    |       |
|            |       |       *   |#      |
|            |  
     |           |       |
+------------+-------+-----------+-------+

2. Why need OOB shift opration in mtk ecc driver?

Since the BBM located at oob(1) 1st byte in mtk ecc data format, the
standard BBM position need at the 1st in the whole OOB area logically.

Just take the data flow in prepare_io_req when write
with MTD_OPS_AUTO_OOB for example:

source:      data buf           oob buf
+---------------------------+------------+
|                           |
            |
|                           |            |
|               
            |            |
+---------------------------+------------+

1st: mtd_ooblayout_set_databytes
        data buf              oob buf
+---------------------------+------------+
|                           |
            |
|                           |*@@@@@@     |
|               
            |            |
+---------------------------+------------+
(*): BBM, is reserve byte when set databyte
(@): the free OOB data byte

2nd: BBM swap

           data buf            oob buf
    sector(0)    sector(1)
+---------------------------+------------+
|             |             |
            |
|             |       *     |#@@@@@@     |
|             | 
            |            |
+---------------------------+------------+
(#): stand for one byte main data
(*): stand for the BBM

3rd: sector OOB shift

           data buf            oob buf
    sector(0)    sector(1)   oob(1) oob(0)
+---------------------------+------------+
|             |             |
      |     |
|             |       *     |@@@   |#@@  |
|             | 
            |      |     |
+---------------------------+------------+

The mtk ECC engine will auto place the data struct on flash
as bellow finally.

   sector(0)   oob(1)   sector(1)   oob(0)
+------------+-------+-----------+-------+
|            |       |       
    |       |
|            |@@@    |       *   |#@@    |
|            |  
     |           |       |
+------------+-------+-----------+-------+

The read data flow in finish_io_req is reverse with prepare_io_req when
write.

On Tue, 2021-11-09 at 13:05 +0100, Miquel Raynal wrote:
> Hi Xiangsheng,
> 
> Has we discussed a while ago:
> 
> 	...it is possible that I did not test with MTD_OPS_AUTO_OOB
> 	recently and indeed the ECC bytes will missing in this case. In
> 	the write path, maybe the ->prepare_io hooks should spread the
> 	user data following req->mode in req.oobbuf before computing
> 	the codes. Similar logic should be applied to the read path.
> 
> Can you please add a patch for this situation in your next iteration?
> It does not look like this situation has been handled.
> 

As talked above, I may put the set/get OOB data bytes to each ecc
driver not at the spinand driver.

This may have some advantage:
1) Some ECC engine may protect the OOB bytes not only the main data.
They will have same issue when read/write with MTD_OPS_AUTO_OOB.
For this case, the OOB data bytes must set/get in
prepare/finish_io_req.

2) For the syndrome layout, the data format on the flash may not be
consistent with nand specification. It`s better handled by the special
ecc engine.

Can you agree with this modification?
And, I will work on this and send in next iteration.

> xiangsheng.hou at mediatek.com wrote on Fri, 22 Oct 2021 10:40:21 +0800:
> 
> > For syndrome layout, ECC/free byte in oob layout and main
> > data are interleaved. For this case, it is better to set/get
> > oob data bytes in ECC engine.
> 
> I don't understand the last sentence

Just as the discussion talked above.

> > 
> > For MTK ECC engine, although it can auto place data as sector +
> > oob free + oob ecc for one page in pipelined. However, the bad
> > mark will be not fit with nand spec. Therefore, there have bad
> > mark swap operation in ecc engine.
> > 
> > But, the swap opeation(between bbm 0xff with 1byte main data) will
> > lead to more one byte than oobavailable.
> 
> Sorry but again, I don't understand what you mean.
> 

           data buf            oob buf
+---------------------------+------------+
|             |             |
            |
|             |       *     |#@@@@@@     |
|             | 
            |            |
+---------------------------+------------+
(#): stand for one byte main data
(*): stand for the BBM
(@): the free OOB data byte

Take write ooblen is mtd->oobavail for example,
the data in standard OOB area will be one more main data byte, due to
the BBM swap operation, lead to the expected OOB len as
(mtd->oobavail + 1). This is not as expected for one page.

> > Set oob databytes after
> > bad mark swap will lead to lost one oob free byte.
> 
> We don't care about free OOB bytes really.
> 

The MTD_OPS_AUTO_OOB need handle the free OOB bytes. Some filesystem
(like jffs2) also need store data at free OOB for management.

> > 
> > Therefore, just try to modify the spi nand framework for review.
> > And this may be common for the interleaved case.
> 
> I don't get how falling back to a memcpy will solve your problem? Can
> you please provide an anscii figure or something more visual to let
> us
> understand?

As talked above, the BBM swap and OOB shift handled at mtk ecc driver.
And the mtk controller will auto place data as mtk ECC data format in
pipelined.

Thanks
Xiangsheng Hou