Hello.
Thanks to help from this list I could get my serial port to work with coreboot, and have been slowly progressing getting a little more log messages each time. I was keeping some questions to ask about what I'm doing that I pretended to send once I got coreboot to boot, in case I resolve them alone in the way, but It's getting longer than I thought and I thought I may ask now just in case I go too astray. Specially since skimming Qi's patch I don't see he needed all these changes for a similar board. I'm sending just the summary of the questions, I may send more details if you want or start different threads if that's best.
Details on the mainboard in my previous message: http://www.coreboot.org/pipermail/coreboot/2010-June/058666.html
1.- In order to get sb700_lpc_init in sb700_early_setup.c to work I've got to modifiy pci_locate_device in order to refrain from scanning some functions in pci bus 0. For most of the adresses without a device, the system hanged when trying to get the pci id (I thought it should have returned FFF... but it stopped cold there, apparently). Serialice does no id scan, it simply uses hardcoded device addresses. And I don't know what's the purpose of scanning for devices for my mainboard that early, I guess it's because sb700_lpc_init it's used in other places where it makes sense, but then I wonder whether my change is acceptable for other mainboards, or I should somehow configure some exclusion parameter so that it can still scan all devices if needed for some other card. Or maybe better I should hardcode de device addresses in sb700_lpc_init and don't call pci_locate_device. Is there a policy about hard coded bus, dev,fn, or pci_locate_device ?
2.- I'm using a Phenom II x4 910e which is revision RB_C3. I've got to add it to some constants like AMD_FAM10_ALL and the like for ht init and some cpu errata. I've also added some new errata and modified some existing one. (I've modified CPU name assignment too, I hope it works). While looking at the latests revisions of AMD revision guide and BIOS and Kernel Developer Guide (BKDG), I realised there are more CPU models than the ones identified by the constants in amddefs.h. In fact I think a 32 bit flag value is used and there are more new CPU revisions to add than bits left unused (2). So I haven't added the new revisions from the current docs, like I did for processor names, since my RB_C3 already had a constant. But maybe something should be done to foresee the new revisions ? Has people already used RB_C3 processors with coreboot? Should soem bits that are used in the same cases be folded into one, or should the value be extended to 64 bits or something ?
3.- It currently stops at fidvid setting, after starting other cores (stage2, I think, it's hard to read the log when they all write at once). Sometimes it continues until decompressing CBFS and stops when entering ram stage, but I'm trying to concentrate in the earliest point of failure. By browsing fidvid.c and the BKDG it seems it is not up to date, in particular apparently has fixed values for rev B CPUs, and I can't find some of the procedures required by current BKDG. I suspect some use of configuration data before setting it up, after power up, to calculate ramp or slam values for voltage transitions, and apparently it won't work with all voltages (maybe some cases can't happen?). In fact I can't seem to understand how to code some of the missing(?) procedures (like setting vid and fid for dual power plane mainboards, which amd docs say it's mainboard-specific and I can't find either hooks to use in fibvid.c to make it dependend on the mainboard or hints on what do I have to tell the voltage regulators in ASUS M4a77td-pro to comply with AMD docs). So I wonder whether I'm looking to unused code, or whether not implementing the whole BKDG is on purpose, or I'm just not understanding what I read.
So, in general, is all this normal, because I'm using hardware that hasn't been used before with coreboot, or I'm just lost in irrelevant details and everything should just work, so I should look elsewhere ?
Thanks.
Xavi Drudis Ferran wrote:
1.- In order to get sb700_lpc_init in sb700_early_setup.c to work I've got to modifiy pci_locate_device in order to refrain from scanning some functions in pci bus 0.
Hm. Which fn?
And I don't know what's the purpose of scanning for devices for my mainboard that early, I guess it's because sb700_lpc_init it's used in other places where it makes sense,
I don't know if it makes sense. That's not neccessarily the case. It depends on if the SB700 can have a different BDF on different boards.
but then I wonder whether my change is acceptable for other mainboards,
No, I don't think it is.
or I should somehow configure some exclusion parameter so that it can still scan all devices if needed for some other card.
This is also not so nice IMO.
Or maybe better I should hardcode de device addresses in sb700_lpc_init and don't call pci_locate_device. Is there a policy about hard coded bus, dev,fn, or pci_locate_device ?
In general nothing should be hard coded that is not hard coded in documentation. Ie. if BDF *can* change, then it can not be hard coded. However, the BDF should already be available from devicetree.cb, and hopefully that can be used also for early setup.
2.- I'm using a Phenom II x4 910e which is revision RB_C3. I've got to add it to some constants like AMD_FAM10_ALL and the like for ht init and some cpu errata. I've also added some new errata and modified some existing one. (I've modified CPU name assignment too, I hope it works).
Please send patches for all these types of changes that you have already done. Send many patches with one isolated change in every patch. The smaller each patch is, the easier it is to commit it and also review and comment on any things that should be improved.
While looking at the latests revisions of AMD revision guide and BIOS and Kernel Developer Guide (BKDG), I realised there are more CPU models than the ones identified by the constants in amddefs.h. In fact I think a 32 bit flag value is used and there are more new CPU revisions to add than bits left unused (2). So I haven't added the new revisions from the current docs, like I did for processor names, since my RB_C3 already had a constant. But maybe something should be done to foresee the new revisions ?
Certainly. I for one think that the good fix is to make a single long data structure which gathers that CPU revision information in one place.
Has people already used RB_C3 processors with coreboot?
I doubt it.
Should soem bits that are used in the same cases be folded into one, or should the value be extended to 64 bits or something ?
I like the idea of a data structure better. We're using C here so there is no reason to spread one piece of logical information out over multiple discrete variable types. A revision bitfield and an array of names doesn't make sense when there could be a structure with everything gathered in one place.
3.- It currently stops at fidvid setting, after starting other cores (stage2, I think, it's hard to read the log when they all write at once). Sometimes it continues until decompressing CBFS and stops when entering ram stage, but I'm trying to concentrate in the earliest point of failure.
This is very wise.
By browsing fidvid.c and the BKDG it seems it is not up to date, in particular apparently has fixed values for rev B CPUs, and I can't find some of the procedures required by current BKDG. I suspect some use of configuration data before setting it up, after power up, to calculate ramp or slam values for voltage transitions, and apparently it won't work with all voltages (maybe some cases can't happen?). In fact I can't seem to understand how to code some of the missing(?) procedures (like setting vid and fid for dual power plane mainboards, which amd docs say it's mainboard-specific and I can't find either hooks to use in fibvid.c to make it dependend on the mainboard or hints on what do I have to tell the voltage regulators in ASUS M4a77td-pro to comply with AMD docs). So I wonder whether I'm looking to unused code, or whether not implementing the whole BKDG is on purpose, or I'm just not understanding what I read.
I think the code just has not been developed yet. The existing code is in no way complete, and all your improvements are very much welcome.
or I'm just lost in irrelevant details
Certainly not, you've identified very important points where coreboot needs to be improved.
Thanks!
//Peter
Xavi Drudis Ferran wrote:
1.- In order to get sb700_lpc_init in sb700_early_setup.c to work I've got to modifiy pci_locate_device in order to refrain from scanning some functions in pci bus 0.
Hm. Which fn?
Sorry, I should have said devices. I think it scans every function of every device, reg. 00 and I told it not to do so for any function of some devices (those with nothing attached in my board). I'll send the patch later.
I don't know if it makes sense. That's not neccessarily the case. It depends on if the SB700 can have a different BDF on different boards.
I'd say no, but what do I know?
but then I wonder whether my change is acceptable for other mainboards,
No, I don't think it is.
Ok, then I'll change it.
or I should somehow configure some exclusion parameter so that it can still scan all devices if needed for some other card.
This is also not so nice IMO.
And not all that easy, so discarding it.
In general nothing should be hard coded that is not hard coded in documentation. Ie. if BDF *can* change, then it can not be hard coded. However, the BDF should already be available from devicetree.cb, and hopefully that can be used also for early setup.
Ah, I'll see how to access de devicetree.cb, then. But I think it was hardcoded in docs. They said dev 0 func 0x14 IIRC. I'll check again.
Please send patches for all these types of changes that you have already done. Send many patches with one isolated change in every patch. The smaller each patch is, the easier it is to commit it and also review and comment on any things that should be improved.
Ok. I have to clean up the code a little and incorporate las weekend svn changes, and then I'll break the changes in patches and start sending them. Beware that they are not completely tested (my board does not boot yet).
Certainly. I for one think that the good fix is to make a single long data structure which gathers that CPU revision information in one place.
[...]
I like the idea of a data structure better. We're using C here so there is no reason to spread one piece of logical information out over multiple discrete variable types. A revision bitfield and an array of names doesn't make sense when there could be a structure with everything gathered in one place.
Some of the tricks that are currently being done with the bitfield are harder with a structure. There is an array that is basically a ruleset for msr init and its elements include bitmasks to decide which rule fires for which cpu. If we use structs I guess we need lists of alternative structs in those rules, some values for each field meaning don't care or some such. And I have to check if there is any such design in parts compiled with romcc (lists are hard there, I gather). I guess if make says to me it compiles a file with romcc this file will be compiled with romcc for anyone with any configuration, right?
Has people already used RB_C3 processors with coreboot?
I doubt it.
Ok, thanks.
I think the code just has not been developed yet. The existing code is in no way complete, and all your improvements are very much welcome.
I'll do what I can and when I can, I have little spare time, and it looks it'll take long... I'll try to do at least whatever I need for my setup and as much more as I can without bending over... But I'll leave some parts to do, and may very easily get the others wrong. Sometimes you have to decide whether you support cache scrubbing or cache flush on halt, or similar features that are a bit esoteric for me, so I guess I'll begin by supporting none.
This is very wise.
[...]
Certainly not, you've identified very important points where coreboot needs to be improved.
Fine, thanks, I feel a little more sane (not so much, just as much as before I started with coreboot...)
Thanks!
To you and the rest of the coreboot team.
I'll send more details/patches later in new threads.
On Mon, Aug 16, 2010 at 06:21:31PM +0200, Xavi Drudis Ferran wrote:
Xavi Drudis Ferran wrote:
1.- In order to get sb700_lpc_init in sb700_early_setup.c to work I've got to modifiy pci_locate_device in order to refrain from scanning some functions in pci bus 0.
Hm. Which fn?
Sorry, I should have said devices. I think it scans every function of every device, reg. 00 and I told it not to do so for any function of some devices (those with nothing attached in my board). I'll send the patch later.
Here's the patch. I'll attach lspci output with the propietary BIOS in order to compare with the excluded devices. I'm testing without the Nvidia VGA, though. With svn 5701 + the earlier patch patch.warnerror + patch.serial1 that I'll attach here, my mainboard outputs to serial and stops at setAMDMSR (after start other cores, so I might misread the log)
I don't know if it makes sense. That's not neccessarily the case. It depends on if the SB700 can have a different BDF on different boards.
I'd say no, but what do I know?
but then I wonder whether my change is acceptable for other mainboards,
No, I don't think it is.
Please don't commit this patch. It's just an illustration on what I did to have serial output, but it may break other boards, or (I don't think so) the same mainboard with other hardware attached.
It seems I have to sign off even if I'm asking not to be commited so:
Signed off by: Xavi Drudis Ferran xdrudis@tinet.cat
In general nothing should be hard coded that is not hard coded in documentation. Ie. if BDF *can* change, then it can not be hard coded. However, the BDF should already be available from devicetree.cb, and hopefully that can be used also for early setup.
Ah, I'll see how to access de devicetree.cb, then. But I think it was hardcoded in docs. They said dev 0 func 0x14 IIRC. I'll check again.
I'm rereading docs for SB700 linked from http://www.coreboot.org/Datasheets#AMD_SB700.2FSB710.2FSB750
And I only see hardcoded BDF in the docs (in diagrams, chapter titles, text and assembler examples).
So I thik I'll change sb700_early_init to use hardcoded BDF unless someone thinks otherwise.
Ok. I have to clean up the code a little and incorporate las weekend svn changes, and then I'll break the changes in patches and start sending them. Beware that they are not completely tested (my board does not boot yet).
It's getting late. I'll continue tomorrow.
-
Peter Stuge -
Xavi Drudis Ferran -
xdrudis