[PATCH v3 2/3] omap3 nand: cleanup virtual address usages
Ghorai, Sukumar
s-ghorai at ti.com
Wed May 19 13:24:19 EDT 2010
Vimal,
> -----Original Message-----
> From: Vimal Singh [mailto:vimal.newwork at gmail.com]
> Sent: 2010-05-19 21:00
> To: Ghorai, Sukumar
> Cc: linux-omap at vger.kernel.org; linux-mtd at lists.infradead.org;
> tony at atomide.com; sakoman at gmail.com; mike at compulab.co.il;
> Artem.Bityutskiy at 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 <s-ghorai at ti.com> 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 <s-ghorai at ti.com>
> [...]
>
> >
> > @@ -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_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 description.
>
>
> > }
> >
> > #ifdef CONFIG_MTD_NAND_OMAP_PREFETCH_DMA
> > @@ -448,8 +412,10 @@ static inline int omap_nand_dma_transfer(struct
> mtd_info *mtd, void *addr,
> > /* setup and start DMA using dma_addr */
> > wait_for_completion(&info->comp);
> >
> > - while (0x3fff & (prefetch_status = gpmc_prefetch_status()))
> > - ;
> > + do {
> > + gpmc_hwcontrol(info->gpmc_cs,
> > + GPMC_PREFETCH_COUNT, 0, 0,
> &prefetch_status);
> > + } while (prefetch_status);
> > /* disable and stop the PFPW engine */
> > gpmc_prefetch_reset();
> >
> > @@ -502,7 +468,7 @@ static void omap_write_buf_dma_pref(struct mtd_info
> *mtd,
> > omap_write_buf_pref(mtd, buf, len);
> > else
> > /* start transfer in DMA mode */
> > - omap_nand_dma_transfer(mtd, buf, len, 0x1);
> > + omap_nand_dma_transfer(mtd, (u_char *) buf, len, 0x1);
>
> This is already fixed. See commit:
> http://git.infradead.org/mtd-
> 2.6.git/commitdiff/bdaefc41627b6f2815ef7aa476dfa4ebb3ad499f
[Ghorai] thanks I will omit this from this patch
>
>
> Rest, patches looks good. It is a good clean-up all together.
[Ghorai] Is it possible for you to check once again if you have any additional comments! This is to identify the issue if any at early stage.
>
> --
> Regards,
> Vimal Singh
More information about the linux-mtd
mailing list