[U-Boot] [PATCH 2/2] MPC8308ERDB: minimal support for devboard from Freescale

[Ilya Yanok <yanok@emcraft.com>]

Dear Wolfgang,

On 21.06.2010 11:44, Wolfgang Denk wrote:
>>   MAKEALL                                   |    1 +
>>   Makefile                                  |    3 +
>>   board/freescale/mpc8308erdb/Makefile      |   52 +++
>>   board/freescale/mpc8308erdb/config.mk     |    1 +
>>   board/freescale/mpc8308erdb/mpc8308erdb.c |  154 ++++++++
>>   board/freescale/mpc8308erdb/sdram.c       |  126 +++++++
>>   include/configs/MPC8308ERDB.h             |  572 +++++++++++++++++++++++++++++
>>   7 files changed, 909 insertions(+), 0 deletions(-)
>>   create mode 100644 board/freescale/mpc8308erdb/Makefile
>>   create mode 100644 board/freescale/mpc8308erdb/config.mk
>>   create mode 100644 board/freescale/mpc8308erdb/mpc8308erdb.c
>>   create mode 100644 board/freescale/mpc8308erdb/sdram.c
>>   create mode 100644 include/configs/MPC8308ERDB.h
>>      
> Entry to MAINTAINERS missing.
>    

Should I add you as a maintainer or myself?

>> diff --git a/Makefile b/Makefile
>> index 55bb964..0dc2678 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2233,6 +2233,9 @@ TASREG_config :		unconfig
>>   kmeter1_config: unconfig
>>   	@$(MKCONFIG) kmeter1 powerpc mpc83xx kmeter1 keymile
>>
>> +MPC8308ERDB_config: unconfig
>> +	@$(MKCONFIG) -a MPC8308ERDB powerpc mpc83xx mpc8308erdb freescale
>>      
> NAK. Please rebase your code against the "next" branch. We don't
> accept any board entries to the top level Makefile any more. Please
> add to boards.cfg instead.
>    

Done.

