[Skiboot] [PATCH 1/2] pci: Add a framework for quirks

[Russell Currey]

On Mon, 2017-02-20 at 10:04 +1100, Gavin Shan wrote:
> On Fri, Feb 17, 2017 at 04:40:18PM +1100, Benjamin Herrenschmidt wrote:
> > From: Russell Currey <ruscur at russell.cc>
> > 
> > In future we may want to be able to do fixups for specific PCI devices in
> > skiboot, so add a small framework for doing this.
> > 
> > Signed-off-by: Russell Currey <ruscur at russell.cc>
> > Signed-off-by: Benjamin Herrenschmidt <benh at kernel.crashing.org>
> > ---
> > core/Makefile.inc   |  2 +-
> > core/pci-quirk.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
> > core/pci.c          |  3 +++
> > include/pci-quirk.h | 36 ++++++++++++++++++++++++++++++++++++
> > 4 files changed, 82 insertions(+), 1 deletion(-)
> > create mode 100644 core/pci-quirk.c
> > create mode 100644 include/pci-quirk.h
> > 
> > diff --git a/core/Makefile.inc b/core/Makefile.inc
> > index 2167044..734fb1b 100644
> > --- a/core/Makefile.inc
> > +++ b/core/Makefile.inc
> > @@ -8,7 +8,7 @@ CORE_OBJS += pci-opal.o fast-reboot.o device.o exceptions.o
> > trace.o affinity.o
> > CORE_OBJS += vpd.o hostservices.o platform.o nvram.o nvram-format.o hmi.o
> > CORE_OBJS += console-log.o ipmi.o time-utils.o pel.o pool.o errorlog.o
> > CORE_OBJS += timer.o i2c.o rtc.o flash.o sensor.o ipmi-opal.o
> > -CORE_OBJS += flash-subpartition.o bitmap.o buddy.o
> > +CORE_OBJS += flash-subpartition.o bitmap.o buddy.o pci-quirk.o
> > 
> > ifeq ($(SKIBOOT_GCOV),1)
> > CORE_OBJS += gcov-profiling.o
> > diff --git a/core/pci-quirk.c b/core/pci-quirk.c
> > new file mode 100644
> > index 0000000..708f07f
> > --- /dev/null
> > +++ b/core/pci-quirk.c
> > @@ -0,0 +1,42 @@
> > +/* Copyright 2016 IBM Corp.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at
> > + *
> > + * 	http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > + * implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include <skiboot.h>
> > +#include <pci.h>
> > +#include <pci-quirk.h>
> > +#include <ast.h>
> > +
> > +/* Quirks are: {fixup function, vendor ID, (device ID or PCI_ANY_ID)} */
> > +static const struct pci_quirk quirk_table[] = {
> > +	{0}
> > +};
> > +
> > +void pci_handle_quirk(struct phb *phb,
> > +		      struct pci_device *pd,
> > +		      struct dt_node *np,
> > +		      uint16_t vendor_id,
> > +		      uint16_t device_id)
> > +{
> > +	const struct pci_quirk *quirks = quirk_table;
> > +
> > +	while (quirks->vendor_id) {
> > +		if (vendor_id == quirks->vendor_id &&
> > +		    (quirks->device_id == PCI_ANY_ID ||
> > +		     device_id == quirks->device_id))
> > +			quirks->fixup(phb, pd, np);
> > +		quirks++;
> > +	}
> > +}
> > diff --git a/core/pci.c b/core/pci.c
> > index 0a9f8cd..7e764a0 100644
> > --- a/core/pci.c
> > +++ b/core/pci.c
> > @@ -19,6 +19,7 @@
> > #include <pci.h>
> > #include <pci-cfg.h>
> > #include <pci-slot.h>
> > +#include <pci-quirk.h>
> > #include <timebase.h>
> > #include <device.h>
> > #include <fsp.h>
> > @@ -1402,6 +1403,8 @@ static void pci_add_one_device_node(struct phb *phb,
> > 	if (intpin)
> > 		dt_add_property_cells(np, "interrupts", intpin);
> > 
> > +	pci_handle_quirk(phb, pd, np, vdid & 0xffff, vdid >> 16);
> > +
> 
> The device node, vendor and device ID have cached to the PCI device
> (struct pci_device). So we needn't pass them to pci_handle_quirk(),
> they can be picked from PCI device instead, if we won't pass device
> node or vdid other than those are associated with the PCI device.

Good catch - the DT node is cached and the vendor and device ID are encoded
together.  I think it's clearer having the separation logic outside the quirks
framework, especially since the matching logic is different between vendor and
device.

> 
> Also, the memory BAR of one specific PCI device isn't populted. It
> means we can't fixup the registers in memory BAR and have to do that
> in kernel. With this patch applied, we're going to have some fixups
> in skiboot and others in kernel. It's prone to be out of sychronization
> between skiboot and kernel. So it's possible to do the quirks in kernel
> only?

As Ben explained this wasn't for touching registers of the device, I will
mention that in the commit message of v2.

> 
> > 	/* XXX FIXME: Add a few missing ones such as
> > 	 *
> > 	 *  - devsel-speed (!express)
> > diff --git a/include/pci-quirk.h b/include/pci-quirk.h
> > new file mode 100644
> > index 0000000..c2ceb55
> > --- /dev/null
> > +++ b/include/pci-quirk.h
> > @@ -0,0 +1,36 @@
> > +/* Copyright 2016 IBM Corp.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at
> > + *
> > + * 	http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > + * implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef __PCI_QUIRK_H
> > +#define __PCI_QUIRK_H
> > +
> > +#include <pci.h>
> > +
> > +#define PCI_ANY_ID 0xFFFF
> > +
> > +struct pci_quirk {
> > +	void (*fixup)(struct phb *, struct pci_device *, struct dt_node *);
> 
> Similar to above, we needn't pass device node which can be picked from
> the PCI device instance.

Yes, thanks for the review.

> 
> > +	uint16_t vendor_id;
> > +	uint16_t device_id;
> > +};
> > +
> > +void pci_handle_quirk(struct phb *phb,
> > +		      struct pci_device *pd,
> > +		      struct dt_node *np,
> > +		      uint16_t vendor_id,
> > +		      uint16_t device_id);
> > +
> > +#endif /* __PCI_QUIRK_H */
> 
> Thanks,
> Gavin
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot