[coreboot] superiotool sometimes skips further SuperIO chips in the system - suggested remedy included
Frantisek.Rysanek at post.cz
Mon Jan 3 22:23:34 CET 2011
thanks for the polite responses from Mr. Hendricks and Herr Reinauer.
I have some further notes... As top-posting is a big no-no at least
in the LKML, see my further comments way below :-)
On 28 Dec 2010 at 23:51, David Hendricks wrote:
Heh, I hacked that "if (chip_found) return;" stuff into SuperI/O tool
a few months ago (http://www.coreboot.org/pipermail/coreboot/2010-
The reason was to avoid duplicate listings. In my specific case, I
was working with an ITE chip that does not need an entry/exit
sequence. So when probe_regs_ite() was called, it would find my chip
5 times and dump its registers 5 times. Sorry if it's causing you
headaches now :-/
I think long-term we should make superiotool more intelligent with
regard to duplicate entries. For example, if a chip has been found at
0x2e, then superiotool should stop probing devices on port 0x2e and
continue probing devices on ports 0x4e, 0x3f0, etc (as listed in
superioto_ports_table). I think we could accomplish this easily:
- Add a return code to each chip's probe_idregs_* function.
- Update the loop in main() which iterates thru
superio_ports_table. If a probe_idregs_* function returns a
specific code to indicate a chip has been found, then record the
corresponding superio port entry and avoid further calls to
superio_ports_table[i].probe_idregs() on that port.
On 30 Dec 2010 at 12:41, Stefan Reinauer wrote:
Therefore, after the first chip detected (of any kind),
only the first init sequence is ever tried, in any subsequent calls
to the aforementioned probe_* functions.
That function is called with a port address. There can only be one
chip at one port address. So leaving the function after a chip has
been found at that address seems like the right thing to do. The bug
is that chip_found is a global variable and not a local variable.
Attached patch should fix the issue.
And an equivalent change is needed for winbond.c I guess.
======= Okay here are my comments: =======
I have to say that I agree with Mr. Hendricks.
Once you find a chip at a particular port, it should not make sense
to probe *at that particular port* again. So you need some global way
of tagging I/O ports already occupied.
I do not think that making the "chip_found" local to the particular
vendor-specific probe function would solve the problem. Yes it does
make it scan all the different init sequences across all the ports,
but other vendor-specific modules will re-scan the ports anyway (as
the info about ports already occupied doesn't exist in the global
Now... the way the topmost superio_ports_table[vendor]->ports is
organized, indeed the same port will be scanned by multiple probes.
The I/O port is a "leaf attribute" of a vendor in the data model, and
is not indexed upon in any way.
I believe this calls for a slight "reinforcement" of the data model.
Specifically: It seems to me that superiotool.c would have to keep an
additional index (even just a list with some manipulator functions)
of ports already known to be occupied.
We'll need to retain the mapping of vendor->ports.
It doesn't seem very fruitful to re-factor the table as port->vendors
:-) Or does it?
Side note: at a fist glance, it seemd to me that for some advanced
functionality (writing to ports, separation of "probe" from "dump"
etc) it might be interesting to create a global list of "devices"
detected. This would have the neat side effect of having a list of
the IO ports already probed... but at a closer look, it seems that
the vendor-specific modules are much too "polymorphic", the "device
private data" would have to be polymorphic (vendor-specific), and all
the vendor-specific code would have to be somewhat heavily refactored
in quite an individual fashion, to meet the new and more refined
"abstraction model"... looks like too much work and very little
I have to say that I've dipped my toes in some intermediate-level
C++, but I can also sculpt encapsulation and polymorphism in bare
C... yet in this case I don't think there's any call for either of
Another minor point: it seems interesting to me that the
superio_ports_table is declared+defined "static const"
_in_the_header_file_. Thus, each module that includes superiotool.h
instantiates that table, even though only superiotool.c ever makes
any use of it. The obvious solution would be to move the
superio_ports_table to superiotool.c. Or maybe break it up and make
the tables of ports "private" to the individual vendor modules,
managed/walked by the per-module probe functions. Yet for the moment
I admit that there is indeed certain beauty (elegance?) in having all
that data in a single table in superiotool.c, in a common
structure... so I'd probably just keep that arrangement :-)
I am a newcomer :-) Sorry for talking so much.
At the same time, I don't like adding further layers of cruft on top
of each other - especially if this has been incited by my own
The Superiotool looks like a neatly compact piece of source code -
small enough for me to understand / encompass :-)
I'd like to offer a bit of my own time to add such a global index of
"ports occupied" (likely not "devices detected" - see above why).
I'd also have to touch all the vendor-specific modules, but only very
slightly (to make the probe functions return a result code). I'd have
to undo the interim hacks (sorry) added by Mr. Hendricks and Mr.
I'd probably start from the version that I've checked out right now
from SVN. I don't know how to return to an earlier version, and
besides there has been a further detection update to the ITE module,
on top of Mr. Reinauer's recent change...
Would you be interested?
It might take me days of real time - my job and family allow
relatively little time for ad hoc meddling... :-)
More information about the coreboot