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

[David Jander]

On Friday 20 August 2010 11:51:18 am Stefano Babic wrote:
> Hi David,
> 
> in addition to Wolfgang's comments:
> > +static u32 system_rev;
> > +struct io_board_ctrl *mx51_io_board;
> 
> Structure is not used, and probably does not match your board. You
> should drop it.

Ok, I was already wondering what this did here.... I'll just remove it.

> > +#define POUT_HS (PAD_CTL_DRV_HIGH | PAD_CTL_SRE_FAST)
> > +#define POUT_MS (PAD_CTL_DRV_MAX | PAD_CTL_SRE_FAST)
> > +#define POUT_LS (PAD_CTL_DRV_MEDIUM)
> > +#define PIN_HYS (PAD_CTL_HYS_ENABLE)
> > +#define PIN_HYS_PULL (PAD_CTL_HYS_ENABLE | PAD_CTL_PKE_ENABLE |
> > PAD_CTL_PUE_PULL) +#define PIN_HYS_KEEP (PAD_CTL_HYS_ENABLE |
> > PAD_CTL_PKE_ENABLE)
> > +#define PIO_OD  (PIN_HYS_PULL | PAD_CTL_22K_PU |
> > PAD_CTL_ODE_OPENDRAIN_ENABLE | PAD_CTL_DRV_MEDIUM)
> 
> Consider to put these defines in include/asm/arch-mx51/iomux.h. They
> could be usefult for other boards, too.

Hmmm. In that case they should have other names, and one should probably make 
the set complete with all combinations, and I doubt it'll make sense anymore 
then....
This subset of possibilities is too specific to our board I fear.
I had already been complaining about some seemingly arbitrary combinations of 
IO-pad settings being used in SoC-driver code in the linux kernel. IMHO, one 
should never choose a specific combination of settings without excactly 
knowing it is electrically correct for that specific pin on that specific 
board... not only because it works, but also from EMC considerations.
I also think, that in theory this would make the kernel board-specific in that 
case... not something one would want I believe. For that reason, I keep 
thinking that correct and thorough IO-pad setup specific for each board should 
be done in the boot-loader.... never in the kernel.

OTOH, maybe one could come up with some commonly-used combinations... but 
which ones?

> > +	/* Raise the core frequency to 800MHz */
> > +	/* printf("Core at 400 MHz!\n"); */
> > +	/* writel(0x1, &mxc_ccm->cacrr); */
> > +	writel(0x0, &mxc_ccm->cacrr);
> 
> Comment is misleading. Remove what is not needed.

Sorry, I was too lazy to make a configuration option out of this... will fix 
it.

> > +	/* Setup other io's */
> > +	for(t=0; other_io_conf[t].pin>=0; t++) {
> 
> Add spaces when needed.

Ok.

> > +#ifdef BOARD_LATE_INIT
> 
> Probably it is better (and in mx51evk, too) to remove the #ifdef,
> because we need to initialize the pmic, else some functionalities cannot
> work. It is mandatory that power_init is called.

I agree. I had always been "about to remove it" ;-)

> > diff --git a/board/Protonic/prtlvt2/prtlvt2.h
> > b/board/Protonic/prtlvt2/prtlvt2.h +#ifndef
> > __BOARD_FREESCALE_MX51_EVK_H__
> > +#define __BOARD_FREESCALE_MX51_EVK_H__
> > +
> > +#ifndef __ASSEMBLY__
> > +struct io_board_ctrl {
> > +	u16 led_ctrl;		/* 0x00 */
> > +	u16 resv1[0x03];
> > +	u16 sb_stat;		/* 0x08 */
> > +	u16 resv2[0x03];
> > +	u16 int_stat;		/* 0x10 */
> > +	u16 resv3[0x07];
> > +	u16 int_rest;		/* 0x20 */
> > +	u16 resv4[0x0B];
> > +	u16 int_mask;		/* 0x38 */
> > +	u16 resv5[0x03];
> > +	u16 id1;		/* 0x40 */
> > +	u16 resv6[0x03];
> > +	u16 id2;		/* 0x48 */
> > +	u16 resv7[0x03];
> > +	u16 version;		/* 0x50 */
> > +	u16 resv8[0x03];
> > +	u16 id3;		/* 0x58 */
> > +	u16 resv9[0x03];
> > +	u16 sw_reset;		/* 0x60 */
> > +};
> > +#endif
> 
> Is this structure really used ? I have not seen in code. Or does it come
> only from mx51evk ? You can remove the whole file, if it is not needed.

No idea what it does. It's copied from MX51EVK. I will try removing it.

Best regards,

-- 
David Jander
Protonic Holland.