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

[xiangsheng.hou <xiangsheng.hou@mediatek.com>]

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?

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

> > +/*
> > + * mtk_ecc_data_format() - Convert to/from MTK ECC on-flash data
> > format
> > + *
> > + * MTK ECC engine organize page data by section, the on-flash
> > format as bellow:
> > + * ||          section 0         ||          section 1          ||
> > ...
> > + * || data | OOB free | OOB ECC || data || OOB free | OOB ECC ||
> > ...
> > + *
> > + * Terefore, it`s necessary to convert data when reading/writing
> > in raw mode.
> > + */
> > +static void mtk_ecc_data_format(struct nand_device *nand,
> 
> mtk_ecc_reorganize_data_layout()?

Will be changed.

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

> > +			memcpy(mtk_ecc_section_ptr(nand, i),
> > +			       databuf + mtk_ecc_data_off(nand, i),
> > +			       step_size);
> > +
> > +			memcpy(mtk_ecc_oob_free_ptr(nand, i),
> > +			       oobbuf + mtk_ecc_oob_free_position(nand,
> > i),
> > +			       eng->oob_free);
> > +
> > +			memcpy(mtk_ecc_oob_free_ptr(nand, i) + eng-
> > >oob_free,
> > +			       oobbuf + eng->oob_free * eng->nsteps +
> > +			       i * eng->oob_ecc,
> > +			       eng->oob_ecc);
> > +		}
> > +
> > +		req->databuf.out = eng->bounce_page_buf;
> > +		req->oobbuf.out = eng->bounce_oob_buf;
> > +	} else {
> > +		databuf = req->databuf.in;
> > +		oobbuf = req->oobbuf.in;
> > +
> > +		/*
> > +		 * Convert the on-flash MTK ECC data format to
> > +		 * destination databuf and oobbuf.
> > +		 */
> > +		memcpy(eng->bounce_page_buf, databuf,
> > +		       nanddev_page_size(nand));
> > +		memcpy(eng->bounce_oob_buf, oobbuf,
> > +		       nanddev_per_page_oobsize(nand));
> > +
> > +		for (i = 0; i < eng->nsteps; i++) {
> > +			memcpy(databuf + mtk_ecc_data_off(nand, i),
> > +			       mtk_ecc_section_ptr(nand, i),
> > step_size);
> > +
> > +			memcpy(oobbuf + mtk_ecc_oob_free_position(nand,
> > i),
> > +			       mtk_ecc_section_ptr(nand, i) +
> > step_size,
> > +			       eng->oob_free);
> > +
> > +			memcpy(oobbuf + eng->oob_free * eng->nsteps +
> > +			       i * eng->oob_ecc,
> > +			       mtk_ecc_section_ptr(nand, i) + step_size
> > +			       + eng->oob_free,
> > +			       eng->oob_ecc);
> > +
> > +			if (i == eng->bbm_ctl.section)
> > +				eng->bbm_ctl.bbm_swap(nand,
> > +						      databuf, oobbuf);
> > +		}
> > +	}
> > +}
> > +
> > 
> > 
> > +/**
> > + * struct mtk_ecc_engine - Information relative to the ECC
> > + * @req_ctx: Save request context and tweak the original request
> > to fit the
> > + *           engine needs
> > + * @oob_per_section: OOB size for each section to store OOB
> > free/ECC bytes
> > + * @oob_per_section_idx: The index for @oob_per_section in spare
> > size array
> > + * @oob_ecc: OOB size for each section to store the ECC parity
> > + * @oob_free: OOB size for each section to store the OOB free
> > bytes
> > + * @oob_free_protected: OOB free bytes will be protected by the
> > ECC engine
> > + * @section_size: The size of each section
> > + * @read_empty: Indicate whether empty page for one read operation
> > + * @nsteps: The number of the sections
> > + * @src_page_buf: Buffer used to store source data buffer when
> > write
> > + * @src_oob_buf: Buffer used to store source OOB buffer when write
> > + * @bounce_page_buf: Data bounce buffer
> > + * @bounce_oob_buf: OOB bounce buffer
> > + * @ecc: The ECC engine private data structure
> > + * @ecc_cfg: The configuration of each ECC operation
> > + * @bbm_ctl: Information relative to the BBM swap
> > + */
> > +struct mtk_ecc_engine {
> > +	struct nand_ecc_req_tweak_ctx req_ctx;
> > +
> > +	u32 oob_per_section;
> > +	u32 oob_per_section_idx;
> > +	u32 oob_ecc;
> > +	u32 oob_free;
> > +	u32 oob_free_protected;
> > +	u32 section_size;
> > +
> > +	bool read_empty;
> > +	u32 nsteps;
> > +
> > +	u8 *src_page_buf;
> > +	u8 *src_oob_buf;
> > +	u8 *bounce_page_buf;
> > +	u8 *bounce_oob_buf;
> > +
> > +	struct mtk_ecc *ecc;
> > +	struct mtk_ecc_config ecc_cfg;
> > +	struct mtk_ecc_bbm_ctl bbm_ctl;
> > +};
> 
> This and above should not be exported and be located in the driver.
> 

I will fix this.

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