On Thu, May 05, 2011 at 12:33:49PM -0700, Jesse Barnes wrote:
> OBFF (optimized buffer flush/fill), where supported, can help improve
> energy efficiency by giving devices information about when interrupts
> and other activity will have a reduced power impact. It requires
> support from both the device and system (i.e. not only does the device
> need to respond to OBFF messages, but the platform must be capable of
> generating and routing them to the end point).
I'm really confused by the OBFF spec. There's all kinds of caveats on
enabling OBFF; for example, it seems that the WAKE# signal might be shared
between all devices below a given switch ... but how are we to know?
Figure 6-19 in the 10Nov10 spec shows a case where _all_ the devices
connected to Switch B would need to support OBFF in order to enable it.
And not necessarily directly connected either; the device below Switch
A shares WAKE# with the devices directly below the root port.
I think we should probably leave it up to the BIOS to configure OBFF.
I don't think we can do it.
That said, I did notice a bug ...
> + /* Make sure the topology supports OBFF as well */
> + if (dev->bus) {
> + ret = pci_enable_obff(dev->bus->self, type);
> + if (ret)
> + return ret;
> + }
> +
> + pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
> + if (cap & PCI_EXP_OBFF_WAKE)
> + ctrl |= PCI_EXP_OBFF_WAKE_EN;
> + else {
> + switch (type) {
> + case PCI_EXP_OBFF_SIGNAL_L0:
> + ctrl |= PCI_EXP_OBFF_MSGA_EN;
> + break;
> + case PCI_EXP_OBFF_SIGNAL_ALWAYS:
> + ctrl |= PCI_EXP_OBFF_MSGB_EN;
> + break;
> + default:
> + WARN(1, "bad OBFF signal type\n");
> + return -ENOTSUPP;
> + }
> + }
If Device 1 asks for L0 and Device 2 asks for Always, any component above
them in the hierarchy gets WAKE set, which isn't going to work out very well.
It should look like this:
switch (type) {
case PCI_EXP_OBFF_SIGNAL_L0:
if (!(ctrl & PCI_EXP_OBFF_WAKE_EN))
ctrl |= PCI_EXP_OBFF_MSGA_EN;
break;
case PCI_EXP_OBFF_SIGNAL_ALWAYS:
ctrl &= ~PCI_EXP_OBFF_WAKE_EN;
ctrl |= PCI_EXP_OBFF_MSGB_EN;
break;
> + pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
> + ctrl &= ~(PCI_EXP_OBFF_MSGA_EN | PCI_EXP_OBFF_MSGA_EN |
> + PCI_EXP_OBFF_WAKE_EN);
That simplifies to ctrl &= ~PCI_EXP_OBFF_WAKE_EN;
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
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
