Re: [RFC,v3 3/5] spi: add Mediatek SPI Nand controller driver

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

Hello,

xiangsheng.hou@mediatek.com wrote on Fri, 12 Nov 2021 16:40:24 +0800:

> Hi Miquel,
> 
> On Tue, 2021-11-09 at 12:46 +0100, Miquel Raynal wrote:
> > Hi Xiangsheng,
> > 
> > xiangsheng.hou@mediatek.com wrote on Fri, 22 Oct 2021 10:40:19 +0800:
> >   
> > > This version the SPI driver cowork with MTK pipelined
> > > HW ECC engine.
> > > 
> > > Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com>
> > > ---
> > > +
> > > +static int mtk_snfi_ecc_init(struct nand_device *nand)
> > > +{
> > > +	struct nand_ecc_props *reqs = &nand->ecc.requirements;
> > > +	struct nand_ecc_props *user = &nand->ecc.user_conf;
> > > +	struct nand_ecc_props *conf = &nand->ecc.ctx.conf;
> > > +	struct mtk_snfi *snfi = mtk_nand_to_spi(nand);
> > > +	struct mtk_ecc_engine *eng;
> > > +	u32 spare, idx;
> > > +	int ret;
> > > +
> > > +	eng = kzalloc(sizeof(*eng), GFP_KERNEL);
> > > +	if (!eng)
> > > +		return -ENOMEM;
> > > +
> > > +	nand->ecc.ctx.priv = eng;
> > > +	nand->ecc.engine->priv = eng;
> > > +
> > > +	/* Configure the correction depending on the NAND device
> > > topology */
> > > +	if (user->step_size && user->strength) {
> > > +		conf->step_size = user->step_size;
> > > +		conf->strength = user->strength;
> > > +	} else if (reqs->step_size && reqs->strength) {
> > > +		conf->step_size = reqs->step_size;
> > > +		conf->strength = reqs->strength;
> > > +	}
> > > +
> > > +	/*
> > > +	 * align eccstrength and eccsize.
> > > +	 * The MTK HW ECC engine only support 512 and 1024 eccsize.
> > > +	 */
> > > +	if (conf->step_size < 1024) {
> > > +		if (nand->memorg.pagesize > 512 &&
> > > +			    snfi->caps->max_sector_size > 512) {
> > > +			conf->step_size = 1024;
> > > +			conf->strength <<= 1;
> > > +		} else {
> > > +			conf->step_size = 512;
> > > +		}
> > > +	} else {
> > > +		conf->step_size = 1024;
> > > +	}
> > > +
> > > +	ret = mtk_snfi_set_spare_per_sector(nand, snfi, &spare, &idx);
> > > +
> > > +	/* These will be used by the snfi driver */
> > > +	snfi->ecc.page_size = nand->memorg.pagesize;
> > > +	snfi->ecc.spare_per_sector = spare;
> > > +	snfi->ecc.spare_idx = idx;
> > > +	snfi->ecc.sectors = nand->memorg.pagesize / conf->step_size;
> > > +
> > > +	/* These will be used by HW ECC engine */
> > > +	eng->oob_per_sector = spare;
> > > +	eng->nsteps = snfi->ecc.sectors;  
> > 
> > I believe most of this function should move into mtk_ecc.c.  
> 
> I'm also confused about this when coding.
> Obviously, most of the code logic belong to the ecc driver.
> 
> However, some ecc related parameter have to config at the snfi
> controller register, such as sector size, available oob bytes for each
> sector used to calculate ecc level. The are all attribute defined at
> the snfi controller register.
> 
> How about I move these code logic, sector size and useable spare size
> for each sector which belog to the snfi controller attribute to the ecc
> driver, parse and config when mtk_snfi_ecc_prepare_io_req in the snfi
> driver?

Yes, do the whole computation in the ECC driver, just provide the value
to write to the snfi driver through a custom helper to hide all ECC
operation from the SPI side.

> 
> >   
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int mtk_snfi_ecc_init_ctx(struct nand_device *nand)
> > > +{
> > > +	struct nand_ecc_engine_ops *ops = mtk_ecc_get_pipelined_ops();
> > > +	int ret;
> > > +
> > > +	ret = mtk_snfi_ecc_init(nand);
> > > +	if (ret) {
> > > +		pr_info("mtk snfi ecc init fail!\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	return ops->init_ctx(nand);
> > > +}
> > > +
> > > +static void mtk_snfi_ecc_cleanup_ctx(struct nand_device *nand)
> > > +{
> > > +	struct nand_ecc_engine_ops *ops = mtk_ecc_get_pipelined_ops();
> > > +
> > > +	ops->cleanup_ctx(nand);
> > > +}
> > > +
> > > +static int mtk_snfi_prepare_for_ecc(struct nand_device *nand,
> > > +					struct mtk_snfi *snfi)
> > > +{
> > > +	struct nand_ecc_props *conf = &nand->ecc.ctx.conf;
> > > +	struct mtk_ecc_engine *eng = nand->ecc.ctx.priv;
> > > +	u32 val;
> > > +
> > > +	switch (nand->memorg.pagesize) {
> > > +	case 512:
> > > +		val = PAGEFMT_512_2K | PAGEFMT_SEC_SEL_512;
> > > +		break;
> > > +	case KB(2):
> > > +		if (conf->step_size == 512)
> > > +			val = PAGEFMT_2K_4K | PAGEFMT_SEC_SEL_512;
> > > +		else
> > > +			val = PAGEFMT_512_2K;
> > > +		break;
> > > +	case KB(4):
> > > +		if (conf->step_size == 512)
> > > +			val = PAGEFMT_4K_8K | PAGEFMT_SEC_SEL_512;
> > > +		else
> > > +			val = PAGEFMT_2K_4K;
> > > +		break;
> > > +	case KB(8):
> > > +		if (conf->step_size == 512)
> > > +			val = PAGEFMT_8K_16K | PAGEFMT_SEC_SEL_512;
> > > +		else
> > > +			val = PAGEFMT_4K_8K;
> > > +		break;
> > > +	case KB(16):
> > > +		val = PAGEFMT_8K_16K;
> > > +		break;
> > > +	default:
> > > +		dev_err(snfi->dev, "invalid page len: %d\n",
> > > +				nand->memorg.pagesize);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	snfi->fdm_size = eng->fdm_size;
> > > +	snfi->fdm_ecc_size = eng->fdm_ecc_size;
> > > +
> > > +	val |= snfi->ecc.spare_idx << PAGEFMT_SPARE_SHIFT;
> > > +	val |= snfi->fdm_size << PAGEFMT_FDM_SHIFT;
> > > +	val |= snfi->fdm_ecc_size << PAGEFMT_FDM_ECC_SHIFT;
> > > +	writel(val, snfi->regs + NFI_PAGEFMT);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int mtk_snfi_ecc_prepare_io_req(struct nand_device *nand,
> > > +						struct nand_page_io_req
> > > *req)
> > > +{
> > > +	struct nand_ecc_engine_ops *ops = mtk_ecc_get_pipelined_ops();
> > > +	struct mtk_snfi *snfi = mtk_nand_to_spi(nand);
> > > +	int ret;
> > > +
> > > +	snfi->ecc.enabled = (req->mode != MTD_OPS_RAW);  
> > 
> > Shouldn't you check ecc.enabled before calling prepare_for_ecc ?  
> 
> The funcion name may make you confused.
> Actually, the prepare_for_ecc function did not include any logic about
> ecc enable or not.Only config pagesize/sparesize/sector size to snfi
> controller register.
> 
> I will modify the function name mtk_snfi_prepare_for_ecc,
> mtk_snfi_config for example.

Yes please.

> 
> >   
> > > +	ret = mtk_snfi_prepare_for_ecc(nand, snfi);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return ops->prepare_io_req(nand, req);
> > > +}
> > > +
> > > +static int mtk_snfi_ecc_finish_io_req(struct nand_device *nand,
> > > +						struct nand_page_io_req
> > > *req)
> > > +{
> > > +	struct nand_ecc_engine_ops *ops = mtk_ecc_get_pipelined_ops();
> > > +	struct mtk_ecc_engine *eng = nand->ecc.ctx.priv;
> > > +	struct mtk_snfi *snfi = mtk_nand_to_spi(nand);
> > > +
> > > +	if (snfi->ecc.enabled) {  
> > 
> > I am currently looking at a better way of keeping this
> > information, while being safer against concurrent accesses. So
> > far parallel operations are not supported so this is safe, but
> > we might improve the core in a little while and I don't want
> > this to be an issue. My next iteration on the Macronix engine will
> > solve this.
> >   
> Look forward to the next interation.
> 
> Thanks
> Xiangsheng Hou


Thanks,
Miquèl

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