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

Commit Message

Xiangsheng Hou Oct. 22, 2021, 2:40 a.m. UTC

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.

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. Set oob databytes after
bad mark swap will lead to lost one oob free byte.

Therefore, just try to modify the spi nand framework for review.
And this may be common for the interleaved case.

Signed-off-by: Xiangsheng Hou <[e-mail]>
---
 drivers/mtd/nand/spi/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Miquel Raynal Nov. 9, 2021, 12:05 p.m. UTC | #1

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.

[e-mail] 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

> 
> 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.

> Set oob databytes after
> bad mark swap will lead to lost one oob free byte.

We don't care about free OOB bytes really.

> 
> 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?

> 
> Signed-off-by: Xiangsheng Hou <[e-mail]>
> ---
>  drivers/mtd/nand/spi/core.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 2c8685f1f2fa..32a4707982c5 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -401,7 +401,8 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
>  		       req->datalen);
>  
>  	if (req->ooblen) {
> -		if (req->mode == MTD_OPS_AUTO_OOB)
> +		if (req->mode == MTD_OPS_AUTO_OOB &&
> +			nand->ecc.user_conf.placement != NAND_ECC_PLACEMENT_INTERLEAVED)
>  			mtd_ooblayout_get_databytes(mtd, req->oobbuf.in,
>  						    spinand->oobbuf,
>  						    req->ooboffs,
> @@ -442,7 +443,8 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
>  		       req->datalen);
>  
>  	if (req->ooblen) {
> -		if (req->mode == MTD_OPS_AUTO_OOB)
> +		if (req->mode == MTD_OPS_AUTO_OOB &&
> +			nand->ecc.user_conf.placement != NAND_ECC_PLACEMENT_INTERLEAVED)
>  			mtd_ooblayout_set_databytes(mtd, req->oobbuf.out,
>  						    spinand->oobbuf,
>  						    req->ooboffs,


Thanks,
Miquèl

Xiangsheng Hou Nov. 12, 2021, 8:33 a.m. UTC | #2

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.

> [e-mail] 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

Miquel Raynal Nov. 22, 2021, 9:01 a.m. UTC | #3

Hi xiangsheng,

Thanks for the figures, they are useful too understand better your
situation.

[e-mail] wrote on Fri, 12 Nov 2021 16:33:34
+0800:

> 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

Let's call this protected free OOB bytes

> part2: OOB bytes not be protected by ecc engine

Let's call this unprotected free OOB bytes

> part3: OOB bytes to store ecc parity for (sector data + FDM ECC bytes)

Let's call this 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.

Not necessarily. What if you just provide a translation in
prepare/finish which basically switches from one representation to the
other?

Whatever operation you try to do, we don't really care where all these
sections will be located once in memory, do we?

> 
> 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.
> 
> > [e-mail] 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
> 
> 
> 
> 
> 


Thanks,
Miquèl

Xiangsheng Hou Nov. 26, 2021, 8:51 a.m. UTC | #4

Hi Miquel,

On Mon, 2021-11-22 at 10:01 +0100, Miquel Raynal wrote:
> Hi xiangsheng,
> 
> Thanks for the figures, they are useful too understand better your
> situation.
> 
> [e-mail] wrote on Fri, 12 Nov 2021 16:33:34
> +0800:
> > 
> > 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.
> 
> Not necessarily. What if you just provide a translation in
> prepare/finish which basically switches from one representation to
> the
> other?
> 

For current design which mtd_ooblayout_set_databytes in
spinand_write_to_cache_op when MTD_AUTO_OOB_MODE can not work for BBM
swap situation.

In mtk nand_ecc_prepare_io_req, there have BBM swap lead to the
oobbuf[0] have one byte main data. The expected OOB data bytes will be
OOB availabe + 1 more byte. The mtd_ooblayout_set_databytes only set
OOB available bytes (will skip the reserve byte for BBM), lead to one
byte OOB data byte lost.

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

I will send next interation in the next few days, adjust set/get OOB
data byte operation in each ECC engine and only memcpy OOB buf in
write/read cache when AUTO OOB mode, which can both solve the this
situation and the AUTO OOB issue.

> Whatever operation you try to do, we don't really care where all
> these
> sections will be located once in memory, do we?

Yes, all the divergence need be handled at each ECC engine.

Thanks
Xiangsheng Hou

Patch

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 2c8685f1f2fa..32a4707982c5 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -401,7 +401,8 @@  static int spinand_read_from_cache_op(struct spinand_device *spinand,
 		       req->datalen);
 
 	if (req->ooblen) {
-		if (req->mode == MTD_OPS_AUTO_OOB)
+		if (req->mode == MTD_OPS_AUTO_OOB &&
+			nand->ecc.user_conf.placement != NAND_ECC_PLACEMENT_INTERLEAVED)
 			mtd_ooblayout_get_databytes(mtd, req->oobbuf.in,
 						    spinand->oobbuf,
 						    req->ooboffs,
@@ -442,7 +443,8 @@  static int spinand_write_to_cache_op(struct spinand_device *spinand,
 		       req->datalen);
 
 	if (req->ooblen) {
-		if (req->mode == MTD_OPS_AUTO_OOB)
+		if (req->mode == MTD_OPS_AUTO_OOB &&
+			nand->ecc.user_conf.placement != NAND_ECC_PLACEMENT_INTERLEAVED)
 			mtd_ooblayout_set_databytes(mtd, req->oobbuf.out,
 						    spinand->oobbuf,
 						    req->ooboffs,