[U-Boot] [PATCH] EXYNOS: SMDK5250: Add support for DDR3 memory.
Doug Anderson
dianders at chromium.org
Fri Feb 10 00:23:33 CET 2012
Hatim,
Overall comments:
* Random delays are evil. Please explain each sdelay() call. Be sure
to include information about why exactly 65,536 iterations through a
delay loop is necessary in each case.
* For functions that are nearly the same between dmc_init.c and
dmc_init_ddr3.c should be unified. If possible, unify everything.
* If we really need to have two files, please submit a separate patch
to rename dmc_init.c to dmc_init_lpddr2.c
* If we really need to have two files, please reorder functions so
that functions that are in both dmc_init_ddr3.c and in
dmc_init_lpddr2.c are in the same order for easy diffing.
* In general, avoid defining hex registers. Instead, define things
symbolically. See comments about DMC_MEMCONTROL_VAL below as an
example.
* I've started reviewing this before the patches from Chander. I'll
probably go back and review that next, if I have time. Expect similar
comments there.
See below for some scattered comments. Note that I didn't fully
review everything, so I'd expect another round will be needed.
On Wed, Feb 8, 2012 at 3:44 AM, Hatim Ali <hatim.rv at samsung.com> wrote:
> --- a/board/samsung/smdk5250/dmc_init.c
> +++ b/board/samsung/smdk5250/dmc_init.c
> @@ -248,7 +248,7 @@ static void config_rdlvl(struct exynos5_dmc *dmc,
> * ctrl_gateduradj, rdlvl_pass_adj
> * rdlvl_rddataPadj
> */
> - val = SET_RDLVL_RDDATAPADJ;
> + val = SET_RDLVL_RDDATA_ADJ;
This doesn't seem related to DDR3. Perhaps it should be split into a
separate patch?
> @@ -361,8 +361,8 @@ void mem_ctrl_init()
> config_zq(phy0_ctrl, phy1_ctrl);
>
> /* Operation Mode : LPDDR2 */
> - val = PHY_CON0_RESET_VAL;
> - SET_CTRL_DDR_MODE(val, DDR_MODE_LPDDR2);
> + val = CTRL_DDR_MODE(LPDDR2);
> + val |= BYTE_RDLVL_EN;
I'd prefer this (and related header change) to be in a separate patch as well.
> +/* APLL : 1GHz */
> +/* MCLK_CDREX: MCLK_CDREX_677*/
> +/* LPDDR support: DDR3 */
I assume that future CLs will relax some of these restrictions?
> + /* Sending PALL cmd on
> + * Channel0-Chip0
> + * Channel0-Chip1
> + * Channel1-Chip0
> + * Channel1-Chip1
> + */
nit: please use proper commenting style. AKA:
/*
* Sending PALL cmd on
* - Channel0-Chip0
* - Channel0-Chip1
* - Channel1-Chip0
* - Channel1-Chip1
*/
> + for (channel = 0; channel < CONFIG_DMC_CHANNELS; channel++) {
> + SET_CMD_CHANNEL(mask, channel);
> + for (chip = 0; chip < CONFIG_CHIPS_PER_CHANNEL; chip++) {
> + SET_CMD_CHIP(mask, chip);
> + val = DIRECT_CMD_PALL | mask;
> + writel(val, &dmc->directcmd);
> + sdelay(0x10000);
As with all sdelays, please explain.
> + }
> + }
> +
> +}
> +
> +
> +static void dmc_directcmd(struct exynos5_dmc *dmc)
> +{
> + unsigned long channel, mask = 0, val;
> +
> + /* Selecting Channel0-Chip0 */
> + SET_CMD_CHANNEL(mask, 0);
> + SET_CMD_CHIP(mask, 0);
> +
> + /* Sending NOP cmd on Channel0-Chip0 */
> + val = DIRECT_CMD_NOP | mask;
> + writel(val, &dmc->directcmd);
> + sdelay(0x10000);
> +
> + dmc_directcmd_PALL(dmc);
> +
> + /* Selecting Chip 0*/
> + mask = 0;
> + SET_CMD_CHIP(mask, 0);
> + for (channel = 0; channel < CONFIG_DMC_CHANNELS; channel++) {
> + /* Selecting Channel */
> + SET_CMD_CHANNEL(mask, channel);
> +
> + if (channel == 1) {
> + /* Sending NOP cmd on Channel1-Chip0 */
> + val = DIRECT_CMD_NOP | mask;
> + writel(val, &dmc->directcmd);
> + sdelay(0x10000);
> + }
> + /* Sending MRS/EMRS command and ZQINIT command
> + * on Channel0-Chip0
> + */
> + val = DIRECT_CMD_MRS1 | mask;
> + writel(val, &dmc->directcmd);
> + sdelay(0x10000);
> +
> + val = DIRECT_CMD_MRS2 | mask;
> + writel(val, &dmc->directcmd);
> + sdelay(0x10000);
> +
> + val = DIRECT_CMD_MRS3 | mask;
> + writel(val, &dmc->directcmd);
> + sdelay(0x10000);
> +
> + val = DIRECT_CMD_MRS4 | mask;
> + writel(val, &dmc->directcmd);
> + sdelay(0x10000);
> +
> + val = DIRECT_CMD_ZQINIT | mask;
> + writel(val, &dmc->directcmd);
> + sdelay(0x10000);
> + }
> +}
> +
> +static void update_reset_dll(struct exynos5_dmc *dmc)
> +{
* Unify with LPDDR2 version and/or explain differences.
> +static void config_memory(struct exynos5_dmc *dmc)
> +{
> + /*
> + * Dynamic Clock: Always Running
> + * Memory Burst length: 8
> + * Number of chips: 1
> + * Memory Bus width: 32 bit
> + * Memory Type: DDR3
> + * Additional Latancy for PLL: 0 Cycle
> + */
> + writel(DMC_MEMCONTROL_VAL, &dmc->memcontrol);
I like the explanation of what value is being set, but:
* The description belongs in the header file, not here. Then, you can
unify source for LPDDR2 and DDR3.
* Instead of defining 'DMC_MEMCONFIG0_VAL' to be 0x00001323, you
should construct it from its various bits. This eliminates the need
for this documentation completely and makes sure that the
documentation and value are never out of date. For instance, you
could do this:
#define DMC_MEMCONTROL_VAL ( \
DMC_MEMCONTROL_CLK_STOP_DISABLE | \
DMC_MEMCONTROL_DPWRDN_DISABLE | \
DMC_MEMCONTROL_DPWRDN_ACTIVE_PRECHARGE | \
DMC_MEMCONTROL_TP_DISABLE | \
DMC_MEMCONTROL_DSREF_DISABLE | \
(0 << DMC_MEMCONTROL_ADD_LAT_PALL_CYCLE_OFFSET) | \
DMC_MEMCONTROL_MEM_TYPE_DDR3 | \
DMC_MEMCONTROL_MEM_WIDTH_32BIT | \
DMC_MEMCONTROL_NUM_CHIP_1 | \
DMC_MEMCONTROL_BL_8 | \
DMC_MEMCONTROL_PZQ_DISABLE | \
DMC_MEMCONTROL_MRR_BYTE_7_0 | \
)
...same comments apply to all of the other #defines of random hex values.
> +static void reset_phy_ctrl(void)
> +{
> + struct exynos5_clock *clk = (struct exynos5_clock *)EXYNOS5_CLOCK_BASE;
> + writel(PHY_RESET_VAL, &clk->lpddr3phy_ctrl);
> + writel(PHY_SET_VAL, &clk->lpddr3phy_ctrl);
> + writel(PHY_RESET_VAL, &clk->lpddr3phy_ctrl);
> + writel(PHY_SET_VAL, &clk->lpddr3phy_ctrl);
> +}
* Please include a comment explaining why you need to set values
twice. Is this errata?
* You leave the clock in a different state than the same-named
function in mmc_init.c. Why?
* Can you explain what the values of SET and RESET mean here? What
are you trying to accomplish?
> +static void config_zq(struct exynos5_phy_control *phy0_ctrl,
> + struct exynos5_phy_control *phy1_ctrl)
* If we really need a separate copy of config_zq, please move this
function higher in the file so function ordering matches "dmc_init.c"
* Please unify with the copy in "dmc_init.c". At the moment, there
are 4 differences. Are they each needed? Current differences:
-> Uses "val = val |" instead of "val |=". Not needed.
-> Adds | of ZQ_CLK_EN. This appears to be a no-op, since
PHY_CON16_RESET_VAL already includes this. Not needed
-> Removes setting of ZQ_MODE_NOTERM, but still writes to the
registers twice. Removing ZQ_MODE_NOTERM seems like a reasonable
thing to do, but I don't understand why registers need to be written
twice.
-> There is double the sdelay here. As with all sdelays, please explain.
> diff --git a/board/samsung/smdk5250/setup.h b/board/samsung/smdk5250/setup.h
> index 1276fd3..2d7e5a4 100644
> --- a/board/samsung/smdk5250/setup.h
> +++ b/board/samsung/smdk5250/setup.h
> @@ -354,12 +365,18 @@
> #define CONFIG_IV_SIZE 0x07
>
> #define PHY_RESET_VAL (0 << 0)
> +#define PHY_SET_VAL (1 << 0)
Are you sure this is the right name of these values? From the docs I
see, I would have assumed that RESET was 1 and RESET_OFF was 0.
Also: IMHO these names should include the register name so that they
have some meaning, AKA:
#define LPDDR3PHY_CTRL_PHY_RESET (1 << 0)
#define LPDDR3PHY_CTRL_PHY_RESET_OFF (0 << 0)
>> +/* Choose DDR Type below
> + * * Uncomment the type of DDR present on your board
> + * */
> +#define CONFIG_DDR3
> +/*
> + * #define CONFIG_LPDDR2
> + */
> +
Is there any way to detect which of the two boards you have automatically?
-Doug
More information about the U-Boot
mailing list