Re: ARM: dts: omap3: NAND support - how?

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

On 04/19/2013 09:53 AM, Christoph Fritz wrote:
> On Fri, 2013-04-19 at 09:00 -0500, Jon Hunter wrote:
>> On 04/19/2013 07:02 AM, Christoph Fritz wrote:
>>> so I hacked the missing values to omap2_nand_gpmc_retime():
>>>
>>> From 8868823925441a824fe0d3143614482f25fb379b Mon Sep 17 00:00:00 2001
>>> From: Christoph Fritz <chf.fritz@googlemail.com>
>>> Date: Fri, 19 Apr 2013 12:41:11 +0200
>>> Subject: [PATCH] [RFC] ARM: OMAP2+: nand: add missing gpmc timing values
>>>
>>> This patch adds missing gpmc timing values to omap2_nand_gpmc_retime().
>>> ---
>>>  arch/arm/mach-omap2/gpmc-nand.c |   10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
>>> index d9c2719..d8bb241 100644
>>> --- a/arch/arm/mach-omap2/gpmc-nand.c
>>> +++ b/arch/arm/mach-omap2/gpmc-nand.c
>>> @@ -58,6 +58,9 @@ static int omap2_nand_gpmc_retime(
>>>  	/* Read */
>>>  	t.adv_rd_off = gpmc_t->adv_rd_off;
>>>  	t.oe_on  = t.adv_on;
>>> +	if (cpu_is_omap34xx()) {
>>> +		t.oe_on = gpmc_t->oe_on;
>>> +	}
>>
>> How about just setting gpmc,adv-on-ns, then you don't need the above.
>>
>>>  	t.access = gpmc_t->access;
>>>  	t.oe_off = gpmc_t->oe_off;
>>>  	t.cs_rd_off = gpmc_t->cs_rd_off;
>>> @@ -69,11 +72,18 @@ static int omap2_nand_gpmc_retime(
>>>  	if (cpu_is_omap34xx()) {
>>
>> We should get rid of cpu_is_omap34xx() here as this is handled by
>> gpmc_cs_set_timings().
>>
>>>  		t.wr_data_mux_bus = gpmc_t->wr_data_mux_bus;
>>>  		t.wr_access = gpmc_t->wr_access;
>>> +		t.we_on = gpmc_t->we_on;
>>
>> How about just setting gpmc,adv-on-ns, then you don't need the above.
>>
>>>  	}
>>>  	t.we_off = gpmc_t->we_off;
>>>  	t.cs_wr_off = gpmc_t->cs_wr_off;
>>>  	t.wr_cycle = gpmc_t->wr_cycle;
>>>  
>>> +	if (cpu_is_omap34xx()) {
>>> +		t.bool_timings = gpmc_t->bool_timings;
>>> +		t.cycle2cycle_delay = gpmc_t->cycle2cycle_delay;
>>> +		t.page_burst_access = gpmc_t->page_burst_access;
>>> +	}
>>
>> The above timings are applicable to all gpmc versions and so you should
>> not need to make this dependent on cpu_is_omap34xx(). So would be
>> worthwhile spending a patch to add the above timings to this function.
> 
> Before sending this patch, I'd like to get your opinion on this
> approach:
> 
> From 015f1e8006f8f85818b6bbd5ba00dc6b4ae48b65 Mon Sep 17 00:00:00 2001
> From: Christoph Fritz <chf.fritz@googlemail.com>
> Date: Fri, 19 Apr 2013 12:41:11 +0200
> Subject: [RFC][PATCH] ARM: OMAP2+: nand: reorganize gpmc timing values
> 
> This patch removes omap2_nand_gpmc_retime() which was used to quirk
> some timing values before gpmc_cs_set_timings(). Due to recent changes,
> gpmc_cs_set_timings() has evolved so that there is no more need for
> omap2_nand_gpmc_retime().
> 
> Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com>
> ---
>  arch/arm/mach-omap2/gpmc-nand.c |   40 +--------------------------------------
>  1 file changed, 1 insertion(+), 39 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
> index d9c2719..c8044b0 100644
> --- a/arch/arm/mach-omap2/gpmc-nand.c
> +++ b/arch/arm/mach-omap2/gpmc-nand.c
> @@ -43,44 +43,6 @@ static struct platform_device gpmc_nand_device = {
>  	.resource	= gpmc_nand_resource,
>  };
>  
> -static int omap2_nand_gpmc_retime(
> -				struct omap_nand_platform_data *gpmc_nand_data,
> -				struct gpmc_timings *gpmc_t)
> -{
> -	struct gpmc_timings t;
> -	int err;
> -
> -	memset(&t, 0, sizeof(t));
> -	t.sync_clk = gpmc_t->sync_clk;
> -	t.cs_on = gpmc_t->cs_on;
> -	t.adv_on = gpmc_t->adv_on;
> -
> -	/* Read */
> -	t.adv_rd_off = gpmc_t->adv_rd_off;
> -	t.oe_on  = t.adv_on;
> -	t.access = gpmc_t->access;
> -	t.oe_off = gpmc_t->oe_off;
> -	t.cs_rd_off = gpmc_t->cs_rd_off;
> -	t.rd_cycle = gpmc_t->rd_cycle;
> -
> -	/* Write */
> -	t.adv_wr_off = gpmc_t->adv_wr_off;
> -	t.we_on  = t.oe_on;
> -	if (cpu_is_omap34xx()) {
> -		t.wr_data_mux_bus = gpmc_t->wr_data_mux_bus;
> -		t.wr_access = gpmc_t->wr_access;
> -	}
> -	t.we_off = gpmc_t->we_off;
> -	t.cs_wr_off = gpmc_t->cs_wr_off;
> -	t.wr_cycle = gpmc_t->wr_cycle;
> -
> -	err = gpmc_cs_set_timings(gpmc_nand_data->cs, &t);
> -	if (err)
> -		return err;
> -
> -	return 0;
> -}
> -
>  static bool gpmc_hwecc_bch_capable(enum omap_ecc ecc_opt)
>  {
>  	/* support only OMAP3 class */
> @@ -131,7 +93,7 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
>  				gpmc_get_client_irq(GPMC_IRQ_COUNT_EVENT);
>  
>  	if (gpmc_t) {
> -		err = omap2_nand_gpmc_retime(gpmc_nand_data, gpmc_t);
> +		err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t);
>  		if (err < 0) {
>  			dev_err(dev, "Unable to set gpmc timings: %d\n", err);
>  			return err;
> 

Thanks for sending this. I would agree with this approach. The retime
function seems very redundant looking at what it does.

Grep'ing through the source, the only place I see a board file call
gpmc_nand_init() and pass timings is in
arch/arm/mach-omap2/board-flash.c. To keep the gpmc configuration
consistent, I would also suggest making the following change so that
oe_on and we_on are programmed as they would be by the current retime
function.

diff --git a/arch/arm/mach-omap2/board-flash.c
b/arch/arm/mach-omap2/board-flash.c
index c33adea..946a7516 100644
--- a/arch/arm/mach-omap2/board-flash.c
+++ b/arch/arm/mach-omap2/board-flash.c
@@ -116,6 +116,8 @@ struct gpmc_timings nand_default_timings[1] = {
                .adv_rd_off = 24,
                .adv_wr_off = 36,

+               .we_on = 6,
+               .oe_on = 6,
                .we_off = 30,
                .oe_off = 48,

Cheers
Jon