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

[Vimal Singh]

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@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.

>
[...]
>> > > +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