Vimal, > -----Original Message----- > From: Vimal Singh [mailto:vimal.newwork@xxxxxxxxx] > Sent: 2010-05-20 00:01 > To: Ghorai, Sukumar > Cc: linux-omap@xxxxxxxxxxxxxxx; linux-mtd@xxxxxxxxxxxxxxxxxxx; > tony@xxxxxxxxxxx; sakoman@xxxxxxxxx; mike@xxxxxxxxxxxxxx; > Artem.Bityutskiy@xxxxxxxxx; peter.barada@xxxxxxxxx > Subject: Re: [PATCH v3 1/3] omap3 gpmc: functionality enhancement > > On Wed, May 19, 2010 at 11:34 PM, Ghorai, Sukumar <s-ghorai@xxxxxx> 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@xxxxxxxxxxxxxxx/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@xxxxxxxxxxxxxxx/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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
