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

[Stefano Babic]

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

-- 
=====================================================================
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
=====================================================================