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

["Rafael J. Wysocki" <rjw@sisk.pl>]

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 <bhelgaas@google.com> wrote:
> > On Fri, Apr 12, 2013 at 4:44 PM, Yinghai Lu <yinghai@kernel.org> 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 <yinghai@kernel.org>
> >> ---
> >>  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.