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 <aligu...@us.ibm.com>
> > ---
> > 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 <stddef.h> /* offsetof */
> > #include <stdbool.h>
> > #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.
>
> ...
>
