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@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