On Wed, May 19, 2010 at 10:54 PM, Ghorai, Sukumar <s-ghorai@xxxxxx> 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 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.
Yeah, I can understand. I tried to review it fully, but can not say if
still something left and gets attention later on. Even sometimes they
will become visible when you fix current one.
As of now this is all what I had.
--
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
