Re: [U-Boot-Users] [PATCH 3/3] ppc4xx: HCU4/5. Cleanups

Hi Niklaus,

On Monday 04 February 2008, Niklaus Giger wrote:

• Fix some coding style violations.
• Use in/out_u16/32 where appropriate.
• Use register names from ppc405.h.
• Fix trace useage for Lauterbach.
• Fix detection of vxWorks EDR.
• Remove obsolete generation HCU2.
• Renamed fixed_hcu4_sdram to init_ppc405_sdram.

Thanks. Look really good now. Just a few nitpicking comments below.

Signed-off-by: Niklaus Giger niklaus.giger@netstal.com

board/netstal/common/fixed_sdram.c | 2 +- board/netstal/common/hcu_flash.c | 14 ------ board/netstal/common/nm.h | 11 ++++- board/netstal/common/nm_bsp.c | 4 +- board/netstal/hcu4/hcu4.c | 78 ++++++++++++++++-------------------- board/netstal/hcu5/hcu5.c | 8 +-- board/netstal/hcu5/sdram.c | 32 +++++++++------ 7 files changed, 68 insertions(+), 81 deletions(-)

diff --git a/board/netstal/common/fixed_sdram.c b/board/netstal/common/fixed_sdram.c index 8082f60..1df790c 100644 --- a/board/netstal/common/fixed_sdram.c +++ b/board/netstal/common/fixed_sdram.c @@ -44,7 +44,7 @@ void show_sdram_registers(void) } #endif

