[U-Boot] [PATCH-V2 2/4] omap3: Consolidate SDRC related operations

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de]
> Sent: Thursday, May 06, 2010 1:38 AM
> To: Hiremath, Vaibhav
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH-V2 2/4] omap3: Consolidate SDRC related
> operations
> 
> Dear hvaibhav at ti.com,
> 
[Hiremath, Vaibhav] Thanks Denk for your comments, see my response below -

> In message <1272034546-26041-4-git-send-email-hvaibhav at ti.com> you wrote:
> > From: Vaibhav Hiremath <hvaibhav at ti.com>
> >
> > Consolidated SDRC related functions into one file - sdrc.c
> >
> > And also replaced sdrc_init with generic memory init
> > function (mem_init), this generalization of omap memory setup
> > is necessary to support the new emif4 interface introduced in AM3517.
> >
> > Signed-off-by: Vaibhav Hiremath <hvaibhav at ti.com>
> > Signed-off-by: Sanjeev Premi <premi at ti.com>
> 
> > diff --git a/arch/arm/cpu/arm_cortexa8/omap3/Makefile
> b/arch/arm/cpu/arm_cortexa8/omap3/Makefile
> > index 136b163..8cc7802 100644
> > --- a/arch/arm/cpu/arm_cortexa8/omap3/Makefile
> > +++ b/arch/arm/cpu/arm_cortexa8/omap3/Makefile
> > @@ -33,6 +33,9 @@ COBJS	+= board.o
> >  COBJS	+= clock.o
> >  COBJS	+= gpio.o
> >  COBJS	+= mem.o
> > +ifdef	CONFIG_SDRC
> > +COBJS	+= sdrc.o
> > +endif
> 
> Please don't use 'ifdef" here; instead, use `COBJS-$(CONFIG_SDRC)'
[Hiremath, Vaibhav] ok, will incorporate in next version.


> 
> > diff --git a/arch/arm/include/asm/arch-omap3/cpu.h
> b/arch/arm/include/asm/arch-omap3/cpu.h
> > index aa8de32..a49af10 100644
> > --- a/arch/arm/include/asm/arch-omap3/cpu.h
> > +++ b/arch/arm/include/asm/arch-omap3/cpu.h
> > @@ -183,6 +183,7 @@ struct sms {
> >  /* SDRC */
> >  #ifndef __KERNEL_STRICT_NAMES
> >  #ifndef __ASSEMBLY__
> > +#if defined(CONFIG_SDRC)
> >  struct sdrc_cs {
> >  	u32 mcfg;		/* 0x80 || 0xB0 */
> >  	u32 mr;			/* 0x84 || 0xB4 */
> > @@ -215,6 +216,8 @@ struct sdrc {
> >  	u8 res4[0xC];
> >  	struct sdrc_cs cs[2];	/* 0x80 || 0xB0 */
> >  };
> > +
> > +#endif	/* CONFIG_SDRC */
> 
> I don't like such a #ifdef here - it is absolutely necessary? Why?
> 
[Hiremath, Vaibhav] Denk,

This is common file being used for all OMAP series of devices (OMAP2, OMAP3 and AM35x family) and OMAP2/3 family supports SDRC controller and AM35x family support EMIF4.

And due to this difference we need to add this #ifdef.

> > diff --git a/arch/arm/include/asm/arch-omap3/sys_proto.h
> b/arch/arm/include/asm/arch-omap3/sys_proto.h
> > index 34bd515..34e4e0d 100644
> > --- a/arch/arm/include/asm/arch-omap3/sys_proto.h
> > +++ b/arch/arm/include/asm/arch-omap3/sys_proto.h
> > @@ -31,8 +31,10 @@ void prcm_init(void);
> >  void per_clocks_enable(void);
> >
> >  void memif_init(void);
> > +#if defined(CONFIG_SDRC)
> >  void sdrc_init(void);
> >  void do_sdrc_init(u32, u32);
> > +#endif
> 
> Ditto - please drop this #ifdef.
> 
[Hiremath, Vaibhav] Same as above.

Thanks,
Vaibhav

> 
> 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
> People seldom know what they want until you give them what  they  ask
> for.