[U-Boot] [PATCH] ARM: Add support for edb93xx boards

[Wolfgang Denk]

Dear Matthias Kaehlcke,

In message <20091207233313.GA31886 at darwin> you wrote:
> 
> > Please don't do scripting in the Makefile. Move this logic into your
> > board config file instead.
> 
> i got the inspiration to handle it this way from the U-Boot Makefile
> (Total5100, TQM5200, et al). could you please point me to an example
> of a well done multi-board configuration file?

This is the old style, which we want to get rid of.

See for example here:

2439 MPC8536DS_NAND_config \
2440 MPC8536DS_SDCARD_config \
2441 MPC8536DS_SPIFLASH_config \
2442 MPC8536DS_36BIT_config \
2443 MPC8536DS_config:       unconfig
2444         @$(MKCONFIG) -t $(@:_config=) MPC8536DS ppc mpc85xx mpc8536ds freescale

> same here, further i don't understand how to set the TEXT_BASE from
> the board configuration file. any pointer?

See for example board/freescale/mpc8536ds/config.mk

> > U-Boot uses assembler only when ther eis no reasonable way to
> > implement the code in C, and I don't see any such justification here.
> > Please rewrite all this in C.
> 
> i'm just starting to get my feet wet with low level initialization and
> supposed sdram setup is always done in assembler.

There is no reason for assembly. We almost always do this in C.

> > > diff --git a/board/edb93xx/u-boot.lds b/board/edb93xx/u-boot.lds
> > > new file mode 100644
> > > index 0000000..76caef3
> > > --- /dev/null
> > > +++ b/board/edb93xx/u-boot.lds
> > 
> > 
> > This looks pretty generic. Please explain why exactly you think you
> > need a board specific linker script here, instead of using the generic
> > one?
> 
> the ep93xx expects to find the pattern 'CRUS' (in ASCII) at position
> 0x1000. is there a more canonical way to achieve this?

This is a processor feature, right? So it's the same for all ep93xx
based boards? Then why put that in a board specific directory?

> as the header files contain a GPL header i interpreted that it's ok to
> submit them without signed-off-by lines from the original authors (what
> doesn't mean that i claim authorship on their work):

It's OK, but it would be even better if we had their S-o-b's, too.
Maybe you can ask them?

> > > diff --git a/include/configs/edb93xx.h b/include/configs/edb93xx.h
> > > new file mode 100644
> > > index 0000000..6c4576b
> > > --- /dev/null
> > > +++ b/include/configs/edb93xx.h
> > ...
> > > +/* Initial environment and monitor configuration options. */
> > > +#define CONFIG_ETHADDR			08:00:3E:26:0A:5B
> > > +#define CONFIG_NETMASK			255.255.255.0
> > > +#define CONFIG_IPADDR			192.168.99.225
> > > +#define CONFIG_SERVERIP			192.168.99.1
> > > +#define CONFIG_GATEWAYIP		192.168.99.1
> > 
> > NAK. We don't allow board specific settings like these.
> 
> hm, a whole bunch of boards do this, so i thought that's the way to
> go. what is the correct thing, not defining this values at all?

Right. You don't really want to have several boards all using the
same MAC address...

> i'm not working for cirrus, so i think getting my own set of MAC
> address is not really what i'm supposed to do. any further advice?

It's up to the hardware manufacturer to provide MAC addresses.

> > > +#define CONFIG_ENV_ADDR		0x60040000
> > > +#if (defined(CONFIG_EDB9301) || defined(CONFIG_EDB9302) ||\
> > > +     defined(CONFIG_EDB9302A))
> > > +#define CONFIG_ENV_SECT_SIZE	0x00020000
> > > +#elif (defined(CONFIG_EDB9307) || defined(CONFIG_EDB9307A) ||\
> > > +       defined(CONFIG_EDB9312) || defined(CONFIG_EDB9315) ||\
> > > +       defined(CONFIG_EDB9315A))
> > > +#define CONFIG_ENV_SECT_SIZE	0x00040000
> > > +#endif
> > > +#define CONFIG_ENV_ADDR_REDUND	(CONFIG_ENV_ADDR + CONFIG_ENV_SECT_SIZE)
> > > +
> > > +#define CONFIG_ENV_SIZE		CONFIG_ENV_SECT_SIZE
> > > +#define CONFIG_ENV_SIZE_REDUND	CONFIG_ENV_SIZE
> > > +
> > > +#define CONFIG_SYS_JFFS2_FIRST_BANK	0
> > > +#if (defined(CONFIG_EDB9301) || defined(CONFIG_EDB9302) ||\
> > > +     defined(CONFIG_EDB9302A))
> > > +#define CONFIG_SYS_JFFS2_FIRST_SECTOR	28
> > > +#elif (defined(CONFIG_EDB9307) || defined(CONFIG_EDB9307A) ||\
> > > +       defined(CONFIG_EDB9312) || defined(CONFIG_EDB9315) ||\
> > > +       defined(CONFIG_EDB9315A))
> > > +#define CONFIG_SYS_JFFS2_FIRST_SECTOR	14
> > > +#endif
> > 
> > Hm... this looks very much like being completely unmaintainable.
> > 
> > Please simplify the code - in your own interest.
> 
> i'm not sure if i understand what you mean bt simplifying in this case
> 
> do you propose something like
> 
> #ifdef CONFIG_EDB9301
> #define CONFIG_ENV_SECT_SIZE	0x00020000
> #elif defined(CONFIG_EDB9302)
> #define CONFIG_ENV_SECT_SIZE	0x00020000
> #elif ...
> 
> #ifdef CONFIG_EDB9301
> #define CONFIG_SYS_JFFS2_FIRST_SECTOR	28
> #elif defined(CONFIG_EDB9302)
> #define CONFIG_SYS_JFFS2_FIRST_SECTOR	28
> #elif ...

No, that would even be worse.

> or even
> 
> #ifdef CONFIG_EDB9301
> #define CONFIG_ENV_SECT_SIZE	0x00020000
> #define CONFIG_SYS_JFFS2_FIRST_SECTOR	28
> #elif ...
> 
> though the settings aren't related?

It would still be better to have everything related to one
configuration bundled together - otherwise you will not be able to see
in the code which settings apply to which board.

Also note that something like CONFIG_SYS_JFFS2_FIRST_SECTOR is pretty
much unlucky - you make yoursself dependent on specific hardware
features. Why not use for example an offset, so flash chips with other
sector sizes can be dropped in? 

Eventually you want to check if mtdparts would not be a better
approach here?

> with the current approach i tried to avoid redundancy in the
> declarations, with the result of more complex conditions. please give
> me a pointer to the preferred way to do this kind of stuff.

Keep the code simple, so you can read it without too much thinking.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I distrust all systematisers, and avoid them. The will  to  a  system
shows a lack of honesty.
- Friedrich Wilhelm Nietzsche _Götzen-Dämmerung [The Twilight of  the
Idols]_ ``Maxims and Missiles'' no. 26