Hi, Stefano, 2010/12/7 Stefano Babic <sba...@denx.de>: > On 12/06/2010 11:57 AM, Jason Liu wrote: > > Hi Jason, > >> Add initial support for mx53 with the following change, >> >> - Add the iomux support and the pin definition, >> - Add the regs definition, clean up some unused def from mx51, >> - Add the low level init support, make use the freq input of setup_pll macro >> >> This patch has been tested on MX51 Babbage 3.0 > > I admit I am confused. If you are adding support for a new SoC, I am > expecting you test on an evaluation board with that SoC on board, not on > a MX51. Of course, the patch should be tested on the mx51evk as well for > regression tests.
Yes, I'm adding this to show that this patch has been tested on mx51evk and not affect mx51evk function. > > The patch adds dead code until the first board with the MX.53 goes to > mainline. As I see for all new SoCs introduced in u-boot, it is really > desired to have a patchset including the first board supporting that > SoC. In this way, we are able to better understand all changes required > by your patches. Yes, agree. The patch for mx53 board support will coming soon. > > If on the other side you want to fix something broken in the actual > iomux code for i.MX51, this should be done in different patch, adding > the part for the i.MX53 later. > >> #define MUX_PIN_NUM_MAX (((MUX_I_END - MUX_I_START) >> 2) + 1) >> @@ -44,20 +44,22 @@ static inline u32 get_mux_reg(iomux_pin_name_t pin) >> { >> u32 mux_reg = PIN_TO_IOMUX_MUX(pin); >> >> - if (is_soc_rev(CHIP_REV_2_0) < 0) { >> - /* >> - * Fixup register address: >> - * i.MX51 TO1 has offset with the register >> - * which is define as TO2. >> - */ >> - if ((pin == MX51_PIN_NANDF_RB5) || >> - (pin == MX51_PIN_NANDF_RB6) || >> - (pin == MX51_PIN_NANDF_RB7)) >> - ; /* Do nothing */ >> - else if (mux_reg >= 0x2FC) >> - mux_reg += 8; >> - else if (mux_reg >= 0x130) >> - mux_reg += 0xC; > > I know all this crap is for MX51 in the TO1 version, even if I do not > know if there are boards with this first version of the processor. > Probably we must maintain this stuff for compatibility. Really I would > like to remove it completely ;-). Do you mind that I remove it completely here? > >> + if (is_soc_type(MX_CPU_MX51)) { > > It seems to me you are mixing the check of the microprocessor type at > compile time (#ifdef CONFIG_MX51) and at runtime with this new function. > I think we should be consistent. If #define CONFIG_MX51 is defined, > there should be no way for this function to return false, and then makes > no sense to define a runtime function if we have already stated on which > processor u-boot is running. The motivation to use " is_soc_type(CPU_TYPE) "is to remove the some #ifdef CONFIG_MX51in this file, I can change back to the following, + #if defined (MX_CPU_MX51) + if (is_soc_rev(CHIP_REV_2_0) < 0) { + code snip + } + #endif Is this ok? > >> +/* >> + * This function configures input path. >> + * >> + * @param input index of input select register >> + * @param config the binary value of elements >> + */ >> +void mxc_iomux_set_input(iomux_input_select_t input, u32 config) > > Probably it should be better to add a comment that this function > supports pins in daisy-chain, as this feature is described in the > reference manual. Yes, agree. > >> diff --git a/arch/arm/cpu/armv7/mx5/soc.c b/arch/arm/cpu/armv7/mx5/soc.c > >> #if defined(CONFIG_MX51) >> -#define CPU_TYPE 0x51000 >> +#define CPU_TYPE MX_CPU_MX51 >> +#elif defined(CONFIG_MX53) >> +#define CPU_TYPE MX_CPU_MX53 >> #else >> #error "CPU_TYPE not defined" >> #endif > > See my previous comment. You have already defined CONFIG_MX51 and > CONFIG_MX53, and probably we do not need additionally defines for the > same goal. Yes, if we remove the runtime check, this def can be removed. > >> diff --git a/arch/arm/include/asm/arch-mx5/imx-regs.h >> b/arch/arm/include/asm/arch-mx5/imx-regs.h >> old mode 100644 >> new mode 100755 >> diff --git a/arch/arm/include/asm/arch-mx5/iomux.h >> b/arch/arm/include/asm/arch-mx5/iomux.h >> index 0d91a24..760371b 100644 >> --- a/arch/arm/include/asm/arch-mx5/iomux.h >> +++ b/arch/arm/include/asm/arch-mx5/iomux.h >> @@ -70,108 +70,6 @@ typedef enum iomux_pad_config { >> PAD_CTL_DRV_VOT_HIGH = 0x1 << 13,/* High voltage mode */ >> } iomux_pad_config_t; >> >> -/* various IOMUX input select register index */ >> -typedef enum iomux_input_select { > > Agree. iomux_input_select is used only in iomux.c, so better to move it > in the implementation file. OK, > > >> diff --git a/arch/arm/include/asm/arch-mx5/mx5x_pins.h >> b/arch/arm/include/asm/arch-mx5/mx5x_pins.h >> old mode 100644 >> new mode 100755 >> index a564fce..a5cd773 >> --- a/arch/arm/include/asm/arch-mx5/mx5x_pins.h >> +++ b/arch/arm/include/asm/arch-mx5/mx5x_pins.h > >> + MX53_AUDMUX_P5_INPUT_DA_AMX_SELECT_I, >> + MX53_AUDMUX_P5_INPUT_DB_AMX_SELECT_I, >> + MX53_AUDMUX_P5_INPUT_RXCLK_AMX_SELECT_INPUT, >> + MX53_AUDMUX_P5_INPUT_RXFS_AMX_SELECT_INPUT, >> + MX53_AUDMUX_P5_INPUT_TXCLK_AMX_SELECT_INPUT, >> + MX53_AUDMUX_P5_INPUT_TXFS_AMX_SELECT_INPUT, >> + MX53_CAN1_IPP_IND_CANRX_SELECT_INPUT, /*0x760*/ > > What is the meaning of this comment ? It should have nothing to do with > this enumeration type, The same globally in this structure. OK, I can remove it. Thanks for the review. BR, Jason > > 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 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot