Re: [Qemu-devel] [PATCH v3 10/14] hw/lan9118: Add basic 16-bit mode support.

[Peter Maydell]

On 12 December 2011 06:43, Evgeny Voevodin <address@hidden> wrote:
>
> Signed-off-by: Evgeny Voevodin <address@hidden>
> ---
>  hw/devices.h |    2 +-
>  hw/lan9118.c |  115 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 109 insertions(+), 8 deletions(-)
>
> diff --git a/hw/devices.h b/hw/devices.h
> index 1a55c1e..24cae4c 100644
> --- a/hw/devices.h
> +++ b/hw/devices.h
> @@ -10,7 +10,7 @@ struct MemoryRegion;
>  void smc91c111_init(NICInfo *, uint32_t, qemu_irq);
>
>  /* lan9118.c */
> -void lan9118_init(NICInfo *, uint32_t, qemu_irq);
> +DeviceState *lan9118_init(NICInfo *, uint32_t, qemu_irq);

Don't do this. lan9118_init is just a legacy convenience
function. If you need something more than what it does
then just instantiate the qdev device where you use it.

In particular for your board you want to set the mode_16bit
property, and properties must be set before the device has
qdev_init called on it, so you can't use lan9118_init()
anyway.

>
>  /* tsc210x.c */
>  uWireSlave *tsc2102_init(qemu_irq pint);
> diff --git a/hw/lan9118.c b/hw/lan9118.c
> index ee8b2ea..e44e5f8 100644
> --- a/hw/lan9118.c
> +++ b/hw/lan9118.c
> @@ -212,6 +212,17 @@ typedef struct {
>     int rxp_offset;
>     int rxp_size;
>     int rxp_pad;
> +
> +    uint32_t write_word_prev_offset;
> +    uint32_t write_word_n;
> +    uint16_t write_word_l;
> +    uint16_t write_word_h;
> +    uint32_t read_word_prev_offset;
> +    uint32_t read_word_n;
> +    uint32_t read_long;
> +
> +    uint32_t mode_16bit;
> +
>  } lan9118_state;
>
>  static void lan9118_update(lan9118_state *s)
> @@ -345,6 +356,9 @@ static void lan9118_reset(DeviceState *d)
>     s->mac_mii_data = 0;
>     s->mac_flow = 0;
>
> +    s->read_word_n = 0;
> +    s->write_word_n = 0;
> +
>     phy_reset(s);
>
>     s->eeprom_writable = 0;
> @@ -896,11 +910,11 @@ static void lan9118_tick(void *opaque)
>  }
>
>  static void lan9118_writel(void *opaque, target_phys_addr_t offset,
> -                           uint64_t val, unsigned size)
> +                           uint32_t val)
>  {
>     lan9118_state *s = (lan9118_state *)opaque;
>     offset &= 0xff;
> -
> +
>     //DPRINTF("Write reg 0x%02x = 0x%08x\n", (int)offset, val);
>     if (offset >= 0x20 && offset < 0x40) {
>         /* TX FIFO */
> @@ -1029,8 +1043,47 @@ static void lan9118_writel(void *opaque, 
> target_phys_addr_t offset,
>     lan9118_update(s);
>  }
>
> -static uint64_t lan9118_readl(void *opaque, target_phys_addr_t offset,
> -                              unsigned size)
> +static void lan9118_writew(void *opaque, target_phys_addr_t offset,
> +                           uint32_t val)
> +{
> +    lan9118_state *s = (lan9118_state *)opaque;
> +    offset &= 0xff;
> +
> +    if (s->write_word_prev_offset != (offset & ~0x3)) {
> +        /* New offset, reset word counter */
> +        s->write_word_n = 0;
> +        s->write_word_prev_offset = offset & ~0x3;
> +    }
> +
> +    if (offset & 0x2) {
> +        s->write_word_h = val;
> +    } else {
> +        s->write_word_l = val;
> +    }
> +
> +    //DPRINTF("Writew reg 0x%02x = 0x%08x\n", (int)offset, val);
> +    s->write_word_n++;
> +    if (s->write_word_n == 2) {
> +        s->write_word_n = 0;
> +        lan9118_writel(s, offset & ~3, s->write_word_l +
> +                (s->write_word_h << 16));
> +    }
> +}
> +
> +static void lan9118_write(void *opaque, target_phys_addr_t offset,
> +                          uint64_t val, unsigned size)
> +{
> +    switch (size) {
> +    case 2:
> +        return lan9118_writew(opaque, offset, (uint32_t)val);
> +    case 4:
> +        return lan9118_writel(opaque, offset, (uint32_t)val);
> +    }
> +
> +    hw_error("lan9118_write: Bad size 0x%x\n", size);
> +}
> +
> +static uint32_t lan9118_readl(void *opaque, target_phys_addr_t offset)
>  {
>     lan9118_state *s = (lan9118_state *)opaque;
>
> @@ -1065,6 +1118,9 @@ static uint64_t lan9118_readl(void *opaque, 
> target_phys_addr_t offset,
>     case CSR_TX_CFG:
>         return s->tx_cfg;
>     case CSR_HW_CFG:
> +        if (s->mode_16bit) {
> +            return s->hw_cfg & ~0x4;
> +        }
>         return s->hw_cfg | 0x4;

We should just set hw_cfg to include the correct value of this bit
on reset (and make sure it can't be written to). Then the read function
can just return s->hw_cfg without any fiddling about.

>     case CSR_RX_DP_CTRL:
>         return 0;
> @@ -1103,9 +1159,51 @@ static uint64_t lan9118_readl(void *opaque, 
> target_phys_addr_t offset,
>     return 0;
>  }
>
> +static uint32_t lan9118_readw(void *opaque, target_phys_addr_t offset)
> +{
> +    lan9118_state *s = (lan9118_state *)opaque;
> +    uint32_t val;
> +
> +    if (s->read_word_prev_offset != (offset & ~0x3)) {
> +        /* New offset, reset word counter */
> +        s->read_word_n = 0;
> +        s->read_word_prev_offset = offset & ~0x3;
> +    }
> +
> +    s->read_word_n++;
> +    if (s->read_word_n == 1) {
> +        s->read_long = lan9118_readl(s, offset & ~3);
> +    } else {
> +        s->read_word_n = 0;
> +    }
> +
> +    if (offset & 2) {
> +        val = s->read_long >> 16;
> +    } else {
> +        val = s->read_long & 0xFFFF;
> +    }
> +
> +    //DPRINTF("Readw reg 0x%02x, val 0x%x\n", (int)offset, val);
> +    return val;
> +}
> +
> +static uint64_t lan9118_read(void *opaque, target_phys_addr_t offset,
> +                             unsigned size)
> +{
> +    switch (size) {
> +    case 2:
> +        return lan9118_readw(opaque, offset);
> +    case 4:
> +        return lan9118_readl(opaque, offset);
> +    }
> +
> +    hw_error("lan9118_read: Bad size 0x%x\n", size);
> +    return 0;
> +}
> +
>  static const MemoryRegionOps lan9118_mem_ops = {
> -    .read = lan9118_readl,
> -    .write = lan9118_writel,
> +    .read = lan9118_read,
> +    .write = lan9118_write,
>     .endianness = DEVICE_NATIVE_ENDIAN,
>  };

Don't change the MemoryRegionOps / functions for the 32 bit case.
Instead have a second MemoryRegionOps lan9118_16bit_mem_ops with
your functions (which should just call the lan9118_readl/writel
passing 4 as the size), and register the correct MemoryRegionOps
depending on the value of the mode_16bit property.

> @@ -1162,6 +1260,7 @@ static SysBusDeviceInfo lan9118_info = {
>     .qdev.reset = lan9118_reset,
>     .qdev.props = (Property[]) {
>         DEFINE_NIC_PROPERTIES(lan9118_state, conf),
> +        DEFINE_PROP_UINT32("mode_16bit", lan9118_state, mode_16bit, 0),
>         DEFINE_PROP_END_OF_LIST(),
>     }
>  };
> @@ -1173,7 +1272,7 @@ static void lan9118_register_devices(void)
>
>  /* Legacy helper function.  Should go away when machine config files are
>    implemented.  */
> -void lan9118_init(NICInfo *nd, uint32_t base, qemu_irq irq)
> +DeviceState *lan9118_init(NICInfo *nd, uint32_t base, qemu_irq irq)
>  {
>     DeviceState *dev;
>     SysBusDevice *s;
> @@ -1185,6 +1284,8 @@ void lan9118_init(NICInfo *nd, uint32_t base, qemu_irq 
> irq)
>     s = sysbus_from_qdev(dev);
>     sysbus_mmio_map(s, 0, base);
>     sysbus_connect_irq(s, 0, irq);
> +
> +    return dev;
>  }
>
>  device_init(lan9118_register_devices)
> --
> 1.7.4.1
>