From: Wolfgang Denk <wd@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) board support
Date: Mon, 27 Nov 2006 16:49:01 +0100 [thread overview]
Message-ID: <20061127154901.CE3E7353B57@atlas.denx.de> (raw)
In-Reply-To: Your message of "27 Nov 2006 10:12:33 +0800." <1164593553.28194.2.camel@localhost.localdomain>
Hello,
in message <1164593553.28194.2.camel@localhost.localdomain> you wrote:
>
> How about the progress of mpc7448hpc2 board in u-boot?
I had a look at your code.
I see a few problems:
1) it does not merge cleanly with the current top of tree in the git
repo; there are conflicts with common/cfi_flash.c
2) there are lots of coding style violations: C++ comments in
drivers/tsi108_i2c.cand include/configs/mpc7448hpc2.h; trailing
white space in board/mpc7448hpc2/asm_init.S,
board/mpc7448hpc2/mpc7448hpc2.c, board/mpc7448hpc2/tsi108_init.c,
cpu/74xx_7xx/cpu.c, cpu/74xx_7xx/speed.c, doc/README.mpc7448hpc2,
drivers/tsi108_i2c.c, include/configs/mpc7448hpc2.h,
include/tsi108.h; indentation not by TABs at least in
board/mpc7448hpc2/asm_init.S, include/configs/mpc7448hpc2.h;
trailing empty lines in doc/README.mpc7448hpc2.
Note that these are just examples; the same problems may be present
in other files as well. Please check ALL your code.
Also, you don't place spaces as reqyired by the Coding Style (for
example, we normally have spaces before the '(' of a function
call).
Also, the Coding Style discourages using '{ ... }' for simple
one-line conditionals.
3) Your Makefiles don't support building in a separate directory
4) Indentation looks not nice. For example, in
board/mpc7448hpc2/asm_init.S we have:
+ mfspr r3,1014 /* read MSSCR0 */
+ rlwinm. r3,r3,27,31,31 /* get processor ID number */
+ mtspr SPRN_PIR,r3 /* Save CPU ID */
+ sync
+ bne init_done
+ b do_tsi108_init
This is not indented by TAB's, and why don;t you align the
arguments of the "bne" and "b" instructions like the rest?
Or take this:
+#define READ_SPD(byte_num) \
+ addis r3, 0, byte_num at l;\
+ or r3, r3, r10;\
+ ori r3, r3, 0x0A;\
+ stw r3, SD_I2C_CTRL1(r4);\
+ li r3, I2C_CNTRL2_START;\
+ stw r3, SD_I2C_CTRL2(r4);\
+ eieio;\
+ sync;\
+ li r3, 0x100;\
Or this:
+ READ_SPD(12) /* get Refresh Rate */
+ beq check_next_slot
+ li r5, ERR_RFRSH_RATE
+ cmpi 0,0,r3,SPD_MIN_RFRSH
+ ble spd_fail
+ cmpi 0,0,r3,SPD_MAX_RFRSH
+ bgt spd_fail
+ addi r3,r3,-SPD_MIN_RFRSH
+ rlwinm r3,r3,2,0,31
+ lis r5,refresh_rates at h
+ ori r5,r5,refresh_rates at l
+ lwzx r5,r5,r3 /* get refresh rate in nSec */
+ divwu r5,r5,r9 /* calculate # of SDC clocks */
+ stw r5,SD_REFRESH(r4) /* Set refresh rate */
Sorry, but I consider this unreadable.
5) board/mpc7448hpc2/mpc7448hpc2.c contains yet another memory test.
Do we really need another copy of this code?
6) Some files - like mpc7448hpc2/tsi108_init.c - contain deep magic
with little or no comments (see for example function
board_early_init_f()). I guess you want to add some more comments
here and there...
7) The output of your code seems to be pretty different from what we
have on other boards; see again board_early_init_f():
+ printf("BUS! %d MHz\n", get_board_bus_clk() / 1000000);
+ printf("MEM! %d MHz\n", gd->mem_clk / 1000000);
Can you please try keeping the look and feel we have on most other
boards?
8) Your code adds some data structures globally (like
hid1_7447A_multipliers_x_10[] in cpu/74xx_7xx/speed.c) which are
probably not needed for all processors. Maybe you can use some
#ifdef's here to prevent adding lots of dead code / data to most
board configurations?
9) The login here looks weird to me - is this correct?
cpu/74xx_7xx/speed.c:
...
+#ifdef CFG_CONFIG_BUS_CLK
+ gd->bus_clk = get_board_bus_clk();
+#else
+ gd->bus_clk = CFG_BUS_CLK;
+#endif
10) Please keep your line length within the allowed limits.
11) Please don't define CONFIG_ETHADDR / CONFIG_ETH1ADDR in your board
config file. It is really evil when all boards have the same MAC
addresses. Also, are the addresses you used officially assigned
ones?
Same is for CONFIG_IPADDR, CONFIG_SERVERIP, CONFIG_NETMASK,
CONFIG_GATEWAYIP - it may save some time to have these set during
development, but for a public source version I don't ever want to
see these.
12) In lib_ppc/extable.c you add code with a "#ifdef
CFG_EXCEPTION_AFTER_RELOCATE; there is absolutely no explanation
nor comment anywhere why you think this is necessary.
Please clean up and resubmit.
Best regards,
Wolfgang Denk
--
Software Engineering: Embedded and Realtime Systems, Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Am besten betrachten Sie Fehlermeldungen als eine Art Psycho-Test,
mit dem herausgefunden werden soll, wie belastbar Sie sind."
- Dr. R. Wonneberger, Kompaktf?hrer LaTeX, Kap. 1.6: Fehlermeldungen
next prev parent reply other threads:[~2006-11-27 15:49 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-08-11 10:44 [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) board support Zang Roy-r61911
2006-10-30 7:11 ` [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) boardsupport Zang Roy-r61911
2006-10-30 9:01 ` Wolfgang Denk
2006-10-30 9:23 ` Zang Roy-r61911
2006-11-03 4:54 ` Zang Roy-r61911
2006-11-07 2:32 ` [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2(Taiga) board support Zang Roy-r61911
2006-11-13 4:02 ` [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) boardsupport Zang Roy-r61911
2006-11-13 6:12 ` Jaksa David
2006-11-13 6:36 ` Zang Roy-r61911
2006-11-27 2:12 ` [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2 (Taiga) board support Zang Roy-r61911
2006-11-27 15:49 ` Wolfgang Denk [this message]
2006-11-28 1:51 ` Zang Roy-r61911
2006-12-01 2:31 ` Zang Roy-r61911
2006-12-01 8:59 ` Wolfgang Denk
2006-12-01 11:23 ` Zang Roy-r61911
2006-12-01 8:09 ` Zang Roy-r61911
2006-12-01 14:31 ` Wolfgang Denk
2006-12-04 2:04 ` Zang Roy-r61911
2006-12-05 2:31 ` Zang Roy-r61911
2006-12-06 5:32 ` Jaksa David
2006-12-08 15:51 ` Zang Roy-r61911
2006-12-14 4:50 ` Zang Roy-r61911
2006-12-20 2:25 ` Jaksa David
2006-12-22 8:36 ` Zang Roy-r61911
2006-12-22 10:02 ` Wolfgang Denk
2006-12-23 11:59 ` Zang Roy-r61911
2007-01-08 2:13 ` Zang Roy-r61911
2007-01-15 2:43 ` Zang Roy-r61911
2007-01-15 21:21 ` Wolfgang Denk
2007-01-23 1:58 ` Zang Roy-r61911
2007-01-29 1:50 ` Zang Roy-r61911
2007-02-12 3:24 ` Zang Roy-r61911
2007-03-08 10:45 ` Wolfgang Denk
2007-03-09 2:03 ` Zang Roy-r61911
2007-03-09 7:40 ` Wolfgang Denk
2007-04-06 1:22 ` Zang Roy-r61911
2007-04-11 23:25 ` Zang Roy-r61911
2007-04-18 15:33 ` Wolfgang Denk
2007-04-18 23:44 ` Zang Roy-r61911
2006-11-02 9:27 ` [U-Boot-Users] [PATCH 00/10 v3]: " Zang Roy-r61911
2006-11-08 8:10 [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2(Taiga)board support Ch.Vict
2006-11-08 8:31 ` Zang Roy-r61911
2006-11-08 8:53 ` Ch.Vict
2006-11-10 7:37 ` Ch.Vict
2006-11-10 7:45 ` [U-Boot-Users] [PATCH 00/07 v2]: Add mpc7448hpc2(Taiga) board support Zang Roy-r61911
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20061127154901.CE3E7353B57@atlas.denx.de \
--to=wd@denx.de \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.