> -----Original Message----- > From: Sergei Shtylyov [mailto:sshtylyov@xxxxxxxxxx] > Sent: Tuesday, February 15, 2011 4:18 AM > To: Xu, Andiry > Cc: gregkh@xxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > dbrownell@xxxxxxxxxxxxxxxxxxxxx; sarah.a.sharp@xxxxxxxxxxxxxxx; He, Alex > Subject: Re: [PATCH v6 1/2] USB host: Move AMD PLL quirk to pci-quirks.c > > Hello. > > 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. > > > Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx> > > Cc: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx> > > Cc: Alex He <alex.he@xxxxxxx> > [...] > > > diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c > > index 76179c3..1ec8060 100644 > > --- a/drivers/usb/host/ehci-pci.c > > +++ b/drivers/usb/host/ehci-pci.c > > @@ -44,35 +44,6 @@ static int ehci_pci_reinit(struct ehci_hcd *ehci, > struct pci_dev *pdev) > > return 0; > > } > > > > -static int ehci_quirk_amd_SB800(struct ehci_hcd *ehci) > > -{ > > - struct pci_dev *amd_smbus_dev; > > - u8 rev = 0; > > - > > - amd_smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL); > > - if (!amd_smbus_dev) > > - return 0; > > - > > - pci_read_config_byte(amd_smbus_dev, PCI_REVISION_ID, &rev); > > - if (rev < 0x40) { > > - pci_dev_put(amd_smbus_dev); > > - amd_smbus_dev = NULL; > > - return 0; > > - } > > - > > - if (!amd_nb_dev) > > - amd_nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x1510, NULL); > > - if (!amd_nb_dev) > > - ehci_err(ehci, "QUIRK: unable to get AMD NB device\n"); > > - > > - ehci_info(ehci, "QUIRK: Enable AMD SB800 L1 fix\n"); > > - > > - pci_dev_put(amd_smbus_dev); > > - amd_smbus_dev = NULL; > > - > > - return 1; > > -} > > - > > Er, this shouldn't apply to the current mainline anymore as this > function > has been renamed and extended to probe for device ID 0x780B -- see: > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux- > 2.6.git;a=commit;h=baab93afc2844b68d57b0dcca5e1d34c5d7cf411 > > This commit is not yet merged to Greg's 'usb-next' branch, when it > will be, > there's gonna be a reject... > This commit is merged into linux-next. I asked Greg whether it need to be resubmitted but haven't received reply yet. > > diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci- > quirks.c > > index 4c502c8..344b25a 100644 > > --- a/drivers/usb/host/pci-quirks.c > > +++ b/drivers/usb/host/pci-quirks.c > > @@ -52,6 +52,266 @@ > > #define EHCI_USBLEGCTLSTS 4 /* legacy control/status */ > > #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 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; > > + > > +static DEFINE_SPINLOCK(amd_lock); > > + > > +int usb_amd_find_chipset_info(void) > > +{ > > + u8 rev = 0; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&amd_lock, flags); > > + > > + amd_chipset.probe_count++; > > + /* probe only once */ > > + if (amd_chipset.probe_count > 1) { > > + spin_unlock_irqrestore(&amd_lock, flags); > > + return amd_chipset.probe_result; > > + } > > + > > + amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, > NULL); > > + if (amd_chipset.smbus_dev) { > > + pci_read_config_byte(amd_chipset.smbus_dev, > > + PCI_REVISION_ID, &rev); > > There is no need to read the revision ID from the register, it's > stored in > the 'revision' field of 'struct pci_dev', i.e. 'amd_chipset.smbus_dev- > >revision'. > I was going to post the patch replacing the register access but then > realized that you're moving the code from ehci-pci.c and ohci-pci.c here. > This part of code is moved from EHCI/OHCI driver so I didn't change the implementation. If a resubmit is needed I'll modify it. 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
