From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48855) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T8rVq-00075y-Ug for qemu-devel@nongnu.org; Tue, 04 Sep 2012 07:41:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T8rVl-0007W0-QW for qemu-devel@nongnu.org; Tue, 04 Sep 2012 07:41:46 -0400 Received: from goliath.siemens.de ([192.35.17.28]:30042) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T8rVl-0007Vr-Fw for qemu-devel@nongnu.org; Tue, 04 Sep 2012 07:41:41 -0400 Message-ID: <5045E8F1.1040804@siemens.com> Date: Tue, 04 Sep 2012 13:41:37 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <7f5a672e348b8f917dcc92c67e2ae5f7ce75ea60.1346665074.git.julien.grall@citrix.com> <5045ACB5.40106@web.de> <5045E5AB.4020007@citrix.com> <5045E7F0.1030205@siemens.com> In-Reply-To: <5045E7F0.1030205@siemens.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V7 4/8] hw/acpi_piix4.c: replace register_ioport* List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Julien Grall Cc: "qemu-devel@nongnu.org" , Stefano Stabellini , "avi@redhat.com" , "afaerber@suse.de" , "kraxel@redhat.com" On 2012-09-04 13:37, Jan Kiszka wrote: > On 2012-09-04 13:27, Julien Grall wrote: >> On 09/04/2012 08:24 AM, Jan Kiszka wrote: >>> On 2012-09-03 12:03, Julien Grall wrote: >>> >>>> This patch replaces all register_ioport* with the new memory API. It >>>> permits >>>> to use the new Memory stuff like listener. >>>> >>>> Signed-off-by: Julien Grall >>>> --- >>>> hw/acpi_piix4.c | 151 >>>> +++++++++++++++++++++++++++++++++++++++++++------------ >>>> 1 files changed, 119 insertions(+), 32 deletions(-) >>>> >>>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c >>>> index 1ad45ce..527dfc1 100644 >>>> --- a/hw/acpi_piix4.c >>>> +++ b/hw/acpi_piix4.c >>>> @@ -41,8 +41,7 @@ >>>> >>>> #define GPE_BASE 0xafe0 >>>> #define GPE_LEN 4 >>>> -#define PCI_UP_BASE 0xae00 >>>> -#define PCI_DOWN_BASE 0xae04 >>>> +#define PCI_BASE 0xae00 >>>> #define PCI_EJ_BASE 0xae08 >>>> #define PCI_RMV_BASE 0xae0c >>>> >>>> @@ -55,7 +54,8 @@ struct pci_status { >>>> >>>> typedef struct PIIX4PMState { >>>> PCIDevice dev; >>>> - IORange ioport; >>>> + MemoryRegion pm_io; >>>> + uint32_t pm_io_base; >>>> ACPIREGS ar; >>>> >>>> APMState apm; >>>> @@ -64,6 +64,11 @@ typedef struct PIIX4PMState { >>>> uint32_t smb_io_base; >>>> >>>> MemoryRegion smb_io; >>>> + MemoryRegion acpi_io; >>>> + MemoryRegion acpi_hot_io; >>>> + PortioList pci_hot_port_list; >>>> + MemoryRegion pciej_hot_io; >>>> + MemoryRegion pcirmv_hot_io; >>>> >>>> qemu_irq irq; >>>> qemu_irq smi_irq; >>>> @@ -110,10 +115,10 @@ static void pm_tmr_timer(ACPIREGS *ar) >>>> pm_update_sci(s); >>>> } >>>> >>>> -static void pm_ioport_write(IORange *ioport, uint64_t addr, unsigned >>>> width, >>>> - uint64_t val) >>>> +static void pm_ioport_write(void *opaque, target_phys_addr_t addr, >>>> + uint64_t val, unsigned width) >>>> { >>>> - PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport); >>>> + PIIX4PMState *s = opaque; >>>> >>>> if (width != 2) { >>>> PIIX4_DPRINTF("PM write port=0x%04x width=%d val=0x%08x\n", >>>> @@ -139,11 +144,11 @@ static void pm_ioport_write(IORange *ioport, >>>> uint64_t addr, unsigned width, >>>> (unsigned int)val); >>>> } >>>> >>>> -static void pm_ioport_read(IORange *ioport, uint64_t addr, unsigned >>>> width, >>>> - uint64_t *data) >>>> +static uint64_t pm_ioport_read(void *opaque, target_phys_addr_t addr, >>>> + unsigned width) >>>> { >>>> - PIIX4PMState *s = container_of(ioport, PIIX4PMState, ioport); >>>> - uint32_t val; >>>> + PIIX4PMState *s = opaque; >>>> + uint64_t val; >>>> >>>> switch(addr) { >>>> case 0x00: >>>> @@ -163,12 +168,18 @@ static void pm_ioport_read(IORange *ioport, >>>> uint64_t addr, unsigned width, >>>> break; >>>> } >>>> PIIX4_DPRINTF("PM readw port=0x%04x val=0x%04x\n", (unsigned >>>> int)addr, val); >>>> - *data = val; >>>> + >>>> + return val; >>>> } >>>> >>>> -static const IORangeOps pm_iorange_ops = { >>>> +static const MemoryRegionOps pm_io_ops = { >>>> .read = pm_ioport_read, >>>> .write = pm_ioport_write, >>>> + .endianness = DEVICE_LITTLE_ENDIAN, >>>> + .impl = { >>>> + .min_access_size = 2, >>>> + .max_access_size = 2, >>>> + }, >>>> }; >>>> >>>> static void apm_ctrl_changed(uint32_t val, void *arg) >>>> @@ -185,7 +196,8 @@ static void apm_ctrl_changed(uint32_t val, void >>>> *arg) >>>> } >>>> } >>>> >>>> -static void acpi_dbg_writel(void *opaque, uint32_t addr, uint32_t val) >>>> +static void acpi_dbg_writel(void *opaque, target_phys_addr_t addr, >>>> + uint64_t val, unsigned size) >>>> { >>>> PIIX4_DPRINTF("ACPI: DBG: 0x%08x\n", val); >>>> } >>>> @@ -200,8 +212,18 @@ static void pm_io_space_update(PIIX4PMState *s) >>>> >>>> /* XXX: need to improve memory and ioport allocation */ >>>> PIIX4_DPRINTF("PM: mapping to 0x%x\n", pm_io_base); >>>> - iorange_init(&s->ioport,&pm_iorange_ops, pm_io_base, 64); >>>> - ioport_register(&s->ioport); >>>> + >>>> + if (!s->pm_io_base) { >>>> + memory_region_add_subregion(pci_address_space_io(&s->dev), >>>> + pm_io_base,&s->pm_io); >>>> >>> This doesn't work reliably. Add the region in a disabled state (ie. the >>> default) during init, only toggle enable and update the base here. >>> >> What is the default base address ? The datasheet >> doesn't give default value. But the base address seems to >> always be 0xb000. Is there any drawbacks to used it ? > > According to the spec > (http://www.intel.com/assets/pdf/datasheet/290562.pdf), the reset value > must be 0. More precisely, the base is 0, the corresponding register is 1 after reset (page 120, Power Management Base Address)). Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux