Dear Martha M Stan,
In message <12526727172732-git-send-email-mm...@silicontkx.com> you wrote:
> Signed-off-by: Martha M Stan <mm...@silicontkx.com>
> ---
> board/davedenx/aria/aria.c | 2 +-
> board/esd/mecp5123/mecp5123.c | 2 +-
> board/freescale/mpc5121ads/mpc5121ads.c | 2 +-
> cpu/mpc512x/fixed_sdram.c | 94
> ++++++++++++++++++++-----------
> include/asm-ppc/immap_512x.h | 5 +-
> include/asm-ppc/mpc512x.h | 2 +-
> include/configs/aria.h | 6 +--
> include/configs/mecp5123.h | 5 +-
> include/configs/mpc5121ads.h | 6 +--
> 9 files changed, 72 insertions(+), 52 deletions(-)
...
> diff --git a/cpu/mpc512x/fixed_sdram.c b/cpu/mpc512x/fixed_sdram.c
> index d906903..eb0811c 100644
> --- a/cpu/mpc512x/fixed_sdram.c
> +++ b/cpu/mpc512x/fixed_sdram.c
> @@ -20,23 +20,73 @@
> * MA 02111-1307 USA
> *
> */
> -
> +#define DEBUG 0xff
Please don't do this in released code.
> #include <common.h>
> #include <asm/io.h>
> #include <asm/mpc512x.h>
>
> +/* config settings in order of the 4 mddrc cfg registers */
> +u32 default_mddrc_config[4] = {
> + CONFIG_SYS_MDDRC_SYS_CFG,
> + CONFIG_SYS_MDDRC_TIME_CFG0,
> + CONFIG_SYS_MDDRC_TIME_CFG1,
> + CONFIG_SYS_MDDRC_TIME_CFG2
> +};
As you are playing tricks with the Refresh and Run bits below, this
needs a comment that explains what the values mean, and how to set up
this list.
> +u32 default_init_sequence[] = {
Please add a comment what all this means.
> + CONFIG_SYS_MICRON_NOP,
> + CONFIG_SYS_MICRON_NOP,
...
> + CONFIG_SYS_MICRON_PCHG_ALL,
> + CONFIG_SYS_MICRON_NOP
> +};
> +
> /*
> * fixed sdram init:
> * The board doesn't use memory modules that have serial presence
> * detect or similar mechanism for discovery of the DRAM settings
> */
> -long int fixed_sdram(void)
> +long int fixed_sdram(u32 *mddrc_config, u32 *dram_init_sequence, int
> seq_table_size)
Line too long.
> @@ -46,7 +96,7 @@ long int fixed_sdram(void)
> sync_law(&im->sysconf.ddrlaw.ar);
>
> /* Enable DDR */
> - out_be32(&im->mddrc.ddr_sys_config, CONFIG_SYS_MDDRC_SYS_CFG_EN);
> + out_be32(&im->mddrc.ddr_sys_config, MDDRC_SYS_CFG_EN);
Please stick with the CONFIG_SYS_ name.
> /* Initialize DDR Priority Manager */
> out_be32(&im->mddrc.prioman_config1, CONFIG_SYS_MDDRCGRP_PM_CFG1);
> @@ -74,40 +124,18 @@ long int fixed_sdram(void)
> out_be32(&im->mddrc.lut_table4_alternate_lower,
> CONFIG_SYS_MDDRCGRP_LUT4_AL);
>
> /* Initialize MDDRC */
> - out_be32(&im->mddrc.ddr_sys_config, CONFIG_SYS_MDDRC_SYS_CFG);
> - out_be32(&im->mddrc.ddr_time_config0, CONFIG_SYS_MDDRC_TIME_CFG0);
> - out_be32(&im->mddrc.ddr_time_config1, CONFIG_SYS_MDDRC_TIME_CFG1);
> - out_be32(&im->mddrc.ddr_time_config2, CONFIG_SYS_MDDRC_TIME_CFG2);
> + out_be32(&im->mddrc.ddr_sys_config, mddrc_config[0]);
> + out_be32(&im->mddrc.ddr_time_config0, mddrc_config[1] &
> MDDRC_TIME_CFG0_RFRSH0);
> + out_be32(&im->mddrc.ddr_time_config1, mddrc_config[2]);
> + out_be32(&im->mddrc.ddr_time_config2, mddrc_config[3]);
Why is the " & MDDRC_TIME_CFG0_RFRSH0" needed? Please comment.
> /* Initialize DDR */
...
> + for (i = 0; i < seq_table_size; i++)
> + out_be32(&im->mddrc.ddr_command, dram_init_sequence[i]);
>
> /* Start MDDRC */
> - out_be32(&im->mddrc.ddr_time_config0, CONFIG_SYS_MDDRC_TIME_CFG0_RUN);
> - out_be32(&im->mddrc.ddr_sys_config, CONFIG_SYS_MDDRC_SYS_CFG_RUN);
> + out_be32(&im->mddrc.ddr_time_config0, mddrc_config[1]);
> + out_be32(&im->mddrc.ddr_sys_config, mddrc_config[0] &
> MDDRC_SYS_CFG_RUN);
Please add comment that explains the "& MDDRC_SYS_CFG_RUN" part.
> diff --git a/include/asm-ppc/immap_512x.h b/include/asm-ppc/immap_512x.h
> index 24e6c69..13d3d0e 100644
> --- a/include/asm-ppc/immap_512x.h
> +++ b/include/asm-ppc/immap_512x.h
> @@ -45,7 +45,6 @@
> #define IMMRBAR_BASE_ADDR 0xFFF00000 /* Base address mask */
> #define IMMRBAR_RES ~(IMMRBAR_BASE_ADDR)
>
> -
> #ifndef __ASSEMBLY__
> typedef struct law512x {
> u32 bar; /* Base Addr Register */
> @@ -341,6 +340,10 @@ typedef struct ddr512x {
> u32 res2[0x3AD];
> } ddr512x_t;
>
> +/* MDDRC SYS CFG and Timing CFG0 Registers */
> +#define MDDRC_SYS_CFG_EN 0xF0000000
> +#define MDDRC_SYS_CFG_RUN ~(0x01 << 28)
> +#define MDDRC_TIME_CFG0_RFRSH0 0x0000FFFF
I find this inverse logic confusing. I recommend to use direct values
here, and "& ~FOO" above, then everybody sees what that means.
> diff --git a/include/asm-ppc/mpc512x.h b/include/asm-ppc/mpc512x.h
> index 20456f5..6a65492 100644
> --- a/include/asm-ppc/mpc512x.h
> +++ b/include/asm-ppc/mpc512x.h
> @@ -50,7 +50,7 @@ static inline void sync_law(volatile void *addr)
> /*
> * Prototypes
> */
> -extern long int fixed_sdram(void);
> +extern long int fixed_sdram(u32 *mddrc_config, u32 *dram_init_sequence, int
> seq_table_size);
Line too long.
> diff --git a/include/configs/aria.h b/include/configs/aria.h
> index 4211113..a6fa168 100644
> --- a/include/configs/aria.h
> +++ b/include/configs/aria.h
> @@ -143,14 +143,10 @@
> (0 << 0) /* FIFO_UV_EN */ \
> )
>
> -#define CONFIG_SYS_MDDRC_SYS_CFG_RUN (CONFIG_SYS_MDDRC_SYS_CFG & ~(1 << 28))
> +#define CONFIG_SYS_MDDRC_TIME_CFG0 0x030C3D2E
> #define CONFIG_SYS_MDDRC_TIME_CFG1 0x55D81189
> #define CONFIG_SYS_MDDRC_TIME_CFG2 0x34790863
>
> -#define CONFIG_SYS_MDDRC_SYS_CFG_EN 0xF0000000
> -#define CONFIG_SYS_MDDRC_TIME_CFG0 0x00003D2E
> -#define CONFIG_SYS_MDDRC_TIME_CFG0_RUN 0x030C3D2E
> -
> #define CONFIG_SYS_MICRON_NOP 0x01380000
> #define CONFIG_SYS_MICRON_PCHG_ALL 0x01100400
> #define CONFIG_SYS_MICRON_EMR ( (1 << 24) | /* CMD_REQ */ \
Should not all these CONFIG_SYS_MICRON_* definitions now also be
eliminated / merged? This affects include/configs/aria.h,
include/configs/mecp5123.h, and include/configs/mpc5121ads.h .
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: w...@denx.de
You don't have to worry about me. I might have been born yesterday...
but I stayed up all night.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
