[U-Boot] [PATCH 2/9] MX51: Add initial support for the Freescale MX51

[Wolfgang Denk]

Dear Stefano Babic,

In message <1263212760-27272-3-git-send-email-sbabic at denx.de> you wrote:
> The patch add initial support for the Freescale i.MX51 processor
> (family arm cortex_a8).
...
> --- /dev/null
> +++ b/cpu/arm_cortexa8/mx51/clock.c
...
> +/*
> + * Calculate the frequence of this pll.
> + */

Typo: frequency

> +u32 get_mcu_main_clk(void)
> +{
> +	u32 reg, freq;
> +
> +	reg = (__raw_readl(&mxc_ccm->cacrr) & MXC_CCM_CACRR_ARM_PODF_MASK) >>
> +	    MXC_CCM_CACRR_ARM_PODF_OFFSET;

Indentation not by TAB.

...
> +/***************************************************/
> +
> +U_BOOT_CMD(
> +	mx51clocks,	CONFIG_SYS_MAXARGS,	1,	do_mx51_showclocks,
> +	"display mx51 clocks\n",
> +	""
> +);

It makes little sense to implement arch specific commands that will
most probably repeated - I don;t want to see a "mx51clocks" command
here, and "mx25clocks", "mx27clocks", "mx31clocks", "mx35clocks", ...
commands on other systems. Just name this "clockinfo" (which is more
in line with the existing commands as "bdinfo", "reginfo", "flinfo"
etc.) or, if you insist, "showclocks" or just "clocks".

...
> +/*!
> + * This function is used to configure a pin through the IOMUX module.
> + * @param  pin		a pin number as defined in \b #iomux_pin_name_t
> + * @param  cfg		an output function as defined in \b #iomux_pin_cfg_t
> + *
> + * @return 		0 if successful; Non-zero otherwise

Lines too long, please check globally.

And please get rid of the '\b' stuff in the comments (and eventually
of the "/*!" things etc., too.).

> +void mxc_iomux_set_input(iomux_input_select_t input, u32 config)
> +{
> +	u32 reg;
> +
> +	if (is_soc_rev(CHIP_REV_2_0) < 0) {
> +		if (input == MUX_IN_IPU_IPP_DI_0_IND_DISPB_SD_D_SELECT_INPUT)
> +			input -= 4;
> +		else if (input ==
> +			 MUX_IN_IPU_IPP_DI_1_IND_DISPB_SD_D_SELECT_INPUT)
> +			input -= 3;
> +		else if (input >= MUX_IN_KPP_IPP_IND_COL_6_SELECT_INPUT)
> +			input -= 2;
> +		else if (input >=
> +			 MUX_IN_HSC_MIPI_MIX_PAR_SISG_TRIG_SELECT_INPUT)
> +			input -= 5;
> +		else if (input >=
> +			 MUX_IN_HSC_MIPI_MIX_IPP_IND_SENS1_DATA_EN_SELECT_INPUT)
> +			input -= 3;
> +		else if (input >= MUX_IN_ECSPI2_IPP_IND_SS_B_3_SELECT_INPUT)
> +			input -= 2;
> +		else if (input >= MUX_IN_CCM_PLL1_BYPASS_CLK_SELECT_INPUT)
> +			input -= 1;
> +		reg = IOMUXSW_INPUT_CTL + (input << 2) + INPUT_CTL_START_TO1;
> +	} else
> +		reg = IOMUXSW_INPUT_CTL + (input << 2) + INPUT_CTL_START;

This is pretty much unreadable, and even more difficult to
understand. Add comment to explain the logic, and fix indentation in
wrapped lines (use TABs only).

> diff --git a/cpu/arm_cortexa8/mx51/lowlevel_init.S b/cpu/arm_cortexa8/mx51/lowlevel_init.S
> new file mode 100644
> index 0000000..843a7b8
> --- /dev/null
> +++ b/cpu/arm_cortexa8/mx51/lowlevel_init.S
> @@ -0,0 +1,320 @@

> +/*
> + * return soc version
> + * 	0x10:  TO1
> + *	0x20:  TO2
> + *	0x30:  TO3
> + */
> +.macro check_soc_version ret, tmp
> +.endm

??? Please don;t add dead code.

> +/*
> + * L2CC Cache setup/invalidation/disable
> + */
> +.macro init_l2cc
> +	/* explicitly disable L2 cache */
> +        mrc 15, 0, r0, c1, c0, 1
> +        bic r0, r0, #0x2
> +        mcr 15, 0, r0, c1, c0, 1
> +
> +        /* reconfigure L2 cache aux control reg */
> +        mov r0, #0xC0                   /* tag RAM */
> +        add r0, r0, #0x4                /* data RAM */
> +        orr r0, r0, #(1 << 24)          /* disable write allocate delay */
> +        orr r0, r0, #(1 << 23)          /* disable write allocate combine */
> +        orr r0, r0, #(1 << 22)          /* disable write allocate */

Indentation by TAB only, please. Please check and fix coding style
globally, in allpatches.

> +/* MAX (Multi-Layer AHB Crossbar Switch) setup */
> +.macro init_max
> +.endm /* init_max */

No dead code, please.

> +/* M4IF setup */
> +.macro init_m4if
> +	/* VPU and IPU given higher priority (0x4)
> +	 * IPU accesses with ID=0x1 given highest priority (=0xA)
> +	 */
> +	ldr r0, =M4IF_BASE_ADDR
> +
> +	ldr r1, =0x00000203
> +	str r1, [r0, #0x40]
> +
> +	ldr r1, =0x0
> +	str r1, [r0, #0x44]
> +
> +	ldr r1, =0x00120125
> +	str r1, [r0, #0x9C]
> +
> +	ldr r1, =0x001901A3
> +	str r1, [r0, #0x48]
> +
> +/*
> +	ldr r1, =0x00000a01
> +	str r1, [r0, #0x48]
> +	ldr r1, =0x00000404
> +	str r1, [r0, #0x40]
> +*/

No dead code please. Please fix globally.


> +#if defined(CONFIG_DISPLAY_CPUINFO)
> +int print_cpuinfo(void)
> +{
> +	printf("CPU:   Freescale i.MX51 family %d.%dV at %d MHz\n",
> +	       (get_board_rev() & 0xFF) >> 4,
> +	       (get_board_rev() & 0xF),
> +		get_mcu_main_clk() / 1000000);

This is wrong. You ar enot printing a _BOARD_ revision here, but a
processor revision. Please fix.

> +ulong get_timer_masked(void)
> +{
> +	ulong val = __raw_readl(&cur_gpt->counter);
> +	val /= (CONFIG_MX51_CLK32 / CONFIG_SYS_HZ);
> +	if (val >= lastinc)
> +		timestamp += (val - lastinc);
> +	else
> +		timestamp += ((0xFFFFFFFF / (CONFIG_MX51_CLK32 / CONFIG_SYS_HZ))
> +				- lastinc) + val;

Please use braces for multi-line statements.

> +	lastinc = val;
> +	return val;
> +}
> +
> +ulong get_timer(ulong base)
> +{
> +	return get_timer_masked() - base;
> +}
> +
> +void set_timer(ulong t)
> +{
> +	timestamp = t;
> +}
> +
> +/* delay x useconds AND perserve advance timstamp value */
> +void __udelay(unsigned long usec)
> +{
> +	unsigned long now, last = readl(&cur_gpt->counter);
> +	long tmo = usec * (CONFIG_MX51_CLK32 / 1000) / 1000;
> +
> +	if (!tmo)
> +		tmo = 1;
> +
> +	while (tmo > 0) {
> +		now = readl(&cur_gpt->counter);
> +		if (last > now)
> +			tmo -= 0xFFFFFFFF - last + now;
> +		else
> +			tmo -= now - last;

Is this correct wrap-around handling?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The use of anthropomorphic terminology when  dealing  with  computing
systems is a symptom of professional immaturity.   -- Edsger Dijkstra