Re: [coreboot] [PATCH] Geode GX2 auto DRAM detect patch
***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@hetnet.nl>
Thanks, Nils
On Wed, Oct 13, 2010 at 2:51 PM, Nils <njacobs8@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
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.
On Thu, Oct 14, 2010 at 3:02 PM, Nils <njacobs8@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
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.
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
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
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
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.
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
On Fri, Oct 15, 2010 at 7:10 AM, Nils <njacobs8@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
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
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
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.
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.
participants (5)
-
Myles Watson -
Nils -
ron minnich -
Stefan Reinauer -
Uwe Hermann