From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Tue, 10 Apr 2007 10:30:20 +0200 Subject: [U-Boot-Users] Antw: Re: [PATCH] Add first Netstal board HCU4 In-Reply-To: References: <200702092137.23677.niklaus.giger@member.fsf.org> <200704041730.19108.sr@denx.de> Message-ID: <200704101030.21228.sr@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Niklaus, please find some comments below: On Friday 06 April 2007 18:16, Niklaus Giger wrote: > diff --git a/cpu/ppc4xx/40x_spd_sdram.c b/cpu/ppc4xx/40x_spd_sdram.c > index 19c4f76..f1e9b38 100644 > --- a/cpu/ppc4xx/40x_spd_sdram.c > +++ b/cpu/ppc4xx/40x_spd_sdram.c > @@ -104,6 +104,7 @@ > > /* function prototypes */ > int spd_read(uint addr); > +void program_ecc (void *bank_base_addr, unsigned long num_bytes, int bank); > > > /* > @@ -306,6 +307,10 @@ long int spd_sdram(int(read_spd)(uint addr)) > sdram0_ecccfg = 0xf << SDRAM0_ECCCFG_SHIFT; > ecc_on = 1; > } else { > +#if defined(CONFIG_ECC) > + debug("%s: no ECC as spd 11: %d 6: %d 14: %d\n", __FUNCTION__, > + read_spd(11), read_spd(6), read_spd(14)); > +#endif > sdram0_ecccfg = 0; > ecc_on = 0; > } > @@ -426,7 +431,9 @@ long int spd_sdram(int(read_spd)(uint addr)) > * program all the registers. > * -------------------------------------------------------------------*/ > > -#define mtsdram0(reg, data) mtdcr(memcfga,reg);mtdcr(memcfgd,data) > +#define mfsdram0(reg, data) { mtdcr(memcfga,reg);data = mfdcr(memcfgd); } > +#define mtsdram0(reg, data) mtdcr(memcfga,reg);mtdcr(memcfgd,data) > + As Wolfgang already mentioned, this needs to be implemented differently. I know, the current implementation lacks this too. I suggest to remove these #defines from this file completely and move them to include/ppc405.h (as done in include/ppc440.h already): /* * Macros for accessing the indirect SDRAM controller registers */ #define mtsdram(reg, data) do { mtdcr(memcfga,reg);mtdcr(memcfgd,data); } while (0) #define mfsdram(reg, data) do { mtdcr(memcfga,reg);data = mfdcr(memcfgd); } while (0) And please change the name from "mtsdram0" to "mtsdram" so they match the defines done in include/ppc440.h. > /* disable memcontroller so updates work */ > mtsdram0( mem_mcopt1, 0 ); > > @@ -449,9 +456,11 @@ long int spd_sdram(int(read_spd)(uint addr)) > /* SDRAM have a power on delay, 500 micro should do */ > udelay(500); > sdram0_cfg = SDRAM0_CFG_DCE | SDRAM0_CFG_BRPF(1) | SDRAM0_CFG_ECCDD | > SDRAM0_CFG_EMDULR; Your patch is line wrapped. Please make sure this doesn't happen with the next version. > - if (ecc_on) > - sdram0_cfg |= SDRAM0_CFG_MEMCHK; > mtsdram0(mem_mcopt1, sdram0_cfg); > +#ifdef CONFIG_ECC > + if (ecc_on) > + program_ecc(0, total_size, 0); > +#endif > > return (total_size); > } > @@ -466,4 +475,75 @@ int spd_read(uint addr) > return 0; > } > > +#define SDRAM_ECCCFG_CE0 0x00800000 /* ECC Correction Enable for Bank 0 */ > +#define SDRAM_ECCCFG_CE1 0x00400000 /* ECC Correction Enable for Bank 1 */ > +#define SDRAM_ECCCFG_CE2 0x00200000 /* ECC Correction Enable for Bank 2 */ > +#define SDRAM_ECCCFG_CE3 0x00100000 /* ECC Correction Enable for Bank 3 */ > + > +#define SDRAM_ECCESR_ERROR_MASK 0xFFF0F000 /* All possible ECC errors */ Please move these defines to the include/ppc405.h header. > +#define ECC_TEST_VALUE 0xaffeaffe > + > +/* > + * Prepare for ECC operation > + * Step 1: Enable ECC generation but not checks > + * Step 2: Fill all memory > + * Step 3: Enable ECC generation and checks > + * Only programmed for and tested on a PPC405GPr board using: > + * bank 0 and 32 bit wide !!! > + */ > +void program_ecc (void *bank_base_addr, unsigned long num_bytes, int bank) > +{ > + unsigned long current_address; > + unsigned long end_address; > + unsigned long address_increment; > + unsigned long cfg0; Please insert a blank line after the variable declarations. > + if (bank != 0) > + { > + printf("\n%s: only bank 0 supported", __FUNCTION__); > + return; > + } > + > + /* > + * get Memory Controller Options 0 data > + */ > + mfsdram0(mem_mcopt1, cfg0); > + > + cfg0 &= ~SDRAM0_CFG_EMDULR & ~SDRAM0_CFG_MEMCHK; I find this better readable: cfg0 &= ~(SDRAM0_CFG_EMDULR | SDRAM0_CFG_MEMCHK); > + debug("%s: length 0x%x bytes bank_base_addr %p\n", __FUNCTION__, > + num_bytes, bank_base_addr); > + debug("%s: cfg0 disable checking -> 0x%08x\n", __FUNCTION__, cfg0); > + /* > + * reset the bank_base address > + */ > + mtsdram0(mem_ecccf, 0); /* disable correction */ > + mtsdram0(mem_eccerr, SDRAM_ECCESR_ERROR_MASK); /* Clear all errors */ > + mtsdram0(mem_mcopt1, cfg0); > + > + address_increment = 4; > + current_address = (unsigned long)(bank_base_addr); > + end_address = (unsigned long)(bank_base_addr) + num_bytes; > + > + while (current_address < end_address) { > + *((unsigned long*)current_address) = 0; > + current_address += address_increment; > + } > + > + mtsdram0(mem_eccerr, SDRAM_ECCESR_ERROR_MASK); /* Clear all errors */ > + > + debug("%s: cfg0 enable checking\n", __FUNCTION__); > + mtsdram0(mem_ecccf, SDRAM_ECCCFG_CE0); /* enable correction */ > + printf("ECC "); > + > +#ifdef DEBUG > + { /* A small sanity check */ > + unsigned long *check; > + check= (unsigned long *)bank_base_addr; > + *check=ECC_TEST_VALUE; Spaces before and after the "=" please. > + if (*check != ECC_TEST_VALUE) > + debug("%s: checking at %p is 0x%x failed\n", > + __FUNCTION__, check, *check); > + } > +#endif > +} > + > #endif /* CONFIG_SPD_EEPROM */ > > Best regards > > Stefan Roese wrote: > > Hi Niklaus, > > > > On Wednesday 14 February 2007 18:10, Niklaus Giger wrote: > >> Here it is: > >> > >> diff --git a/cpu/ppc4xx/spd_sdram.c b/cpu/ppc4xx/spd_sdram.c > > > > Could you please generate a new patch against the current git > > repository? I reorganized the SPD files. "40x_spd_sdram.c" is now > > the file you want. > >