Am Mittwoch, 2. Juni 2010 10:59:56 schrieb Du, Alek: > >-----Original Message----- > >From: Oliver Neukum [mailto:oneukum@xxxxxxx] > >Sent: Wednesday, June 02, 2010 4:41 PM > >To: Du, Alek > >Cc: greg@xxxxxxxxx; david-b@xxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; Pan, > >Jacob jun > >Subject: Re: [PATCH 2/3] EHCI: EHCI 1.1 addendum: Basic LPM feature support > >> @@ -855,7 +873,16 @@ static int ehci_urb_enqueue ( > >> default: > >> if (!qh_urb_transaction (ehci, urb, &qtd_list, mem_flags)) > >> return -ENOMEM; > >> - return submit_async(ehci, urb, &qtd_list, mem_flags); > >> + status = submit_async(ehci, urb, &qtd_list, mem_flags); > >> + > >> + /* check device LPM cap after set address */ > >> + if (usb_pipecontrol(urb->pipe)) { > >> + if (((struct usb_ctrlrequest *)urb->setup_packet) > >> + ->bRequest == USB_REQ_SET_ADDRESS && > >> + ehci->has_lpm) > >> + ehci_lpm_check(ehci, urb->dev->portnum); > >> + } > >> + return status; > > > >This is very ugly. The check should not be here. > > > Hmmm... ok, let me find a better place, or do you have any suggestions? It needs to be checked for a new device and might change with a reset. As new devices are reset anyway I suggest you use the hcd->driver->reset_device hook of EHCI. > > > > >> +/* > >> + * this function is called to put a link into L1 state. the steps are: > >> + * - verify HC supports LPM > >> + * - make sure all pipe idle on the link > >> + * - shutdown all qh on the pipe > >> + * - send LPM packet > >> + * - confirm device ack > >> + */ > >> +static unsigned ehci_lpm_check(struct ehci_hcd *ehci, int port) > >> +{ > >[..] > >> + if (retval != -ETIMEDOUT) { > >> + ehci_dbg(ehci, "LPM: device ACK for LPM\n"); > >> + val32 |= PORT_LPM; > >> + /* > >> + * now device should be in L1 sleep, let's wake up the device > >> + * so that we can complete enumeration. > >> + */ > >> + ehci_writel(ehci, val32, portsc); > >> + msleep(10); > >> + val32 |= PORT_RESUME; > >> + ehci_writel(ehci, val32, portsc); > >> + } else { > >> + ehci_dbg(ehci, "LPM: device does not ACK, disable LPM %d\n", > >> + retval); > >> + val32 &= ~PORT_LPM; > >> + retval = -ETIMEDOUT; > >> + ehci_writel(ehci, val32, portsc); > >> + } > > > >This is bold. Do you have any experimental data how real devices react? > > For most current devices, of course, will go to the "LPM: device does not ACK" way, and > this has no side effects. We used the on board USB OTG port as USB device that could > support LPM to do the test. I am afraid that some devices might show side effects. Have you actually tested on devices that don't support LPM? > >> + return retval; > >> +} > >> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c > >> index 11a79c4..0c39f49 100644 > >> --- a/drivers/usb/host/ehci-q.c > >> +++ b/drivers/usb/host/ehci-q.c > >> @@ -643,6 +643,16 @@ qh_urb_transaction ( > >> sizeof (struct usb_ctrlrequest), > >> token | (2 /* "setup" */ << 8), 8); > >> > >> + if (((struct usb_ctrlrequest *)urb->setup_packet)->bRequest > >> + == USB_REQ_SET_ADDRESS) { > >> + /* for LPM capable HC, set up device address*/ > >> + int dev_address = ((struct usb_ctrlrequest *) > >> + (urb->setup_packet))->wValue; > >> + if (ehci->has_lpm) > >> + ehci_lpm_set_da(ehci, dev_address, > >> + urb->dev->portnum); > >> + } > > > >No. Please make an explicit hook for this in usbcore. > > Thanks, but I do not know if we can make it ehci specific? Does anything but a subset of EHCI support LPM? I am not sure that I understand the question. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
