diff mbox series

[3/5] usb/ohci: Move USBPortOps related functions together

Message ID 8e2deb83f2bf2846f9115962e1e2fedf5a56317f.1642339238.git.balaton@eik.bme.hu (mailing list archive)
State New, archived
Headers show
Series Misc OHCI clean ups | expand

Commit Message

BALATON Zoltan Jan. 16, 2022, 1:20 p.m. UTC 
This also allows removing two forward declarations

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/usb/hcd-ohci.c | 204 +++++++++++++++++++++++-----------------------
 1 file changed, 100 insertions(+), 104 deletions(-)

Comments

Philippe Mathieu-Daudé Jan. 16, 2022, 9:14 p.m. UTC | #1 
On 16/1/22 14:20, BALATON Zoltan wrote:
> This also allows removing two forward declarations
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/usb/hcd-ohci.c | 204 +++++++++++++++++++++++-----------------------
>   1 file changed, 100 insertions(+), 104 deletions(-)

> +static const MemoryRegionOps ohci_mem_ops = {
> +    .read = ohci_mem_read,
> +    .write = ohci_mem_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +/* USBPortOps */
> +static void ohci_attach(USBPort *port1)
> +{
> +    OHCIState *s = port1->opaque;
> +    OHCIPort *port = &s->rhport[port1->index];
> +    uint32_t old_state = port->ctrl;
> +
> +    /* set connect status */
> +    port->ctrl |= OHCI_PORT_CCS | OHCI_PORT_CSC;
> +
> +    /* update speed */
> +    if (port->port.dev->speed == USB_SPEED_LOW) {
> +        port->ctrl |= OHCI_PORT_LSDA;
> +    } else {
> +        port->ctrl &= ~OHCI_PORT_LSDA;
> +    }
> +
> +    /* notify of remote-wakeup */
> +    if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) {
> +        ohci_set_interrupt(s, OHCI_INTR_RD);
> +    }
> +
> +    trace_usb_ohci_port_attach(port1->index);
> +
> +    if (old_state != port->ctrl) {
> +        ohci_set_interrupt(s, OHCI_INTR_RHSC);
> +    }
> +}
> +
>   static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev)
>   {
>       if (ohci->async_td &&
> @@ -1799,11 +1735,71 @@ static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev)
>       }
>   }

We could have ohci_attach() just before ohci*_detach(),
anyhow:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> -static const MemoryRegionOps ohci_mem_ops = {
> -    .read = ohci_mem_read,
> -    .write = ohci_mem_write,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> -};
> +static void ohci_child_detach(USBPort *port1, USBDevice *child)
> +{
> +    OHCIState *s = port1->opaque;
> +
> +    ohci_async_cancel_device(s, child);
> +}
> +
> +static void ohci_detach(USBPort *port1)
> +{
> +    OHCIState *s = port1->opaque;
> +    OHCIPort *port = &s->rhport[port1->index];
> +    uint32_t old_state = port->ctrl;
> +
> +    ohci_async_cancel_device(s, port1->dev);
> +
> +    /* set connect status */
> +    if (port->ctrl & OHCI_PORT_CCS) {
> +        port->ctrl &= ~OHCI_PORT_CCS;
> +        port->ctrl |= OHCI_PORT_CSC;
> +    }
> +    /* disable port */
> +    if (port->ctrl & OHCI_PORT_PES) {
> +        port->ctrl &= ~OHCI_PORT_PES;
> +        port->ctrl |= OHCI_PORT_PESC;
> +    }
> +    trace_usb_ohci_port_detach(port1->index);
> +
> +    if (old_state != port->ctrl) {
> +        ohci_set_interrupt(s, OHCI_INTR_RHSC);
> +    }
> +}
BALATON Zoltan Jan. 17, 2022, 10:23 a.m. UTC | #2 
On Sun, 16 Jan 2022, Philippe Mathieu-Daudé wrote:
> On 16/1/22 14:20, BALATON Zoltan wrote:
>> This also allows removing two forward declarations
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/usb/hcd-ohci.c | 204 +++++++++++++++++++++++-----------------------
>>   1 file changed, 100 insertions(+), 104 deletions(-)
>
>> +static const MemoryRegionOps ohci_mem_ops = {
>> +    .read = ohci_mem_read,
>> +    .write = ohci_mem_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>> +/* USBPortOps */
>> +static void ohci_attach(USBPort *port1)
>> +{
>> +    OHCIState *s = port1->opaque;
>> +    OHCIPort *port = &s->rhport[port1->index];
>> +    uint32_t old_state = port->ctrl;
>> +
>> +    /* set connect status */
>> +    port->ctrl |= OHCI_PORT_CCS | OHCI_PORT_CSC;
>> +
>> +    /* update speed */
>> +    if (port->port.dev->speed == USB_SPEED_LOW) {
>> +        port->ctrl |= OHCI_PORT_LSDA;
>> +    } else {
>> +        port->ctrl &= ~OHCI_PORT_LSDA;
>> +    }
>> +
>> +    /* notify of remote-wakeup */
>> +    if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) {
>> +        ohci_set_interrupt(s, OHCI_INTR_RD);
>> +    }
>> +
>> +    trace_usb_ohci_port_attach(port1->index);
>> +
>> +    if (old_state != port->ctrl) {
>> +        ohci_set_interrupt(s, OHCI_INTR_RHSC);
>> +    }
>> +}
>> +
>>   static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev)
>>   {
>>       if (ohci->async_td &&
>> @@ -1799,11 +1735,71 @@ static void ohci_async_cancel_device(OHCIState 
>> *ohci, USBDevice *dev)
>>       }
>>   }
>
> We could have ohci_attach() just before ohci*_detach(),

