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

[David Jander]

Dear Wolfgang,

On Thursday 19 August 2010 03:03:54 pm Wolfgang Denk wrote:
>[...]
> >  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.

Oops. Thanks for pointing out. Will add maintainer info.

> > +	/* 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.

Ok. Will fix that.

> > +	/* 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.

Ok.

>[...]
> > +	pmic_reg_write(REG_POWER_MISC, GPO4ADIN);
> 
> It would really be great if someone cold clean up this mess in
> "include/fsl_pmic.h"

Yes, but fixing it will probably break a lot of other code. Am I supposed to 
do that and fix everything that I can find breaking and hope for the rest 
(which I can't test myself)?

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

I agree.

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

I agree. Should I propose a fix first (could be a lot of work)?

> > +	/* 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?

Yes I do. It was copied almost literally from the MX51EVK code, and somehow I 
think this should go elsewhere anyway....
I'll see what I can come up with.

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

Ok.

> > +#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.

Ok.

> > +	/* 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.

Again, this is copy/pasted from MX51EVK code somewhere. Will try to improve 
this.

> ...
> 
> > +#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.]

Ok.

> > +		"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.

Ooops. This wasn't meant to be here :-(
Anyway, I am currently working on implementing access to SPI-NOR-flash, and 
this was there only for convinience while developing on one board. It was 
meant to be removed, though.
Btw, I am also trying to fix the awfully broken mxc_spi driver. The spi_xfer() 
function assumes 32-bit word transfers (because the hardware does it like 
this, and FSL didn't bother adapting the driver to linux/u-boot spi 
conventions), so I need to introduce a second spi_xfer_fsl() function for all 
existing, but broken device drivers that assume fsl-byte-ordering (like the 
PMIC driver) and fix the original spi_xfer(). Is that acceptable?

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

This also wasn't supposed to be here....

> > +#define CONFIG_ARP_TIMEOUT	200UL
> 
> Is this really needed on your hardware?

Honestly I didn't even know what this is for. It's from MX51EVK.

> > +#define CONFIG_ENV_SECT_SIZE    (128 * 1024)
> > +#define CONFIG_ENV_SIZE		CONFIG_ENV_SECT_SIZE
> 
> Please use TABs for vertical alignment.

Excuse this dumb question about u-boot coding-style: Are tabs supposed to be 
used 8-spaces wide?

And I thought my version was much cleaner than the MX51EVK BSP which is 
already in u-boot.... :-(

Best regards,

-- 
David Jander
Protonic Holland.