Wolfgang Denk wrote:
> Dear Grzegorz,
>
> In message <12331552753467-git-send-email-...@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,5...@0"
>> +#define OF_SOC "soc5...@f0000000"
>> +#define OF_TBCLK (bd->bi_busfreq / 4)
>> +#define OF_STDOUT_PATH "/soc5...@f0000000/ser...@2600"
>
> Do we really need this?
OF_CPU, OF_SOC and OF_TBCLK is used and I removed OF_STDOUT_PATH.
regards,
Grzesiek
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
