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

[Tom Rini]

On 10/27/2011 02:18 PM, Igor Grinberg wrote:
> On 10/26/2011 11:13 PM, Tom Rini wrote:
>> 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.
> 
> You have two spaces between the sentences, why is that?

Old habit.

>> Beagleboard rev C5, xM rev A:
>> Tested-by: Tom Rini <trini at ti.com>
>> Beagleboard xM rev C:
>> Tested-by: Matt Ranostay <mranostay at gmail.com>
>> Beagleboard rev B7, C2, xM rev B:
>> Tested-by: Matt Porter <mporter at ti.com>
>> Signed-off-by: Tom Rini <trini at ti.com>
>> ---
>>  arch/arm/include/asm/arch-omap3/mem.h |   24 +++++
>>  board/ti/beagle/beagle.c              |  160 ++++++++++++++++++++++++++++++++-
>>  board/ti/beagle/config.mk             |   33 -------
>>  include/configs/omap3_beagle.h        |   60 ++++++++++++-
>>  4 files changed, 242 insertions(+), 35 deletions(-)
>>  delete mode 100644 board/ti/beagle/config.mk
> 
> config.mk removal does not belong to that patch...
> It should be a separate one, say cleanup patch.

I'll see if I can restructure things and kill that off first then.

> 
>> diff --git a/arch/arm/include/asm/arch-omap3/mem.h b/arch/arm/include/asm/arch-omap3/mem.h
>> index af3504c..a784813 100644
>> --- a/arch/arm/include/asm/arch-omap3/mem.h
>> +++ b/arch/arm/include/asm/arch-omap3/mem.h
>> @@ -171,6 +171,30 @@ enum {
>>  #define MICRON_V_MR ((MICRON_WBST << 9) | (MICRON_CASL << 4) | \
>>  	(MICRON_SIL << 3) | (MICRON_BL))
>>  
>> +
>> +/* Micron part (200MHz optimized) 5 ns
>> +  */
>> +#define MICRON_TDAL_200   6
>> +#define MICRON_TDPL_200   3
>> +#define MICRON_TRRD_200   2
>> +#define MICRON_TRCD_200   3
>> +#define MICRON_TRP_200    3
>> +#define MICRON_TRAS_200   8
>> +#define MICRON_TRC_200   11
>> +#define MICRON_TRFC_200  15
>> +#define MICRON_V_ACTIMA_200 ((MICRON_TRFC_200 << 27) | (MICRON_TRC_200 << 22) | (MICRON_TRAS_200 << 18) \
>> +		| (MICRON_TRP_200 << 15) | (MICRON_TRCD_200 << 12) |(MICRON_TRRD_200 << 9) | \
>> +		(MICRON_TDPL_200 << 6) | (MICRON_TDAL_200))
> 
> MICRON_TDAL_200 does not need parenthesis.

As I said to Sanjeev this is all yanked right from X-loader so yes,
there's spacing and so on problems (and I swore git send-email --subject
modified subject for all patches, but I guess not).

[snip]
>> +#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)
> 
> This is definitely needs a cleanup...
> Consider Sanjeev's proposal. If it will not work for some reason,
> you need at least to use writel() readl() io accessors.

At first pass you can't call gpmc_init() as-is this early, but I'll
switch over to readl/writel as a start.

> 
>> +
>> +/* nand_command: Send a flash command to the flash chip */
>> +static void nand_command(unsigned char command)
>> +{
>> + 	WRITE_NAND_COMMAND(command, NAND_ADDR);
>> +
>> +  	if (command == NAND_CMD_RESET) {
>> +		unsigned char ret_val;
>> +		nand_command(NAND_CMD_STATUS);
>> +		do {
>> +			ret_val = READ_NAND(NAND_ADDR);/* wait till ready */
>> +  		} while ((ret_val & 0x40) != 0x40);
> 
> You should be using some kind of timeout, so you will not stuck in here
> without being noticed.

OK.  I've been wondering if we shouldn't somehow make a
not-tied-to-full-mtd nand_command more available since I suspect a few
other boards will be in a similar situation, for probing early on.

[snip]
>> +	*mr = MICRON_V_MR;
>> +	switch (get_board_revision()) {
>> +	case REVISION_C4:
>> +		if (identify_xm_ddr() == NUMONYX_MCP) {
>> +			*cs_cfg = 0x4;
>> +			*mcfg = 0x04590099;
>> +			*ctrla = NUMONYX_V_ACTIMA_165;
>> +			*ctrlb = NUMONYX_V_ACTIMB_165;
>> +			*rfr_ctrl = SDP_3430_SDRC_RFR_CTRL_165MHz;
> 
> aligning the assignments above (and below) will make it much more readable

I'm really not a fan of spacing out assignments but I'll see what
CodingSytle says.

[snip]
>> -#ifdef CONFIG_GENERIC_MMC
>> +#if defined(CONFIG_GENERIC_MMC) && !defined(CONFIG_SPL_BUILD)
> 
> This should be another patch.

Really?  I'm adding SPL support here and that's what introduces the need
to not build this on just CONFIG_GENERIC_MMC.

[snip]
>> +#define CONFIG_SPL_BSS_START_ADDR	0x80000000
>> +#define CONFIG_SPL_BSS_MAX_SIZE		0x80000		/* 512 KB */
> 
> alignment

I'll double check but all of them but this one is fine once applied.

Thanks.

-- 
Tom