[U-Boot] [PATCH] Support for Embedtronics/MRFSA board.

Albin Tonnerre albin.tonnerre at free-electrons.com
Sat Aug 29 21:05:36 CEST 2009


Hi Sami,

Please note that I'm not an u-boot custodian, so what follows is by no means
authoritative.
However, I do have some comments about your code:

On Sat, Aug 29, 2009 at 08:22:09PM +0300, Sami Kantoluoto wrote :
> diff --git a/board/embedtronics/mrfsa/mrfsa.c b/board/embedtronics/mrfsa/mrfsa.c
> new file mode 100644
> index 0000000..6420c4b
> --- /dev/null
> +++ b/board/embedtronics/mrfsa/mrfsa.c
> @@ -0,0 +1,481 @@
> +/*
> + * (C) Copyright 2007-2008
> + * Stelian Pop <stelian.pop at leadtechdesign.com>
> + * Lead Tech Design <www.leadtechdesign.com>
> + *
> + * (C) Copyright 2009
> + * Sami Kantoluoto <sami.kantoluoto at embedtronics.fi>
> + * Embedtronics Oy <www.embedtronics.fi>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <asm/arch/at91_common.h>
> +#include <asm/arch/at91sam9260.h>
> +#include <asm/arch/at91sam9260_matrix.h>
> +#include <asm/arch/at91sam9_smc.h>
> +#include <asm/arch/at91_pmc.h>
> +#include <asm/arch/at91_wdt.h>
> +#include <asm/arch/at91_rstc.h>
> +#include <asm/arch/hardware.h>
> +#include <asm/arch/at91_spi.h>
> +#include <asm/arch/at91_pio.h>
> +#include <asm/arch/gpio.h>
> +#include <asm/arch/io.h>
> +#if defined(CONFIG_RESET_PHY_R) || defined(CONFIG_MACB)
> +#include <net.h>
> +#include <netdev.h>
> +#endif
> +
> +#include "ks8995ma.h"
> +#include "iopins.h"
> +
> +#ifndef	CONFIG_NO_VLANS
> +#define	CONFIG_NO_VLANS	1
> +#endif
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/* ------------------------------------------------------------------------- */
> +/*
> + * Miscelaneous platform dependent initialisations
> + */
> +
> +static void mrfsa_serial_hw_init (void)
> +{
> +#ifdef CONFIG_USART0
> +	at91_set_A_periph (AT91_PIN_PB4, 1);	/* TXD0 */
> +	at91_set_A_periph (AT91_PIN_PB5, 0);	/* RXD0 */
> +	at91_sys_write (AT91_PMC_PCER, 1 << AT91_ID_US0);
> +#endif
> +
> +#ifdef CONFIG_USART1
> +	at91_set_A_periph (AT91_PIN_PB6, 1);	/* TXD1 */
> +	at91_set_A_periph (AT91_PIN_PB7, 0);	/* RXD1 */
> +	at91_sys_write (AT91_PMC_PCER, 1 << AT91_ID_US1);
> +#endif
> +
> +#ifdef CONFIG_USART2
> +	at91_set_A_periph (AT91_PIN_PB8, 1);	/* TXD2 */
> +	at91_set_A_periph (AT91_PIN_PB9, 0);	/* RXD2 */
> +	at91_sys_write (AT91_PMC_PCER, 1 << AT91_ID_US2);
> +#endif
> +
> +#ifdef CONFIG_USART3		/* DBGU */
> +	at91_set_A_periph (AT91_PIN_PB14, 0);	/* DRXD */
> +	at91_set_A_periph (AT91_PIN_PB15, 1);	/* DTXD */
> +	at91_sys_write (AT91_PMC_PCER, 1 << AT91_ID_SYS);
> +#endif
> +}

There's already an at91_serial_hw_init() function which exactly performs that,
why don't you use it?

> +static void mrfsa_spi_hw_init (void)
> +{
> +	unsigned long mode;
> +
> +	/* Enable clock for SPI */
> +	at91_sys_write (AT91_PMC_PCER, 1 << AT91SAM9260_ID_SPI0);
> +
> +	at91_set_A_periph (AT91_PIN_PA3, 1);	/* SPI0_NPCS0 */
> +	at91_set_B_periph (AT91_PIN_PC11, 1);	/* SPI0_NPCS1 */
> +	at91_set_gpio_output (PIN_SPI0_CSETH, 1);	/* CSETH */
> +	at91_set_gpio_output (PIN_SPI0_CSSR, 1);	/* CSSR */
> +	at91_set_gpio_output (PIN_SPI0_CSZB, 1);	/* CSZB */
> +
> +	at91_set_A_periph (AT91_PIN_PA0, 0);	/* SPI0_MISO */
> +	at91_set_A_periph (AT91_PIN_PA1, 0);	/* SPI0_MOSI */
> +	at91_set_A_periph (AT91_PIN_PA2, 0);	/* SPI0_SPCK */
> +}
> +

