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

Miquel Raynal
2021-11-22T08:57:47Z

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