Hi Wolfgang!

Thanks for your time on reviewing the u-boot patch for Coldfire - i can imagine it was quite a lot of work ...

I have to admit that the coldfire u-boot port was a quick-hack intended as prototype that at least was usefull for me :-)

Unforntunatly i'm not involved in any Coldfire related development at the moment and pretty busy with other projects, so i won't have the time right now to sort out the issue you have discovered.

But i will at least try to provide some usefull comments for each of the items listed below, so that other Coldfire/u-boot users can start cleaning things up, so that the patch will be free of any licencing or technical issues. In this context, please feel free to redistribute this mail to whomever you think it might be usefull for.

One general note: the whole lot of "PPCisms" are deriving from the fact that i copied and renamed several PPC files from the u-boot tree and modified them until they did the job.

Although certain files are even holding PPC assembly instructions, or other obviously PPC related routines, the bootloader will still compile and work (IMHO pretty well) for the coldfire because the appropriate code will not be compiled (ifdef'ed out) or is just dead code in the resulting binary.

Wolfgang Denk wrote:

OK, we will leave the "pre-bootloader" out of the U-Boot tree. Please set up a FTP server or such where Coldfire users can get it from.

I guess the best place for doing so would be www.uclinux.org (where i still have an account - not accessible at the moment, but that's different story)

Please provide a doc/README.Coldfire which explains this procedure.

I will add appropriate notes to the existing coldfire specfic installation instruction when the cleanups have been finished.

I am afraid your patch is far from being usable:

That's a question of definition! :-)

To be serious: as the file name of the patch indicates, it was intended to be applied for u-boot-0.4.0. I didn't have done anything on it since then ...

• "examples/syscall.S" does not exist any more; I omitted this part of the patch

Fine for me! i actually never tried to start anything als than linux :-)

• "include/asm-m68k/8xx_immap.h" does not make any sense to me - why do you copy a MPC8xx specific file to the "include/asm-m68k/" header tree? Omitted.

The situation is that the FEC is almost the same one as used by MPC8xx, so the related structures and definitions are pretty usefull in 8xx_immap.h. We could rename the file to something like mcf5xxx_fec.h and throw out any non-FEC related stuff (iirc, non of the other structures and definitions are used). I guess the "most clean" way would be to put the FEC driver into the "drivers" subdirectory since the FEC actually became an archtecture independend device. But doing so would probably imply a lot of work, because it would probably break all MPC8xx[x] implementations for quite some time.

• "include/asm-m68k/arch-coldfire/dummy.h" is effectively an empty file which is nowhere referenced. Omitted.

IIRC, the situation is that the Makefile (as taken from the PPC tree) likes to create a link to a platform dependend sub-directory. Unfortnualty, an empty sub-directoy does not survive in a patch file and such won't be created when applying the patch - then the Makefile will stop operation because of a missing directory. I could have modified the Makefile to not take care about different platforms, but i thought that in near or far future, somebody may like to port u-boot to other M68k(-like) architectures such as Amiga, Mac or Cris, and it may be necessary to provide architecture specific files. So at least the "structural" requirements are met this way ...

• "include/asm-m68k/bitops.h" reads: "Bit string operations on the ppc" -- on the PPC??? Actually the file does contain PowerPC assembler instructions which will never work on M68K. Omitted.

Actually it does work, because the __INLINE definitions (containing the ppc-assembly code) are not used: the "pure C" inlined prototypes at the end of the file are used, instead. But true: this stuff should be deleted to avoid confusions like this one.

• "include/asm-m68k/byteorder.h" contains more PowerPC assembler instructions which will never work on M68K. Omitted.

Same thing: most of the definitions are not activly used in the coldfire port - needs cleanup to avoid confusion.

• "include/asm-m68k/cache.h" is another verbatim copy of a PowerPC file which makes no sense for M68K. Omitted.

dito

• "include/asm-m68k/io.h" is effectively an empty file - please verify that this is OK!

That's more or less ok: lot of the *.h files exist because they are included by the ppc-derived *.c files and i wanted to maintain the given structure as close as possible so that it is possible to easily add related features if necessary. That's the reason why lot of the header files seem to have non-sense content, but u-boot for coldfire will still compile and work. In the current case, inb()/outb() do not need to be defined simply because all register accesses are directly memory referenced for coldfire (*(volatile ...)).

• "include/asm-m68k/m5272.h" and "include/asm-m68k/m5282.h" contain this: (C) Copyright 2001, Key Technology (http://www.keyww.com) To become part of U-Boot these files must be available under GPL. If you can guarantee that we may use these files under GPL please fix the file headers.

Interessting thing! I'm glad that you stumbled of this issue: i have copied over the m5272.h file from the COLILO ("coldfire linux loader" which is the pre-bootloader for M5272C3). COLILO is distributed under the terms of the GPL, so i didn't took too much care about the file headers. We might check if a newer release of COLILO has a GPLed header in this file. Otherwise we need to check (together with the author of COLILO) back with the current copyright holder. The m5282.h is just a copy of the m5272.h file with the approprate changes for 5282 (changes done by me).

• "include/asm-m68k/mcftimer.h" and "include/asm-m68k/mcfuart.h" contain this: (C) Copyright 1999-2002, Greg Ungerer (gerg@snapgear.com) (C) Copyright 2000, Lineo Inc. (www.lineo.com) To become part of U-Boot these files must be available under GPL. If you can guarantee that we may use these files under GPL please fix the file headers.

This is a similar issue: that file was taken from the uClinux kernel source tree from include/asm-m68knommu/mcfuart.h, and guess what: even the latest release of the kernel "linux-2.6.0-test11" still holds that header!! (somebody should inform the LKML) - it should be easy to sort out that issue, i guess, unless the companies that bought snapgear and lineo are trying to go the SCO way :-)

• "include/asm-m68k/mmu.h" contains (as a comment says): "PowerPC memory management structures" which makes no sense for M68K. Omitted.

True, but just as like as for the other files: maybe somebody may like to port u-boot for MMU-full Amigas (BTW.: there is an MMU-full coldfire coming, soon). Then at least we are using a defined "infrastructure". But certainly agreed: we need to get rid of any "PPCisms" in these files. The files could IMHO stay, even if empty, who cares?

• "include/asm-m68k/processor.h" is a PowerPC file which makes no sense for M68K. Omitted.

dito

• "include/configs/M5272C3.h" and "include/configs/M5282EVB.h" contain neither a copyright notice nor a GPL header. Pleae fix.

M5272C3.h is IIRC a copy of TQM860L.h, so it should actually include your standard header with a minor modification:

/* * (C) Copyright 2000, 2001, 2002 * Wolfgang Denk, DENX Software Engineering, wd@denx.de. * Coldfire support contributed by Bernhard Kuhn, bkuhn@metrowerks.com * * See file CREDITS for list of people who contributed to this * project. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License as * published by the Free Software Foundation; either version 2 of * the License, or (at your option) any later version. * * 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 * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program; if not, write to the Free Software * Foundation, Inc., 59 Temple Place, Suite 330, Boston, * MA 02111-1307 USA */

The same header can be applied to M5282EVB.h since it is derived from M5272C3.h

• "lib_m68k/cache.c" is effectively an empty file. Omitted.

same "structural maintainance idea" as for certain *.h files ...

• "lib_m68k/config.mk" defines "-D__linux__ -D__uClinux__", but both is most probably wrong for U-Boot. Omitted.

I'm not sure about that: some of the header files are simply copied over from the uClinux kernel (such as mcfuart.h) and iirc, these files need these definitions in order to be compilable. But true, for a clean solution this issue should be fixed in the appropriate files, rather than applying this quick-hack.

• "lib_m68k/ctype.c" contains the same cose as already present in "lib_generic/ctype.c". Omitted.

Then we should use the generic one, right? :-)

