Re: [RFC,v3 2/5] mtd: ecc: realize Mediatek HW ECC driver

Miquel Raynal
2021-11-22T08:59:29Z

Hi xiangsheng,

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

> Hi Miquel,
>
> On Tue, 2021-11-09 at 13:18 +0100, Miquel Raynal wrote:
> > Hi Xiangsheng,
> >
> > xiangsheng.hou@mediatek.com wrote on Fri, 22 Oct 2021 10:40:18 +0800:
> >
> > > The v3 driver realize Mediatek HW ECC engine with pipelined
> > > case.
> >
> > v3 driver? I guess you are talking about the hardware?
> >
>
> Just standard for the RFC v3 patch.
>
> > I don't think 'realize' makes much sense here. Perhaps the title
> > could
> > be:
> > "mtd: ecc: mtk: Convert to the ECC infrastructure
> >
>
> I will fix it at next iteration.
>
> > >
> > > Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com>
> > > ---
> > > drivers/mtd/nand/core.c | 10 +-
> > > drivers/mtd/nand/ecc.c | 88 +++++++
> > > drivers/mtd/nand/mtk_ecc.c | 488
> > > ++++++++++++++++++++++++++++++++++++
> > > include/linux/mtd/mtk_ecc.h | 38 +++
> > > include/linux/mtd/nand.h | 11 +
> > > 5 files changed, 632 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c
> > > index 5e13a03d2b32..b228b4d13b7a 100644
> > > --- a/drivers/mtd/nand/core.c
> > > +++ b/drivers/mtd/nand/core.c
> > > @@ -232,7 +232,9 @@ static int nanddev_get_ecc_engine(struct
> > > nand_device *nand)
> > > nand->ecc.engine = nand_ecc_get_on_die_hw_engine(nand);
> > > break;
> > > case NAND_ECC_ENGINE_TYPE_ON_HOST:
> > > - pr_err("On-host hardware ECC engines not supported
> > > yet\n");
> > > + nand->ecc.engine =
> > > nand_ecc_get_on_host_hw_engine(nand);
> > > + if (PTR_ERR(nand->ecc.engine) == -EPROBE_DEFER)
> > > + return -EPROBE_DEFER;
> >
> > Please base your series on top of my previous work (even if not 100%
> > stable yet) to avoid leaking core changes here. They already exist,
> > no
> > need to duplicate them.
>
> Will be remoed in next iteration.
>
> > > +static int mt7986_ecc_regs[] = {
> > > + [ECC_ENCPAR00] = 0x300,
> > > + [ECC_ENCIRQ_EN] = 0x80,
> > > + [ECC_ENCIRQ_STA] = 0x84,
> > > + [ECC_DECDONE] = 0x124,
> > > + [ECC_DECIRQ_EN] = 0x200,
> > > + [ECC_DECIRQ_STA] = 0x204,
> > > +};
> > > +
> > > static inline void mtk_ecc_wait_idle(struct mtk_ecc *ecc,
> > > enum mtk_ecc_operation op)
> > > {
> > > @@ -447,6 +464,464 @@ unsigned int mtk_ecc_get_parity_bits(struct
> > > mtk_ecc *ecc)
> > > }
> > > EXPORT_SYMBOL(mtk_ecc_get_parity_bits);
> > >
> > > +static inline int data_off(struct nand_device *nand, int i)
> >
> > Please always prefix all your helpers with mtk_ecc_
> >
> > > +{
> > > + int eccsize = nand->ecc.ctx.conf.step_size;
> > > +
> > > + return i * eccsize;
> > > +}
> > > +
> > > +static inline int fdm_off(struct nand_device *nand, int i)
> >
> > What is fdm?
>
> As talked in the set/get OOB bytes series, fdm stand for flash disk
> management data. It`s OOB bytes besides the ecc parity in each sector
> OOB area. That is free OOB bytes and the BBM.
>
> >
> > > +{
> > > + struct mtk_ecc_engine *eng = nand->ecc.ctx.priv;
> > > + int poi;
> > > +
> > > + if (i < eng->bad_mark.sec)
> >
> > sec does not mean anything to me
>
> sec standard for sector since mtk ECC engine organize data as sector in
> unit. And there have BBM swap operation in ecc driver to ensure BBM
> position consistent with nand specification. Please refer to the
> set/get oob series.

As a general rule, you may refer to the datasheet keywords because it's
easier when reading the driver and the doc but in general I will ask
you to be very clear about the terms you use: the NAND core contains
tens of different drivers, all coming from different companies with
different wordings. Please try to translate the MTK working into
something that is understandable from someone not in your company.

FDM is not a generic term, sector is not the word we generally use
(which in general refers to disks), etc.

> > > + poi = (i + 1) * eng->fdm_size;
> > > + else if (i == eng->bad_mark.sec)
> > > + poi = 0;
> > > + else
> > > + poi = i * eng->fdm_size;
> > > +
> > > + return poi;
> > > +}
> > > +
> > > +static inline int mtk_ecc_data_len(struct nand_device *nand)
> > > +{
> > > + struct mtk_ecc_engine *eng = nand->ecc.ctx.priv;
> > > + int eccsize = nand->ecc.ctx.conf.step_size;
> > > + int eccbytes = eng->oob_ecc;
> > > +
> > > + return eccsize + eng->fdm_size + eccbytes;
> > > +}
> > > +
> > > +static inline u8 *mtk_ecc_sec_ptr(struct nand_device *nand, int
> > > i)
> > > +{
> > > + struct mtk_ecc_engine *eng = nand->ecc.ctx.priv;
> > > +
> > > + return eng->page_buf + i * mtk_ecc_data_len(nand);
> > > +}
> > > +
> > > +static inline u8 *mtk_ecc_fdm_ptr(struct nand_device *nand, int i)
> > > +{
> > > + struct mtk_ecc_engine *eng = nand->ecc.ctx.priv;
> > > + int eccsize = nand->ecc.ctx.conf.step_size;
> > > +
> > > + return eng->page_buf + i * mtk_ecc_data_len(nand) + eccsize;
> > > +}
> > > +
> > > +static void mtk_ecc_no_bad_mark_swap(struct nand_device *a, u8 *b,
> > > + u8 *c)
> > > +{
> > > + /* nop */
> > > +}
> > > +
> > > +static void mtk_ecc_bad_mark_swap(struct nand_device *nand, u8
> > > *databuf,
> > > + u8 *oobbuf)
> > > +{
> > > + struct mtk_ecc_engine *eng = nand->ecc.ctx.priv;
> > > + int step_size = nand->ecc.ctx.conf.step_size;
> > > + u32 bad_pos = eng->bad_mark.pos;
> > > +
> > > + bad_pos += eng->bad_mark.sec * step_size;
> > > +
> > > + swap(oobbuf[0], databuf[bad_pos]);
> >
> > please change "bad" or "bad mark" by "bbm" which is the name used
> > everywhere else in this subsystem.
> >
>
> Will fix these at next iteration.
>
> > > +}
> > > +
> > > +static void mtk_ecc_set_bad_mark_ctl(struct mtk_ecc_bad_mark_ctl
> > > *bm_ctl,
> > > + struct mtd_info *mtd)
> > > +{
> > > + struct nand_device *nand = mtd_to_nanddev(mtd);
> > > +
> > > + if (mtd->writesize == 512) {
> > > + bm_ctl->bm_swap = mtk_ecc_no_bad_mark_swap;
> > > + } else {
> > > + bm_ctl->bm_swap = mtk_ecc_bad_mark_swap;
> > > + bm_ctl->sec = mtd->writesize / mtk_ecc_data_len(nand);
> > > + bm_ctl->pos = mtd->writesize % mtk_ecc_data_len(nand);
> >
> > sec? pos? what does this mean?
>
> sec stand for sector and pos stand for position.
>
> mtk_ecc_set_bad_mark_ctl function is the logic to calculate the BBM
> swap at which sector and the position in the sector.
>
> > > + *
> > > + * Mediatek HW ECC engine organize data/oob free/oob ecc by
> > > sector,
> > > + * the data format for one page as bellow:
> > > + * || sector 0 || sector 1 ||
> > > ...
> > > + * || data | fdm | oob ecc || data || fdm | oob ecc ||
> > > ...
> > > + *
> > > + * Terefore, it`s necessary to covert data when read/write in
> > > MTD_OPS_RAW.
> >
> > it is ... convert ... reading/writing in raw mode.
> >
> > > + * These data include bad mark, sector data, fdm data and oob ecc.
> > > + */
> > > +static void mtk_ecc_data_format(struct nand_device *nand,
> > > + u8 *databuf, u8 *oobbuf, bool write)
> > > +{
> > > + struct mtk_ecc_engine *eng = nand->ecc.ctx.priv;
> > > + int step_size = nand->ecc.ctx.conf.step_size;
> > > + int i;
> > > +
> > > + if (write) {
> > > + for (i = 0; i < eng->nsteps; i++) {
> > > + if (i == eng->bad_mark.sec)
> > > + eng->bad_mark.bm_swap(nand,
> > > + databuf, oobbuf);
> > > + memcpy(mtk_ecc_sec_ptr(nand, i),
> > > + databuf + data_off(nand, i),
> > > step_size);
> > > +
> > > + memcpy(mtk_ecc_fdm_ptr(nand, i),
> > > + oobbuf + fdm_off(nand, i),
> > > + eng->fdm_size);
> > > +
> > > + memcpy(mtk_ecc_fdm_ptr(nand, i) + eng-
> > > >fdm_size,
> > > + oobbuf + eng->fdm_size * eng->nsteps
> > > +
> > > + i * eng->oob_ecc,
> > > + eng->oob_ecc);
> > > +
> > > + /* swap back when write */
> > > + if (i == eng->bad_mark.sec)
> > > + eng->bad_mark.bm_swap(nand,
> > > + databuf, oobbuf);
> > > + }
> > > + } else {
> > > + for (i = 0; i < eng->nsteps; i++) {
> > > + memcpy(databuf + data_off(nand, i),
> > > + mtk_ecc_sec_ptr(nand, i),
> > > step_size);
> > > +
> > > + memcpy(oobbuf + fdm_off(nand, i),
> > > + mtk_ecc_sec_ptr(nand, i) +
> > > step_size,
> > > + eng->fdm_size);
> > > +
> > > + memcpy(oobbuf + eng->fdm_size * eng->nsteps +
> > > + i * eng->oob_ecc,
> > > + mtk_ecc_sec_ptr(nand, i) + step_size
> > > + + eng->fdm_size,
> > > + eng->oob_ecc);
> > > +
> > > + if (i == eng->bad_mark.sec)
> > > + eng->bad_mark.bm_swap(nand,
> > > + databuf, oobbuf);
> > > + }
> > > + }
> > > +}
> > > +
> > > +static void mtk_ecc_fdm_shift(struct nand_device *nand,
> > > + u8 *dst_buf, u8 *src_buf)
> > > +{
> > > + struct mtk_ecc_engine *eng = nand->ecc.ctx.priv;
> > > + u8 *poi;
> > > + int i;
> > > +
> > > + for (i = 0; i < eng->nsteps; i++) {
> > > + if (i < eng->bad_mark.sec)
> > > + poi = src_buf + (i + 1) * eng->fdm_size;
> > > + else if (i == eng->bad_mark.sec)
> > > + poi = src_buf;
> > > + else
> > > + poi = src_buf + i * eng->fdm_size;
> > > +
> > > + memcpy(dst_buf + i * eng->fdm_size, poi, eng-
> > > >fdm_size);
> > > + }
> > > +}
> > > +
> > > +int mtk_ecc_prepare_io_req_pipelined(struct nand_device *nand,
> > > + struct nand_page_io_req *req)
> > > +{
> > > + struct mtk_ecc_engine *eng = nand->ecc.ctx.priv;
> > > + struct mtd_info *mtd = nanddev_to_mtd(nand);
> > > + u8 *buf = eng->page_buf;
> > > +
> > > + nand_ecc_tweak_req(&eng->req_ctx, req);
> > > +
> > > + if (req->mode == MTD_OPS_RAW) {
> > > + if (req->type == NAND_PAGE_WRITE) {
> > > + /* change data/oob buf to MTK HW ECC data
> > > format */
> > > + mtk_ecc_data_format(nand, req->databuf.in,
> > > + req->oobbuf.in, true);
> > > + req->databuf.out = buf;
> > > + req->oobbuf.out = buf + nand->memorg.pagesize;
> > > + }
> > > + } else {
> > > + eng->ecc_cfg.op = ECC_DECODE;
> > > + if (req->type == NAND_PAGE_WRITE) {
> > > + memset(eng->oob_buf, 0xff, nand-
> > > >memorg.oobsize);
> > > + mtd_ooblayout_set_databytes(mtd, req-
> > > >oobbuf.out,
> > > + eng->oob_buf,
> > > + req->ooboffs,
> > > + mtd->oobavail);
> > > + eng->bad_mark.bm_swap(nand,
> > > + req->databuf.in, eng-
> > > >oob_buf);
> > > + mtk_ecc_fdm_shift(nand, req->oobbuf.in,
> > > + eng->oob_buf);
> > > +
> > > + eng->ecc_cfg.op = ECC_ENCODE;
> > > + }
> > > +
> > > + eng->ecc_cfg.mode = ECC_NFI_MODE;
> > > + eng->ecc_cfg.sectors = eng->nsteps;
> > > + return mtk_ecc_enable(eng->ecc, &eng->ecc_cfg);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int mtk_ecc_finish_io_req_pipelined(struct nand_device *nand,
> > > + struct nand_page_io_req *req)
> > > +{
> > > + struct mtk_ecc_engine *eng = nand->ecc.ctx.priv;
> > > + struct mtd_info *mtd = nanddev_to_mtd(nand);
> > > + struct mtk_ecc_stats stats;
> > > + u8 *buf = eng->page_buf;
> > > + int ret;
> > > +
> > > + if (req->type == NAND_PAGE_WRITE) {
> > > + if (req->mode != MTD_OPS_RAW) {
> > > + mtk_ecc_disable(eng->ecc);
> > > + mtk_ecc_fdm_shift(nand, eng->oob_buf,
> > > + req->oobbuf.in);
> > > + eng->bad_mark.bm_swap(nand,
> > > + req->databuf.in, eng->oob_buf);
> > > + }
> > > + nand_ecc_restore_req(&eng->req_ctx, req);
> > > +
> > > + return 0;
> > > + }
> > > +
> > > + if (req->mode == MTD_OPS_RAW) {
> > > + memcpy(buf, req->databuf.in,
> > > + nand->memorg.pagesize);
> > > + memcpy(buf + nand->memorg.pagesize,
> > > + req->oobbuf.in, nand->memorg.oobsize);
> > > +
> > > + /* change MTK HW ECC data format to data/oob buf */
> > > + mtk_ecc_data_format(nand, req->databuf.in,
> > > + req->oobbuf.in, false);
> > > + nand_ecc_restore_req(&eng->req_ctx, req);
> > > +
> > > + return 0;
> > > + }
> > > +
> > > + ret = mtk_ecc_wait_done(eng->ecc, ECC_DECODE);
> > > + if (ret)
> > > + return -ETIMEDOUT;
> >
> > You should go to an error path and disable the engine.
> > >
>
> I Will fix this and other coding style issue at next iteration.
>
> Thanks
> Xiangsheng Hou

Thanks,
Miquèl