[+cc Rafael, linux-acpi]
On Thu, Apr 25, 2013 at 2:39 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
> On Fri, Apr 12, 2013 at 4:44 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
>> After they are put in add-on resources, they will be safely claimed later.
>
> It took me a while to understand what's going on here, but this is
> actually a pretty cool idea. It would be nice to explain a little
> about what the patch is doing and how these "pseudo-BARs" work, though
> :)
After thinking about this overnight, I'm not so sure about this. I
think the point of this is to manage resources that are consumed by
the device but don't conform to the PCI spec. Most resources don't
need quirks because they're either described by standard BAR registers
or by the exceptions for "legacy I/O devices," e.g., IDE or VGA.
I would expect that the "architected" way to deal with non-standard
resources like this is to describe them via a PNP0C02 or similar ACPI
device. That way, the OS would not need device-specific quirks like
this.
If we do add something like this patch, we're also adding a way for
Linux to *write* these non-standard BARs, which means they can become
out-of-sync with the ACPI description of them. I think the BIOS is
entitled to assume that if it describes a resource via _CRS and it
doesn't provide a corresponding _SRS, the resource will remain
unchanged by the OS.
Bjorn
>> Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
>> ---
>> drivers/pci/quirks.c | 95 +++++++++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 79 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 2dac170..6706182 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -382,14 +382,14 @@ static void quirk_ali7101_acpi(struct pci_dev *dev)
>> }
>> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101, quirk_ali7101_acpi);
>>
>> -static void piix4_io_quirk(struct pci_dev *dev, const char *name, unsigned int port, unsigned int enable)
>> +static int piix4_read_io(struct pci_dev *dev, struct resource *res, int port)
>
> We're reading from "port," which is really the address of a dword in
> config space, not a port, so I would have called it "pos" like
> __pci_read_base() did. If I understand correctly, we're reading a
> BAR-like register that contains the base address of an I/O port
> region.
>
> "piix4_read_io_base(struct pci_dev *dev, struct resource *res, int
> pos)" would make more sense to me.
>
>> {
>> u32 devres;
>> u32 mask, size, base;
>> + struct pci_bus_region bus_region;
>>
>> pci_read_config_dword(dev, port, &devres);
>> - if ((devres & enable) != enable)
>> - return;
>> +
>> mask = (devres >> 16) & 15;
>> base = devres & 0xffff;
>> size = 16;
>> @@ -399,23 +399,54 @@ static void piix4_io_quirk(struct pci_dev *dev, const char *name, unsigned int p
>> break;
>> size = bit;
>> }
>> - /*
>> - * For now we only print it out. Eventually we'll want to
>> - * reserve it (at least if it's in the 0x1000+ range), but
>> - * let's get enough confirmation reports first.
>> - */
>> base &= -size;
>> - dev_info(&dev->dev, "%s PIO at %04x-%04x\n", name, base, base + size - 1);
>> +
>> + bus_region.start = base;
>> + bus_region.end = base + size - 1;
>> + res->flags |= IORESOURCE_IO;
>> + pcibios_bus_to_resource(dev, res, &bus_region);
>> + dev_info(&dev->dev, "PIO at %pR\n", res);
>> +
>> + return 0;
>> }
>> +static int piix4_write_io(struct pci_dev *dev, struct resource *res, int port)
>> +{
>> + u32 devres;
>> + struct pci_bus_region bus_region;
>> + unsigned size = to_pci_dev_addon_resource(res)->size;
>>
>> -static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int port, unsigned int enable)
>> + pcibios_resource_to_bus(dev, &bus_region, res);
>> +
>> + pci_read_config_dword(dev, port, &devres);
>> + devres &= 0xffff0000 | (size - 1);
>> + devres |= bus_region.start & 0xffff & (~(size - 1));
>> + pci_write_config_dword(dev, port, devres);
>> +
>> + return 0;
>> +}
>> +static struct resource_ops piix4_io_ops = {
>> + .read = piix4_read_io,
>> + .write = piix4_write_io,
>> +};
>> +static void piix4_io_quirk(struct pci_dev *dev, char *name, unsigned int port,
>> + unsigned int enable)
>
> I think these (piix4_io_quirk() and piix4_mem_quirk()) would read
> better if they were something like:
>
> piix4_add_io_bar(struct pci_dev *dev, unsigned int pos, u32
> enable, char *name);
>
>> {
>> u32 devres;
>> - u32 mask, size, base;
>>
>> pci_read_config_dword(dev, port, &devres);
>> if ((devres & enable) != enable)
>> return;
>> +
>> + pci_add_dev_addon_resource(dev, port, 0, &piix4_io_ops, name);
>> +}
>> +
>> +static int piix4_read_mem(struct pci_dev *dev, struct resource *res, int port)
>> +{
>> + u32 devres;
>> + u32 mask, size, base;
>> + struct pci_bus_region bus_region;
>> +
>> + pci_read_config_dword(dev, port, &devres);
>> base = devres & 0xffff0000;
>> mask = (devres & 0x3f) << 16;
>> size = 128 << 16;
>> @@ -425,12 +456,44 @@ static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int
>> break;
>> size = bit;
>> }
>> - /*
>> - * For now we only print it out. Eventually we'll want to
>> - * reserve it, but let's get enough confirmation reports first.
>> - */
>> base &= -size;
>> - dev_info(&dev->dev, "%s MMIO at %04x-%04x\n", name, base, base + size - 1);
>> + bus_region.start = base;
>> + bus_region.end = base + size - 1;
>> + res->flags |= IORESOURCE_MEM;
>> + pcibios_bus_to_resource(dev, res, &bus_region);
>> + dev_info(&dev->dev, "MMIO at %pR\n", res);
>> +
>> + return 0;
>> +}
>> +static int piix4_write_mem(struct pci_dev *dev, struct resource *res, int port)
>> +{
>> + u32 devres;
>> + struct pci_bus_region bus_region;
>> + unsigned size = to_pci_dev_addon_resource(res)->size;
>> +
>> + pcibios_resource_to_bus(dev, &bus_region, res);
>> +
>> + pci_read_config_dword(dev, port, &devres);
>> + devres &= 0x0000ffff | ((size - 1) & 0xffff0000);
>> + devres |= bus_region.start & 0xffff0000 & (~(size - 1));
>> + pci_write_config_dword(dev, port, devres);
>> +
>> + return 0;
>> +}
>> +static struct resource_ops piix4_mem_ops = {
>> + .read = piix4_read_mem,
>> + .write = piix4_write_mem,
>> +};
>> +static void piix4_mem_quirk(struct pci_dev *dev, char *name, unsigned int port,
>> + unsigned int enable)
>> +{
>> + u32 devres;
>> +
>> + pci_read_config_dword(dev, port, &devres);
>> + if ((devres & enable) != enable)
>> + return;
>> +
>> + pci_add_dev_addon_resource(dev, port, 0, &piix4_mem_ops, name);
>> }
>>
>> /*
>> --
>> 1.8.1.4
>>
--
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
