Re: [PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion

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

Hi Afzal,

On 06/11/2012 09:01 AM, Afzal Mohammed wrote:
> Reorganize gpmc-onenand initialization so that changes
> required for gpmc driver migration can be made smooth.
> 
> Ensuring sync read/write are disabled in onenand cannot be
> expect to work properly unless GPMC is setup, this has been
> removed. 

So I think I see what you are saying, but the above is not 100% clear. I
think that what you are saying is that omap2_onenand_set_async_mode() is
attempting to configure the onenand registers before configuring the
gpmc interface and hence this is not correct.

> Two instances of doing the same has been clubbed
> into one as onenand driver always calls indirectly
> "omap2_onenand_set_sync_mode", and has placed such that
> configuring onenand for async read/write would happen once
> GPMC is setup for async mode (which happens even for sync
> mode, before configuring to sync mode).

It may be clearer to say that _set_sync_mode is always called during
onenand setup and this will call _set_async_mode. So instead of calling
_set_async_mode from _set_sync_mode, just call it directly during the
gpmc_onenand_init().

> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> ---
>  arch/arm/mach-omap2/gpmc-onenand.c |   24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
> index 71d7c07..fd4c48d 100644
> --- a/arch/arm/mach-omap2/gpmc-onenand.c
> +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> @@ -38,10 +38,9 @@ static struct platform_device gpmc_onenand_device = {
>  	.resource	= &gpmc_onenand_resource,
>  };
>  
> -static int omap2_onenand_set_async_mode(int cs, void __iomem *onenand_base)
> +static int omap2_onenand_set_async_mode(int cs)
>  {
>  	struct gpmc_timings t;
> -	u32 reg;
>  	int err;
>  
>  	const int t_cer = 15;
> @@ -55,11 +54,6 @@ static int omap2_onenand_set_async_mode(int cs, void __iomem *onenand_base)
>  	const int t_wpl = 40;
>  	const int t_wph = 30;
>  
> -	/* 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);
> -
>  	memset(&t, 0, sizeof(t));
>  	t.sync_clk = 0;
>  	t.cs_on = 0;
> @@ -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);

I agree with getting rid of the first instance at the beginning of
_set_async_mode, but why get rid of the above one? Are you assuming that
by default it is in async mode? Could be nice to keep it to be explicit.

>  	return 0;
>  }
> @@ -191,19 +181,21 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
>  	u32 reg;
>  	bool clk_dep = false;
>  
> +	/* 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);
> +

Should we only set these after we have checked the flags and depending
on which flags are set?

>  	if (cfg->flags & ONENAND_SYNC_READ) {
>  		sync_read = 1;
>  	} else if (cfg->flags & ONENAND_SYNC_READWRITE) {
>  		sync_read = 1;
>  		sync_write = 1;
>  	} else
> -		return omap2_onenand_set_async_mode(cs, onenand_base);
> +		return 0;
>  
>  	if (!freq) {
>  		/* Very first call freq is not known */
> -		err = omap2_onenand_set_async_mode(cs, onenand_base);
> -		if (err)
> -			return err;
>  		freq = omap2_onenand_get_freq(cfg, onenand_base, &clk_dep);
>  		first_time = 1;
>  	}
> @@ -421,6 +413,8 @@ void __init gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
>  	gpmc_onenand_resource.end = gpmc_onenand_resource.start +
>  							ONENAND_IO_SIZE - 1;
>  
> +	omap2_onenand_set_async_mode(gpmc_onenand_data->cs);
> +

What about putting this in the gpmc_onenand_setup() function before the
call to _set_sync_mode? Seems nice to have these together.

Cheers
Jon