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

[Wolfgang Denk]

Dear Hannes,

In message <1391615224-26493-1-git-send-email-oe5hpm at oevsv.at> you 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 ++++
>  board/BuR/bur_kwb/board.c        |  522 ++++++++++++++++++++++++++++++++++++
>  board/BuR/bur_kwb/board.h        |   18 ++
>  board/BuR/bur_kwb/mux.c          |  213 +++++++++++++++
>  board/BuR/bur_kwb/u-boot.lds     |  101 +++++++

See previous comments.  Repeating the "bur_" part in the board
directory name is redundant; I recommend to drop that.


> + * History
> + * =======
> + * 17.12.2013	hpm		created

Please drop this.  Please fix globally.

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

We already have debug(); please use existing standard facilities.

> +			if ('\r' == *str) {
...
> +			} else if ('\n' == *str) {

Please avoid this reversed notation style.  Please fix globally.

...
> +}
> +int drawrecticle(unsigned int x, unsigned int y,
> +		 unsigned int w, unsigned int h, unsigned int color)
> +{

CodingStyle says: "In source files, separate functions with one blank
line."  Please fix globally.

> +			setPix(&screen, j, i, color);

We do not allow CamelCase identifiers.  Please fix globally.

> +int prints(const char *fmt, ...)
...
> +int printsxy(unsigned int x, unsigned int y, const char *fmt, ...)
etc.

BTW - are you not reinventing the wheel here?  What about functions
like video_drawstring(), video_putchar(), ... ?

> index 0000000..5a1b15f
> --- /dev/null
> +++ b/board/BuR/bur_kwb/board.c
> @@ -0,0 +1,522 @@

Pretty significant parts of this file appear to be identical to code
in board/BuR/bur_tseries/board.c as added by your earlier patch.
Please factor out such common code into a common/ directory so it can
be shared across boards.

> + * History
> + * =======
> + * xx.10.2013	hpm	- created, adopted from ti_am335x_evm
> + * 06.12.2013	hpm	- setting MPU_PLL to 600 MHz (as required for
> + *			  industrial)
> + *			- implemented simple boot-mode detection
> + *			- reduced resetpulse for USB2SD controller
> + *			  from 5ms to 1ms
> + * 10.12.2013	hpm	- switch OFF LCD-Screen within SPL-Stage
> + *			  (due to invalid 3V3 pullup resistor)
> + *			- powerUP 3V3 via I2C-Resetcontroller within SPL-Stage
> + * 16.12.2013	hpm	- V1.10: changed bootaddr of VxWorks from
> + *			  0x82000000 to 0x80100000
> + * 24.12.2013	hpm	- V1.11: added LCD-support
> + * 30.01.2014	hpm	- V2.00: ported to new U-Boot (2014) sources

As mentioned before: please drop globally!


> +		printf("echo setting PMIC Register in the RTC.\n");
> +		*(unsigned int *)0x44e3e098 = 0x1f010;
> +		printf("read back PMIC Register: 0x%08x\n",
> +		       *(unsigned int *)0x44e3e098);
> +
> +		year = *(unsigned int *)0x44e3e014;
> +		mon = *(unsigned int *)0x44e3e010;
> +		d   = *(unsigned int *)0x44e3e00C;
> +		h   = *(unsigned int *)0x44e3e008;
> +		min = *(unsigned int *)0x44e3e004;

Please use proper I/O accessors to access hardware registers, and
declare a proper C struct to describe the register layout.  Please fix
globally.

> +static struct cpsw_slave_data cpsw_slaves[] = {
> +	{
> +		.slave_reg_ofs	= 0x208,
> +		.sliver_reg_ofs	= 0xd80,
...
> +		.slave_reg_ofs	= 0x308,
> +		.sliver_reg_ofs	= 0xdc0,

Would it make sense to provide symboolic names for such magic numbers?

And how about some comments to explain the actual settings?


> diff --git a/board/BuR/bur_kwb/board.h b/board/BuR/bur_kwb/board.h
> new file mode 100644
> index 0000000..def41aa
> --- /dev/null
> +++ b/board/BuR/bur_kwb/board.h
> @@ -0,0 +1,18 @@
> +/*
> + * board.h
> + *
> + * BUR-KWB boards information header
> + *
> + * Copyright (C) 2013, Bernecker & Rainer Industrieelektronik GmbH -
> + * http://www.br-automation.com
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#ifndef _BOARD_H_
> +#define _BOARD_H_
> +
> +void enable_uart0_pin_mux(void);
> +void enable_i2c0_pin_mux(void);
> +void enable_board_pin_mux(void);

This file appears to be identical to board/BuR/bur_tseries/board.h as
added by your earlier patch.  Please use a common header file instead.


> diff --git a/board/BuR/bur_kwb/mux.c b/board/BuR/bur_kwb/mux.c
> new file mode 100644
> index 0000000..da4b9f3
> --- /dev/null
> +++ b/board/BuR/bur_kwb/mux.c
> @@ -0,0 +1,213 @@

> +static struct module_pin_mux uart0_pin_mux[] = {
> +	/* UART0_CTS */
> +	{OFFSET(uart0_ctsn), (MODE(7) | PULLUDEN | PULLUP_EN | RXACTIVE)},
> +	/* UART0_RXD */
> +	{OFFSET(uart0_rxd), (MODE(0) | PULLUDEN | PULLUP_EN | RXACTIVE)},
> +	/* UART0_TXD */
> +	{OFFSET(uart0_txd), (MODE(0) | PULLUDEN)},
> +	{-1},
> +};

Again, there is lots of duplicated code.  Please factor out common
parts.  Please fix globally.

> diff --git a/board/BuR/bur_kwb/u-boot.lds b/board/BuR/bur_kwb/u-boot.lds
> new file mode 100644
> index 0000000..021f869
> --- /dev/null
> +++ b/board/BuR/bur_kwb/u-boot.lds

Not needed?  Drop??


> diff --git a/include/configs/bur_kwb.h b/include/configs/bur_kwb.h
> new file mode 100644
> index 0000000..66048dd
> --- /dev/null
> +++ b/include/configs/bur_kwb.h

Large duplicated sections again...


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If the facts don't fit the theory, change the facts.
                                                   -- Albert Einstein