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: off...@denx.de ===================================================================== _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot