On 12/16/2013 10:35 PM, ron minnich wrote:
On Fri, Dec 13, 2013 at 2:29 AM, Kyösti Mälkki kyosti.malkki@gmail.com wrote:
Yet we have had commit 032c23db for 5 months:
which may mean, only, that the commit broke some things and nobody hit those things until 5 months later?
Which is not at all unusual with a change of this type. It's why we prefer that commits that are this far-reaching come with some amount of testing. That patch changed 7 mainboards; which of them were tested?
Before merge, test on kontron/986-lcd-m as was recorded here: http://review.coreboot.org/#/c/3584/
Also lenovo x60 and t60 boards have been often built from master during the last 5 months.
I remember a week or so after this was merged you made that comment about i945 on a related change. I requested literature reference or a test case to see if I should revert. I got neither and the revert never took place.
It pays to listen to Stefan on matters such as these :-)
There are at least two reasons that your request might not have been satisfiable.
- the public docs and the hardware disagree (very common)
- the public docs and the NDA docs disagree (this is very common) and the vendor knows it (also really common)
Until you are more specific on your statement, I am reading it as follows:
Stefan added 'select MMCONF_SUPPORT_DEFAULT' on a mainboard with i945 and the board did not boot.
And sometimes that's about as good a diagnosis as you can get.
Expecting anything more is not always realistic. That's what makes firmware so hard, sometimes.
Meanwhile, we have a commit that broke some hardware. What do you want to do about it? I agree with Aaron, things might need to be tweaked, but ... who's got that old hardware, and the time to do it? Do you have a board of that ilk and the time to figure it out?
How do you intend to resolve the problem caused by this commit?
The process of deciding if i945 needs a fix has begun.
As you may notice, the first thing was to try to find out if the original statement is reliable, backuped up by any documentation, or if the statement was made based on runs on some particular platform.
So far no data from current and known codebase builds supports the statement that some PCI devices would not be accessible over MMCONF on platform with i945-
From a fairly recent codebase we see PCI MMCONF setup for i945 had two flaws in design: a) There are PCI configuration access before MMCONF is enabled which need to be forced to use IO and b) With MMCONF_SUPPORT_DEFAULT, PCI configuration access in early ramstage would use IO instead and fail to access registers in range 0x100-0xfff. There is reason to believe i945 development platform had same problems, which questions the reliability of the made conclusions.
Kyösti
Thanks, Kyosti, sounds like you're on top of it :-)
ron
On Mon, Dec 16, 2013 at 9:17 PM, Kyösti Mälkki kyosti.malkki@gmail.com wrote:
On 12/16/2013 10:35 PM, ron minnich wrote:
On Fri, Dec 13, 2013 at 2:29 AM, Kyösti Mälkki kyosti.malkki@gmail.com wrote:
Yet we have had commit 032c23db for 5 months:
which may mean, only, that the commit broke some things and nobody hit those things until 5 months later?
Which is not at all unusual with a change of this type. It's why we prefer that commits that are this far-reaching come with some amount of testing. That patch changed 7 mainboards; which of them were tested?
Before merge, test on kontron/986-lcd-m as was recorded here: http://review.coreboot.org/#/c/3584/
Also lenovo x60 and t60 boards have been often built from master during the last 5 months.
I remember a week or so after this was merged you made that comment about i945 on a related change. I requested literature reference or a test case to see if I should revert. I got neither and the revert never took place.
It pays to listen to Stefan on matters such as these :-)
There are at least two reasons that your request might not have been satisfiable.
- the public docs and the hardware disagree (very common)
- the public docs and the NDA docs disagree (this is very common) and the vendor knows it (also really common)
Until you are more specific on your statement, I am reading it as follows:
Stefan added 'select MMCONF_SUPPORT_DEFAULT' on a mainboard with i945 and the board did not boot.
And sometimes that's about as good a diagnosis as you can get.
Expecting anything more is not always realistic. That's what makes firmware so hard, sometimes.
Meanwhile, we have a commit that broke some hardware. What do you want to do about it? I agree with Aaron, things might need to be tweaked, but ... who's got that old hardware, and the time to do it? Do you have a board of that ilk and the time to figure it out?
How do you intend to resolve the problem caused by this commit?
The process of deciding if i945 needs a fix has begun.
As you may notice, the first thing was to try to find out if the original statement is reliable, backuped up by any documentation, or if the statement was made based on runs on some particular platform.
So far no data from current and known codebase builds supports the statement that some PCI devices would not be accessible over MMCONF on platform with i945-
From a fairly recent codebase we see PCI MMCONF setup for i945 had two flaws in design: a) There are PCI configuration access before MMCONF is enabled which need to be forced to use IO and b) With MMCONF_SUPPORT_DEFAULT, PCI configuration access in early ramstage would use IO instead and fail to access registers in range 0x100-0xfff. There is reason to believe i945 development platform had same problems, which questions the reliability of the made conclusions.
Kyösti
-
Kyösti Mälkki -
ron minnich