Thank you ron for your comments! actually I was not aware the code I look at is more than 5 years old. You are absolutely right in that the patch was written after reading public ICH7 documentation and I do not know of any problem the current implementation causes. Moreover, I was not aware that the code might do something else than what one can know by reading the code and the public documentation. To be honest, if that was true, I would be disappointed, because I expected coreboot to be a possibility of getting away from private software and "you don't know what your machine does". However, I did debug the code by reading the write-once registers in question before and after the several write operations. The registers do not change their contents but due to the first write operation. By that I concluded the public documentation describes the chip's behavior correctly. After reading your comments about private documentation of course this conclusion might be wrong though.
At the moment I believe that applying the patch does no harm. At least this is my current knowledge by running it at the MacBook2,1 for a couple of hours. Switching forth and back between coreboot versions (back to safe ones) works. I do not know if this was true for other boards. Who knows, it might(!) fix some 5 year old bug? In case you were interessted, I uploaded the output of inteltool when running the macbook without the patch, that is by compiling [1] and by compiling the patch on top of [1]. Please see 5324 without patch 5387: http://paste.flashrom.org/view.php?id=2051 5324 plus patch 5387: http://paste.flashrom.org/view.php?id=2052 diff -y without vs with patch 5387: http://paste.flashrom.org/view.php?id=2053 any light shedding is much appreciated! best regards, Mono [1] http://review.coreboot.org/#/c/5324/ On Sun, Mar 16, 2014 at 09:26:09AM -0700, ron minnich wrote: > So do I understand it correctly, that this patch is for hardware that has > been working, in some cases for 5 years; that it has > been written after reading a public doc; and that it was not developed in > response to a known bug or issue? > > Code that does not conform to a public doc is NOT a bug. > > A word to the wise: in all too many cases, the public docs and the private > docs disagree. In some cases, the private docs directly > contradict the public docs. And, in still other cases, the hardware > contradicts the private docs. That's what makes this so much fun. > > It's the implementation of the hardware, not the documentation, that > matters. > > Changes made in response to a reading of public docs, that are not known to > resolve an actual observed problem, are > a very risky thing to do, especially with old chipsets. They should be > rejected outright unless they are confirmed to fix a problem. > > We've had people push this kind of change in the past and break boards. > > So tread carefully. > > ron > -- > coreboot mailing list: [email protected] > http://www.coreboot.org/mailman/listinfo/coreboot -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
