[U-Boot] [PATCH] MX5: Add initial support for mx53

Jason Liu liu.h.jason at gmail.com
Tue Dec 7 03:58:19 CET 2010


Hi, Stefano,

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


More information about the U-Boot mailing list