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.
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.
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 ;-).
> + 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.
> +/*> + * 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.
> @@ -269,9 +284,12 @@ lowlevel_init:> mov pc,lr> > /* Board level setting value */> -DDR_PERCHARGE_CMD: .word 0x04008008> -DDR_REFRESH_CMD: .word 0x00008010> -DDR_LMR1_W: .word 0x00338018> -DDR_LMR_CMD: .word 0xB2220000> -DDR_TIMING_W: .word 0xB02567A9> -DDR_MISC_W: .word 0x000A0104
Good catch, this is dead code.
> 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.
> 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.
> 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.
Best regards,
Stefano Babic

Hi, Stefano,
2010/12/7 Stefano Babic <sbabic@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: office@denx.de> =====================================================================> _______________________________________________> U-Boot mailing list> U-Boot@lists.denx.de> http://lists.denx.de/mailman/listinfo/u-boot>

On 12/07/2010 03:58 AM, Jason Liu wrote:
>> 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?
Really I do not know. This is clearly a workaround for the first MX.51
version (TO1), and it makes no sense for actual processor's revision. I
do not know if there are mx51evk with this revision on board, but if
there are we should maintain this code for compatibility reason.
> 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?
I think so. If we were able to do everything on runtime, then we could
proceed removing all #ifdef stuff. However, we need the #ifdef in the
header files to select the different internal structures between MX.51
and MX.53.
Best regards,
Stefano Babic

Hi, Stefano,
2010/12/7 Stefano Babic <sbabic@denx.de>:
> On 12/07/2010 03:58 AM, Jason Liu wrote:>>>> 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?>> Really I do not know. This is clearly a workaround for the first MX.51> version (TO1), and it makes no sense for actual processor's revision. I> do not know if there are mx51evk with this revision on board, but if> there are we should maintain this code for compatibility reason.
OK, Let's keep it.
>>> 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?>> I think so. If we were able to do everything on runtime, then we could> proceed removing all #ifdef stuff. However, we need the #ifdef in the> header files to select the different internal structures between MX.51> and MX.53.
OK, I will do the change.
Thanks for the review.
>> 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@denx.de> =====================================================================>