Same here. at91_spi0_hw_init() seems to do a large part of that, you should
consider using it

> +static int spi_transfer (unsigned csr, unsigned cspin, const void *src,
> +			 void *dst, int len)
> +{
> +	unsigned long mode;
> +	unsigned int timeout;
> +
> +	/* make sure SPI is not clocking data */
> +	writel (AT91_SPI_TXTDIS + AT91_SPI_RXTDIS,
> +		AT91_BASE_SPI + AT91_SPI_PTCR);
> +
> +	/* select slave parameters */
> +	mode = readl (AT91_BASE_SPI + AT91_SPI_MR);
> +	mode &= 0xfff0ffff;
> +	writel (mode | (((0xf & ~(1 << csr)) << 16) & AT91_SPI_PCS),
> +		AT91_BASE_SPI + AT91_SPI_MR);
> +
> +	/* Initialize the Transmit and Receive Pointer */
> +	writel ((unsigned int)dst, AT91_BASE_SPI + AT91_SPI_RPR);
> +	writel ((unsigned int)src, AT91_BASE_SPI + AT91_SPI_TPR);
> +
> +	/* Initialize the Transmit and Receive Counters */
> +	writel (len, AT91_BASE_SPI + AT91_SPI_RCR);
> +	writel (len, AT91_BASE_SPI + AT91_SPI_TCR);
> +
> +	/* arm simple, non interrupt dependent timer */
> +	reset_timer_masked ();
> +	timeout = 0;
> +
> +	/* select slave */
> +	at91_set_gpio_value (cspin, 0);
> +
> +	/* transmit data */
> +	writel (AT91_SPI_TXTEN + AT91_SPI_RXTEN, AT91_BASE_SPI + AT91_SPI_PTCR);
> +	while (!(readl (AT91_BASE_SPI + AT91_SPI_SR) & AT91_SPI_RXBUFF) &&
> +	       ((timeout = get_timer_masked ()) < CONFIG_SYS_SPI_WRITE_TOUT)) ;
> +	writel (AT91_SPI_TXTDIS + AT91_SPI_RXTDIS,
> +		AT91_BASE_SPI + AT91_SPI_PTCR);
> +
> +	/* deselect slave */
> +	at91_set_gpio_value (cspin, 1);
> +
> +	/* return error on failure */
> +	if (timeout >= CONFIG_SYS_SPI_WRITE_TOUT) {
> +		printf ("Error Timeout\n\r");
> +		return -1;
> +	}
> +
> +	/* success! */
> +	return 0;
> +}
> +

Why do you need this one ? drivers/spi/atmel_spi.c should provide you with the
spi_xfer function, which likely does what you want. All you need is

#define CONFIG_ATMEL_SPI in your board config.h

> +int dram_init (void)
> +{
> +	gd->bd->bi_dram[0].start = PHYS_SDRAM;
> +	gd->bd->bi_dram[0].size = PHYS_SDRAM_SIZE;
> +	return 0;
> +}

You should use get_ram_size() here, as it will help detecting is your RAM is
somehow damaged.

