[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[Benedict, Michael]

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 */

[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[Timur Tabi]

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
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[Benedict, Michael]

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.

[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[Timur Tabi]

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
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[Benedict, Michael]

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

[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[Timur Tabi]

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.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[Kim Phillips]

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

[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[Timur Tabi]

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
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[Timur Tabi]

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
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[Benedict, Michael]

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

[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[Timur Tabi]

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
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[Benedict, Michael]

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

[U-Boot-Users] [PATCH] Fix memory initialization on MPC8349ITX

[Timur Tabi]

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

-- 
Timur Tabi
Linux Kernel Developer @ Freescale