Change in coreboot[master]: sb/intel/i82371eb: Add support for reconfiguring GPO22/23

[Keith Hui (Code Review)]

Keith Hui has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

sb/intel/i82371eb: Add support for reconfiguring GPO22/23

XOE# and XDIR# can be used as GPOs 23/22 if X-Bus functionality is not
required. Turns out asus/p2b-ls is using them to control termination
for the onboard SCSI buses. Add support to allow this reconfiguration.

Change-Id: I2dab6fafbd67a98ed1cac1ffcf9352be4a87c3e9
Signed-off-by: Keith Hui 
---
M src/mainboard/asus/p2b-ls/devicetree.cb
M src/southbridge/intel/i82371eb/chip.h
M src/southbridge/intel/i82371eb/isa.c
3 files changed, 15 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/38352/1

diff --git a/src/mainboard/asus/p2b-ls/devicetree.cb b/src/mainboard/asus/p2b-ls/devicetree.cb
index a9901b4..5e55278 100644
--- a/src/mainboard/asus/p2b-ls/devicetree.cb
+++ b/src/mainboard/asus/p2b-ls/devicetree.cb
@@ -8,6 +8,10 @@
     device pci 0.0 on end		# Host bridge
     device pci 1.0 on end		# PCI/AGP bridge
     chip southbridge/intel/i82371eb	# Southbridge
+      register "gpo" = "0x7fbfb9ff"
+      register "gpo22_enable" = "1"
+      register "lid_polarity" = "1"
+      register "thrm_polarity" = "1"
       device pci 4.0 on			# ISA bridge
         chip superio/winbond/w83977tf	# Super I/O (FIXME: It's W83977EF!)
           device pnp 3f0.0 on		# Floppy
diff --git a/src/southbridge/intel/i82371eb/chip.h b/src/southbridge/intel/i82371eb/chip.h
index 28975a2..5b23ab1 100644
--- a/src/southbridge/intel/i82371eb/chip.h
+++ b/src/southbridge/intel/i82371eb/chip.h
@@ -28,6 +28,10 @@
    int ide1_drive1_udma33_enable:1;
    int ide_legacy_enable:1;
    int usb_enable:1;
+	int gpo22_enable:1; /* GPO22/GPO23 (1) vs. XDIR#/XOE# (0) */
+	int gpo22:1;
+	int gpo23:1;
+	int pad:5;
    /* acpi */
    u32 gpo; /* gpio output default */
    u8 lid_polarity;
diff --git a/src/southbridge/intel/i82371eb/isa.c b/src/southbridge/intel/i82371eb/isa.c
index bb88f7d..fd63440 100644
--- a/src/southbridge/intel/i82371eb/isa.c
+++ b/src/southbridge/intel/i82371eb/isa.c
@@ -28,6 +28,7 @@
 #include <arch/acpigen.h>
 #endif
 #include "i82371eb.h"
+#include "chip.h"

#if CONFIG(IOAPIC)
 static void enable_intel_82093aa_ioapic(void)
