[U-Boot] [PATCH 2/3] Add board specific code for da850 EVM

[Sudhakar Rajashekhara]

Hi,

On Mon, Dec 21, 2009 at 15:48:38, Nick Thompson wrote:
> On 18/12/09 10:23, Sudhakar Rajashekhar wrote:
> > Nick Thompson <nick.thompson <at> ge.com> writes:
> > 
> >>
> >> On 16/12/09 10:24, Sudhakar Rajashekhara wrote:
> >>> Provides initial support for TI OMAP-L138/DA850 SoC devices on
> >>> a Logic PD EVM board.
> >>>
> >>> Provides:
> >>> Initial boot and configuration.
> >>> Support for i2c.
> >>> UART support (console).
> >>>
> >>> Signed-off-by: Sudhakar Rajashekhara <sudhakar.raj <at> ti.com>
> >>> ---
> >>>  board/davinci/da8xxevm/Makefile         |    1 +
> >>>  board/davinci/da8xxevm/da850evm.c       |  124 
> > +++++++++++++++++++++++++++++++
> >>>  include/asm-arm/arch-davinci/hardware.h |    1 +
> >>>  3 files changed, 126 insertions(+), 0 deletions(-)
> >>>  create mode 100644 board/davinci/da8xxevm/da850evm.c
> >>>
> >>
> >> [...]
> >>
> >>> diff --git a/board/davinci/da8xxevm/da850evm.c 
> > b/board/davinci/da8xxevm/da850evm.c
> >>> new file mode 100644
> >>> index 0000000..92548e2
> >>> --- /dev/null
> >>> +++ b/board/davinci/da8xxevm/da850evm.c
> >>> @@ -0,0 +1,124 @@
> >>> +/*
> >>> + * (C) Copyright 2009, Texas Instruments, Inc. http://www.ti.com/
> >>> + *
> >>> + * Based on da830evm.c
> >>
> >> At this point, this seem to be a copy of da830evm.c (well except the first 
> > few lines above).
> >>
> > 
> > I don't think you have observed that the PINMUX registers are different on 
> > da850. So we are at risk of adding many ifdefs now itself if we combine da830 
> > and da850. Also, there are few things which differ between da830 and da850, for 
> > example, MAC address is stored in I2C eeprom on da830 and in SPI eeprom on 
> > da850. You can have a look at http://www.arago-project.org/git/people/?
> > p=sekhar/u-boot-omapl1.git;a=summary to understand how the da830 and da850 
> > files look later when support for most peripherals are added.
> > 
> > To avoid code cluttering, I prefer using two separate files for da830 and da850.
> 
> Yes, the pinmux table definitions are different, but I don't see how this
> requires duplicating code.
> 
> In the latest patches for da830, the pinmux setup is table driven. It would
> be very simple to pull out the two tables into da830 and da850 specific
> files (selected by the build system) and use common pinmux code to setup the
> pins appropriately. The code part for this is simply:
> 
> 	/* configure pinmux settings */
> 	if (davinci_configure_pin_mux_items(pinmuxes, ARRAY_SIZE(pinmuxes)))
> 		return 1;
> 
> That would be exactly the same line of code for both devices.
> 
> It should be possible to put i2c (da830) and spi (da850) equivalent functions
> into that same file to read the MAC address. Abstractions of this form are
> also likely to make the code more readable. [In fact the function is already
> abstracted into misc.c, but it only provides for i2c currently]
> 
> There might be an issue with functions supported on one platform that are not
> supported on the other, but these could also be abstracted out. They also
> ought to be few because of the similarities of the devices.
> 
> Not doing so puts us at risk of increasing commmits and maintenance. There is
> no need to clutter the code - it can be layered and abstracted to handle
> differences cleanly.
> 

I tried modifying the code in the way discussed above, but during the process I
realized that by doing so it is becoming more difficult to add a new board based
on da830/da850 SoC. Also, I feel it is better to extract out the common code in
some Soc related file (say da8xx.c in cpu/arm926ejs/davinci) and still have two
separate board files. Common code like IRQ disabling etc. can be implemented in
this SoC specific file and this function will be called from the board files.
Even in Linux kernel, two different board files are being maintained for da830
and da850 and there is a SoC specific file where the common code exists.

Regards,
Sudhakar