Re: [RFC,v4,2/5] mtd: nand: ecc: mtk: Convert to the ECC infrastructure

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi xiangsheng.hou,

xiangsheng.hou@mediatek.com wrote on Tue, 14 Dec 2021 11:32:14 +0800:

> Hi Miquel,
> 
> 
> On Mon, 2021-12-13 at 10:29 +0100, Miquel Raynal wrote:
> > Hi Xiangsheng,
> > 
> > xiangsheng.hou@mediatek.com wrote on Sat, 11 Dec 2021 11:25:46 +0800:  
> > >   
> > > >     
> > > > > > > +				struct nand_page_io_req *req)
> > > > > > > +{
> > > > > > > +	struct mtk_ecc_engine *eng = nand_to_ecc_ctx(nand);
> > > > > > > +	int step_size = nand->ecc.ctx.conf.step_size;
> > > > > > > +	void *databuf, *oobbuf;
> > > > > > > +	int i;
> > > > > > > +
> > > > > > > +	if (req->type == NAND_PAGE_WRITE) {
> > > > > > > +		databuf = (void *)req->databuf.out;
> > > > > > > +		oobbuf = (void *)req->oobbuf.out;
> > > > > > > +
> > > > > > > +		/*
> > > > > > > +		 * Convert the source databuf and oobbuf to MTK
> > > > > > > ECC
> > > > > > > +		 * on-flash data format.
> > > > > > > +		 */
> > > > > > > +		for (i = 0; i < eng->nsteps; i++) {
> > > > > > > +			if (i == eng->bbm_ctl.section)
> > > > > > > +				eng->bbm_ctl.bbm_swap(nand,
> > > > > > > +						      databuf,
> > > > > > > oobbuf);      
> > > > > > 
> > > > > > Do you really need this swap? Isn't the overall move enough
> > > > > > to
> > > > > > put
> > > > > > the
> > > > > > BBM at the right place?
> > > > > >       
> > > > > 
> > > > > For OPS_RAW mode, need organize flash data in the MTK ECC
> > > > > engine
> > > > > data
> > > > > format. Other operation in this function only organize data by
> > > > > section
> > > > > and not include BBM swap.
> > > > > 
> > > > > For other mode, this function will not be called.    
> > > > 
> > > > Can you try to explain this with an ascii schema again? I'm sorry
> > > > but
> > > > I
> > > > don't follow it. Is the BBM placed in the first bytes of the
> > > > first
> > > > oob
> > > > area by the engine? Or is it place somewhere else?
> > > >     
> > > 
> > > Yes, the BBM will at the first OOB area in NAND standard layout
> > > after
> > > BBM swap.
> > > 
> > > 0. Differential on-flash data layout
> > > 
> > > NAND standard page layout
> > > +------------------------------+------------+  
> > > >                              |            |
> > > >            main area         |   OOB area |
> > > >                              |            |  
> > > 
> > > +------------------------------+------------+
> > > 
> > > MTK ECC on-flash page layout (2 section for example)
> > > +------------+--------+------------+--------+  
> > > >            |        |            |        |    
> > > > section(0) | OOB(0) | section(1) | OOB(1) |
> > > >            |        |            |        |   
> > > 
> > > +------------+--------+------------+--------+  
> > 
> > I think we are aligned on that part.
> > 
> > The BBM is purely a user conception, it is not something wired in the
> > hardware. What I mean is: why do *you* think the BBM should be
> > located
> > in the middle of section #1 ?  
> 
> Take NAND page 2KB and OOB 64 bytes for example.
> 
> For the NAND perspective the BBM is located at OOB #0, the column
> address is 2048 in one page, and this should not only a user
> conception.
> 
> No matter what layout, the BBM position need at column 2048(OOB #0).
> Because of the NAND device specification arrange this.
> 
> That is, The BBM position of a worn bad block(user mark) need
> consistent with a factory bad block(flash vendor mark).

Yeah actually you're right, I guess the factory bad blocks will be
located at the "wrong" position regarding the engine layout...

Fine for this function.

I'll send today a v5 for the series with a new
helper: nand_ecc_get_engine_dev() (so you don't need to have your own)
as well as a change in the API of spi_mem_generic_supports_op() that
you must use.

Thanks,
Miquèl

> For the MTK ECC engine reorganize data by section in unit.
> The step size will be 1024 bytes and the OOB area for each section will
> be 32 bytes with NAND page 2KB and OOB 64 bytes.
> 
> The on-flash page data lauout for MTK ECC engine,
> column 2048 will be main data in the middle of section #1.
> +----------------+-------+----------------+-------+
> |                |       |                |       |
> |     1024B      |  32B  |    1024B       |  32B  |
> |                |       |                |       |
> +----------------+-------+----------------+-------+
> 
> > 
> > There is one layout: the layout from the NAND/MTD perspective.
> > There is another layout: the layout of your ECC engine.
> > 
> > Just consider that the BBM should be at byte 0 of OOB #0 and you will
> > not need any BBM swap operation anymore. I don't understand why you
> > absolutely want to put it in section #1.  
> 
> For the read/write both in OPS_RAW mode, the data layout will be
> organized by the ECC engine, no matter how the layout comply with, this
> will be workable.
> 
> However, it will be chaotic when mix OPS_RAW operation with
> OPS_AUTO_OOB/OPS_PLACE_OOB.
> 
> That is, the OPS_RAW mode on-flash data layout need comply with
> OPS_AUTO_OOB/OPS_PLACE_OOB, this is the reorganize function purpose.
> 
> Just take the mtd/tests/nandbiterrs.c for example.
> It will write data in OPS_PLACE_OOB mode and rewrite data in OPS_RAW
> mode after insert one bitflip. Then read in OPS_PLACE_OOB mode to check
> the bitflips.
> 
> > > The standard BBM position will be section(1) main data,
> > > need do the BBM swap operation.
> > > 
> > > request buffer include req->databuf and req->oobbuf.
> > > +----------------------------+  
> > > >                            |
> > > >     req->databuf           |
> > > >                            |  
> > > 
> > > +----------------------------+
> > > 
> > > +-------------+  
> > > >             |
> > > > req->oobbuf |
> > > >             |  
> > > 
> > > +-------------+
> > > 
> > > 1. For the OPS_RAW mode
> > > 
> > > Expect the on-flash data format is like MTK ECC layout.
> > > The snfi controller will put the on-flash data as is
> > > spi_mem_op->data.buf.out.
> > > 
> > > Therefore, the ECC engine have to reorganize the request
> > > data and OOB buffer in 4 part for each section in
> > > OPS_RAW mode.
> > > 
> > > 1) BBM swap, only for the section need do the swap
> > > 2) section main data
> > > 3) OOB free data
> > > 4) OOB ECC data
> > > 
> > > The BBM swap will ensure the BBM position in MTK ECC
> > > on-flash layout is same as NAND standard layout in
> > > OPS_RAW mode.
> > > 
> > > for (i = 0; i < eng->nsteps; i++) {
> > > 
> > >         /* part 1: BBM swap */
> > >         if (i == eng->bbm_ctl.section)
> > >                 eng->bbm_ctl.bbm_swap(nand,
> > >                                       databuf, oobbuf);
> > > 
> > > 	/* part 2: main data in this section */
> > >         memcpy(mtk_ecc_section_ptr(nand, i),
> > >                databuf + mtk_ecc_data_off(nand, i),
> > >                step_size);
> > > 
> > >         /* part 3: OOB free data */
> > >         memcpy(mtk_ecc_oob_free_ptr(nand, i),
> > >                oobbuf + mtk_ecc_oob_free_position(nand, i),
> > >                eng->oob_free);
> > > 
> > >         /* part 4: OOB ECC data */
> > >         memcpy(mtk_ecc_oob_free_ptr(nand, i) + eng->oob_free,
> > >                oobbuf + eng->oob_free * eng->nsteps +
> > >                i * eng->oob_ecc,
> > >                eng->oob_ecc);
> > > }
> > > 
> > > 2. For non OPS_RAW mode
> > > 
> > > The snfi have a function called auto format with ECC enable.
> > > This will auto reorganize the request data and oob data in
> > > MTK ECC page layout by the snfi controller except the BBM position.
> > > 
> > > Therefore, the ECC engine only need do the BBM swap after set OOB
> > > data
> > > bytes in OPS_AUTO or after memcpy oob data in OPS_PLACE_OOB for
> > > write
> > > operation.
> > > 
> > > The BBM swap also ensure the BBM position in MTK ECC on-flash
> > > layout is
> > > same as NAND standard layout in non OPS_RAW mode.
> > > 
> > > if (req->ooblen) {
> > >         if (req->mode == MTD_OPS_AUTO_OOB) {
> > >                 ret = mtd_ooblayout_set_databytes(mtd,
> > >                                                   req->oobbuf.out,
> > >                                                   eng-  
> > > >bounce_oob_buf,  
> > >                                                   req->ooboffs,
> > >                                                   mtd->oobavail);
> > >                 if (ret)
> > >                         return ret;
> > >         } else {
> > >                 memcpy(eng->bounce_oob_buf + req->ooboffs,
> > >                        req->oobbuf.out,
> > >                        req->ooblen);
> > >         }
> > > }
> > > 
> > > eng->bbm_ctl.bbm_swap(nand, (void *)req->databuf.out,
> > >                       eng->bounce_oob_buf);
> > >   
> 
> Thanks
> Xiangsheng Hou

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek