Re: [PATCH v3 2/3] omap3 nand: cleanup virtual address usages

From: Vimal Singh <vimal.newwork@gmail.com>
To: Sukumar Ghorai <s-ghorai@ti.com>
Cc: linux-omap@vger.kernel.org, linux-mtd@lists.infradead.org,
	tony@atomide.com, sakoman@gmail.com, mike@compulab.co.il,
	Artem.Bityutskiy@nokia.com
Subject: Re: [PATCH v3 2/3] omap3 nand: cleanup virtual address usages
Date: Wed, 19 May 2010 21:00:04 +0530	[thread overview]
Message-ID: <AANLkTinEaP9cN5A24K9Zwd4L9Fq4pzxQqqY44r7NSl4-@mail.gmail.com> (raw)
In-Reply-To: <1274181389-7488-3-git-send-email-s-ghorai@ti.com>

On Tue, May 18, 2010 at 4:46 PM, Sukumar Ghorai <s-ghorai@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@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.

>
>        /* 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(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

Rest, patches looks good. It is a good clean-up all together.

-- 
Regards,
Vimal Singh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html