Hi Bin, 2016-01-19 17:25 GMT+08:00 Bin Meng <bmeng...@gmail.com>: > Hi Miao, > > On Tue, Jan 19, 2016 at 10:46 AM, Miao Yan <yanmiaob...@gmail.com> wrote: >> Hi Bin, >> >> 2016-01-16 21:23 GMT+08:00 Bin Meng <bmeng...@gmail.com>: >>> Hi Miao, >>> >>> On Fri, Jan 15, 2016 at 11:12 AM, Miao Yan <yanmiaob...@gmail.com> wrote: >>>> Enable ACPI IO space for piix4 (for pc board) and ich9 (for q35 board) >>>> >>>> Signed-off-by: Miao Yan <yanmiaob...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot