Re: [PATCH v3 2/3] omap3 nand: cleanup virtual address usages
* Vimal Singh [100519 11:02]:
> On Wed, May 19, 2010 at 10:54 PM, Ghorai, Sukumar wrote:
> >> > /* take care of subpage reads */
> >> > for (; len % 4 != 0; ) {
> >> > *buf++ = __raw_readb(info->nand.IO_ADDR_R);
> >> > len--;
> >> > }
> >> > - p = (u32 *) buf;
> >>
> >> Above code had an issue, which was fixed by this commit:
> >> http://git.infradead.org/mtd-
> >> 2.6.git/commitdiff/c3341d0ceb4de1680572024f50233403c6a8b10d
> >>
> >> I would suggest you to prepare your patch on MTD tree.
> > [Ghorai] Patches started posting on lo. And lets continue the same.
>
> Not sure about this. Its your/Tony's call.
I'd assume that fix is on it's way to the mainline kernel,
looks like a trivial rebase after the merge window is over.
Anyways, this will not make it this merge window, we're
out of time.
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] omap3 nand: cleanup virtual address usages
On Wed, May 19, 2010 at 10:54 PM, Ghorai, Sukumar wrote:
>> > /* take care of subpage reads */
>> > for (; len % 4 != 0; ) {
>> > *buf++ = __raw_readb(info->nand.IO_ADDR_R);
>> > len--;
>> > }
>> > - p = (u32 *) buf;
>>
>> Above code had an issue, which was fixed by this commit:
>> http://git.infradead.org/mtd-
>> 2.6.git/commitdiff/c3341d0ceb4de1680572024f50233403c6a8b10d
>>
>> I would suggest you to prepare your patch on MTD tree.
> [Ghorai] Patches started posting on lo. And lets continue the same.
Not sure about this. Its your/Tony's call.
>>
>> >
>> > /* configure and start prefetch transfer */
>> > ret = gpmc_prefetch_enable(info->gpmc_cs, 0x0, len, 0x0);
>> > @@ -307,17 +265,18 @@ static void omap_read_buf_pref(struct mtd_info
>> *mtd, u_char *buf, int len)
>> > else
>> > omap_read_buf8(mtd, buf, len);
>> > } else {
>> > + p = (u32 *) buf;
>> > do {
>> > - pfpw_status = gpmc_prefetch_status();
>> > - r_count = ((pfpw_status >> 24) & 0x7F) >> 2;
>> > - ioread32_rep(info->nand_pref_fifo_add, p,
>> r_count);
>> > + gpmc_hwcontrol(info->gpmc_cs,
>> > + GPMC_PREFETCH_FIFO_CNT, 0, 0, &r_count);
>> > + r_count = r_count >> 2;
>> > + ioread32_rep(info->nand.IO_ADDR_R, p, r_count);
>> > p += r_count;
>> > - len -= r_count << 2;
>> > + len -= (r_count << 2);
>>
>> Braces are not required here.
> [Ghorai] thanks
>>
>> > } while (len);
>> > -
>>
>> After call to 'gpmc_prefetch_enable', next line are:
>> if (ret) {
>> /* PFPW engine is busy, use cpu copy method */
>> if (info->nand.options & NAND_BUSWIDTH_16)
>> ...
>> ...
>> > - /* disable and stop the PFPW engine */
>> > - gpmc_prefetch_reset(info->gpmc_cs);
>> > }
>>
>> So, if above 'if' fails, driver will not get prefetch engine (it was
>> already busy). Then it doesn't makes sense to call for __reset__.
> [Ghorai] I will take this clean up as 4th patch. As its not matching with
> patch description.
>>
>> > + /* disable and stop the PFPW engine */
>> > + gpmc_prefetch_reset(info->gpmc_cs);
>>
>> (Also see my comments on your other patch.)
> [Ghorai] Agree and I will take this kind of cleanup as 4th patch
>>
>> > }
>> >
>> > /**
>> > @@ -331,13 +290,13 @@ static void omap_write_buf_pref(struct mtd_info
>> *mtd,
>> > {
>> > struct omap_nand_info *info = container_of(mtd,
>> > struct omap_nand_info,
>> mtd);
>> > - uint32_t pfpw_status = 0, w_count = 0;
>> > + uint32_t pref_count = 0, w_count = 0;
>> > int i = 0, ret = 0;
>> > - u16 *p = (u16 *) buf;
>> > + u16 *p;
>> >
>> > /* take care of subpage writes */
>> > if (len % 2 != 0) {
>> > - writeb(*buf, info->nand.IO_ADDR_R);
>> > + writeb(*buf, info->nand.IO_ADDR_W);
>> > p = (u16 *)(buf + 1);
>> > len--;
>> > }
>> > @@ -351,17 +310,22 @@ static void omap_write_buf_pref(struct mtd_info
>> *mtd,
>> > else
>> > omap_write_buf8(mtd, buf, len);
>> > } else {
>> > - pfpw_status = gpmc_prefetch_status();
>> > - while (pfpw_status & 0x3FFF) {
>> > - w_count = ((pfpw_status >> 24) & 0x7F) >> 1;
>> > + p = (u16 *) buf;
>> > + while (len) {
>> > + gpmc_hwcontrol(info->gpmc_cs,
>> > + GPMC_PREFETCH_FIFO_CNT, 0, 0,
>> &w_count);
>> > + w_count = w_count >> 1;
>> > for (i = 0; (i < w_count) && len; i++, len -= 2)
>> > - iowrite16(*p++, info-
>> >nand_pref_fifo_add);
>> > - pfpw_status = gpmc_prefetch_status();
>> > + iowrite16(*p++, info->nand.IO_ADDR_W);
>> > }
>> > -
>> > - /* disable and stop the PFPW engine */
>> > - gpmc_prefetch_reset(info->gpmc_cs);
>> > + /* wait for data to flushed-out before reset the
>> prefetch */
>> > + do {
>> > + gpmc_hwcontrol(info->gpmc_cs,
>> > + GPMC_PREFETCH_COUNT, 0, 0, &pref_count);
>> > + } while (pref_count);
>> > }
>> > + /* disable and stop the PFPW engine */
>> > + gpmc_prefetch_reset(info->gpmc_cs);
>>
>> Same as above.
> [Ghorai] Agree and I will take this kind of cleanup as 4th patch, as its not
> matching with patch descri
RE: [PATCH v3 2/3] omap3 nand: cleanup virtual address usages
Vimal,
> -Original Message-
> From: Vimal Singh [mailto:vimal.neww...@gmail.com]
> Sent: 2010-05-19 21:00
> To: Ghorai, Sukumar
> Cc: linux-omap@vger.kernel.org; linux-...@lists.infradead.org;
> t...@atomide.com; sako...@gmail.com; m...@compulab.co.il;
> artem.bityuts...@nokia.com
> Subject: Re: [PATCH v3 2/3] omap3 nand: cleanup virtual address usages
>
> On Tue, May 18, 2010 at 4:46 PM, Sukumar Ghorai wrote:
> > This patch removes direct reference of gpmc address from generic nand
> platform code.
> > Nand platform code now uses wrapper functions which are implemented in
> gpmc module.
> >
> > Signed-off-by: Sukumar Ghorai
> [...]
>
> >
> > @@ -287,16 +246,15 @@ static void omap_read_buf_pref(struct mtd_info
> *mtd, u_char *buf, int len)
> > {
> > struct omap_nand_info *info = container_of(mtd,
> > struct omap_nand_info,
> mtd);
> > - uint32_t pfpw_status = 0, r_count = 0;
> > + u32 r_count = 0;
> > int ret = 0;
> > - u32 *p = (u32 *)buf;
> > + u32 *p;
> >
> > /* take care of subpage reads */
> > for (; len % 4 != 0; ) {
> > *buf++ = __raw_readb(info->nand.IO_ADDR_R);
> > len--;
> > }
> > - p = (u32 *) buf;
>
> Above code had an issue, which was fixed by this commit:
> http://git.infradead.org/mtd-
> 2.6.git/commitdiff/c3341d0ceb4de1680572024f50233403c6a8b10d
>
> I would suggest you to prepare your patch on MTD tree.
[Ghorai] Patches started posting on lo. And lets continue the same.
>
> >
> > /* configure and start prefetch transfer */
> > ret = gpmc_prefetch_enable(info->gpmc_cs, 0x0, len, 0x0);
> > @@ -307,17 +265,18 @@ static void omap_read_buf_pref(struct mtd_info
> *mtd, u_char *buf, int len)
> > else
> > omap_read_buf8(mtd, buf, len);
> > } else {
> > + p = (u32 *) buf;
> > do {
> > - pfpw_status = gpmc_prefetch_status();
> > - r_count = ((pfpw_status >> 24) & 0x7F) >> 2;
> > - ioread32_rep(info->nand_pref_fifo_add, p,
> r_count);
> > + gpmc_hwcontrol(info->gpmc_cs,
> > + GPMC_PREFETCH_FIFO_CNT, 0, 0, &r_count);
> > + r_count = r_count >> 2;
> > + ioread32_rep(info->nand.IO_ADDR_R, p, r_count);
> > p += r_count;
> > - len -= r_count << 2;
> > + len -= (r_count << 2);
>
> Braces are not required here.
[Ghorai] thanks
>
> > } while (len);
> > -
>
> After call to 'gpmc_prefetch_enable', next line are:
>if (ret) {
>/* PFPW engine is busy, use cpu copy method */
>if (info->nand.options & NAND_BUSWIDTH_16)
>...
>...
> > - /* disable and stop the PFPW engine */
> > - gpmc_prefetch_reset(info->gpmc_cs);
> > }
>
> So, if above 'if' fails, driver will not get prefetch engine (it was
> already busy). Then it doesn't makes sense to call for __reset__.
[Ghorai] I will take this clean up as 4th patch. As its not matching with patch
description.
>
> > + /* disable and stop the PFPW engine */
> > + gpmc_prefetch_reset(info->gpmc_cs);
>
> (Also see my comments on your other patch.)
[Ghorai] Agree and I will take this kind of cleanup as 4th patch
>
> > }
> >
> > /**
> > @@ -331,13 +290,13 @@ static void omap_write_buf_pref(struct mtd_info
> *mtd,
> > {
> > struct omap_nand_info *info = container_of(mtd,
> > struct omap_nand_info,
> mtd);
> > - uint32_t pfpw_status = 0, w_count = 0;
> > + uint32_t pref_count = 0, w_count = 0;
> > int i = 0, ret = 0;
> > - u16 *p = (u16 *) buf;
> > + u16 *p;
> >
> > /* take care of subpage writes */
> > if (len % 2 != 0) {
> > - writeb(*buf, info->nand.IO_ADDR_R);
> > + writeb(*buf, info->nand.IO_ADDR_W);
> > p = (u16 *)(buf + 1);
> > len--;
> > }
> > @@ -351,17 +310,22 @@ static void omap_write_buf_pref(struct mtd_inf
Re: [PATCH v3 2/3] omap3 nand: cleanup virtual address usages
On Tue, May 18, 2010 at 4:46 PM, Sukumar Ghorai wrote:
> This patch removes direct reference of gpmc address from generic nand
> platform code.
> Nand platform code now uses wrapper functions which are implemented in gpmc
> module.
>
> Signed-off-by: Sukumar Ghorai
[...]
>
> @@ -287,16 +246,15 @@ static void omap_read_buf_pref(struct mtd_info *mtd,
> u_char *buf, int len)
> {
> struct omap_nand_info *info = container_of(mtd,
> struct omap_nand_info, mtd);
> - uint32_t pfpw_status = 0, r_count = 0;
> + u32 r_count = 0;
> int ret = 0;
> - u32 *p = (u32 *)buf;
> + u32 *p;
>
> /* take care of subpage reads */
> for (; len % 4 != 0; ) {
> *buf++ = __raw_readb(info->nand.IO_ADDR_R);
> len--;
> }
> - p = (u32 *) buf;
Above code had an issue, which was fixed by this commit:
http://git.infradead.org/mtd-2.6.git/commitdiff/c3341d0ceb4de1680572024f50233403c6a8b10d
I would suggest you to prepare your patch on MTD tree.
>
> /* configure and start prefetch transfer */
> ret = gpmc_prefetch_enable(info->gpmc_cs, 0x0, len, 0x0);
> @@ -307,17 +265,18 @@ static void omap_read_buf_pref(struct mtd_info *mtd,
> u_char *buf, int len)
> else
> omap_read_buf8(mtd, buf, len);
> } else {
> + p = (u32 *) buf;
> do {
> - pfpw_status = gpmc_prefetch_status();
> - r_count = ((pfpw_status >> 24) & 0x7F) >> 2;
> - ioread32_rep(info->nand_pref_fifo_add, p, r_count);
> + gpmc_hwcontrol(info->gpmc_cs,
> + GPMC_PREFETCH_FIFO_CNT, 0, 0, &r_count);
> + r_count = r_count >> 2;
> + ioread32_rep(info->nand.IO_ADDR_R, p, r_count);
> p += r_count;
> - len -= r_count << 2;
> + len -= (r_count << 2);
Braces are not required here.
> } while (len);
> -
After call to 'gpmc_prefetch_enable', next line are:
if (ret) {
/* PFPW engine is busy, use cpu copy method */
if (info->nand.options & NAND_BUSWIDTH_16)
...
...
> - /* disable and stop the PFPW engine */
> - gpmc_prefetch_reset(info->gpmc_cs);
> }
So, if above 'if' fails, driver will not get prefetch engine (it was
already busy). Then it doesn't makes sense to call for __reset__.
> + /* disable and stop the PFPW engine */
> + gpmc_prefetch_reset(info->gpmc_cs);
(Also see my comments on your other patch.)
> }
>
> /**
> @@ -331,13 +290,13 @@ static void omap_write_buf_pref(struct mtd_info *mtd,
> {
> struct omap_nand_info *info = container_of(mtd,
> struct omap_nand_info, mtd);
> - uint32_t pfpw_status = 0, w_count = 0;
> + uint32_t pref_count = 0, w_count = 0;
> int i = 0, ret = 0;
> - u16 *p = (u16 *) buf;
> + u16 *p;
>
> /* take care of subpage writes */
> if (len % 2 != 0) {
> - writeb(*buf, info->nand.IO_ADDR_R);
> + writeb(*buf, info->nand.IO_ADDR_W);
> p = (u16 *)(buf + 1);
> len--;
> }
> @@ -351,17 +310,22 @@ static void omap_write_buf_pref(struct mtd_info *mtd,
> else
> omap_write_buf8(mtd, buf, len);
> } else {
> - pfpw_status = gpmc_prefetch_status();
> - while (pfpw_status & 0x3FFF) {
> - w_count = ((pfpw_status >> 24) & 0x7F) >> 1;
> + p = (u16 *) buf;
> + while (len) {
> + gpmc_hwcontrol(info->gpmc_cs,
> + GPMC_PREFETCH_FIFO_CNT, 0, 0,
> &w_count);
> + w_count = w_count >> 1;
> for (i = 0; (i < w_count) && len; i++, len -= 2)
> - iowrite16(*p++, info->nand_pref_fifo_add);
> - pfpw_status = gpmc_prefetch_status();
> + iowrite16(*p++, info->nand.IO_ADDR_W);
> }
> -
> - /* disable and stop the PFPW engine */
> - gpmc_prefetch_reset(info->gpmc_cs);
> + /* wait for data to flushed-out before reset the prefetch */
> + do {
> + gpmc_hwcontrol(info->gpmc_cs,
> + GPMC_PREFETCH_COUNT, 0, 0, &pref_count);
> + } while (pref_count);
> }
> + /* disable and stop the PFPW engine */
> + gpmc_prefetch_reset(info->gpmc_cs);
Same as above.
> }
>
> #ifdef CONFIG_MTD_NAND_OMAP_PREFETCH_DMA
> @@ -448,8 +412,10 @@ static inline int omap_nand_dma_transfer(stru
