Re: [PATCH v2 2/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Jon Hunter <jon-hunter@ti.com>]

Hi Afzal,

On 06/19/2012 12:57 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Mon, Jun 18, 2012 at 21:31:46, Hunter, Jon wrote:
> 
>>> @@ -95,10 +89,6 @@ static int omap2_onenand_set_async_mode(int cs, void __iomem *onenand_base)
>>>  	if (err)
>>>  		return err;
>>>  
>>> -	/* Ensure sync read and sync write are disabled */
>>> -	reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
>>> -	reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
>>> -	writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
>>
>> Sorry if I was not clear, but I was still thinking that we keep the
>> above instance of setting async mode.
> :
>>>  static int gpmc_onenand_setup(void __iomem *onenand_base, int *freq_ptr)
>>>  {
>>>  	struct device *dev = &gpmc_onenand_device.dev;
>>> +	u32 reg;
>>> +
>>> +	/* Ensure sync read and sync write are disabled */
>>> +	reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
>>> +	reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
>>> +	writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
>>
>> ... and here we should just call omap2_onenand_set_async_mode(), ...
> 
> If omap2_onenand_set_async_mode is called from gpmc_onenand_setup & removed
> from gpmc_onenand_init, when using new gpmc driver interface, i.e. using
> gpmc_onenand_update [1], we lost opportunity to provide gpmc driver with
> configuration details for async mode causing a big warning about each
> timings & configurations that was left by bootloader (which is meant only
> for cases where Kernel can't configure GPMC). Of course, we can put
> omap2_onenand_set_async_mode in gpmc_onenand_update too, but then
> unnecessarily, omap2_onenand_sey_async_mode would be called twice.
> 
> Please note that gpmc_onenand_setup is a callback by onenand driver.

Ok, I see your point. So today the _set_async_mode() is really doing two
things ...

1. It is calculating the timings
2. Programming the timings and setting async mode.

I think that it would be best if we were to make _set_async_mode() into
two functions, for example ...

1. omap2_onenand_get_async_timings()
2. omap2_onenand_set_async_timings()

Where the omap2_onenand_get_async_timings() calculates the timings and
can be used in both legacy mode and with the driver. Then
omap2_onenand_set_sync_timings() is just used in the legacy mode where
we don't have the driver to configure the chip-select.

Do you think that could work?

We could also do the same for omap2_onenand_set_sync_mode().

Cheers
Jon