[U-Boot-Users] [PATCH] Add first Netstal board HCU4
Hi
We at Netstal Maschinen (http://www.netstal.com/) will probably use the U-Boot in our next board, based on a PPC440EPx.
The reason we choose u-boot over a proprietary boot eprom are still the same as described in my earlier e-mail from February 2005 http://sourceforge.net/mailarchive/message.php?msg_id=10996256
Outside my activities at Netstal I had ported U-boot in 2005 to run on our current board (based on a PPC405GPr). I used it to get acquainted with Linux and Xenomai and one board is currently used by Xenomais buildbot. I would like to get this board into the U-Boot repository to have a solid base for our new board and to be able to test various features before getting hand onto our new board (somewhere in March/April).
As I am new to the U-Boot and Git, I am not sure, whether I got everything right in my first try to submit a patch. I did a) git clone rsync://source.denx.net/git/u-boot.git u-boot.git b) Added my files, testing them, etc c) edited .git/config with my personal info d) git-commit --all --signoff -m "Add first Netstal board HCU4" e) gz'ed the resulting 0001-Add-first-Netstal-board-HCU4.txt to reduce the size from 52KB to 15 KB. f) as between a) and c) passed various week I g) git pull As there were no conflicts, I rebuilded my target, tested again. Then I called h) ./MAKEALL Most target run okay, but I have errors like Configuring for p3mx board... p3mx.c:48:18: error: p3mx.h: No such file or directory p3mx.c:48:18: error: p3mx.h: No such file or directory which I believe are unrelated to my changes.
I am unsatisfied that I had to add a flash.c to my board, but I have at the moment no idea how a clean solution should look like. But I think in the various flash.c there is a lot of duplicated code around.
As our board has ECC memory, I would like to submit in a second step a patch to get ECC working on PPC405.
Any suggestion on how to improve this patch are welcome.
Best regards
Niklaus Giger -- As my first try from my workplace was considered spam I try it with my home e-mail. Signed-off-by: Niklaus Giger niklaus.giger@netstal.com
Hi Niklaus,
On Friday 09 February 2007 21:37, Niklaus Giger wrote:
h) ./MAKEALL Most target run okay, but I have errors like Configuring for p3mx board... p3mx.c:48:18: error: p3mx.h: No such file or directory p3mx.c:48:18: error: p3mx.h: No such file or directory which I believe are unrelated to my changes.
That's my fault. Fixed soon.
I am unsatisfied that I had to add a flash.c to my board, but I have at the moment no idea how a clean solution should look like. But I think in the various flash.c there is a lot of duplicated code around.
Yes, that has mostly historic reasons. So why do you have to use a board specific flash driver and not the common cfi driver?
As our board has ECC memory, I would like to submit in a second step a patch to get ECC working on PPC405.
So the current patch doesn't include this ECC support? Do you use the 4xx SPD driver (cpu/ppc/spd_sdram.c) or the no SPD driver (cpu/ppc/sdram.c)? Or even a board specif one?
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk Office: Kirchenstr. 5, D-82194 Groebenzell, Germany =====================================================================
Am Samstag, 10. Februar 2007 08:59 schrieb Stefan Roese:
Hi Niklaus,
On Friday 09 February 2007 21:37, Niklaus Giger wrote:
h) ./MAKEALL Most target run okay, but I have errors like Configuring for p3mx board... p3mx.c:48:18: error: p3mx.h: No such file or directory p3mx.c:48:18: error: p3mx.h: No such file or directory which I believe are unrelated to my changes.
That's my fault. Fixed soon.
I am unsatisfied that I had to add a flash.c to my board, but I have at the moment no idea how a clean solution should look like. But I think in the various flash.c there is a lot of duplicated code around.
Yes, that has mostly historic reasons. So why do you have to use a board specific flash driver and not the common cfi driver?
Because I started with a board that had a board specific flash.c, which worked. And a first attempt to use the common CFI failed, as I mixed too many changes. But I will try next week to follow you suggestion.
As our board has ECC memory, I would like to submit in a second step a patch to get ECC working on PPC405.
So the current patch doesn't include this ECC support? Do you use the 4xx SPD driver (cpu/ppc/spd_sdram.c) or the no SPD driver (cpu/ppc/sdram.c)? Or even a board specif one?
Yes it does not.
I did not want to mix in a single patch a new board and a new feature. And as it is the first time I work with git I not know how to easily work on two different patches at the same time. But I will try next week to come up with a patch for ECC.
I think I use cpu/ppc/spd_sdram.c as I call in my init_dram procedure dram_size = spd_sdram(0); Alternatively I first brought up my board using the known good values for the sdram registers in my procedure fixed_hcu4_sdram, which I left for debugging purposes #if !defined(CONFIG_SPD_EEPROM).
Best regards, Stefan
Thanks for your tips.
Best regards
On Saturday 10 February 2007 10:01, Niklaus Giger wrote:
Yes, that has mostly historic reasons. So why do you have to use a board specific flash driver and not the common cfi driver?
Because I started with a board that had a board specific flash.c, which worked. And a first attempt to use the common CFI failed, as I mixed too many changes. But I will try next week to follow you suggestion.
What flash devices do you use on your board?
So the current patch doesn't include this ECC support? Do you use the 4xx SPD driver (cpu/ppc/spd_sdram.c) or the no SPD driver (cpu/ppc/sdram.c)? Or even a board specif one?
Yes it does not.
I did not want to mix in a single patch a new board and a new feature. And as it is the first time I work with git I not know how to easily work on two different patches at the same time. But I will try next week to come up with a patch for ECC.
I think I use cpu/ppc/spd_sdram.c as I call in my init_dram procedure dram_size = spd_sdram(0);
Yes, you are using the common 4xx SPD driver then.
Alternatively I first brought up my board using the known good values for the sdram registers in my procedure fixed_hcu4_sdram, which I left for debugging purposes #if !defined(CONFIG_SPD_EEPROM).
I'll try to take a deeper look at your patch on Monday.
Thanks for your tips.
You're welcome.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk Office: Kirchenstr. 5, D-82194 Groebenzell, Germany =====================================================================
Hi
Attached you find a patch which enables ECC for our board. At the moment it is pretty PPC405GPr specific, as I have no other board around to test. I have also a Yosemite and a Sequoia, but they do not seem to have a correct SPD-info. I will submit a patch once I have our PPC440EPx board up and running.
For the startup I added a small hint that ECC is working:
I2C: ready DRAM: ECC 32 MB FLASH: 512 kB
Stefan Roesesr@denx.de 10.02.07 20:15:23 >>>
On Saturday 10 February 2007 10:01, Niklaus Giger wrote:
Yes, that has mostly historic reasons. So why do you have to use a board specific flash driver and not the common cfi driver?
Because I started with a board that had a board specific flash.c, which worked. And a first attempt to use the common CFI failed, as I mixed too many changes. But I will try next week to follow you suggestion.
What flash devices do you use on your board?
I have a 8 bit boot-eprom AMD AM29LV040B and 8,16 or 32 MB of CFI-Flash (one 16 bit-wide bank), e.g. Intel 28F028J3A
<..> Best regards, Niklaus
begin 666 ecc.patch M9&EF9B`M+6=I="!A+V-P=2]P<&,T>'@O<W!D7W-D<F%M+F,@8B]C<'4O<'!C M-'AX+W-P9%]S9')A;2YC"F]L9"!M;V1E(#$P,#8T-`IN97<@;6]D92`Q,#`W M-34*:6YD97@@8S(T-#4V8BXN8V$Y,C)D-`HM+2T@82]C<'4O<'!C-'AX+W-P M9%]S9')A;2YC"BLK*R!B+V-P=2]P<&,T>'@O<W!D7W-D<F%M+F,*0$`@+30R M+#8@*S0R+#<@0$`@(VEN8VQU9&4@/&-O;6UO;BYH/@H@(VEN8VQU9&4@/&%S M;2]P<F]C97-S;W(N:#X*("-I;F-L=61E(#QI,F,N:#X*("-I;F-L=61E(#QP M<&,T>'@N:#X**R-I;F-L=61E(#QA<VTO:6\N:#X*(`H@(VEF9&5F($-/3D9) M1U]34$1?14504D]-"B`*0$`@+3DX+#8@*SDY+#@@0$`@(V5L<V4*(",@9&5F M:6YE(%-01%]%4E(H>"D@9&@>R!P<FEN=&8H>"D[(')E='5R;B@P*3L@?2!W M:&EL92`H,"D*("-E;F1I9@H@"BMV;VED('!R;V=R86U?96-C("AV;VED("IB M86YK7V)A<V5?861D<BP@=6YS:6=N960@;&]N9PD@;G5M7V)Y=&5S+"!I;G0@ M8F%N:RD["BL*("-D969I;F4@<V1R86U?2%I?=&]?;G,H:&5R='HI("@Q,#`P M,#`P,#`P+RAH97)T>BDI"B`*("\J(&9U;F-T:6]N('!R;W1O='EP97,@*B* M0$`@+3,P-"PV("LS,#<L,3`@0$`@(V5N9&EF"B`)"7-D<F%M,%]E8V-C9F<@ M/2`P>&8@/#P@4T1204TP7T5#0T-&1U]32$E&5#L*(`D)96-C7V]N(#T@,3L* M(`E](&5L<V4@>PHK(VEF(&1E9FEN960H0T].1DE'7T5#0RD@)B8@9&5F:6YE M9"A$14)51RD**PD)<')I;G1F*"(E<SH@;F@14-#(&%S('-P9"`Q,3H@)60@ M("`V.B`E9"`Q-#H@)61<;B(L(%]?1E5.0U1)3TY?7RP**PD)("`@("`@(')E M861?<W!D*#$Q*2P@<F5A9%]S<&0H-BDL(')E861?<W!D*#$T*2D["BLC96YD M:68*(`D)<V1R86TP7V5C8V-F9R`](#`["B`)"65C8U]O;B`](#`["B`)?0I` M0"`M-#(T+#<@*S0S,2PX($!`("-E;F1I9@H@"2`J('!R;V=R86T@86QL('1H M92!R96=I<W1E<G,N"B`)("H@+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM M+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2HO"B`* M+2-D969I;F4@;71S9')A;3`H<F5G+"!D871A*2`@;71D8W(H;65M8V9G82QR M96<I.VUT9&-R*&UE;6-F9V0L9&%T82D**R-D969I;F4@;71S9')A;3`H<F5G M+"!D871A*2`@("!M=&1C<BAM96UC9F=A+')E9RD[;71D8W(H;65M8V9G9"QD M871A*0HK(V1E9FEN92!M9G-D<F%M,"AR96<L(&1A=&$I("![(&UT9&-R*&UE M;6-F9V$L<F5G*3MD871A(#T@;69D8W(H;65M8V9G9"D[('T*(`DO*B!D:7-A M8FQE(&UE;6-O;G1R;VQL97(@<V@=7!D871E<R!W;W)K("HO"B`);71S9')A M;3`H(&UE;5]M8V]P=#$L(#`@*3L*(`I`0"`M-#0W+#$P("LT-34L,3`@0$`@ M(V5N9&EF"B`)+RH@4T1204T@:&%V92!A('!O=V5R(&]N(&1E;&%Y+`D@-3`P M(&UI8W)O('-H;W5L9"!D;R`J+PH@"75D96QA>2@U,#`I.PH@"7-D<F%M,%]C M9F<@/2!31%)!33!?0T9'7T1#12!(%-$4D%-,%]#1D=?0E)01B@Q*2!(%-$ M4D%-,%]#1D=?14-#1$0@?"!31%)!33!?0T9'7T5-1%5,4CL*+0EI9B`H96-C M7V]N*0HM"0ES9')A;3!?8V9G('P](%-$4D%-,%]#1D=?345-0TA+.PH@"6UT M<V1R86TP*&UE;5]M8V]P=#$L('-D<F%M,%]C9F<I.PHM"BLC:69D968@0T]. M1DE'7T5#0PHK"6EF("AE8V-?;VXI('!R;V=R86U?96-C*#`L('1O=&%L7W-I M>F4L(#`I.PHK(V5N9&EF"B`)<F5T=7)N("AT;W1A;%]S:7IE*3L*('T*(`I` M0"`M-CDV+#@@*S<P-"PV($!`('9O:60@<')O9W)A;5]T<C`@*'5N<VEG;F5D M(&QO;F<J(&1I;6U?<&*(`H@=F]I9"!P<F]G<F%M7W1R,2`H=F]I9"D["B`* M+79O:60@<')O9W)A;5]E8V,@*'5N<VEG;F5D(&QO;F<)(&YU;5]B>71E<RD[ M"BT*('5N<VEG;F5D"B!L;VYG("!P<F]G<F%M7V)X8W(H=6YS:6=N960@;&]N M9RH@9&EM;5]P;W!U;&%T960L"B`)"2`@('5N<VEG;F5D(&-H87(J(&EI8S!? M9&EM;5]A9&1R+`I`0"`M,3<X-2PT-R`K,3<Y,2PW-B!`0"`C96YD:68*(`ER M971U<FXH8F%N:U]B87-E7V%D9'(I.PH@?0H@"BUV;VED('!R;V=R86U?96-C M("AU;G-I9VYE9"!L;VYG"2!N=6U?8GET97,I"BLC96YD:68@+RH@0T].1DE' M7S0T,"`J+PHK"BLC9&5F:6YE(%-$4D%-7T5#0T-&1U]#13`)"3!X,#`X,#`P M,#`)+RH@14-#($-O<G)E8W1I;VX@16YA8FQE(&9O<B!"86YK(#`)*B**R-D M969I;F4@4T1204U?14-#0T9'7T-%,0D),'@P,#0P,#`P,`DO*B!%0T,@0V]R M<F5C=&EO;B!%;F%B;&4@9F]R($)A;FL@,0DJ+PHK(V1E9FEN92!31%)!35]% M0T-#1D=?0T4R"0DP>#`P,C`P,#`P"2\J($5#0R!#;W)R96-T:6]N($5N86)L M92!F;W(@0F%N:R`R"2HO"BLC9&5F:6YE(%-$4D%-7T5#0T-&1U]#13,)"3!X M,#`Q,#`P,#`)+RH@14-#($-O<G)E8W1I;VX@16YA8FQE(&9O<B!"86YK(#,) M*B**PHK(V1E9FEN92!31%)!35]%0T-%4U)?15)23U)?34%32R`@("`@("`@ M(#!X1D9&,$8P,#`@("`@("`O*B!!;&P@<&]S<VEB;&4@14-#(&5R<F]R<R`J M+PHK(V1E9FEN92!%0T-?5$535%]604Q512`P>&%F9F5A9F9E"BL**R\J(`HK M("H@4')E<&%R92!F;W(@14-#(&]P97)A=&EO;@HK("H@4W1E<"`Q.B!%;F%B M;&4@14-#(&=E;F5R871I;VX@8G5T(&YO="!C:&5C:W,**R`J(%-T97`@,CH@ M1FEL;"!A;&P@;65M;W)Y"BL@*B!3=&5P(#,Z($5N86)L92!%0T,@9V5N97)A M=&EO;B!A;F0@8VAE8VMS"BL@*B!/;FQY('!R;V=R86UM960@9F]R(&%N9"!T M97-T960@;VX@82!04$,T,#5'4'(@8F]A<F0@=7-I;F<@;VYL>2!B86YK(#`@ M86YD(#,R(&)I="!W:61E("$A(0HK("HO"BMV;VED('!R;V=R86U?96-C("AV M;VED("IB86YK7V)A<V5?861D<BP@=6YS:6=N960@;&]N9R!N=6U?8GET97,L M(&EN="!B86YK*0H@>PHM"75N<VEG;F5D(&QO;F<@8F%N:U]B87-E7V%D9'([ M"B`)=6YS:6=N960@;&]N9R!C=7)R96YT7V%D9')E<W,["B`)=6YS:6=N960@ M;&]N9R!E;F1?861D<F5S<SL*(`EU;G-I9VYE9"!L;VYG(&%D9')E<W-?:6YC M<F5M96YT.PH@"75N<VEG;F5D(&QO;F<@8V9G,#L**PEI9BAB86YK("$](#`I M('L**PD)<')I;G1F*")<;B5S.B!O;FQY(&)A;FL@,"!S=7!P;W)T960B+"`@ M7U]&54Y#5$E/3E]?*3L**PE]"B`*(`DO*@H@"2`J(&=E="!-96UO<GD@0V]N M=')O;&QE<B!/<'1I;VYS(#`@9&%T80H@"2`J+PHM"6UF<V1R86TH;65M7V-F M9S`L(&-F9S`I.PHM"BL);69S9')A;3`H;65M7VUC;W!T,2P@8V9G,"D["BL) M"BL)8V9G,"`F/2!^4T1204TP7T-&1U]%34153%(@)B!^4T1204TP7T-&1U]- M14U#2$L["BLC:69D968@1$5"54<**PEP<FEN=&8H(B5S.B!L96YG=&@@,'@E M>"!B>71E<R!B86YK7V)A<V5?861D<B`E<%QN(BP@(%]?1E5.0U1)3TY?7RP@ M(&YU;5]B>71E<RP@8F%N:U]B87-E7V%D9'(I.PHK"7!R:6YT9B@B)7,Z(&-F M9S`@9&ES86)L92!C:&5C:VEN9R`M/B`P>"4P.'A<;B(L("!?7T953D-424]. M7U\L("!C9F<P*3L**R-E;F1I9@H@"2\J"B`)("H@<F5S970@=&AE(&)A;FM? M8F%S92!A9&1R97-S"B`)("HO"BT)8F%N:U]B87-E7V%D9'(@/2!#1D=?4T12 M04U?0D%313L**PEM='-D<F%M,"AM96U?96-C8V8L("`P*3L@("`@("`@("`@ M+RH@9&ES86)L92!C;W)R96-T:6]N("HO"BL);71S9')A;3`H;65M7V5C8V5R M<BP@4T1204U?14-#15-27T524D]27TU!4TLI.R`O*B!#;&5A<B!A;&P@97)R M;W)S("HO"BL);71S9')A;3`H;65M7VUC;W!T,2P@8V9G,"D["B`*+0EI9B`H M*&-F9S`@)B!31%)!35]#1D<P7TU#2$M?34%32RD@(3T@4T1204U?0T9',%]- M0TA+7TY/3BD@>PHM"0EM='-D<F%M*&UE;5]C9F<P+"`H8V9G,"`F('Y31%)! M35]#1D<P7TU#2$M?34%32RD@?`HM"0D)4T1204U?0T9',%]-0TA+7T=%3BD[ M"BL)861D<F5S<U]I;F-R96UE;G0@/2`T.PHK"6-U<G)E;G1?861D<F5S<R`] M("AU;G-I9VYE9"!L;VYG*2AB86YK7V)A<V5?861D<BD["BL)96YD7V%D9')E M<W,@/2`H=6YS:6=N960@;&]N9RDH8F%N:U]B87-E7V%D9'(I("L@;G5M7V)Y M=&5S.PH@"BT)"6EF("@H8V9G,"`F(%-$4D%-7T-&1S!?1$U71%]-05-+*2`] M/2!31%)!35]#1D<P7T1-5T1?,S(I('L*+0D)"6%D9')E<W-?:6YC<F5M96YT M(#T@-#L*+0D)?2!E;'-E('L*+0D)"6%D9')E<W-?:6YC<F5M96YT(#T@.#L* M+0D)?0HK"7=H:6QE("AC=7)R96YT7V%D9')E<W,@/"!E;F1?861D<F5S<RD@ M>PHK"0DJ*"AU;G-I9VYE9"!L;VYG*BEC=7)R96YT7V%D9')E<W,I(#T@,#L* M*PD)8W5R<F5N=%]A9&1R97-S("L](&%D9')E<W-?:6YC<F5M96YT.PHK"7T* M(`HM"0EC=7)R96YT7V%D9')E<W,@/2`H=6YS:6=N960@;&]N9RDH8F%N:U]B M87-E7V%D9'(I.PHM"0EE;F1?861D<F5S<R`]("AU;G-I9VYE9"!L;VYG*2AB M86YK7V)A<V5?861D<BD@*R!N=6U?8GET97,["BL);71S9')A;3`H;65M7V5C M8V5R<BP@4T1204U?14-#15-27T524D]27TU!4TLI.R`O*B!#;&5A<B!A;&P@ M97)R;W)S("HO"B`*+0D)=VAI;&4@*&-U<G)E;G1?861D<F5S<R`(&5N9%]A M9&1R97-S*2!["BT)"0DJ*"AU;G-I9VYE9"!L;VYG*BEC=7)R96YT7V%D9')E M<W,I(#T@,'@P,#`P,#`P,#L*+0D)"6-U<G)E;G1?861D<F5S<R`K/2!A9&1R M97-S7VEN8W)E;65N=#L*+0D)?0HK(VEF9&5F($1%0E5'"BL)<')I;G1F*"(E M<SH@8V9G,"!E;F%B;&4@8VAE8VMI;F=<;B(L("!?7T953D-424].7U\I.PHK M(V5N9&EF"BL);71S9')A;3`H;65M7V5C8V-F+"!31%)!35]%0T-#1D=?0T4P M*3L@("`@("`@("`O*B!E;F%B;&4@8V]R<F5C=&EO;B`J+PHK"7!R:6YT9B@B M14-#("(I.PH@"BT)"6UT<V1R86TH;65M7V-F9S`L("AC9F<P("8@?E-$4D%- M7T-&1S!?34-(2U]-05-+*2!"BT)"0E31%)!35]#1D<P7TU#2$M?0TA+*3L* M*R-I9F1E9B!$14)51PHK"7L@+RH@02!S;6%L;"!S86YI='D@8VAE8VL@*B* M*PD)=6YS:6=N960@;&]N9R`J8VAE8VL["BL)"6-H96-K/2`H=6YS:6=N960@ M;&]N9R`J*6)A;FM?8F%S95]A9&1R.PHK"0DJ8VAE8VL]14-#7U1%4U1?5D%, M544["BL)"6EF*"IC:&5C:R`A/2!%0T-?5$535%]604Q512D**PD)"7!R:6YT M9B@B)7,Z(&-H96-K:6YG(&%T("5P(&ES(#!X)7@@9F%I;&5D7&XB+"`@7U]& M54Y#5$E/3E]?+"!C:&5C:RP@*F-H96-K*3L*(`E]"BLC96YD:68)"B!]"B`* M+2-E;F1I9B`O*B!#3TY&24=?-#0P("HO"BT*("-E;F1I9B`O*B!#3TY&24=? ..4U!$7T5%4%)/32`J+PH` ` end
Hi Niklaus,
On Monday 12 February 2007 19:13, Niklaus Giger wrote:
Attached you find a patch which enables ECC for our board.
Sorry, can't decode the attachment. Could you please send it inline? Thanks.
At the moment it is pretty PPC405GPr specific, as I have no other board around to test. I have also a Yosemite and a Sequoia, but they do not seem to have a correct SPD-info.
The 440EP (Yosemite) has a DDR controller and the 440EPx (Sequoia) has a total different DDR2 controller. And both boards you mentioned are not equipped with DIMM modules and therefore don't have an SPD EEPROM.
I will submit a patch once I have our PPC440EPx board up and running.
How far are with your 440EPx porting?
What flash devices do you use on your board?
I have a 8 bit boot-eprom AMD AM29LV040B and 8,16 or 32 MB of CFI-Flash (one 16 bit-wide bank), e.g. Intel 28F028J3A
Autsch! I really don't like such braindead designs (8bit _and_ 16bit flash on one EBC). Makes software really complicated. Please don't do this on your 440EPx design. ;-)
BTW: Sorry, but I the review of your patch will take a little longer, since we are on the Embedded World fair in Nuerberg this week.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk Office: Kirchenstr. 5, D-82194 Groebenzell, Germany =====================================================================
Stefan Roesesr@denx.de 12.02.07 20:25:24 >>>
Hi Niklaus,
On Monday 12 February 2007 19:13, Niklaus Giger wrote:
Attached you find a patch which enables ECC for our board.
Sorry, can't decode the attachment. Could you please send it inline? Thanks.
Here it is:
diff --git a/cpu/ppc4xx/spd_sdram.c b/cpu/ppc4xx/spd_sdram.c old mode 100644 new mode 100755 index c24456b..ca922d4 --- a/cpu/ppc4xx/spd_sdram.c +++ b/cpu/ppc4xx/spd_sdram.c @@ -42,6 +42,7 @@ #include <common.h> #include <asm/processor.h> #include <i2c.h> #include <ppc4xx.h> +#include <asm/io.h>
#ifdef CONFIG_SPD_EEPROM
@@ -98,6 +99,8 @@ #else # define SPD_ERR(x) do { printf(x); return(0); } while (0) #endif
+void program_ecc (void *bank_base_addr, unsigned long num_bytes, int bank); + #define sdram_HZ_to_ns(hertz) (1000000000/(hertz))
/* function prototypes */ @@ -304,6 +307,10 @@ #endif sdram0_ecccfg = 0xf << SDRAM0_ECCCFG_SHIFT; ecc_on = 1; } else { +#if defined(CONFIG_ECC) && defined(DEBUG) + printf("%s: no ECC as spd 11: %d 6: %d 14: %d\n", __FUNCTION__, + read_spd(11), read_spd(6), read_spd(14)); +#endif sdram0_ecccfg = 0; ecc_on = 0; } @@ -424,7 +431,8 @@ #endif * program all the registers. * -------------------------------------------------------------------*/
-#define mtsdram0(reg, data) mtdcr(memcfga,reg);mtdcr(memcfgd,data) +#define mtsdram0(reg, data) mtdcr(memcfga,reg);mtdcr(memcfgd,data) +#define mfsdram0(reg, data) { mtdcr(memcfga,reg);data = mfdcr(memcfgd); } /* disable memcontroller so updates work */ mtsdram0( mem_mcopt1, 0 );
@@ -447,10 +455,10 @@ #endif /* SDRAM have a power on delay, 500 micro should do */ udelay(500); sdram0_cfg = SDRAM0_CFG_DCE | SDRAM0_CFG_BRPF(1) | SDRAM0_CFG_ECCDD | SDRAM0_CFG_EMDULR; - if (ecc_on) - sdram0_cfg |= SDRAM0_CFG_MEMCHK; mtsdram0(mem_mcopt1, sdram0_cfg); - +#ifdef CONFIG_ECC + if (ecc_on) program_ecc(0, total_size, 0); +#endif return (total_size); }
@@ -696,8 +704,6 @@ void program_tr0 (unsigned long* dimm_po
void program_tr1 (void);
-void program_ecc (unsigned long num_bytes); - unsigned long program_bxcr(unsigned long* dimm_populated, unsigned char* iic0_dimm_addr, @@ -1785,47 +1791,76 @@ #endif return(bank_base_addr); }
-void program_ecc (unsigned long num_bytes) +#endif /* CONFIG_440 */ + +#define SDRAM_ECCCFG_CE0 0x00800000 /* ECC Correction Enable for Bank 0 */ +#define SDRAM_ECCCFG_CE1 0x00400000 /* ECC Correction Enable for Bank 1 */ +#define SDRAM_ECCCFG_CE2 0x00200000 /* ECC Correction Enable for Bank 2 */ +#define SDRAM_ECCCFG_CE3 0x00100000 /* ECC Correction Enable for Bank 3 */ + +#define SDRAM_ECCESR_ERROR_MASK 0xFFF0F000 /* All possible ECC errors */ +#define ECC_TEST_VALUE 0xaffeaffe + +/* + * Prepare for ECC operation + * Step 1: Enable ECC generation but not checks + * Step 2: Fill all memory + * Step 3: Enable ECC generation and checks + * Only programmed for and tested on a PPC405GPr board using only bank 0 and 32 bit wide !!! + */ +void program_ecc (void *bank_base_addr, unsigned long num_bytes, int bank) { - unsigned long bank_base_addr; unsigned long current_address; unsigned long end_address; unsigned long address_increment; unsigned long cfg0; + if(bank != 0) { + printf("\n%s: only bank 0 supported", __FUNCTION__); + }
/* * get Memory Controller Options 0 data */ - mfsdram(mem_cfg0, cfg0); - + mfsdram0(mem_mcopt1, cfg0); + + cfg0 &= ~SDRAM0_CFG_EMDULR & ~SDRAM0_CFG_MEMCHK; +#ifdef DEBUG + printf("%s: length 0x%x bytes bank_base_addr %p\n", __FUNCTION__, num_bytes, bank_base_addr); + printf("%s: cfg0 disable checking -> 0x%08x\n", __FUNCTION__, cfg0); +#endif /* * reset the bank_base address */ - bank_base_addr = CFG_SDRAM_BASE; + mtsdram0(mem_ecccf, 0); /* disable correction */ + mtsdram0(mem_eccerr, SDRAM_ECCESR_ERROR_MASK); /* Clear all errors */ + mtsdram0(mem_mcopt1, cfg0);
- if ((cfg0 & SDRAM_CFG0_MCHK_MASK) != SDRAM_CFG0_MCHK_NON) { - mtsdram(mem_cfg0, (cfg0 & ~SDRAM_CFG0_MCHK_MASK) | - SDRAM_CFG0_MCHK_GEN); + address_increment = 4; + current_address = (unsigned long)(bank_base_addr); + end_address = (unsigned long)(bank_base_addr) + num_bytes;
- if ((cfg0 & SDRAM_CFG0_DMWD_MASK) == SDRAM_CFG0_DMWD_32) { - address_increment = 4; - } else { - address_increment = 8; - } + while (current_address < end_address) { + *((unsigned long*)current_address) = 0; + current_address += address_increment; + }
- current_address = (unsigned long)(bank_base_addr); - end_address = (unsigned long)(bank_base_addr) + num_bytes; + mtsdram0(mem_eccerr, SDRAM_ECCESR_ERROR_MASK); /* Clear all errors */
- while (current_address < end_address) { - *((unsigned long*)current_address) = 0x00000000; - current_address += address_increment; - } +#ifdef DEBUG + printf("%s: cfg0 enable checking\n", __FUNCTION__); +#endif + mtsdram0(mem_ecccf, SDRAM_ECCCFG_CE0); /* enable correction */ + printf("ECC ");
- mtsdram(mem_cfg0, (cfg0 & ~SDRAM_CFG0_MCHK_MASK) | - SDRAM_CFG0_MCHK_CHK); +#ifdef DEBUG + { /* A small sanity check */ + unsigned long *check; + check= (unsigned long *)bank_base_addr; + *check=ECC_TEST_VALUE; + if(*check != ECC_TEST_VALUE) + printf("%s: checking at %p is 0x%x failed\n", __FUNCTION__, check, *check); } +#endif }
-#endif /* CONFIG_440 */ - #endif /* CONFIG_SPD_EEPROM */
<..>
How far are with your 440EPx porting?
It is basically a copy of the sequoia minus NAND and NOR chips, as we will only have a Boot-Flash which will fetch everything into RAM from our human interface board. Layout is under way, first HW will come late in March or sometimes in April.
What flash devices do you use on your board?
I have a 8 bit boot-eprom AMD AM29LV040B and 8,16 or 32 MB of CFI-Flash (one 16 bit-wide bank), e.g. Intel 28F028J3A
Autsch! I really don't like such braindead designs (8bit _and_ 16bit flash on one EBC). Makes software really complicated. Please don't do this on your 440EPx design. ;-)
As stated above we only will have 8 bit boot-eprom AMD AM29LV040B. And I do not care if our old board has no support for the CFI.
BTW: Sorry, but I the review of your patch will take a little longer, since we are on the Embedded World fair in Nuerberg this week.
No problem. I can wait.
Best regards
Niklaus
Hi Niklaus,
On Wednesday 14 February 2007 18:10, Niklaus Giger wrote:
Here it is:
diff --git a/cpu/ppc4xx/spd_sdram.c b/cpu/ppc4xx/spd_sdram.c
Could you please generate a new patch against the current git repository? I reorganized the SPD files. "40x_spd_sdram.c" is now the file you want.
Please see some comments already below.
old mode 100644 new mode 100755 index c24456b..ca922d4 --- a/cpu/ppc4xx/spd_sdram.c +++ b/cpu/ppc4xx/spd_sdram.c @@ -42,6 +42,7 @@ #include <common.h> #include <asm/processor.h> #include <i2c.h> #include <ppc4xx.h> +#include <asm/io.h>
#ifdef CONFIG_SPD_EEPROM
@@ -98,6 +99,8 @@ #else # define SPD_ERR(x) do { printf(x); return(0); } while (0) #endif
+void program_ecc (void *bank_base_addr, unsigned long num_bytes, int bank); +
Line wrapped. Please make sure next time, that your mail client doesn't wrap the lines.
#define sdram_HZ_to_ns(hertz) (1000000000/(hertz))
/* function prototypes */ @@ -304,6 +307,10 @@ #endif sdram0_ecccfg = 0xf << SDRAM0_ECCCFG_SHIFT; ecc_on = 1; } else { +#if defined(CONFIG_ECC) && defined(DEBUG)
printf("%s: no ECC as spd 11: %d 6: %d 14: %d\n", __FUNCTION__,read_spd(11), read_spd(6), read_spd(14));+#endif
Better would be to use the debug() macro here:
#if defined(CONFIG_ECC) debug("%s: no ECC as spd 11: %d 6: %d 14: %d\n", __FUNCTION__, read_spd(11), read_spd(6), read_spd(14)); #endif
sdram0_ecccfg = 0; ecc_on = 0;} @@ -424,7 +431,8 @@ #endif * program all the registers. * -------------------------------------------------------------------*/
-#define mtsdram0(reg, data) mtdcr(memcfga,reg);mtdcr(memcfgd,data) +#define mtsdram0(reg, data) mtdcr(memcfga,reg);mtdcr(memcfgd,data) +#define mfsdram0(reg, data) { mtdcr(memcfga,reg);data = mfdcr(memcfgd); } /* disable memcontroller so updates work */ mtsdram0( mem_mcopt1, 0 );
@@ -447,10 +455,10 @@ #endif /* SDRAM have a power on delay, 500 micro should do */ udelay(500); sdram0_cfg = SDRAM0_CFG_DCE | SDRAM0_CFG_BRPF(1) | SDRAM0_CFG_ECCDD | SDRAM0_CFG_EMDULR;
- if (ecc_on)
sdram0_cfg |= SDRAM0_CFG_MEMCHK;+#ifdef CONFIG_ECC
- if (ecc_on) program_ecc(0, total_size, 0);
+#endif
New line after the if () statement please.
return (total_size); }
@@ -696,8 +704,6 @@ void program_tr0 (unsigned long* dimm_po
void program_tr1 (void);
-void program_ecc (unsigned long num_bytes);
unsigned long program_bxcr(unsigned long* dimm_populated, unsigned char* iic0_dimm_addr, @@ -1785,47 +1791,76 @@ #endif return(bank_base_addr); }
-void program_ecc (unsigned long num_bytes) +#endif /* CONFIG_440 */
+#define SDRAM_ECCCFG_CE0 0x00800000 /* ECC Correction Enable for Bank 0 */ +#define SDRAM_ECCCFG_CE1 0x00400000 /* ECC Correction Enable for Bank 1 */ +#define SDRAM_ECCCFG_CE2 0x00200000 /* ECC Correction Enable for Bank 2 */ +#define SDRAM_ECCCFG_CE3 0x00100000 /* ECC Correction Enable for Bank 3 */ + +#define SDRAM_ECCESR_ERROR_MASK 0xFFF0F000 /* All possible ECC errors */ +#define ECC_TEST_VALUE 0xaffeaffe
+/*
- Prepare for ECC operation
- Step 1: Enable ECC generation but not checks
- Step 2: Fill all memory
- Step 3: Enable ECC generation and checks
- Only programmed for and tested on a PPC405GPr board using only bank 0
and 32 bit wide !!!
- */
+void program_ecc (void *bank_base_addr, unsigned long num_bytes, int bank) {
- unsigned long bank_base_addr; unsigned long current_address; unsigned long end_address; unsigned long address_increment; unsigned long cfg0;
- if(bank != 0) {
printf("\n%s: only bank 0 supported", __FUNCTION__);- }
No parentheses needed here. And a space after the "if".
/* * get Memory Controller Options 0 data */
- mfsdram(mem_cfg0, cfg0);
- mfsdram0(mem_mcopt1, cfg0);
- cfg0 &= ~SDRAM0_CFG_EMDULR & ~SDRAM0_CFG_MEMCHK;
+#ifdef DEBUG
- printf("%s: length 0x%x bytes bank_base_addr %p\n", __FUNCTION__,
num_bytes, bank_base_addr);
- printf("%s: cfg0 disable checking -> 0x%08x\n", __FUNCTION__, cfg0);
+#endif
debug()
/* * reset the bank_base address */
- bank_base_addr = CFG_SDRAM_BASE;
- mtsdram0(mem_ecccf, 0); /* disable correction */
- mtsdram0(mem_eccerr, SDRAM_ECCESR_ERROR_MASK); /* Clear all errors */
- mtsdram0(mem_mcopt1, cfg0);
- if ((cfg0 & SDRAM_CFG0_MCHK_MASK) != SDRAM_CFG0_MCHK_NON) {
mtsdram(mem_cfg0, (cfg0 & ~SDRAM_CFG0_MCHK_MASK) |SDRAM_CFG0_MCHK_GEN);
- address_increment = 4;
- current_address = (unsigned long)(bank_base_addr);
- end_address = (unsigned long)(bank_base_addr) + num_bytes;
if ((cfg0 & SDRAM_CFG0_DMWD_MASK) == SDRAM_CFG0_DMWD_32) {address_increment = 4;} else {address_increment = 8;}
- while (current_address < end_address) {
*((unsigned long*)current_address) = 0;current_address += address_increment;- }
current_address = (unsigned long)(bank_base_addr);end_address = (unsigned long)(bank_base_addr) + num_bytes;
- mtsdram0(mem_eccerr, SDRAM_ECCESR_ERROR_MASK); /* Clear all errors */
while (current_address < end_address) {*((unsigned long*)current_address) = 0x00000000;current_address += address_increment;}+#ifdef DEBUG
- printf("%s: cfg0 enable checking\n", __FUNCTION__);
+#endif
- mtsdram0(mem_ecccf, SDRAM_ECCCFG_CE0); /* enable correction */
- printf("ECC ");
mtsdram(mem_cfg0, (cfg0 & ~SDRAM_CFG0_MCHK_MASK) |SDRAM_CFG0_MCHK_CHK);+#ifdef DEBUG
- { /* A small sanity check */
unsigned long *check;check= (unsigned long *)bank_base_addr;*check=ECC_TEST_VALUE;if(*check != ECC_TEST_VALUE)printf("%s: checking at %p is 0x%x failed\n", __FUNCTION__, check,*check);
Coding style.
I will review the next version more thoroughly. And faster, I promise.
Sorry again for the delay.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk Office: Kirchenstr. 5, D-82194 Groebenzell, Germany =====================================================================
Hi Stefan
Here my reworked patch. Tried a follow all of your comments.
cpu/ppc4xx/40x_spd_sdram.c | 86 ++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 83 insertions(+), 3 deletions(-)
diff --git a/cpu/ppc4xx/40x_spd_sdram.c b/cpu/ppc4xx/40x_spd_sdram.c index 19c4f76..f1e9b38 100644 --- a/cpu/ppc4xx/40x_spd_sdram.c +++ b/cpu/ppc4xx/40x_spd_sdram.c @@ -104,6 +104,7 @@
/* function prototypes */ int spd_read(uint addr); +void program_ecc (void *bank_base_addr, unsigned long num_bytes, int bank);
/* @@ -306,6 +307,10 @@ long int spd_sdram(int(read_spd)(uint addr)) sdram0_ecccfg = 0xf << SDRAM0_ECCCFG_SHIFT; ecc_on = 1; } else { +#if defined(CONFIG_ECC) + debug("%s: no ECC as spd 11: %d 6: %d 14: %d\n", __FUNCTION__, + read_spd(11), read_spd(6), read_spd(14)); +#endif sdram0_ecccfg = 0; ecc_on = 0; } @@ -426,7 +431,9 @@ long int spd_sdram(int(read_spd)(uint addr)) * program all the registers. * -------------------------------------------------------------------*/
-#define mtsdram0(reg, data) mtdcr(memcfga,reg);mtdcr(memcfgd,data) +#define mfsdram0(reg, data) { mtdcr(memcfga,reg);data = mfdcr(memcfgd); } +#define mtsdram0(reg, data) mtdcr(memcfga,reg);mtdcr(memcfgd,data) + /* disable memcontroller so updates work */ mtsdram0( mem_mcopt1, 0 );
@@ -449,9 +456,11 @@ long int spd_sdram(int(read_spd)(uint addr)) /* SDRAM have a power on delay, 500 micro should do */ udelay(500); sdram0_cfg = SDRAM0_CFG_DCE | SDRAM0_CFG_BRPF(1) | SDRAM0_CFG_ECCDD | SDRAM0_CFG_EMDULR; - if (ecc_on) - sdram0_cfg |= SDRAM0_CFG_MEMCHK; mtsdram0(mem_mcopt1, sdram0_cfg); +#ifdef CONFIG_ECC + if (ecc_on) + program_ecc(0, total_size, 0); +#endif
return (total_size); } @@ -466,4 +475,75 @@ int spd_read(uint addr) return 0; }
+#define SDRAM_ECCCFG_CE0 0x00800000 /* ECC Correction Enable for Bank 0 */ +#define SDRAM_ECCCFG_CE1 0x00400000 /* ECC Correction Enable for Bank 1 */ +#define SDRAM_ECCCFG_CE2 0x00200000 /* ECC Correction Enable for Bank 2 */ +#define SDRAM_ECCCFG_CE3 0x00100000 /* ECC Correction Enable for Bank 3 */ + +#define SDRAM_ECCESR_ERROR_MASK 0xFFF0F000 /* All possible ECC errors */ +#define ECC_TEST_VALUE 0xaffeaffe + +/* + * Prepare for ECC operation + * Step 1: Enable ECC generation but not checks + * Step 2: Fill all memory + * Step 3: Enable ECC generation and checks + * Only programmed for and tested on a PPC405GPr board using: + * bank 0 and 32 bit wide !!! + */ +void program_ecc (void *bank_base_addr, unsigned long num_bytes, int bank) +{ + unsigned long current_address; + unsigned long end_address; + unsigned long address_increment; + unsigned long cfg0; + if (bank != 0) + { + printf("\n%s: only bank 0 supported", __FUNCTION__); + return; + } + + /* + * get Memory Controller Options 0 data + */ + mfsdram0(mem_mcopt1, cfg0); + + cfg0 &= ~SDRAM0_CFG_EMDULR & ~SDRAM0_CFG_MEMCHK; + debug("%s: length 0x%x bytes bank_base_addr %p\n", __FUNCTION__, + num_bytes, bank_base_addr); + debug("%s: cfg0 disable checking -> 0x%08x\n", __FUNCTION__, cfg0); + /* + * reset the bank_base address + */ + mtsdram0(mem_ecccf, 0); /* disable correction */ + mtsdram0(mem_eccerr, SDRAM_ECCESR_ERROR_MASK); /* Clear all errors */ + mtsdram0(mem_mcopt1, cfg0); + + address_increment = 4; + current_address = (unsigned long)(bank_base_addr); + end_address = (unsigned long)(bank_base_addr) + num_bytes; + + while (current_address < end_address) { + *((unsigned long*)current_address) = 0; + current_address += address_increment; + } + + mtsdram0(mem_eccerr, SDRAM_ECCESR_ERROR_MASK); /* Clear all errors */ + + debug("%s: cfg0 enable checking\n", __FUNCTION__); + mtsdram0(mem_ecccf, SDRAM_ECCCFG_CE0); /* enable correction */ + printf("ECC "); + +#ifdef DEBUG + { /* A small sanity check */ + unsigned long *check; + check= (unsigned long *)bank_base_addr; + *check=ECC_TEST_VALUE; + if (*check != ECC_TEST_VALUE) + debug("%s: checking at %p is 0x%x failed\n", + __FUNCTION__, check, *check); + } +#endif +} + #endif /* CONFIG_SPD_EEPROM */
Best regards
Stefan Roese wrote:
Hi Niklaus,
On Wednesday 14 February 2007 18:10, Niklaus Giger wrote:
Here it is:
diff --git a/cpu/ppc4xx/spd_sdram.c b/cpu/ppc4xx/spd_sdram.c
Could you please generate a new patch against the current git repository? I reorganized the SPD files. "40x_spd_sdram.c" is now the file you want.
Hi Niklaus,
On Friday 06 April 2007 18:16, Niklaus Giger wrote:
Here my reworked patch. Tried a follow all of your comments.
Thanks. Just wanted to let you know that I have seen your patches and will review them directly after the Easter vacation.
Viele Grüße, Stefan
===================================================================== DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk Office: Kirchenstr. 5, D-82194 Groebenzell, Germany =====================================================================
Dear Niklaus,
in message ev5roc$qoo$1@sea.gmane.org you wrote:
+#define mtsdram0(reg, data) mtdcr(memcfga,reg);mtdcr(memcfgd,data)
This is bad style. Any such multi-statement macro definition should be encapsulated in a "do { ... } while (0)" frame.
if (bank != 0){printf("\n%s: only bank 0 supported", __FUNCTION__);return;}
Bad brace style.
Indentation not by TAB.
Please clean up!
Best regards,
Wolfgang Denk
Dear Niklaus,
in message ev5roc$qoo$1@sea.gmane.org you wrote:
Here my reworked patch. Tried a follow all of your comments.
There are some coding style violations: indentation not by TAB.
Please write comments in English language, thanks.
include/configs/hcu4.h is formatted in a way that makes it mostly unreadable. You may want to clean this up a bit.
You use a private flash driver for something that looks as if it was CFI compatible - why don't you use the CFI driver?
Best regards,
Wolfgang Denk
On Friday 06 April 2007 21:39, Wolfgang Denk wrote:
in message ev5roc$qoo$1@sea.gmane.org you wrote:
Here my reworked patch. Tried a follow all of your comments.
There are some coding style violations: indentation not by TAB.
Please write comments in English language, thanks.
include/configs/hcu4.h is formatted in a way that makes it mostly unreadable. You may want to clean this up a bit.
You use a private flash driver for something that looks as if it was CFI compatible - why don't you use the CFI driver?
This has been discussed before (I asked the same question). IIRC the board used an 8bit wide non CFI-compatible FLASH and an 16bit wide CFI compatible FLASH device. With the current CFI implementation this setup is not supported so a "custom" driver has to be used.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk Office: Kirchenstr. 5, D-82194 Groebenzell, Germany =====================================================================
Hi Niklaus,
please find some comments below:
On Friday 06 April 2007 18:16, Niklaus Giger wrote:
diff --git a/cpu/ppc4xx/40x_spd_sdram.c b/cpu/ppc4xx/40x_spd_sdram.c index 19c4f76..f1e9b38 100644 --- a/cpu/ppc4xx/40x_spd_sdram.c +++ b/cpu/ppc4xx/40x_spd_sdram.c @@ -104,6 +104,7 @@
/* function prototypes */ int spd_read(uint addr); +void program_ecc (void *bank_base_addr, unsigned long num_bytes, int bank);
/* @@ -306,6 +307,10 @@ long int spd_sdram(int(read_spd)(uint addr)) sdram0_ecccfg = 0xf << SDRAM0_ECCCFG_SHIFT; ecc_on = 1; } else { +#if defined(CONFIG_ECC)
debug("%s: no ECC as spd 11: %d 6: %d 14: %d\n", __FUNCTION__,read_spd(11), read_spd(6), read_spd(14));+#endif sdram0_ecccfg = 0; ecc_on = 0; } @@ -426,7 +431,9 @@ long int spd_sdram(int(read_spd)(uint addr)) * program all the registers. * -------------------------------------------------------------------*/
-#define mtsdram0(reg, data) mtdcr(memcfga,reg);mtdcr(memcfgd,data) +#define mfsdram0(reg, data) { mtdcr(memcfga,reg);data = mfdcr(memcfgd); } +#define mtsdram0(reg, data) mtdcr(memcfga,reg);mtdcr(memcfgd,data)
As Wolfgang already mentioned, this needs to be implemented differently. I know, the current implementation lacks this too. I suggest to remove these #defines from this file completely and move them to include/ppc405.h (as done in include/ppc440.h already):
/* * Macros for accessing the indirect SDRAM controller registers */ #define mtsdram(reg, data) do { mtdcr(memcfga,reg);mtdcr(memcfgd,data); } while (0) #define mfsdram(reg, data) do { mtdcr(memcfga,reg);data = mfdcr(memcfgd); } while (0)
And please change the name from "mtsdram0" to "mtsdram" so they match the defines done in include/ppc440.h.
/* disable memcontroller so updates work */ mtsdram0( mem_mcopt1, 0 );@@ -449,9 +456,11 @@ long int spd_sdram(int(read_spd)(uint addr)) /* SDRAM have a power on delay, 500 micro should do */ udelay(500); sdram0_cfg = SDRAM0_CFG_DCE | SDRAM0_CFG_BRPF(1) | SDRAM0_CFG_ECCDD | SDRAM0_CFG_EMDULR;
Your patch is line wrapped. Please make sure this doesn't happen with the next version.
if (ecc_on)sdram0_cfg |= SDRAM0_CFG_MEMCHK; mtsdram0(mem_mcopt1, sdram0_cfg);+#ifdef CONFIG_ECC
if (ecc_on)program_ecc(0, total_size, 0);+#endif
return (total_size);} @@ -466,4 +475,75 @@ int spd_read(uint addr) return 0; }
+#define SDRAM_ECCCFG_CE0 0x00800000 /* ECC Correction Enable for Bank 0 */ +#define SDRAM_ECCCFG_CE1 0x00400000 /* ECC Correction Enable for Bank 1 */ +#define SDRAM_ECCCFG_CE2 0x00200000 /* ECC Correction Enable for Bank 2 */ +#define SDRAM_ECCCFG_CE3 0x00100000 /* ECC Correction Enable for Bank 3 */
+#define SDRAM_ECCESR_ERROR_MASK 0xFFF0F000 /* All possible ECC errors */
Please move these defines to the include/ppc405.h header.
+#define ECC_TEST_VALUE 0xaffeaffe
+/*
- Prepare for ECC operation
- Step 1: Enable ECC generation but not checks
- Step 2: Fill all memory
- Step 3: Enable ECC generation and checks
- Only programmed for and tested on a PPC405GPr board using:
- bank 0 and 32 bit wide !!!
- */
+void program_ecc (void *bank_base_addr, unsigned long num_bytes, int bank) +{
unsigned long current_address;unsigned long end_address;unsigned long address_increment;unsigned long cfg0;
Please insert a blank line after the variable declarations.
if (bank != 0){printf("\n%s: only bank 0 supported", __FUNCTION__);return;}/** get Memory Controller Options 0 data*/mfsdram0(mem_mcopt1, cfg0);cfg0 &= ~SDRAM0_CFG_EMDULR & ~SDRAM0_CFG_MEMCHK;
I find this better readable:
cfg0 &= ~(SDRAM0_CFG_EMDULR | SDRAM0_CFG_MEMCHK);
debug("%s: length 0x%x bytes bank_base_addr %p\n", __FUNCTION__,num_bytes, bank_base_addr);debug("%s: cfg0 disable checking -> 0x%08x\n", __FUNCTION__, cfg0);/** reset the bank_base address*/mtsdram0(mem_ecccf, 0); /* disable correction */mtsdram0(mem_eccerr, SDRAM_ECCESR_ERROR_MASK); /* Clear all errors */mtsdram0(mem_mcopt1, cfg0);address_increment = 4;current_address = (unsigned long)(bank_base_addr);end_address = (unsigned long)(bank_base_addr) + num_bytes;while (current_address < end_address) {*((unsigned long*)current_address) = 0;current_address += address_increment;}mtsdram0(mem_eccerr, SDRAM_ECCESR_ERROR_MASK); /* Clear all errors */debug("%s: cfg0 enable checking\n", __FUNCTION__);mtsdram0(mem_ecccf, SDRAM_ECCCFG_CE0); /* enable correction */printf("ECC ");+#ifdef DEBUG
{ /* A small sanity check */unsigned long *check;check= (unsigned long *)bank_base_addr;*check=ECC_TEST_VALUE;
Spaces before and after the "=" please.
if (*check != ECC_TEST_VALUE)debug("%s: checking at %p is 0x%x failed\n",__FUNCTION__, check, *check);}+#endif +}
#endif /* CONFIG_SPD_EEPROM */
Best regards
Stefan Roese wrote:
Hi Niklaus,
On Wednesday 14 February 2007 18:10, Niklaus Giger wrote:
Here it is:
diff --git a/cpu/ppc4xx/spd_sdram.c b/cpu/ppc4xx/spd_sdram.c
Could you please generate a new patch against the current git repository? I reorganized the SPD files. "40x_spd_sdram.c" is now the file you want.
Hi Niklaus,
sorry for the delay. Could you please resubmit the HCU4 patch against the current top-of-git repository? I promise to review it now quickly.
Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk Office: Kirchenstr. 5, D-82194 Groebenzell, Germany =====================================================================
Hi Stefan
Here it is. bzipped because of the size limitation. I reworked it slightly and tried harder to follow the Linux kernel coding style.
I used the following commands for my workflow (Could not find a clear entry in the WiKi for a simple example tailored to the u-boot).
$ git clone http://source.denx.net/git/u-boot.git u-boot.git $ cd u-boot.git
Then I added the following lines to .git/config
[user] name = "Niklaus Giger" email = "niklaus.giger@netstal.com"
<added/edited some files> $ git add <for each new directory or file> Used "git status" to check whether I added all new files correcty. Used "git diff origin --check" to look for trailing/superfluous whitespaces. Used "git diff origin -p --summary origin" to preview my changes. Once I was satisfied, I preceeded with $ git-commit --all --signoff -m "Add first Netstal board HCU4: Second try" $ git-format-patch origin $ mv 0001-Add-first-Netstal-board-HCU4-Second-try.patch \ Add-first-Netstal-board-HCU4-Second-try.patch $ gzip Add-first-Netstal-board-HCU4-Second-try.patch
Is this a good workflow?
Stefan Roesesr@denx.de 04.04.07 17:40:00 >>>
Hi Niklaus,
sorry for the delay. Could you please resubmit the HCU4 patch against
the
current top-of-git repository? I promise to review it now quickly.
I will be absent from office the next two weeks, but will have a look at your comments sooner.
Best regards
Hi Niklaus,
On Friday 06 April 2007 18:22, Niklaus Giger wrote:
Here it is. bzipped because of the size limitation. I reworked it slightly and tried harder to follow the Linux kernel coding style.
Thanks. That's good. But unfortunately you had to gzip it because of the size limitation. This would not have been necessary if you had split your patch up into 2 or 3 mails. I would recommend something like:
[PATCH 1/3]: ppc4xx: Add Netstal common board directory [PATCH 2/3]: ppc4xx: Add Netstal HCU4 board support [PATCH 3/3]: ppc4xx: Add Netstal HCU4 board config file
In-line patches make my reviewing easier. Thanks.
I used the following commands for my workflow (Could not find a clear entry in the WiKi for a simple example tailored to the u-boot).
$ git clone http://source.denx.net/git/u-boot.git u-boot.git $ cd u-boot.git
Then I added the following lines to .git/config
[user] name = "Niklaus Giger" email = "niklaus.giger@netstal.com"
<added/edited some files> $ git add <for each new directory or file> Used "git status" to check whether I added all new files correcty. Used "git diff origin --check" to look for trailing/superfluous whitespaces. Used "git diff origin -p --summary origin" to preview my changes. Once I was satisfied, I preceeded with $ git-commit --all --signoff -m "Add first Netstal board HCU4: Second try"
Please add a little more descriptive commit message: Like which CPU is used.
$ git-format-patch origin $ mv 0001-Add-first-Netstal-board-HCU4-Second-try.patch \ Add-first-Netstal-board-HCU4-Second-try.patch $ gzip Add-first-Netstal-board-HCU4-Second-try.patch
Is this a good workflow?
I'm still using cogito, so I can't really comment on the core git usage.
Please clean up the issues Wolfgang addressed and resubmit. Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk Office: Kirchenstr. 5, D-82194 Groebenzell, Germany =====================================================================
participants (4)
-
Niklaus Giger -
Niklaus Giger -
Stefan Roese -
Wolfgang Denk