The ohci_child_detach() function (which the next patch merges with 
ohci_async_cancel_device) is used by ohci_detach() but not ohci_attach() 
that's why I've put it between attach and detach. Attach and detach are 
less related than ohci_child_detach and ohci_detach in my opinion. Attach 
and detach are still close enough after the next patch but without moving 
child_detach and detach apart.

> anyhow:
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks for the review.

Regards,
BALATON Zoltan
diff mbox series

Patch

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 6d762973eb..d1907afd3f 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -58,8 +58,6 @@  struct ohci_hcca {
 #define ED_WBACK_OFFSET offsetof(struct ohci_ed, head)
 #define ED_WBACK_SIZE   4
 
-static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev);
-
 /* Bitfields for the first word of an Endpoint Desciptor.  */
 #define OHCI_ED_FA_SHIFT  0
 #define OHCI_ED_FA_MASK   (0x7f<<OHCI_ED_FA_SHIFT)
@@ -261,92 +259,6 @@  static inline void ohci_set_interrupt(OHCIState *ohci, uint32_t intr)
     ohci_intr_update(ohci);
 }
 
-/* Attach or detach a device on a root hub port.  */
-static void ohci_attach(USBPort *port1)
-{
-    OHCIState *s = port1->opaque;
-    OHCIPort *port = &s->rhport[port1->index];
-    uint32_t old_state = port->ctrl;
-
-    /* set connect status */
-    port->ctrl |= OHCI_PORT_CCS | OHCI_PORT_CSC;
-
-    /* update speed */
-    if (port->port.dev->speed == USB_SPEED_LOW) {
-        port->ctrl |= OHCI_PORT_LSDA;
-    } else {
-        port->ctrl &= ~OHCI_PORT_LSDA;
-    }
-
-    /* notify of remote-wakeup */
-    if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) {
-        ohci_set_interrupt(s, OHCI_INTR_RD);
-    }
-
-    trace_usb_ohci_port_attach(port1->index);
-
-    if (old_state != port->ctrl) {
-        ohci_set_interrupt(s, OHCI_INTR_RHSC);
-    }
-}
-
-static void ohci_detach(USBPort *port1)
-{
-    OHCIState *s = port1->opaque;
-    OHCIPort *port = &s->rhport[port1->index];
-    uint32_t old_state = port->ctrl;
-
-    ohci_async_cancel_device(s, port1->dev);
-
-    /* set connect status */
-    if (port->ctrl & OHCI_PORT_CCS) {
-        port->ctrl &= ~OHCI_PORT_CCS;
-        port->ctrl |= OHCI_PORT_CSC;
-    }
-    /* disable port */
-    if (port->ctrl & OHCI_PORT_PES) {
-        port->ctrl &= ~OHCI_PORT_PES;
-        port->ctrl |= OHCI_PORT_PESC;
-    }
-    trace_usb_ohci_port_detach(port1->index);
-
-    if (old_state != port->ctrl) {
-        ohci_set_interrupt(s, OHCI_INTR_RHSC);
-    }
-}
-
-static void ohci_wakeup(USBPort *port1)
-{
-    OHCIState *s = port1->opaque;
-    OHCIPort *port = &s->rhport[port1->index];
-    uint32_t intr = 0;
-    if (port->ctrl & OHCI_PORT_PSS) {
-        trace_usb_ohci_port_wakeup(port1->index);
-        port->ctrl |= OHCI_PORT_PSSC;
-        port->ctrl &= ~OHCI_PORT_PSS;
-        intr = OHCI_INTR_RHSC;
-    }
-    /* Note that the controller can be suspended even if this port is not */
-    if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) {
-        trace_usb_ohci_remote_wakeup(s->name);
-        /* This is the one state transition the controller can do by itself */
-        s->ctl &= ~OHCI_CTL_HCFS;
-        s->ctl |= OHCI_USB_RESUME;
-        /* In suspend mode only ResumeDetected is possible, not RHSC:
-         * see the OHCI spec 5.1.2.3.
-         */
-        intr = OHCI_INTR_RD;
-    }
-    ohci_set_interrupt(s, intr);
-}
-
-static void ohci_child_detach(USBPort *port1, USBDevice *child)
-{
-    OHCIState *s = port1->opaque;
-
-    ohci_async_cancel_device(s, child);
-}
-
 static USBDevice *ohci_find_device(OHCIState *ohci, uint8_t addr)
 {
     USBDevice *dev;
@@ -634,17 +546,6 @@  static int ohci_copy_iso_td(OHCIState *ohci,
     return 0;
 }
 
-static void ohci_process_lists(OHCIState *ohci, int completion);
-
-static void ohci_async_complete_packet(USBPort *port, USBPacket *packet)
-{
-    OHCIState *ohci = container_of(packet, OHCIState, usb_packet);
-
-    trace_usb_ohci_async_complete();
-    ohci->async_complete = true;
-    ohci_process_lists(ohci, 1);
-}
-
 #define USUB(a, b) ((int16_t)((uint16_t)(a) - (uint16_t)(b)))
 
 static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
@@ -1789,6 +1690,41 @@  static void ohci_mem_write(void *opaque,
     }
 }
 
