[patch] add intel_piix4_gpo_set()

[Luc Verhaegen]

This is code from a an old board enable that i sent in 3 weeks ago. This 
board enable was not necessary (as flashing worked just fine without it 
too). But this function was also used to clean up the board enable for 
the epox ep bx3.

I have tracked down the person for whom i wrote this board enable 2 
years ago: irc user nyu, aka Robert Millan.

Robert, can you verify that this code is not a regression for you?

Uwe, in the original mail thread 
(http://www.coreboot.org/pipermail/coreboot/2009-June/049789.html) you 
had several suggestions. I have taken over unsigned int and the 
bitshift, but i do mot like to put "PIIX4{,E,M}" everywhere. 
"PIIX4{,E,M}" all over clutters up the place, and i fear that printing 
this to the user will generate more confusion than it will ever remove.
Instead i have adjusted the initial function comment to mention this so 
that developers can rest assured in future that this will also be valid 
for their future board enables.

Luc Verhaegen.

[Luc Verhaegen]

On Tue, Jul 07, 2009 at 03:10:32PM +0200, Luc Verhaegen wrote:
> This is code from a an old board enable that i sent in 3 weeks ago. This 
> board enable was not necessary (as flashing worked just fine without it 
> too). But this function was also used to clean up the board enable for 
> the epox ep bx3.
>
> I have tracked down the person for whom i wrote this board enable 2 
> years ago: irc user nyu, aka Robert Millan.
>
> Robert, can you verify that this code is not a regression for you?
>
> Uwe, in the original mail thread 
> (http://www.coreboot.org/pipermail/coreboot/2009-June/049789.html) you 
> had several suggestions. I have taken over unsigned int and the 
> bitshift, but i do mot like to put "PIIX4{,E,M}" everywhere. 
> "PIIX4{,E,M}" all over clutters up the place, and i fear that printing 
> this to the user will generate more confusion than it will ever remove.
> Instead i have adjusted the initial function comment to mention this so 
> that developers can rest assured in future that this will also be valid 
> for their future board enables.
>
> Luc Verhaegen.

Urgh, should've been an "@" instead of " at "... Bouncing original 
message to victims as well.

Luc Verhaegen.

[Robert Millan]

On Tue, Jul 07, 2009 at 03:10:32PM +0200, Luc Verhaegen wrote:
> This is code from a an old board enable that i sent in 3 weeks ago. This 
> board enable was not necessary (as flashing worked just fine without it 
> too). But this function was also used to clean up the board enable for 
> the epox ep bx3.
>
> I have tracked down the person for whom i wrote this board enable 2 
> years ago: irc user nyu, aka Robert Millan.
>
> Robert, can you verify that this code is not a regression for you?

Unfortunately I don't currently have suitable RAM to power on that board.

If it's important, I could try to borrow it from somewhere.

[Carl-Daniel Hailfinger]

On 07.07.2009 17:12, Robert Millan wrote:
> On Tue, Jul 07, 2009 at 03:10:32PM +0200, Luc Verhaegen wrote:
> > This is code from a an old board enable that i sent in 3 weeks ago. This 
> > board enable was not necessary (as flashing worked just fine without it 
> > too). But this function was also used to clean up the board enable for 
> > the epox ep bx3.
> >
> > I have tracked down the person for whom i wrote this board enable 2 
> > years ago: irc user nyu, aka Robert Millan.
> >
> > Robert, can you verify that this code is not a regression for you?
>
> Unfortunately I don't currently have suitable RAM to power on that board.
>
> If it's important, I could try to borrow it from somewhere.

This would be very appreciated. This is the second oldest unmerged patch
and I'd like to close the issue.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 25.09.2009 00:18, Carl-Daniel Hailfinger wrote:
> On 07.07.2009 17:12, Robert Millan wrote:
> > On Tue, Jul 07, 2009 at 03:10:32PM +0200, Luc Verhaegen wrote:
> > > This is code from a an old board enable that i sent in 3 weeks ago. This 
> > > board enable was not necessary (as flashing worked just fine without it 
> > > too). But this function was also used to clean up the board enable for 
> > > the epox ep bx3.
> > >
> > > I have tracked down the person for whom i wrote this board enable 2 
> > > years ago: irc user nyu, aka Robert Millan.
> > >
> > > Robert, can you verify that this code is not a regression for you?
> >
> > Unfortunately I don't currently have suitable RAM to power on that board.
> >
> > If it's important, I could try to borrow it from somewhere.
>
> This would be very appreciated. This is the second oldest unmerged patch
> and I'd like to close the issue.

I just talked to Robert and he said we shouldn't wait for him. Luc, can
you commit?

Regards,
Carl-Daniel

[Luc Verhaegen]

On Mon, Oct 05, 2009 at 02:30:29PM +0200, Carl-Daniel Hailfinger wrote:
> On 25.09.2009 00:18, Carl-Daniel Hailfinger wrote:
> > On 07.07.2009 17:12, Robert Millan wrote:
> > > On Tue, Jul 07, 2009 at 03:10:32PM +0200, Luc Verhaegen wrote:
> > > > This is code from a an old board enable that i sent in 3 weeks ago. This 
> > > > board enable was not necessary (as flashing worked just fine without it 
> > > > too). But this function was also used to clean up the board enable for 
> > > > the epox ep bx3.
> > > >
> > > > I have tracked down the person for whom i wrote this board enable 2 
> > > > years ago: irc user nyu, aka Robert Millan.
> > > >
> > > > Robert, can you verify that this code is not a regression for you?
> > >
> > > Unfortunately I don't currently have suitable RAM to power on that board.
> > >
> > > If it's important, I could try to borrow it from somewhere.
> >
> > This would be very appreciated. This is the second oldest unmerged patch
> > and I'd like to close the issue.
>
> I just talked to Robert and he said we shouldn't wait for him. Luc, can
> you commit?
>
> Regards,
> Carl-Daniel

If someone acks again ;)

I only got an ack from uwe when most of this code was still part of 
another board enable which turned out to be unnecessary.

Luc Verhaegen.

[Carl-Daniel Hailfinger]

On 07.07.2009 15:10, Luc Verhaegen wrote:
> /* dual function that need special enable. */
> if ((gpo >= 22) && (gpo <= 26)) {
> tmp = pci_read_long(dev, 0xB0); /* GENCFG */
> switch (gpo) {
> case 22: /* XBUS: XDIR#/GPO22 */
> case 23: /* XBUS: XOE#/GPO23 */
> 	tmp |= 1 << 28;
> 	break;
> case 24: /* RTCSS#/GPO24 */
> 	tmp |= 1 << 29;
> 	break;
> case 25: /* RTCALE/GPO25 */
> 	tmp |= 1 << 30;
> 	break;
> case 26: /* KBCSS#/GPO26 */
> 	tmp |= 1 << 31;
> 	break;
> default:

If the default case triggers, you found a compiler bug. Remove.
> 	fprintf(stderr, "\nERROR: Unsupported PIIX4 GPO%d.\n", gpo);
> 	return -1;
> }
> pci_write_long(dev, 0xB0, tmp);
> }
> /* GPO {0,8,27,28,30} are always available. */
> dev = pci_dev_find(0x8086, 0x7113);	/* Intel PIIX4 PM */
> if (!dev) {
> fprintf(stderr, "\nERROR: Intel PIIX4 PM not found.\n");
> return -1;
> }
> /* PM IO base */
> base = pci_read_long(dev, 0x40) & 0x0000FFC0;
> tmp = INL(base + 0x34); /* GPO register */
> if (raise)
> tmp |= 0x01 << gpo;
> else
> tmp |= ~(0x01 << gpo);
> OUTL(tmp, base + 0x34);
> return 0;
>
> +}
>
> +/**
> Suited for VIAs EPIA M and MII, and maybe other CLE266 based EPIAs.
> We don't need to do this when using coreboot, GPIO15 is never lowered there.
>
> @@ -440,18 +512,7 @@
>   */
>  static int board_epox_ep_bx3(const char *name)
>  {
> uint8_t tmp;
> /* Raise GPIO22. */
> tmp = INB(0x4036);
> OUTB(tmp, 0xEB);
> tmp |= 0x40;
> OUTB(tmp, 0x4036);
> OUTB(tmp, 0xEB);
> return 0;
> return intel_piix4_gpo_set(22, 1);
>
> }
>
> /**

Ahem. This patch is not functionally equivalent. Not at all.

The old code:
- Read 8-bit value from port 0x4036. Write said value to port 0xeb. Set
bit 6 in value. Write value again to port 0xeb and write value to port
0x4036. No config space accesses.

The new code:
- Set bit 28 of 32-bit value in PCI config space of PIIX4 ISA at 0xb0.
Get PMBASE from PCI config space of PIIX4 PM. Read 32-bit value from
port PMBASE+0x34. Set bit 22 in value. Write value back to PMBASE+0x34.
Assuming PMBASE=0x4000, this is equivalent to setting bit 6 in the 8-bit
value at port 0x4036.

Difference:
- Port 0xeb is not written or read in the new code.
- PCI config space of PIIX4 ISA at 0xb0 was not read or written in the
old code.

I'd like to see an explanation for this.

Regards,
Carl-Daniel

[Luc Verhaegen]

On Tue, Oct 06, 2009 at 04:27:52PM +0200, Carl-Daniel Hailfinger wrote:
> On 07.07.2009 15:10, Luc Verhaegen wrote:
> > /* dual function that need special enable. */
> > if ((gpo >= 22) && (gpo <= 26)) {
> > tmp = pci_read_long(dev, 0xB0); /* GENCFG */
> > switch (gpo) {
> > case 22: /* XBUS: XDIR#/GPO22 */
> > case 23: /* XBUS: XOE#/GPO23 */
> > 	tmp |= 1 << 28;
> > 	break;
> > case 24: /* RTCSS#/GPO24 */
> > 	tmp |= 1 << 29;
> > 	break;
> > case 25: /* RTCALE/GPO25 */
> > 	tmp |= 1 << 30;
> > 	break;
> > case 26: /* KBCSS#/GPO26 */
> > 	tmp |= 1 << 31;
> > 	break;
> > default:
>
> If the default case triggers, you found a compiler bug. Remove.

Sure.
> > 	fprintf(stderr, "\nERROR: Unsupported PIIX4 GPO%d.\n", gpo);
> > 	return -1;
> > }
> > pci_write_long(dev, 0xB0, tmp);
> > }
> > /* GPO {0,8,27,28,30} are always available. */
> > dev = pci_dev_find(0x8086, 0x7113);	/* Intel PIIX4 PM */
> > if (!dev) {
> > fprintf(stderr, "\nERROR: Intel PIIX4 PM not found.\n");
> > return -1;
> > }
> > /* PM IO base */
> > base = pci_read_long(dev, 0x40) & 0x0000FFC0;
> > tmp = INL(base + 0x34); /* GPO register */
> > if (raise)
> > tmp |= 0x01 << gpo;
> > else
> > tmp |= ~(0x01 << gpo);
> > OUTL(tmp, base + 0x34);
> > return 0;
> >
> > +}
> >
> > +/**
> > Suited for VIAs EPIA M and MII, and maybe other CLE266 based EPIAs.
> > We don't need to do this when using coreboot, GPIO15 is never lowered there.
> >
> > @@ -440,18 +512,7 @@
> >   */
> >  static int board_epox_ep_bx3(const char *name)
> >  {
> > uint8_t tmp;
> > /* Raise GPIO22. */
> > tmp = INB(0x4036);
> > OUTB(tmp, 0xEB);
> > tmp |= 0x40;
> > OUTB(tmp, 0x4036);
> > OUTB(tmp, 0xEB);
> > return 0;
> > return intel_piix4_gpo_set(22, 1);
> >
> > }
> >
> > /**
>
> Ahem. This patch is not functionally equivalent. Not at all.
>
> The old code:
> Read 8-bit value from port 0x4036. Write said value to port 0xeb. Set
>
> bit 6 in value. Write value again to port 0xeb and write value to port
> 0x4036. No config space accesses.
>
> The new code:
> Set bit 28 of 32-bit value in PCI config space of PIIX4 ISA at 0xb0.
>
> Get PMBASE from PCI config space of PIIX4 PM. Read 32-bit value from
> port PMBASE+0x34. Set bit 22 in value. Write value back to PMBASE+0x34.
> Assuming PMBASE=0x4000, this is equivalent to setting bit 6 in the 8-bit
> value at port 0x4036.
>
> Difference:
> Port 0xeb is not written or read in the new code.
> PCI config space of PIIX4 ISA at 0xb0 was not read or written in the
>
> old code.
>
> I'd like to see an explanation for this.
>
> Regards,
> Carl-Daniel

outb to 0xEB is a typical delay, and has proven to be not that useful in 
reality. I still need to fix up my p5a code too.

The second is: now the code makes sense. Instead of writing to random 
unknown io addresses, we now know exactly what it is we are doing.

I am trying to move all my board enables in this direction; where 
possible try to find out where the io address comes from and then try to 
find out exactly what it is that is happening.

But yeah, this is why i wanted this code to be tested.

Luc Verhaegen.

[Carl-Daniel Hailfinger]

On 06.10.2009 16:54, Luc Verhaegen wrote:
> On Tue, Oct 06, 2009 at 04:27:52PM +0200, Carl-Daniel Hailfinger wrote:
> > On 07.07.2009 15:10, Luc Verhaegen wrote:
> > > /* dual function that need special enable. */
> > > if ((gpo >= 22) && (gpo <= 26)) {
> > > tmp = pci_read_long(dev, 0xB0); /* GENCFG */
> > > switch (gpo) {
> > > case 22: /* XBUS: XDIR#/GPO22 */
> > > case 23: /* XBUS: XOE#/GPO23 */
> > > 	tmp |= 1 << 28;
> > > 	break;
> > > case 24: /* RTCSS#/GPO24 */
> > > 	tmp |= 1 << 29;
> > > 	break;
> > > case 25: /* RTCALE/GPO25 */
> > > 	tmp |= 1 << 30;
> > > 	break;
> > > case 26: /* KBCSS#/GPO26 */
> > > 	tmp |= 1 << 31;
> > > 	break;
> > > default:
> >
> > If the default case triggers, you found a compiler bug. Remove.
>
> Sure.
> > > 	fprintf(stderr, "\nERROR: Unsupported PIIX4 GPO%d.\n", gpo);
> > > 	return -1;
> > > }
> > > pci_write_long(dev, 0xB0, tmp);
> > > }
> > > /* GPO {0,8,27,28,30} are always available. */
> > > dev = pci_dev_find(0x8086, 0x7113);	/* Intel PIIX4 PM */
> > > if (!dev) {
> > > fprintf(stderr, "\nERROR: Intel PIIX4 PM not found.\n");
> > > return -1;
> > > }
> > > /* PM IO base */
> > > base = pci_read_long(dev, 0x40) & 0x0000FFC0;
> > > tmp = INL(base + 0x34); /* GPO register */
> > > if (raise)
> > > tmp |= 0x01 << gpo;
> > > else
> > > tmp |= ~(0x01 << gpo);
> > > OUTL(tmp, base + 0x34);
> > > return 0;
> > >
> > > +}
> > >
> > > +/**
> > > Suited for VIAs EPIA M and MII, and maybe other CLE266 based EPIAs.
> > > We don't need to do this when using coreboot, GPIO15 is never lowered there.
> > >
> > > @@ -440,18 +512,7 @@
> > >   */
> > >  static int board_epox_ep_bx3(const char *name)
> > >  {
> > > uint8_t tmp;
> > > /* Raise GPIO22. */
> > > tmp = INB(0x4036);
> > > OUTB(tmp, 0xEB);
> > > tmp |= 0x40;
> > > OUTB(tmp, 0x4036);
> > > OUTB(tmp, 0xEB);
> > > return 0;
> > > return intel_piix4_gpo_set(22, 1);
> > >
> > > }
> > >
> > > /**
> >
> > Ahem. This patch is not functionally equivalent. Not at all.
> >
> > The old code:
> > Read 8-bit value from port 0x4036. Write said value to port 0xeb. Set
> >
> > bit 6 in value. Write value again to port 0xeb and write value to port
> > 0x4036. No config space accesses.
> >
> > The new code:
> > Set bit 28 of 32-bit value in PCI config space of PIIX4 ISA at 0xb0.
> >
> > Get PMBASE from PCI config space of PIIX4 PM. Read 32-bit value from
> > port PMBASE+0x34. Set bit 22 in value. Write value back to PMBASE+0x34.
> > Assuming PMBASE=0x4000, this is equivalent to setting bit 6 in the 8-bit
> > value at port 0x4036.
> >
> > Difference:
> > Port 0xeb is not written or read in the new code.
> > PCI config space of PIIX4 ISA at 0xb0 was not read or written in the
> >
> > old code.
> >
> > I'd like to see an explanation for this.
> >
> > Regards,
> > Carl-Daniel
>
> outb to 0xEB is a typical delay, and has proven to be not that useful in 
> reality. I still need to fix up my p5a code too.
>
> The second is: now the code makes sense. Instead of writing to random 
> unknown io addresses, we now know exactly what it is we are doing.
>
> I am trying to move all my board enables in this direction; where 
> possible try to find out where the io address comes from and then try to 
> find out exactly what it is that is happening.
>
> But yeah, this is why i wanted this code to be tested.

Given that Robert probably won't test any time soon, I'd say go ahead.
Please make sure you remember to kill the default case in switch() as
mentioned above, and it would be great if you could include my
"Difference" paragraph and your explanation for it in the commit message
so the discussion is not lost.

Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[Luc Verhaegen]

On Thu, Oct 22, 2009 at 05:23:56PM +0200, Carl-Daniel Hailfinger wrote:
> Given that Robert probably won't test any time soon, I'd say go ahead.
> Please make sure you remember to kill the default case in switch() as
> mentioned above, and it would be great if you could include my
> "Difference" paragraph and your explanation for it in the commit message
> so the discussion is not lost.
>
> Acked-by: Carl-Daniel Hailfinger
>
> Regards,
> Carl-Daniel

Done and done. -> r803

Thanks,

Luc Verhaegen.