[U-Boot] [PATCH 2/4] x86: qemu: setup PM IO base for ACPI in southbridge

[Bin Meng]

Hi Miao,

On Tue, Jan 19, 2016 at 10:46 AM, Miao Yan <yanmiaobest at gmail.com> wrote:
> Hi Bin,
>
> 2016-01-16 21:23 GMT+08:00 Bin Meng <bmeng.cn at gmail.com>:
>> Hi Miao,
>>
>> On Fri, Jan 15, 2016 at 11:12 AM, Miao Yan <yanmiaobest at gmail.com> wrote:
>>> Enable ACPI IO space for piix4 (for pc board) and ich9 (for q35 board)
>>>
>>> Signed-off-by: Miao Yan <yanmiaobest at gmail.com>
>>> ---
>>>  arch/x86/cpu/qemu/qemu.c                | 39 +++++++++++++++++++++++++++++++++
>>>  arch/x86/include/asm/arch-qemu/device.h |  8 +++++++
>>>  2 files changed, 47 insertions(+)
>>>
>>> diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c
>>> index 46111c9..e7d8a6c 100644
>>> --- a/arch/x86/cpu/qemu/qemu.c
>>> +++ b/arch/x86/cpu/qemu/qemu.c
>>> @@ -15,6 +15,41 @@
>>>
>>>  static bool i440fx;
>>>
>>> +static void enable_pm_piix(void)
>>> +{
>>> +       u8 en;
>>> +       u16 device, cmd;
>>> +
>>> +       device = x86_pci_read_config16(PIIX_PM, PCI_DEVICE_ID);
>>> +       if (device != PCI_DEVICE_ID_INTEL_82371AB_3)
>>> +               return;
>>
>> Guess the check is already covered in qemu_chipset_init().
>
>
> Do you mean this check ?
>
>     device = x86_pci_read_config16(PCI_BDF(0, 0, 0), PCI_DEVICE_ID);
>     i440fx = (device == PCI_DEVICE_ID_INTEL_82441);
>
> So is it guaranteed that PIIX_PM is always on that BDF ?

I believe so. If you look at the codes in qemu.c, the variable "static
bool i440fx" is used to distinguish QEMU machine pc and q35. It does
not check whether the chipset is i440fx, or PIIX which is the chipset
connected to i440fx.

>
> IMO, we are operating on another chipset, and we better make
> sure it's the one we expect, besides, an extra check won't do any harm.
>

Yes, that makes sense. So if we go with your way, maybe we need expand
"static bool i440fx" to multiple variables and use correct variable to
check? But this looks a bit complex than a single variable.

>
>>
>>> +
>>> +       /* Set the PM I/O base. */
>>
>> nits: please remove the ending period. Please fix this globally in this file.
>>
>>> +       x86_pci_write_config32(PIIX_PM, PMBA, DEFAULT_PMBASE | 1);
>>> +
>>> +       /* Enable access to the PM I/O space. */
>>> +       cmd = x86_pci_read_config16(PIIX_PM, PCI_COMMAND);
>>> +       cmd |= PCI_COMMAND_IO;
>>> +       x86_pci_write_config16(PIIX_PM, PCI_COMMAND, cmd);
>>> +
>>> +       /* PM I/O Space Enable (PMIOSE). */
>>> +       en = x86_pci_read_config8(PIIX_PM, PMREGMISC);
>>> +       en |= PMIOSE;
>>> +       x86_pci_write_config8(PIIX_PM, PMREGMISC, en);
>>> +}
>>> +
>>> +static void enable_pm_ich9(void)
>>> +{
>>> +       u16 device;
>>> +
>>> +       device = x86_pci_read_config16(ICH9_PM, PCI_DEVICE_ID);
>>> +       if (device != PCI_DEVICE_ID_INTEL_ICH9_8)
>>> +               return;
>>
>> Guess the check is already covered in qemu_chipset_init().
>>
>>> +
>>> +       /* Set the PM I/O base. */
>>> +       x86_pci_write_config32(ICH9_PM, PMBA, DEFAULT_PMBASE | 1);
>>> +}
>>> +
>>>  static void qemu_chipset_init(void)
>>>  {
>>>         u16 device, xbcs;
>>> @@ -53,10 +88,14 @@ static void qemu_chipset_init(void)
>>>                 xbcs = x86_pci_read_config16(PIIX_ISA, XBCS);
>>>                 xbcs |= APIC_EN;
>>>                 x86_pci_write_config16(PIIX_ISA, XBCS, xbcs);
>>> +
>>> +               enable_pm_piix();
>>>         } else {
>>>                 /* Configure PCIe ECAM base address */
>>>                 x86_pci_write_config32(PCI_BDF(0, 0, 0), PCIEX_BAR,
>>>                                        CONFIG_PCIE_ECAM_BASE | BAR_EN);
>>> +
>>> +               enable_pm_ich9();
>>>         }
>>>
>>>         qemu_fwcfg_init();
>>> diff --git a/arch/x86/include/asm/arch-qemu/device.h b/arch/x86/include/asm/arch-qemu/device.h
>>> index 75a435e..2e11100 100644
>>> --- a/arch/x86/include/asm/arch-qemu/device.h
>>> +++ b/arch/x86/include/asm/arch-qemu/device.h
>>> @@ -13,9 +13,17 @@
>>>  #define PIIX_ISA       PCI_BDF(0, 1, 0)
>>>  #define PIIX_IDE       PCI_BDF(0, 1, 1)
>>>  #define PIIX_USB       PCI_BDF(0, 1, 2)
>>> +#define PIIX_PM        PCI_BDF(0, 1, 3)
>>> +#define ICH9_PM        PCI_BDF(0, 0x1f, 0)
>>>  #define I440FX_VGA     PCI_BDF(0, 2, 0)
>>>
>>>  #define QEMU_Q35       PCI_BDF(0, 0, 0)
>>>  #define Q35_VGA                PCI_BDF(0, 1, 0)
>>>
>>> +#define PMBA           0x40
>>> +#define DEFAULT_PMBASE 0xe400
>>
>> See arch/x86/cpu/quark/Kconfig we have ACPI_PM1_BASE already. Maybe we
>> need consolidate this to introduce a similar one for QEMU.
>
> OK, will fix this.
>
>>
>>> +#define PM_IO_BASE     DEFAULT_PMBASE
>>
>> PM_IO_BASE is not referenced anywhere.
>>
>>> +#define PMREGMISC      0x80
>>> +#define PMIOSE         (1 << 0)
>>> +
>>
>> Please move these register defines to include/asm/arch-qemu/qemu.h
>
> OK, will fix this.
>
>>
>>>  #endif /* _QEMU_DEVICE_H_ */
>>> --
>>

Regards,
Bin