[Patch] Flashrom: board enable for Soyo SY-6BA+III.

[Andrew Morgan]

Acked-by: Andrew Morgan

Some output below, it wrote the ROM just fine. :)

flashrom v0.9.0-r601
No coreboot table found.
Found chipset "Intel PIIX4/4E/4M", enabling flash write... OK.
Calibrating delay loop... OK.
Found chip "SST SST39SF020A" (256 KB) at physical address 0xfffc0000.
Flash image seems to be a legacy BIOS. Disabling checks.
Writing flash chip... Programming page: 0063 at address: 0x0003f000
COMPLETE.

[Uwe Hermann]

On Thu, Jun 18, 2009 at 02:47:33AM +0200, Luc Verhaegen wrote:
> Board enable for soyo sy-6ba+iii.
>
> This board enable drops GPO26 on the southbridge (PIIX4). Since there
> was another board raising GPO22 on the same southbridge, a general
> routine was created reducing both board enables to a single line.
>
> This routine does full gpo pin checking and filters out those which
> are most likely in use already, enables dual-function gpo pins properly,
> and then sets the relevant gpo line. It should be fully implemented for
> all gpo setting on PIIX4, except when a supposed used gpo pin does happen
> to be available for gpio.
>
> The board itself predates proper subsystem id usage and can therefor not
> be autodetected.
>
> Signed-off-by: Luc Verhaegen

With the changes below:
Acked-by: Uwe Hermann
> Helper function to raise/drop a given gpo line on intel PIIX4*
> +static int intel_piix4_gpo_set(int gpo, int raise)

Rename to *_piix4x_* maybe, to make it clear that PIIX4E and PIIX4M are
also supported.

'gpo' should be 'unsigned int' I guess, there are no negative GPOs.
> dev = pci_dev_find(0x8086, 0x7110);	/* Intel PIIX4 ISA bridge */

"PIIX4{,E,M}" please, to be explicit about what is supported.
> if (!dev) {
> fprintf(stderr, "\nERROR: Intel PIIX4 ISA bridge not found.\n");

PIIX4{,E,M}
> /* sanity check */
> if ((gpo < 0) || (gpo > 30)) {

You can drop the < 0 check by making gpo 'unsigned int'.
> fprintf(stderr, "\nERROR: Intel PIIX4 has no GPO%d.\n", gpo);

PIIX4{,E,M}
> 	tmp |= 0x10000000;
> 	break;
> case 24: /* RTCSS#/GPO24 */
> 	tmp |= 0x20000000;
> 	break;
> case 25: /* RTCALE/GPO25 */
> 	tmp |= 0x40000000;
> 	break;
> case 26: /* KBCSS#/GPO26 */
> 	tmp |= 0x80000000;

Please consider using
    		tmp |= (1 << 31);
etc. as that is (IMHO) much easier to read. In the 0x0x80000000 case you have
to first count the zeros (and concentrate while doing so, in order not
to count too many/few) and then fire up the HEX2BIN converter in
your brain in order to figure out which bit this line is actually trying
to set. The (1 << 31) form is much nicer on the eye and brain.
> 	break;
> default:
> 	fprintf(stderr, "\nERROR: Unsupported PIIX4 GPO%d.\n", gpo);

PIIX4{,E,M}
> dev = pci_dev_find(0x8086, 0x7113);	/* Intel PIIX4 PM */

PIIX4{,E,M}
> if (!dev) {
> fprintf(stderr, "\nERROR: Intel PIIX4 PM not found.\n");

PIIX4{,E,M}
> 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.
>
> @@ -413,18 +485,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);

Nice!

This also makes the board_epox_ep_bx3() more generic, as the 0x4036
address might not always be 0x4036 if $SOMETHING (BIOS? OS?) decideѕ
to map it elsewhere, correct?

Uwe.

[Peter Stuge]

Uwe Hermann wrote:
> This also makes the board_epox_ep_bx3() more generic, as the 0x4036
> address might not always be 0x4036 if $SOMETHING (BIOS? OS?) decideѕ
> to map it elsewhere, correct?

Yep.

//Peter

[Luc Verhaegen]

On Thu, Jun 18, 2009 at 01:54:32PM +0200, Uwe Hermann wrote:
> On Thu, Jun 18, 2009 at 02:47:33AM +0200, Luc Verhaegen wrote:
> > Board enable for soyo sy-6ba+iii.
> >
> > This board enable drops GPO26 on the southbridge (PIIX4). Since there
> > was another board raising GPO22 on the same southbridge, a general
> > routine was created reducing both board enables to a single line.
> >
> > This routine does full gpo pin checking and filters out those which
> > are most likely in use already, enables dual-function gpo pins properly,
> > and then sets the relevant gpo line. It should be fully implemented for
> > all gpo setting on PIIX4, except when a supposed used gpo pin does happen
> > to be available for gpio.
> >
> > The board itself predates proper subsystem id usage and can therefor not
> > be autodetected.
> >
> > Signed-off-by: Luc Verhaegen
>
> With the changes below:
> Acked-by: Uwe Hermann

Sadly, we then found out on irc that this was not needed. I should chase 
down the owner of the epox board and see wether he can test this.

Thanks for the thorough review though, i hope the main function can 
still make it in.
> Nice!
>
> This also makes the board_epox_ep_bx3() more generic, as the 0x4036
> address might not always be 0x4036 if $SOMETHING (BIOS? OS?) decide??
> to map it elsewhere, correct?

Yup, it is a bios decision, so it could be that 0x4000 never moves, but 
other boards can then also use this board enable when they raise the 
same pin.

Luc Verhaegen.