• "lib_m68k/kgdb.c" is a PowerPC file which makes no sense for M68K. Omitted.

Again, copied over from the ppc tree at the beginning of the development and doesn't provide any usefull functionality at the moment, except for being a place-holder for future use.

• "lib_m68k/ldiv.c" contains the same cose as already present in "lib_generic/ldiv.c". Omitted.
• "lib_m68k/Makefile" needs major cleanup (see above). Omitted.
• "lib_m68k/ticks.S" is a PowerPC file which makes no sense for M68K. Omitted.

Ok, needs cleanup.

• "lib_m68k/time.c" contains a dummy implementation of init_timebase() [which probably makes no sense on M68K at all] - please verify that this is OK!

We can probably delete it, unless this function may be usefull for other m68k-ports (which i don't know)

• All files in the "utils/coldfire/M5272C3/bdm/" directory are either missing a copyright notice and a GPL header, or they contain

proprietary

copyright notices. Omitted.

We can make this files part of the pre-loader distribution(s), or seperate them otherwise - these files are not vital for the compilation process.

• "cpu/coldfire/cpu_init.c" contains verbatim copy of a MPC8xx specific file which makes no sense for M68K. Omitted.

IIRC, only cpu_init_r() is called.

• "cpu/coldfire/fec.h" is effectively an empty file - please verify that this is OK!

the actual fec register defintions are in 8xx_immap.h

• "cpu/coldfire/interrupts.c" fails to implement irq_install_handler() / irq_free_handler() / enable_interrupts() and disable_interrupts() [and probably more] functions - please verify that this is OK!

The current implementation of u-boot for coldfire is not using any interrupts. Status registers are pooled.

• "cpu/coldfire/kgdb.S" is a verbatim copy of a MPC8xx specific file which makes no sense for M68K. Omitted.
• "cpu/coldfire/kgdb.c" is broken / missing. Omitted.

again, legacy code :-) needs cleanup.

• "cpu/coldfire/speed.c" contains only dummy definitions for critical functions get_gclk_freq() and get_bus_freq() - please verify that this is OK!

To my knowladge, for the EVBs, these frequencies can only be altered by changing the crystals, so it's probably ok to not taking care about core and bus frequency for now.

I have added the remaining stuff which is obviously not usable in this form. Please help to clean up and add the missing parts.

As indicated above: i can't afford to spend time on the cleanup work, right now, sorry. However, if somebody is volunteering to do the necessary cleanups, then i will try to assist her or him as good as possible.

best regards

Bernhard