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

[Stefano Babic]

On 01/19/2011 08:35 AM, Wolfgang Denk wrote:
> Dear Stefano Babic,
>> +	if (!reg) {
>> +		reg = __REG(ROMPATCH_REV);
> 
> __REG()?

It should not be..

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

I supposed to have already replaced all of them, but I have missed some
of them.

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

I think your question mark is related to the fact that it is not so
clear why I set the mac address to all zero. At first glance, it makes
no sense.

However, the reason is that the i.MX35 does not have a MAC address
stored in its internal fuses as other i.MX processors
(i.MX27/i.MX25/i.MX51), but I want to expose the same interface for all
i.MX processors. In this way, I can avoid nasty #ifdef in code.
As I can see in u-boot code, the exception now in code is the i.MX31,
that was the first i.MX processor added to mainline. It has still a lot
of own functions (I mean, something like mx31_*, such as mx31_gpio.
mx31_iomux, and so on).

> What do these "!<" markers mean?

They have no useful meaning and I must drop them.

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

iomux_config_mux is already void for mx5 processors. Of course, it must
return void for MX35, too. And mxc_request_mux as well.
I will check all these functions to remove inconsistencies.

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

Thanks, I will fix it.

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

Of course - I will fix it.

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

No, this not. I have already set a structure for the Clock Module, that
it is used in most part of code (ccm_regs). However, these offsets are
used in the assembly file (lowlevel_init.S), and I cannot use the
structure. It seems to me the only structure needed, and it seems to me
not necessary to add an asm-offsets.h only for it. For this reason. I
let it in imx-regs.h


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

I have already removed the usage of this macro, I forget to drop its
define. Thanks, I will remove it.

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

I will check this issue globally.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================