On Mon, 2014-01-13 at 11:31 -0500, Kevin O'Connor wrote:
On Thu, Jan 02, 2014 at 07:00:45PM +0200, Marcel Apfelbaum wrote:
On resume, the OS queries the power management event that caused it. In order to complete this task, it executes some reads to the piix pm io space. This all happens before the OS has a chance to restore the PCI config space for devices, so it is bios's responsibility to make sure the pm IO space is configured correctly. (During suspend, the piix pm configuration space is lost).
Note: For 'ordinary' pci devices the config space is saved by the OS on sleep and restored on resume.
Signed-off-by: Marcel Apfelbaum marcel.a@redhat.com
This patch is based on Michael S. Tsirkin's patch: [SeaBIOS] [PATCH] seabios: call pci_init_device on resume If Michael agrees with it, I'll add him to signed-off section.
Notes:
- Without this patch the OS gets stuck because it tries repeatedly to read/write to pm io space, but the memory region is not enabled, so -1 is returned.
- After resume the OS does not actually use the pm base address configured by the bios to get the IO ports, but uses the value from the ACPI FADT table actually. However, as a side effect of the configuration, the pm-io space is enabled by Qemu and the OS can continue the the boot sequence.
- Bioses used for hardware like coreboot have the same init sequence for piix, see enable_pm from src/southbridge/intel/i82371eb/early_pm.c.
Thanks. SeaBIOS isn't responsible for PCI setup on CSM/coreboot, so the patch must check for CONFIG_QEMU.
Sure thing, Sorry I missed that, I'll add it to V2.
Also, I think we can simplify this a bit - how about the patch below (untested)?
Hi Kevin, I followed the patch and indeed it is smaller and it does the job, but - We also have a q35 chipset and we'll need a global variable also for it. - If we will have other devices that need special attention on resume we will be ready for them (low chance, but you never know). - Finally, the pci_resume will look a little strange and unclear with the new "if" statements (that means, this is the board and we saved the value)
What do you think? Thanks, Marcel
-Kevin
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index a24b8ff..1c90059 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -230,10 +230,8 @@ static void apple_macio_setup(struct pci_device *pci, void *arg) pci_set_io_region_addr(pci, 0, 0x80800000, 0); }
-/* PIIX4 Power Management device (for ACPI) */ -static void piix4_pm_setup(struct pci_device *pci, void *arg) +static void piix4_pm_config_setup(u16 bdf) {
- u16 bdf = pci->bdf; // acpi sci is hardwired to 9 pci_config_writeb(bdf, PCI_INTERRUPT_LINE, 9);
@@ -241,6 +239,15 @@ static void piix4_pm_setup(struct pci_device *pci, void *arg) pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */ pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1); pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */ +}
+static int PiixPMBDF = -1;
+/* PIIX4 Power Management device (for ACPI) */ +static void piix4_pm_setup(struct pci_device *pci, void *arg) +{
PiixPMBDF = pci->bdf;
piix4_pm_config_setup(pci->bdf);
acpi_pm1a_cnt = PORT_ACPI_PM_BASE + 0x04; pmtimer_setup(PORT_ACPI_PM_BASE + 0x08);
@@ -858,3 +865,12 @@ pci_setup(void)
pci_enable_default_vga();}
+void +pci_resume(void) +{
- if (!CONFIG_QEMU)
return;- if (PiixPMBDF >= 0)
piix4_pm_config_setup(PiixPMBDF);+} diff --git a/src/resume.c b/src/resume.c index d69429c..0c0e60e 100644 --- a/src/resume.c +++ b/src/resume.c @@ -100,6 +100,7 @@ s3_resume(void)
pic_setup(); smm_setup();
pci_resume();
s3_resume_vga();
diff --git a/src/util.h b/src/util.h index 1b7d525..b8acbfa 100644 --- a/src/util.h +++ b/src/util.h @@ -99,6 +99,7 @@ void mtrr_setup(void); // fw/pciinit.c extern const u8 pci_irqs[4]; void pci_setup(void); +void pci_resume(void);
// fw/pirtable.c extern struct pir_header *PirAddr;