HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40741 )
Change subject: sb/intel/i82801gx: Fix 16-bit PCI_COMMAND register ......................................................................
sb/intel/i82801gx: Fix 16-bit PCI_COMMAND register
Change-Id: I11b8743234cb1292db8c930edecf8fb5c47d63fd Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/southbridge/intel/i82801gx/azalia.c M src/southbridge/intel/i82801gx/i82801gx.c M src/southbridge/intel/i82801gx/ide.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801gx/usb.c M src/southbridge/intel/i82801gx/usb_ehci.c 6 files changed, 20 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/40741/1
diff --git a/src/southbridge/intel/i82801gx/azalia.c b/src/southbridge/intel/i82801gx/azalia.c index 4a2b50e..775326c 100644 --- a/src/southbridge/intel/i82801gx/azalia.c +++ b/src/southbridge/intel/i82801gx/azalia.c @@ -230,8 +230,7 @@ pci_write_config32(dev, 0x120, reg32);
/* Set Bus Master */ - reg32 = pci_read_config32(dev, PCI_COMMAND); - pci_write_config32(dev, PCI_COMMAND, reg32 | PCI_COMMAND_MASTER); + pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER);
pci_write_config8(dev, 0x3c, 0x0a); // unused?
diff --git a/src/southbridge/intel/i82801gx/i82801gx.c b/src/southbridge/intel/i82801gx/i82801gx.c index 1a5366f..77161f5 100644 --- a/src/southbridge/intel/i82801gx/i82801gx.c +++ b/src/southbridge/intel/i82801gx/i82801gx.c @@ -54,23 +54,23 @@
void i82801gx_enable(struct device *dev) { - u32 reg32; + u16 reg16;
if (!dev->enabled) { printk(BIOS_DEBUG, "%s: Disabling device\n", dev_path(dev));
/* Ensure memory, io, and bus master are all disabled */ - reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO); - pci_write_config32(dev, PCI_COMMAND, reg32); + reg16 = pci_read_config16(dev, PCI_COMMAND); + reg16 &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO); + pci_write_config16(dev, PCI_COMMAND, reg16);
/* Hide this device if possible */ ich_hide_devfn(dev->path.pci.devfn); } else { /* Enable SERR */ - reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_SERR; - pci_write_config32(dev, PCI_COMMAND, reg32); + reg16 = pci_read_config16(dev, PCI_COMMAND); + reg16 |= PCI_COMMAND_SERR; + pci_write_config16(dev, PCI_COMMAND, reg16);
if (dev->path.pci.devfn == PCI_DEVFN(31, 2)) { printk(BIOS_DEBUG, "Set SATA mode early\n"); diff --git a/src/southbridge/intel/i82801gx/ide.c b/src/southbridge/intel/i82801gx/ide.c index b6b30ef..dadcbb9 100644 --- a/src/southbridge/intel/i82801gx/ide.c +++ b/src/southbridge/intel/i82801gx/ide.c @@ -14,6 +14,7 @@ static void ide_init(struct device *dev) { u16 ideTimingConfig; + u16 reg16; u32 reg32; u32 enable_primary, enable_secondary;
@@ -30,8 +31,8 @@ enable_secondary = config->ide_enable_secondary; }
- reg32 = pci_read_config32(dev, PCI_COMMAND); - pci_write_config32(dev, PCI_COMMAND, reg32 | PCI_COMMAND_IO | PCI_COMMAND_MASTER); + reg16 = pci_read_config16(dev, PCI_COMMAND); + pci_write_config16(dev, PCI_COMMAND, reg16 | PCI_COMMAND_IO | PCI_COMMAND_MASTER);
/* Native Capable, but not enabled. */ pci_write_config8(dev, 0x09, 0x8a); diff --git a/src/southbridge/intel/i82801gx/pcie.c b/src/southbridge/intel/i82801gx/pcie.c index 4398ad5..4de62e2 100644 --- a/src/southbridge/intel/i82801gx/pcie.c +++ b/src/southbridge/intel/i82801gx/pcie.c @@ -47,9 +47,7 @@ printk(BIOS_DEBUG, "Initializing ICH7 PCIe bridge.\n");
/* Enable Bus Master */ - reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER; - pci_write_config32(dev, PCI_COMMAND, reg32); + pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER);
/* Set Cache Line Size to 0x10 */ // This has no effect but the OS might expect it diff --git a/src/southbridge/intel/i82801gx/usb.c b/src/southbridge/intel/i82801gx/usb.c index d4b559a..0a0c8ae 100644 --- a/src/southbridge/intel/i82801gx/usb.c +++ b/src/southbridge/intel/i82801gx/usb.c @@ -10,14 +10,14 @@
static void usb_init(struct device *dev) { - u32 reg32; + u16 reg16; u8 reg8;
/* USB Specification says the device must be Bus Master */ printk(BIOS_DEBUG, "UHCI: Setting up controller.. ");
- reg32 = pci_read_config32(dev, PCI_COMMAND); - pci_write_config32(dev, PCI_COMMAND, reg32 | PCI_COMMAND_MASTER); + reg16 = pci_read_config16(dev, PCI_COMMAND); + pci_write_config16(dev, PCI_COMMAND, reg16 | PCI_COMMAND_MASTER);
// Erratum pci_write_config8(dev, 0xca, 0x00); diff --git a/src/southbridge/intel/i82801gx/usb_ehci.c b/src/southbridge/intel/i82801gx/usb_ehci.c index d127496..d246de5 100644 --- a/src/southbridge/intel/i82801gx/usb_ehci.c +++ b/src/southbridge/intel/i82801gx/usb_ehci.c @@ -14,14 +14,15 @@ { struct resource *res; u8 *base; + u16 reg16; u32 reg32; u8 reg8;
printk(BIOS_DEBUG, "EHCI: Setting up controller.. "); - reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER; - reg32 |= PCI_COMMAND_SERR; - pci_write_config32(dev, PCI_COMMAND, reg32); + reg16 = pci_read_config16(dev, PCI_COMMAND); + reg16 |= PCI_COMMAND_MASTER; + reg16 |= PCI_COMMAND_SERR; + pci_write_config16(dev, PCI_COMMAND, reg16);
reg32 = pci_read_config32(dev, 0xdc); reg32 |= (1 << 31) | (1 << 27);
Hello build bot (Jenkins), Nico Huber, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40741
to look at the new patch set (#2).
Change subject: sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register ......................................................................
sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register
Change-Id: I11b8743234cb1292db8c930edecf8fb5c47d63fd Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/southbridge/intel/i82801gx/azalia.c M src/southbridge/intel/i82801gx/i82801gx.c M src/southbridge/intel/i82801gx/ide.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801gx/usb.c M src/southbridge/intel/i82801gx/usb_ehci.c 6 files changed, 20 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/40741/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40741 )
Change subject: sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register ......................................................................
Patch Set 2: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40741 )
Change subject: sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register ......................................................................
Patch Set 2: Code-Review+2
would have preferred to review something that gets rid of the unncessary writes ;)
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40741 )
Change subject: sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40741/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/usb_ehci.c:
https://review.coreboot.org/c/coreboot/+/40741/2/src/southbridge/intel/i8280... PS2, Line 25: pci_write_config16(dev, PCI_COMMAND, reg16); why not pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER | PCI_COMMAND_SERR)?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40741 )
Change subject: sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40741/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/usb_ehci.c:
https://review.coreboot.org/c/coreboot/+/40741/2/src/southbridge/intel/i8280... PS2, Line 25: pci_write_config16(dev, PCI_COMMAND, reg16);
why not pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER | PCI_COMMAND_SERR)?
Maybe as a follow-up?
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40741 )
Change subject: sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40741/2/src/southbridge/intel/i8280... File src/southbridge/intel/i82801gx/usb_ehci.c:
https://review.coreboot.org/c/coreboot/+/40741/2/src/southbridge/intel/i8280... PS2, Line 25: pci_write_config16(dev, PCI_COMMAND, reg16);
Maybe as a follow-up?
Done
Hello build bot (Jenkins), Nico Huber, Paul Menzel, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40741
to look at the new patch set (#3).
Change subject: sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register ......................................................................
sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register
Change-Id: I11b8743234cb1292db8c930edecf8fb5c47d63fd Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/southbridge/intel/i82801gx/azalia.c M src/southbridge/intel/i82801gx/i82801gx.c M src/southbridge/intel/i82801gx/ide.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801gx/usb.c M src/southbridge/intel/i82801gx/usb_ehci.c 6 files changed, 16 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/40741/3
Hello build bot (Jenkins), Nico Huber, Paul Menzel, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40741
to look at the new patch set (#4).
Change subject: sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register ......................................................................
sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register
Change-Id: I11b8743234cb1292db8c930edecf8fb5c47d63fd Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M 3rdparty/intel-microcode M src/southbridge/intel/i82801gx/azalia.c M src/southbridge/intel/i82801gx/i82801gx.c M src/southbridge/intel/i82801gx/ide.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801gx/usb.c M src/southbridge/intel/i82801gx/usb_ehci.c 7 files changed, 11 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/40741/4
Hello build bot (Jenkins), Nico Huber, Paul Menzel, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40741
to look at the new patch set (#5).
Change subject: sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register ......................................................................
sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register
Change-Id: I11b8743234cb1292db8c930edecf8fb5c47d63fd Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/southbridge/intel/i82801gx/azalia.c M src/southbridge/intel/i82801gx/i82801gx.c M src/southbridge/intel/i82801gx/ide.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801gx/usb.c M src/southbridge/intel/i82801gx/usb_ehci.c 6 files changed, 10 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/40741/5
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40741 )
Change subject: sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register ......................................................................
Patch Set 5: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40741 )
Change subject: sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register ......................................................................
sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register
Change-Id: I11b8743234cb1292db8c930edecf8fb5c47d63fd Signed-off-by: Elyes HAOUAS ehaouas@noos.fr Reviewed-on: https://review.coreboot.org/c/coreboot/+/40741 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/southbridge/intel/i82801gx/azalia.c M src/southbridge/intel/i82801gx/i82801gx.c M src/southbridge/intel/i82801gx/ide.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801gx/usb.c M src/southbridge/intel/i82801gx/usb_ehci.c 6 files changed, 10 insertions(+), 21 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/southbridge/intel/i82801gx/azalia.c b/src/southbridge/intel/i82801gx/azalia.c index 4a2b50e..775326c 100644 --- a/src/southbridge/intel/i82801gx/azalia.c +++ b/src/southbridge/intel/i82801gx/azalia.c @@ -230,8 +230,7 @@ pci_write_config32(dev, 0x120, reg32);
/* Set Bus Master */ - reg32 = pci_read_config32(dev, PCI_COMMAND); - pci_write_config32(dev, PCI_COMMAND, reg32 | PCI_COMMAND_MASTER); + pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER);
pci_write_config8(dev, 0x3c, 0x0a); // unused?
diff --git a/src/southbridge/intel/i82801gx/i82801gx.c b/src/southbridge/intel/i82801gx/i82801gx.c index 1a5366f..eae16db 100644 --- a/src/southbridge/intel/i82801gx/i82801gx.c +++ b/src/southbridge/intel/i82801gx/i82801gx.c @@ -54,23 +54,21 @@
void i82801gx_enable(struct device *dev) { - u32 reg32; + u16 reg16;
if (!dev->enabled) { printk(BIOS_DEBUG, "%s: Disabling device\n", dev_path(dev));
/* Ensure memory, io, and bus master are all disabled */ - reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO); - pci_write_config32(dev, PCI_COMMAND, reg32); + reg16 = pci_read_config16(dev, PCI_COMMAND); + reg16 &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO); + pci_write_config16(dev, PCI_COMMAND, reg16);
/* Hide this device if possible */ ich_hide_devfn(dev->path.pci.devfn); } else { /* Enable SERR */ - reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_SERR; - pci_write_config32(dev, PCI_COMMAND, reg32); + pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_SERR);
if (dev->path.pci.devfn == PCI_DEVFN(31, 2)) { printk(BIOS_DEBUG, "Set SATA mode early\n"); diff --git a/src/southbridge/intel/i82801gx/ide.c b/src/southbridge/intel/i82801gx/ide.c index b6b30ef..cc3e740 100644 --- a/src/southbridge/intel/i82801gx/ide.c +++ b/src/southbridge/intel/i82801gx/ide.c @@ -30,8 +30,7 @@ enable_secondary = config->ide_enable_secondary; }
- reg32 = pci_read_config32(dev, PCI_COMMAND); - pci_write_config32(dev, PCI_COMMAND, reg32 | PCI_COMMAND_IO | PCI_COMMAND_MASTER); + pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_MASTER);
/* Native Capable, but not enabled. */ pci_write_config8(dev, 0x09, 0x8a); diff --git a/src/southbridge/intel/i82801gx/pcie.c b/src/southbridge/intel/i82801gx/pcie.c index 4398ad5..4de62e2 100644 --- a/src/southbridge/intel/i82801gx/pcie.c +++ b/src/southbridge/intel/i82801gx/pcie.c @@ -47,9 +47,7 @@ printk(BIOS_DEBUG, "Initializing ICH7 PCIe bridge.\n");
/* Enable Bus Master */ - reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER; - pci_write_config32(dev, PCI_COMMAND, reg32); + pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER);
/* Set Cache Line Size to 0x10 */ // This has no effect but the OS might expect it diff --git a/src/southbridge/intel/i82801gx/usb.c b/src/southbridge/intel/i82801gx/usb.c index d4b559a..d8c55e0 100644 --- a/src/southbridge/intel/i82801gx/usb.c +++ b/src/southbridge/intel/i82801gx/usb.c @@ -10,14 +10,12 @@
static void usb_init(struct device *dev) { - u32 reg32; u8 reg8;
/* USB Specification says the device must be Bus Master */ printk(BIOS_DEBUG, "UHCI: Setting up controller.. ");
- reg32 = pci_read_config32(dev, PCI_COMMAND); - pci_write_config32(dev, PCI_COMMAND, reg32 | PCI_COMMAND_MASTER); + pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER);
// Erratum pci_write_config8(dev, 0xca, 0x00); diff --git a/src/southbridge/intel/i82801gx/usb_ehci.c b/src/southbridge/intel/i82801gx/usb_ehci.c index d127496..08211c2 100644 --- a/src/southbridge/intel/i82801gx/usb_ehci.c +++ b/src/southbridge/intel/i82801gx/usb_ehci.c @@ -18,10 +18,7 @@ u8 reg8;
printk(BIOS_DEBUG, "EHCI: Setting up controller.. "); - reg32 = pci_read_config32(dev, PCI_COMMAND); - reg32 |= PCI_COMMAND_MASTER; - reg32 |= PCI_COMMAND_SERR; - pci_write_config32(dev, PCI_COMMAND, reg32); + pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER | PCI_COMMAND_SERR);
reg32 = pci_read_config32(dev, 0xdc); reg32 |= (1 << 31) | (1 << 27);
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40741 )
Change subject: sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register ......................................................................
Patch Set 6:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3032 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3031 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3030 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3029
Please note: This test is under development and might not be accurate at all!
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40741 )
Change subject: sb/intel/i82801gx: Fix 16-bit read/write PCI_COMMAND register ......................................................................
Patch Set 6:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3038 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3037 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3036 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3035
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
HAOUAS Elyes (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)