Re: [PATCH 2/3] mtd: rawnand: Export nand_read_page_hwecc_oob_first()

[Paul Cercueil <paul@crapouillou.net>]

Hi,

Le ven., oct. 15 2021 at 11:35:15 +0200, Miquel Raynal 
<miquel.raynal@bootlin.com> 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.

-Paul