> -----Original Message-----
> From: Tom [mailto:tom....@windriver.com]
> Sent: Sunday, February 07, 2010 9:44 PM
> To: Hiremath, Vaibhav
> Cc: u-boot@lists.denx.de; Paulraj, Sandeep; Premi, Sanjeev
> Subject: Re: [PATCH 1/3] OMAP3: Consolidate SDRC related operations
>
> 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.. '
[Hiremath, Vaibhav] Ok, will update in next version.
>
> >
> > 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.
[Hiremath, Vaibhav] I will separate such changes into separate commit and
submit it to list.
>
> > ---
> > 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
[Hiremath, Vaibhav] Ok, will update in next version.
>
> > + * 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
[Hiremath, Vaibhav] Ok.
Thanks Tom for the review, I will update the patch series and submit it again.
Thanks,
Vaibhav
>
> > + * 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
