On Fri, 21 Jan 2011, Andiry Xu wrote:
> This patch moves the AMD PLL quirk code in OHCI/EHCI driver to pci-quirks.c,
> and exports the functions to be used by xHCI driver later.
>
> AMD PLL quirk disable the optional PM feature inside specific
> SB700/SB800/Hudson-2/3 platforms under the following conditions:
>
> 1. If an isochronous device is connected to OHCI/EHCI/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.
>
> Without AMD PLL quirk, USB isochronous stream may stutter or have breaks
> occasionally, which greatly impair the performance of audio/video streams.
>
> Currently AMD PLL quirk is implemented in OHCI and EHCI driver, and will be
> added to xHCI driver too. They are doing similar things actually, so move
> the quirk code to pci-quirks.c, which has several advantages:
>
> 1. Remove duplicate defines and functions in OHCI/EHCI (and xHCI) driver and
> make them cleaner;
> 2. AMD chipset information will be probed only once and then stored.
> Currently they're probed during every OHCI/EHCI initialization, move
> the detect code to pci-quirks.c saves the repeat detect cost;
> 3. Build up synchronization among OHCI/EHCI/xHCI driver. In current
> code, every host controller enable/disable PLL only according to
> its own status, and may enable PLL while there is still isoc transfer on
> other HCs. Move the quirk to pci-quirks.c prevents this issue.
I see one big problem with this patch. It will break on systems that
don't have PCI.
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 6fee3cd..5655832 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -114,12 +114,10 @@ MODULE_PARM_DESC(hird, "host initiated resume duration, +1 for each 75us\n");
>
> #define INTR_MASK (STS_IAA | STS_FATAL | STS_PCD | STS_ERR | STS_INT)
>
> -/* for ASPM quirk of ISOC on AMD SB800 */
> -static struct pci_dev *amd_nb_dev;
> -
> /*-------------------------------------------------------------------------*/
>
> #include "ehci.h"
> +#include "pci-quirks.h"
> #include "ehci-dbg.c"
>
> /*-------------------------------------------------------------------------*/
> @@ -507,6 +505,7 @@ static void ehci_work (struct ehci_hcd *ehci)
> static void ehci_stop (struct usb_hcd *hcd)
> {
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> + struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
This statement is blatantly wrong. Plenty of EHCI controllers are not
PCI devices.
> ehci_dbg (ehci, "stop\n");
>
> @@ -532,10 +531,9 @@ static void ehci_stop (struct usb_hcd *hcd)
> spin_unlock_irq (&ehci->lock);
> ehci_mem_cleanup (ehci);
>
> - if (amd_nb_dev) {
> - pci_dev_put(amd_nb_dev);
> - amd_nb_dev = NULL;
> - }
> + if (pdev->vendor == PCI_VENDOR_ID_ATI ||
> + pdev->vendor == PCI_VENDOR_ID_AMD)
> + usb_amd_dev_put();
In the end, I think you were better off with the original quirk-flag
approach. At least, it works for all architectures and platforms.
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 4c502c8..99dc93a 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
Notice that this file is not included in the kernel unless CONFIG_PCI
is set.
> +static struct amd_chipset_info {
> + struct pci_dev *nb_dev;
> + struct pci_dev *smbus_dev;
> + int nb_type;
> + int sb_type;
> + int isoc_reqs;
> + int probe_count;
> + int probe_result;
> +} amd_chipset;
> +inline int usb_amd_need_pll_quirk(void)
> +{
> + return amd_chipset.probe_result;
> +}
> +EXPORT_SYMBOL_GPL(usb_amd_need_pll_quirk);
You can't inline an EXPORTed function. The EXPORT prevents the
compiler from making it inline.
Also, builds will fail when drivers try to call
usb_amd_need_pll_quirk() and related functions on systems with
CONFIG_PCI not enabled. This bug probably was present in earlier
versions of this patch but I failed to notice it.
Alan Stern
--
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
