On Wed, Jan 12, 2011 at 04:25:45PM +0800, Xu, Andiry wrote:
> > > +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;
...
> > > +
> > > + 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.
Then please do not call it ret. Make it have a descriptive name,
because when I see ret I think "this is a temporary variable used to
store the return value", not "this is a global state variable".
I think using static variables in this way is discouraged in the kernel.
Is there no structure you can stick this information in that can be
shared across all host controllers? Something in the USB core perhaps?
Or why not combine amd_nb_type, amd_sb_type, probe_count, amd_nb_dev,
amd_smbus_dev, and this new variable into one structure called
"amd_chipset" or something?
>
> > > +}
> > > +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.
Ok, that makes sense.
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
