[U-Boot] [PATCH] add ASTRO MCF5373L board

[Wolfgang Wegner]

Dear Wolfgang Denk,

thank you for all the comments and suggestions and sorry for that
many reasons to complain!
I just would like to ask a few questions in-line.

On Thu, Dec 17, 2009 at 11:33:07PM +0100, Wolfgang Denk wrote:
> Dear Wolfgang Wegner,
> 
> In message <1260378648-19232-1-git-send-email-w.wegner at astro-kom.de> you wrote:
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2263,6 +2263,18 @@ M5485HFE_config :	unconfig
> >  TASREG_config :		unconfig
> >  	@$(MKCONFIG) $(@:_config=) m68k mcf52x2 tasreg esd
> >  
> > +astro_mcf5373l_config \
> > +astro_mcf5373l_ram_config :	unconfig
> > +	@if [ "$@" = "astro_mcf5373l_ram_config" ] ; then \
> > +		echo "#define CONFIG_MONITOR_IS_IN_RAM"	>> $(obj)include/config.h ; \
> > +		echo "TEXT_BASE = 0x40020000" > $(obj)board/astro/mcf5373l/config.tmp ; \
> > +		$(XECHO) "... for RAM boot ..." ; \
> > +	else \
> > +		echo "TEXT_BASE = 0x00000000" > $(obj)board/astro/mcf5373l/config.tmp ; \
> > +		$(XECHO) "... for FLASH boot ..." ; \
> > +	fi
> > +	@$(MKCONFIG) -a astro_mcf5373l m68k mcf532x mcf5373l astro
> 
> Please keep lists sorted, and don't add such scripting to the
> Makefile. It is not needed any more.

How can I avoid this scripting? I took the freescale EVM boards as a
reference. Could you please point me to a better example on how to
set these variables, especially TEXT_BASE, on a per-target basis?

> > +	/* PAR_T0IN -> GPIO */
> > +	gpiop->par_timer &= 0xfc;
> 
> Please use I/O accessors to access device registers. Please fix
> globally.

Sorry if this is a stupid question, but so this should read:

tmp_char = readb(&gpiop->par_timer);
tmp_char &= 0xfc;
writeb(tmp_char, &gpiop->par_timer);

And how about word and long-word registers? As far as I understand
from asm-m68k/io.h, the I/O accessors do byte-swap, how does this
make sense on a big-endian-only device?

> Do you really need a board specific linker script?

Probably not in this case, but unfortunately this is what all
current Coldfire boards do, and I am not familiar enough with
ld to overlook which consequences changing this might have.

(It may be trivial for MCF53xx, but for other devices like
MCF5445x with the possibility for serial boot, there are
big differences between the different linker files.)

> > +void do_crc (unsigned char data)
> > +{
> 
> Cool. Yet another CRC function :-(

I have no choice here. The CRC algorithm is dictated, so either
I can use an already existing CRC function implementing exactly
this scheme (sorry for not having searched for it yet), or I have
to implement it here. :-(

> > +		s = flash_full_status_check_nb (flash_info, fl_sector,
> > +						0, "erase");
> > +		if (s != ERR_BUSY) {
> > +			if (s == ERR_OK) {
> > +				flash_sect_count++;
> > +				flash_prog_stat = FL_STAT_IDLE;
> > +			}
> > +			else
> 
> And again. Please fix indentation globally.

Sorry to ask, but which part of indentation is wrong here?
(Except the else/if you quoted in the other examples)

Regards,
Wolfgang