On Tue, Jan 11, 2011 at 05:57:02PM +0800, Andiry Xu wrote:
> This patch move the AMD PLL quirk code in OHCI/EHCI driver to pci-quirks.c,
> and export 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 have several advantages:
>
> 1. Remove repeat 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.
So is the PLL shared across the OHCI/EHCI/xHCI hosts? I could
understand it being shared across the OHCI/EHCI hosts, but it seems a
bit odd to also share it across the xHCI host. It also seems a bit odd
to have an EHCI or OHCI host controller in a system with an xHCI host
controller, since xHCI is supposed to handle all device speeds.
Are you sure the PLL is shared by all three hosts, and there's an EHCI
or OHCI host in the same system as an xHCI host?
I have a couple more comments below.
> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
> index 76179c3..d52be7a 100644
> --- a/drivers/usb/host/ehci-pci.c
> +++ b/drivers/usb/host/ehci-pci.c
...
> /* called during probe() after chip reset completes */
> static int ehci_pci_setup(struct usb_hcd *hcd)
> {
> @@ -131,8 +102,8 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
> /* cache this readonly data; minimize chip reads */
> ehci->hcs_params = ehci_readl(ehci, &ehci->caps->hcs_params);
>
> - if (ehci_quirk_amd_SB800(ehci))
> - ehci->amd_l1_fix = 1;
> + if (amd_find_chipset_info())
> + ehci->amd_pll_fix = 1;
>
> retval = ehci_halt(ehci);
> if (retval)
...
> diff --git a/drivers/usb/host/ohci-pci.c b/drivers/usb/host/ohci-pci.c
> index 36ee9a6..4b7efbb 100644
> --- a/drivers/usb/host/ohci-pci.c
> +++ b/drivers/usb/host/ohci-pci.c
...
> @@ -168,11 +150,14 @@ static int ohci_quirk_nec(struct usb_hcd *hcd)
> static int ohci_quirk_amd700(struct usb_hcd *hcd)
> {
> struct ohci_hcd *ohci = hcd_to_ohci(hcd);
> + struct pci_dev *amd_smbus_dev;
> u8 rev = 0;
>
> - if (!amd_smbus_dev)
> - amd_smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI,
> - PCI_DEVICE_ID_ATI_SBX00_SMBUS, NULL);
> + if (amd_find_chipset_info())
> + ohci->flags |= OHCI_QUIRK_AMD_PLL;
> +
> + amd_smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI,
> + PCI_DEVICE_ID_ATI_SBX00_SMBUS, NULL);
> if (!amd_smbus_dev)
> return 0;
...
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 4c502c8..0272dc1 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -53,6 +53,229 @@
> #define EHCI_USBLEGCTLSTS_SOOE (1 << 13) /* SMI on ownership change */
>
>
> +/* AMD quirk use */
> +#define AB_REG_BAR_LOW 0xe0
> +#define AB_REG_BAR_HIGH 0xe1
> +#define AB_REG_BAR_SB700 0xf0
> +#define AB_INDX(addr) ((addr) + 0x00)
> +#define AB_DATA(addr) ((addr) + 0x04)
> +#define AX_INDXC 0x30
> +#define AX_DATAC 0x34
> +
> +#define NB_PCIE_INDX_ADDR 0xe0
> +#define NB_PCIE_INDX_DATA 0xe4
> +#define PCIE_P_CNTL 0x10040
> +#define BIF_NB 0x10002
> +#define NB_PIF0_PWRDOWN_0 0x01100012
> +#define NB_PIF0_PWRDOWN_1 0x01100013
> +
> +static struct pci_dev *amd_nb_dev;
> +static struct pci_dev *amd_smbus_dev;
> +static int amd_nb_type;
> +static int amd_sb_type;
> +static int probe_count;
> +static DEFINE_SPINLOCK(amd_lock);
> +
> +int amd_find_chipset_info(void)
> +{
> + u8 rev = 0;
> + static int ret;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&amd_lock, flags);
> +
> + probe_count++;
> + /* probe only once */
> + if (probe_count > 1) {
> + spin_unlock_irqrestore(&amd_lock, flags);
> + return ret;
> + }
> +
> + spin_unlock_irqrestore(&amd_lock, flags);
> +
> + amd_smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
So you're making the PCI core scan the bus for an ATI chipset? Won't
that take a lot of time at boot? Will the scan happen if the driver is
loaded on a system without ATI or AMD chipsets?
Is there a way you can avoid the scan on non-AMD systems, perhaps by
looking at the PCI device you're probing and seeing that it's not an ATI
or AMD USB host controller?
> + if (amd_smbus_dev) {
> + pci_read_config_byte(amd_smbus_dev, PCI_REVISION_ID, &rev);
> + if (rev >= 0x40)
> + amd_sb_type = 1;
> + else if (rev >= 0x30 && rev <= 0x3b)
> + amd_sb_type = 3;
> + } else {
> + amd_smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x780b, NULL);
> + if (!amd_smbus_dev)
> + return 0;
> + pci_read_config_byte(amd_smbus_dev, PCI_REVISION_ID, &rev);
> + if (rev >= 0x11 && rev <= 0x18)
> + amd_sb_type = 2;
> + }
> +
> + if (amd_sb_type == 0) {
> + if (amd_smbus_dev) {
> + pci_dev_put(amd_smbus_dev);
> + amd_smbus_dev = NULL;
> + }
> + return 0;
> + }
> +
> + amd_nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
> + if (amd_nb_dev) {
> + amd_nb_type = 1;
> + } else {
> + amd_nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x1510, NULL);
> + if (amd_nb_dev) {
> + amd_nb_type = 2;
> + } else {
> + amd_nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
> + 0x9600, NULL);
> + if (amd_nb_dev)
> + amd_nb_type = 3;
> + }
> + }
> +
> + ret = 1;
> + printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
> +
> + return ret;
Why not just say "return 1;" here?
> +}
> +EXPORT_SYMBOL_GPL(amd_find_chipset_info);
> +
> +/*
> + * The hardware normally enables the A-link power management feature, which
> + * lets the system lower the power consumption in idle states.
> + *
> + * Without this quirk, isochronous stream on OHCI/EHCI/xHCI controllers of
> + * some AMD platforms may stutter or have breaks occasionally.
> + *
> + * This USB quirk prevents the link going into that lower power state
> + * during isochronous transfers.
> + */
Can you swap the last two sentences? It makes more sense that way.
> +void amd_quirk_pll(int disable)
> +{
> + u32 addr, addr_low, addr_high, val;
> + u32 bit = disable ? 0 : 1;
> + static int isoc_reqs;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&amd_lock, flags);
> +
> + if (disable) {
> + isoc_reqs++;
> + if (isoc_reqs > 1) {
> + spin_unlock_irqrestore(&amd_lock, flags);
> + return;
> + }
> + } else {
> + isoc_reqs--;
> + if (isoc_reqs > 0) {
> + spin_unlock_irqrestore(&amd_lock, flags);
> + return;
> + }
> + }
> +
> + spin_unlock_irqrestore(&amd_lock, flags);
> +
> + if (amd_sb_type == 1 || amd_sb_type == 2) {
> + outb_p(AB_REG_BAR_LOW, 0xcd6);
> + addr_low = inb_p(0xcd7);
> + outb_p(AB_REG_BAR_HIGH, 0xcd6);
> + addr_high = inb_p(0xcd7);
> + addr = addr_high << 8 | addr_low;
> +
> + outl_p(0x30, AB_INDX(addr));
> + outl_p(0x40, AB_DATA(addr));
> + outl_p(0x34, AB_INDX(addr));
> + val = inl_p(AB_DATA(addr));
> + } else if (amd_sb_type == 3) {
> + pci_read_config_dword(amd_smbus_dev, AB_REG_BAR_SB700, &addr);
> + outl(AX_INDXC, AB_INDX(addr));
> + outl(0x40, AB_DATA(addr));
> + outl(AX_DATAC, AB_INDX(addr));
> + val = inl(AB_DATA(addr));
> + } else
> + return;
> +
> + if (disable) {
> + val &= ~0x08;
> + val |= (1 << 4) | (1 << 9);
> + } else {
> + val |= 0x08;
> + val &= ~((1 << 4) | (1 << 9));
> + }
> + outl_p(val, AB_DATA(addr));
> +
> + if (!amd_nb_dev)
> + return;
> +
> + if (amd_nb_type == 1 || amd_nb_type == 3) {
> + addr = PCIE_P_CNTL;
> + pci_write_config_dword(amd_nb_dev, NB_PCIE_INDX_ADDR, addr);
> + pci_read_config_dword(amd_nb_dev, NB_PCIE_INDX_DATA, &val);
> +
> + val &= ~(1 | (1 << 3) | (1 << 4) | (1 << 9) | (1 << 12));
> + val |= bit | (bit << 3) | (bit << 12);
> + val |= ((!bit) << 4) | ((!bit) << 9);
> + pci_write_config_dword(amd_nb_dev, NB_PCIE_INDX_DATA, val);
> +
> + addr = BIF_NB;
> + pci_write_config_dword(amd_nb_dev, NB_PCIE_INDX_ADDR, addr);
> +
> + pci_read_config_dword(amd_nb_dev, NB_PCIE_INDX_DATA, &val);
> + val &= ~(1 << 8);
> + val |= bit << 8;
> + pci_write_config_dword(amd_nb_dev, NB_PCIE_INDX_DATA, val);
> + } else if (amd_nb_type == 2) {
> + addr = NB_PIF0_PWRDOWN_0;
> + pci_write_config_dword(amd_nb_dev, NB_PCIE_INDX_ADDR, addr);
> + pci_read_config_dword(amd_nb_dev, NB_PCIE_INDX_DATA, &val);
> + if (disable)
> + val &= ~(0x3f << 7);
> + else
> + val |= 0x3f << 7;
> +
> + pci_write_config_dword(amd_nb_dev, NB_PCIE_INDX_DATA, val);
> +
> + addr = NB_PIF0_PWRDOWN_1;
> + pci_write_config_dword(amd_nb_dev, NB_PCIE_INDX_ADDR, addr);
> + pci_read_config_dword(amd_nb_dev, NB_PCIE_INDX_DATA, &val);
> + if (disable)
> + val &= ~(0x3f << 7);
> + else
> + val |= 0x3f << 7;
> +
> + pci_write_config_dword(amd_nb_dev, NB_PCIE_INDX_DATA, val);
> + }
> +
> + return;
> +}
> +EXPORT_SYMBOL_GPL(amd_quirk_pll);
I really can't comment on the AMD-specific code without documentation
for the chipsets. You should send this patch for review by the folks
that did the original AMD PLL quirks, as Alan suggested.
> +
> +void amd_nb_dev_put(void)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&amd_lock, flags);
> +
> + probe_count--;
> + if (probe_count > 0) {
> + spin_unlock_irqrestore(&amd_lock, flags);
> + return;
> + }
> +
> + spin_unlock_irqrestore(&amd_lock, flags);
Should probe_count just be an atomic variable? I think it would be less
expensive to have the CPU uses atomic increment and decrement
instructions than manage a lock (potentially across multiple cores) and
modify a normal integer.
> +
> + if (amd_nb_dev) {
> + pci_dev_put(amd_nb_dev);
> + amd_nb_dev = NULL;
> + }
> + if (amd_smbus_dev) {
> + pci_dev_put(amd_smbus_dev);
> + amd_smbus_dev = NULL;
> + }
> + amd_nb_type = 0;
> + amd_sb_type = 0;
> +}
> +EXPORT_SYMBOL_GPL(amd_nb_dev_put);
> +
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
