On Fri, 20 Oct 2017 11:29:18 +0200
Stefan Agner <stefan@xxxxxxxx> wrote:
> Hi Miquel,
>
> Thanks for the work on this, happy to see the new interface is moving
> forward. Some comments below.
>
> On 2017-10-18 16:36, Miquel Raynal wrote:
> > From: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> >
> > The core currently send the READ0 and SEQIN+PAGEPROG commands in
> > nand_do_read/write_ops(). This is inconsistent with
> > ->read/write_oob[_raw]() hooks behavior which are expected to send
> > these commands.
> >
> > There's already a flag (NAND_ECC_CUSTOM_PAGE_ACCESS) to inform the core
> > that a specific controller wants to send the READ/SEQIN+PAGEPROG
> > commands on its own, but it's an opt-in flag, and existing drivers are
> > unlikely to be updated to pass it.
> >
> > Moreover, some controllers cannot dissociate the READ/PAGEPROG commands
> > from the associated data transfer and ECC engine activation, and
> > developers have to hack things in their ->cmdfunc() implementation to
> > handle such complex cases, or have to accept the perf penalty of sending
> > twice the same command.
> > To address this problem we are planning on adding a new interface which
> > is passed all information about a NAND operation (including the amount
> > of data to transfer) and replacing all calls to ->cmdfunc() to calls to
> > this new ->exec_op() hook. But, in order to do that, we need to have all
> > ->cmdfunc() calls placed near their associated ->read/write_buf/byte()
> > calls.
> >
> > Modify the core and relevant drivers to make NAND_ECC_CUSTOM_PAGE_ACCESS
> > the default case, and remove this flag.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > [miquel.raynal@xxxxxxxxxxxxxxxxxx: rebased and fixed some conflicts]
> > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/mtd/nand/atmel/nand-controller.c | 7 ++-
> > drivers/mtd/nand/bf5xx_nand.c | 6 ++-
> > drivers/mtd/nand/brcmnand/brcmnand.c | 13 +++--
> > drivers/mtd/nand/cafe_nand.c | 6 +--
> > drivers/mtd/nand/denali.c | 14 +++++-
> > drivers/mtd/nand/docg4.c | 12 +++--
> > drivers/mtd/nand/fsl_elbc_nand.c | 6 +--
> > drivers/mtd/nand/fsl_ifc_nand.c | 6 +--
> > drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 30 ++++++------
> > drivers/mtd/nand/hisi504_nand.c | 6 +--
> > drivers/mtd/nand/lpc32xx_mlc.c | 5 +-
> > drivers/mtd/nand/lpc32xx_slc.c | 11 +++--
> > drivers/mtd/nand/mtk_nand.c | 10 ++--
> > drivers/mtd/nand/nand_base.c | 68 +++++++++------------------
> > drivers/mtd/nand/nand_micron.c | 1 -
> > drivers/mtd/nand/sh_flctl.c | 6 +--
> > drivers/mtd/nand/sunxi_nand.c | 34 +++++++++-----
> > drivers/mtd/nand/tango_nand.c | 1 -
> > drivers/mtd/nand/vf610_nfc.c | 6 +--
> > drivers/staging/mt29f_spinand/mt29f_spinand.c | 7 ++-
> > include/linux/mtd/rawnand.h | 11 -----
> > 21 files changed, 142 insertions(+), 124 deletions(-)
> >
>
> <snip>
>
> > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > index c8ceaecd8065..3f2b903158c1 100644
> > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > @@ -1043,6 +1043,8 @@ static int gpmi_ecc_read_page(struct mtd_info
> > *mtd, struct nand_chip *chip,
> > unsigned int max_bitflips = 0;
> > int ret;
> >
> > + nand_read_page_op(chip, page, 0, NULL, 0);
> > +
> > dev_dbg(this->dev, "page number is : %d\n", page);
> > ret = read_page_prepare(this, buf, nfc_geo->payload_size,
> > this->payload_virt, this->payload_phys,
> > @@ -1220,12 +1222,13 @@ static int gpmi_ecc_read_subpage(struct
> > mtd_info *mtd, struct nand_chip *chip,
> > meta = geo->metadata_size;
> > if (first) {
> > col = meta + (size + ecc_parity_size) * first;
> > - chip->cmdfunc(mtd, NAND_CMD_RNDOUT, col, -1);
>
> Shouldn't we add nand_change_read_column_op here?
>
>
> >
> > meta = 0;
> > buf = buf + first * size;
> > }
> >
> > + nand_read_page_op(chip, page, col, NULL, 0);
> > +
> > /* Save the old environment */
> > r1_old = r1_new = readl(bch_regs + HW_BCH_FLASH0LAYOUT0);
> > r2_old = r2_new = readl(bch_regs + HW_BCH_FLASH0LAYOUT1);
> > @@ -1277,6 +1280,9 @@ static int gpmi_ecc_write_page(struct mtd_info
> > *mtd, struct nand_chip *chip,
> > int ret;
> >
> > dev_dbg(this->dev, "ecc write page.\n");
> > +
> > + nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> > +
> > if (this->swap_block_mark) {
> > /*
> > * If control arrives here, we're doing block mark swapping.
> > @@ -1338,7 +1344,10 @@ static int gpmi_ecc_write_page(struct mtd_info
> > *mtd, struct nand_chip *chip,
> > payload_virt, payload_phys);
> > }
> >
> > - return 0;
> > + if (ret)
> > + return ret;
> > +
> > + return nand_prog_page_end_op(chip);
> > }
> >
> > /*
> > @@ -1472,8 +1481,8 @@ static int gpmi_ecc_read_page_raw(struct mtd_info *mtd,
> > uint8_t *oob = chip->oob_poi;
> > int step;
> >
> > - chip->read_buf(mtd, tmp_buf,
> > - mtd->writesize + mtd->oobsize);
> > + nand_read_page_op(chip, page, 0, tmp_buf,
> > + mtd->writesize + mtd->oobsize);
> >
> > /*
> > * If required, swap the bad block marker and the data stored in the
> > @@ -1617,24 +1626,19 @@ static int gpmi_ecc_write_page_raw(struct mtd_info *mtd,
> > tmp_buf[mtd->writesize] = swap;
> > }
> >
> > - chip->write_buf(mtd, tmp_buf, mtd->writesize + mtd->oobsize);
> > -
> > - return 0;
> > + return nand_prog_page_op(chip, page, 0, tmp_buf,
> > + mtd->writesize + mtd->oobsize);
> > }
> >
> > static int gpmi_ecc_read_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
> > int page)
> > {
> > - nand_read_page_op(chip, page, 0, NULL, 0);
> > -
> > return gpmi_ecc_read_page_raw(mtd, chip, NULL, 1, page);
> > }
> >
> > static int gpmi_ecc_write_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
> > int page)
> > {
> > - nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> > -
> > return gpmi_ecc_write_page_raw(mtd, chip, NULL, 1, page);
> > }
> >
> > @@ -1806,9 +1810,7 @@ static int mx23_write_transcription_stamp(struct
> > gpmi_nand_data *this)
> > /* Write the first page of the current stride. */
> > dev_dbg(dev, "Writing an NCB fingerprint in page 0x%x\n", page);
> >
> > - nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> > - chip->ecc.write_page_raw(mtd, chip, buffer, 0, page);
> > - status = nand_prog_page_end_op(chip);
> > + status = chip->ecc.write_page_raw(mtd, chip, buffer, 0, page);
> > if (status)
> > dev_err(dev, "[%s] Write failed.\n", __func__);
> > }
>
> <snip>
>
> > diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> > index 8037d4b48a05..80d31a58e558 100644
> > --- a/drivers/mtd/nand/vf610_nfc.c
> > +++ b/drivers/mtd/nand/vf610_nfc.c
> > @@ -560,7 +560,7 @@ static int vf610_nfc_read_page(struct mtd_info
> > *mtd, struct nand_chip *chip,
> > int eccsize = chip->ecc.size;
> > int stat;
> >
> > - vf610_nfc_read_buf(mtd, buf, eccsize);
> > + nand_read_page_op(chip, page, 0, buf, eccsize);
> > if (oob_required)
> > vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
> >
> > @@ -580,7 +580,7 @@ static int vf610_nfc_write_page(struct mtd_info
> > *mtd, struct nand_chip *chip,
> > {
> > struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> >
> > - vf610_nfc_write_buf(mtd, buf, mtd->writesize);
>
> We currently ignore NAND_CMD_SEQIN in ->cmdfunc anyway, so I think this
> change would not even be necessary here.
Well, the thing is, we're trying to keep the logic unchanged. Even if
for your driver ->cmdfunc(NAND_CMD_SEQIN) is a NOP, I'd prefer to be
consistent and still replace this ->write_buf() call by a
nand_prog_page_begin_op() one.
>
> This is a NAND controller which will benefit from the new interface. I
> plan to convert the driver to the new interface, maybe I manage to do
> that soon so we have a second driver making use of the new interface.
Great!!! Let us know if you need any help.
Regards,
Boris
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
