[U-Boot-Users] [PATCH] Add MIMC200 board - now uses board_eth_init()

[Haavard Skinnemoen]

Mark Jackson <mpfj at mimc.co.uk> wrote:
> The MIMC200 board is based on Atmel's NGW100 dev kit,
> but with an extra 8MByte FLASH and 128KByte FRAM.

Do you have a link with some more information about the board?

I also think your mailer mangles whitespace. Thunderbird can apparently
be fixed; please see the instructions here:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/email-clients.txt

> board/atmel/mimc200/Makefile   |   40 +++++++++
> board/atmel/mimc200/config.mk  |    3 +
> board/atmel/mimc200/mimc200.c  |  158 ++++++++++++++++++++++++++++++++++++
> board/atmel/mimc200/u-boot.lds |   73 +++++++++++++++++

Is this really an Atmel board? Note that the directory under "board" is
supposed to indicate the _board_ vendor, not the chip vendor.

> --- a/cpu/at32ap/at32ap700x/clk.c
> +++ b/cpu/at32ap/at32ap700x/clk.c
> @@ -65,4 +65,12 @@ void clk_init(void)
>     /* Use PLL0 as main clock */
>     sm_writel(PM_MCCTRL, SM_BIT(PLLSEL));
> #endif
> +
> +#ifdef CONFIG_MIMC200
> +    // enable gclk outputs
> +    //AVR32_PM.gcctrl[0] = 0x0004; /* LVDS at 10MHz */
> +    sm_writel(PM_GCCTRL, 0x00000004);
> +    //AVR32_PM.gcctrl[1] = 0x0216; /* Ethernet at 25MHz if PLL running */
> +    //sm_writel(PM_GCCTRL + 4, 0x00000216);
> +#endif

Please define a gclk_init() function in your board file and move this
stuff there. That's what Hammerhead ended up doing.

The gclk_init() hook isn't in mainline yet. I'll create a "next" branch
that you can base your work on in the mean time.

> +#ifndef CONFIG_MIMC200
>     gpio_select_periph_A(GPIO_PIN_PC18, 0);    /* SPD    */
> #endif
> +#endif
> }
> 
> void gpio_enable_macb1(void)
> @@ -129,8 +131,10 @@ void gpio_enable_macb1(void)
>     gpio_select_periph_B(GPIO_PIN_PC29, 0);    /* RXD2    */
>     gpio_select_periph_B(GPIO_PIN_PC30, 0);    /* RXD3    */
>     gpio_select_periph_B(GPIO_PIN_PC24, 0);    /* RXCK    */
> +#ifndef CONFIG_MIMC200
>     gpio_select_periph_B(GPIO_PIN_PD15, 0);    /* SPD    */
> #endif
> +#endif

I'd prefer a more generic define here...or possibly some sort of
parameter that can be passed from the board code. Let's leave that for
later though -- this is fine for now.

> +#ifdef CONFIG_MIMC200
> +    // setup Data Flash chip select (NCS2)
> +    hsmc3_writel(MODE2, 0x20121003);
> +    hsmc3_writel(CYCLE2, 0x000a0009);
> +    hsmc3_writel(PULSE2, 0x0a060806);
> +    hsmc3_writel(SETUP2, 0x00030102);
> +
> +    // setup FRAM chip select (NCS3)
> +    hsmc3_writel(MODE3, 0x10120001);
> +    hsmc3_writel(CYCLE3, 0x001e001d);
> +    hsmc3_writel(PULSE3, 0x08040704);
> +    hsmc3_writel(SETUP3, 0x02050204);
> +#endif

Hmm, ok, I guess you currently don't have much choice but put to those
here. Let's make a mental note that this should be improved later.

> void serial_putc(char c)
> {
> +#if defined(CONFIG_MIMC200_DBGLINK)
> +    // only output serial data if DEBUG link connected
> +    // this is connected to PIOE_21
> +    if (gpio_get_value(GPIO_PIN_PE21) == 0)
> +    {
> +#endif

As others have noted, this is pretty ugly. There must be a better way
to do this...but I don't know exactly how. Moving this test to a
separate function and providing a dummy stub for the case when
CONFIG_MIMC200_DBGLINK is not set might be a good first step. Something like

#if defined(CONFIG_MIMC200_DBGLINK)
static int usart_is_disabled(void)
{
	return gpio_get_value(GPIO_PIN_PE21) != 0;
}
#else
static int usart_is_disabled(void)
{
	return 0;
}
#endif

then you can simply do

void serial_putc(char c)
{
	if (usart_is_disabled())
		return;

	/* do the usual stuff here */
}

Alternatively, we could do some tricks involving weak functions here
and move the actual test into the board code.

Thanks for posting this, but please Cc me when posting new avr32 board
patches. I think I missed you first submission.

Haavard