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

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

Hello,

xiangsheng.hou@mediatek.com wrote on Fri, 10 Dec 2021 17:09:14 +0800:

> Hi Miquel,
> 
> On Thu, 2021-12-09 at 11:32 +0100, Miquel Raynal wrote:
> > Hi Xiangsheng,
> > 
> > xiangsheng.hou@mediatek.com wrote on Tue, 30 Nov 2021 16:31:59 +0800:
> >   
> > > 
> > > +static void mtk_ecc_no_bbm_swap(struct nand_device *a, u8 *b, u8
> > > *c)
> > > +{
> > > +	/* nop */  
> > 
> > Is this really useful?  
> 
> For 512 bytes page size, it is no need to do BBM swap due to the ECC
> engine step size will be 512 bytes.
> 
> However, there have 512 bytes SLC NAND page size in history, although
> have not seen such SPI/Parallel NAND device for now.
> 
> Do you think there no need to consider this small page device?

Actually I was talking about the empty helper itself. But let's keep
that aside for now, it's fine.

> 
> >   
> > > +}
> > > +
> > > +static void mtk_ecc_bbm_swap(struct nand_device *nand, u8
> > > *databuf, u8 *oobbuf)
> > > +{
> > > +	struct mtk_ecc_engine *eng = nand_to_ecc_ctx(nand);
> > > +	int step_size = nand->ecc.ctx.conf.step_size;
> > > +	u32 bbm_pos = eng->bbm_ctl.position;
> > > +
> > > +	bbm_pos += eng->bbm_ctl.section * step_size;
> > > +
> > > +	swap(oobbuf[0], databuf[bbm_pos]);
> > > +}
> > > +
> > > +static void mtk_ecc_set_bbm_ctl(struct mtk_ecc_bbm_ctl *bbm_ctl,
> > > +				struct nand_device *nand)
> > > +{
> > > +	if (nanddev_page_size(nand) == 512) {
> > > +		bbm_ctl->bbm_swap = mtk_ecc_no_bbm_swap;
> > > +	} else {
> > > +		bbm_ctl->bbm_swap = mtk_ecc_bbm_swap;
> > > +		bbm_ctl->section = nanddev_page_size(nand) /
> > > +				   mtk_ecc_data_len(nand);
> > > +		bbm_ctl->position = nanddev_page_size(nand) %
> > > +				    mtk_ecc_data_len(nand);
> > > +	}
> > > +}
> > > 
> > > +
> > > +static struct device *mtk_ecc_get_engine_dev(struct device *dev)
> > > +{
> > > +	struct platform_device *eccpdev;
> > > +	struct device_node *np;
> > > +
> > > +	/*
> > > +	 * The device node is only the host controller,
> > > +	 * not the actual ECC engine when pipelined case.
> > > +	 */
> > > +	np = of_parse_phandle(dev->of_node, "nand-ecc-engine", 0);
> > > +	if (!np)
> > > +		return NULL;
> > > +
> > > +	eccpdev = of_find_device_by_node(np);
> > > +	if (!eccpdev) {
> > > +		of_node_put(np);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	platform_device_put(eccpdev);
> > > +	of_node_put(np);
> > > +
> > > +	return &eccpdev->dev;
> > > +}  
> > 
> > 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

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


Thanks,
Miquèl

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