[coreboot] [RFT] Please test patch set intel/i945: Only write CID, PN and TCID once

[Mono]

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: coreboot at coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot