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.
> /* 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.
> 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?
Sarah Sharp
--
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