+static const MemoryRegionOps ohci_mem_ops = {
+    .read = ohci_mem_read,
+    .write = ohci_mem_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+/* USBPortOps */
+static void ohci_attach(USBPort *port1)
+{
+    OHCIState *s = port1->opaque;
+    OHCIPort *port = &s->rhport[port1->index];
+    uint32_t old_state = port->ctrl;
+
+    /* set connect status */
+    port->ctrl |= OHCI_PORT_CCS | OHCI_PORT_CSC;
+
+    /* update speed */
+    if (port->port.dev->speed == USB_SPEED_LOW) {
+        port->ctrl |= OHCI_PORT_LSDA;
+    } else {
+        port->ctrl &= ~OHCI_PORT_LSDA;
+    }
+
+    /* notify of remote-wakeup */
+    if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) {
+        ohci_set_interrupt(s, OHCI_INTR_RD);
+    }
+
+    trace_usb_ohci_port_attach(port1->index);
+
+    if (old_state != port->ctrl) {
+        ohci_set_interrupt(s, OHCI_INTR_RHSC);
+    }
+}
+
 static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev)
 {
     if (ohci->async_td &&
@@ -1799,11 +1735,71 @@  static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev)
     }
 }
 
-static const MemoryRegionOps ohci_mem_ops = {
-    .read = ohci_mem_read,
-    .write = ohci_mem_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
+static void ohci_child_detach(USBPort *port1, USBDevice *child)
+{
+    OHCIState *s = port1->opaque;
+
+    ohci_async_cancel_device(s, child);
+}
+
+static void ohci_detach(USBPort *port1)
+{
+    OHCIState *s = port1->opaque;
+    OHCIPort *port = &s->rhport[port1->index];
+    uint32_t old_state = port->ctrl;
+
+    ohci_async_cancel_device(s, port1->dev);
+
+    /* set connect status */
+    if (port->ctrl & OHCI_PORT_CCS) {
+        port->ctrl &= ~OHCI_PORT_CCS;
+        port->ctrl |= OHCI_PORT_CSC;
+    }
+    /* disable port */
+    if (port->ctrl & OHCI_PORT_PES) {
+        port->ctrl &= ~OHCI_PORT_PES;
+        port->ctrl |= OHCI_PORT_PESC;
+    }
+    trace_usb_ohci_port_detach(port1->index);
+
+    if (old_state != port->ctrl) {
+        ohci_set_interrupt(s, OHCI_INTR_RHSC);
+    }
+}
+
+static void ohci_wakeup(USBPort *port1)
+{
+    OHCIState *s = port1->opaque;
+    OHCIPort *port = &s->rhport[port1->index];
+    uint32_t intr = 0;
+    if (port->ctrl & OHCI_PORT_PSS) {
+        trace_usb_ohci_port_wakeup(port1->index);
+        port->ctrl |= OHCI_PORT_PSSC;
+        port->ctrl &= ~OHCI_PORT_PSS;
+        intr = OHCI_INTR_RHSC;
+    }
+    /* Note that the controller can be suspended even if this port is not */
+    if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) {
+        trace_usb_ohci_remote_wakeup(s->name);
+        /* This is the one state transition the controller can do by itself */
+        s->ctl &= ~OHCI_CTL_HCFS;
+        s->ctl |= OHCI_USB_RESUME;
+        /* In suspend mode only ResumeDetected is possible, not RHSC:
+         * see the OHCI spec 5.1.2.3.
+         */
+        intr = OHCI_INTR_RD;
+    }
+    ohci_set_interrupt(s, intr);
+}
+
+static void ohci_async_complete_packet(USBPort *port, USBPacket *packet)
+{
+    OHCIState *ohci = container_of(packet, OHCIState, usb_packet);
+
+    trace_usb_ohci_async_complete();
+    ohci->async_complete = true;
+    ohci_process_lists(ohci, 1);
+}
 
 static USBPortOps ohci_port_ops = {
     .attach = ohci_attach,