patch: lxirq understander

[ron minnich]

Can I get feedback on this one? It's the lx irq understander. I'd like
to get this in so others can give it a try.

I want to finish this as I need to write an LX NB understander, in an
attempt to do a first-principals pass at 'what the hell is wrong with
the dbe62'.

Signed-off-by: Ronald G. Minnich

ron

[Carl-Daniel Hailfinger]

On 09.07.2008 18:10, ron minnich wrote:
> Can I get feedback on this one? It's the lx irq understander. I'd like
> to get this in so others can give it a try.
>
> I want to finish this as I need to write an LX NB understander, in an
> attempt to do a first-principals pass at 'what the hell is wrong with
> the dbe62'.
>
> Signed-off-by: Ronald G. Minnich

Absolutely awesome!

Acked-by: Carl-Daniel Hailfinger

I'd like to suggest a different directory name, though:
util/lxirq_understander ;-)

Regards,
Carl-Daniel

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> On 09.07.2008 18:10, ron minnich wrote:
> > Can I get feedback on this one? It's the lx irq understander. I'd like
> > to get this in so others can give it a try.
> >
> > I want to finish this as I need to write an LX NB understander, in an
> > attempt to do a first-principals pass at 'what the hell is wrong with
> > the dbe62'.
> >
> > Signed-off-by: Ronald G. Minnich
>
> Absolutely awesome!
>
> Acked-by: Carl-Daniel Hailfinger
>
> I'd like to suggest a different directory name, though:
> util/lxirq_understander ;-)

Can we call it geodetool or something, and advance it with all the other
stuff that will follow?

tools with _ in their name have bad names.

[ron minnich]

So I started to fix it up with names and respond to the other comments.

Turns out you can't do this:
#include <pci/pci.h>

#include <device/pci_ids.h>

with our pci_ids.h because our pci_ids.h includes stuff that is not
ids -- e.g. class types etc.

So we conflict:
In file included from lxirq.c:44:
../../include/device/pci_ids.h:132:1: error:
"PCI_CLASS_CRYPT_ENTERTAINMENT" redefined
In file included from /usr/include/pci/pci.h:13,
                 from lxirq.c:34:
/usr/include/pci/header.h:975:1: error: this is the location of the
previous definition

So I lost patience as I have to go to work now :-)

I will try to put this on my web page somewhere.

I'll drop it for now. But somebody will need this tool  in the future.
They'll write it again. We need to have a way to get little utilities
like this into the tree. They are pretty typical of the throw-away
tools engineers write while doing board bringup. Thing is, they get
written and lost because they're crude and ugly. But they are very
useful.

Thanks

ron

[Stefan Reinauer]

ron minnich wrote:
> So I started to fix it up with names and respond to the other comments.
>
> Turns out you can't do this:
> #include <pci/pci.h>
>
> #include <device/pci_ids.h>

Why would you pull in both libpci and coreboot headers?

libpci pretty much brings its own PCI ID database.
> with our pci_ids.h because our pci_ids.h includes stuff that is not
> ids -- e.g. class types etc.
>
> So we conflict:
> In file included from lxirq.c:44:
> ../../include/device/pci_ids.h:132:1: error:
> "PCI_CLASS_CRYPT_ENTERTAINMENT" redefined
> In file included from /usr/include/pci/pci.h:13,
>                  from lxirq.c:34:
> /usr/include/pci/header.h:975:1: error: this is the location of the
> previous definition
>
> So I lost patience as I have to go to work now :-)
>
> I will try to put this on my web page somewhere.
>
> I'll drop it for now. But somebody will need this tool  in the future.
> They'll write it again. We need to have a way to get little utilities
> like this into the tree. They are pretty typical of the throw-away
> tools engineers write while doing board bringup. Thing is, they get
> written and lost because they're crude and ugly. But they are very
> useful.

We have a collection of such utilities, and they're quite nice... 
mptable, pirq, inteltool, superiotool, there is even one for sc520 iirc.

Things are improving over time. Think of how flash_and_burn starting as 
a shellscript calling setpci and it evolved into flashrom eventually.

I think it's awesome that you write another one of these tools... 
They're an important step towards automation of coreboot ports

[ron minnich]

On Thu, Jul 24, 2008 at 9:16 AM, Stefan Reinauer  wrote:
> ron minnich wrote:
> > So I started to fix it up with names and respond to the other comments.
> >
> > Turns out you can't do this:
> > #include <pci/pci.h>
> >
> > #include <device/pci_ids.h>
>
> Why would you pull in both libpci and coreboot headers?

so I can say AMD instead of 0x1022. the standard PCI includes don't have that.
> libpci pretty much brings its own PCI ID database.

