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

[Vimal Singh]

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


-- 
Regards,
Vimal Singh