hvaib...@ti.com wrote:
> From: Vaibhav Hiremath <hvaib...@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 <hvaib...@ti.com>
> Signed-off-by: Sanjeev Premi <pr...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
