[U-Boot] [PATCH 1/7] Add support for MX35 processor

[Wolfgang Denk <wd@denx.de>]

Dear Stefano Babic,

In message <1295012124-15551-1-git-send-email-sbabic@denx.de> you wrote:
> The patch adds basic support for the Freescale's i.MX35
> (arm1136 based) processor.
> 
> Signed-off-by: Stefano Babic <sbabic@denx.de>
...
> +u32 get_cpu_rev(void)
> +{
> +	int reg;
> +	struct iim_regs *iim =
> +		(struct iim_regs *)IIM_BASE_ADDR;
> +	reg = readl(&iim->iim_srev);
> +	if (!reg) {
> +		reg = __REG(ROMPATCH_REV);

__REG()?

NAK.  Please use I/O accessors.  Please fix globally.

> +void imx_get_mac_from_fuse(unsigned char *mac)
> +{
> +	int i;
> +
> +	for (i = 0; i < 6; i++)
> +		mac[i] = 0;

	memset(mac, 0, 6);

?

> +/*
> + * IOMUX register (base) addresses
> + */
> +enum iomux_reg_addr {
> +	IOMUXGPR = IOMUXC_BASE_ADDR,
> +	/*!< General purpose */
> +	IOMUXSW_MUX_CTL = IOMUXC_BASE_ADDR + 4,
> +	/*!< MUX control */
> +	IOMUXSW_MUX_END = IOMUXC_BASE_ADDR + 0x324,
> +	/*!< last MUX control register */
> +	IOMUXSW_PAD_CTL = IOMUXC_BASE_ADDR + 0x328,
> +	/*!< Pad control */
> +	IOMUXSW_PAD_END = IOMUXC_BASE_ADDR + 0x794,
> +	/*!< last Pad control register */
> +	IOMUXSW_INPUT_CTL = IOMUXC_BASE_ADDR + 0x7AC,
> +	/*!< input select register */
> +	IOMUXSW_INPUT_END = IOMUXC_BASE_ADDR + 0x9F4,
> +	/*!< last input select register */
> +};

What do these "!<" markers mean?

> +/*
> + * This function is used to configure a pin through the IOMUX module.
> + * FIXED ME: for backward compatible. Will be static function!
> + * @param  pin		a pin number as defined in \b #iomux_pin_name_t
> + * @param  cfg		an output function as defined in \b #iomux_pin_cfg_t
> + *
> + * @return		0 if successful; Non-zero otherwise

When does the "otherwise" happen?

> + */
> +static int iomux_config_mux(iomux_pin_name_t pin, iomux_pin_cfg_t cfg)
> +{
> +	u32 mux_reg = PIN_TO_IOMUX_MUX(pin);
> +
> +	if (mux_reg != NON_MUX_I) {
> +		mux_reg += IOMUXGPR;
> +		__REG(mux_reg) = cfg;
> +	}
> +
> +	return 0;
> +}

Should we make this function return "void" ?

> +/* delay x useconds AND perserve advance timstamp value */
> +/* GPTCNT is now supposed to tick 1 by 1 us. */

s/perserve/preserve/ ?

Incorrect multiline comment style.

> diff --git a/arch/arm/include/asm/arch-mx35/imx-regs.h b/arch/arm/include/asm/arch-mx35/imx-regs.h
> new file mode 100644
> index 0000000..f382960
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-mx35/imx-regs.h
...
> +#define __REG(x)     (*((volatile u32 *)(x)))
> +#define __REG16(x)   (*((volatile u16 *)(x)))
> +#define __REG8(x)    (*((volatile u8 *)(x)))

NAK!! Please use I/O accessors.  Please fix globally.

> +/* CCM */
> +#define CLKCTL_CCMR		0x00
> +#define CLKCTL_PDR0		0x04
> +#define CLKCTL_PDR1		0x08
> +#define CLKCTL_PDR2		0x0C
> +#define CLKCTL_PDR3		0x10
> +#define CLKCTL_PDR4		0x14
> +#define CLKCTL_RCSR		0x18
> +#define CLKCTL_MPCTL		0x1C
> +#define CLKCTL_PPCTL		0x20
> +#define CLKCTL_ACMR		0x24
> +#define CLKCTL_COSR		0x28
> +#define CLKCTL_CGR0		0x2C
> +#define CLKCTL_CGR1		0x30
> +#define CLKCTL_CGR2		0x34
> +#define CLKCTL_CGR3		0x38

NAK!! Please use C struct.  Please fix globally.

> +#if 0
> +extern unsigned int mxc_get_clock(enum mxc_clock clk);
> +#endif

Please remove such dead code.  Please fix globally.

> +extern unsigned int get_board_rev(void);
> +extern int is_soc_rev(int rev);
> +extern int sdhc_init(void);
> +
> +#define fixup_before_linux	\
> +	{		\
> +		volatile unsigned long *l2cc_ctl = (unsigned long *)0x30000100;\

0x30000100 ? Don't we have a #define for it?

> +/*
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:

Please fix these license headers - all of these.  They say where you
can _get_ License Version 2 or later, but they don't say which version
actually applies.  Please make clear that License v2+ applies.



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
By the way, ALL software projects are done by iterative  prototyping.
Some companies call their prototypes "releases", that's all.