[RFC][patch] NAND partial page read functionality

[Artem Bityutskiy]

On Thu, 2007-12-13 at 18:15 +0000, Alexey Korolev wrote:
> Hi 
> 
> Here is a patch providing partial page read functionality for NAND
> devices. 
> 
> In many cases it gives performacne boost. I've added this feature
> enabling under chip->options flag. 
> Setting NAND_PART_READ option in board driver will enable this feature.
> 
> This is example how partial page read could affect stat time perfromance. 

Hi, this stuff looks nice and useful for me. Few suggestions:

1. Please, use -p option when you generate patches.

2. We already have notion of subpage in nand_base.c. We already can
write subpages. Your patch is about reading subpages, so I suggest you
to do something like s/partial/subpage/. E.g., UBI utilizes sub-page
writes.

3. Run nand-tests to make sure you did not brake anything:
git://git.infradead.org/~ahunter/nand-tests.git

Question: what was the flash you tested this on? Judging from the
comments it was a 512-byte page NAND, right?

==========================
Also, an separate idea I'd suggest you too look at, but after you have
your patch in:

Glance in how nand_base.c does sub-page write. It writes whole NAND page
anyway, but fills the subpages which it does not want to write with
0xFFs. I am not sure, but I suspect it could be smarter and write only
the requested subpage. If this is true, and you will do this, then the
notion of subpage could go away.

Indeed, you could declare mtd->writesize = subpage size then, not NAND
page size. Then, for example, for 512-page-byte NANDs you'd have
256-byte mtd->writesize, and JFFS2 would have 256-byte write-buffer, and
you'd speed up jffs2 writes, because it would write less space when it
synchronizes the write-buffer, and it would waste less space, which
means less Garbage-collection.
==========================

I did not really check the code, but few notes after a quick look:

>  /**
> + * nand_read_partial - [REPLACABLE] software ecc based partial page read function
> + * @mtd:	mtd info structure
> + * @chip:	nand chip info structure
> + * @dataofs	offset of requested data within the page 
> + * @readlen	data length 
> + * @buf:	buffer to store read data
> + */
> +static int nand_read_partial(struct mtd_info * mtd,struct nand_chip *chip, uint32_t data_offs, uint32_t readlen, uint8_t *bufpoi)
> +{
> +	int start_step, end_step, num_steps;
> +	uint32_t *eccpos = chip->ecc.layout->eccpos;
> +	uint8_t *p;
> +	int data_col_addr, i;
> +	int datafrag_len, eccfrag_len;
> +	
> +	/* Column address wihin the page aligned to ECC size (256bytes). */
Why this comment says about 256 bytes. AFAIU, this will be 512 bytes in
case of 2048-byte NAND page.

> +	start_step = data_offs / chip->ecc.size;
> +	end_step = (data_offs + readlen - 1) / chip->ecc.size;
> +	num_steps = end_step - start_step + 1;
> +
> +	/* Data size aligned to ECC ecc.size*/
> +	datafrag_len = num_steps * chip->ecc.size;
> +	eccfrag_len = num_steps * chip->ecc.bytes;
> +
> +	data_col_addr = start_step * chip->ecc.size;
> +	/* If we read not a page aligned data */
> +	if (data_col_addr != 0) {
> +		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_col_addr, -1);
> +	}
Redundant curly brackets

> +
> +	p = bufpoi + data_col_addr;
> +	chip->read_buf(mtd, p, datafrag_len);
> +
> +	/* Calculate  ECC */
> +	for (i = 0; i<eccfrag_len ; i += chip->ecc.bytes, p += chip->ecc.size)
> +		chip->ecc.calculate(mtd, p, &chip->buffers->ecccalc[i]);
> +
> +	/* The performance will be improved more if to position offsets 
> +	 * according to ecc.pos. But in this case we may face issues 
> +	 * on chips with gaps in ecc positions. So it is disabled yet*/
> +	chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize, -1);
> +	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> +	for (i = 0; i < eccfrag_len; i++)
> +		chip->buffers->ecccode[i] = chip->oob_poi[eccpos[i + start_step * chip->ecc.bytes]];
This double array indexing is rather unreadable.
> +
> +	p = bufpoi + data_col_addr;
> +	for (i = 0; i<eccfrag_len ; i += chip->ecc.bytes, p += chip->ecc.size) {
> +		int stat;

...

>  /* Options valid for Samsung large page devices */
>  #define NAND_SAMSUNG_LP_OPTIONS \
> @@ -183,6 +185,8 @@
>  #define NAND_MUST_PAD(chip) (!(chip->options & NAND_NO_PADDING))
>  #define NAND_HAS_CACHEPROG(chip) ((chip->options & NAND_CACHEPRG))
>  #define NAND_HAS_COPYBACK(chip) ((chip->options & NAND_COPYBACK))
> +#define NAND_CANPARTREAD(chip) ((chip->options & NAND_PART_READ) &&\
> +				(chip->ecc.mode == NAND_ECC_SOFT))

Why chip->ecc.mode == NAND_ECC_SOFT?

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)