Vimal, > -----Original Message----- > From: Vimal Singh [mailto:vimal.newwork@xxxxxxxxx] > Sent: 2010-05-19 21:00 > To: Ghorai, Sukumar > Cc: linux-omap@xxxxxxxxxxxxxxx; linux-mtd@xxxxxxxxxxxxxxxxxxx; > tony@xxxxxxxxxxx; sakoman@xxxxxxxxx; mike@xxxxxxxxxxxxxx; > Artem.Bityutskiy@xxxxxxxxx > 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@xxxxxx> 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@xxxxxx> > [...] > > > > > @@ -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 -- 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
