[PATCH v3 1/3] omap3 gpmc: functionality enhancement

[Ghorai, Sukumar]

Vimal,

> -----Original Message-----
> From: Vimal Singh [mailto:vimal.newwork at gmail.com]
> Sent: 2010-05-20 00:01
> To: Ghorai, Sukumar
> Cc: linux-omap at vger.kernel.org; linux-mtd at lists.infradead.org;
> tony at atomide.com; sakoman at gmail.com; mike at compulab.co.il;
> Artem.Bityutskiy at nokia.com; peter.barada at gmail.com
> Subject: Re: [PATCH v3 1/3] omap3 gpmc: functionality enhancement
> 
> On Wed, May 19, 2010 at 11:34 PM, Ghorai, Sukumar <s-ghorai at ti.com> wrote:
> >> > > +
> >> > > +       case GPMC_CONFIG_RDY_BSY:
> >> > > +               regval  = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> >> > > +               regval |= WR_RD_PIN_MONITORING;
> >> > > +               gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, regval);
> >> > > +               break;
> >> >
> >> > IIRC, at least in OMAP2/3, ready/busy pin is not in use (not
> connected).
> >>
> >> On the Logic OMAP3530 LV SOM and Torpedo modules, the R/B# pin of the
> >> NAND in the Micron mt29c2g4maklajg-6it POP part is connected to the
> >> WAIT0 pin on the OMAP3530 and I'm looking to use it to speed up NAND
> >> accesses
> > [Ghorai] So better keep this feature,
> 
> Yes, looks like there are some boards which can still take advantage of
> this.
> 
> >>
> [...]
> >> > > @@ -456,13 +572,20 @@ EXPORT_SYMBOL(gpmc_prefetch_enable);
> >> > >  /**
> >> > >  * gpmc_prefetch_reset - disables and stops the prefetch engine
> >> > >  */
> >> > > -void gpmc_prefetch_reset(void)
> >> > > +int gpmc_prefetch_reset(int cs)
> >> > >  {
> >> > > +       if (gpmc_pref_used == cs)
> >> > > +               gpmc_pref_used = -EINVAL;
> >> > > +       else
> >> > > +               return -EINVAL;
> >> > > +
> >> >
> >> > This is also not required. As, this function will be called only if
> >> > prefetch was used.
> > [Ghorai] Agree. Can you see this input too?
> > http://www.mail-archive.com/linux-omap@vger.kernel.org/msg28520.html
> 
> Exactly, this is what I am telling here. Enable prefetch engine call
> is already being check for *busy* or not.
> 
> >
> [...]
> >> > > +int gpmc_ecc_init(int cs, int ecc_size)
> >> > > +{
> >> > > +       unsigned int val = 0x0;
> >> > > +
> >> > > +       /* check if ecc engine already by another cs */
> >> > > +       if (gpmc_ecc_used == -EINVAL)
> >> > > +               gpmc_ecc_used = cs;
> >> > > +       else
> >> > > +               return -EBUSY;
> >> > Here few things need be to consider:
> >> > 1. 'init' is supposed to done once for every instance of driver
> during
> >> probe
> >> > 2. But ECC engine, too, have only one instance at a time, So
> >> > 3. As long as all NAND chip are supposed to use same ECC machenism,
> we
> >> > can go for only one time 'init' for all drivers, perhaps in
> >> > gpmc_nand.c.
> >> > 4. But in case, different instances of driver (or NAND chip) requires
> >> > different ECC machenism (for ex. Hamming or BCH, or even with
> >> > different capabilities of error correction),
> >> > this will no longer vailid. Then rather we should have something like
> >> > 'gpmc_ecc_config' call to configer ECC engine for everytime a driver
> >> > needs it (something like as it is done for prefetch engine).
> > [Ghorai]
> > a. do you think it will reduce the throughput?
> No. But in current implementation it will be called for each instance
> driver. (see my 3rd point)
> 
> > b. Moreover I think we will take this as 5th patch as cleanup/
> improvemnt.
> > c. And how to know that ECC engine is in used other driver should not
> use it? Any bit to know that ecc engine is busy, as we check for prefetch?
> Do not really remember config registers. Perhaps there is no way.
> But I guess you should check into register GPMC_ECC_CONFIG at bit 1.
> This is the bit we are setting to enable ECC calculation, IIRC.
> 
> > d. any further input on http://www.mail-archive.com/linux-
> omap at vger.kernel.org/msg28520.html
> And this what I was suggesting in my point 4. In my example
> 'gpmc_ecc_config' is analogy to 'gpmc_ecc_request'.
> I said *config*, since in such scenario you need to configer HW
> ECCconfig register everytime as well, rather just checking
> availability and enabling.
[Ghorai] still I feel we should not mix this patch with cleanup. And yes if possible this will be the 5th one as cleanup.
4th one - prefetch cleanup
5th one - ecc cleanup.
Do you think still missing anything for this patch?

> 
> >
> [...]
> >> > > +int gpmc_ecc_reset(int cs)
> >> > > +{
> >> > > +       if (gpmc_ecc_used == cs)
> >> > > +               gpmc_ecc_used = -EINVAL;
> >> > > +       else
> >> > > +               return -EINVAL;
> >> > > +
> >> > > +       return 0;
> >> > > +}
> 
> I guess in this function you should also clear gpmc ecc config
> register explicitly.
> 
> 
> --
> Regards,
> Vimal Singh