Re: [flashrom] [patch] add intel_piix4_gpo_set()

[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