[U-Boot] [PATCH 08/12] OMAP3 SPL: Add identify_pop_memory function

[Igor Grinberg <grinberg@compulab.co.il>]

Hi Tom,

On 11/08/11 17:21, Tom Rini wrote:
> On 11/08/2011 12:45 AM, Igor Grinberg wrote:
>> On 11/07/11 22:05, Tom Rini wrote:
>>> A number of boards are populated with a PoP chip for both DDR and NAND
>>> memory.  So to determine DDR timings the NAND chip needs to be probed
>>> and mfr/id returned to the board to make decisions with.  All of this
>>> code is put into spl_pop_probe.c and controlled via
>>> CONFIG_SPL_OMAP3_POP_PROBE.
>>
>> I don't see how POP is different from other types of packages
>> in terms of DRAM.
>> The same thing can be true also for non-POP packages.
>> What I'm saying here is, I understand the necessity of that code,
>> but why call it POP specific?
>> If it is not POP specific, then please call it some other way
>> (e.g. ...DRAM_NAND_PROBE).
>> Also, hypothetically, some other means can be used for DRAM type
>> identification, so it will be a good thing to split it, but again
>> it is only hypothetically and it is only my thoughts, so you don't
>> have to...
> 
> Well, that gets at why I called it spl_pop_probe.  If you have a POP
> package, this is how you would do the probe.  I can see in theory
> wanting to probe NAND as a way to see board rev on a non-POP package,
> but I'd like to see a real example first.

That's the problem we see in Linux OMAP...
some guys don't think forward and submit stuff on a per case basis,
then when it comes to a bit different case,
they are trying to reuse (which is fine) and end up renaming stuff
all around - generating huge diff stat.
Why not just do the generic stuff from the start of it?
Why wait for a new case?
It is pretty simple, just don't name it POP, so it can serve w/o
any confusion for more cases.

> 
>>> diff --git a/arch/arm/cpu/armv7/omap3/Makefile b/arch/arm/cpu/armv7/omap3/Makefile
>>> index 8e85891..772f3d4 100644
>>> --- a/arch/arm/cpu/armv7/omap3/Makefile
>>> +++ b/arch/arm/cpu/armv7/omap3/Makefile
>>> @@ -31,6 +31,9 @@ COBJS	+= board.o
>>>  COBJS	+= clock.o
>>>  COBJS	+= mem.o
>>>  COBJS	+= sys_info.o
>>> +ifdef CONFIG_SPL_BUILD
>>> +COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE)	+= spl_pop_probe.o
>>> +endif
>>
>> Can't CONFIG_SPL_OMAP3_..._PROBE symbol default to "no"
>> and depend on CONFIG_SPL_BUILD, so you don't need to enclose
>> it in #ifdef?
> 
> But then it would build for both SPL and non-SPL cases.

No, it should not.
What do you think of the following:
In the Makefile have only:
COBJS-$(CONFIG_SPL_OMAP3_POP_PROBE)	+= spl_pop_probe.o

Then in the spl_pop_probe.c have this type of check:
#ifndef CONFIG_SPL_BUILD
# error CONFIG_SPL_OMAP3_POP_PROBE requires CONFIG_SPL_BUILD
#endif

This way, you require the CONFIG_SPL_OMAP3_POP_PROBE symbol
be a part of the CONFIG_SPL_BUILD symbols group.

> 
> [snip]
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program; if not, write to the Free Software
>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>> + * MA 02111-1307 USA
>>
>> The address is subject to change so probably it will be
>> a good thing to drop the address part (but leave the rest).
> 
> Just following existing examples.

Existing examples can be wrong and as Wolfgang said, it is not a
good justification, but I don't really mind, just trying to raise
people awareness.

> 
>>
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <linux/mtd/nand.h>
>>> +#include <asm/io.h>
>>> +#include <asm/arch/sys_proto.h>
>>> +#include <asm/arch/mem.h>
>>> +
>>> +#ifdef CONFIG_SPL_BUILD
>>
>> no need for this #ifdef, the whole file compilation depends
>> on that symbol being defined.
> 
> True, will fix, thanks.
> 
>>
>>> +static struct gpmc *gpmc_config = (struct gpmc *)GPMC_BASE;
>>> +
>>> +/* nand_command: Send a flash command to the flash chip */
>>> +static void nand_command(u8 command)
>>> +{
>>> +	writeb(command, &gpmc_config->cs[0].nand_cmd);
>>> +
>>> +	if (command == NAND_CMD_RESET) {
>>> +		unsigned char ret_val;
>>> +		nand_command(NAND_CMD_STATUS);
>>
>> This recursion looks redundant.
>> Why not just replace it with:
>> writeb(NAND_CMD_STATUS, &gpmc_config->cs[0].nand_cmd);
> 
> OK, thanks.
> 
>>> +		do {
>>> +			/* Wait until ready */
>>> +			ret_val = readl(&gpmc_config->cs[0].nand_dat);
>>> +		} while ((ret_val & 0x40) != 0x40);
>>
>> Can't 0x40 magic be defined to have some more understandable name?
>> Probably kind of NAND_CMD_READY mask?
> 
> I'll see if the datasheet defines this particular bit of magic.
> 
>>
>>> +	}
>>> +}
>>> +
>>> +/*
>>> + * Many boards ship with a PoP chip of both NAND and DDR, so we need
>>> + * probe the NAND side, very earily, to see what it says and pass this
>>
>> s/earily/early/
> 
> Thanks.
> 
>>> + * along to the board.  The board code will then use this information
>>> + * to decide what DDR timings to use.
>>> + */
>>> +void identify_pop_memory(int *mfr, int *id)
>>> +{
>>> +	/* Make sure that we have setup GPMC for NAND correctly. */
>>> +	writel(M_NAND_GPMC_CONFIG1, &gpmc_config->cs[0].config1);
>>> +	writel(M_NAND_GPMC_CONFIG2, &gpmc_config->cs[0].config2);
>>> +	writel(M_NAND_GPMC_CONFIG3, &gpmc_config->cs[0].config3);
>>> +	writel(M_NAND_GPMC_CONFIG4, &gpmc_config->cs[0].config4);
>>> +	writel(M_NAND_GPMC_CONFIG5, &gpmc_config->cs[0].config5);
>>> +	writel(M_NAND_GPMC_CONFIG6, &gpmc_config->cs[0].config6);
>>> +
>>> +	/* Enable the GPMC Mapping */
>>> +	writel((((GPMC_SIZE_128M & 0xF) << 8) | ((NAND_BASE >> 24) & 0x3F) |
>>> +				(1 << 6)), &gpmc_config->cs[0].config7);
>>> +
>>> +	sdelay(2000);
>>> +
>>> +	/* Issue a RESET and then READID */
>>> +	nand_command(NAND_CMD_RESET);
>>> +	nand_command(NAND_CMD_READID);
>>> +
>>> +	writeb(0x0, &gpmc_config->cs[0].nand_adr);
>>
>> It would be nice to have a comment, why the above is needed.
> 
> OK.
> 
> --
> Tom
> 

-- 
Regards,
Igor.