superiotool sometimes skips further SuperIO chips in the system - suggested remedy included

[Frantisek Rysanek]

Dear maintainers of the superiotool,

I have the following "feature request" (maybe not a bug report):
I have this idea that superiotool should be able to detect more than 
one SuperIO chip in the system. Such detection of multiple chips may 
work in some cases, but may fail in others.

The primary cause seems to be that
  winbond.c :: probe_idregs_winbond()
and 
   ite.c :: probe_idregs_ite()
call 
   if (chip_found) return;   // this is a global variable!
after every "init sequence" tried.

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.

My immediate hack was to comment out all occurrences of
   if (chip_found) return;
in those probe functions.
Makes me wonder if this has some adverse effects, such as repeatedly 
scanning some EFER bases / init sequences by probes belonging to 
different chip families.
And my conclusion for the moment is that this repeated probing should 
be harmless, and that the current behavior (stop after the first init 
sequence tried) is flawed anyway :-)

My motivation is simple: right now I'm working with an embedded PC 
that has two SuperIO chips on the inside: W83627 + W83977.
But I've seen other models in the past as well.

Your SuperIOtool is quite useful for a quick overview of the SuperIO 
chips in the system, including a thorough dump of their registers.
In the past I myself have written and reused some simple snippet of 
code / a tiny library (in DOS) to help me debug/tweak SuperIO stuff, 
but I never got as far as making it truly universal and extensible. 
As for SuperIOtool, I could fancy a writing capability, and maybe an 
external configuration file, potentially allowing for extensions 
without coding in C, maybe human-readable interpretations / 
annotations of the register dump etc - but I admit that's too much 
work to be worth the bother, certainly for me at the moment :-)

Keep up the excellent work that you're doing...

Frank Rysanek

superiotool sometimes skips further SuperIO chips in the system - suggested remedy included

[David Hendricks]

On Tue, Dec 28, 2010 at 4:13 AM, Frantisek Rysanek <
[e-mail]> wrote:
> Dear maintainers of the superiotool,
>
> I have the following "feature request" (maybe not a bug report):
> I have this idea that superiotool should be able to detect more than
> one SuperIO chip in the system. Such detection of multiple chips may
> work in some cases, but may fail in others.
>
> The primary cause seems to be that
>  winbond.c :: probe_idregs_winbond()
> and
>   ite.c :: probe_idregs_ite()
> call
>   if (chip_found) return;   // this is a global variable!
> after every "init sequence" tried.
>
> 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.
>
> My immediate hack was to comment out all occurrences of
>   if (chip_found) return;
> in those probe functions.
> Makes me wonder if this has some adverse effects, such as repeatedly
> scanning some EFER bases / init sequences by probes belonging to
> different chip families.
> And my conclusion for the moment is that this repeated probing should
> be harmless, and that the current behavior (stop after the first init
> sequence tried) is flawed anyway :-)

Heh, I hacked that "if (chip_found) return;" stuff into SuperI/O tool a few
months ago (
http://www.coreboot.org/pipermail/coreboot/2010-August/059607.html).

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.
> My motivation is simple: right now I'm working with an embedded PC
> that has two SuperIO chips on the inside: W83627 + W83977.
> But I've seen other models in the past as well.
>
> Your SuperIOtool is quite useful for a quick overview of the SuperIO
> chips in the system, including a thorough dump of their registers.
> In the past I myself have written and reused some simple snippet of
> code / a tiny library (in DOS) to help me debug/tweak SuperIO stuff,
> but I never got as far as making it truly universal and extensible.
> As for SuperIOtool, I could fancy a writing capability, and maybe an
> external configuration file, potentially allowing for extensions
> without coding in C, maybe human-readable interpretations /
> annotations of the register dump etc - but I admit that's too much
> work to be worth the bother, certainly for me at the moment :-)
>
> Keep up the excellent work that you're doing...
>
> Frank Rysanek

superiotool sometimes skips further SuperIO chips in the system - suggested remedy included

[Stefan Reinauer]

On 12/28/10 11:51 PM, David Hendricks wrote:
> On Tue, Dec 28, 2010 at 4:13 AM, Frantisek Rysanek
> <[e-mail] > wrote:
> Dear maintainers of the superiotool,
>
> I have the following "feature request" (maybe not a bug report):
> I have this idea that superiotool should be able to detect more than
> one SuperIO chip in the system. Such detection of multiple chips may
> work in some cases, but may fail in others.
>
> The primary cause seems to be that
>  winbond.c :: probe_idregs_winbond()
> and
>   ite.c :: probe_idregs_ite()
> call
>   if (chip_found) return;   // this is a global variable!
> after every "init sequence" tried.
>
> 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.

Stefan

superiotool sometimes skips further SuperIO chips in the system - suggested remedy included

[Stefan Reinauer]

On 12/30/10 12:13 PM, Stefan Reinauer wrote:
> On 12/28/10 11:51 PM, David Hendricks wrote:
> > On Tue, Dec 28, 2010 at 4:13 AM, Frantisek Rysanek
> > <[e-mail] > wrote:
> > Dear maintainers of the superiotool,
> >
> > I have the following "feature request" (maybe not a bug report):
> > I have this idea that superiotool should be able to detect more than
> > one SuperIO chip in the system. Such detection of multiple chips may
> > work in some cases, but may fail in others.
> >
> > The primary cause seems to be that
> >  winbond.c :: probe_idregs_winbond()
> > and
> >   ite.c :: probe_idregs_ite()
> > call
> >   if (chip_found) return;   // this is a global variable!
> > after every "init sequence" tried.
> >
> > 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.

superiotool sometimes skips further SuperIO chips in the system - suggested remedy included

[Frantisek Rysanek]

Dear gentlemen,

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-
August/059607.html). 
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 
scope).

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 
immediate use.
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 
that.

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 
"feedback".
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. 
Reinauer.
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... :-)

Frank Rysanek

superiotool sometimes skips further SuperIO chips in the system - suggested remedy included

[Peter Stuge]

Frantisek Rysanek wrote:
> 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).

Great! Please send the patch with Signed-off-by according to the
coreboot development guidelines: (they are short)

http://www.coreboot.org/Development_Guidelines#How_to_contribute
> Would you be interested?

Yes. I suggest using a fixed size array for the ports:

#define MAX_SUPERIOS 10
static unsigned char found_superios = 0;
static unsigned short superio_port[MAX_SUPERIOS];

and when looping over devices, skip already-known ports:

for(each device) {
  for(i=0;i<found_superios;i++)
    if(cur_device_port == superio_port[i])
      goto nextdev;
  if(probe(cur_device_port))
    superio_port[found_superios++] = cur_device_port;
nextdev:
}

//Peter

superiotool sometimes skips further SuperIO chips in the system - suggested remedy included

[Peter Stuge]

Stefan Reinauer wrote:
> That function is called with a port address. There can only be one chip
> at one port address.

But the same function can be called with a different port, and should
find another chip. I think David's suggestion is the way to go.

//Peter