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

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

Hi Xiangsheng,

xiangsheng.hou@mediatek.com wrote on Sat, 11 Dec 2021 11:25:46 +0800:

> Hi Miquel,
> 
> On Fri, 2021-12-10 at 10:34 +0100, Miquel Raynal wrote:
> > Hello,
> > 
> > xiangsheng.hou@mediatek.com wrote on Fri, 10 Dec 2021 17:09:14 +0800:  
> > >   
> > > > As this will be the exact same function for all the pipelined
> > > > engines,
> > > > I am tempted to put this in the core. I'll soon send a iteration,
> > > > stay
> > > > tuned.
> > > >     
> > > 
> > > Look forward to the function.  
> > 
> > I sent the new version yesterday but I
> > * forgot to CC: you
> > * forgot about that function as well
> > 
> > Let's ignore this comment for now, send your driver with the same
> > function in it and I'll clean that up later.
> > 
> > Here is the new iteration, sorry for forgetting to send it to you as
> > well:
> >   
> https://lore.kernel.org/linux-mtd/20211209174046.535229-1-miquel.raynal@bootlin.com/T/
> > And here is a Github branch as well:
> > https://github.com/miquelraynal/linux/tree/ecc-engine  
> 
> Got it, Thanks.
> 
> >   
> > > > > +				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 ?

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.

> 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


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/