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

[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.