[SeaBIOS] [PATCH] seabios: restore piix pm config registers after resume

[Michael S. Tsirkin]

On Mon, Jan 13, 2014 at 11:31:20AM -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 at 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.
> 
> Also, I think we can simplify this a bit - how about the patch below
> (untested)?
> 
> -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)

so this does nothing unless piix4_pm_setup run but
if it did why do we need to resume anything?

> +        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;