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

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Tom [mailto:Tom.Rix at windriver.com]
> Sent: Sunday, February 07, 2010 9:44 PM
> To: Hiremath, Vaibhav
> Cc: u-boot at lists.denx.de; Paulraj, Sandeep; Premi, Sanjeev
> Subject: Re: [PATCH 1/3] OMAP3: Consolidate SDRC related operations
> 
> 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.. '
[Hiremath, Vaibhav] Ok, will update in next version.

> 
> >
> > 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.
[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