Hi Bin,
2016-01-19 17:25 GMT+08:00 Bin Meng bmeng.cn@gmail.com:
Hi Miao,
On Tue, Jan 19, 2016 at 10:46 AM, Miao Yan yanmiaobest@gmail.com wrote:
Hi Bin,
2016-01-16 21:23 GMT+08:00 Bin Meng bmeng.cn@gmail.com:
Hi Miao,
On Fri, Jan 15, 2016 at 11:12 AM, Miao Yan yanmiaobest@gmail.com wrote:
Enable ACPI IO space for piix4 (for pc board) and ich9 (for q35 board)
Signed-off-by: Miao Yan yanmiaobest@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