[U-Boot] [PATCH 4/4] Added initial support for PRTLVT2-based boards.

[Wolfgang Denk]

Dear David Jander,

In message <00b6eb8822a36b67a5e6154fd256e51e605fe47a.1282213859.git.david at protonic.nl> you wrote:
>
>  board/Protonic/prtlvt2/Makefile     |   48 ++++
>  board/Protonic/prtlvt2/config.mk    |   25 ++
>  board/Protonic/prtlvt2/imximage.cfg |  171 ++++++++++++
>  board/Protonic/prtlvt2/prtlvt2.c    |  513 +++++++++++++++++++++++++++++++++++
>  board/Protonic/prtlvt2/prtlvt2.h    |   50 ++++
>  boards.cfg                          |    1 +
>  include/configs/prtlvt2.h           |  203 ++++++++++++++
>  7 files changed, 1011 insertions(+), 0 deletions(-)
>  create mode 100644 board/Protonic/prtlvt2/Makefile
>  create mode 100644 board/Protonic/prtlvt2/config.mk
>  create mode 100644 board/Protonic/prtlvt2/imximage.cfg
>  create mode 100644 board/Protonic/prtlvt2/prtlvt2.c
>  create mode 100644 board/Protonic/prtlvt2/prtlvt2.h
>  create mode 100644 include/configs/prtlvt2.h

Entry to MAINTAINERS missing.

...
> +	/* SCL_2V8, SDA_2V8 on KEY_COL4 and KEY_COL5 */
> +	{MX51_PIN_KEY_COL4,    IOMUX_CONFIG_ALT3, PIO_OD, MUX_IN_I2C2_IPP_SCL_IN_SELECT_INPUT, INPUT_CTL_PATH1},
> +	{MX51_PIN_KEY_COL5,    IOMUX_CONFIG_ALT3, PIO_OD, MUX_IN_I2C2_IPP_SCL_IN_SELECT_INPUT, INPUT_CTL_PATH1},

Lines too long, please fix globally.

...
> +	/* Write needed to update Charger 0 */
> +	/* Charge voltage=4.2V, Current=full-on, Plim=800mW, charger sw, battfet off */

Incorrect multiline comment format, please fix globally.

> +	pmic_reg_write(REG_CHARGE, VCHRG0 | VCHRG1 /*| VCHRG2 */ |
> +		ICHRG0 | ICHRG1 |  ICHRG2 | ICHRG3 | FETOVRD |
> +		PLIM0 /* |  PLIM1 */ |
> +	    CHRGLEDEN | CHGAUTOB | CYCLB);
> +	
> +	/* Configure Power Control, enable PWRON switches */
> +	pmic_reg_write(REG_POWER_CTL2, RESTARTEN | 
> +		PWRON1RSTEN | PWRON2RSTEN | PWRON3RSTEN |
> +		PWRON1DBNC0 | PWRON2DBNC0 | PWRON3DBNC0 | 
> +		WDIRESET | STBYDLY0);
> +	
> +	/* power up the system first */
> +	// FIXME: Is this bit still there in rev 2?
> +	// This bit was called PWUP??

No C++ comments, please!

> +	pmic_reg_write(REG_POWER_MISC, GPO4ADIN);

It would really be great if someone cold clean up this mess in
"include/fsl_pmic.h"

Using an "enum" for register definitions is just horrible.

I am well aware that you did not introduce this code, but reading this
feels is if my nails are rolling up.

> +	/* Set core voltage to 1.1V */
> +	val = pmic_reg_read(REG_SW_0);
> +	val = (val & (~0x80001F)) | 0x14;
> +	pmic_reg_write(REG_SW_0, val);
> +
> +	/* Setup VCC (SW2) to 1.225 */
> +	val = pmic_reg_read(REG_SW_1);
> +	val = (val & (~0x80001F)) | 0x19;
> +	pmic_reg_write(REG_SW_1, val);
> +
> +	/* Setup 1V2_DIG1 (SW3) to 1.2 */
> +	val = pmic_reg_read(REG_SW_2);
> +	val = (val & (~0x80001F)) | 0x18;
> +	pmic_reg_write(REG_SW_2, val);
> +	udelay(50);

