From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Nf9Fr-0004KZ-Ei for qemu-devel@nongnu.org; Wed, 10 Feb 2010 04:53:07 -0500 Received: from [199.232.76.173] (port=42960 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Nf9Fq-0004KK-SH for qemu-devel@nongnu.org; Wed, 10 Feb 2010 04:53:07 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1Nf9Fo-0007Be-V1 for qemu-devel@nongnu.org; Wed, 10 Feb 2010 04:53:06 -0500 Received: from mx1.redhat.com ([209.132.183.28]:4739) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Nf9Fo-0007BF-9n for qemu-devel@nongnu.org; Wed, 10 Feb 2010 04:53:04 -0500 Date: Wed, 10 Feb 2010 11:49:45 +0200 From: "Michael S. Tsirkin" Message-ID: <20100210094929.GF14063@redhat.com> References: <1265752899-26980-1-git-send-email-aliguori@us.ibm.com> <1265752899-26980-10-git-send-email-aliguori@us.ibm.com> <4B7252EA.7000300@mail.berlios.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4B7252EA.7000300@mail.berlios.de> Subject: [Qemu-devel] Re: [PATCH 09/15] eepro100: convert to new pci interface List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Weil Cc: Anthony Liguori , qemu-devel@nongnu.org, Michael Tsirkin On Wed, Feb 10, 2010 at 07:32:10AM +0100, Stefan Weil wrote: > See my inline comments. > > > Anthony Liguori schrieb: > > - Removed some dead defines for TARGET_I386 so that we could build once > > > > Signed-off-by: Anthony Liguori > > --- > > hw/eepro100.c | 238 ++++++++++++++------------------------------------------- > > 1 files changed, 57 insertions(+), 181 deletions(-) > > > > diff --git a/hw/eepro100.c b/hw/eepro100.c > > index b33dbb8..16230c9 100644 > > --- a/hw/eepro100.c > > +++ b/hw/eepro100.c > > @@ -33,10 +33,6 @@ > > * Open Source Software Developer Manual > > */ > > > > -#if defined(TARGET_I386) > > -# warning "PXE boot still not working!" > > -#endif > > - > > > > You did not fix PXE boot here, did you? > So the warning or a comment should stay there. > > > #include /* offsetof */ > > #include > > #include "hw.h" > > > ... > > > > -static void ioport_write2(void *opaque, uint32_t addr, uint32_t val) > > -{ > > - EEPRO100State *s = opaque; > > - eepro100_write2(s, addr - s->region[1], val); > > -} > > - > > -static void ioport_write4(void *opaque, uint32_t addr, uint32_t val) > > -{ > > - EEPRO100State *s = opaque; > > - eepro100_write4(s, addr - s->region[1], val); > > -} > > - > > /***********************************************************/ > > /* PCI EEPRO100 definitions */ > > > > -static void pci_map(PCIDevice * pci_dev, int region_num, > > - pcibus_t addr, pcibus_t size, int type) > > +static void pci_io_write(PCIDevice *dev, pcibus_t addr, int size, > > + uint32_t value) > > { > > - EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev); > > - > > - TRACE(OTHER, logout("region %d, addr=0x%08"FMT_PCIBUS", " > > - "size=0x%08"FMT_PCIBUS", type=%d\n", > > - region_num, addr, size, type)); > > + EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, dev); > > > > Please don't change the name of the PCIDevice pointer argument > from pci_dev to dev. > > This dev, dev in DO_UPCAST is ugly and misleading. It's a matter of taste: I actually think it's pretty: I never remember which argument of DO_UPCAST is which, and if both are dev it doesn't matter. > > > > > - assert(region_num == 1); > > - register_ioport_write(addr, size, 1, ioport_write1, s); > > - register_ioport_read(addr, size, 1, ioport_read1, s); > > - register_ioport_write(addr, size, 2, ioport_write2, s); > > - register_ioport_read(addr, size, 2, ioport_read2, s); > > - register_ioport_write(addr, size, 4, ioport_write4, s); > > - register_ioport_read(addr, size, 4, ioport_read4, s); > > - > > - s->region[region_num] = addr; > > -} > > - > > -/***************************************************************************** > > - * > > - * Memory mapped I/O. > > - * > > - ****************************************************************************/ > > - > > -static void pci_mmio_writeb(void *opaque, target_phys_addr_t addr, uint32_t val) > > -{ > > - EEPRO100State *s = opaque; > > - //~ logout("addr=%s val=0x%02x\n", regname(addr), val); > > - eepro100_write1(s, addr, val); > > -} > > - > > -static void pci_mmio_writew(void *opaque, target_phys_addr_t addr, uint32_t val) > > -{ > > - EEPRO100State *s = opaque; > > - //~ logout("addr=%s val=0x%02x\n", regname(addr), val); > > - eepro100_write2(s, addr, val); > > -} > > - > > -static void pci_mmio_writel(void *opaque, target_phys_addr_t addr, uint32_t val) > > -{ > > - EEPRO100State *s = opaque; > > - //~ logout("addr=%s val=0x%02x\n", regname(addr), val); > > - eepro100_write4(s, addr, val); > > -} > > - > > -static uint32_t pci_mmio_readb(void *opaque, target_phys_addr_t addr) > > -{ > > - EEPRO100State *s = opaque; > > - //~ logout("addr=%s\n", regname(addr)); > > - return eepro100_read1(s, addr); > > -} > > - > > -static uint32_t pci_mmio_readw(void *opaque, target_phys_addr_t addr) > > -{ > > - EEPRO100State *s = opaque; > > - //~ logout("addr=%s\n", regname(addr)); > > - return eepro100_read2(s, addr); > > -} > > - > > -static uint32_t pci_mmio_readl(void *opaque, target_phys_addr_t addr) > > -{ > > - EEPRO100State *s = opaque; > > - //~ logout("addr=%s\n", regname(addr)); > > - return eepro100_read4(s, addr); > > + if (size == 1) { > > + eepro100_write1(s, addr, value); > > + } else if (size == 2) { > > + eepro100_write2(s, addr, value); > > + } else { > > + eepro100_write4(s, addr, value); > > + } > > } > > > > -static CPUWriteMemoryFunc * const pci_mmio_write[] = { > > - pci_mmio_writeb, > > - pci_mmio_writew, > > - pci_mmio_writel > > -}; > > - > > -static CPUReadMemoryFunc * const pci_mmio_read[] = { > > - pci_mmio_readb, > > - pci_mmio_readw, > > - pci_mmio_readl > > -}; > > - > > -static void pci_mmio_map(PCIDevice * pci_dev, int region_num, > > - pcibus_t addr, pcibus_t size, int type) > > +static uint32_t pci_io_read(PCIDevice *dev, pcibus_t addr, int size) > > { > > > > Don't change pci_dev -> dev. See above. > > ... >