[U-Boot] [PATCH] board: Add support for B&R KWB Motherboard

[Tom Rini]

On Wed, Feb 05, 2014 at 04:47:04PM +0100, Hannes Petermaier wrote:

> Adds support for Bernecker & Rainer Industrieelektronik GmbH KWB
> Motherboard, using TI's AM3352 SoC.
> 
> Most of code is derived from TI's AM335x_EVM
> 
> Signed-off-by: Hannes Petermaier <oe5hpm at oevsv.at>
> Cc: trini at ti.com
> ---
>  board/BuR/bur_kwb/Makefile       |   16 ++
>  board/BuR/bur_kwb/am335xScreen.c |  548 ++++++++++++++++++++++++++++++++++++++
>  board/BuR/bur_kwb/am335xScreen.h |   45 ++++

Minor nit, this should be am335x_screen.[ch]

> +ifeq ($(CONFIG_SPL_BUILD),y)
> +obj-y	:= mux.o
> +endif
> +ifeq ($(CONFIG_SYS_SCREEN),y)
> +obj-y   += am335xScreen.o
> +endif
> +obj-y	+= board.o

obj-$(CONFIG_SPL_BUILD) +=
obj-$(CONFIG_SYS_SCREEN) +=

> diff --git a/board/BuR/bur_kwb/am335xScreen.c b/board/BuR/bur_kwb/am335xScreen.c
[snip]
> +#if defined(CONFIG_SYS_SCREEN) && !defined(CONFIG_SPL_BUILD)

You shouldn't need this.  It'll be discarded if unused.

> +
> +#define DEBUG
> +#ifdef DEBUG
> +#define DBG(...)		printf(__VA_ARGS__)
> +#else
> +#define DBG(...)
> +#endif /* DEBUG */

We provide debug(...) already.

> +#define		SCREEN_HMAX		1366
> +#define		SCREEN_VMAX		768

For consistency, please '#define<space>' globally, thanks.

> +static const unsigned char char_tab2[257*8] = {

A comment about what this is please.

> +static void setPix(struct screen_typ *pscr,
> +		    unsigned int x, unsigned int y, unsigned int color)
> +{
> +	unsigned int addroffset = 0;
> +
> +	/* braces not for function, only for better reading by human :-) */
> +	addroffset = y * (pscr->resx*4) + (x*4);
> +	*(unsigned int *)(pscr->pfb+addroffset) = (color & 0x00FFFFFF);
> +}
> +static void outch(struct screen_typ *pscr,

Newline after functions, fix globally.  Also set_pix(), etc, also fix
globally.

> +++ b/board/BuR/bur_kwb/board.c
[snip]
> +	if (i2c_probe(TPS65217_CHIP_PM)) {
> +		printf("PMIC (0x%02x) not found! skip further initalization.\n",
> +		       TPS65217_CHIP_PM);

You should be able to do this with puts() since it's all constants.

> +	/*
> +	 * Increase USB current limit to 1300mA or 1800mA and set
> +	 * the MPU voltage controller as needed.
> +	 */
> +	if (dpll_mpu_opp100.m == MPUPLL_M_1000) {
> +		usb_cur_lim = TPS65217_USB_INPUT_CUR_LIMIT_1800MA;
> +		mpu_vdd = TPS65217_DCDC_VOLT_SEL_1325MV;
> +	} else {
> +		usb_cur_lim = TPS65217_USB_INPUT_CUR_LIMIT_1300MA;
> +		mpu_vdd = TPS65217_DCDC_VOLT_SEL_1275MV;
> +	}

Since you've got a tps65217 (like beaglebone) you can drop the tps65250
include earlier in the file.

> +++ b/board/BuR/bur_kwb/u-boot.lds

Since you don't support NOR, you should be able to use the generic
linker script.

> +++ b/include/configs/bur_kwb.h
[snip]
> +#include <config_cmd_default.h>
> +/* -------------------------------------------------------------------------*/
> +/* -- undefinig unused features --                                          */
> +/* prior defined from config_defaults.h */
> +#undef	CONFIG_BOOTM_LINUX
> +#undef	CONFIG_BOOTM_NETBSD
> +#undef	CONFIG_BOOTM_PLAN9
> +#undef	CONFIG_BOOTM_RTEMS
> +#undef	CONFIG_GZIP
> +#undef	CONFIG_ZLIB

Space not tab please.

> +#define CONFIG_SYS_NO_FLASH

Duplicate.

> +/* MMC/SD IP block */
> +#if defined(CONFIG_EMMC_BOOT)

So, CONFIG_EMMC_BOOT is used in am335x_evm.h since we have to support
building for a few different variants.  In this case it looks like you
can just drop setting EMMC_BOOT in boards.cfg and then simplify a lot of
the logic in the config file too.

> + #define CONFIG_MMC
> + #define CONFIG_GENERIC_MMC
> + #define CONFIG_OMAP_HSMMC
> + #define CONFIG_CMD_MMC
> + #define CONFIG_SUPPORT_EMMC_BOOT

So, are you making use of 'mmc open' / 'mmc close' on this platform to
work with the eMMC boot partitions today?

> +/*
> + * Our platforms make use of SPL to initalize the hardware (primarily
> + * memory) enough for full U-Boot to be loaded.  We also support Falcon
> + * Mode so that the Linux kernel can be booted directly from SPL
> + * instead, if desired.  We make use of the general SPL framework found
> + * under common/spl/.  Given our generally common memory map, we set a
> + * number of related defaults and sizes here.
> + */
> +#define CONFIG_SPL
> +#define CONFIG_SPL_FRAMEWORK
> +#undef CONFIG_SPL_OS_BOOT

Just don't set CONFIG_SPL_OS_BOOT, and drop the function from board.c
that you've got too.

> +#define CONFIG_SYS_NS16550_COM1		0x44e09000	/* Base EVM has UART0 */
> +#define CONFIG_SYS_NS16550_COM2		0x48022000	/* UART1 */
> +#define CONFIG_SYS_NS16550_COM3		0x48024000	/* UART2 */
> +#define CONFIG_SYS_NS16550_COM4		0x481a6000	/* UART3 */
> +#define CONFIG_SYS_NS16550_COM5		0x481a8000	/* UART4 */
> +#define CONFIG_SYS_NS16550_COM6		0x481aa000	/* UART5 */

Do you use / care about all uarts?

And it looks like there's other parts of the config file that could be
cleaned up a bit too.  Perhaps you can leverage
include/configs/ti_armv7_common.h ?  If not, I'd really like to hear
about the shortcomings there as one of the goals with it is to make
adding new boards easier and have a smaller code footprint.

Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140205/5e53f258/attachment.pgp>