>> --- /dev/null
>> +++ b/board/freescale/mpc8308erdb/mpc8308erdb.c
>> @@ -0,0 +1,154 @@
>> +/*
>> + * Copyright (C) 2010 Freescale Semiconductor, Inc.
>> + * Copyright (C) 2010 Ilya Yanok, Emcraft Systems, yanok at emcraft.com
>> + *
>> + * Author: Freescale unknown
>>      
> Maybe "Initial author" ?
>    

Dropped this line.

>> +int board_early_init_f(void)
>> +{
>> +	immap_t *im = (immap_t *)CONFIG_SYS_IMMR;
>> +
>> +	if (in_be32(&im->pmc.pmccr1)&  PMCCR1_POWER_OFF)
>> +		gd->flags |= GD_FLG_SILENT;
>>      
> What exactly is this good for?
>    

That's for making board silent then it comes out of sleep (not printing 
version info, checkcpu() output and so on). Actually I've not tested 
sleep/wakeup functionality but the code looks correct.

>> +/*
>> + * Miscellaneous late-boot configurations
>> + *
>> + * If a VSC7385 microcode image is present, then upload it.
>> +*/
>> +int misc_init_r(void)
>> +{
>> +	int rc = 0;
>>      
> Please drop the variable.
>    

Done.

>> +#ifdef CONFIG_VSC7385_IMAGE
>> +	if (vsc7385_upload_firmware((void *) CONFIG_VSC7385_IMAGE,
>> +		CONFIG_VSC7385_IMAGE_SIZE)) {
>> +		puts("Failure uploading VSC7385 microcode.\n");
>> +		rc = 1;
>>      
> 	return 1;
>
>    
>> +	}
>> +#endif
>> +
>> +	return rc;
>>      
> 	return 0;
>
>    
>> +int board_eth_init(bd_t *bis)
>> +{
>> +	cpu_eth_init(bis);	/* Initialize TSECs first */
>>      
> I think it's wrong to ignore the return code here.
>    

What makes you think so? What can we do with the return code here? Print 
warning? If we return error from board_eth_init() calling code will call 
cpu_eth_init() again which is useless as we have already called it.

>> +	return pci_eth_init(bis);
>> +}
>> +
>> +
>>      
> Please remove trailing empty lines.
>    

Fixed.

>> +/* Fixed sdram init -- doesn't use serial presence detect.
>> + *
>> + * This is useful for faster booting in configs where the RAM is unlikely
>> + * to be changed, or for things like NAND booting where space is tight.
>> + */
>> +static long fixed_sdram(void)
>> +{
>> +	immap_t *im = (immap_t *)CONFIG_SYS_IMMR;
>> +	u32 msize = CONFIG_SYS_DDR_SIZE * 1024 * 1024;
>> +	u32 msize_log2 = __ilog2(msize);
>> +
>> +	out_be32(&im->sysconf.ddrlaw[0].bar,
>> +			CONFIG_SYS_DDR_SDRAM_BASE&  0xfffff000);
>> +	out_be32(&im->sysconf.ddrlaw[0].ar, LBLAWAR_EN | (msize_log2 - 1));
>> +	out_be32(&im->sysconf.ddrcdr, CONFIG_SYS_DDRCDR_VALUE);
>> +
>> +	/*
>> +	 * Erratum DDR3 requires a 50ms delay after clearing DDRCDR[DDR_cfg],
>> +	 * or the DDR2 controller may fail to initialize correctly.
>> +	 */
>> +	udelay(50000);
>> +
>> +	out_be32(&im->ddr.csbnds[0].csbnds, (msize - 1)>>  24);
>> +	out_be32(&im->ddr.cs_config[0], CONFIG_SYS_DDR_CS0_CONFIG);
>> +
>> +	/* Currently we use only one CS, so disable the other bank. */
>> +	out_be32(&im->ddr.cs_config[1], 0);
>> +
>> +	out_be32(&im->ddr.sdram_clk_cntl, CONFIG_SYS_DDR_SDRAM_CLK_CNTL);
>> +	out_be32(&im->ddr.timing_cfg_3, CONFIG_SYS_DDR_TIMING_3);
>> +	out_be32(&im->ddr.timing_cfg_1, CONFIG_SYS_DDR_TIMING_1);
>> +	out_be32(&im->ddr.timing_cfg_2, CONFIG_SYS_DDR_TIMING_2);
>> +	out_be32(&im->ddr.timing_cfg_0, CONFIG_SYS_DDR_TIMING_0);
>> +
>> +	if (in_be32(&im->pmc.pmccr1)&  PMCCR1_POWER_OFF) {
>> +		out_be32(&im->ddr.sdram_cfg,
>> +			CONFIG_SYS_DDR_SDRAM_CFG | SDRAM_CFG_BI);
>> +	} else {
>> +		out_be32(&im->ddr.sdram_cfg, CONFIG_SYS_DDR_SDRAM_CFG);
>> +	}
>> +
>> +	out_be32(&im->ddr.sdram_cfg2, CONFIG_SYS_DDR_SDRAM_CFG2);
>> +	out_be32(&im->ddr.sdram_mode, CONFIG_SYS_DDR_MODE);
>> +	out_be32(&im->ddr.sdram_mode2, CONFIG_SYS_DDR_MODE2);
>> +
>> +	out_be32(&im->ddr.sdram_interval, CONFIG_SYS_DDR_INTERVAL);
>> +	sync();
>> +
>> +	/* enable DDR controller */
>> +	setbits_be32(&im->ddr.sdram_cfg, SDRAM_CFG_MEM_EN);
>> +	sync();
>> +
>> +	return msize;
>>      
> Please test RAM and verify the size using get_mem_size().
>    

Done.

>> +
>> +/*
>> + * Hardware Reset Configuration Word
>> + * if CLKIN is 66.66MHz, then
>> + * CSB = 133MHz, DDRC = 266MHz, LBC = 133MHz
>> + * We choose the A type silicon as default, so the core is 400Mhz.
>> + */
>> +#define CONFIG_SYS_HRCW_LOW (\
>> +	HRCWL_LCL_BUS_TO_SCB_CLK_1X1 |\
>> +	HRCWL_DDR_TO_SCB_CLK_2X1 |\
>> +	HRCWL_SVCOD_DIV_2 |\
>> +	HRCWL_CSB_TO_CLKIN_4X1 |\
>> +	HRCWL_CORE_TO_CSB_3X1)
>> +/*
>> + * There are neither HRCWH_PCI_HOST nor HRCWH_PCI1_ARBITER_ENABLE bits
>> + * in 8308's HRCWH according to the manual, but original Freescale's
>> + * code has them and I've expirienced some problems using the board
>> + * with BDI3000 attached when I've tried to set these bits to zero
>> + * (UART doesn't work after the 'reset run' command).
>> + */
>> +#define CONFIG_SYS_HRCW_HIGH (\
>> +	HRCWH_PCI_HOST |\
>> +	HRCWH_PCI1_ARBITER_ENABLE |\
>> +	HRCWH_CORE_ENABLE |\
>> +	HRCWH_FROM_0X00000100 |\
>> +	HRCWH_BOOTSEQ_DISABLE |\
>> +	HRCWH_SW_WATCHDOG_DISABLE |\
>> +	HRCWH_ROM_LOC_LOCAL_16BIT |\
>> +	HRCWH_RL_EXT_LEGACY |\
>> +	HRCWH_TSEC1M_IN_RGMII |\
>> +	HRCWH_TSEC2M_IN_RGMII |\
>> +	HRCWH_BIG_ENDIAN)
>>      
>
> Kim, can you please comment?
>
>
>    
>> +#include<config_cmd_default.h>
>> +
>> +#define CONFIG_CMD_PING
>> +#define CONFIG_CMD_DHCP
>> +#define CONFIG_CMD_NET
>> +#define CONFIG_CMD_I2C
>> +#define CONFIG_CMD_MII
>> +#define CONFIG_CMD_DATE
>> +#define CONFIG_CMD_PCI
>>      
> Please sort list.
>    

Fixed.

I'l wait for a while for other comments and then repost the updated patches.

Regards, Ilya.