[U-Boot] [PATCH 1/3] OMAP3: Consolidate SDRC related operations

[Tom]

hvaibhav at ti.com wrote:
> From: Vaibhav Hiremath <hvaibhav at ti.com>
> 
> Consolidated all SDRC related functions/operations into separate
> file - sdrc.c.

Please add a long comment to explain why this is necessary.
Something like..
'Generalizing omap memory setup is necessary to support the new emif4 interface
that for am3517 uses.. '

> 
> Signed-off-by: Vaibhav Hiremath <hvaibhav at ti.com>
> Signed-off-by: Sanjeev Premi <premi at ti.com>

There is a regression.
Please check devkit8000
cpu/arm_cortexa8/omap3/libomap3.a(board.o): In function `checkboard':
.../cpu/arm_cortexa8/omap3/board.c:313: undefined reference to `is_mem_sdr'
cpu/arm_cortexa8/omap3/libomap3.a(board.o): In function `s_init':
.../cpu/arm_cortexa8/omap3/board.c:228: undefined reference to `mem_init'
cpu/arm_cortexa8/omap3/libomap3.a(mem.o): In function `mem_ok':
.../cpu/arm_cortexa8/omap3/mem.c:92: undefined reference to `get_sdr_cs_offset'
lib_arm/libarm.a(board.o):(.data+0x28): undefined reference to `dram_init'

The biggest problem with this patch is that though the comment says it is
a code movement patch, it has other changes it in.  These changes must
be separated into its own patch(s).

Because of these problems, this is only a partial review.

> ---
>  cpu/arm_cortexa8/omap3/Makefile        |    3 +
>  cpu/arm_cortexa8/omap3/board.c         |   34 +------

<snip>

>  			u32 size)
>  {
> diff --git a/cpu/arm_cortexa8/omap3/sdrc.c b/cpu/arm_cortexa8/omap3/sdrc.c
> new file mode 100644
> index 0000000..9a46155
> --- /dev/null
> +++ b/cpu/arm_cortexa8/omap3/sdrc.c
> @@ -0,0 +1,186 @@
> +/*
> + * Functions related to SDRC.
> + *
> + * This file has been created after exctracting and consolidating
> + * the SDRC related content from mem.c and board.c.
> + *
> + * Copyright (C) 2009 Texas Instruments Incorporated - http://www.ti.com/
> + *

Because this is code movement, include the original copyrights

> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +

<snip>

> +
> +	if (!mem_ok(cs))
> +		writel(0, &sdrc_base->cs[cs].mcfg);
> +}
> +
> +/**

Follow the multi-line comment.
Remove the extra '*'
This happens other places in this patch.
Fix globally

> + * dram_init - Sets uboots idea of sdram size
> + */
> +int dram_init(void)
> +{
> +	DECLARE_GLOBAL_DATA_PTR;
> +	unsigned int size0 = 0, size1 = 0;
> +
> +	size0 = get_sdr_cs_size(CS0);
> +	/*
> +	 * If a second bank of DDR is attached to CS1 this is
> +	 * where it can be started.  Early init code will init
> +	 * memory on CS0.
> +	 */
> +	if ((sysinfo.mtype == DDR_COMBO) || (sysinfo.mtype == DDR_STACKED)) {
> +		do_sdrc_init(CS1, NOT_EARLY);
> +		make_cs1_contiguous();
> +
> +		size1 = get_sdr_cs_size(CS1);

This is different that a code movement change.
The comment of the change does not match what you have done.
Split the real changes into a separate patch.

Tom