@@ -63,6 +64,7 @@
 static void isa_init(struct device *dev)
 {
    u32 reg32;
+	struct southbridge_intel_i82371eb_config *sb = dev->chip_info;

/* Initialize the real time clock (RTC). */
    cmos_init(0);
@@ -77,9 +79,13 @@
    /*
     * The PIIX4 can support the full ISA bus, or the Extended I/O (EIO)
     * bus, which is a subset of ISA. We select the full ISA bus here.
+	 * Some boards use GPO22/23. That will be taken care of as well.
     */
    reg32 = pci_read_config32(dev, GENCFG);
-	reg32 |= ISA;	/* Select ISA, not EIO. */
+
+	reg32 |= ISA |	/* Select ISA, not EIO. */
+		(sb->gpo22_enable << 28); /* Select GPO22/23 if configured. */
+
    pci_write_config16(dev, GENCFG, reg32);

/* Initialize ISA DMA. */

[Keith Hui (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38352

to look at the new patch set (#2).

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

sb/intel/i82371eb: Add support for reconfiguring GPO22/23

XOE# and XDIR# can be used as GPOs 23/22 if X-Bus functionality is not
required. Turns out asus/p2b-ls is using them to control termination
for the onboard SCSI buses. Add support to allow this reconfiguration.

Change-Id: I2dab6fafbd67a98ed1cac1ffcf9352be4a87c3e9
Signed-off-by: Keith Hui 
---
M src/southbridge/intel/i82371eb/chip.h
M src/southbridge/intel/i82371eb/isa.c
2 files changed, 11 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/38352/2

[Keith Hui (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38352

to look at the new patch set (#3).

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

sb/intel/i82371eb: Add support for reconfiguring GPO22/23

XOE# and XDIR# can be used as GPOs 23/22 if X-Bus functionality is not
required. Turns out asus/p2b-ls is using them to control termination
for the onboard SCSI buses. Add support to allow this reconfiguration.

Change-Id: I2dab6fafbd67a98ed1cac1ffcf9352be4a87c3e9
Signed-off-by: Keith Hui 
---
M src/southbridge/intel/i82371eb/chip.h
M src/southbridge/intel/i82371eb/isa.c
2 files changed, 12 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/38352/3

[Idwer Vollering (Code Review)]

Idwer Vollering has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

Patch Set 3: Code-Review+2

I see you finally found out how to bring up SCSI!

[Keith Hui (Code Review)]

Hello Patrick Rudolph, Idwer Vollering, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38352

to look at the new patch set (#4).

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

sb/intel/i82371eb: Add support for reconfiguring GPO22/23

XOE# and XDIR# can be used as GPOs 23/22 if X-Bus functionality is not
required. Turns out asus/p2b-ls is using them to control termination
for the onboard SCSI buses. Add support to allow this reconfiguration.

Change-Id: I2dab6fafbd67a98ed1cac1ffcf9352be4a87c3e9
Signed-off-by: Keith Hui 
---
M src/southbridge/intel/i82371eb/chip.h
M src/southbridge/intel/i82371eb/isa.c
2 files changed, 11 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/38352/4

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

Patch Set 4: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
File src/southbridge/intel/i82371eb/chip.h:

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
PS4, Line 32: int gpo22:1;
            : 	int gpo23:1;
            : 	int pad:5;
These seem to be unused

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
File src/southbridge/intel/i82371eb/isa.c:

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
PS4, Line 89: 32
This has been switched to 32 bits, I guess the previous 16-bit write was a bug?

[Keith Hui (Code Review)]

Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
File src/southbridge/intel/i82371eb/chip.h:

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
PS4, Line 32: int gpo22:1;
            : 	int gpo23:1;
            : 	int pad:5;
> These seem to be unused

pad is padding to whole bytes.

gpo22 and gpo23 are future provisions to allow controlling asus/p2b-ls SCSI bus termination via nvram options. Option table for that board is a todo, maybe in a big push to Make P2B-LS Great Again...

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
File src/southbridge/intel/i82371eb/isa.c:

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
PS4, Line 89: 32
> This has been switched to 32 bits, I guess the previous 16-bit write was a bug?

Previously only the low 16 bits were changed so that worked. Now this has to be 32-bit because bit 28 also changed.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
File src/southbridge/intel/i82371eb/chip.h:

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
PS4, Line 32: int gpo22:1;
            : 	int gpo23:1;
            : 	int pad:5;
> pad is padding to whole bytes. […]

I don't think padding is required. After all, the struct isn't used as a whole. Also, the struct isn't packed, so the padding would be added automatically.

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
File src/southbridge/intel/i82371eb/isa.c:

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
PS4, Line 89: 32
> Previously only the low 16 bits were changed so that worked. […]

Ack

[Keith Hui (Code Review)]

Hello Patrick Rudolph, Angel Pons, Idwer Vollering, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38352

to look at the new patch set (#5).

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

sb/intel/i82371eb: Add support for reconfiguring GPO22/23

XOE# and XDIR# can be used as GPOs 23/22 if X-Bus functionality is not
required. Turns out asus/p2b-ls is using them to control termination
for the onboard SCSI buses. Add support to allow this reconfiguration.

Change-Id: I2dab6fafbd67a98ed1cac1ffcf9352be4a87c3e9
Signed-off-by: Keith Hui 
---
M src/southbridge/intel/i82371eb/chip.h
M src/southbridge/intel/i82371eb/isa.c
2 files changed, 10 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/38352/5

[Keith Hui (Code Review)]

Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
File src/southbridge/intel/i82371eb/isa.c:

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
PS4, Line 89: 32
> Previously only the low 16 bits were changed so that worked. […]

Done

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

Patch Set 5: Code-Review+1

Nice!

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

Patch Set 5: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
File src/southbridge/intel/i82371eb/chip.h:

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
PS4, Line 32: int gpo22:1;
            : 	int gpo23:1;
            : 	int pad:5;
> I don't think padding is required. After all, the struct isn't used as a whole. […]

The compiler will automatically layout the struct, and we don't care about the layout a lot (because it's never sent into hardware verbatim) so no padding needed here.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

Patch Set 5: Code-Review+1

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38352/8//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38352/8//COMMIT_MSG@7 
PS8, Line 7: Add support for reconfiguring GPO22/23
For the future, instead of *Add support* you can use the shorter *Support*.
> Support reconfiguring GPO22/23

Though I am not native speaker.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Add support for reconfiguring GPO22/23
......................................................................

Patch Set 8: Code-Review+2

(2 comments)

https://review.coreboot.org/c/coreboot/+/38352/8//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38352/8//COMMIT_MSG@7 
PS8, Line 7: Add support for reconfiguring GPO22/23
> For the future, instead of *Add support* you can use the shorter *Support*. […]

"Add support for" sounds more natural to me.

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
File src/southbridge/intel/i82371eb/chip.h:

https://review.coreboot.org/c/coreboot/+/38352/4/src/southbridge/intel/i8237... 
PS4, Line 32: int gpo22:1;
            : 	int gpo23:1;
            : 	int pad:5;
> The compiler will automatically layout the struct, and we don't care about the layout a lot (because […]

Done

Change in coreboot[master]: sb/intel/i82371eb: Support reconfiguring GPO22/23

[Keith Hui (Code Review)]

Hello Patrick Rudolph, Angel Pons, Idwer Vollering, Paul Menzel, build bot (Jenkins), Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38352

to look at the new patch set (#10).

Change subject: sb/intel/i82371eb: Support reconfiguring GPO22/23
......................................................................

sb/intel/i82371eb: Support reconfiguring GPO22/23

XOE# and XDIR# can be used as GPOs 23/22 if X-Bus functionality is not
required. Turns out asus/p2b-ls is using them to control termination
for the onboard SCSI buses. Add support to allow this reconfiguration.

Change-Id: I2dab6fafbd67a98ed1cac1ffcf9352be4a87c3e9
Signed-off-by: Keith Hui 
---
M src/southbridge/intel/i82371eb/chip.h
M src/southbridge/intel/i82371eb/isa.c
2 files changed, 9 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/38352/10

Change in coreboot[master]: sb/intel/i82371eb: Support reconfiguring GPO22/23

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Support reconfiguring GPO22/23
......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38352/8//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38352/8//COMMIT_MSG@7 
PS8, Line 7: Add support for reconfiguring GPO22/23
> "Add support for" sounds more natural to me.

Ack

Change in coreboot[master]: sb/intel/i82371eb: Support reconfiguring GPO22/23

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Support reconfiguring GPO22/23
......................................................................

Patch Set 10: Code-Review+2

Change in coreboot[master]: sb/intel/i82371eb: Support reconfiguring GPO22/23

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Support reconfiguring GPO22/23
......................................................................

sb/intel/i82371eb: Support reconfiguring GPO22/23

XOE# and XDIR# can be used as GPOs 23/22 if X-Bus functionality is not
required. Turns out asus/p2b-ls is using them to control termination
for the onboard SCSI buses. Add support to allow this reconfiguration.

Change-Id: I2dab6fafbd67a98ed1cac1ffcf9352be4a87c3e9
Signed-off-by: Keith Hui 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38352
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Georgi 
---
M src/southbridge/intel/i82371eb/chip.h
M src/southbridge/intel/i82371eb/isa.c
2 files changed, 9 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved

diff --git a/src/southbridge/intel/i82371eb/chip.h b/src/southbridge/intel/i82371eb/chip.h
index 28975a2..9862082 100644
--- a/src/southbridge/intel/i82371eb/chip.h
+++ b/src/southbridge/intel/i82371eb/chip.h
@@ -28,6 +28,9 @@
    int ide1_drive1_udma33_enable:1;
    int ide_legacy_enable:1;
    int usb_enable:1;
+	int gpo22_enable:1; /* GPO22/GPO23 (1) vs. XDIR#/XOE# (0) */
+	int gpo22:1;
+	int gpo23:1;
    /* acpi */
    u32 gpo; /* gpio output default */
    u8 lid_polarity;
diff --git a/src/southbridge/intel/i82371eb/isa.c b/src/southbridge/intel/i82371eb/isa.c
index bb88f7d..a57d61a 100644
--- a/src/southbridge/intel/i82371eb/isa.c
+++ b/src/southbridge/intel/i82371eb/isa.c
@@ -28,6 +28,7 @@
 #include <arch/acpigen.h>
 #endif
 #include "i82371eb.h"
+#include "chip.h"

#if CONFIG(IOAPIC)
 static void enable_intel_82093aa_ioapic(void)
@@ -63,6 +64,7 @@
 static void isa_init(struct device *dev)
 {
    u32 reg32;
+	struct southbridge_intel_i82371eb_config *sb = dev->chip_info;

/* Initialize the real time clock (RTC). */
    cmos_init(0);
@@ -80,7 +82,10 @@
     */
    reg32 = pci_read_config32(dev, GENCFG);
    reg32 |= ISA;	/* Select ISA, not EIO. */
-	pci_write_config16(dev, GENCFG, reg32);
+
+	/* Some boards use GPO22/23. Select it if configured. */
+	reg32 = ONOFF(sb->gpo22_enable, reg32, GPO2223);
+	pci_write_config32(dev, GENCFG, reg32);

/* Initialize ISA DMA. */
    isa_dma_init();

Change in coreboot[master]: sb/intel/i82371eb: Support reconfiguring GPO22/23

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38352 )

Change subject: sb/intel/i82371eb: Support reconfiguring GPO22/23
......................................................................

Patch Set 11:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1050
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1049
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1048

Please note: This test is under development and might not be accurate at all!