[U-Boot-Users] [PATCH] 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.
Signed-off-by: Bruce Leonard Bruce_Leonard@selinc.com
diff --git a/include/configs/MPC8349ITX.h b/include/configs/MPC8349ITX.h index 37bbfb3..e0c0227 100644 --- a/include/configs/MPC8349ITX.h +++ b/include/configs/MPC8349ITX.h @@ -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 */
Benedict, Michael wrote:
Hello, This is my first patch submission to u-boot, so let me know if the format needs to be updated.
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.
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
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 wrote:
<improvements/fixes to patch and patch format>
Thank you, stay tuned for another attempt at the patch.
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.
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.
Benedict, Michael wrote:
I am not aware of my board having a problematic DDR.
Ok, I must be confused then.
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.
Well, I am! I've never had any problems with my ITX and this code.
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(*).
When you boot the board, what does the "CPU:" line say?
Timur Tabi wrote:
Benedict, Michael wrote:
I think the big question is how many people using the 8349ITX are using git sources.
Well, I am! I've never had any problems with my ITX and this code.
Okay, now I am more confused.
When you boot the board, what does the "CPU:" line say?
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
Benedict, Michael wrote:
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?
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.
On Tue, 24 Apr 2007 11:51:33 -0500 "Benedict, Michael" MBenedict@twacs.com wrote:
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.
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.
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.
Signed-off-by: Bruce Leonard Bruce_Leonard@selinc.com
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.
diff --git a/include/configs/MPC8349ITX.h b/include/configs/MPC8349ITX.h index 37bbfb3..e0c0227 100644 --- a/include/configs/MPC8349ITX.h +++ b/include/configs/MPC8349ITX.h @@ -149,6 +149,8 @@ */ #define CFG_DDR_BASE 0x00000000 /* DDR is system memory*/
your mailer is wrapping lines.
#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)
tabs, then spaces, please.
Thanks,
Kim
Benedict, Michael wrote:
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
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 wrote:
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.
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 wrote:
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.
The instability persists. I had debugging on, and the debug line in spd_sdram.c still prints this value as 0x00000000. Cheers, Michael
Benedict, Michael wrote:
The instability persists. I had debugging on, and the debug line in spd_sdram.c still prints this value as 0x00000000.
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 wrote:
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.
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
Benedict, Michael wrote:
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?
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
participants (3)
-
Benedict, Michael -
Kim Phillips -
Timur Tabi