| Message ID | 1294277942-1252-2-git-send-email-cdhmanning@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
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?
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?
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
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(-)