[PATCH]ich_gpio_dump util

Here you go, this work great, thanks again Stefan.

Signed-off-by: Joseph Smith

Thanks - Joe

Quoting [e-mail]:
> Here you go, this work great, thanks again Stefan.
>
> Signed-off-by: Joseph Smith
>
> Thanks - Joe

Could I get an Acker??

Thanks - Joe

[PATCH]ich_gpio_dump util ACK???

Quoting [e-mail]:
> Quoting [e-mail]:
> > Here you go, this work great, thanks again Stefan.
> >
> > Signed-off-by: Joseph Smith
> >
> > Thanks - Joe
>
> Could I get an Acker??
>
> Thanks - Joe

[Uwe Hermann]

Maybe we want something like this in superiotool? It's not _really_
superio-related, but as we now start dumping EC information and other
stuff in superiotool we might also do this.

Having extra tools for such small stuff feels a bit strange.

On Sun, Feb 24, 2008 at 12:04:16PM -0500, [e-mail] wrote:
> Index: intel-debugtools/ich_gpio_dump/ich_gpio.c
>
> --- intel-debugtools/ich_gpio_dump/ich_gpio.c	(revision 0)
> +++ intel-debugtools/ich_gpio_dump/ich_gpio.c	(revision 0)
> @@ -0,0 +1,95 @@
> +/*
> dump gpio on intel ICH series southbridges
> Copyright (C) 2008 by coresystems GmbH
> written by Stefan Reinauer
> modded for all ICH's by Joseph Smith

If it's modded enough to be non-trivial, add

Copyright (C) 2008 Joseph Smith

otherwise please drop this line. The license header should only contain
copyright holders IMO.
> +int map_gpio(uint16_t gpio)
> +{
> int i;
> unsigned long size=0x40;
> for (i=0; i<size; i+=4) {
> printf("gpiobase+0x%04x: 0x%08x\n", i, inl(gpio+i));
> }
> return 0;
>
> +}

Please run indent over the code so it complies with the coding
standards.
> +int main(int argc, char *argv[])
> +{
> struct pci_access *pacc;
> struct pci_dev *sb;
> uint16_t gpiobadd;
> uint16_t device;
> if (iopl(3)) { printf("You need to be root.\n"); exit(1); }
> pacc = pci_alloc();
> pci_init(pacc);
> pci_scan_bus(pacc);
> sb = pci_get_dev(pacc, 0, 0, 0x1f, 0);
> if (!sb) {
> printf("No southbridge found.\n");
> pci_cleanup(pacc);
> exit(1);
> }
> if (pci_read_word(sb, 0) != 0x8086) {
> printf("Not an Intel southbridge.\n");
> pci_free_dev(sb);
> pci_cleanup(pacc);
> exit(1);
> }
> printf("Intel Southbridge: %04x:%04x\n",
> pci_read_word(sb, 0), pci_read_word(sb, 2));
> device = pci_read_word(sb, 2);
> if (device < 0x2640) {
> gpiobadd = pci_read_word(sb, 0x58) & 0xfffc; 
> } else if (device >= 0x2640) {
> gpiobadd = pci_read_word(sb, 0x48) & 0xfffc;
> }
> printf("GPIOBASE = 0x%04x\n\n", gpiobadd);

Hm, maybe this can even be done with some small shell script magic and
lspci and similar standard tools? If so, I'd prefer such a script
and / or just a small code snippet in the wiki?

Uwe.

Quoting Uwe Hermann :
> Maybe we want something like this in superiotool? It's not _really_
> superio-related, but as we now start dumping EC information and other
> stuff in superiotool we might also do this.

I agree it would be cool to have something like this in superiotool. I  
know for smsc it would be very helpful for a dump of "Runtime  
Registers".
> Having extra tools for such small stuff feels a bit strange.

reguarless of it's size it is still a tool:-)
> On Sun, Feb 24, 2008 at 12:04:16PM -0500, [e-mail] wrote:
> > Index: intel-debugtools/ich_gpio_dump/ich_gpio.c
> >
> > --- intel-debugtools/ich_gpio_dump/ich_gpio.c	(revision 0)
> > +++ intel-debugtools/ich_gpio_dump/ich_gpio.c	(revision 0)
> > @@ -0,0 +1,95 @@
> > +/*
> > dump gpio on intel ICH series southbridges
> > Copyright (C) 2008 by coresystems GmbH
> > written by Stefan Reinauer
> > modded for all ICH's by Joseph Smith
>
> If it's modded enough to be non-trivial, add
>
> Copyright (C) 2008 Joseph Smith
>
> otherwise please drop this line. The license header should only contain
> copyright holders IMO.

ok
> > +int map_gpio(uint16_t gpio)
> > +{
> > int i;
> > unsigned long size=0x40;
> > for (i=0; i<size; i+=4) {
> > printf("gpiobase+0x%04x: 0x%08x\n", i, inl(gpio+i));
> > }
> > return 0;
> >
> > +}
>
> Please run indent over the code so it complies with the coding
> standards.

ok
> > +int main(int argc, char *argv[])
> > +{
> > struct pci_access *pacc;
> > struct pci_dev *sb;
> > uint16_t gpiobadd;
> > uint16_t device;
> > if (iopl(3)) { printf("You need to be root.\n"); exit(1); }
> > pacc = pci_alloc();
> > pci_init(pacc);
> > pci_scan_bus(pacc);
> > sb = pci_get_dev(pacc, 0, 0, 0x1f, 0);
> > if (!sb) {
> > printf("No southbridge found.\n");
> > pci_cleanup(pacc);
> > exit(1);
> > }
> > if (pci_read_word(sb, 0) != 0x8086) {
> > printf("Not an Intel southbridge.\n");
> > pci_free_dev(sb);
> > pci_cleanup(pacc);
> > exit(1);
> > }
> > printf("Intel Southbridge: %04x:%04x\n",
> > pci_read_word(sb, 0), pci_read_word(sb, 2));
> > device = pci_read_word(sb, 2);
> > if (device < 0x2640) {
> > gpiobadd = pci_read_word(sb, 0x58) & 0xfffc;
> > } else if (device >= 0x2640) {
> > gpiobadd = pci_read_word(sb, 0x48) & 0xfffc;
> > }
> > printf("GPIOBASE = 0x%04x\n\n", gpiobadd);
>
> Hm, maybe this can even be done with some small shell script magic and
> lspci and similar standard tools? If so, I'd prefer such a script
> and / or just a small code snippet in the wiki?

Not sure how you could do this with lspci??? It can be don with dd but  
the output is not as pretty:-)
> Uwe.

Thanks for the feedback Uwe, You seem sort quiet these days.....

Thanks - Joe

[Carl-Daniel Hailfinger]

On 26.02.2008 21:52, [e-mail] wrote:
> Quoting Uwe Hermann :
> > Maybe we want something like this in superiotool? It's not _really_
> > superio-related, but as we now start dumping EC information and other
> > stuff in superiotool we might also do this.
>
> I agree it would be cool to have something like this in superiotool. I  
> know for smsc it would be very helpful for a dump of "Runtime  
> Registers".
> > Having extra tools for such small stuff feels a bit strange.
>
> reguarless of it's size it is still a tool:-)

There is another utility which reads the mcp55 configuration registers 
which is non-trivial as well. I vote to keep southbridge dumping and 
superio dumping separate.

Regards,
Carl-Daniel

Quoting Carl-Daniel Hailfinger :
> On 26.02.2008 21:52, [e-mail] wrote:
> > Quoting Uwe Hermann :
> > > Maybe we want something like this in superiotool? It's not _really_
> > > superio-related, but as we now start dumping EC information and other
> > > stuff in superiotool we might also do this.
> >
> > I agree it would be cool to have something like this in   
> > superiotool. I know for smsc it would be very helpful for a dump of  
> >  "Runtime  Registers".
> > > Having extra tools for such small stuff feels a bit strange.
> >
> > reguarless of it's size it is still a tool:-)
>
> There is another utility which reads the mcp55 configuration registers
> which is non-trivial as well. I vote to keep southbridge dumping and
> superio dumping separate.
>
> Regards,
> Carl-Daniel

ACK that:-)

Thanks - Joe

[Carl-Daniel Hailfinger]

On 24.02.2008 18:04, [e-mail] wrote:
> Here you go, this work great, thanks again Stefan.
>
> Signed-off-by: Joseph Smith

Maybe Stefan should add his signoff as well if it is his code?

Regards,
Carl-Daniel

[PATCH]ich_gpio_dump util Stefan?

Quoting Carl-Daniel Hailfinger :
> On 24.02.2008 18:04, [e-mail] wrote:
> > Here you go, this work great, thanks again Stefan.
> >
> > Signed-off-by: Joseph Smith
>
> Maybe Stefan should add his signoff as well if it is his code?
>
> Regards,
> Carl-Daniel

I agree:-))

Thanks - Joe

[PATCH]ich_gpio_dump util Stefan?

[Stefan Reinauer]

* [e-mail]  [080227 00:02]:
> Quoting Carl-Daniel Hailfinger :
> > On 24.02.2008 18:04, [e-mail] wrote:
> > > Here you go, this work great, thanks again Stefan.
> > >
> > > Signed-off-by: Joseph Smith
> > Maybe Stefan should add his signoff as well if it is his code?

Signed-off-by: Stefan Reinauer

[PATCH]ich_gpio_dump util ACK?

> Signed-off-by: Stefan Reinauer

Here is the util patch updated with suggestions from Uwe.

Signed-off-by: Joseph Smith

Thanks - Joe

[PATCH]ich_gpio_dump util ACK?

[ron minnich]

A few things caught my eye:
+	if (iopl(3)) { printf("You need to be root.\n"); exit(1); }

Just use perror. This is going to be confusing if there is an error
and you're running as root :-)

There are a few cases where there are pci cleanups and frees and such
and then an immediate exit. I applaud the sentiment but, really,
you're just as safe exiting without the cleanup. Just a thought ...

thanks, this is neat stuff.

ron

[PATCH]ich_gpio_dump util ACK?

Quoting ron minnich :
> A few things caught my eye:
> if (iopl(3)) { printf("You need to be root.\n"); exit(1); }
>
> Just use perror. This is going to be confusing if there is an error
> and you're running as root :-)
>
> There are a few cases where there are pci cleanups and frees and such
> and then an immediate exit. I applaud the sentiment but, really,
> you're just as safe exiting without the cleanup. Just a thought ...
>
> thanks, this is neat stuff.
>
> ron

Ok, I changed the printf to perror and removed the pci frees and  
cleanups from the errors and just left it on the main function.  
Attached is the revised patch.

Signed-off-by: Joseph Smith

Thanks - Joe

[PATCH]ich_gpio_dump util ACK?

[ron minnich]

I should have been clearer.

+		perror("You need to be root.\n");

No, the issue here is the iopl failed. In many cases it will be "you
need to be root" but you can't be sure in all.

So you want this:

+		perror("Enabling IO ports via iopl(3)");

perror will take care of telling why; you need to let the user know
what the program
was trying to do.

Also, just FYI:
+	printf("GPIOBASE = 0x%04x\n\n", gpiobadd);
+
+	pci_free_dev(sb);
+	pci_cleanup(pacc);
+
+	map_gpio(gpiobadd);

It's clean coding style, but the pci_free_dev and pci_cleanup are not
really necessary ... your program is leaving right after the map_gpio,
and these operations are not doing anything that you need to. Plus, if
someone wants to add some more looking around in the pci space, later,
they'll have to move this free/cleanup code around. But not really a
huge deal either way.

Thanks

ron

[PATCH]ich_gpio_dump util ACK?

Quoting ron minnich :
> I should have been clearer.
> perror("You need to be root.\n");
>
> No, the issue here is the iopl failed. In many cases it will be "you
> need to be root" but you can't be sure in all.
>
> So you want this:
> perror("Enabling IO ports via iopl(3)");
>
> perror will take care of telling why; you need to let the user know
> what the program
> was trying to do.
>
> Also, just FYI:
> printf("GPIOBASE = 0x%04x\n\n", gpiobadd);
> pci_free_dev(sb);
> pci_cleanup(pacc);
> map_gpio(gpiobadd);
>
> It's clean coding style, but the pci_free_dev and pci_cleanup are not
> really necessary ... your program is leaving right after the map_gpio,
> and these operations are not doing anything that you need to. Plus, if
> someone wants to add some more looking around in the pci space, later,
> they'll have to move this free/cleanup code around. But not really a
> huge deal either way.
>
> Thanks
>
> ron

Ok, I got ya on the perror.....
And if the pci_free_dev and pci_cleanup are not very important I have  
removed them also.

Signed-off-by: Joseph Smith

Thanks - Joe

[PATCH]ich_gpio_dump util ACK?

> Signed-off-by: Stefan Reinauer

Here is the util patch updated with suggestions from Ron.
Signed-off-by: Joseph Smith

Please Ack and commit?

Thanks - Joe

[PATCH]ich_gpio_dump util ACK?

[Carl-Daniel Hailfinger]

On 10.03.2008 01:56, [e-mail] wrote:
> > Signed-off-by: Stefan Reinauer
>
> Here is the util patch updated with suggestions from Ron.
> Signed-off-by: Joseph Smith
>
> Please Ack and commit?

Acked-by: Carl-Daniel Hailfinger

Does the "please commit" mean you don't have commit rights?

Did we decide on a directory name yet?

intel-debugtools/
ich_gpio_dump/

I'd like to keep all present and future Intel utilities in one directory
below util/ without deeper structure.

Regards,
Carl-Daniel

[PATCH]ich_gpio_dump util ACK?

[Peter Stuge]

On Mon, Mar 10, 2008 at 01:01:24PM +0100, Carl-Daniel Hailfinger wrote:
> Did we decide on a directory name yet?
>
> intel-debugtools/
> ich_gpio_dump/
>
> I'd like to keep all present and future Intel utilities in one
> directory below util/ without deeper structure.

ichtools or inteltools then?

//Peter

[PATCH]ich_gpio_dump util ACK?

Quoting Peter Stuge :
> On Mon, Mar 10, 2008 at 01:01:24PM +0100, Carl-Daniel Hailfinger wrote:
> > Did we decide on a directory name yet?
> >
> > intel-debugtools/
> > ich_gpio_dump/
> >
> > I'd like to keep all present and future Intel utilities in one
> > directory below util/ without deeper structure.
>
> ichtools or inteltools then?

inteltools sounds good:-)

Thanks - Joe

[PATCH]ich_gpio_dump util ACK?

[Carl-Daniel Hailfinger]

On 10.03.2008 16:30, [e-mail] wrote:
> Quoting Peter Stuge :
> > On Mon, Mar 10, 2008 at 01:01:24PM +0100, Carl-Daniel Hailfinger wrote:
> > > Did we decide on a directory name yet?
> > >
> > > intel-debugtools/
> > > ich_gpio_dump/
> > >
> > > I'd like to keep all present and future Intel utilities in one
> > > directory below util/ without deeper structure.
> >
> > ichtools or inteltools then?
>
> inteltools sounds good:-)

Could you resend the patch? The last one you sent had this chunk:
> Index: intel-debugtools/ich_gpio_dump/ich_gpio.c
>
> --- intel-debugtools/ich_gpio_dump/ich_gpio.c   (revision 0)
> +++ intel-debugtools/ich_gpio_dump/ich_gpio.c   (revision 0)
> @@ -0,0 +1,94 @@

However, the file was only 91 lines long. Did you hand-edit the patch
in some way?

Regards,
Carl-Daniel

[PATCH]ich_gpio_dump util ACK?

Quoting Carl-Daniel Hailfinger :
> On 10.03.2008 16:30, [e-mail] wrote:
> > Quoting Peter Stuge :
> > > On Mon, Mar 10, 2008 at 01:01:24PM +0100, Carl-Daniel Hailfinger wrote:
> > > > Did we decide on a directory name yet?
> > > >
> > > > intel-debugtools/
> > > > ich_gpio_dump/
> > > >
> > > > I'd like to keep all present and future Intel utilities in one
> > > > directory below util/ without deeper structure.
> > >
> > > ichtools or inteltools then?
> >
> > inteltools sounds good:-)
>
> Could you resend the patch? The last one you sent had this chunk:
> > Index: intel-debugtools/ich_gpio_dump/ich_gpio.c
> >
> > --- intel-debugtools/ich_gpio_dump/ich_gpio.c   (revision 0)
> > +++ intel-debugtools/ich_gpio_dump/ich_gpio.c   (revision 0)
> > @@ -0,0 +1,94 @@
>
> However, the file was only 91 lines long. Did you hand-edit the patch
> in some way?

yes I hand edited it for Rons requested changes. Here is an un-edited  
rvised patch.

Thanks - Joe

[PATCH]ich_gpio_dump util ACK?

[Carl-Daniel Hailfinger]

On 10.03.2008 18:11, [e-mail] wrote:
> Quoting Carl-Daniel Hailfinger :
> > On 10.03.2008 16:30, [e-mail] wrote:
> > > Quoting Peter Stuge :
> > > > On Mon, Mar 10, 2008 at 01:01:24PM +0100, Carl-Daniel Hailfinger
> > > > wrote:
> > > > > Did we decide on a directory name yet?
> > > > >
> > > > > intel-debugtools/
> > > > > ich_gpio_dump/
> > > > >
> > > > > I'd like to keep all present and future Intel utilities in one
> > > > > directory below util/ without deeper structure.
> > > >
> > > > ichtools or inteltools then?
> > >
> > > inteltools sounds good:-)
> >
> > However, the file was only 91 lines long. Did you hand-edit the patch
> > in some way?
>
> yes I hand edited it for Rons requested changes. Here is an un-edited
> rvised patch.

Committed in r3132.

Regards,
Carl-Daniel

[PATCH]ich_gpio_dump util ACK?

Quoting Carl-Daniel Hailfinger :
> On 10.03.2008 01:56, [e-mail] wrote:
> > > Signed-off-by: Stefan Reinauer
> >
> > Here is the util patch updated with suggestions from Ron.
> > Signed-off-by: Joseph Smith
> >
> > Please Ack and commit?
>
> Acked-by: Carl-Daniel Hailfinger
>
> Does the "please commit" mean you don't have commit rights?

No, not yet anyways....
> Did we decide on a directory name yet?
>
> intel-debugtools/
> ich_gpio_dump/
>
> I'd like to keep all present and future Intel utilities in one directory
> below util/ without deeper structure.

That's fine, that was Stefan's suggestion to create a sub directory  
just for intel tools, but util/ich_gpio_dump/ is fine with me.

Thanks - Joe

[PATCH]ich_gpio_dump util ACK?

[ron minnich]

On Sun, Mar 9, 2008 at 4:56 PM,   wrote:
> > Signed-off-by: Stefan Reinauer
>
> Here is the util patch updated with suggestions from Ron.
>
> Signed-off-by: Joseph Smith
>
> Please Ack and commit?
>
> Thanks - Joe

After all my commenting I had better ack :-)

Acked-by: Ronald G. Minnich