[U-Boot] [PATCH] Add support for the digsy MTC board.

[Grzegorz Bernacki <gjb@semihalf.com>]

Wolfgang Denk wrote:
> Dear Grzegorz,
> 
> In message <12331552753467-git-send-email-gjb@semihalf.com> you wrote:
>> This is the InterControl custom device based on the MPC5200B chip.
> ...
> 
> General comment: there is a lot of code which could use a few comments
> so the reader has a chance to understand what exactly you are doing.
> 
>> --- /dev/null
>> +++ b/board/digsymtc/digsymtc.c
> ...
>> +static void sdram_start(int hi_addr)
>> +{
>> +	long hi_addr_bit = hi_addr ? 0x01000000 : 0;
>> +	long control = SDRAM_CONTROL | hi_addr_bit;
>> +
>> +	/* unlock mode register */
>> +	*(vu_long *)MPC5XXX_SDRAM_CTRL = control | 0x80000000;
>> +	__asm__ volatile ("sync");
>> +
>> +	/* precharge all banks */
>> +	*(vu_long *)MPC5XXX_SDRAM_CTRL = control | 0x80000002;
>> +	__asm__ volatile ("sync");
>> +
>> +	/* auto refresh */
>> +	*(vu_long *)MPC5XXX_SDRAM_CTRL = control | 0x80000004;
>> +	__asm__ volatile ("sync");
>> +
>> +	/* set mode register */
>> +	*(vu_long *)MPC5XXX_SDRAM_MODE = SDRAM_MODE;
>> +	__asm__ volatile ("sync");
>> +
>> +	/* normal operation */
>> +	*(vu_long *)MPC5XXX_SDRAM_CTRL = control;
>> +	__asm__ volatile ("sync");
> 
> Please use proper I/O accessor functions instead of volatile pointer
> accesses, here and in all other similar places as well...
> 
> You can then get rid of the "sync"s as well.
> 

Ok, I changed it in whole file.

>> +phys_size_t initdram(int board_type)
>> +{
>> +	ulong dramsize = 0;
>> +	ulong dramsize2 = 0;
>> +	uint svr, pvr;
>> +#ifndef CONFIG_SYS_RAMBOOT
>> +	ulong test1, test2;
>> +
>> +	/* setup SDRAM chip selects */
>> +	*(vu_long *)MPC5XXX_SDRAM_CS0CFG = 0x0000001C;	/* 512MB at 0x0 */
>> +	*(vu_long *)MPC5XXX_SDRAM_CS1CFG = 0x80000000;	/* disabled */
> 
> ... like here and below.
> 
>> +	if (s != NULL) { puts(", "); puts(s); }
> 
> Please use proper indentation.
> 

ok, done

>> +void set_ethaddr(void)
> 
> Please add "static" here.
> 

ok, done 

>> +int last_stage_init (void)
>> +{
>> +	set_ethaddr();
>> +	return 0;
> 
> You should do this only as long as "ethaddr" is not already set in the
> environment.
> 

ok, done

>> --- /dev/null
>> +++ b/board/digsymtc/eeprom.h
> ...
>> +
>> +#define	EEPROM_ADDR		CONFIG_SYS_I2C_EEPROM_ADDR
>> +#define	EEPROM_LEN		1024
>> +#define	EEPROM_IDENT		2408
>> +#define	EEPROM_ADDR_IDENT	0x0000
>> +#define	EEPROM_ADDR_LEN_SYS	0x0002
>> +#define	EEPROM_ADDR_LEN_SYSCFG	0x0004
>> +#define	EEPROM_OFF_ETHADDR	23
> 
> You want to add some comments what all these magic defines mean...
> 

comments added

>> diff --git a/cpu/mpc5xxx/ide.c b/cpu/mpc5xxx/ide.c
>> index df5b4ac..07d33e3 100644
>> --- a/cpu/mpc5xxx/ide.c
>> +++ b/cpu/mpc5xxx/ide.c
>> @@ -12,7 +12,7 @@
>>   *
>>   * This program is distributed in the hope that it will be useful,
>>   * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>   * GNU General Public License for more details.
>>   *
>>   * You should have received a copy of the GNU General Public License
> 
> Please don't arbitrarily reformat the code.
> 

ok, I restored original layout

>> --- /dev/null
>> +++ b/include/configs/digsymtc.h
> ...
>> +#define CONFIG_PSC_CONSOLE	4	/* console is on PSC4  */
> ...
>> +	"console=ttyPSC1\0"				\
> 
> This looks fishy to me - is it PSC1 or PSC4 ?
> 

Linux enumerates ttyPSC starting from zero, so PSC3 is ttyPSC0 and
ttyPSC4 is ttyPSC4. Some time ago ttyPSC number could be enforced
by setting port-number field in dts, but lately it was removed.
 
> 
>> +#define OF_CPU			"PowerPC,5200 at 0"
>> +#define OF_SOC			"soc5200 at f0000000"
>> +#define OF_TBCLK		(bd->bi_busfreq / 4)
>> +#define OF_STDOUT_PATH		"/soc5200 at f0000000/serial at 2600"
> 
> Do we really need this?

OF_CPU, OF_SOC and OF_TBCLK is used and I removed OF_STDOUT_PATH.

regards,
Grzesiek