[U-Boot] [PATCH] ppc4xx: Remove duplicated code for Sequoia NAND booting version

[U-Boot] [PATCH] ppc4xx: Remove duplicated code for Sequoia NAND booting version

[Stefan Roese]

Signed-off-by: Stefan Roese <sr@denx.de>
---
 board/amcc/sequoia/sdram.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/board/amcc/sequoia/sdram.c b/board/amcc/sequoia/sdram.c
index c26e6ee..6df4c6d 100644
--- a/board/amcc/sequoia/sdram.c
+++ b/board/amcc/sequoia/sdram.c
@@ -44,7 +44,7 @@ extern void denali_core_search_data_eye(void);
  * for the 4k NAND boot image so define bus_frequency to 133MHz here
  * which is save for the refresh counter setup.
  */
-#define get_bus_freq(val)	133000000
+#define get_bus_freq(val)	133333333
 #endif
 
 /*************************************************************************
@@ -55,11 +55,7 @@ extern void denali_core_search_data_eye(void);
 phys_size_t initdram (int board_type)
 {
 #if !defined(CONFIG_NAND_U_BOOT) || defined(CONFIG_NAND_SPL)
-#if !defined(CONFIG_NAND_SPL)
 	ulong speed = get_bus_freq(0);
-#else
-	ulong speed = 133333333;	/* 133MHz is on the safe side	*/
-#endif
 
 	mtsdram(DDR0_02, 0x00000000);
 
-- 
1.6.2.3

[U-Boot] [PATCH] ppc4xx: Remove duplicated code for Sequoia NAND booting version

[Wolfgang Denk]

Dear Stefan Roese,

In message <1239788018-27468-1-git-send-email-sr@denx.de> you wrote:
> Signed-off-by: Stefan Roese <sr@denx.de>
> ---
>  board/amcc/sequoia/sdram.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
> 
> diff --git a/board/amcc/sequoia/sdram.c b/board/amcc/sequoia/sdram.c
> index c26e6ee..6df4c6d 100644
> --- a/board/amcc/sequoia/sdram.c
> +++ b/board/amcc/sequoia/sdram.c
> @@ -44,7 +44,7 @@ extern void denali_core_search_data_eye(void);
>   * for the 4k NAND boot image so define bus_frequency to 133MHz here
>   * which is save for the refresh counter setup.
>   */
> -#define get_bus_freq(val)	133000000
> +#define get_bus_freq(val)	133333333
>  #endif

To me that does not look exactly like duplicated code removal...

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
It's a small world, but I wouldn't want to paint it.

[U-Boot] [PATCH] ppc4xx: Remove duplicated code for Sequoia NAND booting version

[Stefan Roese]

Hi Wolfgang,

On Wednesday 15 April 2009, Wolfgang Denk wrote:
> > +++ b/board/amcc/sequoia/sdram.c
> > @@ -44,7 +44,7 @@ extern void denali_core_search_data_eye(void);
> >   * for the 4k NAND boot image so define bus_frequency to 133MHz here
> >   * which is save for the refresh counter setup.
> >   */
> > -#define get_bus_freq(val)	133000000
> > +#define get_bus_freq(val)	133333333
> >  #endif
>
> To me that does not look exactly like duplicated code removal...

Here a closer look at the patch:

-#define get_bus_freq(val)	133000000
+#define get_bus_freq(val)	133333333
 #endif
 
 /*************************************************************************
@@ -55,11 +55,7 @@ extern void denali_core_search_data_eye(void);
 phys_size_t initdram (int board_type)
 {
 #if !defined(CONFIG_NAND_U_BOOT) || defined(CONFIG_NAND_SPL)
-#if !defined(CONFIG_NAND_SPL)
 	ulong speed = get_bus_freq(0);
-#else
-	ulong speed = 133333333;	/* 133MHz is on the safe side	*/
-#endif


As you can see, the top patch part changes the define of get_bus_frequency() 
to 133333333. This define was only enabled (via ifdef) for the 
CONFIG_NAND_SPL part:

#if defined(CONFIG_NAND_SPL)
/* Using cpu/ppc4xx/speed.c to calculate the bus frequency is too big
 * for the 4k NAND boot image so define bus_frequency to 133MHz here
 * which is save for the refresh counter setup.
 */
#define get_bus_freq(val)       133333333
#endif

And now the 2nd patch part makes sure that this define is really used.

So what the patch does is to remove an unused code part. Is this what you are 
complaining about? If really needed I could resend this patch with a modified 
patch subject.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] ppc4xx: Remove duplicated code for Sequoia NAND booting version

[Wolfgang Denk]

Dear Stefan Roese,

In message <200904160651.28962.sr@denx.de> you wrote:
>
> > > -#define get_bus_freq(val)	133000000
> > > +#define get_bus_freq(val)	133333333
> > >  #endif
> >
> > To me that does not look exactly like duplicated code removal...
...
> So what the patch does is to remove an unused code part. Is this what you are 
> complaining about? If really needed I could resend this patch with a modified 
> patch subject.

No, I complain that you also change a vital costant without even
mentioning.

Normally, this should be split into two separate patches., but at
least it should be stated in the commit message.

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
What can it profit a man to gain the whole world and to come  to  his
property with a gastric ulcer, a blown prostate, and bifocals?
                                     -- John Steinbeck, _Cannery Row_

[U-Boot] [PATCH] ppc4xx: Remove duplicated code for Sequoia NAND booting version

[Stefan Roese]

On Thursday 16 April 2009, Wolfgang Denk wrote:
> > So what the patch does is to remove an unused code part. Is this what you
> > are complaining about? If really needed I could resend this patch with a
> > modified patch subject.
>
> No, I complain that you also change a vital costant without even
> mentioning.

The constant was not used at all (as I mentioned in my last mail). So changing 
it to the really used value seems logical to me.

> Normally, this should be split into two separate patches., but at
> least it should be stated in the commit message.

I'll resend the patch with a changed commit text later.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================