[U-Boot-Users] [PATCH] Add support for hammerhead AVR32 board

[Haavard Skinnemoen]

Hi Alex,

Please see below for some (delayed) review comments.

On Mon, 17 Mar 2008 11:31:42 +0100
Alex <mailinglist at miromico.ch> wrote:

> I corrected the patch. I tried to run MAKEALL, but it fails, as I don't have all those cross-compilers
> installed. Am I missing something? Do I need to install them, or what is the correct procedure?

You can run MAKEALL for one particular architecture by doing

CROSS_COMPILE=<target>- ./MAKEALL <arch>

where <arch> can be a major architecture (e.g. ppc, arm, etc.) or a
subarchitecture/platform (e.g. 4xx, ARM9, etc.)

> diff -Naur old/u-boot-avr32/board/miromico/hammerhead/eth.c new/u-boot-avr32/board/miromico/hammerhead/eth.c
> --- old/u-boot-avr32/board/miromico/hammerhead/eth.c	1970-01-01 01:00:00.000000000 +0100
> +++ new/u-boot-avr32/board/miromico/hammerhead/eth.c	2008-03-14 16:06:35.000000000 +0100

> +extern int macb_eth_initialize(int id, void *regs, unsigned int phy_addr);

Hrm...we should really move this to a separate header, but let's wait
until the current discussions reach some sort of conclusion.

> +#ifdef CONFIG_CMD_NET
> +void board_eth_initialize(bd_t *bi)
> +{
> +	macb_eth_initialize(0, (void *)MACB0_BASE, bi->bi_phy_id[0]);
> +}
> +#endif
> diff -Naur old/u-boot-avr32/board/miromico/hammerhead/hammerhead.c new/u-boot-avr32/board/miromico/hammerhead/hammerhead.c
> --- old/u-boot-avr32/board/miromico/hammerhead/hammerhead.c	1970-01-01 01:00:00.000000000 +0100
> +++ new/u-boot-avr32/board/miromico/hammerhead/hammerhead.c	2008-03-14 15:34:22.000000000 +0100

> +	/* Select GCLK3 peripheral function. We'll need it as clock output
> +     * for ethernet PHY. */

There's a bit of whitespace damage here and there, as others have
pointed out.

> +void board_init_info(void)
> +{
> +	gd->bd->bi_phy_id[0] = 0x01;
> +}

We should probably get rid of this function. Don't worry about it for
now though.

> diff -Naur old/u-boot-avr32/board/miromico/hammerhead/u-boot.lds new/u-boot-avr32/board/miromico/hammerhead/u-boot.lds
> --- old/u-boot-avr32/board/miromico/hammerhead/u-boot.lds	1970-01-01 01:00:00.000000000 +0100
> +++ new/u-boot-avr32/board/miromico/hammerhead/u-boot.lds	2008-03-14 14:57:37.000000000 +0100

> +	. = ALIGN(32);
> +	__flashprog_start = .;
> +	.flashprog : {
> +		*(.flashprog)
> +	}
> +	. = ALIGN(32);
> +	__flashprog_end = .;

I don't think this block is needed since you're using the CFI flash
driver. It should probably be culled from the ATNGW100 linker script
too.

> diff -Naur old/u-boot-avr32/cpu/at32ap/at32ap700x/gpio.c new/u-boot-avr32/cpu/at32ap/at32ap700x/gpio.c
> --- old/u-boot-avr32/cpu/at32ap/at32ap700x/gpio.c	2008-03-17 10:23:49.000000000 +0100
> +++ new/u-boot-avr32/cpu/at32ap/at32ap700x/gpio.c	2008-03-17 10:10:37.000000000 +0100
> @@ -142,3 +142,14 @@
>   	gpio_select_periph_A(GPIO_PIN_PA15, 0);	/* DATA3 */
>   }
>   #endif
> +
> +#ifdef CONFIG_HAMMERHEAD
> +/*
> + * Hammerhead board uses GCLK3 (Periph A on PB29) as 25MHz clock output
> + * for ethernet PHY.
> + */
> +void gpio_enable_gclk3(void)
> +{
> +  gpio_select_periph_A(GPIO_PIN_PB29, 0); /* GCLK3         */
> +}
> +#endif

