[U-Boot] [PATCH] Add NanoBone board support

[Mark Jackson]

On 16/04/13 16:48, Tom Rini wrote:
> On Tue, Apr 16, 2013 at 04:02:34PM +0100, Mark Jackson wrote:
>
> [snip]
>> +static struct emif_regs ddr3_emif_reg_data = {
>> +	.sdram_config = MT41J128MJT125_EMIF_SDCFG,
>> +	.ref_ctrl = MT41J128MJT125_EMIF_SDREF,
>> +	.sdram_tim1 = MT41J128MJT125_EMIF_TIM1,
>> +	.sdram_tim2 = MT41J128MJT125_EMIF_TIM2,
>> +	.sdram_tim3 = MT41J128MJT125_EMIF_TIM3,
>> +	.zq_config = MT41J128MJT125_ZQ_CFG,
>> +	.emif_ddr_phy_ctlr_1 = MT41J128MJT125_EMIF_READ_LATENCY,
>
> On the READ_LATENCY, you probably want | PHY_EN_DYN_PWRDN for that
> (since it's not part of MT41J128MJT125_EMIF_READ_LATENCY) bit of
> savings.

Okay, I'd not looked at the DDR setup in great detail, simply copying 
what the EVM-SK did.

>> +static int do_switch_ecc(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>
> Why are we bringing this in?

There's currently no way to enable BCH8 ECC mode.  I need this so that 
all writes to the on-board NAND have the correct ECC values.

There's a similar bunch of code in the OMAP3 board.c file, so I just 
copied that principal.

Ah ... I've just seen this changed recently (don't know why my code even 
compiles now as the headers are different !!).  I'll just back this out 
and use the existing version.

>> +	config_ddr(303, MT41J128MJT125_IOCTRL_VALUE, &ddr3_data,
>> +		   &ddr3_cmd_ctrl_data, &ddr3_emif_reg_data, 0);
>
> And are you unable to run at 400MHz here?  Or just weren't aware /
> having tried updating the timing parameters, etc?

Again, I just copied the EVM-SK code.

DDR3 is a new technology for us, but now the board essentially works I 
should (when I have time !!) be able do some further timing tweaks.

>> +static struct cpsw_platform_data cpsw_data = {
>> +	.mdio_base		= CPSW_MDIO_BASE,
>> +	.cpsw_base		= CPSW_BASE,
>> +	.mdio_div		= 0xff,
>> +	.channels		= 8,
>> +	.cpdma_reg_ofs		= 0x800,
>> +	.slaves			= 2,
>
> Is this all that was needed for both interfaces to work in U-Boot, along
> with a config change or two?  If so, I should take a stab at getting
> both working on the EVM-SK.

Yes ... the config change is just to set CONFIG_PHY_ADDR to the correct 
value.

Unfortunately, since the CONFIG_PHY_ADDR value is "hard-coded" into 
quite a bit of the uboot code, to change ports, you need to re-compile 
!!  Not ideal, and I don't suppose it's too much work to get both
working together, but I don't see much demand for it.

>> +	writel(PORT1_MII_MODE_ENABLE | PORT2_MII_MODE_ENABLE, &cdev->miisel);
>> +	cpsw_slaves[0].phy_if = cpsw_slaves[1].phy_if = PHY_INTERFACE_MODE_MII;
>
> Can't you just do that in the struct?  And avoid the checkpatch warning
> too.

Sure, will do.

>> +#define CONFIG_SYS_NO_FLASH
>
> But you have NOR.  Is it not working yet?

We do have NOR and it's working, but it doesn't contain any "user 
accessible" data.  We use it as long-term storage using a custom layout, 
so there's no point in exposing it to uboot.

>> +/*
>> + * memtest works on 8 MB in DRAM after skipping 32MB from
>> + * start addr of ram disk
>> + */
>> +#define CONFIG_SYS_MEMTEST_START	(PHYS_DRAM_1 + (64 * 1024 * 1024))
>> +#define CONFIG_SYS_MEMTEST_END		(CONFIG_SYS_MEMTEST_START \
>> +					+ (8 * 1024 * 1024))
>
> Please see doc/README.memory-test and then eiter update the range to be
> most of memory or drop these and add #undef CONFIG_CMD_MEMTEST.

I'll just drop memtest.

>> +#define CONFIG_SYS_HZ			1000 /* 1ms clock */
> [snip]
>> +#define CONFIG_SYS_HZ			1000
>
> I see this is a copy/paste problem too from am335x_evm.h, can you fix
> there and pcm051.h?

Sure, I'll post a "remove duplicate #defines" patch.

>> +#define CONFIG_SERIAL_MULTI
>
> Same (not needed anymore, not sure how it didn't get removed before).

Ok.

>> +#define CONFIG_CMD_EEPROM
>> +#define CONFIG_SYS_I2C_EEPROM_ADDR	0x50	/* Main EEPROM */
>> +#define CONFIG_SYS_I2C_EEPROM_ADDR_LEN	2
>> +#define CONFIG_SYS_I2C_MULTI_EEPROMS
>
> Do you really have the EEPROM at 0x50 like the TI boards do (and then at
> other offets for capes/daughter cards) ?

Yes, but it doesn't contain the same TI EEPROM data structure.  In fact, 
at the moment, it doesn't contain anything (!!).

It's just there for future expansion .... in which case, should I remove 
the #defines for the time being ?

>> +#define	CONFIG_SYS_NAND_ECCTOTAL	(CONFIG_SYS_NAND_ECCBYTES * \
>> +						CONFIG_SYS_NAND_ECCSTEPS)
>> +
>> +#define	CONFIG_SYS_NAND_U_BOOT_START	CONFIG_SYS_TEXT_BASE
>
> Another problem duplicated from am335x_evm.h :(

I'll add it to the "remove duplicates" patch.

>> +#define MTDPARTS_DEFAULT		"mtdparts=nand:" \
>> +					"128k(spl)," \
>> +					"1792k(boot)," \
>
> This isn't quite right.  Your first 128k chunks are ROM-checked
> redundant locations for SPL to live (hence the write of U-Boot at 512k
> in).

Ah, I see ... so that's part of on-chip "bios" ... nice!!

I'll put them back in.

>> +#undef CONFIG_ENV_IS_NOWHERE
>
> Did you ever set this?  I don't think so... on am335x_evm.h we do since
> we can build for SPI env or NAND env.

Okay.

> All that said, yay for another board to try and help flush out generic
> problems in am33xx land!

Well, this board certainly knock the socks off it's predecessor (based 
on the AVR32).  We're getting a 10-14x performance boost :-)

Thanks for the review ... I'll post a v2 shortly.

Mark J.