> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Wednesday, January 12, 2011 4:07 AM > To: Xu, Andiry > Cc: gregkh@xxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > dbrownell@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH 1/2] USB host: Move AMD PLL quirk to pci-quirks.c > > 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? > The PLL is set in SB/NB registers, not OCHI/EHCI/xHCI registers. Here "PLL share" means share the quirk code. PLL is set differently according to the SB/NB models, regardless of USB host controllers. Although xHCI can handle all device speeds, I think it's common that people plug USB2.0/1.1 devices to EHCI/OHCI controllers, even if there are xHCI controllers on the platform too. And I think it's regular that a system has OHCI/EHCI/xHCI controllers at the same time. For example, my develop platform has 2 EHCI, 3 OHCI and 2 xHCI controllers. AFAIK, many platforms on the market have xHCI as well as EHCI and OHCI/UHCI controllers. > > 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? > Sure. I'll add the check of pdev->vendor and scan the chipset only on AMD/ATI platforms. > > + 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? > ret is a static variable. The logic of this function is: The first caller probes the whole chipset info, and set ret depends on the probe result; The following callers just increase probe_count and return ret, save the cost to probe again. So cannot just return 1 here. I'll modify the function that the first caller should hold the spinlock until it sets the ret value. > > +} > > +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. > Sure, will do. > > +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. > I've thought about using atomic variables. But amd_find_chipset_info() needs a spinlock to protect the probe process. And atomic_inc_and_test() just compares the variable to 0. I'm not sure whether clauses like "if (atomic_inc_return(v) > 1)" spoils the atomic attributes. 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
