On Thu, 5 May 2011 13:51:58 -0600
Matthew Wilcox <matthew@xxxxxx> wrote:
> On Thu, May 05, 2011 at 12:33:50PM -0700, Jesse Barnes wrote:
> > +bool pci_ltr_supported(struct pci_dev *dev)
> > +{
> > + int pos;
> > + u32 cap;
> > +
> > + if (!pci_is_pcie(dev))
> > + return false;
> > +
> > + pos = pci_pcie_cap(dev);
> > + if (!pos)
> > + return false;
> > +
> > + pci_read_config_dword(dev, pos + PCI_EXP_DEVCAP2, &cap);
> > +
> > + return cap & PCI_EXP_DEVCAP2_LTR;
> > +}
>
> Missing EXPORT_SYMBOL? (throughout)
Oops, will fix. I guess drivers would have run into that pretty
quickly. :)
>
> > +/**
> > + * pci_set_ltr - set LTR latency values
> > + * @dev: PCI device
> > + * @latencies: LTR latency values & scaling
> > + *
> > + * Set the LTR cap registers to the values provided by @latencies.
> > + */
> > +int pci_set_ltr(struct pci_dev *dev, struct pci_ltr_latencies *latencies)
> > +{
>
> I don't like this API. Why not just pass a u32 snoop_lat and nosnoop_lat
> (in ns) and do:
>
> int scale = 0;
> while (latency > 1023) {
> latency = (latency + 31) / 32;
> scale++;
> }
>
> (a u32 latency gives you up to 4 seconds of latency, which is surely
> more than any device can tolerate ... and doesn't let you max out scale)
Agreed, not sure why I thought this was a good API when I coded it up.
Taking simple ns is much easier from a caller perspective.
> > + int pos, ret;
> > + u32 val;
> > +
> > + if (!pci_ltr_supported(dev))
> > + return -ENOTSUPP;
> > +
> > + if (latencies->max_snoop_value > PCI_LTR_VALUE_MASK ||
> > + latencies->max_nosnoop_value > PCI_LTR_VALUE_MASK)
> > + return -EINVAL;
> > +
> > + if ((latencies->max_snoop_scale >
> > + (PCI_LTR_SCALE_MASK >> PCI_LTR_SCALE_SHIFT)) ||
> > + (latencies->max_nosnoop_scale >
> > + (PCI_LTR_SCALE_MASK >> PCI_LTR_SCALE_SHIFT)))
> > + return -EINVAL;
> > +
> > + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
> > + if (!pos)
> > + return -ENOTSUPP;
> > +
> > + val = (latencies->max_snoop_scale << PCI_LTR_SCALE_SHIFT) |
> > + latencies->max_snoop_value;
> > + ret = pci_write_config_dword(dev, pos + PCI_LTR_MAX_SNOOP_LAT, val);
> > + if (ret != 4)
> > + return -EIO;
>
> Spec indicates these are word values ... can probably hit both registers
> with a single dword write though.
May as well split the writes up just to avoid confusion.
Thanks a lot for checking it out.
--
Jesse Barnes, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
