[PATCH v2 1/2] omap3 nand: cleanup for not to use GPMC virtual address

[Ghorai, Sukumar]

> -----Original Message-----
> From: Vimal Singh [mailto:vimal.newwork at gmail.com]
> Sent: 2010-05-17 19:57
> To: Ghorai, Sukumar
> Cc: linux-omap at vger.kernel.org; Artem.Bityutskiy at nokia.com;
> tony at atomide.com; sakoman at gmail.com; linux-mtd at lists.infradead.org
> Subject: Re: [PATCH v2 1/2] omap3 nand: cleanup for not to use GPMC
> virtual address
> 
> On Mon, May 17, 2010 at 9:52 AM, Ghorai, Sukumar <s-ghorai at ti.com> wrote:
> [...]
> >> > @@ -108,11 +108,27 @@ static u32 gpmc_read_reg(int idx)
> >> >        return __raw_readl(gpmc_base + idx);
> >> >  }
> >> >
> >> > +static void gpmc_cs_write_byte(int cs, int idx, u8 val)
> >> > +{
> >> > +       void __iomem *reg_addr;
> >> > +
> >> > +       reg_addr = gpmc_base + GPMC_CS0_BASE + (cs * GPMC_CS_SIZE) +
> >> idx;
> >> > +       __raw_writeb(val, reg_addr);
> >> > +}
> >> > +
> >> > +static u8 gpmc_cs_read_byte(int cs, int idx)
> >> > +{
> >> > +       void __iomem *reg_addr;
> >> > +
> >> > +       reg_addr = gpmc_base + GPMC_CS0_BASE + (cs * GPMC_CS_SIZE) +
> >> idx;
> >> > +       return __raw_readb(reg_addr);
> >> > +}
> >> > +
> >>
> >> I do not think we need these functions.
> > [Ghorai] This is used in gpmc_hwcontrol() and to get the nand status
> from omap2.c.
> 
> Yes, I can see that. But I think you should read complete register
> (32-bits) and the manipulate them accordingly.
> 
> >
> [...]
> >> > @@ -229,14 +191,15 @@ static void omap_read_buf8(struct mtd_info
> *mtd,
> >> u_char *buf, int len)
> >> >  */
> >> >  static void omap_write_buf8(struct mtd_info *mtd, const u_char *buf,
> >> int len)
> >> >  {
> >> > -       struct omap_nand_info *info = container_of(mtd,
> >> > -                                               struct
> omap_nand_info,
> >> mtd);
> >> > +       u32     status;
> >> > +       struct nand_chip *nand = mtd->priv;
> >> >        u_char *p = (u_char *)buf;
> >> >
> >> >        while (len--) {
> >> > -               iowrite8(*p++, info->nand.IO_ADDR_W);
> >> > -               while (GPMC_BUF_EMPTY == (readl(info->gpmc_baseaddr +
> >> > -                                               GPMC_STATUS) &
> >> GPMC_BUF_FULL));
> >> > +               iowrite8(*p++, nand->IO_ADDR_W);
> >> > +               gpmc_hwcontrol(0, 0, GPMC_GET_SET_STATUS, 0,
> &status);
> >> If I am not mistaking, 2nd argument is 'cs', correct? And then, why
> >> are you hard coding this?
> >> Different boards will have NAND chip present at different 'cs'.
> >> Please have a look at uses of 'gpmc_hwcontrol' elsewhere as well for
> this.
> > [Ghorai] I agree.
> >>
> >> Again, say, you got '(status & GPMC_BUF_FULL) != GPMC_BUF_EMPTY', then:
> >>
> >> > +               while (GPMC_BUF_EMPTY == (status & GPMC_BUF_FULL))
> >> > +                       ;
> >>
> >> You got in an infinite loop here?
> > [Ghorai] if you see carefully this is same as existing code. Let me
> check if any better solution.
> 
> No. Look carefully. In previous code 'gpmc status' was being read in
> each loop, while in your code you read it once and then you never look
> for updated value.
> That's why your code is going into infinite loop
[Ghorai] ok. thanks
> 
> 
> --
> Regards,
> Vimal Singh