> +#endif /* CONFIG_HW_WATCHDOG */
> diff --git a/common/console.c b/common/console.c
> index 867c12c..fb6e409 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -311,6 +311,12 @@ int getc(void)
>  
>  int tstc(void)
>  {
> +#ifdef  CONFIG_HW_WATCHDOG
> +	hw_watchdog_reset();
> +#endif
> +#ifdef  CONFIG_WATCHDOG
> +	watchdog_reset();
> +#endif
>  #ifdef CONFIG_DISABLE_CONSOLE
>  	if (gd->flags & GD_FLG_DISABLE_CONSOLE)
>  		return 0;
> diff --git a/cpu/arm926ejs/at91/timer.c b/cpu/arm926ejs/at91/timer.c
> index 811bb3c..03fce54 100644
> --- a/cpu/arm926ejs/at91/timer.c
> +++ b/cpu/arm926ejs/at91/timer.c
> @@ -83,7 +83,12 @@ int timer_init(void)
>  unsigned long long get_ticks(void)
>  {
>  	ulong now = READ_TIMER;
> -
> +#ifdef  CONFIG_HW_WATCHDOG
> +	hw_watchdog_reset();
> +#endif
> +#ifdef  CONFIG_WATCHDOG
> +	watchdog_reset();
> +#endif
>  	if (now >= lastinc)	/* normal mode (non roll) */
>  		/* move stamp forward with absolut diff ticks */
>  		timestamp += (now - lastinc);

You should provide a rationale for making such a change. So far this seems to
have worked for everyone, so your change is unlikely to be accepted unless you
justify why this is needed.

> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> index c184353..9e2e771 100644
> --- a/drivers/net/macb.c
> +++ b/drivers/net/macb.c
> @@ -291,6 +291,7 @@ static int macb_recv(struct eth_device *netdev)
>  	return 0;
>  }
>  
> +#ifndef	CONFIG_NO_PHY
>  static void macb_phy_reset(struct macb_device *macb)
>  {
>  	struct eth_device *netdev = &macb->netdev;
> @@ -403,6 +404,7 @@ static int macb_phy_init(struct macb_device *macb)
>  		return 1;
>  	}
>  }
> +#endif	/* CONFIG_NO_PHY */
>  
>  static int macb_init(struct eth_device *netdev, bd_t *bd)
>  {
> @@ -463,8 +465,10 @@ static int macb_init(struct eth_device *netdev, bd_t *bd)
>  #endif
>  #endif /* CONFIG_RMII */
>  
> +#ifndef	CONFIG_NO_PHY
>  	if (!macb_phy_init(macb))
>  		return -1;
> +#endif	/* CONFIG_NO_PHY */
>  
>  	/* Enable TX and RX */
>  	macb_writel(macb, NCR, MACB_BIT(TE) | MACB_BIT(RE));

Same here. You should explain what problems you're trying to fix.

> diff --git a/include/configs/mrfsa.h b/include/configs/mrfsa.h
> new file mode 100644
> index 0000000..83ad18b
> --- /dev/null
> +++ b/include/configs/mrfsa.h
> @@ -0,0 +1,197 @@
> +/*
> + * (C) Copyright 2009
> + * Sami Kantoluoto <sami.kantoluoto at embedtronics.fi>
> + * Embedtronics Oy <www.embedtronics.fi>
> + *
> + * Configuation settings for the MRFSA board.
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#ifndef __CONFIG_H
> +#define __CONFIG_H
> +
> +/* ARM asynchronous clock */
> +#define AT91_MAIN_CLOCK		20000000	/* 20.000 MHz crystal */
> +#define AT91C_MASTER_CLOCK	133333333	/* peripheral clock */

AT91C_MASTER_CLOCK is not needed

> +#define CONFIG_SYS_HZ		1000
> +
> +#define CONFIG_ARM926EJS	1	/* This is an ARM926EJS Core    */
> +
> +#define AT91_CPU_NAME		"AT91SAM9G20"

Neither is AT91_CPU_NAME

> +#define CONFIG_AT91SAM9G20	1	/* It's an Atmel AT91SAM9G20 SoC */
> +
> +#define CONFIG_ARCH_CPU_INIT
> +#undef CONFIG_USE_IRQ		/* we don't need IRQ/FIQ stuff  */
> +
> +#define CONFIG_CMDLINE_TAG	1	/* enable passing of ATAGs      */
> +#define CONFIG_SETUP_MEMORY_TAGS 1
> +#define CONFIG_INITRD_TAG	1
> +
> +#define CONFIG_SKIP_LOWLEVEL_INIT
> +#define CONFIG_SKIP_RELOCATE_UBOOT
> +
> +/*
> + * Hardware drivers
> + */
> +#define CONFIG_ATMEL_USART	1
> +#undef CONFIG_USART0
> +#undef CONFIG_USART1
> +#undef CONFIG_USART2
> +#define CONFIG_USART3		1	/* USART 3 is DBGU */
> +
> +/* LED */
> +#undef	CONFIG_AT91_LED
> +
> +#define CONFIG_BOOTDELAY	3
> +
> +/*
> + * BOOTP options
> + */
> +#define CONFIG_BOOTP_BOOTFILESIZE	1
> +#define CONFIG_BOOTP_BOOTPATH		1
> +#define CONFIG_BOOTP_GATEWAY		1
> +#define CONFIG_BOOTP_HOSTNAME		1
> +
> +/*
> + * Command line configuration.
> + */
> +#include <config_cmd_default.h>
> +#undef CONFIG_CMD_BDI
> +#undef CONFIG_CMD_FPGA
> +#undef CONFIG_CMD_IMI
> +#undef CONFIG_CMD_IMLS
> +#undef CONFIG_CMD_LOADS
> +#undef CONFIG_CMD_SOURCE
> +
> +#define	CONFIG_CMD_MMC		1
> +#define CONFIG_CMD_PING		1
> +#define CONFIG_CMD_DHCP		1
> +#define CONFIG_CMD_USB		1
> +#define	CONFIG_CMD_FAT		1
> +#define	CONFIG_CMD_EXT2			1
> +
> +#define	CONFIG_MMC		1
> +#define	CONFIG_ATMEL_MCI	1
> +
> +#define	CONFIG_DOS_PARTITION		1	/* support DOS partition table */
> +
> +/* SDRAM */
> +#define CONFIG_NR_DRAM_BANKS		1
> +#define PHYS_SDRAM			0x20000000
> +#define PHYS_SDRAM_SIZE			0x04000000	/* 64 megs */
> +
> +/* DataFlash */
> +#define CONFIG_ATMEL_DATAFLASH_SPI
> +#define CONFIG_HAS_DATAFLASH		1
> +#define CONFIG_SYS_SPI_WRITE_TOUT		(5*CONFIG_SYS_HZ)
> +#define CONFIG_SYS_MAX_DATAFLASH_BANKS		2
> +#define CONFIG_SYS_DATAFLASH_LOGIC_ADDR_CS0	0xC0000000	/* CS0 */
> +#define CONFIG_SYS_DATAFLASH_LOGIC_ADDR_CS1	0xD0000000	/* CS1 */
> +#define AT91_SPI_CLK			15000000
> +
> +#define DATAFLASH_TCSS			(0x22 << 16)
> +#define DATAFLASH_TCHS			(0x1 << 24)
> +
> +/* NAND flash - no nand flash on this board */
> +#ifdef CONFIG_CMD_NAND
> +#error No NAND FLASH
> +#endif
> +#define	CONFIG_SYS_NO_NAND			1

There's no such thing as CONFIG_SYS_NO_NAND

> +/* NOR flash - no real flash on this board */
> +#define CONFIG_SYS_NO_FLASH			1
> +
> +/* Ethernet */
> +#define CONFIG_MACB			1
> +#define	CONFIG_NO_PHY			1
> +#undef	CONFIG_RMII
> +#define	CONFIG_NET_MULTI		1
> +#define CONFIG_NET_RETRY_COUNT		20
> +#define CONFIG_RESET_PHY_R		1
> +
> +/* USB */
> +#define CONFIG_USB_ATMEL
> +#define CONFIG_USB_OHCI_NEW		1
> +#define CONFIG_DOS_PARTITION		1
> +#define CONFIG_SYS_USB_OHCI_CPU_INIT		1
> +#define CONFIG_SYS_USB_OHCI_REGS_BASE		0x00500000	/* AT91SAM9260_UHP_BASE */
> +#define CONFIG_SYS_USB_OHCI_SLOT_NAME		"at91sam9g20"
> +#define CONFIG_SYS_USB_OHCI_MAX_ROOT_PORTS	2
> +#define CONFIG_USB_STORAGE		1
> +
> +#define CONFIG_SYS_LOAD_ADDR			0x22000000	/* load address */
> +
> +#define CONFIG_SYS_MEMTEST_START		PHYS_SDRAM
> +#define CONFIG_SYS_MEMTEST_END			0x23e00000
> +
> +#ifdef CONFIG_SYS_USE_DATAFLASH_CS0
> +
> +/* bootstrap + u-boot + env + linux in dataflash on CS0 */
> +#define CONFIG_ENV_IS_IN_DATAFLASH	1
> +#define CONFIG_SYS_MONITOR_BASE	(CONFIG_SYS_DATAFLASH_LOGIC_ADDR_CS0 + 0x8400)
> +#define CONFIG_ENV_OFFSET		0x4200
> +#define CONFIG_ENV_ADDR		(CONFIG_SYS_DATAFLASH_LOGIC_ADDR_CS0 + CONFIG_ENV_OFFSET)
> +#define CONFIG_ENV_SIZE		0x4200
> +#define CONFIG_BOOTCOMMAND	"cp.b 0xC0042000 0x22000000 0x210000; bootm"
> +#define CONFIG_BOOTARGS		"console=ttyS0,115200 "			\
> +				"root=/dev/mmcblk0p1 rootwait rw"
> +
> +#elif CONFIG_SYS_USE_DATAFLASH_CS1
> +
> +/* bootstrap + u-boot + env + linux in dataflash on CS1 */
> +#define CONFIG_ENV_IS_IN_DATAFLASH	1
> +#define CONFIG_SYS_MONITOR_BASE	(CONFIG_SYS_DATAFLASH_LOGIC_ADDR_CS1 + 0x8400)
> +#define CONFIG_ENV_OFFSET		0x4200
> +#define CONFIG_ENV_ADDR		(CONFIG_SYS_DATAFLASH_LOGIC_ADDR_CS1 + CONFIG_ENV_OFFSET)
> +#define CONFIG_ENV_SIZE		0x4200
> +#define CONFIG_BOOTCOMMAND	"cp.b 0xD0042000 0x22000000 0x210000; bootm"
> +#define CONFIG_BOOTARGS		"console=ttyS0,115200 "			\
> +				"root=/dev/mmcblk0p1 rootwait rw"
> +
> +#endif
> +
> +#define CONFIG_BAUDRATE		115200
> +#define CONFIG_SYS_BAUDRATE_TABLE	{115200 , 19200, 38400, 57600, 9600 }
> +
> +#define CONFIG_SYS_PROMPT		"U-Boot> "
> +#define CONFIG_SYS_CBSIZE		256
> +#define CONFIG_SYS_MAXARGS		16
> +#define CONFIG_SYS_PBSIZE		(CONFIG_SYS_CBSIZE + sizeof(CONFIG_SYS_PROMPT) + 16)
> +#define CONFIG_SYS_LONGHELP		1
> +#define CONFIG_CMDLINE_EDITING	1
> +
> +/*
> + * Size of malloc() pool
> + */
> +#define CONFIG_SYS_MALLOC_LEN		ROUND(3 * CONFIG_ENV_SIZE + 128*1024, 0x1000)
> +#define CONFIG_SYS_GBL_DATA_SIZE	128	/* 128 bytes for initial data */
> +
> +#define CONFIG_STACKSIZE	(32*1024)	/* regular stack */
> +
> +#ifdef CONFIG_USE_IRQ
> +#error CONFIG_USE_IRQ not supported
> +#endif
> +
> +#define	CONFIG_HW_WATCHDOG		1	/* we've hardware watchdog */
> +
> +#define	BOARD_LATE_INIT			1
> +#define	MMCI_BASE			AT91SAM9260_BASE_MCI

As per my previous email, this really should be done at some common level.
Defining this - especially on boards which have only 1 controller - in every
config file is nothing but duplication

> +#endif
> diff --git a/lib_arm/bootm.c b/lib_arm/bootm.c
> index 128b7e3..e882d9b 100644
> --- a/lib_arm/bootm.c
> +++ b/lib_arm/bootm.c
> @@ -27,6 +27,11 @@
>  #include <u-boot/zlib.h>
>  #include <asm/byteorder.h>
>  
> +#if defined(CONFIG_HW_WATCHDOG) || defined(CONFIG_WATCHDOG)
> +#include <watchdog.h>
> +#endif
> +
> +
>  DECLARE_GLOBAL_DATA_PTR;
>  
>  #if defined (CONFIG_SETUP_MEMORY_TAGS) || \
> @@ -123,8 +128,16 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
>  	}
>  #endif
>  
> +#if defined(CONFIG_HW_WATCHDOG) || defined(CONFIG_WATCHDOG)
> +	WATCHDOG_RESET();
> +#endif
> +
>  	cleanup_before_linux ();
>  
> +#if defined(CONFIG_HW_WATCHDOG) || defined(CONFIG_WATCHDOG)
> +	WATCHDOG_RESET();
> +#endif
> +
>  	theKernel (0, machid, bd->bi_boot_params);
>  	/* does not return */

You might want to provide a rationale for this change, too.

Regards,
-- 
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com


More information about the U-Boot mailing list