[PATCH 0/2] Add optional mtd nand alignment for subpage reading

[PATCH 0/2] Add optional mtd nand alignment for subpage reading

[Charles Manning]

Subpage reading already provides a certain degree of alignment in that it
aligns the reads to 16-bits if the bus is 16-bits wide.

For some situations this is not enough. For eample, the OMAP2 prefetch
engine only works with u32 aligned buffers and read sizes.

This patch set adds a mechanism to plug in an alignment function and
provides an example using the omap2 nand driver.

Comments welcome

-- Charles

[PATCH 1/2] mtd: Add nand_base feature to align subpage read buffers

[Charles Manning]

Some nand controllers (eg. omap2) need to have their buffers u32 aligned
to use high speed transfer mechanisms.

This commit provides a way to plug in an alignment function and provides a
32-bit algnment function.

Signed-off-by: Charles Manning <cdhmanning@gmail.com>
---
 drivers/mtd/nand/nand_base.c |   22 +++++++++++++++++++++-
 include/linux/mtd/nand.h     |    5 +++--
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 1f75a1b..e8c2432 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1156,6 +1156,22 @@ static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip,
 }
 
 /**
+ * nand_align_subpage32 - function to align subpage read to 32-bits
+ * @mtd:	mtd info structure
+ * @buf:	pointer to pointer of buffer that needs to be aligned
+ * @len:	pointer to length that needs to be aligned.
+ */
+
+void nand_align_subpage32(uint8_t **buf, int *len)
+{
+	int diff = (int)(*buf) & 3;
+	*buf =  *buf - diff;
+	*len = *len + diff;
+	*len = (*len + 3) & ~3;
+}
+EXPORT_SYMBOL(nand_align_subpage32);
+
+/**
  * nand_read_subpage - [REPLACABLE] software ecc based sub-page read function
  * @mtd:	mtd info structure
  * @chip:	nand chip info structure
@@ -1169,6 +1185,7 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 	int start_step, end_step, num_steps;
 	uint32_t *eccpos = chip->ecc.layout->eccpos;
 	uint8_t *p;
+	uint8_t *b;
 	int data_col_addr, i, gaps = 0;
 	int datafrag_len, eccfrag_len, aligned_len, aligned_pos;
 	int busw = (chip->options & NAND_BUSWIDTH_16) ? 2 : 1;
@@ -1222,7 +1239,10 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 
 		chip->cmdfunc(mtd, NAND_CMD_RNDOUT,
 					mtd->writesize + aligned_pos, -1);
-		chip->read_buf(mtd, &chip->oob_poi[aligned_pos], aligned_len);
+		b =  &chip->oob_poi[aligned_pos];
+		if(chip->align_subpage)
+			chip->align_subpage(&b, &aligned_len);
+		chip->read_buf(mtd, b, aligned_len);
 	}
 
 	for (i = 0; i < eccfrag_len; i++)
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 63e17d0..6ea6177 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -474,6 +474,7 @@ struct nand_buffers {
  *			additional error status checks (determine if errors are
  *			correctable).
  * @write_page:		[REPLACEABLE] High-level page write function
+ * @align_subpage:	[OPTIONAL] Aligns subpage read buffer.
  */
 
 struct nand_chip {
@@ -501,7 +502,7 @@ struct nand_chip {
 			int status, int page);
 	int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip,
 			const uint8_t *buf, int page, int cached, int raw);
-
+	void (*align_subpage)(uint8_t **buf, int *len);
 	int chip_delay;
 	unsigned int options;
 
@@ -597,7 +598,7 @@ extern int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 			   int allowbbt);
 extern int nand_do_read(struct mtd_info *mtd, loff_t from, size_t len,
 			size_t *retlen, uint8_t *buf);
