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@xxxxxxxxx>
> > ---
> > 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
