>-----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? > >> +/* >> + * 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. >> + 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? Alek > > 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