-
+extern void nand_align_subpage32(uint8_t **buf, int *len);
 /**
  * struct platform_nand_chip - chip level device structure
  * @nr_chips:		max. number of chips to scan for
-- 
1.7.1.5.g49342

Re: [PATCH 1/2] mtd: Add nand_base feature to align subpage read buffers

[Artem Bityutskiy]

Hi,

would it please be possible improve comments and the commit message and
make them explain better which problem the patch solves, what is the
problem with the current code, how exactly you solve the problem, and
why your solution is the best among other alternative solutions?

I think the below commit messages is too short and does not explain
this.

On Thu, 2011-01-06 at 14:39 +1300, Charles Manning wrote:
> Some nand controllers (eg. omap2) need to have their buffers u32 aligned
> to use high speed transfer mechanisms.
> 
> This commit provides a way to plug in an alignment function and provides a
> 32-bit algnment function.
> 
> Signed-off-by: Charles Manning <cdhmanning@gmail.com>
> ---
>  drivers/mtd/nand/nand_base.c |   22 +++++++++++++++++++++-
>  include/linux/mtd/nand.h     |    5 +++--
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 1f75a1b..e8c2432 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1156,6 +1156,22 @@ static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip,
>  }
>  
>  /**
> + * nand_align_subpage32 - function to align subpage read to 32-bits
> + * @mtd:	mtd info structure
> + * @buf:	pointer to pointer of buffer that needs to be aligned
> + * @len:	pointer to length that needs to be aligned.
> + */
> +
Why blank line here?
> +void nand_align_subpage32(uint8_t **buf, int *len)
> +{
> +	int diff = (int)(*buf) & 3;
> +	*buf =  *buf - diff;
It is really not obvious that this buffer does not come from user-space,
but it is some internal chip->buffers->databuf, so you actually can step
back. Cold you please put some comment about this?
> +	*len = *len + diff;
> +	*len = (*len + 3) & ~3;
> +}
> +EXPORT_SYMBOL(nand_align_subpage32);

In general, do you think it is a good thing that MTD NAND core always
reads to internal buffer first, then copies the data to the user buffer?
To me it looks like bad idea because we end up with unwanted overhead.
There is a benefit - if you read the same NAND page twice, you get the
data from the buffer the second time. But this does not work with
sub-pages, and I think this MTD core is wrong place for this. If someone
wants caching, he could use the Linux page cache as a layer on top of
MTD core, instead of this castrated one NAND page cache.

Anyway, what I want to say is that the current architecture is bad, IMO.
Namely, this "always read to own buffer" thing is bad. But your patch
relays on this, so if some one some day wants to improve MTD NAND core
in this respect, he'll end up with more job.

Is it possible to invent some solution which does not rely on the fact
that you deal with an internal buffer, but instead, assumes that you may
be dealing with a user buffer?

> +
> +/**
>   * nand_read_subpage - [REPLACABLE] software ecc based sub-page read function
>   * @mtd:	mtd info structure
>   * @chip:	nand chip info structure
> @@ -1169,6 +1185,7 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
>  	int start_step, end_step, num_steps;
>  	uint32_t *eccpos = chip->ecc.layout->eccpos;
>  	uint8_t *p;
> +	uint8_t *b;

The current style prefers
uint8_t *p, *b;
for some reasons.
>  	int data_col_addr, i, gaps = 0;
>  	int datafrag_len, eccfrag_len, aligned_len, aligned_pos;
>  	int busw = (chip->options & NAND_BUSWIDTH_16) ? 2 : 1;
> @@ -1222,7 +1239,10 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
>  
>  		chip->cmdfunc(mtd, NAND_CMD_RNDOUT,
>  					mtd->writesize + aligned_pos, -1);
> -		chip->read_buf(mtd, &chip->oob_poi[aligned_pos], aligned_len);
> +		b =  &chip->oob_poi[aligned_pos];
> +		if(chip->align_subpage)
> +			chip->align_subpage(&b, &aligned_len);
> +		chip->read_buf(mtd, b, aligned_len);
>  	}
>  
>  	for (i = 0; i < eccfrag_len; i++)
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 63e17d0..6ea6177 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -474,6 +474,7 @@ struct nand_buffers {
>   *			additional error status checks (determine if errors are
>   *			correctable).
>   * @write_page:		[REPLACEABLE] High-level page write function
> + * @align_subpage:	[OPTIONAL] Aligns subpage read buffer.
>   */

Would it please be possible to add more comments to the code about this
align_subpage stuff, for people who try to read it in the future?

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

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mtd: Add nand_base feature to align subpage read buffers

[Charles Manning]

On Tuesday 18 January 2011 21:13:43 Artem Bityutskiy wrote:
> Hi,
>
> would it please be possible improve comments and the commit message and
> make them explain better which problem the patch solves, what is the
> problem with the current code, how exactly you solve the problem, and
> why your solution is the best among other alternative solutions?
>
> I think the below commit messages is too short and does not explain
> this.

OK. I will resubmit this done a different way. The patch I submitted does not 
work as generally as it should.


