Re: [coreboot] [PATCH] Geode GX2 auto DRAM detect patch

[Nils]

***Ping***

It would be nice if someone finds the time to ack/commit or comment my patch.

On 10/7/10 , Nils wrote:
 This patch brings the adapted Geode LX auto DRAM detect
code  to GX2 ,
cleans up some files and adds a processor speed setting function in human 
readable Mhz.

Signed-off-by: Nils Jacobs <njacobs8(a)hetnet.nl>
Thanks, Nils

[PATCH] Geode GX2 auto DRAM detect patch

[Myles Watson]

      On Wed, Oct 13, 2010 at 2:51 PM, Nils <njacobs8(a)hetnet.nl> wrote:
 
 ***Ping***

 It would be nice if someone finds the time to ack/commit or comment my patch.
Your patch is rather long.  It would be easier to review if you split
out the white space & comment fixes, then the usage of msr names
instead of magic values.  It looks like this would make your patch
much smaller.  Another thing that would help would be if you used svn
cp so that it's obvious how you changed the lx code to adapt it for
the gx2.  When a patch looks like it has hundreds of lines of new
code, it's understandable that it could take longer to review.

+#define DIMM0 0xA0
+#define DIMM1 0xA2

-#include "northbridge/amd/gx2/raminit.h"
-
-	/* This is needed because ROMCC doesn`t now the ctz bitop */
-static inline unsigned int ctz(unsigned int n)
+static inline int spd_read_byte(unsigned int device, unsigned int address)
 {
-	int zeros;
+	if (device != DIMM0)
+		return 0xFF;	/* No DIMM1, don't even try. */

Why do you define DIMM1 to be 0xA2 (a reasonable value) if there is no
DIMM1?  Could we leave it undefined or define it to be something
obviously bogus?

Thanks,
Myles

[PATCH] Geode GX2 auto DRAM detect patch

[Nils]

      Hello Myles,
Thanks for the review.

Op donderdag 14 oktober 2010 18:53:43 schreef u:
  Your patch is rather long.  It would be easier to
review if you split
 out the white space & comment fixes, then the usage of msr names
 instead of magic values.  It looks like this would make your patch
 much smaller.  Another thing that would help would be if you used svn
 cp so that it's obvious how you changed the lx code to adapt it for
 the gx2.  When a patch looks like it has hundreds of lines of new
 code, it's understandable that it could take longer to review.
Sorry! I will try to do better next time.
 
  +#define DIMM0 0xA0
 +#define DIMM1 0xA2
 
 -#include "northbridge/amd/gx2/raminit.h"
 -
 -	/* This is needed because ROMCC doesn`t now the ctz bitop */
 -static inline unsigned int ctz(unsigned int n)
 +static inline int spd_read_byte(unsigned int device, unsigned int address)
  {
 -	int zeros;
 +	if (device != DIMM0)
 +		return 0xFF;	/* No DIMM1, don't even try. */
 
 Why do you define DIMM1 to be 0xA2 (a reasonable value) if there is no
 DIMM1?  Could we leave it undefined or define it to be something
 obviously bogus?
I am not the designer of that code i copied it from lippert/roadrunner-lx .
When i leave DIMM1 undefined i get following errors/warnings:

In file included from src/mainboard/wyse/s50/romstage.c:50:
src/northbridge/amd/gx2/raminit.c: In function ‘checkDDRMax’:
src/northbridge/amd/gx2/raminit.c:176: error: ‘DIMM1’ undeclared (first use in 
this function)
src/northbridge/amd/gx2/raminit.c:176: error: (Each undeclared identifier is 
reported only once
src/northbridge/amd/gx2/raminit.c:176: error: for each function it appears 
in.)
src/northbridge/amd/gx2/raminit.c: In function ‘set_refresh_rate’:
src/northbridge/amd/gx2/raminit.c:212: error: ‘DIMM1’ undeclared (first use in 
this function)
src/northbridge/amd/gx2/raminit.c: In function ‘setCAS’:
src/northbridge/amd/gx2/raminit.c:299: error: ‘DIMM1’ undeclared (first use in 
this function)
src/northbridge/amd/gx2/raminit.c: In function ‘set_latencies’:
src/northbridge/amd/gx2/raminit.c:335: error: ‘DIMM1’ undeclared (first use in 
this function)
src/northbridge/amd/gx2/raminit.c: In function ‘set_extended_mode_registers’:
src/northbridge/amd/gx2/raminit.c:431: error: ‘DIMM1’ undeclared (first use in 
this function)
src/northbridge/amd/gx2/raminit.c: In function ‘sdram_set_spd_registers’:
src/northbridge/amd/gx2/raminit.c:482: error: ‘DIMM1’ undeclared (first use in 
this function)
make: *** [build/mainboard/wyse/s50/romstage.pre.inc] Fout 1
n

Thanks,Nils.

[PATCH] Geode GX2 auto DRAM detect patch

[Myles Watson]

      On Thu, Oct 14, 2010 at 3:02 PM, Nils <njacobs8(a)hetnet.nl> wrote:
  Hello Myles,
 Thanks for the review.

 Op donderdag 14 oktober 2010 18:53:43 schreef u:
  Your patch is rather long.  It would be easier to
review if you split
 out the white space & comment fixes, then the usage of msr names
 instead of magic values.  It looks like this would make your patch
 much smaller.  Another thing that would help would be if you used svn
 cp so that it's obvious how you changed the lx code to adapt it for
 the gx2.  When a patch looks like it has hundreds of lines of new
 code, it's understandable that it could take longer to review.
 Sorry! I will try to do better next time.
No need to apologize.  I was just trying
to make it less frustrating.
The white space, comment fixes, and some of the other simple changes
could be reviewed by almost anyone.

 
> +#define DIMM0 0xA0
> +#define DIMM1 0xA2
  +     if
(device != DIMM0)
 +             return 0xFF;    /* No DIMM1, don't even try. */

 Why do you define DIMM1 to be 0xA2 (a reasonable value) if there is no
 DIMM1?  Could we leave it undefined or define it to be something
 obviously bogus?
 I am not the designer of that code i copied it from lippert/roadrunner-lx .
 When i leave DIMM1 undefined i get following errors/warnings:
Maybe we should
define it to be 0xFF.  In the future, I would think
that these values will move into Kconfig, and I think it will make
that conversion easier if we already know which values are bogus.

Thanks,
Myles

[PATCH] Geode GX2 auto DRAM detect patch

[Nils]

      Op donderdag 14 oktober 2010 23:23:39 schreef u:
  Maybe we should define it to be 0xFF.  In the future,
I would think
 that these values will move into Kconfig, and I think it will make
 that conversion easier if we already know which values are bogus.
That sounds reasonable and builds ok so its fine with me.
Should i redo the patch for that?

Thanks, nils.

[PATCH] Geode GX2 auto DRAM detect patch

[Myles Watson]

  Op donderdag 14 oktober 2010 23:23:39 schreef u:
  Maybe we should define it to be 0xFF.  In the
future, I would think
 that these values will move into Kconfig, and I think it will make
 that conversion easier if we already know which values are bogus.
 
 That sounds reasonable and builds ok so its fine with me.
 Should i redo the patch for that?
No.  I was just thinking that it would be an easy
change while you were
splitting the patch.  I'm not familiar enough with the Geode code to feel
comfortable committing it as is.

Thanks,
Myles

[PATCH] Geode GX2 auto DRAM detect patch

[ron minnich]

      I used to be familiar with that code and I find a 1600 line patch a
little daunting. Who's testing which board?

If a board can not be tested, I think it ought to be left alone or
removed. Otherwise you risk leaving a board in a broken state, which
someone else will discover at a later date.

ron

[PATCH] Geode GX2 auto DRAM detect patch

[Stefan Reinauer]

       On 10/14/10 3:12 PM, ron minnich wrote:
  I used to be familiar with that code and I find a 1600
line patch a
 little daunting. Who's testing which board?

 If a board can not be tested, I think it ought to be left alone or
 removed. Otherwise you risk leaving a board in a broken state, which
 someone else will discover at a later date.
Agreed. However if Nils can test this on some hardware and is confident
about it I think we should go for it.

Stefan

[PATCH] Geode GX2 auto DRAM detect patch

[Nils]

      Op vrijdag 15 oktober 2010 00:12:43 schreef u:
  I used to be familiar with that code and I find a 1600
line patch a
 little daunting. Who's testing which board?
 
 If a board can not be tested, I think it ought to be left alone or
 removed. Otherwise you risk leaving a board in a broken state, which
 someone else will discover at a later date.
 
 ron
 
As i stated in the original message i tested the patch on my Geode GX2 Wyse 
S50 board.
I didn`t test it on the Rumba and Frontrunner because i don`t have them and 
people who have them don`t seem to have time/interest to test.
But i don`t think that is very imported because both boards are broken in the 
current state for years.

As i began to test and patch the whole GX2 tree was a mess.
For instance the VSA code was non functional because Ron had committed a 
unfinished patchset in r4611 that was obviously not tested!

The Rumba target has a nonfunctional DRAM detection routine in romstage.c .(I 
tested it thats why i used an other one in S50 romstage)
It probably never worked since Ollie commited it in r2235 and that is probably 
the reason why the code was changed in the Frontrunner and later the Olpc 
targets to use hardcoded values (hack).
I think the Rumba target was abandoned at that time in favor for the 
Frontrunner board that got committed.

I don`t think the Frontrunner board works at the present state either.
I think i found a double bug in the cpureginit.c code that exists since the 
first commit in r2248 ! (The FooGlue setup should only be done for CS5535 
boards and uses a wrong address)
I am working on a patch for that.
And there are probably more bugs in the CS5535 code witch the Fronrunner uses 
that i can not test.

The PLL setup code in the romstage.c files is nonfunctional because it is 
hardcoded in pll_rst.c to 366Mhz.
The wrong PLL setup values that are still in Rumba and S50 (and were in Olpc) 
let the processor run on 100Mhz if enabled!

I think the GX2 code was never realy finished on any target and was/is a mess.
Because exept for my Wyse S50 i don`t think there is a working board.
(there is not even a free available working VSA blob for the current GX2 tree, 
i had to make it myself!)

So i think my patches are a big improvement and it is hard to "break" someones 
board with it.

I am not planning to do a lot of little patches and abuild and boot test all 
of them separately as that cost me a lot of extra time.
As i understand it only a few, for the real user unimportant, white space and 
comment change patches would get acked and committed and no one dares to ack 
the real patches.

I feel as if i was the only one cares about non functional code in the GX2 
tree.

Maybe Ron is right and all of the non functional boards (i think at least 90 
percent of coreboot) should be deleted.
Hardly anybody boot tests there big self acked and committed patches nowadays.
(i understand that is mostly impossable to do)

Sorry for the long rant.
I won`t bother you any longer.

Thanks,Nils.

[PATCH] Geode GX2 auto DRAM detect patch

[Myles Watson]

  I am not planning to do a lot of little patches and
abuild and boot test all
 of them separately as that cost me a lot of extra time.
Being part of a community certainly takes more of your time.  We hope
that the end product is better because of it.  When you spend more
time to make reviewing easy, more time will be spent reviewing your
code instead of searching through unimportant changes.

There would be no need to abuild test or boot test the split
patches...  they could still be committed at the same time.

  I won`t bother you any longer.
Submitting patches isn't a bother :)

Thanks,
Myles

[PATCH] Geode GX2 auto DRAM detect patch

[ron minnich]

      On Fri, Oct 15, 2010 at 7:10 AM, Nils <njacobs8(a)hetnet.nl> wrote:

  As i stated in the original message i tested the patch
on my Geode GX2 Wyse
 S50 board.
 I didn`t test it on the Rumba and Frontrunner because i don`t have them and
 people who have them don`t seem to have time/interest to test.
 But i don`t think that is very imported because both boards are broken in the
 current state for years.
yes, you are completely right. And those rumba boards at least should
be removed. You've done very good work and I don't think you should
have to worry about  nonexistent boards. You are also right that GX2
was/is a mess and your cleanup is quite a huge improvement.

  I think the Rumba target was abandoned at that time in
favor for the
 Frontrunner board that got committed.
They actually did work. But we did drop them once the real OLPC
hardware came along.

  The PLL setup code in the romstage.c files is
nonfunctional because it is
 hardcoded in pll_rst.c to 366Mhz.
 The wrong PLL setup values that are still in Rumba and S50 (and were in Olpc)
 let the processor run on 100Mhz if enabled!
There are a lot of cases where settings that were "wrong" were the
only ones that worked. What can I say?

  So i think my patches are a big improvement and it is
hard to "break" someones
 board with it.
Your patches are an enormous improvement. What I was trying to say is
that we should take your patches and remove boards that can not be
tested. We've got a lot of historical boards which nobody cares about,
so let's remove them.

  I am not planning to do a lot of little patches and
abuild and boot test all
 of them separately as that cost me a lot of extra time.
No one is asking you to. My feeling is that if there is no one around
to test a 6-year-old board (rumba) then maybe it is time to remove it.

  I won`t bother you any longer.
Well, I'm sorry if that happens, because your changes are very good stuff.

ron

[PATCH] Geode GX2 auto DRAM detect patch

[Uwe Hermann]

      On Fri, Oct 15, 2010 at 08:30:20AM -0700, ron minnich wrote:
  They actually did work.
Sound good. In that case we should definately keep them. At most, we
should _fix_ them, but definately not _remove_ them.


  No one is asking you to. My feeling is that if there
is no one around
 to test a 6-year-old board (rumba) then maybe it is time to remove it.
No, we should not remove it.


Uwe.
-- 
http://hermann-uwe.de     | http://sigrok.org
http://randomprojects.org | http://unmaintained-free-software.org

[PATCH] Geode GX2 auto DRAM detect patch

[Uwe Hermann]

      On Fri, Oct 15, 2010 at 04:10:26PM +0200, Nils wrote:
  As i stated in the original message i tested the patch
on my Geode GX2 Wyse 
 S50 board.
 I didn`t test it on the Rumba and Frontrunner because i don`t have them and 
 people who have them don`t seem to have time/interest to test.
  But i don`t think that is very imported because both
boards are broken in the 
 current state for years.
I agree. Testing on one of the GX2 boards is fully sufficient, nobody
can always test _all_ boards of a given chipset.

We're also routinely doing massive changes on the tree, some without
much hardware-testing at all, so your patch -- being boot-tested on
hardware -- is already in much better shape than many others we already
committed.

 
  I don`t think the Frontrunner board works at the
present state either.
 I think i found a double bug in the cpureginit.c code that exists since the 
 first commit in r2248 ! (The FooGlue setup should only be done for CS5535 
 boards and uses a wrong address)
 I am working on a patch for that.
Great, thanks!

 
  I think the GX2 code was never realy finished on any
target and was/is a mess.
Yep.


  Because exept for my Wyse S50 i don`t think there is a
working board.
 (there is not even a free available working VSA blob for the current GX2 tree, 
 i had to make it myself!)
Can you update any wiki instructions wrt to VSA which may be out of
date, and or shall we upload a known-good blob to the wiki?

 
  So i think my patches are a big improvement and it is
hard to "break" someones 
 board with it.
Yes, I fully agree.

 
  I am not planning to do a lot of little patches and
abuild and boot test all 
 of them separately as that cost me a lot of extra time.
 As i understand it only a few, for the real user unimportant, white space and 
 comment change patches would get acked and committed and no one dares to ack 
 the real patches.
Nah, we will review and commit this stuff, don't worry. But I agree with
Myles that mega-patches containing both whitespace changes and random
other functional changes are hard to review. Let us know if you can
split the stuff into, say, 2-3 distinct patches, one with whitespace
and coding style changes and 1-2 with the actual code changes.

If not, I'll try to separate at least parts of the whitespace stuff and
commit it, so the rest becomes more readable. Then we'll review that
and commit.


  Maybe Ron is right and all of the non functional
boards (i think at least 90 
 percent of coreboot) should be deleted.
No, definately not. I whole-heartedly disagree with any notions of
"let's drop this board, it wasn't tested recently" and I will object to
any patches in that direction.


Sorry it took so long to review your changes, but don't be discouraged,
we will definately merge your changes. Please let us know if you're going
to post updated/splitted patches or whether we should try to split
your patch.


Thanks, Uwe.
-- 
http://hermann-uwe.de     | http://sigrok.org
http://randomprojects.org | http://unmaintained-free-software.org

[PATCH] Geode GX2 auto DRAM detect patch

[Nils]

      I am happy to see there is still some interest in GX2 patches.
I will try to split the patch up in 2-3 patches.
Unfortunately i will probably have very little time for hobby next week so it 
can take a while.

Thanks, Nils.

[PATCH] Geode GX2 auto DRAM detect patch

[Nils]

      Hi Uwe,
It seems i forgot to answer this question from you:
  Can you update any wiki instructions wrt to VSA which
may be out of
 date, and or shall we upload a known-good blob to the wiki?
I think the wiki only
mentions the LX vsa witch is not usable for GX2 boards.
I rather not distribute my vsa at the moment because i don't know yet if it is 
completely working.
I used softvga in my vsa but it seems not to work yet.
If i don't get it working i might remove it again and i think it is not good 
to have different expirimental/broken vsa blobs on the internet floating around.

When i finish my GX2 updating i will send a patch to update gplvsa to make both 
LX and GX2 blobs and supply the working result blob.

Thanks, Nils.