Re: [U-Boot-Users] [PATCH 4/5] Support Analogue Micro's ASP8347DB board.
Dear Pantelis,
in message 20061129172613.24638.18292.stgit@pantathon.hol.gr you wrote:
Add support for Analogue Micro's ASP8347DB board. The board is originally shipped with RedBoot. All appropriate settings are migrated to u-boot & the old kernel booted; a drop in bootloader replacement.
In addition to my previous comments abot necessary coding style cleanup ((indentation by TABs, trailing white space, C++ comments, line length, etc.) please cleanup the following items:
--- a/MAKEALL +++ b/MAKEALL @@ -130,7 +130,7 @@ LIST_8260=" \ #########################################################################
LIST_83xx=" \
- TQM834x MPC8349EMDS \
- TQM834x MPC8349EMDS ASP8347DB \
Please keep lists sorted.
--- a/Makefile +++ b/Makefile @@ -1597,6 +1597,9 @@ TQM834x_config: unconfig MPC8349EMDS_config: unconfig @$(MKCONFIG) $(@:_config=) ppc mpc83xx mpc8349emds
+ASP8347DB_config: unconfig
- @$(MKCONFIG) $(@:_config=) ppc mpc83xx asp8347db
Please keep lists sorted.
--- /dev/null +++ b/board/asp8347db/asp8347db.c
...
+long int initdram (int board_type) +{
- long size, known_size;
- /* 128MB */
- known_size = 128 << 20;
...
- /* size detection */
- debug("\n");
- size = get_ram_size(CFG_DDR_BASE, known_size);
- if (size != known_size)
debug("Detected other size than what expected!\n");
You probably want to remove this for the production version?
...
+static void move64(unsigned long long *src, unsigned long long *dest) +{ +#if defined(CONFIG_MPC8260) || defined(CONFIG_MPC824X)
Isn't it unlikely that any of these conditions is true on a 834x board?
+#if defined (CFG_DRAM_TEST_DATA)
+unsigned long long pattern[] = {
- 0xaaaaaaaaaaaaaaaaULL,
- 0xccccccccccccccccULL,
- 0xf0f0f0f0f0f0f0f0ULL,
- 0xff00ff00ff00ff00ULL,
- 0xffff0000ffff0000ULL,
- 0xffffffff00000000ULL,
- 0x00000000ffffffffULL,
- 0x0000ffff0000ffffULL,
- 0x00ff00ff00ff00ffULL,
- 0x0f0f0f0f0f0f0f0fULL,
- 0x3333333333333333ULL,
- 0x5555555555555555ULL,
+};
+/*********************************************************************/ +/* NAME: mem_test_data() - test data lines for shorts and opens */ +/* */ +/* DESCRIPTION: */ +/* Tests data lines for shorts and opens by forcing adjacent data */ +/* to opposite states. Because the data lines could be routed in */ +/* an arbitrary manner the must ensure test patterns ensure that */ +/* every case is tested. By using the following series of binary */ +/* patterns every combination of adjacent bits is test regardless */ +/* of routing. */
Do we really need yet another copy of this code?
+/*********************************************************************/ +/* NAME: testdram() - calls any enabled memory tests */
...
This would be at least the 5th copy of this code.
Please don't do this. Let's clean this up and use one common verion - if needed at all, as there is other memtest code, too.
--- /dev/null +++ b/board/asp8347db/fpga.c
...
+static void mdelay(int ms) +{
- ulong start = get_timer(0);
- ulong delay;
- delay = (ms * CFG_HZ) / 1000;
- while (get_timer(start) < delay) {
udelay (1000);WATCHDOG_RESET(); /* Trigger watchdog, if needed */
Note: this is redundant, as udelay() will already trigger the watchdog.
Best regards,
Wolfgang Denk
On 29 Νοε 2006, at 11:32 ΜΜ, Wolfgang Denk wrote:
Dear Pantelis,
in message 20061129172613.24638.18292.stgit@pantathon.hol.gr you wrote:
Add support for Analogue Micro's ASP8347DB board. The board is originally shipped with RedBoot. All appropriate settings are migrated to u-boot & the old kernel booted; a drop in bootloader replacement.In addition to my previous comments abot necessary coding style cleanup ((indentation by TABs, trailing white space, C++ comments, line length, etc.) please cleanup the following items:
--- a/MAKEALL +++ b/MAKEALL @@ -130,7 +130,7 @@ LIST_8260=" \
##################################################################### ####
LIST_83xx=" \
- TQM834x MPC8349EMDS \
- TQM834x MPC8349EMDS ASP8347DB \
Please keep lists sorted.
OK
--- a/Makefile +++ b/Makefile @@ -1597,6 +1597,9 @@ TQM834x_config: unconfig MPC8349EMDS_config: unconfig @$(MKCONFIG) $(@:_config=) ppc mpc83xx mpc8349emds
+ASP8347DB_config: unconfig
- @$(MKCONFIG) $(@:_config=) ppc mpc83xx asp8347db
Please keep lists sorted.
OK
--- /dev/null +++ b/board/asp8347db/asp8347db.c
...
+long int initdram (int board_type) +{
- long size, known_size;
- /* 128MB */
- known_size = 128 << 20;
...
- /* size detection */
- debug("\n");
- size = get_ram_size(CFG_DDR_BASE, known_size);
- if (size != known_size)
debug("Detected other size than what expected!\n");You probably want to remove this for the production version?
Well, here's the thing, if the board's DRAM is not soldered properly this will probably trigger - think of it as a cheap memory test.
But it's not biggie to go...
...
+static void move64(unsigned long long *src, unsigned long long *dest) +{ +#if defined(CONFIG_MPC8260) || defined(CONFIG_MPC824X)
Isn't it unlikely that any of these conditions is true on a 834x board?
OK
+#if defined (CFG_DRAM_TEST_DATA)
+unsigned long long pattern[] = {
- 0xaaaaaaaaaaaaaaaaULL,
- 0xccccccccccccccccULL,
- 0xf0f0f0f0f0f0f0f0ULL,
- 0xff00ff00ff00ff00ULL,
- 0xffff0000ffff0000ULL,
- 0xffffffff00000000ULL,
- 0x00000000ffffffffULL,
- 0x0000ffff0000ffffULL,
- 0x00ff00ff00ff00ffULL,
- 0x0f0f0f0f0f0f0f0fULL,
- 0x3333333333333333ULL,
- 0x5555555555555555ULL,
+};
+/
/ +/* NAME: mem_test_data() - test data lines for shorts and opens */ +/* */ +/* DESCRIPTION: */ +/* Tests data lines for shorts and opens by forcing adjacent data */ +/* to opposite states. Because the data lines could be routed in */ +/* an arbitrary manner the must ensure test patterns ensure that */ +/* every case is tested. By using the following series of binary */ +/* patterns every combination of adjacent bits is test regardless */ +/* of routing. */
Do we really need yet another copy of this code?
+/
/ +/* NAME: testdram() - calls any enabled memory tests */
...
This would be at least the 5th copy of this code.
Please don't do this. Let's clean this up and use one common verion - if needed at all, as there is other memtest code, too.
Well, there is not a common memory test code. I could move this to a common area. But I'll only modify this board to use it; the rest is the job of their respective maintainers.
--- /dev/null +++ b/board/asp8347db/fpga.c
...
+static void mdelay(int ms) +{
- ulong start = get_timer(0);
- ulong delay;
- delay = (ms * CFG_HZ) / 1000;
- while (get_timer(start) < delay) {
udelay (1000);WATCHDOG_RESET(); /* Trigger watchdog, if needed */Note: this is redundant, as udelay() will already trigger the watchdog.
Nice catch - forgot it.
Best regards,
Wolfgang Denk
-- Software Engineering: Embedded and Realtime Systems, Embedded Linux Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "Consistency requires you to be as ignorant today as you were a year ago." - Bernard Berenson
Regards
Pantelis
Dear Pantelis,
in message DE665B74-967F-486A-B382-C83196EC8D03@embeddedalley.com you wrote:
Well, here's the thing, if the board's DRAM is not soldered properly this will probably trigger - think of it as a cheap memory test.
Well, you would see the unexpected size printed with the startup messages. That's something you should always look for: memory sizes, clock frequencies, etc.
Please don't do this. Let's clean this up and use one common verion - if needed at all, as there is other memtest code, too.
Well, there is not a common memory test code. I could move this to a
Wrong, there is.
First, there is common/memsize.c which includes a simple but pretty efficient (and fast) memory test.
Then there is common/cmd_mem.c with it's two different implementations of do_mem_mtest(), depending if CFG_ALT_MEMTEST is set or not.
And finally, there is post/memory.c which is where you copied the code from. Why didn't you use post/memory.c ?
area. But I'll only modify this board to use it; the rest is the job of their respective maintainers.
Agreed.
Best regards,
Wolfgang Denk
participants (2)
-
Pantelis Antoniou -
Wolfgang Denk