[U-Boot] [PATCH 2/2] OMAP3: Add SPL support to Beagleboard

[Tom Rini <trini@ti.com>]

On 10/27/2011 05:46 AM, Premi, Sanjeev wrote:
>> -----Original Message-----
>> From: u-boot-bounces at lists.denx.de 
>> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Rini, Tom
>> Sent: Thursday, October 27, 2011 2:44 AM
>> To: u-boot at lists.denx.de
>> Subject: [U-Boot] [PATCH 2/2] OMAP3: Add SPL support to Beagleboard
>>
>> This introduces 200MHz Micron parts timing information based 
>> on x-loader
>> and re-organizes the file slightly for grouping.  The memory 
>> init logic
>> is also based on what x-loader does in these cases.  Note that while
>> previously u-boot would be flashed in with SW ECC in this case it now
>> must be flashed with HW ECC.
>>
>> Beagleboard rev C5, xM rev A:
>> Tested-by: Tom Rini <trini@ti.com>
>> Beagleboard xM rev C:
>> Tested-by: Matt Ranostay <mranostay@gmail.com>
>> Beagleboard rev B7, C2, xM rev B:
>> Tested-by: Matt Porter <mporter@ti.com>
>> Signed-off-by: Tom Rini <trini@ti.com>
>> ---
> 
> [snip]...[snip]
> 
>>  
>> +#ifdef CONFIG_SPL_BUILD
>> +
>> +#define MICRON_DDR	0
>> +#define NUMONYX_MCP	1
>> +#define MICRON_MCP	2
>> +
>> +#define NAND_CMD_STATUS		0x70
>> +#define NAND_CMD_READID		0x90
>> +#define NAND_CMD_RESET		0xff
>> +
>> +#define GPMC_NAND_COMMAND_0      (OMAP34XX_GPMC_BASE+0x7C)
>> +#define GPMC_NAND_ADDRESS_0      (OMAP34XX_GPMC_BASE+0x80)
>> +#define GPMC_NAND_DATA_0	 (OMAP34XX_GPMC_BASE+0x84)
>> +
>> +#define WRITE_NAND_COMMAND(d, adr) \
>> +	do {*(volatile u16 *)GPMC_NAND_COMMAND_0 = d;} while(0)
>> +#define WRITE_NAND_ADDRESS(d, adr) \
>> +	do {*(volatile u16 *)GPMC_NAND_ADDRESS_0 = d;} while(0)
>> +#define READ_NAND(adr)          (*(volatile u16 *)GPMC_NAND_DATA_0)
>> +
> 
> I am not yet familiar with SPL code, but I would suggest using
> "struct gpmc" instead of hardcoded offsets above.
> 
> [snip]...[snip]
> 
>> +
>> +#define GPMC_CONFIG_CS0_CONFIG1		0x6E000060
>> +#define GPMC_CONFIG_CS0_CONFIG2		0x6E000064
>> +#define GPMC_CONFIG_CS0_CONFIG3		0x6E000068
>> +#define GPMC_CONFIG_CS0_CONFIG4		0x6E00006C
>> +#define GPMC_CONFIG_CS0_CONFIG5		0x6E000070
>> +#define GPMC_CONFIG_CS0_CONFIG6		0x6E000074
>> +#define GPMC_CONFIG_CS0_CONFIG7		0x6E000078
>> +#define OMAP34XX_GPMC_CS0_SIZE		0x8
> 
> Suggest using "struct gpmc_cs" instead of defining offsets.
> 
> [snip]...[snip]
> 
>> +
>> +static int identify_xm_ddr(void)
>> +{
>> +	int mfr, id;
>> +
>> +	/* Make sure that we have setup GPMC for NAND correctly. */
>> +	writel(M_NAND_GPMC_CONFIG1, GPMC_CONFIG_CS0_CONFIG1);
>> +	writel(M_NAND_GPMC_CONFIG2, GPMC_CONFIG_CS0_CONFIG2);
>> +	writel(M_NAND_GPMC_CONFIG3, GPMC_CONFIG_CS0_CONFIG3);
>> +	writel(M_NAND_GPMC_CONFIG4, GPMC_CONFIG_CS0_CONFIG4);
>> +	writel(M_NAND_GPMC_CONFIG5, GPMC_CONFIG_CS0_CONFIG5);
>> +	writel(M_NAND_GPMC_CONFIG6, GPMC_CONFIG_CS0_CONFIG6);
>> +
>> +	/* Enable the GPMC Mapping */
>> +	writel((((OMAP34XX_GPMC_CS0_SIZE & 0xF) << 8) |
>> +			     ((NAND_BASE >> 24) & 0x3F) |
>> +			     (1 << 6)),  (GPMC_CONFIG_CS0_CONFIG7));
> 
> Same comment as before. Looks like there may be few more places
> where hardcoded addresses/ offsets are being used.

Yeah, this is all a drop-in of the x-loader code which needs some clean
up.  But the overall logic (poke this to determine that) is what I want
to make sure we're going to be OK with.

-- 
Tom