Don't you feel the need to intr=oduce some readable symbolic
constants for all these magic numbers here?

> +	/* Raise the core frequency to 800MHz */
> +	/* printf("Core at 400 MHz!\n"); */
> +	/* writel(0x1, &mxc_ccm->cacrr); */

Please remove dead code.

> +#if 0 /* FIXME: This shouldn't be changed for PRTLVT2 */
> +	/* Set VDIG to 1.25V, VGEN3 to 1.8V, VCAM to 2.6V */
> +	val = pmic_reg_read(REG_SETTING_0);
> +	val &= ~(VCAM_MASK | VGEN3_MASK | VDIG_MASK);
> +	val |= VDIG_1_25 | VGEN3_1_8 | VCAM_2_6;
> +	pmic_reg_write(REG_SETTING_0, val);
> +#endif

Please remove dead code.

> +	/* Turn on backlight */
> +	val = readl(GPIO1_BASE_ADDR + 0x0);
> +	val |= 0x00000004;  /* Make GPIO1_2 high (BLEN) */
> +	writel(val, GPIO1_BASE_ADDR + 0x0);
> +
> +	val = readl(GPIO1_BASE_ADDR + 0x4);
> +	val |= 0x00000004;	/* configure GPIO line as output */
> +	writel(val, GPIO1_BASE_ADDR + 0x4);

We don't accept register base plus offset notation any more. Please
use a C struct to describe the register layout.  Please fix globally.

...
> +#define	CONFIG_EXTRA_ENV_SETTINGS					\
> +		"netdev=eth0\0"						\
> +		"uboot_addr=0xa0000000\0"				\
> +		"uboot=u-boot.bin\0"			\
> +		"loadaddr=0x90800000\0"			\
> +		"bootargs_base=setenv bootargs console=tty "\
> +			"console=ttymxc0,${baudrate}\0"\
> +		"bootargs_nfs=setenv bootargs ${bootargs} root=/dev/nfs "\
> +			"ip=${ipaddr} nfsroot=${nfsserverip}:${nfsroot},v3,tcp\0"\
> +		"bootcmd_net=run bootargs_base bootargs_nfs; "		\
> +			"tftpboot ${loadaddr} ${kernel}; bootm\0" \
> +        "bootcmd_SD=run bootcmd_SD1 bootcmd_SD2\0" \

Indentation by TAB only, please. [Check & fix globally, please.]


> +		"ethaddr=00:00:00:00:00:00\0" \
> +		"ipaddr=192.168.1.244\0" \
> +		"serverip=192.168.1.132\0" \
> +		"nfsserverip=192.168.1.160\0" \

NAK!!

We do not allow network parameters in the default environment (and
especially not broken/incorrect MAC addresses.

> +		"nfsroot=/srv/home/david/Devel/Sandboxes/LEL/XDH/nfsroot\0" \
> +		"kernel=linux-2.6.31-prtlvt2.uImage\0" \

Do you think it makes any sense to your customers to encode your local
settings like "/srv/home/david/Devel/Sandboxes/LEL/XDH/nfsroot" in the
default environment?

I doubt that.

> +#define CONFIG_ARP_TIMEOUT	200UL

Is this really needed on your hardware?



> +#define CONFIG_ENV_SECT_SIZE    (128 * 1024)
> +#define CONFIG_ENV_SIZE		CONFIG_ENV_SECT_SIZE

Please use TABs for vertical alignment.

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 some day we are defeated, well, war has  its  fortunes,  good  and
bad.
	-- Commander Kor, "Errand of Mercy", stardate 3201.7