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

[Miao Yan]

Hi Bin,

2016-01-19 17:25 GMT+08:00 Bin Meng <bmeng.cn at gmail.com>:
> 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.
>

Yes, that's a little bit complex and not necessary if their PCI
addresses are fixed . And I don't think we should do it in this
patchset.

So how do you suggest we do this ? Either I remove the two checks to
make it aligned with the rest or create a separate patch to do the
checks ?



>>
>>>
>>>> +
>>>> +       /* 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