[U-Boot-Users] [PATCH] Add first Netstal board HCU4

[U-Boot-Users] [PATCH] Add first Netstal board HCU4

[Niklaus Giger]

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>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-first-Netstal-board-HCU4.txt.gz
Type: application/x-gzip
Size: 15306 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070209/3d3a05d8/attachment.bin 

[U-Boot-Users] [PATCH] Add first Netstal board HCU4

[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?

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

[U-Boot-Users] [PATCH] Add first Netstal board HCU4

[Niklaus Giger]

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

-- 
Niklaus Giger

[U-Boot-Users] [PATCH] Add first Netstal board HCU4

[Stefan Roese]

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

[U-Boot-Users] Antw: Re: [PATCH] Add first Netstal board HCU4

[Niklaus Giger]

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 Roese<sr@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, at 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 at 9&\@>R!P<FEN=&8H>"D[(')E='5R;B at P*3L@?2!W
M:&EL92`H,"D*("-E;F1I9 at 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 at 4T1204TP7T5#0T-&1U]32$E&5#L*(`D)96-C7V]N(#T@,3L*
M(`E](&5L<V4@>PHK(VEF(&1E9FEN960H0T].1DE'7T5#0RD@)B8 at 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 at H@"2`J('!R;V=R86T at 86QL('1H
M92!R96=I<W1E<G,N"B`)("H at +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 at 4T1204T@:&%V92!A('!O=V5R(&]N(&1E;&%Y+`D@-3`P
M(&UI8W)O('-H;W5L9"!D;R`J+PH@"75D96QA>2 at U,#`I.PH@"7-D<F%M,%]C
M9F<@/2!31%)!33!?0T9'7T1#12!\(%-$4D%-,%]#1D=?0E)01B at 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 at 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 at +RH@0T].1DE'
M7S0T,"`J+PHK"BLC9&5F:6YE(%-$4D%-7T5#0T-&1U]#13`)"3!X,#`X,#`P
M,#`)+RH at 14-#($-O<G)E8W1I;VX at 16YA8FQE(&9O<B!"86YK(#`)*B\**R-D
M969I;F4 at 4T1204U?14-#0T9'7T-%,0D),'@P,#0P,#`P,`DO*B!%0T, at 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 at 14-#($-O<G)E8W1I;VX at 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 at 4')E<&%R92!F;W(@14-#(&]P97)A=&EO;@HK("H at 4W1E<"`Q.B!%;F%B
M;&4 at 14-#(&=E;F5R871I;VX at 8G5T(&YO="!C:&5C:W,**R`J(%-T97`@,CH@
M1FEL;"!A;&P@;65M;W)Y"BL@*B!3=&5P(#,Z($5N86)L92!%0T, at 9V5N97)A
M=&EO;B!A;F0 at 8VAE8VMS"BL@*B!/;FQY('!R;V=R86UM960 at 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 at 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 at 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 at 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 at 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 at 9&ES86)L92!C;W)R96-T:6]N("HO"BL);71S9')A;3`H;65M7V5C8V5R
M<BP at 4T1204U?14-#15-27T524D]27TU!4TLI.R`O*B!#;&5A<B!A;&P at 97)R
M;W)S("HO"BL);71S9')A;3`H;65M7VUC;W!T,2P at 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 at -#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 at 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 at 8V9G,"!E;F%B;&4 at 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 at 8V]R<F5C=&EO;B`J+PHK"7!R:6YT9B at 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 at +RH@02!S;6%L;"!S86YI='D at 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

[U-Boot-Users] Antw: Re: [PATCH] Add first Netstal board HCU4

[Stefan Roese]

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

[U-Boot-Users] Antw: Re: [PATCH] Add first Netstal board HCU4

[Niklaus Giger]

>>> Stefan Roese<sr@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@%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

[U-Boot-Users] Antw: Re: [PATCH] Add first Netstal board HCU4

[Stefan Roese]

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;
>  	mtsdram0(mem_mcopt1, sdram0_cfg);
> -
> +#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
=====================================================================

[U-Boot-Users] [PATCH] Add first Netstal board HCU4

[Stefan Roese]

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

[U-Boot-Users] Antw: Re: [PATCH] Add first Netstal board HCU4

[Niklaus Giger]

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

-- 
Niklaus Giger

[U-Boot-Users] [PATCH] Add first Netstal board HCU4

[Niklaus Giger]

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 at 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 Roese<sr@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

-- 
Niklaus Giger
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Add-first-Netstal-board-HCU4-Second-try.patch.gz
Type: application/x-gzip
Size: 15319 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070406/9494f8bf/attachment.bin 

[U-Boot-Users] Antw: Re: [PATCH] Add first Netstal board HCU4

[Stefan Roese]

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

[U-Boot-Users] Antw: Re: [PATCH] Add first Netstal board HCU4

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
All your people must learn before you can reach for the stars.
	-- Kirk, "The Gamesters of Triskelion", stardate 3259.2

[U-Boot-Users] Antw: Re: [PATCH] Add first Netstal board HCU4

[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

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Where shall I begin, please your Majesty?" he asked. "Begin  at  the
beginning,"  the  King said, gravely, "and go on till you come to the
end: then stop."    - Alice's Adventures in Wonderland, Lewis Carroll

[U-Boot-Users] Antw: Re: [PATCH] Add first Netstal board HCU4

[Stefan Roese]

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

[U-Boot-Users] Antw: Re: [PATCH] Add first Netstal board HCU4

[Stefan Roese]

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

[U-Boot-Users] [PATCH] Add first Netstal board HCU4

[Stefan Roese]

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 at 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
=====================================================================