well, sort of. What did I miss?
> We have a collection of such utilities, and they're quite nice... mptable,
> pirq, inteltool, superiotool, there is even one for sc520 iirc.
>
> Things are improving over time. Think of how flash_and_burn starting as a
> shellscript calling setpci and it evolved into flashrom eventually.
>
> I think it's awesome that you write another one of these tools... They're an
> important step towards automation of coreboot ports

I'll try again tonight.

thanks again.

ron

[ron minnich]

On Thu, Jul 24, 2008 at 3:44 PM, ron minnich  wrote:
> On Thu, Jul 24, 2008 at 9:16 AM, Stefan Reinauer  wrote:
> > ron minnich wrote:
> > > So I started to fix it up with names and respond to the other comments.
> > >
> > > Turns out you can't do this:
> > > #include <pci/pci.h>
> > >
> > > #include <device/pci_ids.h>
> >
> > Why would you pull in both libpci and coreboot headers?
>
> so I can say AMD instead of 0x1022. the standard PCI includes don't have that.
> > libpci pretty much brings its own PCI ID database.
>
> well, sort of. What did I miss?

turns out I did not miss anything. libpci has no pci id database that
I can find.

You have to use a kernel including file from a kernel source tree,
it's not even in include/linux.

Suggestions welcome at this point.

ron

[Carl-Daniel Hailfinger]

On 24.07.2008 17:20, ron minnich wrote:
> So I started to fix it up with names and respond to the other comments.

Cool.
> So I lost patience as I have to go to work now :-)
>
> I will try to put this on my web page somewhere.
>
> I'll drop it for now.

Please send your current state to the list.
> But somebody will need this tool in the future.
> They'll write it again. We need to have a way to get little utilities
> like this into the tree. They are pretty typical of the throw-away
> tools engineers write while doing board bringup. Thing is, they get
> written and lost because they're crude and ugly. But they are very
> useful.

I think a short review round for a utility in the "compiles, works"
state should be enough for a commit below util/.

Regards,
Carl-Daniel

[Uwe Hermann]

On Wed, Jul 09, 2008 at 09:10:26AM -0700, ron minnich wrote:
> Can I get feedback on this one? It's the lx irq understander. I'd like
> to get this in so others can give it a try.
>
> I want to finish this as I need to write an LX NB understander, in an
> attempt to do a first-principals pass at 'what the hell is wrong with
> the dbe62'.
>
> Signed-off-by: Ronald G. Minnich
> Index: util/lxirq/lxirq.c
>
> --- util/lxirq/lxirq.c	(revision 0)
> +++ util/lxirq/lxirq.c	(revision 0)
> @@ -0,0 +1,252 @@
> +/*
> This file is part of the flashrom project.

s/flashrom/lxirq/
> Copyright (C) 2000 Silicon Integrated System Corporation
> Copyright (C) 2004 Tyan Corp
> Copyright (C) 2005-2007 coresystems GmbH

You probably copied some file from flashrom but nothing that's left
looks relevant I think, so this is

Copyright (C) 2008 Ronald G. Minnich
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */

[...]
> +struct pci_access *pacc;	/* For board and chipset_enable */
> +int verbose = 0;
> +struct pci_dev *cs5536_find(void)
> +{
> struct pci_dev *temp;
> struct pci_filter filter;
> pci_filter_init(NULL, &filter);
> filter.vendor = 0x1022;
> filter.device = 0x2090;

Should probably be #defines a la pci_ids.h. I do realize that this is
a quick solution, but making it more readable is still a good idea.
However, is it intended for longer-term, general-prupose usage or
just a temporary hack for finding a specific bug? I.e. do we really want
to commit it and will we need it ever again after the bug was found?
> for (temp = pacc->devices; temp; temp = temp->next)
> if (pci_filter_match(&filter, temp))
> 	return temp;
> return NULL;
>
> +}
>
> +void usage(const char *name)
> +{
> printf("usage: %s\n", name);
> exit(1);
>
> +}
>
> +void print_version(void)
> +{
> printf("flashrom r%s\n", LXIRQ_VERSION);
>
> +}

Incorrect, can be fixed or dropped.
> Index: util/lxirq/Makefile
>
> --- util/lxirq/Makefile	(revision 0)
> +++ util/lxirq/Makefile	(revision 0)

Lots of unneeded / wrong stuff in the Makefile (leftovers from flashrom).

Uwe.

[Peter Stuge]

On Wed, Jul 09, 2008 at 09:10:26AM -0700, ron minnich wrote:
> Can I get feedback on this one?

Sorry Ron, but I don't think this looks so hot. :\
> It's the lx irq understander. I'd like
> to get this in so others can give it a try.

Maybe just send a tarball? I think this util is useful but I would
prefer we not commit it as is because the code is too messy.
> 'what the hell is wrong with the dbe62'

I suggest just dumping out all MSRs after booting with a working
firmware, then dumping them again with the not-so-much-working
firmware, diff -u and look differences up in docs.

I think that will be much less work than codifying the registers.

//Peter