-long int fixed_hcu4_sdram (unsigned int dram_size) +long int init_ppc405_sdram (unsigned int dram_size) { #ifdef DEBUG printf(__FUNCTION__); diff --git a/board/netstal/common/hcu_flash.c b/board/netstal/common/hcu_flash.c index be2cb37..d0322f2 100644 --- a/board/netstal/common/hcu_flash.c +++ b/board/netstal/common/hcu_flash.c @@ -21,18 +21,6 @@

• MA 02111-1307 USA

*/

-/*

• • Modified 4/5/2001
• • Wait for completion of each sector erase command issued
• • 4/5/2001
• • Chris Hallinan - DS4.COM, Inc. - clh@net1plus.com
• • Modified 6/6/2007
• • Added isync
• • Niklaus Giger, Netstal Maschinen, niklaus.giger@netstal.com
• */

#include <common.h> #include <ppc4xx.h> #include <asm/processor.h> @@ -387,7 +375,6 @@ int flash_erase (flash_info_t * info, int s_first, int s_last) /* wait at least 80us - let's wait 1 ms */ udelay (1000);

-#if 0 /* * We wait for the last triggered sector */ @@ -396,7 +383,6 @@ int flash_erase (flash_info_t * info, int s_first, int s_last) wait_for_DQ7 (info, l_sect);

DONE: -#endif /* reset to read mode */ addr = (FLASH_WORD_SIZE *) info->start[0]; addr[0] = (FLASH_WORD_SIZE) 0x00F000F0; /* reset bank */ diff --git a/board/netstal/common/nm.h b/board/netstal/common/nm.h index 2801e13..f6acd47 100644 --- a/board/netstal/common/nm.h +++ b/board/netstal/common/nm.h @@ -27,8 +27,7 @@ extern void set_params_for_sw_install(int install_requested, char *board_name ); extern void common_misc_init_r(void);

enum {

• /* HW_GENERATION_HCU1 is no longer supported */
• HW_GENERATION_HCU2 = 0x10,

• /* HW_GENERATION_HCU1/2 is no longer supported */ HW_GENERATION_HCU3 = 0x10, HW_GENERATION_HCU4 = 0x20, HW_GENERATION_HCU5 = 0x30,

@@ -36,3 +35,11 @@ enum { HW_GENERATION_MCU20 = 0x0a, HW_GENERATION_MCU25 = 0x09, };

+#ifdef CONFIG_405GPr

Why this check here? You should never need 405GPr. CONFIG_405GP should be enough since they are nearly compatible.

+#if defined(DEBUG) +void show_sdram_registers(void); +#endif +long int init_ppc405_sdram (unsigned int dram_size);

Again you are mixing here coding styles: func() and func ().

+#endif

diff --git a/board/netstal/common/nm_bsp.c b/board/netstal/common/nm_bsp.c index c4265bb..89c697c 100644 --- a/board/netstal/common/nm_bsp.c +++ b/board/netstal/common/nm_bsp.c @@ -29,8 +29,7 @@ DECLARE_GLOBAL_DATA_PTR;

typedef struct {u8 id; char *name;} generation_info;

-generation_info generations[7] = {

• {HW_GENERATION_HCU2, "HCU2"},

+generation_info generations[6] = {

I think you can remove the "6" here.

{HW_GENERATION_HCU3, "HCU3"}, {HW_GENERATION_HCU4, "HCU4"}, {HW_GENERATION_HCU5, "HCU5"}, @@ -134,3 +133,4 @@ void common_misc_init_r(void) saveenv(); } }

diff --git a/board/netstal/hcu4/hcu4.c b/board/netstal/hcu4/hcu4.c index 4fbe701..43057a0 100644 --- a/board/netstal/hcu4/hcu4.c +++ b/board/netstal/hcu4/hcu4.c @@ -1,5 +1,5 @@ /*

• *(C) Copyright 2005-2007 Netstal Maschinen AG

• *(C) Copyright 2005-2008 Netstal Maschinen AG
  • Niklaus Giger (Niklaus.Giger@netstal.com)
  • This source code is free software; you can redistribute it

@@ -28,17 +28,10 @@ DECLARE_GLOBAL_DATA_PTR;

#define HCU_MACH_VERSIONS_REGISTER (0x7C000000 + 0xF00000) -#define SYS_SLOT_ADDRESS (0x7C000000 + 0x400000) -#define HCU3_DIGITAL_IO_REGISTER (0x7C000000 + 0x500000) +#define HCU_SLOT_ADDRESS (0x7C000000 + 0x400000) +#define HCU_DIGITAL_IO_REGISTER (0x7C000000 + 0x500000) #define HCU_SW_INSTALL_REQUESTED 0x10

-#undef DEBUG

-#if defined(DEBUG) -void show_sdram_registers(void); -#endif -long int fixed_hcu4_sdram (unsigned int dram_size);

/*

• This function is run very early, out of flash, and before devices are
• initialized. It is called by lib_ppc/board.c:board_init_f by virtue

@@ -49,17 +42,12 @@ long int fixed_hcu4_sdram (unsigned int dram_size);

• anything, not even stack. So be careful.

*/

-#define CPC0_CR0 0xb1 /* Chip control register 0 */ -#define CPC0_CR1 0xb2 /* Chip control register 1 */ /* Attention: If you want 1 microsecs times from the external oscillator

• • use 0x00804051. But this causes problems with u-boot and linux!

• • 0x00004051 is okay for u-boot/linux, but different from old vxworks

values + * 0x00804051 causes problems with u-boot and linux! */ #define CPC0_CR0_VALUE 0x0030103c #define CPC0_CR1_VALUE 0x00004051 -#define CPC0_ECR 0xaa /* Edge condition register */ -#define EBC0_CFG 0x23 /* External Peripheral Control Register */ -#define CPC0_EIRR 0xb6 /* External Interrupt Register */

int board_early_init_f (void) { @@ -70,16 +58,16 @@ int board_early_init_f (void) * IRQ 17-24 RESERVED/UNUSED * IRQ 31 (EXT IRQ 6) (unused) */

• mtdcr (uicsr, 0xFFFFFFFF); /* clear all ints */
• mtdcr (uicer, 0x00000000); /* disable all ints */
• mtdcr (uiccr, 0x00000000); /* set all to be non-critical */
• mtdcr (uicpr, 0xFFFFE000); /* set int polarities */
• mtdcr (uictr, 0x00000000); /* set int trigger levels */
• mtdcr (uicsr, 0xFFFFFFFF); /* clear all ints */

• mtdcr(uicsr, 0xFFFFFFFF); /* clear all ints */
• mtdcr(uicer, 0x00000000); /* disable all ints */
• mtdcr(uiccr, 0x00000000); /* set all to be non-critical */
• mtdcr(uicpr, 0xFFFFE000); /* set int polarities */
• mtdcr(uictr, 0x00000000); /* set int trigger levels */
• mtdcr(uicsr, 0xFFFFFFFF); /* clear all ints */

• mtdcr(CPC0_CR1, CPC0_CR1_VALUE);
• mtdcr(CPC0_ECR, 0x60606000);
• mtdcr(CPC0_EIRR, 0x7c000000);

• mtdcr(cntrl1, CPC0_CR1_VALUE);

• mtdcr(ecr, 0x60606000);

• mtdcr(eirr, 0x7C000000);

  return 0;

} @@ -93,18 +81,19 @@ int board_pre_init (void)

int sys_install_requested(void) {

• u16 *ioValuePtr = (u16 *)HCU3_DIGITAL_IO_REGISTER;
• return (in_be16(ioValuePtr) & HCU_SW_INSTALL_REQUESTED) != 0;

• u16 ioValue = in_be16((u16 *)HCU_DIGITAL_IO_REGISTER);
• return (ioValue & HCU_SW_INSTALL_REQUESTED) != 0;

}

int checkboard (void) {

• u16 *boardVersReg = (u16 *)HCU_MACH_VERSIONS_REGISTER;
• u16 generation = in_be16(boardVersReg) & 0xf0;
• u16 index = in_be16(boardVersReg) & 0x0f;

• u16 boardVersReg = in_be16((u16 *)HCU_MACH_VERSIONS_REGISTER);
• u16 generation = boardVersReg & 0xf0;
• u16 index = boardVersReg & 0x0f;
• /* Cannot be done in board_early_init */
• mtdcr(cntrl0, CPC0_CR0_VALUE);

• /* Cannot be done, in board_early_init */
• mtdcr(CPC0_CR0, CPC0_CR0_VALUE); /* Force /RTS to active. The board it not wired quite
  • correctly to use cts/rtc flow control, so just force the
  • /RST active and forget about it.

@@ -145,8 +134,8 @@ void sdram_init(void) */ u32 hcu_get_slot(void) {

• u16 *slot = (u16 *)SYS_SLOT_ADDRESS;
• return in_be16(slot) & 0x7f;

• u16 slot = in_be16((u16 *)HCU_SLOT_ADDRESS);
• return slot & 0x7f;

}

/* @@ -154,12 +143,12 @@ u32 hcu_get_slot(void) */ u32 get_serial_number(void) {

• u32 *serial = (u32 *)CFG_FLASH_BASE;

• u32 serial = in_be32((u32 *)CFG_FLASH_BASE);

• if (in_be32(serial) == 0xffffffff)

• if (serial == 0xffffffff) return 0;

• return in_be32(serial);

• return serial;

}

@@ -177,12 +166,13 @@ int misc_init_r(void) long int initdram(int board_type) { long dram_size = 0;

• u16 *boardVersReg = (u16 *) HCU_MACH_VERSIONS_REGISTER;
• u16 generation = in_be16(boardVersReg) & 0xf0;
• if (generation == HW_GENERATION_HCU3)

• dram_size = 32*1024*1024;
  

• else dram_size = 64*1024*1024;
• fixed_hcu4_sdram(dram_size);

• u16 boardVersReg = in_be16((u16 *)HCU_MACH_VERSIONS_REGISTER);
• u16 generation = boardVersReg & 0xf0;
• u16 index = boardVersReg & 0x0f;
• if (generation == HW_GENERATION_HCU3 && index < 0xf)

• dram_size = 32 << 20; /* 32 MB - RAM */
  

• else dram_size = 64 << 20; /* 64 MB - RAM */

if (generation == HW_GENERATION_HCU3 && index < 0xf) dram_size = 32 << 20; /* 32 MB - RAM */ else dram_size = 64 << 20; /* 64 MB - RAM */

please.

• init_ppc405_sdram(dram_size);

#ifdef DEBUG show_sdram_registers(); diff --git a/board/netstal/hcu5/hcu5.c b/board/netstal/hcu5/hcu5.c index 2c7afe2..c494e93 100644 --- a/board/netstal/hcu5/hcu5.c +++ b/board/netstal/hcu5/hcu5.c @@ -309,15 +309,13 @@ int misc_init_r(void) */ if (mfspr(dbcr0) & 0x80000000) { /* External debugger alive

•  * enable trace facilty for Lauterback
  

•  * CCR0[DAPUIB]=0 	Enable broadcast of instruction data
  

•  *			to auxiliary processor interface
  

•  * enable trace facilty for Lauterbach
  

  • CCR0[DTB]=0 Enable broadcast of trace information
  • SDR0_PFC0[TRE] Trace signals are enabled instead of

  • 	GPIO49-63
    

  */

• mtspr(ccr0, mfspr(ccr0)  &~ 0x00108000);
  

• mtsdr(SDR0_PFC0, sdr0_pfc1 | 0x00000100);
  

•        mtspr(ccr0, mfspr(ccr0)  &~ (CCR0_DTB));
  

• mtsdr(SDR0_PFC0, sdr0_pfc1 | SDR0_PFC0_TRE_ENABLE);
  

  } return 0;

} diff --git a/board/netstal/hcu5/sdram.c b/board/netstal/hcu5/sdram.c index 5435de1..be9a4cb 100644 --- a/board/netstal/hcu5/sdram.c +++ b/board/netstal/hcu5/sdram.c @@ -165,19 +165,25 @@ static void program_ecc(unsigned long start_address, unsigned long num_bytes) u32 val; char str[] = "ECC generation -"; #if defined(CONFIG_PRAM)

• u32 *magic;
• /* Check whether vxWorks is using EDR logging, if yes zero */
• /* also PostMortem and user reserved memory */
• magic = (u32 *)in_be32((u32 *)(start_address + num_bytes -

• 		       (CONFIG_PRAM*1024) + sizeof(u32)));
  

• debug("\n%s: CONFIG_PRAM %d kB magic 0x%x 0x%p -> 0x%x\n", __FUNCTION__,

•       CONFIG_PRAM,
  

•       start_address + num_bytes - (CONFIG_PRAM*1024) + sizeof(u32),
  

•       magic, in_be32(magic));
  

• if (in_be32(magic) == 0xbeefbabe)

• num_bytes -= (CONFIG_PRAM*1024) - PM_RESERVED_MEM;
  

• u32 *magicPtr;
• u32 magic;
• if ((mfspr(dbcr0) & 0x80000000) == 0) {

• /* only if no external debugger is alive!
  

•  * Check whether vxWorks is using EDR logging, if yes zero
  

•  * also PostMortem and user reserved memory
  

•  */
  

• magicPtr =  (u32 *)(start_address + num_bytes -
  

One space too much after the "=".

• 		(CONFIG_PRAM*1024) + sizeof(u32));
  

• magic = in_be32(magicPtr);
  

• debug("%s:  CONFIG_PRAM %d kB magic 0x%x 0x%p\n",
  

•       __FUNCTION__, CONFIG_PRAM,
  

•       magicPtr, magic);
  

• if (magic == 0xbeefbabe) {
  

• 		printf("%s: preserving at %p\n", __FUNCTION__, magicPtr);
  

• 	num_bytes -= (CONFIG_PRAM*1024) - PM_RESERVED_MEM;
  

• }
  

• }

#endif

sync();

Looks good otherwise. Please clean up and resubmit.

Thanks.

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@denx.de =====================================================================