[U-Boot-Users] Antw: Re: [PATCH] Add first Netstal board HCU4

[Stefan Roese <sr@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.
> >