>
> On Thu, 2011-01-06 at 14:39 +1300, Charles Manning wrote:
> > Some nand controllers (eg. omap2) need to have their buffers u32 aligned
> > to use high speed transfer mechanisms.
> >
> > This commit provides a way to plug in an alignment function and provides
> > a 32-bit algnment function.
> >
> > Signed-off-by: Charles Manning <cdhmanning@gmail.com>
> > ---
> >  drivers/mtd/nand/nand_base.c |   22 +++++++++++++++++++++-
> >  include/linux/mtd/nand.h     |    5 +++--
> >  2 files changed, 24 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 1f75a1b..e8c2432 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -1156,6 +1156,22 @@ static int nand_read_page_swecc(struct mtd_info
> > *mtd, struct nand_chip *chip, }
> >
> >  /**
> > + * nand_align_subpage32 - function to align subpage read to 32-bits
> > + * @mtd:	mtd info structure
> > + * @buf:	pointer to pointer of buffer that needs to be aligned
> > + * @len:	pointer to length that needs to be aligned.
> > + */
> > +
>
> Why blank line here?
>
> > +void nand_align_subpage32(uint8_t **buf, int *len)
> > +{
> > +	int diff = (int)(*buf) & 3;
> > +	*buf =  *buf - diff;
>
> It is really not obvious that this buffer does not come from user-space,
> but it is some internal chip->buffers->databuf, so you actually can step
> back. Cold you please put some comment about this?
>
> > +	*len = *len + diff;
> > +	*len = (*len + 3) & ~3;
> > +}
> > +EXPORT_SYMBOL(nand_align_subpage32);
>
> In general, do you think it is a good thing that MTD NAND core always
> reads to internal buffer first, then copies the data to the user buffer?
> To me it looks like bad idea because we end up with unwanted overhead.
> There is a benefit - if you read the same NAND page twice, you get the
> data from the buffer the second time. But this does not work with
> sub-pages, and I think this MTD core is wrong place for this. If someone
> wants caching, he could use the Linux page cache as a layer on top of
> MTD core, instead of this castrated one NAND page cache.
>
> Anyway, what I want to say is that the current architecture is bad, IMO.
> Namely, this "always read to own buffer" thing is bad. But your patch
> relays on this, so if some one some day wants to improve MTD NAND core
> in this respect, he'll end up with more job.
>
> Is it possible to invent some solution which does not rely on the fact
> that you deal with an internal buffer, but instead, assumes that you may
> be dealing with a user buffer?
>
> > +
> > +/**
> >   * nand_read_subpage - [REPLACABLE] software ecc based sub-page read
> > function * @mtd:	mtd info structure
> >   * @chip:	nand chip info structure
> > @@ -1169,6 +1185,7 @@ static int nand_read_subpage(struct mtd_info *mtd,
> > struct nand_chip *chip, int start_step, end_step, num_steps;
> >  	uint32_t *eccpos = chip->ecc.layout->eccpos;
> >  	uint8_t *p;
> > +	uint8_t *b;
>
> The current style prefers
> uint8_t *p, *b;
> for some reasons.
>
> >  	int data_col_addr, i, gaps = 0;
> >  	int datafrag_len, eccfrag_len, aligned_len, aligned_pos;
> >  	int busw = (chip->options & NAND_BUSWIDTH_16) ? 2 : 1;
> > @@ -1222,7 +1239,10 @@ static int nand_read_subpage(struct mtd_info *mtd,
> > struct nand_chip *chip,
> >
> >  		chip->cmdfunc(mtd, NAND_CMD_RNDOUT,
> >  					mtd->writesize + aligned_pos, -1);
> > -		chip->read_buf(mtd, &chip->oob_poi[aligned_pos], aligned_len);
> > +		b =  &chip->oob_poi[aligned_pos];
> > +		if(chip->align_subpage)
> > +			chip->align_subpage(&b, &aligned_len);
> > +		chip->read_buf(mtd, b, aligned_len);
> >  	}
> >
> >  	for (i = 0; i < eccfrag_len; i++)
> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > index 63e17d0..6ea6177 100644
> > --- a/include/linux/mtd/nand.h
> > +++ b/include/linux/mtd/nand.h
> > @@ -474,6 +474,7 @@ struct nand_buffers {
> >   *			additional error status checks (determine if errors are
> >   *			correctable).
> >   * @write_page:		[REPLACEABLE] High-level page write function
> > + * @align_subpage:	[OPTIONAL] Aligns subpage read buffer.
> >   */
>
> Would it please be possible to add more comments to the code about this
> align_subpage stuff, for people who try to read it in the future?

[PATCH 2/2] omap2 nand: Align subpage reads.

[Charles Manning]

Set align_subpage to align buffer to u32 to use the prefetch engine.

Signed-off-by: Charles Manning <cdhmanning@gmail.com>
---
 drivers/mtd/nand/omap2.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index cd41c58..3630971 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -886,6 +886,7 @@ static int __devinit omap_nand_probe(struct platform_device *pdev)
 #else
 	info->nand.ecc.mode = NAND_ECC_SOFT;
 #endif
+	info->nand.align_subpage = nand_align_subpage32;
 
 	/* DIP switches on some boards change between 8 and 16 bit
 	 * bus widths for flash.  Try the other width if the first try fails.
-- 
1.7.1.5.g49342