[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