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

[Vimal Singh]

On Thu, May 20, 2010 at 11:08 AM, Ghorai, Sukumar <s-ghorai at ti.com> wrote:
> 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?

As long as you take care of current comments, I do not have any
further comment for now.


-- 
Regards,
Vimal Singh