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.
> +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.
> +void set_ethaddr(void)
Please add "static" here.
> +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.
> --- /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...
> 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.
> --- /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 ?
> +#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?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
All he had was nothing, but that was something, and now it had been
taken away. - Terry Pratchett, _Sourcery_
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
