Re: [Qemu-devel] [PATCH 09/15] eepro100: convert to new pci interface

[Stefan Weil]

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 <address@hidden>
> ---
>  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.


>  
> -    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.

...