by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [PATCH v4 27/29] PCI: Make piix4 quirk to use addon_res

On Friday, April 26, 2013 09:22:47 AM Bjorn Helgaas wrote:
> [+cc Rafael, linux-acpi]
>
> On Thu, Apr 25, 2013 at 2:39 PM, Bjorn Helgaas <[email protected]> wrote:
> > On Fri, Apr 12, 2013 at 4:44 PM, Yinghai Lu <[email protected]> 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.

Agreed.

Thanks,
Rafael

> >> Signed-off-by: Yinghai Lu <[email protected]>
> >> ---
> >> 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
> >>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.