From mboxrd@z Thu Jan  1 00:00:00 1970
From: Nishanth Menon <menon.nishanth@gmail.com>
Date: Wed, 04 Mar 2009 09:25:18 +0200
Subject: [U-Boot] [PATCH 1/1] Changes for single binary image for u-boot
 for	NAND/OneNAND flash.
In-Reply-To: <19F8576C6E063C45BE387C64729E73940427BC9F9A@dbde02.ent.ti.com>
References: <1236051689-17867-1-git-send-email-mani.pillai@ti.com>
	<49ACEB4D.3070402@gmail.com>
	<19F8576C6E063C45BE387C64729E73940427BC9F9A@dbde02.ent.ti.com>
Message-ID: <49AE2CDE.7010904@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Pillai, Manikandan said the following on 03/04/2009 08:35 AM:
>>>  #if defined(CONFIG_ENV_IS_IN_NAND)         /* Environment is in Nand
>>>       
>> Flash */ \
>>     
>>> -   || defined(CONFIG_ENV_IS_IN_SPI_FLASH)
>>> +   || defined(CONFIG_ENV_IS_IN_SPI_FLASH) \
>>> +   || (defined(CONFIG_CMD_NAND) && defined(CONFIG_ENV_IS_RUNTIME_SEL))
>>>
>>>       
>> Errr.... ENV_IS_IN_NAND Vs ENV_IS_RUNTIME_SEL is not clear.
>>     
> [Pillai, Manikandan] I am not clear with the query
>
>   

CONFIG_ENV_IS_RUNTIME_SEL is dependent only on CMD_NAND? if I had onenand and NOR, then what?

>>> +void print_board_info(void)
>>> +{
>>> +   u32 btype;
>>> +
>>> +   btype = get_board_type();
>>> +
>>> +   display_board_info(btype);
>>> +}
>>> +
>>>
>>>       
>> I dont think this is related to this...
>>     
> [Pillai, Manikandan] The default EVM support does not have NAND. To build only
> For NAND, you need to enable this. I can put this in a separate patch in a series.
>
>   
my comment -> move this as a seperate patch.. this patch seems to mix up
things doing different things and confusing for a review.
>>> -#if defined(CONFIG_CMD_ONENAND)
>>> +#if defined(CONFIG_CMD_NAND) && defined(CONFIG_ENV_IS_RUNTIME_SEL)
>>> +   gpmc_config = gpmc_m_nand;
>>> +   nand_cs_base = (gpmc_csx_t *)(GPMC_CONFIG_CS0_BASE +
>>> +                   (gpmc_index * GPMC_CONFIG_WIDTH));
>>> +   base = PISMO1_NAND_BASE;
>>> +   size = PISMO1_NAND_SIZE;
>>> +   enable_gpmc_config(gpmc_config, nand_cs_base, base, size);
>>> +   /* NAND and/or ONENAND is to be scanned */
>>> +   is_nand = 0;
>>> +   nand_init();
>>> +   if (nand_info[0].size) {
>>> +           is_nand = 1;
>>> +           f_off = SMNAND_ENV_OFFSET;
>>> +           f_sec = SZ_128K;
>>> +           /* env setup */
>>> +           boot_flash_base = base;
>>> +           boot_flash_off = f_off;
>>> +           boot_flash_sec = f_sec;
>>> +           boot_flash_env_addr = f_off;
>>> +
>>> +           env_name_spec = nand_env_name_spec;
>>> +           env_ptr = nand_env_ptr;
>>> +           env_get_char_spec = nand_env_get_char_spec;
>>> +           env_init = nand_env_init;
>>> +           saveenv = nand_saveenv;
>>> +           env_relocate_spec = nand_env_relocate_spec;
>>> +           gpmc_index++;
>>> +   }
>>>
>>>       
>> with a change like above in a common omap3 file, you are essentially
>> bottlenecking scalability to other OMAP3 platforms which use different
>> NAND. Eg. we assume SZ_128K ;) kinda wrong rt? sector size is dependent
>> on the nand device we plug in..
>>     
> [Pillai, Manikandan] I can plug out the initialization stuff and put it in
> a board dependent file and invoke the same from the common omap3 locations
> for the type of board.
>
>   
that might be a nice idea.. will look for your changes.. :)
>>>     printf("OMAP%s-%s rev %d, CPU-OPP2 L3-165MHz\n", cpu_s,
>>> -          sec_s, get_cpu_rev());
>>> -   printf("%s + %s/%s\n", sysinfo.board_string,
>>> -          mem_s, sysinfo.nand_string);
>>> +           sec_s, get_cpu_rev());
>>> +#if defined(CONFIG_ENV_IS_RUNTIME_SEL)
>>> +           printf("%s + %s/", sysinfo.board_string,
>>> +                   mem_s);
>>> +#if defined(CONFIG_CMD_NAND)
>>> +   if (is_nand)
>>> +           printf("%s\n", "NAND");
>>> +#endif
>>> +#if defined(CONFIG_CMD_ONENAND)
>>> +   if (is_onenand)
>>> +           printf("%s\n", "ONENAND");
>>> +#endif
>>> +#else
>>> +           printf("%s + %s/%s\n", sysinfo.board_string,
>>> +                   mem_s, sysinfo.nand_string);
>>> +#endif
>>>
>>>       
>> I have this feel that we could improve this #ifdef mess.. using
>> variables maybe?
>>     
> [Pillai, Manikandan] other suggestions welcome Personally, I felt
> here the #ifdef is not so dirty.
>   
;) not after a time of adding multiple #ifdefs :D... as I said, how
about using variables to do it?
>>> +#if defined(CONFIG_ENV_IS_RUNTIME_SEL)
>>> +   gpmc_init(); /* in SRAM or SDRAM, finish GPMC */
>>>
>>>       
>> Wooooaaah.. gpmc is TI OMAP2 or OMAP3 only.... NOT in other ARM or PPC
>> or other platforms.. NAK on this.
>>     
> [Pillai, Manikandan] Got your point. But I don't have a solution to this
> Since I EVM is doing a scan,  it requires the gpmc_init to be called late.
> An option is to have another function  board_init_late() which can be used to
> called gpmc_init_late().
>   
that could be an option.. but please keep other platforms in mind when
we send this patch.
Regards,
Nishanth Menon