Fix memory initialization on MPC8349ITX

Hello,
This is my first patch submission to u-boot, so let me know if
the format needs to be updated. Also, I don't know how formal the sign-off procedure has to be. This patch was suggested by Bruce Leonard in his 2007/4/20 email to the list, and so I am counting that as sign-off. This patch is against the MPC83xx custodian tree, so I apologize if this should instead be sent directly to Kim Phillips.

Define CFG_DDR_SDRAM_CLK_CNTL for the MPC8349ITX. This allows ddr->sdram_clk_cntl to be properly initialized, like it was before commit f6eda7f80ccc13d658020268c507d7173cf2e8aa to cpu/mpc83xx/spd_sdram.c

Patch initially suggested by Timur Tabi and implemented / tested by Bruce Leonard.

index 37bbfb3..e0c0227 100644
@@ -149,6 +149,8 @@
  */
 #define CFG_DDR_BASE           0x00000000      /* DDR is system

memory*/
#define CFG_SDRAM_BASE CFG_DDR_BASE
+#define CFG_DDR_SDRAM_CLK_CNTL (DDR_SDRAM_CLK_CNTL_SS_EN | \
+ DDR_SDRAM_CLK_CNTL_CLK_ADJUST_05)
#define CFG_DDR_SDRAM_BASE CFG_DDR_BASE
#define CFG_83XX_DDR_USES_CS0
#define CFG_MEMTEST_START 0x1000 /* memtest region */

There should be a "---" underneath the signed-off-by lines, and comments about the patch should be located *below* the "---". Patches for 83xx should be posted to u-boot-users and emailed to Kim Phillips directly.

You should probably update fixed_sdram() in mpc8349itx.c as well. Currently, it has this code:

	im->ddr.sdram_clk_cntl =
	    DDR_SDRAM_CLK_CNTL_SS_EN | DDR_SDRAM_CLK_CNTL_CLK_ADJUST_05;

That should be changed to

	im->ddr.sdram_clk_cntl = CFG_DDR_SDRAM_CLK_CNTL;

I'll test this code on my ITX, however, I'm curious about one thing. Can you explain why this patch is okay for *all* ITX boards? I remember something about your board having problematic DDR or something. With your patch, all ITX and ITX-GP boards will set sdram_clk_cntl to the new value when SPD is used.

Timur Tabi
Linux Kernel Developer @ Freescale

Thank you, stay tuned for another attempt at the patch.

I am not aware of my board having a problematic DDR. Perhaps this is the proposed fix to the issue you are thinking of? I think the big question is how many people using the 8349ITX are using git sources. Both Bruce and I experienced instability without this patch, to the point the system could not boot to console. After applying this patch, neither of us have witnessed any instability after banging on the system(*).

As I mentioned in a previous email, for the MPC8349ITX I am _reverting_ the configured value for the sdram_clk_cntl to what Freescale shipped in their patched u-boot 1.1.3 and to the value in the mpc83xx git sources prior to October 25, 2006. In fact, I think that a simliar patch might need to be applied for the MPC832XEMDS for exactly the same reasons. I think it would be helpful if you could look at the f6eda7f80ccc13d658020268c507d7173cf2e8aa commit to cpu/mpc83xx/spd_sdram.c and how the sdram_clk_cntl register was initialized before/after this commit. From that, you should (hopefully) either be able to confirm the proposed patch is doing the right thing or tell me that I am not understanding something.
Thank you,
Michael

* - I lied, I do have some issues with the serial layer that I am sorting through. However, I think this is unrelated. The instability prior to this patch was much wider in scope.

Ok, I must be confused then.

Well, I am! I've never had any problems with my ITX and this code.

When you boot the board, what does the "CPU:" line say?

Timur Tabi
Linux Kernel Developer @ Freescale

Okay, now I am more confused.

U-Boot 1.2.0-g6fbf261f-dirty (Apr 19 2007 - 11:00:04) MPC83XX

Clock configuration:
Coherent System Bus: 266 MHz
Core: 533 MHz
Local Bus Controller: 266 MHz
Local Bus: 66 MHz
DDR: 266 MHz
SEC: 88 MHz

  I2C1:                 266 MHz
  I2C2:                 266 MHz
  TSEC1:                266 MHz
  TSEC2:                266 MHz
  USB MPH:               88 MHz
  USB DR:                88 MHz
CPU: MPC8349E, Rev: 11 at 533.333 MHz
Board: Freescale MPC8349E-mITX
UPMA:  Configured for compact flash
I2C:   ready
DRAM:
DIMM type:       K
SPD size:        128
EEPROM size:     256
Memory type:     7
Row addr:        13
Column addr:     10
# of rows:       1
Row density:     64
# of banks:      4
Data width:      64
Chip width:      8
Refresh rate:    82
CAS latencies:   1C
Write latencies: 02
tRP:             56
tRCD:            56

For what its worth... Both Bruce and I are using boards that came with a Kingston KVR400X64C3A/256 DDR memory module. What do you have?
Cheers,
Michael

The same one.

Well, I can't explain it, but I'll test your patch on my board and see if it breaks anything. If not, I'll ack it.

Timur Tabi
Linux Kernel Developer @ Freescale

to:ing or cc:ing me helps me not miss things, but I don't require it. It also helps indicate which tree you want it to go through by inserting, e.g., "mpc83xx: " after "[PATCH]" in the subject line.

I added a link for the linux kernel signoff policy here:

http://www.denx.de/wiki/UBoot/DevelopmentProcess

so I'm guessing this patch would be signed off by Bruce, then Michael.

your mailer is wrapping lines.

I've tested this patch, and it works fine on my ITX and ITX-GP, so please post a new, complete patch and I'll ack it.

Timur Tabi
Linux Kernel Developer @ Freescale

I've been thinking about this. Could you try one more thing before posting the new patch?
In cpu/mpc83xx/spd_sdram.c, could you comment-out this line:

	ddr->sdram_clk_cntl = 0x00000000;

and also *not* define CFG_DDR_SDRAM_CLK_CNTL in MPC8349ITX.h? In other words, I want to see what happens if you don't write anything at all to ddr->sdram_clk_cntl.

Timur Tabi
Linux Kernel Developer @ Freescale

The instability persists. I had debugging on, and the debug line in spd_sdram.c still prints this value as 0x00000000.
Cheers,
Michael

Well, that's odd. I just got a revision 3.1 board here that, when I follow those steps (don't define CFG_DDR_SDRAM_CLK_CNTL and don't set sdram_clk_cntl to 0), it works when before it didn't.

So I'm wondering if we should do both:

1. Set CFG_DDR_SDRAM_CLK_CNTL in MPC8349ITX.h
2. Change spd_sdram() to not write 0 if CFG_DDR_SDRAM_CLK_CNTL is not set.

I just don't understand why some boards have the right value on power-up and some don't, and why some boards work okay with 0 and others don't.

Timur Tabi
Linux Kernel Developer @ Freescale

I have a revision 1.0 board here (I think... The silkscreen in the middle of the board reads "MPC8349E-mITX REV1.0"). Is there any chance the revision can explain the disparity in behavior?
-Michael

They all say "REV 1.0". I was talking about the CPU. I have two 1.0 boards, one with a rev 1.1 CPU and another with a rev 3.1 CPU. You can tell via the U-Boot CPU: line:

CPU: MPC8349E, Rev: 31 at 533.333 MHz

Timur Tabi
Linux Kernel Developer @ Freescale