Re: [Qemu-devel] [RFC 3/8] uhci: do not use old_portio-style callbacks

[Gerd Hoffmann <kraxel@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 973 bytes --]

On 12/23/12 16:32, Hervé Poussineau wrote:
> -static const MemoryRegionPortio uhci_portio[] = {
> -    { 0, 32, 2, .write = uhci_ioport_writew, },
> -    { 0, 32, 2, .read = uhci_ioport_readw, },
> -    { 0, 32, 4, .write = uhci_ioport_writel, },
> -    { 0, 32, 4, .read = uhci_ioport_readl, },
> -    { 0, 32, 1, .write = uhci_ioport_writeb, },
> -    { 0, 32, 1, .read = uhci_ioport_readb, },
> -    PORTIO_END_OF_LIST()
> -};
> +static uint64_t uhci_ioport_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    switch (size) {
> +    case 1:
> +        return uhci_ioport_readb(opaque, (uint32_t)addr);
> +    case 2:
> +        return uhci_ioport_readw(opaque, (uint32_t)addr);
> +    case 4:
> +        return uhci_ioport_readl(opaque, (uint32_t)addr);
> +    default:
> +        return ~0UL;
> +    }
> +}

Aaaargh.  Please don't.  Offloading the size handling to the memory api
is better.  See attached patch.

cheers,
  Gerd

[-- Attachment #2: 0001-uhci-stop-using-portio-lists.patch --]
[-- Type: text/plain, Size: 5193 bytes --]

From 3f5810004dad2ef67cf067d96cf0b983166a454f Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Thu, 3 Jan 2013 12:29:41 +0100
Subject: [PATCH] uhci: stop using portio lists

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-uhci.c |  106 +++++++++++++++--------------------------------------
 trace-events      |    2 -
 2 files changed, 30 insertions(+), 78 deletions(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 0cd68cf..60645aa 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -461,40 +461,11 @@ static const VMStateDescription vmstate_uhci = {
     }
 };
 
-static void uhci_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
+static void uhci_port_write(void *opaque, hwaddr addr,
+                            uint64_t val, unsigned size)
 {
     UHCIState *s = opaque;
 
-    addr &= 0x1f;
-    switch(addr) {
-    case 0x0c:
-        s->sof_timing = val;
-        break;
-    }
-}
-
-static uint32_t uhci_ioport_readb(void *opaque, uint32_t addr)
-{
-    UHCIState *s = opaque;
-    uint32_t val;
-
-    addr &= 0x1f;
-    switch(addr) {
-    case 0x0c:
-        val = s->sof_timing;
-        break;
-    default:
-        val = 0xff;
-        break;
-    }
-    return val;
-}
-
-static void uhci_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
-{
-    UHCIState *s = opaque;
-
-    addr &= 0x1f;
     trace_usb_uhci_mmio_writew(addr, val);
 
     switch(addr) {
@@ -543,6 +514,17 @@ static void uhci_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
         if (s->status & UHCI_STS_HCHALTED)
             s->frnum = val & 0x7ff;
         break;
+    case 0x08:
+        s->fl_base_addr &= 0xffff0000;
+        s->fl_base_addr |= val & ~0xfff;
+        break;
+    case 0x0a:
+        s->fl_base_addr &= 0x0000ffff;
+        s->fl_base_addr |= (val << 16);
+        break;
+    case 0x0c:
+        s->sof_timing = val & 0xff;
+        break;
     case 0x10 ... 0x1f:
         {
             UHCIPort *port;
@@ -574,12 +556,11 @@ static void uhci_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
     }
 }
 
-static uint32_t uhci_ioport_readw(void *opaque, uint32_t addr)
+static uint64_t uhci_port_read(void *opaque, hwaddr addr, unsigned size)
 {
     UHCIState *s = opaque;
     uint32_t val;
 
-    addr &= 0x1f;
     switch(addr) {
     case 0x00:
         val = s->cmd;
@@ -593,6 +574,15 @@ static uint32_t uhci_ioport_readw(void *opaque, uint32_t addr)
     case 0x06:
         val = s->frnum;
         break;
+    case 0x08:
+        val = s->fl_base_addr & 0xffff;
+        break;
+    case 0x0a:
+        val = (s->fl_base_addr >> 16) & 0xffff;
+        break;
+    case 0x0c:
+        val = s->sof_timing;
+        break;
     case 0x10 ... 0x1f:
         {
             UHCIPort *port;
@@ -615,38 +605,6 @@ static uint32_t uhci_ioport_readw(void *opaque, uint32_t addr)
     return val;
 }
 
-static void uhci_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
-{
-    UHCIState *s = opaque;
-
-    addr &= 0x1f;
-    trace_usb_uhci_mmio_writel(addr, val);
-
-    switch(addr) {
-    case 0x08:
-        s->fl_base_addr = val & ~0xfff;
-        break;
-    }
-}
-
-static uint32_t uhci_ioport_readl(void *opaque, uint32_t addr)
-{
-    UHCIState *s = opaque;
-    uint32_t val;
-
-    addr &= 0x1f;
-    switch(addr) {
-    case 0x08:
-        val = s->fl_base_addr;
-        break;
-    default:
-        val = 0xffffffff;
-        break;
-    }
-    trace_usb_uhci_mmio_readl(addr, val);
-    return val;
-}
-
 /* signal resume if controller suspended */
 static void uhci_resume (void *opaque)
 {
@@ -1236,18 +1194,14 @@ static void uhci_frame_timer(void *opaque)
     qemu_mod_timer(s->frame_timer, t_now + frame_t);
 }
 
-static const MemoryRegionPortio uhci_portio[] = {
-    { 0, 32, 2, .write = uhci_ioport_writew, },
-    { 0, 32, 2, .read = uhci_ioport_readw, },
-    { 0, 32, 4, .write = uhci_ioport_writel, },
-    { 0, 32, 4, .read = uhci_ioport_readl, },
-    { 0, 32, 1, .write = uhci_ioport_writeb, },
-    { 0, 32, 1, .read = uhci_ioport_readb, },
-    PORTIO_END_OF_LIST()
-};
-
 static const MemoryRegionOps uhci_ioport_ops = {
-    .old_portio = uhci_portio,
+    .read  = uhci_port_read,
+    .write = uhci_port_write,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .impl.min_access_size = 2,
+    .impl.max_access_size = 2,
+    .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static USBPortOps uhci_port_ops = {
diff --git a/trace-events b/trace-events
index 4b061c6..f68e03b 100644
--- a/trace-events
+++ b/trace-events
@@ -307,8 +307,6 @@ usb_uhci_frame_loop_stop_idle(void) ""
 usb_uhci_frame_loop_continue(void) ""
 usb_uhci_mmio_readw(uint32_t addr, uint32_t val) "addr 0x%04x, ret 0x%04x"
 usb_uhci_mmio_writew(uint32_t addr, uint32_t val) "addr 0x%04x, val 0x%04x"
-usb_uhci_mmio_readl(uint32_t addr, uint32_t val) "addr 0x%04x, ret 0x%08x"
-usb_uhci_mmio_writel(uint32_t addr, uint32_t val) "addr 0x%04x, val 0x%08x"
 usb_uhci_queue_add(uint32_t token) "token 0x%x"
 usb_uhci_queue_del(uint32_t token, const char *reason) "token 0x%x: %s"
 usb_uhci_packet_add(uint32_t token, uint32_t addr) "token 0x%x, td 0x%x"
-- 
1.7.1