> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Wednesday, January 12, 2011 4:34 AM > To: Xu, Andiry > Cc: linux-usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxx; Alan Stern > Subject: Re: [PATCH 2/2] xHCI: Implement AMD PLL quirk > > Comments below. > > On Tue, Jan 11, 2011 at 05:57:22PM +0800, Andiry Xu wrote: > > This patch disable the optional PM feature inside the Hudson3 platform > under > > the following conditions: > > > > 1. If an isochronous device is connected to xHCI port and is active; > > 2. Optional PM feature that powers down the internal Bus PLL when the > link is > > in low power state is enabled. > > > > The PM feature needs to be disabled to eliminate PLL startup delays when > the > > link comes out of low power state. The performance of DMA data transfer > could > > be impacted if system delay were encountered and in addition to the PLL > start > > up delays. Disabling the PM would leave room for unpredictable system > delays > > in order to guarantee uninterrupted data transfer to isochronous audio > or > > video stream devices that require time sensitive information. If data in > an > > audio/video stream was interrupted then erratic audio or video > performance > > may be encountered. > > > > AMD PLL quirk is already implemented in OHCI/EHCI driver. After moving > the > > quirk code to pci-quirks.c and export them, xHCI driver can call it > directly > > without having the quirk implementation in itself. > > > > Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx> > > --- > > drivers/usb/host/xhci-pci.c | 2 ++ > > drivers/usb/host/xhci-ring.c | 29 ++++++++++++++++++++++++++++- > > drivers/usb/host/xhci.c | 3 +++ > > drivers/usb/host/xhci.h | 2 ++ > > 4 files changed, 35 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c > > index bb668a8..08d9ee5 100644 > > --- a/drivers/usb/host/xhci-pci.c > > +++ b/drivers/usb/host/xhci-pci.c > > @@ -81,6 +81,8 @@ static int xhci_pci_setup(struct usb_hcd *hcd) > > } > > if (pdev->vendor == PCI_VENDOR_ID_NEC) > > xhci->quirks |= XHCI_NEC_HOST; > > + if (amd_find_chipset_info()) > > + xhci->quirks |= XHCI_AMD_PLL_FIX; > > Can you please only call amd_find_chipset_info() if the xHCI PCI device > being probed is an AMD host controller? Just check pdev->vendor. > OK. > > /* Make sure the HC is halted. */ > > retval = xhci_halt(xhci); > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > > index df558f6..9697e08 100644 > > --- a/drivers/usb/host/xhci-ring.c > > +++ b/drivers/usb/host/xhci-ring.c > > @@ -609,6 +609,15 @@ static void xhci_giveback_urb_in_irq(struct > xhci_hcd *xhci, > > > > /* Only giveback urb when this is the last td in urb */ > > if (urb_priv->td_cnt == urb_priv->length) { > > + switch (usb_pipetype(urb->pipe)) { > > + case PIPE_ISOCHRONOUS: > > + xhci_to_hcd(xhci)->self.bandwidth_isoc_reqs--; > > + if (xhci_to_hcd(xhci)->self.bandwidth_isoc_reqs == 0) { > > + if (xhci->quirks & XHCI_AMD_PLL_FIX) > > + amd_quirk_pll(0); > > + } > > + break; > > + } > > usb_hcd_unlink_urb_from_ep(hcd, urb); > > Ok, I think putting the decrement here also handles the case where the > isochronous URB was canceled. > > It's a bit hard to figure out at a glance what you're doing with > amd_quirk_pll() here. Instead of using zeros and ones to call into > amd_quirk_pll(), can you add a #define in pci-quirks.h for them. > Perhaps something like: > > #define USB_AMD_PLL_ENABLE 0 > #define USB_AMD_PLL_DISABLE 1 > > That way you'll be less likely to confuse yourself or others. You can > also test for those values in amd_quirk_pll(). > > You might also want to name the amd_quirk_pll() after the action it > performs, not the noun it's performing on. Something like > > usb_control_amd_pll(USB_AMD_PLL_ENABLE); > > is more clear than > > amd_quirk_pll(0); > > I think you'll want that function to have usb_ in the beginning, since > most of the GPL exported functions in the USB core have that naming > convention. You'd have to ask Alan about the naming convention though. > Thanks for the point. I'll consider that. Alan, Are there some naming rules of USB core exported functions? Thanks. > > xhci_dbg(xhci, "Giveback %s URB %p\n", adjective, urb); > > > > @@ -1453,8 +1462,20 @@ td_cleanup: > > > > urb_priv->td_cnt++; > > /* Giveback the urb when all the tds are completed */ > > - if (urb_priv->td_cnt == urb_priv->length) > > + if (urb_priv->td_cnt == urb_priv->length) { > > ret = 1; > > + > > + switch (usb_pipetype(urb->pipe)) { > > + case PIPE_ISOCHRONOUS: > > + xhci_to_hcd(xhci)->self.bandwidth_isoc_reqs--; > > + if (xhci_to_hcd(xhci)->self.bandwidth_isoc_reqs > > + == 0) { > > + if (xhci->quirks & XHCI_AMD_PLL_FIX) > > + amd_quirk_pll(0); > > + } > > + break; > > + } > > + } > > } > > > > return ret; > > @@ -3003,6 +3024,12 @@ static int xhci_queue_isoc_tx(struct xhci_hcd > *xhci, gfp_t mem_flags, > > } > > } > > > > + if (xhci_to_hcd(xhci)->self.bandwidth_isoc_reqs == 0) { > > + if (xhci->quirks & XHCI_AMD_PLL_FIX) > > + amd_quirk_pll(1); > > + } > > + xhci_to_hcd(xhci)->self.bandwidth_isoc_reqs++; > > + > > wmb(); > > start_trb->field[3] |= start_cycle; > > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > index 45e4a31..fd23c98 100644 > > --- a/drivers/usb/host/xhci.c > > +++ b/drivers/usb/host/xhci.c > > @@ -517,6 +517,9 @@ void xhci_stop(struct usb_hcd *hcd) > > del_timer_sync(&xhci->event_ring_timer); > > #endif > > > > + if (xhci->quirks & XHCI_AMD_PLL_FIX) > > + amd_nb_dev_put(); > > + > > What if the host controller is stopped as part of a re-init after a > failed context restore on system resume? Does that still do what you > want? Also, isn't the host controller stopped as part of the > initialization sequence? Why would you want to drop the count on the > PCI device in that case? > > This doesn't seem like the right place to put this call. Do you really > want to know when the driver's PCI shutdown function is called? I know > there's no host controller driver hook for that (even though we have one > for PCI probe), but perhaps you just need to add it? > The amd_nb_dev_put() is called in xhci_stop(), while the function called in xhci_resume() and xhci_pci_setup() is xhci_halt(). Xhci_stop() is called only in usb_remove_hcd() and I think it's just the opposite of xhci_pci_setup(), so increase the probe_count in xhci_pci_setup() and decrease it in xhci_stop() is reasonable. Please correct me if I'm wrong. Thanks, Andiry -- 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