Please drop the #ifdef. --gc-sections will take care of it.

> diff -Naur old/u-boot-avr32/cpu/at32ap/cpu.c new/u-boot-avr32/cpu/at32ap/cpu.c
> --- old/u-boot-avr32/cpu/at32ap/cpu.c	2008-03-17 10:23:49.000000000 +0100
> +++ new/u-boot-avr32/cpu/at32ap/cpu.c	2008-03-14 15:43:09.000000000 +0100
> @@ -81,6 +81,16 @@
>   #endif
>   }
> 
> +#ifdef CONFIG_HAMMERHEAD
> +static void gclk_init(void)
> +{
> +  /* Hammerhead boards uses GCLK3 as 25MHz output to ethernet PHY */
> +
> +  /* Enable GCLK3 with no input divider, from OSC0 (crystal) */
> +  sm_writel( PM_GCCTRL3, SM_BIT(CEN) );
> +}
> +#endif

Hmm...could you turn it into something more generic, like

void gclk_enable(unsigned int id)

and call it from the board code? Then you could get rid of the #ifdefs
and let --gc-sections remove it if it turns out to be unused.

> @@ -105,6 +115,10 @@
>   	     p += CFG_ICACHE_LINESZ)
>   		asm volatile("cache %0, 0x02" : "=m"(*p) :: "memory");

Hmm...another piece of code we can get rid of. This is unrelated to
your patch though.

> +#ifdef CONFIG_HAMMERHEAD
> +	gclk_init();
> +#endif

Please remove this if you follow my suggestion above.

> diff -Naur old/u-boot-avr32/cpu/at32ap/sm.h new/u-boot-avr32/cpu/at32ap/sm.h
> --- old/u-boot-avr32/cpu/at32ap/sm.h	2008-03-17 10:23:49.000000000 +0100
> +++ new/u-boot-avr32/cpu/at32ap/sm.h	2008-03-14 15:51:10.000000000 +0100
> @@ -21,7 +21,16 @@
>   #define SM_PM_IMR				0x0048
>   #define SM_PM_ISR				0x004c
>   #define SM_PM_ICR				0x0050
> -#define SM_PM_GCCTRL				0x0060
> +
> +#define SM_PM_GCCTRL0				0x0060
> +#define SM_PM_GCCTRL1				0x0064
> +#define SM_PM_GCCTRL2				0x0068
> +#define SM_PM_GCCTRL3				0x006c
> +#define SM_PM_GCCTRL4				0x0070
> +#define SM_PM_GCCTRL5				0x0074
> +#define SM_PM_GCCTRL6				0x0078
> +#define SM_PM_GCCTRL7				0x007c

Why not

#define SM_PM_GCCTRL(x)				(0x0060 + 4 * (x))

perhaps as a separate patch?

> diff -Naur old/u-boot-avr32/net/eth.c new/u-boot-avr32/net/eth.c
> --- old/u-boot-avr32/net/eth.c	2008-03-17 10:23:50.000000000 +0100
> +++ new/u-boot-avr32/net/eth.c	2008-03-17 10:11:33.000000000 +0100
> @@ -64,6 +64,7 @@
>   extern int mcffec_initialize(bd_t*);
>   extern int mcdmafec_initialize(bd_t*);
>   extern int at91cap9_eth_initialize(bd_t *);
> +extern int board_eth_initialize(bd_t *);
> 
>   #ifdef CONFIG_API
>   extern void (*push_packet)(volatile void *, int);
> @@ -287,6 +288,9 @@
>   #if defined(CONFIG_AT91CAP9)
>   	at91cap9_eth_initialize(bis);
>   #endif
> +#if defined(CONFIG_HAMMERHEAD)
> +	board_eth_initialize(bis);
> +#endif

We probably want something more generic-sounding like
CONFIG_HAS_BOARD_ETH_INITIALIZE so that boards can migrate over to the
new API one by one. But I'll leave it to Ben to decide how to do this.

Haavard