| Message ID | 20101125153455.GA4097@amt.cnet |
|---|---|
| State | New |
| Headers | show |
Hi, > + dev->info->remote_wakeup_cb = uhci_event; > diff --git a/hw/usb.h b/hw/usb.h > index 00d2802..16de1c9 100644 > --- a/hw/usb.h > +++ b/hw/usb.h > @@ -189,6 +189,11 @@ struct USBDeviceInfo { > */ > int (*handle_data)(USBDevice *dev, USBPacket *p); > > + /* > + * Process remote wakeup request. > + */ > + void (*remote_wakeup_cb)(USBDevice *dev); > + No way. DeviceInfo holds informations about the device *implementation*. Multiple instances will share that, placing runtime data there is just plain wrong. Also this is a callback from the usb device to the usb host adapter, not the other way around. I'd suggest to create a USBBusOps for host adapter callbacks and and stick it into USBBus. cheers, Gerd
See patch 2 for details. v2: - Add remote wakeup callback in USBBus, as suggested by Gerd.
> This patch enables USB UHCI global suspend/resume feature. The OS will > stop the HC once all ports are suspended. If there is activity on the > port(s), an interrupt signalling remote wakeup will be triggered. I'm pretty sure this is wrong. Suspend/resume works based on physical topology, i.e. the resume notification should go to the the port/hub to which the device is connected, not directly to the host controller. Paul
On Fri, Nov 26, 2010 at 12:38:28AM +0000, Paul Brook wrote: > > This patch enables USB UHCI global suspend/resume feature. The OS will > > stop the HC once all ports are suspended. If there is activity on the > > port(s), an interrupt signalling remote wakeup will be triggered. > > I'm pretty sure this is wrong. Suspend/resume works based on physical > topology, i.e. the resume notification should go to the the port/hub to which > the device is connected, not directly to the host controller. If the host controller is in global suspend state, and resume is received on any of its root hub ports (given that remote wakeup is enabled for the given port), the system will be interrupted if interrupt enable bit is set. 2.1.2 USB STATUS REGISTER I/O Address: Base + (02-03h) Default Value: 0000h Attribute: Read/Write Clear size: 16 bits This register indicates pending interrupts and various states of the Host Controller Resume Detect. The Host Controller sets this bit to 1 when it receives a “RESUME” signal from a USB device. This is only valid if the Host Controller is in a global suspend state (bit 3 of Command register = 1). 2.1.7.1 Behavior Under Global or Selective Suspend Scenarios Resume will be recognized in the USBSTS register (bit 2) if resume is received on a suspended or enabled port when the Host Controller is in the global suspend state (USBCMD register bit 3 is set). 4.2.1 RESUME RECEIVED This event indicates that the HC received a RESUME signal from a device on the USB during a global suspend. If this interrupt is enabled in the HC Interrupt Enable register, a hardware interrupt will be signaled to the system allowing the USB to be brought out of the suspend state and returned to normal operation. You are correct in that USB HUB emulation does not propagate resume, but this does not make this patch incorrect.
On 11/26/10 03:15, Marcelo Tosatti wrote: > On Fri, Nov 26, 2010 at 12:38:28AM +0000, Paul Brook wrote: >>> This patch enables USB UHCI global suspend/resume feature. The OS will >>> stop the HC once all ports are suspended. If there is activity on the >>> port(s), an interrupt signalling remote wakeup will be triggered. >> >> I'm pretty sure this is wrong. Suspend/resume works based on physical >> topology, i.e. the resume notification should go to the the port/hub to which >> the device is connected, not directly to the host controller. > You are correct in that USB HUB emulation does not propagate resume, but > this does not make this patch incorrect. Well, it does. When the notification is port based our software model should better reflect that, so we have the chance to add resume propagation to the hub emulation later on. I guess the Ops should be moved from the USBBus to the USBPort to reflect that. This way the hub emulation and the uhci root hub can have different callbacks, which is needed to get this correct. cheers, Gerd
> On 11/26/10 03:15, Marcelo Tosatti wrote: > > On Fri, Nov 26, 2010 at 12:38:28AM +0000, Paul Brook wrote: > >>> This patch enables USB UHCI global suspend/resume feature. The OS will > >>> stop the HC once all ports are suspended. If there is activity on the > >>> port(s), an interrupt signalling remote wakeup will be triggered. > >> > >> I'm pretty sure this is wrong. Suspend/resume works based on physical > >> topology, i.e. the resume notification should go to the the port/hub to > >> which the device is connected, not directly to the host controller. > > > > You are correct in that USB HUB emulation does not propagate resume, but > > this does not make this patch incorrect. > > Well, it does. When the notification is port based our software model > should better reflect that, so we have the chance to add resume > propagation to the hub emulation later on. Exactly. The patch assumes the device is connected to a root hub port. This assumption is incorrect. The device should be sending the resume signal to the port/hub to which it is connected. If that hub is still active it will reactivate the port, and flag a port change notification in the normal manner. If the hub is also suspended it will propagate the resume notification upstream (which may or may not be the root hub). Paul
v3: - Move remote wakeup callback to USBPort - Add subsection
diff --git a/hw/usb-hid.c b/hw/usb-hid.c index 882d933..b7a4dc1 100644 --- a/hw/usb-hid.c +++ b/hw/usb-hid.c @@ -412,6 +412,9 @@ static void usb_hid_changed(USBHIDState *hs) if (hs->datain) hs->datain(hs->datain_opaque); + + if (hs->dev.remote_wakeup) + usb_remote_wakeup(&hs->dev); } static void usb_mouse_event(void *opaque, diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c index 1d83400..674cb0c 100644 --- a/hw/usb-uhci.c +++ b/hw/usb-uhci.c @@ -59,6 +59,7 @@ #define UHCI_PORT_RESET (1 << 9) #define UHCI_PORT_LSDA (1 << 8) +#define UHCI_PORT_RD (1 << 6) #define UHCI_PORT_ENC (1 << 3) #define UHCI_PORT_EN (1 << 2) #define UHCI_PORT_CSC (1 << 1) @@ -501,6 +502,7 @@ static void uhci_ioport_writew(void *opaque, uint32_t addr, uint32_t val) port->ctrl = (port->ctrl & 0x01fb) | (val & ~0x01fb); /* some bits are reset when a '1' is written to them */ port->ctrl &= ~(val & 0x000a); + port->ctrl &= ~(port->ctrl & 0x0040); /* clear port resume detected */ } break; } @@ -593,6 +595,43 @@ static void uhci_resume (void *opaque) } } +static UHCIPort *find_port(UHCIState *s, USBDevice *d) +{ + int i; + + for (i = 0; i < NB_PORTS; i++) { + UHCIPort *port = &s->ports[i]; + + if (d == port->port.dev) { + return port; + } + } + + return NULL; +} + +static void uhci_event(USBDevice *dev) +{ + USBBus *bus = usb_bus_from_device(dev); + UHCIState *s = container_of(bus, UHCIState, bus); + + if (s->cmd & UHCI_CMD_EGSM) { + UHCIPort *port = find_port(s, dev); + + if (!port) { + return; + } + + if (port->ctrl & UHCI_PORT_RD) { + return; + } + + port->ctrl |= UHCI_PORT_RD; + + uhci_resume(s); + } +} + static void uhci_attach(USBPort *port1, USBDevice *dev) { UHCIState *s = port1->opaque; @@ -602,6 +641,7 @@ static void uhci_attach(USBPort *port1, USBDevice *dev) if (port->port.dev) { usb_attach(port1, NULL); } + dev->info->remote_wakeup_cb = uhci_event; /* set connect status */ port->ctrl |= UHCI_PORT_CCS | UHCI_PORT_CSC; diff --git a/hw/usb.c b/hw/usb.c index a326bcf..9b24d49 100644 --- a/hw/usb.c +++ b/hw/usb.c @@ -229,3 +229,9 @@ void usb_send_msg(USBDevice *dev, int msg) /* This _must_ be synchronous */ } + +void usb_remote_wakeup(USBDevice *dev) +{ + if (dev->info->remote_wakeup_cb) + dev->info->remote_wakeup_cb(dev); +} diff --git a/hw/usb.h b/hw/usb.h index 00d2802..16de1c9 100644 --- a/hw/usb.h +++ b/hw/usb.h @@ -189,6 +189,11 @@ struct USBDeviceInfo { */ int (*handle_data)(USBDevice *dev, USBPacket *p); + /* + * Process remote wakeup request. + */ + void (*remote_wakeup_cb)(USBDevice *dev); + const char *product_desc; /* handle legacy -usbdevice command line options */ @@ -317,6 +322,7 @@ void usb_unregister_port(USBBus *bus, USBPort *port); int usb_device_attach(USBDevice *dev); int usb_device_detach(USBDevice *dev); int usb_device_delete_addr(int busnr, int addr); +void usb_remote_wakeup(USBDevice *dev); static inline USBBus *usb_bus_from_device(USBDevice *d) {
This patch enables USB UHCI global suspend/resume feature. The OS will stop the HC once all ports are suspended. If there is activity on the port(s), an interrupt signalling remote wakeup will be triggered. To enable autosuspend for the USB tablet on Linux guests: echo auto > /sys/devices/pci0000:00/0000:00:01.2/usb1/1-1/power/level It reduces CPU consumption of an idle FC12 guest from 2.7% to 0.3%. Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>