Hi Paul,
paul@xxxxxxxxxxxxxxx wrote on Fri, 15 Oct 2021 10:38:00 +0100:
> Hi,
>
> Le ven., oct. 15 2021 at 11:35:15 +0200, Miquel Raynal <miquel.raynal@xxxxxxxxxxx> a écrit :
> > Hi Paul,
> >
> >> >> */
> >> >> >> >> /* An ECC layout for using 4-bit ECC with small-page >> flash, >> storing
> >> >> >> @@ -648,7 +580,7 @@ static int >> davinci_nand_attach_chip(struct >> >> nand_chip *chip)
> >> >> >> } else if (chunks == 4 || chunks == 8) {
> >> >> >> mtd_set_ooblayout(mtd,
> >> >> >> nand_get_large_page_ooblayout());
> >> >> >> - chip->ecc.read_page = >> >> nand_davinci_read_page_hwecc_oob_first;
> >> >> >> + chip->ecc.read_page = nand_read_page_hwecc_oob_first;
> >> >> >> } else {
> >> >> >> return -EIO;
> >> >> >> }
> >> >> >> diff --git a/drivers/mtd/nand/raw/nand_base.c >> >> >> b/drivers/mtd/nand/raw/nand_base.c
> >> >> >> index 3d6c6e880520..cb5f343b9fa2 100644
> >> >> >> --- a/drivers/mtd/nand/raw/nand_base.c
> >> >> >> +++ b/drivers/mtd/nand/raw/nand_base.c
> >> >> >> @@ -3160,6 +3160,75 @@ static int >> nand_read_page_hwecc(struct >> >> nand_chip *chip, uint8_t *buf,
> >> >> >> return max_bitflips;
> >> >> >> }
> >> >> >> >> +/**
> >> >> >> + * nand_read_page_hwecc_oob_first - Hardware ECC page read >> >> with ECC
> >> >> >> + * data read from OOB area
> >> >> >> + * @chip: nand chip info structure
> >> >> >> + * @buf: buffer to store read data
> >> >> >> + * @oob_required: caller requires OOB data read to >> >> chip->oob_poi
> >> >> >> + * @page: page number to read
> >> >> >> + *
> >> >> >> + * Hardware ECC for large page chips, require OOB to be >> read >> >> first. For this
> >> >> >
> >> >> > requires
> >> >> >
> >> >> > With this ECC configuration?
> >> >> >
> >> >> >> + * ECC mode, the write_page method is re-used from ECC_HW. >> >> These >> methods
> >> >> >
> >> >> > I do not understand this sentence nor the next one about >> >> syndrome. I
> >> >> > believe it is related to your engine and should not leak into >> the >> > core.
> >> >> >
> >> >> >> + * read/write ECC from the OOB area, unlike the >> >> ECC_HW_SYNDROME >> support with
> >> >> >> + * multiple ECC steps, follows the "infix ECC" scheme and >> >> >> reads/writes ECC from
> >> >> >> + * the data area, by overwriting the NAND manufacturer bad >> >> block >> markings.
> >> >> >
> >> >> > That's a sentence I don't like. What do you mean exactly?
> >> >> >
> >> >> > What "Infix ECC" scheme is?
> >> >> >
> >> >> > Do you mean that unlike the syndrome mode it *does not* >> >> overwrite the
> >> >> > BBM ?
> >> >> >> I don't mean anything. I did not write that comment. I just >> moved >> the function verbatim with no changes. If something needs >> to be >> fixed, then it needs to be fixed before/after this patch.
> >> >
> >> > Well, this comment should be adapted because as-is I don't think >> it's
> >> > wise to move it around.
> >> >> OK.
> >> >> I think it says that BBM can be overwritten with this >> configuration, but that would be if the OOB layout covers the BBM >> area.
> >
> > If the ooblayout prevents the BBM to be smatched I'm fine and this
> > sentence should disappear because it's misleading.
> >
> >> >> >> >> + */
> >> >> >> +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, >> >> uint8_t >> *buf,
> >> >> >> + int oob_required, int page)
> >> >> >> +{
> >> >> >> + struct mtd_info *mtd = nand_to_mtd(chip);
> >> >> >> + int i, eccsize = chip->ecc.size, ret;
> >> >> >> + int eccbytes = chip->ecc.bytes;
> >> >> >> + int eccsteps = chip->ecc.steps;
> >> >> >> + uint8_t *p = buf;
> >> >> >> + uint8_t *ecc_code = chip->ecc.code_buf;
> >> >> >> + unsigned int max_bitflips = 0;
> >> >> >> +
> >> >> >> + /* Read the OOB area first */
> >> >> >> + ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, >> >> >> mtd->oobsize);
> >> >> >> + if (ret)
> >> >> >> + return ret;
> >> >> >> +
> >> >> >> + ret = nand_read_page_op(chip, page, 0, NULL, 0);
> >> >> >
> >> >> > Definitely not, your are requesting the chip to do the >> read_page
> >> >> > operation twice. You only need a nand_change_read_column I >> >> believe.
> >> >> >> Again, this code is just being moved around - don't shoot >> the >> messenger :)
> >> >
> >> > haha
> >> >
> >> > Well, now you touch the core, so I need to be more careful, and >> the
> >> > code is definitely wrong, so even if we don't move that code off, >> you
> >> > definitely want to fix it in order to improve your performances.
> >> >> I don't see the read_page being done twice?
> >> >> There's one read_oob, one read_page, then read_data in the loop.
> >
> > read_oob and read_page both end up sending READ0 and READSTART so
> > they do request the chip to perform an internal read twice. You
> > need this only once. The call to nand_read_page_op() should be a
> > nand_change_read_column() with no data requested.
>
> OK.
>
> >
> >> >> >> /**
> >> >> >> * nand_read_page_syndrome - [REPLACEABLE] hardware ECC >> >> syndrome >> based page read
> >> >> >> * @chip: nand chip info structure
> >> >> >> diff --git a/include/linux/mtd/rawnand.h >> >> >> b/include/linux/mtd/rawnand.h
> >> >> >> index b2f9dd3cbd69..5b88cd51fadb 100644
> >> >> >> --- a/include/linux/mtd/rawnand.h
> >> >> >> +++ b/include/linux/mtd/rawnand.h
> >> >> >> @@ -1539,6 +1539,8 @@ int nand_read_data_op(struct >> nand_chip >> *chip, >> void *buf, unsigned int len,
> >> >> >> bool force_8bit, bool check_only);
> >> >> >> int nand_write_data_op(struct nand_chip *chip, const void >> *buf,
> >> >> >> unsigned int len, bool force_8bit);
> >> >> >> +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, >> >> uint8_t >> *buf,
> >> >> >> + int oob_required, int page);
> >> >> >
> >> >> > You certainly want to add this symbol closer to the other >> >> read/write
> >> >> > page helpers?
> >> >> >> Where would that be? The other read/write page helpers are >> all >> "static" so they don't appear in any header.
> >> >
> >> > I believe we should keep this header local as long as there are no
> >> > other users.
> >> >> I'll move it to internal.h then.
> >
> > Why do you want to put it there is there is only one user?
>
> But there are two users: davinci_nand.c and (with patch [3/3]) ingenic/ingenic_nand_drv.c.
Oh right I missed that :)
Then please add two preparation patches which:
- fixes the comment (please reword it completely)
- avoid the double reading
And keep the location where you moved it (including the header) as-is.
Thanks,
Miquèl
