[U-Boot] Zoom2 respin II

[U-Boot] Zoom2 respin II

[Tom]

Soon I will be posting a rebase of omap3 zoom2 board with fixes and 
enhancements per comments.

Tom


Big changes.

GPIO Interface
Ported from gpio code from linux.  Used in zoom2 debug_board and led code.
Add documenation to doc/README.omap3

LED Interface
Use existing status_led interface.  Add blue led to color led's.
Add documentation to doc/README.LED

OMAP u-boot.lds
Move this from board to cpu.

Comments

Dirk on 4/4

...
 > +static void zoom2_debug_board_detect (void)
 > +{
 > +    unsigned int val;
 > +    /*
 > +     * GPIO to query for debug board
 > +     * 158 db board query, bank 5, index 30
 > +     */
 > +    gpio_t *gpio5_base = (gpio_t *) OMAP34XX_GPIO5_BASE;
 > +
 > +    val = __raw_readl (&gpio5_base->datain);

Is there any special reason why you use __raw_readl() instead of just 
readl() here? Looking at ARM's io.h, they seem to map to the same macro?

Tom: New gpio interface is used, so this is hidden.  BUT looking closely at
   interface and __raw_readl is still being used.  This was kept because I
   wanted the keep code as close to kernel as possible.

------------------------------------------------------------------------
Dirk 4/4

I'm not sure if I overlooked it in one of the other patches, but if not: 
Do you like to update

doc/README.omap3

Tom: Zoom2 information added to README.omap3 similar to other targets.

------------------------------------------------------------------------

Jean-Christophe Serial 4/2

 >  COBJS    := zoom2.o \
 > > -    debug_board.o
 > > +    debug_board.o \
 > > +    zoom2_serial.o
it could be nice to disactivate it

A : Suprising hard to do.  Logic added to do this for LED.
    COBJS-${CONFIG_STATUS_LED}   += led.o
    Doing something similar leaves the serial_* functions undefined.
    Defining a set of functions to be weak aliases for these functions 
does not
    work cleanly.

 > +#include "zoom2_serial.h"
 > > +
 > > +int zoom2_debug_board_connected (void);
please move to a header

A : Moved to zoom2_serial.h

 > +    if (zoom2_debug_board_connected ()) {
 > > +        NS16550_t com_port = (NS16550_t) base;
 > > +        int baud_divisor = CONFIG_SYS_NS16550_CLK / 16 /
 > > +            CONFIG_BAUDRATE;
aligning it with CONFIG_SYS_NS16550_CLK could be nice
please add an empty line

A : Done

 > +        com_port->mcr = (MCR_DTR | MCR_RTS);
 > > +        com_port->fcr = (FCR_FIFO_EN | FCR_RXSR | FCR_TXSR);
 > > +        com_port->lcr = LCR_BKSE | LCR_8N1;
 > > +        com_port->dll = baud_divisor & 0xff;
 > > +        com_port->dlm = (baud_divisor >> 8) & 0xff;
 > > +        com_port->lcr = LCR_8N1;
clould you explain a fe more what you do here?

A : Added a comment that this was generic setup code.

 > +    if (zoom2_debug_board_connected ()) {
 > > +        NS16550_t port = (NS16550_t) base;
 > > +
 > > +        if (c == '\n')
 > > +            quad_putc_dev (base, '\r');
 > > +
 > > +        NS16550_putc (port, c);
why not
        NS16550_putc ((NS16550_t) base, c);

A : Done in all the port vs. base functions.

 > void quad_setbrg_dev (unsigned long base)
 > > +{
 > > +    if (zoom2_debug_board_connected ()) {
 > > +        NS16550_t port = (NS16550_t) base;
 > > +
 > > +        int clock_divisor = CONFIG_SYS_NS16550_CLK / 16 /
 > > +            CONFIG_BAUDRATE;
 > > +
 > > +        NS16550_reinit (port, clock_divisor);
why don't you use the same in in the imit?

Tom: No good reason.  I was using drivers/serial/serial.c as a guide and 
this is
   how it does it, with the clock_divisor function inlined.

 > +
 > > +#define QUAD_BASE_0      SERIAL_TL16CP754C_BASE
 > > +#define QUAD_BASE_1      (SERIAL_TL16CP754C_BASE + 0x100)
 > > +#define QUAD_BASE_2      (SERIAL_TL16CP754C_BASE + 0x200)
 > > +#define QUAD_BASE_3      (SERIAL_TL16CP754C_BASE + 0x300)
why not
#define QUAD_BASE(x)    (SERIAL_TL16CP754C_BASE + (x << 8))

Tom: That would work too.  I would prefer to keep it as is becuase the 
register
base is less obscured.

 > +#define QUAD_INIT(n)                           \
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^
white space please fix
 > > +int quad_init_##n(void)

Tom: Done, converted


 > +    enable_gpmc_config(gpmc_config,
 > > +               serial_cs_base,
 > > +               SERIAL_TL16CP754C_BASE,
 > > +               GPMC_SIZE_16M);
 > > +#endif
it's board specific please move to board


Tom: Done.


 > -#ifdef CONFIG_OMAP
 > > +#if defined(CONFIG_OMAP) && !defined(CONFIG_OMAP3_ZOOM2)
a config could be better

Tom: I do not use this code, the ifdef is because other omap's do.


 >  #define ONENAND_MAP        0x20000000    /* OneNand addr */
 > >                          /* (actual size small port) */
 > > +#define SERIAL_TL16CP754C_BASE    0x10000000      /* Zoom2 Serial 
chip address */
it's not cpu specfic please move this to the serial driver

Tom: Done, moved to zoom2_serial.h

 > -#define CONFIG_CONS_INDEX        3
 > > -#define CONFIG_SYS_NS16550_COM3        OMAP34XX_UART3
 > > -#define CONFIG_SERIAL3            3    /* UART3 */
is the removed serial will work on the zoom2?

Tom: This, and the other UARTS, are internally connected and should not 
be used.

------------------------------------------------------------------------

Jean-Christophe Zoom2 base 4/2

From
 > > +    .ARM.extab    : { *(.ARM.extab* .gnu.linkonce.armextab.*) }
 > > +    __exidx_start = .;
 > > +    .ARM.exidx    : { *(.ARM.exidx* .gnu.linkonce.armexidx.*) }
 > > +    __exidx_end = .;
no need please remove
 > > +
 > > +    . = ALIGN(4);
 > > +    .data : { *(.data) }
 > > +
all the omap3 share the same lds it wil be nice to regroup it as done 
for the
at91

Tom: Done.  See OMAP-Consolidate-common-u-boot.lds-to-cpu-layer.patch


 > +#define CONFIG_CMD_EXT2        /* EXT2 Support                 */
                               ^^^^^^^^^^^^^^^^^
white space please fix it
 > > +#define CONFIG_CMD_FAT        /* FAT support                  */
ditto and the followiing configs too
 > > +#define CONFIG_CMD_JFFS2    /* JFFS2 Support                */

Tom: Done, tab-ified.


 > +#define CONFIG_JFFS2_PART_OFFSET    0x680000
 > > +#define CONFIG_JFFS2_PART_SIZE        0xf980000    /* size of jffs2 */
you do not active the MTD_PARTS  it could be usefull


Tom: NAND related changes to follow this patch set.


 > +
 > > +#define CONFIG_EXTRA_ENV_SETTINGS \
 > > +    "loadaddr=0x82000000\0" \
load_addr?

Tom: This whole section was removed.  It came from Zoom1 which came from 
beagle
    and does not really apply to zoom2.

 > + */
 > > +#define V_PROMPT            "OMAP3 Zoom2# "
why? why not only define CONFIG_SYS_PROMPT?

Tom: Yes. Good point. Done.

 > +/*
 > > + * 2430 has 12 GP timers, they can be driven by the SysClk 
(12/13/19.2) or by
 > > + * 32KHz clk, or from external sig. This rate is divided by a 
local divisor.
 > > + */
 > > +#define V_PVT                7
as said precedently us the 12Mhz as imput with a PVT at 1 will give us a
better timer

Tom: OK.  This code is no longer in the config file.

------------------------------------------------------------------------

Jean-Christophe debug board 4/2

 > +COBJS    := zoom2.o \
 > > +    debug_board.o
you do not plan to activate it via a CONFIG only?

Tom: No. The debug board is ment to be determined at run time.  The 
board is
    easily detatched and single binary is needed.

 > +
 > > +static int debug_board_connected = DEBUG_BOARD_CONNECTED;
why not set it as -1; and then avoid the first_time int?

Tom: No.  I looked at this and the tradeoff would be more code, I would 
prefer
    to leave it as it but will change it if you really want this.

-------------------------------------------------------------------------

Scott Wood Zoom2 base

 > +#define NAND_NO_RB            1
 > > +#define CONFIG_SYS_NAND_WP

More legacy NAND stuff.  You should probably go through the config and
clean out anything that isn't verifiably necessary.

Tom: I Cleaned out a lot more of the config file.