[U-Boot-Users] Antw: Re: [PATCH] Add first Netstal board HCU4
Stefan Roese
sr at denx.de
Tue Apr 10 10:30:20 CEST 2007
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.
> >
More information about the U-Boot
mailing list