Re: [RFC,v4,3/5] spi: mtk: Add mediatek SPI Nand Flash interface driver

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

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

> Hi Miquel,
> 
> On Thu, 2021-12-09 at 11:20 +0100, Miquel Raynal wrote:
> > Hi Xiangsheng,
> > 
> > xiangsheng.hou@mediatek.com wrote on Tue, 30 Nov 2021 16:32:00 +0800:
> >   
> > > 
> > > +
> > > +static int mtk_snfi_config(struct nand_device *nand,
> > > +			   struct mtk_snfi *snfi)
> > > +{
> > > +	struct mtk_ecc_engine *eng = mtk_snfi_to_ecc_engine(snfi);
> > > +	u32 val;
> > > +
> > > +	switch (nanddev_page_size(nand)) {
> > > +	case 512:
> > > +		val = PAGEFMT_512_2K | PAGEFMT_SEC_SEL_512;
> > > +		break;
> > > +	case KB(2):
> > > +		if (eng->section_size == 512)
> > > +			val = PAGEFMT_2K_4K | PAGEFMT_SEC_SEL_512;
> > > +		else
> > > +			val = PAGEFMT_512_2K;
> > > +		break;
> > > +	case KB(4):
> > > +		if (eng->section_size == 512)
> > > +			val = PAGEFMT_4K_8K | PAGEFMT_SEC_SEL_512;
> > > +		else
> > > +			val = PAGEFMT_2K_4K;
> > > +		break;
> > > +	case KB(8):
> > > +		if (eng->section_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",
> > > +			nanddev_page_size(nand));
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	val |= eng->oob_per_section_idx << PAGEFMT_SPARE_SHIFT;
> > > +	val |= eng->oob_free << PAGEFMT_FDM_SHIFT;
> > > +	val |= eng->oob_free_protected << PAGEFMT_FDM_ECC_SHIFT;
> > > +	writel(val, snfi->regs + NFI_PAGEFMT);  
> > 
> > Shouldn't this be calculated only once?  
> 
> Yes, The mtk_snfi_config function can be only called in prepare_io_req
> at the first time. I will add a variable to indicate config done or not
> to avoid calculate repeatedly.

No, it's fine to write down the configuration in the registers each
time you enter ->prepare() because you do not know if the engine was
used for another device or not. But what you should not have to do is to
recalculate everything before this register write. You need to move the
entire calculation to ->init_ctx() which is called only once for a
given device and then apply the configuration each time you enter
->prepare().

> > > +	return 0;
> > > +}
> > > +
> > > +static int mtk_snfi_ecc_init_ctx(struct nand_device *nand)
> > > +{
> > > +	struct nand_ecc_engine_ops *ops = mtk_ecc_get_pipelined_ops();
> > > +
> > > +	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_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;
> > > +
> > > +	ret = mtk_snfi_config(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_to_ecc_ctx(nand);
> > > +	struct mtk_snfi *snfi = mtk_nand_to_spi(nand);
> > > +
> > > +	if (req->mode != MTD_OPS_RAW)
> > > +		eng->read_empty = readl(snfi->regs + NFI_STA) &
> > > STA_EMP_PAGE;
> > > +
> > > +	return ops->finish_io_req(nand, req);
> > > +}
> > > +
> > > +
> > > +MODULE_LICENSE("GPL v2");
> > > +MODULE_AUTHOR("Xiangsheng Hou <xiangsheng.hou@mediatek.com>");
> > > +MODULE_DESCRIPTION("Mediatek SPI Nand Flash interface driver");  
> > 
> > Otherwise looks good, I believe you can drop the RFC prefix now.
> >   
> 
> I will prepare the formal patch and send for review after internal
> review and test.
> 
> Thanks
> Xiangsheng Hou


Thanks,
Miquèl

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