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.
+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,5200@0" +#define OF_SOC "soc5200@f0000000" +#define OF_TBCLK (bd->bi_busfreq / 4) +#define OF_STDOUT_PATH "/soc5200@f0000000/serial@2600"
Do we really need this?
Best regards,
Wolfgang Denk