[RFC PATCH 0/3] ECAM quirks handling for ARM64 platforms

[RFC PATCH 0/3] ECAM quirks handling for ARM64 platforms

[Tomasz Nowicki]

This series bases on pending ACPI PCI support for ARM64:
https://lkml.org/lkml/2016/5/30/468

Quirk handling relies on an idea of matching MCFG OEM ID and OEM revision
(the ones from standard header of MCFG table). Linker section is used
so that quirks can be registered using special macro (see patches) and
kept self contained.

As an example, last patch presents above mechanism usage for ThunderX PEM driver.

Tomasz Nowicki (3):
  pci, acpi: Match PCI config space accessors against platfrom specific
    ECAM quirks.
  arm64, pci: Start using quirks handling for ACPI based PCI host
    controller.
  pci, pci-thunder-pem: Add ACPI support for ThunderX PEM.

 arch/arm64/kernel/pci.c            |   7 +-
 drivers/acpi/pci_mcfg.c            |  32 +++++++++
 drivers/pci/host/pci-thunder-pem.c | 132 +++++++++++++++++++++++++++++++++----
 include/asm-generic/vmlinux.lds.h  |   7 ++
 include/linux/pci-acpi.h           |  19 ++++++
 5 files changed, 181 insertions(+), 16 deletions(-)

-- 
1.9.1

[RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

[Tomasz Nowicki]

Some platforms may not be fully compliant with generic set of PCI config
accessors. For these cases we implement the way to overwrite accessors
set. Algorithm traverses available quirk list, matches against
<oem_id, oem_rev, domain, bus number> tuple and returns corresponding
PCI config ops. oem_id and oem_rev come from MCFG table standard header.
All quirks can be defined using DECLARE_ACPI_MCFG_FIXUP() macro and
kept self contained. Example:

/* Custom PCI config ops */
static struct pci_generic_ecam_ops foo_pci_ops = {
	.bus_shift	= 24,
	.pci_ops = {
		.map_bus = pci_ecam_map_bus,
		.read = foo_ecam_config_read,
		.write = foo_ecam_config_write,
	}
};

DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, <oem_id_str>, <oem_rev>, <domain_nr>, <bus_nr>);

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 drivers/acpi/pci_mcfg.c           | 32 ++++++++++++++++++++++++++++++++
 include/asm-generic/vmlinux.lds.h |  7 +++++++
 include/linux/pci-acpi.h          | 19 +++++++++++++++++++
 3 files changed, 58 insertions(+)

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index 1847f74..f3d4570 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -22,11 +22,43 @@
 #include <linux/kernel.h>
 #include <linux/pci.h>
 #include <linux/pci-acpi.h>
+#include <linux/pci-ecam.h>
 
 /* Root pointer to the mapped MCFG table */
 static struct acpi_table_mcfg *mcfg_table;
 static int mcfg_entries;
 
+extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
+extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
+
+struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
+{
+	int bus_num = root->secondary.start;
+	int domain = root->segment;
+	struct pci_cfg_fixup *f;
+
+	if (!mcfg_table)
+		return &pci_generic_ecam_ops;
+
+	/*
+	 * Match against platform specific quirks and return corresponding
+	 * CAM ops.
+	 *
+	 * First match against PCI topology <domain:bus> then use OEM ID and
+	 * OEM revision from MCFG table standard header.
+	 */
+	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
+		if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
+		    (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
+		    (!strncmp(f->oem_id, mcfg_table->header.oem_id,
+			      ACPI_OEM_ID_SIZE)) &&
+		    (f->oem_revision == mcfg_table->header.oem_revision))
+			return f->ops;
+	}
+	/* No quirks, use ECAM */
+	return &pci_generic_ecam_ops;
+}
+
 int pci_mcfg_lookup(struct acpi_pci_root *root)
 {
 	struct acpi_mcfg_allocation *mptr, *entry = NULL;
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 6a67ab9..43604fc 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -300,6 +300,13 @@
 		VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;	\
 	}								\
 									\
+	/* ACPI MCFG quirks */						\
+	.acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {	\
+		VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;		\
+		*(.acpi_fixup_mcfg)					\
+		VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;		\
+	}								\
+									\
 	/* Built-in firmware blobs */					\
 	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
 		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index e0e6dfc..b4e8106 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
 extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
 
 extern int pci_mcfg_lookup(struct acpi_pci_root *root);
+extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
 
 static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
 {
@@ -72,6 +73,24 @@ struct acpi_pci_root_ops {
 	int (*prepare_resources)(struct acpi_pci_root_info *info);
 };
 
+struct pci_cfg_fixup {
+	struct pci_ecam_ops *ops;
+	char *oem_id;
+	u32 oem_revision;
+	int domain;
+	int bus_num;
+};
+
+#define PCI_MCFG_DOMAIN_ANY	-1
+#define PCI_MCFG_BUS_ANY	-1
+
+/* Designate a routine to fix up buggy MCFG */
+#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, rev, dom, bus)		\
+	static const struct pci_cfg_fixup __mcfg_fixup_##system##dom##bus\
+	 __used	__attribute__((__section__(".acpi_fixup_mcfg"),		\
+				aligned((sizeof(void *))))) =		\
+	{ ops, oem_id, rev, dom, bus };
+
 extern int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info);
 extern struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 					    struct acpi_pci_root_ops *ops,
-- 
1.9.1

[RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

[Arnd Bergmann]

On Thursday, June 2, 2016 10:41:01 AM CEST Tomasz Nowicki wrote:
> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> +{
> +       int bus_num = root->secondary.start;
> +       int domain = root->segment;
> +       struct pci_cfg_fixup *f;
> +
> +       if (!mcfg_table)
> +               return &pci_generic_ecam_ops;
> +
> +       /*
> +        * Match against platform specific quirks and return corresponding
> +        * CAM ops.
> +        *
> +        * First match against PCI topology <domain:bus> then use OEM ID and
> +        * OEM revision from MCFG table standard header.
> +        */
> +       for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
> +               if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
> +                   (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
> +                   (!strncmp(f->oem_id, mcfg_table->header.oem_id,
> +                             ACPI_OEM_ID_SIZE)) &&
> +                   (f->oem_revision == mcfg_table->header.oem_revision))
> +                       return f->ops;
> +       }
> +       /* No quirks, use ECAM */
> +       return &pci_generic_ecam_ops;
> +}
> +
>  int pci_mcfg_lookup(struct acpi_pci_root *root)

Can you explain the use of pci_ecam_ops instead of pci_ops here?

I was hoping we'd be able to fold all of pci-ecam code into the
generic PCI layer at some point and get rid of pci_ecam_ops in the
process.

	Arnd

[RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

[Tomasz Nowicki]

On 02.06.2016 13:42, Arnd Bergmann wrote:
> On Thursday, June 2, 2016 10:41:01 AM CEST Tomasz Nowicki wrote:
>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
>> +{
>> +       int bus_num = root->secondary.start;
>> +       int domain = root->segment;
>> +       struct pci_cfg_fixup *f;
>> +
>> +       if (!mcfg_table)
>> +               return &pci_generic_ecam_ops;
>> +
>> +       /*
>> +        * Match against platform specific quirks and return corresponding
>> +        * CAM ops.
>> +        *
>> +        * First match against PCI topology <domain:bus> then use OEM ID and
>> +        * OEM revision from MCFG table standard header.
>> +        */
>> +       for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
>> +               if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
>> +                   (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
>> +                   (!strncmp(f->oem_id, mcfg_table->header.oem_id,
>> +                             ACPI_OEM_ID_SIZE)) &&
>> +                   (f->oem_revision == mcfg_table->header.oem_revision))
>> +                       return f->ops;
>> +       }
>> +       /* No quirks, use ECAM */
>> +       return &pci_generic_ecam_ops;
>> +}
>> +
>>   int pci_mcfg_lookup(struct acpi_pci_root *root)
>
> Can you explain the use of pci_ecam_ops instead of pci_ops here?
>

I wanted to get associated bus_shift and use it to setup configuration 
region properly before calling pci_ecam_create. Please see next patch.

Thanks,
Tomasz

[RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

[Arnd Bergmann]

On Thursday, June 2, 2016 2:07:43 PM CEST Tomasz Nowicki wrote:
> On 02.06.2016 13:42, Arnd Bergmann wrote:
> > On Thursday, June 2, 2016 10:41:01 AM CEST Tomasz Nowicki wrote:
> >> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> >> +{
> >> +       int bus_num = root->secondary.start;
> >> +       int domain = root->segment;
> >> +       struct pci_cfg_fixup *f;
> >> +
> >> +       if (!mcfg_table)
> >> +               return &pci_generic_ecam_ops;
> >> +
> >> +       /*
> >> +        * Match against platform specific quirks and return corresponding
> >> +        * CAM ops.
> >> +        *
> >> +        * First match against PCI topology <domain:bus> then use OEM ID and
> >> +        * OEM revision from MCFG table standard header.
> >> +        */
> >> +       for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
> >> +               if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
> >> +                   (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
> >> +                   (!strncmp(f->oem_id, mcfg_table->header.oem_id,
> >> +                             ACPI_OEM_ID_SIZE)) &&
> >> +                   (f->oem_revision == mcfg_table->header.oem_revision))
> >> +                       return f->ops;
> >> +       }
> >> +       /* No quirks, use ECAM */
> >> +       return &pci_generic_ecam_ops;
> >> +}
> >> +
> >>   int pci_mcfg_lookup(struct acpi_pci_root *root)
> >
> > Can you explain the use of pci_ecam_ops instead of pci_ops here?
> >
> 
> I wanted to get associated bus_shift and use it to setup configuration 
> region properly before calling pci_ecam_create. Please see next patch.
> 

I see. It feels really odd to do it this way though, since having a
nonstandard bus_shift essentially means not using anything resembling
ECAM to start with.

I realize that a lot of the host bridges are not ECAM, but because
of this, it would be more logical to have their own pci_ops instead
of pci_ecam_ops.

	Arnd

[RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

[Tomasz Nowicki]

On 02.06.2016 14:32, Arnd Bergmann wrote:
> On Thursday, June 2, 2016 2:07:43 PM CEST Tomasz Nowicki wrote:
>> On 02.06.2016 13:42, Arnd Bergmann wrote:
>>> On Thursday, June 2, 2016 10:41:01 AM CEST Tomasz Nowicki wrote:
>>>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
>>>> +{
>>>> +       int bus_num = root->secondary.start;
>>>> +       int domain = root->segment;
>>>> +       struct pci_cfg_fixup *f;
>>>> +
>>>> +       if (!mcfg_table)
>>>> +               return &pci_generic_ecam_ops;
>>>> +
>>>> +       /*
>>>> +        * Match against platform specific quirks and return corresponding
>>>> +        * CAM ops.
>>>> +        *
>>>> +        * First match against PCI topology <domain:bus> then use OEM ID and
>>>> +        * OEM revision from MCFG table standard header.
>>>> +        */
>>>> +       for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
>>>> +               if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
>>>> +                   (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
>>>> +                   (!strncmp(f->oem_id, mcfg_table->header.oem_id,
>>>> +                             ACPI_OEM_ID_SIZE)) &&
>>>> +                   (f->oem_revision == mcfg_table->header.oem_revision))
>>>> +                       return f->ops;
>>>> +       }
>>>> +       /* No quirks, use ECAM */
>>>> +       return &pci_generic_ecam_ops;
>>>> +}
>>>> +
>>>>    int pci_mcfg_lookup(struct acpi_pci_root *root)
>>>
>>> Can you explain the use of pci_ecam_ops instead of pci_ops here?
>>>
>>
>> I wanted to get associated bus_shift and use it to setup configuration
>> region properly before calling pci_ecam_create. Please see next patch.
>>
>
> I see. It feels really odd to do it this way though, since having a
> nonstandard bus_shift essentially means not using anything resembling
> ECAM to start with.
>
> I realize that a lot of the host bridges are not ECAM, but because
> of this, it would be more logical to have their own pci_ops instead
> of pci_ecam_ops.

Well, we have bus_shift there to express bus shift differentiation. So I 
would say we should change just structure name to prevent misunderstanding.

Thanks,
Tomasz

[RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

[Arnd Bergmann]

On Thursday, June 2, 2016 3:35:34 PM CEST Tomasz Nowicki wrote:
> On 02.06.2016 14:32, Arnd Bergmann wrote:
> > On Thursday, June 2, 2016 2:07:43 PM CEST Tomasz Nowicki wrote:
> >> On 02.06.2016 13:42, Arnd Bergmann wrote:
> >>> On Thursday, June 2, 2016 10:41:01 AM CEST Tomasz Nowicki wrote:
> >>>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> >>>> +{
> >>>> +       int bus_num = root->secondary.start;
> >>>> +       int domain = root->segment;
> >>>> +       struct pci_cfg_fixup *f;
> >>>> +
> >>>> +       if (!mcfg_table)
> >>>> +               return &pci_generic_ecam_ops;
> >>>> +
> >>>> +       /*
> >>>> +        * Match against platform specific quirks and return corresponding
> >>>> +        * CAM ops.
> >>>> +        *
> >>>> +        * First match against PCI topology <domain:bus> then use OEM ID and
> >>>> +        * OEM revision from MCFG table standard header.
> >>>> +        */
> >>>> +       for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
> >>>> +               if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
> >>>> +                   (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
> >>>> +                   (!strncmp(f->oem_id, mcfg_table->header.oem_id,
> >>>> +                             ACPI_OEM_ID_SIZE)) &&
> >>>> +                   (f->oem_revision == mcfg_table->header.oem_revision))
> >>>> +                       return f->ops;
> >>>> +       }
> >>>> +       /* No quirks, use ECAM */
> >>>> +       return &pci_generic_ecam_ops;
> >>>> +}
> >>>> +
> >>>>    int pci_mcfg_lookup(struct acpi_pci_root *root)
> >>>
> >>> Can you explain the use of pci_ecam_ops instead of pci_ops here?
> >>>
> >>
> >> I wanted to get associated bus_shift and use it to setup configuration
> >> region properly before calling pci_ecam_create. Please see next patch.
> >>
> >
> > I see. It feels really odd to do it this way though, since having a
> > nonstandard bus_shift essentially means not using anything resembling
> > ECAM to start with.
> >
> > I realize that a lot of the host bridges are not ECAM, but because
> > of this, it would be more logical to have their own pci_ops instead
> > of pci_ecam_ops.
> 
> Well, we have bus_shift there to express bus shift differentiation. So I 
> would say we should change just structure name to prevent misunderstanding.

I'm not really convinced here. We use the bus_shift for two
completely different things in the end: for sizing the MMIO window
that gets mapped by ACPI and for the pci_ecam_map_bus() function
that isn't actually used for the typical fixups that override the
pci_ops.

I see now that this sneaks in an .init callback for the quirk
through the backdoor, by adding it to the pci_ecam_ops. I think
that is not good: if the idea is to have the config space access
be adapted to various quirks that is one thing, but if we actually
need a function to be called for the quirk we should do just that
and have it be obvious. That function can then override the
pci_ops.

	Arnd

[RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

[Tomasz Nowicki]

Hi Arnd,

Sorry for late response. Please see comments inline.

On 02.06.2016 17:19, Arnd Bergmann wrote:
> On Thursday, June 2, 2016 3:35:34 PM CEST Tomasz Nowicki wrote:
>> On 02.06.2016 14:32, Arnd Bergmann wrote:
>>> On Thursday, June 2, 2016 2:07:43 PM CEST Tomasz Nowicki wrote:
>>>> On 02.06.2016 13:42, Arnd Bergmann wrote:
>>>>> On Thursday, June 2, 2016 10:41:01 AM CEST Tomasz Nowicki wrote:
>>>>>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
>>>>>> +{
>>>>>> +       int bus_num = root->secondary.start;
>>>>>> +       int domain = root->segment;
>>>>>> +       struct pci_cfg_fixup *f;
>>>>>> +
>>>>>> +       if (!mcfg_table)
>>>>>> +               return &pci_generic_ecam_ops;
>>>>>> +
>>>>>> +       /*
>>>>>> +        * Match against platform specific quirks and return corresponding
>>>>>> +        * CAM ops.
>>>>>> +        *
>>>>>> +        * First match against PCI topology <domain:bus> then use OEM ID and
>>>>>> +        * OEM revision from MCFG table standard header.
>>>>>> +        */
>>>>>> +       for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
>>>>>> +               if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
>>>>>> +                   (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
>>>>>> +                   (!strncmp(f->oem_id, mcfg_table->header.oem_id,
>>>>>> +                             ACPI_OEM_ID_SIZE)) &&
>>>>>> +                   (f->oem_revision == mcfg_table->header.oem_revision))
>>>>>> +                       return f->ops;
>>>>>> +       }
>>>>>> +       /* No quirks, use ECAM */
>>>>>> +       return &pci_generic_ecam_ops;
>>>>>> +}
>>>>>> +
>>>>>>     int pci_mcfg_lookup(struct acpi_pci_root *root)
>>>>>
>>>>> Can you explain the use of pci_ecam_ops instead of pci_ops here?
>>>>>
>>>>
>>>> I wanted to get associated bus_shift and use it to setup configuration
>>>> region properly before calling pci_ecam_create. Please see next patch.
>>>>
>>>
>>> I see. It feels really odd to do it this way though, since having a
>>> nonstandard bus_shift essentially means not using anything resembling
>>> ECAM to start with.
>>>
>>> I realize that a lot of the host bridges are not ECAM, but because
>>> of this, it would be more logical to have their own pci_ops instead
>>> of pci_ecam_ops.
>>
>> Well, we have bus_shift there to express bus shift differentiation. So I
>> would say we should change just structure name to prevent misunderstanding.
>
> I'm not really convinced here. We use the bus_shift for two
> completely different things in the end: for sizing the MMIO window
> that gets mapped by ACPI and for the pci_ecam_map_bus() function
> that isn't actually used for the typical fixups that override the
> pci_ops.

Since we overwrite the whole pci_ecam_ops structure (next patch):
-	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,
-			      &pci_generic_ecam_ops);
+	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops);

IMO bus_shift is used in the right way. So if anybody decides to put 
different bus_shift there he also needs to implement map_bus and use 
there bus_shift appropriate to quirk requirements. Obviously we can use 
standard pci_ecam_map_bus() as map_bus but that would mean quirk nature 
needs that, like for ThunderX one.

>
> I see now that this sneaks in an .init callback for the quirk
> through the backdoor, by adding it to the pci_ecam_ops. I think
> that is not good: if the idea is to have the config space access
> be adapted to various quirks that is one thing, but if we actually
> need a function to be called for the quirk we should do just that
> and have it be obvious. That function can then override the
> pci_ops.

Actually we do not need to call a function for each quirk. At the same 
time we already have .init callback adopted to configuration space 
access quirk. This way there is really small amount of code duplication. 
On the other hand I understand that .init call should be more explicit. 
Any suggestions are very appreciated.

Thanks,
Tomasz

[RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

[Christopher Covington]

Hi Tomasz,

Thanks for your work on this.

On 06/02/2016 04:41 AM, Tomasz Nowicki wrote:
> Some platforms may not be fully compliant with generic set of PCI config
> accessors. For these cases we implement the way to overwrite accessors
> set. Algorithm traverses available quirk list, matches against
> <oem_id, oem_rev, domain, bus number> tuple and returns corresponding
> PCI config ops. oem_id and oem_rev come from MCFG table standard header.
> All quirks can be defined using DECLARE_ACPI_MCFG_FIXUP() macro and
> kept self contained. Example:
> 
> /* Custom PCI config ops */
> static struct pci_generic_ecam_ops foo_pci_ops = {
> 	.bus_shift	= 24,
> 	.pci_ops = {
> 		.map_bus = pci_ecam_map_bus,
> 		.read = foo_ecam_config_read,
> 		.write = foo_ecam_config_write,
> 	}
> };
> 
> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, <oem_id_str>, <oem_rev>, <domain_nr>, <bus_nr>);
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>  drivers/acpi/pci_mcfg.c           | 32 ++++++++++++++++++++++++++++++++
>  include/asm-generic/vmlinux.lds.h |  7 +++++++
>  include/linux/pci-acpi.h          | 19 +++++++++++++++++++
>  3 files changed, 58 insertions(+)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index 1847f74..f3d4570 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -22,11 +22,43 @@
>  #include <linux/kernel.h>
>  #include <linux/pci.h>
>  #include <linux/pci-acpi.h>
> +#include <linux/pci-ecam.h>
>  
>  /* Root pointer to the mapped MCFG table */
>  static struct acpi_table_mcfg *mcfg_table;
>  static int mcfg_entries;
>  
> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
> +
> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> +{
> +	int bus_num = root->secondary.start;
> +	int domain = root->segment;
> +	struct pci_cfg_fixup *f;
> +
> +	if (!mcfg_table)
> +		return &pci_generic_ecam_ops;
> +
> +	/*
> +	 * Match against platform specific quirks and return corresponding
> +	 * CAM ops.
> +	 *
> +	 * First match against PCI topology <domain:bus> then use OEM ID and
> +	 * OEM revision from MCFG table standard header.
> +	 */
> +	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
> +		if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
> +		    (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
> +		    (!strncmp(f->oem_id, mcfg_table->header.oem_id,
> +			      ACPI_OEM_ID_SIZE)) &&
> +		    (f->oem_revision == mcfg_table->header.oem_revision))

Is this more likely to be updated between quirky and fixed platforms
than oem_table_id? What do folks think about using oem_table_id instead
of, or in addition to, oem_revision?

In case these details are helpful, here was my simple prototype of an
MCFG based approach:

https://codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-rc1-testing&id=c5d8bc49a198fd8f61f82c7d8f169564d6176b07
https://codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-rc1-testing&id=50bfe77ccd1639e6ce8c7c4fcca187d50e0bead4

Thanks,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

[Gabriele Paoloni]

Hi Cov

> -----Original Message-----
> From: linux-pci-owner at vger.kernel.org [mailto:linux-pci-
> owner at vger.kernel.org] On Behalf Of Christopher Covington
> Sent: 03 June 2016 16:15
> To: Tomasz Nowicki; helgaas at kernel.org; arnd at arndb.de;
> will.deacon at arm.com; catalin.marinas at arm.com; rafael at kernel.org;
> hanjun.guo at linaro.org; Lorenzo.Pieralisi at arm.com; okaya at codeaurora.org;
> jchandra at broadcom.com
> Cc: jcm at redhat.com; linaro-acpi at lists.linaro.org; linux-
> pci at vger.kernel.org; dhdang at apm.com; Liviu.Dudau at arm.com;
> ddaney at caviumnetworks.com; jeremy.linton at arm.com; linux-
> kernel at vger.kernel.org; linux-acpi at vger.kernel.org;
> robert.richter at caviumnetworks.com; Suravee.Suthikulpanit at amd.com;
> msalter at redhat.com; Wangyijing; mw at semihalf.com;
> andrea.gallo at linaro.org; linux-arm-kernel at lists.infradead.org;
> liudongdong (C); Gabriele Paoloni
> Subject: Re: [RFC PATCH 1/3] pci, acpi: Match PCI config space
> accessors against platfrom specific ECAM quirks.
> 
> Hi Tomasz,
> 
> Thanks for your work on this.
> 
> On 06/02/2016 04:41 AM, Tomasz Nowicki wrote:
> > Some platforms may not be fully compliant with generic set of PCI
> config
> > accessors. For these cases we implement the way to overwrite
> accessors
> > set. Algorithm traverses available quirk list, matches against
> > <oem_id, oem_rev, domain, bus number> tuple and returns corresponding
> > PCI config ops. oem_id and oem_rev come from MCFG table standard
> header.
> > All quirks can be defined using DECLARE_ACPI_MCFG_FIXUP() macro and
> > kept self contained. Example:
> >
> > /* Custom PCI config ops */
> > static struct pci_generic_ecam_ops foo_pci_ops = {
> > 	.bus_shift	= 24,
> > 	.pci_ops = {
> > 		.map_bus = pci_ecam_map_bus,
> > 		.read = foo_ecam_config_read,
> > 		.write = foo_ecam_config_write,
> > 	}
> > };
> >
> > DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, <oem_id_str>, <oem_rev>,
> <domain_nr>, <bus_nr>);
> >
> > Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> > ---
> >  drivers/acpi/pci_mcfg.c           | 32
> ++++++++++++++++++++++++++++++++
> >  include/asm-generic/vmlinux.lds.h |  7 +++++++
> >  include/linux/pci-acpi.h          | 19 +++++++++++++++++++
> >  3 files changed, 58 insertions(+)
> >
> > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> > index 1847f74..f3d4570 100644
> > --- a/drivers/acpi/pci_mcfg.c
> > +++ b/drivers/acpi/pci_mcfg.c
> > @@ -22,11 +22,43 @@
> >  #include <linux/kernel.h>
> >  #include <linux/pci.h>
> >  #include <linux/pci-acpi.h>
> > +#include <linux/pci-ecam.h>
> >
> >  /* Root pointer to the mapped MCFG table */
> >  static struct acpi_table_mcfg *mcfg_table;
> >  static int mcfg_entries;
> >
> > +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
> > +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
> > +
> > +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> > +{
> > +	int bus_num = root->secondary.start;
> > +	int domain = root->segment;
> > +	struct pci_cfg_fixup *f;
> > +
> > +	if (!mcfg_table)
> > +		return &pci_generic_ecam_ops;
> > +
> > +	/*
> > +	 * Match against platform specific quirks and return
> corresponding
> > +	 * CAM ops.
> > +	 *
> > +	 * First match against PCI topology <domain:bus> then use OEM ID
> and
> > +	 * OEM revision from MCFG table standard header.
> > +	 */
> > +	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups;
> f++) {
> > +		if ((f->domain == domain || f->domain ==
> PCI_MCFG_DOMAIN_ANY) &&
> > +		    (f->bus_num == bus_num || f->bus_num ==
> PCI_MCFG_BUS_ANY) &&
> > +		    (!strncmp(f->oem_id, mcfg_table->header.oem_id,
> > +			      ACPI_OEM_ID_SIZE)) &&
> > +		    (f->oem_revision == mcfg_table->header.oem_revision))
> 
> Is this more likely to be updated between quirky and fixed platforms
> than oem_table_id? What do folks think about using oem_table_id instead
> of, or in addition to, oem_revision?

>From my understanding we need to stick to this mechanism as (otherwise)
there are platforms out in the field that would need a FW update.

So I don't think that using oem_table_id "instead" is possible; about
"in addition" I think it is doable, but I do not see the advantage much.
I mean that if a platform gets fixed the oem revision should change too,
Right?   

Thanks

Gab

> 
> In case these details are helpful, here was my simple prototype of an
> MCFG based approach:
> 
> https://codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-rc1-
> testing&id=c5d8bc49a198fd8f61f82c7d8f169564d6176b07
> https://codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-rc1-
> testing&id=50bfe77ccd1639e6ce8c7c4fcca187d50e0bead4
> 
> Thanks,
> Cov
> 
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

[David Daney]

On 06/03/2016 08:32 AM, Gabriele Paoloni wrote:
[...]
>>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
>>> +{
>>> +	int bus_num = root->secondary.start;
>>> +	int domain = root->segment;
>>> +	struct pci_cfg_fixup *f;
>>> +
>>> +	if (!mcfg_table)
>>> +		return &pci_generic_ecam_ops;
>>> +
>>> +	/*
>>> +	 * Match against platform specific quirks and return
>> corresponding
>>> +	 * CAM ops.
>>> +	 *
>>> +	 * First match against PCI topology <domain:bus> then use OEM ID
>> and
>>> +	 * OEM revision from MCFG table standard header.
>>> +	 */
>>> +	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups;
>> f++) {
>>> +		if ((f->domain == domain || f->domain ==
>> PCI_MCFG_DOMAIN_ANY) &&
>>> +		    (f->bus_num == bus_num || f->bus_num ==
>> PCI_MCFG_BUS_ANY) &&
>>> +		    (!strncmp(f->oem_id, mcfg_table->header.oem_id,
>>> +			      ACPI_OEM_ID_SIZE)) &&
>>> +		    (f->oem_revision == mcfg_table->header.oem_revision))
>>
>> Is this more likely to be updated between quirky and fixed platforms
>> than oem_table_id? What do folks think about using oem_table_id instead
>> of, or in addition to, oem_revision?
>
>  From my understanding we need to stick to this mechanism as (otherwise)
> there are platforms out in the field that would need a FW update.
>
> So I don't think that using oem_table_id "instead" is possible; about
> "in addition" I think it is doable, but I do not see the advantage much.
> I mean that if a platform gets fixed the oem revision should change too,
> Right?

I think you are correct.  My take away on discussions about using this 
style of quirk matching was that we would require the oem_revision to 
change as different quirks (or lack of quirks) were required.

David Daney


>
> Thanks
>
> Gab
>

[RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

[Jeffrey Hugo]

On 6/3/2016 9:32 AM, Gabriele Paoloni wrote:
> Hi Cov
>
>> -----Original Message-----
>> From: linux-pci-owner at vger.kernel.org [mailto:linux-pci-
>> owner at vger.kernel.org] On Behalf Of Christopher Covington
>> Sent: 03 June 2016 16:15
>> To: Tomasz Nowicki; helgaas at kernel.org; arnd at arndb.de;
>> will.deacon at arm.com; catalin.marinas at arm.com; rafael at kernel.org;
>> hanjun.guo at linaro.org; Lorenzo.Pieralisi at arm.com; okaya at codeaurora.org;
>> jchandra at broadcom.com
>> Cc: jcm at redhat.com; linaro-acpi at lists.linaro.org; linux-
>> pci at vger.kernel.org; dhdang at apm.com; Liviu.Dudau at arm.com;
>> ddaney at caviumnetworks.com; jeremy.linton at arm.com; linux-
>> kernel at vger.kernel.org; linux-acpi at vger.kernel.org;
>> robert.richter at caviumnetworks.com; Suravee.Suthikulpanit at amd.com;
>> msalter at redhat.com; Wangyijing; mw at semihalf.com;
>> andrea.gallo at linaro.org; linux-arm-kernel at lists.infradead.org;
>> liudongdong (C); Gabriele Paoloni
>> Subject: Re: [RFC PATCH 1/3] pci, acpi: Match PCI config space
>> accessors against platfrom specific ECAM quirks.
>>
>> Hi Tomasz,
>>
>> Thanks for your work on this.
>>
>> On 06/02/2016 04:41 AM, Tomasz Nowicki wrote:
>>> Some platforms may not be fully compliant with generic set of PCI
>> config
>>> accessors. For these cases we implement the way to overwrite
>> accessors
>>> set. Algorithm traverses available quirk list, matches against
>>> <oem_id, oem_rev, domain, bus number> tuple and returns corresponding
>>> PCI config ops. oem_id and oem_rev come from MCFG table standard
>> header.
>>> All quirks can be defined using DECLARE_ACPI_MCFG_FIXUP() macro and
>>> kept self contained. Example:
>>>
>>> /* Custom PCI config ops */
>>> static struct pci_generic_ecam_ops foo_pci_ops = {
>>> 	.bus_shift	= 24,
>>> 	.pci_ops = {
>>> 		.map_bus = pci_ecam_map_bus,
>>> 		.read = foo_ecam_config_read,
>>> 		.write = foo_ecam_config_write,
>>> 	}
>>> };
>>>
>>> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, <oem_id_str>, <oem_rev>,
>> <domain_nr>, <bus_nr>);
>>>
>>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>>> ---
>>>  drivers/acpi/pci_mcfg.c           | 32
>> ++++++++++++++++++++++++++++++++
>>>  include/asm-generic/vmlinux.lds.h |  7 +++++++
>>>  include/linux/pci-acpi.h          | 19 +++++++++++++++++++
>>>  3 files changed, 58 insertions(+)
>>>
>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>>> index 1847f74..f3d4570 100644
>>> --- a/drivers/acpi/pci_mcfg.c
>>> +++ b/drivers/acpi/pci_mcfg.c
>>> @@ -22,11 +22,43 @@
>>>  #include <linux/kernel.h>
>>>  #include <linux/pci.h>
>>>  #include <linux/pci-acpi.h>
>>> +#include <linux/pci-ecam.h>
>>>
>>>  /* Root pointer to the mapped MCFG table */
>>>  static struct acpi_table_mcfg *mcfg_table;
>>>  static int mcfg_entries;
>>>
>>> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
>>> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
>>> +
>>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
>>> +{
>>> +	int bus_num = root->secondary.start;
>>> +	int domain = root->segment;
>>> +	struct pci_cfg_fixup *f;
>>> +
>>> +	if (!mcfg_table)
>>> +		return &pci_generic_ecam_ops;
>>> +
>>> +	/*
>>> +	 * Match against platform specific quirks and return
>> corresponding
>>> +	 * CAM ops.
>>> +	 *
>>> +	 * First match against PCI topology <domain:bus> then use OEM ID
>> and
>>> +	 * OEM revision from MCFG table standard header.
>>> +	 */
>>> +	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups;
>> f++) {
>>> +		if ((f->domain == domain || f->domain ==
>> PCI_MCFG_DOMAIN_ANY) &&
>>> +		    (f->bus_num == bus_num || f->bus_num ==
>> PCI_MCFG_BUS_ANY) &&
>>> +		    (!strncmp(f->oem_id, mcfg_table->header.oem_id,
>>> +			      ACPI_OEM_ID_SIZE)) &&
>>> +		    (f->oem_revision == mcfg_table->header.oem_revision))
>>
>> Is this more likely to be updated between quirky and fixed platforms
>> than oem_table_id? What do folks think about using oem_table_id instead
>> of, or in addition to, oem_revision?
>
> From my understanding we need to stick to this mechanism as (otherwise)
> there are platforms out in the field that would need a FW update.
>
> So I don't think that using oem_table_id "instead" is possible; about
> "in addition" I think it is doable, but I do not see the advantage much.
> I mean that if a platform gets fixed the oem revision should change too,
> Right?

Cov and I had a discussion about this, so hopefully I can bring a 
slightly different perspective that will make sense.

We forsee a situation where we have platform A that needs a quirk, and 
platform B that does not.  The OEM id is the same for both platforms as 
they are different platforms from the same OEM.  Using the OEM revision 
field does not seem to be appropriate since these are different 
platforms and the revision field appears to be for the purpose of 
tracking differences within a single platform.  Therefore, Cov is 
proposing using the OEM table id as a mechanism to distinguish platform 
A (needs quirk applied) vs platform B (no quirks) from the same OEM.

>
> Thanks
>
> Gab
>
>>
>> In case these details are helpful, here was my simple prototype of an
>> MCFG based approach:
>>
>> https://codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-rc1-
>> testing&id=c5d8bc49a198fd8f61f82c7d8f169564d6176b07
>> https://codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-rc1-
>> testing&id=50bfe77ccd1639e6ce8c7c4fcca187d50e0bead4
>>
>> Thanks,
>> Cov
>>
>> --
>> Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>


-- 
Jeffrey Hugo
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

[RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

[Gabriele Paoloni]

Hi Jeffrey

> -----Original Message-----
> From: Jeffrey Hugo [mailto:jhugo at codeaurora.org]
> Sent: 03 June 2016 18:00
> To: Gabriele Paoloni; Christopher Covington; Tomasz Nowicki;
> helgaas at kernel.org; arnd at arndb.de; will.deacon at arm.com;
> catalin.marinas at arm.com; rafael at kernel.org; hanjun.guo at linaro.org;
> Lorenzo.Pieralisi at arm.com; okaya at codeaurora.org; jchandra at broadcom.com
> Cc: liudongdong (C); linaro-acpi at lists.linaro.org; jcm at redhat.com;
> dhdang at apm.com; Liviu.Dudau at arm.com; ddaney at caviumnetworks.com;
> jeremy.linton at arm.com; linux-kernel at vger.kernel.org; linux-
> acpi at vger.kernel.org; robert.richter at caviumnetworks.com;
> msalter at redhat.com; Suravee.Suthikulpanit at amd.com; linux-
> pci at vger.kernel.org; Wangyijing; mw at semihalf.com;
> andrea.gallo at linaro.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [RFC PATCH 1/3] pci, acpi: Match PCI config space
> accessors against platfrom specific ECAM quirks.
> 
> On 6/3/2016 9:32 AM, Gabriele Paoloni wrote:
> > Hi Cov
> >
> >> -----Original Message-----
> >> From: linux-pci-owner at vger.kernel.org [mailto:linux-pci-
> >> owner at vger.kernel.org] On Behalf Of Christopher Covington
> >> Sent: 03 June 2016 16:15
> >> To: Tomasz Nowicki; helgaas at kernel.org; arnd at arndb.de;
> >> will.deacon at arm.com; catalin.marinas at arm.com; rafael at kernel.org;
> >> hanjun.guo at linaro.org; Lorenzo.Pieralisi at arm.com;
> okaya at codeaurora.org;
> >> jchandra at broadcom.com
> >> Cc: jcm at redhat.com; linaro-acpi at lists.linaro.org; linux-
> >> pci at vger.kernel.org; dhdang at apm.com; Liviu.Dudau at arm.com;
> >> ddaney at caviumnetworks.com; jeremy.linton at arm.com; linux-
> >> kernel at vger.kernel.org; linux-acpi at vger.kernel.org;
> >> robert.richter at caviumnetworks.com; Suravee.Suthikulpanit at amd.com;
> >> msalter at redhat.com; Wangyijing; mw at semihalf.com;
> >> andrea.gallo at linaro.org; linux-arm-kernel at lists.infradead.org;
> >> liudongdong (C); Gabriele Paoloni
> >> Subject: Re: [RFC PATCH 1/3] pci, acpi: Match PCI config space
> >> accessors against platfrom specific ECAM quirks.
> >>
> >> Hi Tomasz,
> >>
> >> Thanks for your work on this.
> >>
> >> On 06/02/2016 04:41 AM, Tomasz Nowicki wrote:
> >>> Some platforms may not be fully compliant with generic set of PCI
> >> config
> >>> accessors. For these cases we implement the way to overwrite
> >> accessors
> >>> set. Algorithm traverses available quirk list, matches against
> >>> <oem_id, oem_rev, domain, bus number> tuple and returns
> corresponding
> >>> PCI config ops. oem_id and oem_rev come from MCFG table standard
> >> header.
> >>> All quirks can be defined using DECLARE_ACPI_MCFG_FIXUP() macro and
> >>> kept self contained. Example:
> >>>
> >>> /* Custom PCI config ops */
> >>> static struct pci_generic_ecam_ops foo_pci_ops = {
> >>> 	.bus_shift	= 24,
> >>> 	.pci_ops = {
> >>> 		.map_bus = pci_ecam_map_bus,
> >>> 		.read = foo_ecam_config_read,
> >>> 		.write = foo_ecam_config_write,
> >>> 	}
> >>> };
> >>>
> >>> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, <oem_id_str>, <oem_rev>,
> >> <domain_nr>, <bus_nr>);
> >>>
> >>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> >>> ---
> >>>  drivers/acpi/pci_mcfg.c           | 32
> >> ++++++++++++++++++++++++++++++++
> >>>  include/asm-generic/vmlinux.lds.h |  7 +++++++
> >>>  include/linux/pci-acpi.h          | 19 +++++++++++++++++++
> >>>  3 files changed, 58 insertions(+)
> >>>
> >>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> >>> index 1847f74..f3d4570 100644
> >>> --- a/drivers/acpi/pci_mcfg.c
> >>> +++ b/drivers/acpi/pci_mcfg.c
> >>> @@ -22,11 +22,43 @@
> >>>  #include <linux/kernel.h>
> >>>  #include <linux/pci.h>
> >>>  #include <linux/pci-acpi.h>
> >>> +#include <linux/pci-ecam.h>
> >>>
> >>>  /* Root pointer to the mapped MCFG table */
> >>>  static struct acpi_table_mcfg *mcfg_table;
> >>>  static int mcfg_entries;
> >>>
> >>> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
> >>> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
> >>> +
> >>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> >>> +{
> >>> +	int bus_num = root->secondary.start;
> >>> +	int domain = root->segment;
> >>> +	struct pci_cfg_fixup *f;
> >>> +
> >>> +	if (!mcfg_table)
> >>> +		return &pci_generic_ecam_ops;
> >>> +
> >>> +	/*
> >>> +	 * Match against platform specific quirks and return
> >> corresponding
> >>> +	 * CAM ops.
> >>> +	 *
> >>> +	 * First match against PCI topology <domain:bus> then use OEM ID
> >> and
> >>> +	 * OEM revision from MCFG table standard header.
> >>> +	 */
> >>> +	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups;
> >> f++) {
> >>> +		if ((f->domain == domain || f->domain ==
> >> PCI_MCFG_DOMAIN_ANY) &&
> >>> +		    (f->bus_num == bus_num || f->bus_num ==
> >> PCI_MCFG_BUS_ANY) &&
> >>> +		    (!strncmp(f->oem_id, mcfg_table->header.oem_id,
> >>> +			      ACPI_OEM_ID_SIZE)) &&
> >>> +		    (f->oem_revision == mcfg_table->header.oem_revision))
> >>
> >> Is this more likely to be updated between quirky and fixed platforms
> >> than oem_table_id? What do folks think about using oem_table_id
> instead
> >> of, or in addition to, oem_revision?
> >
> > From my understanding we need to stick to this mechanism as
> (otherwise)
> > there are platforms out in the field that would need a FW update.
> >
> > So I don't think that using oem_table_id "instead" is possible; about
> > "in addition" I think it is doable, but I do not see the advantage
> much.
> > I mean that if a platform gets fixed the oem revision should change
> too,
> > Right?
> 
> Cov and I had a discussion about this, so hopefully I can bring a
> slightly different perspective that will make sense.
> 
> We forsee a situation where we have platform A that needs a quirk, and
> platform B that does not.  The OEM id is the same for both platforms as
> they are different platforms from the same OEM.  Using the OEM revision
> field does not seem to be appropriate since these are different
> platforms and the revision field appears to be for the purpose of
> tracking differences within a single platform.  Therefore, Cov is
> proposing using the OEM table id as a mechanism to distinguish platform
> A (needs quirk applied) vs platform B (no quirks) from the same OEM.

Ah yes I see now...

Probably it should be ok to have a check on all three OEM fields.

Thanks for explaining

Gab 

> 
> >
> > Thanks
> >
> > Gab
> >
> >>
> >> In case these details are helpful, here was my simple prototype of
> an
> >> MCFG based approach:
> >>
> >> https://codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-
> rc1-
> >> testing&id=c5d8bc49a198fd8f61f82c7d8f169564d6176b07
> >> https://codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-
> rc1-
> >> testing&id=50bfe77ccd1639e6ce8c7c4fcca187d50e0bead4
> >>
> >> Thanks,
> >> Cov
> >>
> >> --
> >> Qualcomm Innovation Center, Inc.
> >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> >> a Linux Foundation Collaborative Project
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-pci"
> in
> >> the body of a message to majordomo at vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> 
> 
> --
> Jeffrey Hugo
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project

[RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks.

[Hanjun Guo]

On 2016/6/6 15:27, Gabriele Paoloni wrote:
> Hi Jeffrey
>> On 6/3/2016 9:32 AM, Gabriele Paoloni wrote:
>>> Hi Cov
>>>
>>>> Hi Tomasz,
>>>>
>>>> Thanks for your work on this.
>>>>
>>>> On 06/02/2016 04:41 AM, Tomasz Nowicki wrote:
>>>>> Some platforms may not be fully compliant with generic set of PCI
>>>> config
>>>>> accessors. For these cases we implement the way to overwrite
>>>> accessors
>>>>> set. Algorithm traverses available quirk list, matches against
>>>>> <oem_id, oem_rev, domain, bus number> tuple and returns
>> corresponding
>>>>> PCI config ops. oem_id and oem_rev come from MCFG table standard
>>>> header.
>>>>> All quirks can be defined using DECLARE_ACPI_MCFG_FIXUP() macro and
>>>>> kept self contained. Example:
>>>>>
>>>>> /* Custom PCI config ops */
>>>>> static struct pci_generic_ecam_ops foo_pci_ops = {
>>>>> 	.bus_shift	= 24,
>>>>> 	.pci_ops = {
>>>>> 		.map_bus = pci_ecam_map_bus,
>>>>> 		.read = foo_ecam_config_read,
>>>>> 		.write = foo_ecam_config_write,
>>>>> 	}
>>>>> };
>>>>>
>>>>> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, <oem_id_str>, <oem_rev>,
>>>> <domain_nr>, <bus_nr>);
>>>>>
>>>>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>>>>> ---
>>>>>  drivers/acpi/pci_mcfg.c           | 32
>>>> ++++++++++++++++++++++++++++++++
>>>>>  include/asm-generic/vmlinux.lds.h |  7 +++++++
>>>>>  include/linux/pci-acpi.h          | 19 +++++++++++++++++++
>>>>>  3 files changed, 58 insertions(+)
>>>>>
>>>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>>>>> index 1847f74..f3d4570 100644
>>>>> --- a/drivers/acpi/pci_mcfg.c
>>>>> +++ b/drivers/acpi/pci_mcfg.c
>>>>> @@ -22,11 +22,43 @@
>>>>>  #include <linux/kernel.h>
>>>>>  #include <linux/pci.h>
>>>>>  #include <linux/pci-acpi.h>
>>>>> +#include <linux/pci-ecam.h>
>>>>>
>>>>>  /* Root pointer to the mapped MCFG table */
>>>>>  static struct acpi_table_mcfg *mcfg_table;
>>>>>  static int mcfg_entries;
>>>>>
>>>>> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
>>>>> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
>>>>> +
>>>>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
>>>>> +{
>>>>> +	int bus_num = root->secondary.start;
>>>>> +	int domain = root->segment;
>>>>> +	struct pci_cfg_fixup *f;
>>>>> +
>>>>> +	if (!mcfg_table)
>>>>> +		return &pci_generic_ecam_ops;
>>>>> +
>>>>> +	/*
>>>>> +	 * Match against platform specific quirks and return
>>>> corresponding
>>>>> +	 * CAM ops.
>>>>> +	 *
>>>>> +	 * First match against PCI topology <domain:bus> then use OEM ID
>>>> and
>>>>> +	 * OEM revision from MCFG table standard header.
>>>>> +	 */
>>>>> +	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups;
>>>> f++) {
>>>>> +		if ((f->domain == domain || f->domain ==
>>>> PCI_MCFG_DOMAIN_ANY) &&
>>>>> +		    (f->bus_num == bus_num || f->bus_num ==
>>>> PCI_MCFG_BUS_ANY) &&
>>>>> +		    (!strncmp(f->oem_id, mcfg_table->header.oem_id,
>>>>> +			      ACPI_OEM_ID_SIZE)) &&
>>>>> +		    (f->oem_revision == mcfg_table->header.oem_revision))
>>>>
>>>> Is this more likely to be updated between quirky and fixed platforms
>>>> than oem_table_id? What do folks think about using oem_table_id
>> instead
>>>> of, or in addition to, oem_revision?
>>>
>>> From my understanding we need to stick to this mechanism as
>> (otherwise)
>>> there are platforms out in the field that would need a FW update.
>>>
>>> So I don't think that using oem_table_id "instead" is possible; about
>>> "in addition" I think it is doable, but I do not see the advantage
>> much.
>>> I mean that if a platform gets fixed the oem revision should change
>> too,
>>> Right?
>>
>> Cov and I had a discussion about this, so hopefully I can bring a
>> slightly different perspective that will make sense.
>>
>> We forsee a situation where we have platform A that needs a quirk, and
>> platform B that does not.  The OEM id is the same for both platforms as
>> they are different platforms from the same OEM.  Using the OEM revision
>> field does not seem to be appropriate since these are different
>> platforms and the revision field appears to be for the purpose of
>> tracking differences within a single platform.  Therefore, Cov is
>> proposing using the OEM table id as a mechanism to distinguish platform
>> A (needs quirk applied) vs platform B (no quirks) from the same OEM.
>
> Ah yes I see now...
>
> Probably it should be ok to have a check on all three OEM fields.

Just for reference, x86 and IA64 use oem_id and oem_table_id to make a
difference between different platforms, see
acpi_madt_oem_check(char *oem_id, char *oem_table_id) for x86 and ia64,
that can apply to ARM64 on MCFG too.

Thanks
Hanjun

[RFC PATCH 2/3] arm64, pci: Start using quirks handling for ACPI based PCI host controller.

[Tomasz Nowicki]

pci_generic_ecam_ops is used by default. Since there are platforms
which have non-compliant ECAM space we need to overwrite these
accessors prior to PCI buses enumeration. In order to do that
we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
we can use proper PCI config space accessors and bus_shift.

pci_generic_ecam_ops is still used for platforms free from quirks.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 arch/arm64/kernel/pci.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 39f2a40..3a83c28 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -140,6 +140,7 @@ static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root,
 	struct resource cfgres;
 	unsigned int bsz;
 	int err;
+	struct pci_ecam_ops *ops;
 
 	err = pci_mcfg_lookup(root);
 	if (err) {
@@ -147,12 +148,12 @@ static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root,
 		return err;
 	}
 
-	bsz = 1 << pci_generic_ecam_ops.bus_shift;
+	ops = pci_mcfg_get_ops(root);
+	bsz = 1 << ops->bus_shift;
 	cfgres.start = root->mcfg_addr + bus_res->start * bsz;
 	cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
 	cfgres.flags = IORESOURCE_MEM;
-	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,
-			      &pci_generic_ecam_ops);
+	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops);
 	if (IS_ERR(cfg)) {
 		pr_err("%04x:%pR error %ld mapping CAM\n", seg, bus_res,
 		       PTR_ERR(cfg));
-- 
1.9.1

[RFC PATCH 3/3] pci, pci-thunder-pem: Add ACPI support for ThunderX PEM.

[Tomasz Nowicki]

This patch uses DECLARE_ACPI_MCFG_FIXUP to overwrite PCI config accessors.
Also, it provides alternative way to find additional configuration region:
thunder_pem_get_acpi_res is looking for host bridge's child (_HID "THRX0001")
which contains mentioned configuration region description.
See example below:

Device (PEM0) {
    Name (_HID, EISAID ("PNP0A08"))
    Name (_CID, EISAID ("PNP0A03"))

    [...]

    Device (CFG0)
    {
      Name (_HID, "THRX0001") // PEM configuration space resources
      Name (_CRS, ResourceTemplate () {
        QWordMemory(ResourceConsumer, PosDecode, MinFixed, MaxFixed,
          NonCacheable, ReadWrite, 0, 0x87e0c5000000, 0x87E0C5FFFFFF,
          0, 0x01000000)
      })
    }
}

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 drivers/pci/host/pci-thunder-pem.c | 132 +++++++++++++++++++++++++++++++++----
 1 file changed, 119 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
index 91f6fc6..8f002ef 100644
--- a/drivers/pci/host/pci-thunder-pem.c
+++ b/drivers/pci/host/pci-thunder-pem.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-ecam.h>
 #include <linux/platform_device.h>
 
@@ -284,6 +285,83 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
 	return pci_generic_config_write(bus, devfn, where, size, val);
 }
 
+#ifdef CONFIG_ACPI
+
+struct pem_acpi_res {
+	struct resource	resource;
+	int found;
+};
+
+static acpi_status
+thunder_pem_cfg(struct acpi_resource *resource, void *ctx)
+{
+	struct pem_acpi_res *pem_ctx = ctx;
+	struct resource *res = &pem_ctx->resource;
+
+	if ((resource->type != ACPI_RESOURCE_TYPE_ADDRESS64) ||
+	    (resource->data.address32.resource_type != ACPI_MEMORY_RANGE))
+		return AE_OK;
+
+	res->start = resource->data.address64.address.minimum;
+	res->end = resource->data.address64.address.maximum;
+	res->flags = IORESOURCE_MEM;
+
+	pem_ctx->found++;
+	return AE_OK;
+}
+
+static acpi_status
+thunder_pem_find_dev(acpi_handle handle, u32 level, void *ctx, void **ret)
+{
+	struct pem_acpi_res *pem_ctx = ctx;
+	struct acpi_device_info *info;
+	acpi_status status = AE_OK;
+
+	status = acpi_get_object_info(handle, &info);
+	if (ACPI_FAILURE(status))
+		return AE_OK;
+
+	if (strncmp(info->hardware_id.string, "THRX0001", 8) != 0)
+		goto out;
+
+	pem_ctx->found = 0;
+	status = acpi_walk_resources(handle, METHOD_NAME__CRS, thunder_pem_cfg,
+				     pem_ctx);
+	if (ACPI_FAILURE(status))
+		goto out;
+
+	if (pem_ctx->found)
+		status = AE_CTRL_TERMINATE;
+out:
+	kfree(info);
+	return status;
+}
+
+static struct resource *thunder_pem_get_acpi_res(struct device *dev)
+{
+	struct acpi_device *adev = to_acpi_device(dev);
+	acpi_handle handle = acpi_device_handle(adev);
+	struct pem_acpi_res *pem_ctx;
+	acpi_status status;
+
+	pem_ctx = devm_kzalloc(dev, sizeof(*pem_ctx), GFP_KERNEL);
+	if (!pem_ctx)
+		return NULL;
+
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+				     thunder_pem_find_dev, NULL, pem_ctx, NULL);
+	if (ACPI_FAILURE(status) || !pem_ctx->found)
+		return NULL;
+
+	return &pem_ctx->resource;
+}
+#else
+static struct resource *thunder_pem_get_acpi_res(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 static int thunder_pem_init(struct pci_config_window *cfg)
 {
 	struct device *dev = cfg->parent;
@@ -292,24 +370,24 @@ static int thunder_pem_init(struct pci_config_window *cfg)
 	struct thunder_pem_pci *pem_pci;
 	struct platform_device *pdev;
 
-	/* Only OF support for now */
-	if (!dev->of_node)
-		return -EINVAL;
-
 	pem_pci = devm_kzalloc(dev, sizeof(*pem_pci), GFP_KERNEL);
 	if (!pem_pci)
 		return -ENOMEM;
 
-	pdev = to_platform_device(dev);
-
-	/*
-	 * The second register range is the PEM bridge to the PCIe
-	 * bus.  It has a different config access method than those
-	 * devices behind the bridge.
-	 */
-	res_pem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (acpi_disabled) {
+		pdev = to_platform_device(dev);
+
+		/*
+		 * The second register range is the PEM bridge to the PCIe
+		 * bus.  It has a different config access method than those
+		 * devices behind the bridge.
+		 */
+		res_pem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	} else {
+		res_pem = thunder_pem_get_acpi_res(dev);
+	}
 	if (!res_pem) {
-		dev_err(dev, "missing \"reg[1]\"property\n");
+		dev_err(dev, "missing configuration region\n");
 		return -EINVAL;
 	}
 
@@ -362,5 +440,33 @@ static struct platform_driver thunder_pem_driver = {
 };
 module_platform_driver(thunder_pem_driver);
 
+#ifdef CONFIG_ACPI
+
+DECLARE_ACPI_MCFG_FIXUP(&pci_thunder_pem_ops, "CAVIUM", 1,
+			4, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&pci_thunder_pem_ops, "CAVIUM", 1,
+			5, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&pci_thunder_pem_ops, "CAVIUM", 1,
+			6, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&pci_thunder_pem_ops, "CAVIUM", 1,
+			7, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&pci_thunder_pem_ops, "CAVIUM", 1,
+			8, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&pci_thunder_pem_ops, "CAVIUM", 1,
+			9, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&pci_thunder_pem_ops, "CAVIUM", 1,
+			14, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&pci_thunder_pem_ops, "CAVIUM", 1,
+			15, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&pci_thunder_pem_ops, "CAVIUM", 1,
+			16, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&pci_thunder_pem_ops, "CAVIUM", 1,
+			17, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&pci_thunder_pem_ops, "CAVIUM", 1,
+			18, PCI_MCFG_BUS_ANY);
+DECLARE_ACPI_MCFG_FIXUP(&pci_thunder_pem_ops, "CAVIUM", 1,
+			19, PCI_MCFG_BUS_ANY);
+#endif
+
 MODULE_DESCRIPTION("Thunder PEM PCIe host driver");
 MODULE_LICENSE("GPL v2");
-- 
1.9.1

[RFC PATCH 0/3] ECAM quirks handling for ARM64 platforms

[Bjorn Helgaas]

On Thu, Jun 02, 2016 at 10:41:00AM +0200, Tomasz Nowicki wrote:
> This series bases on pending ACPI PCI support for ARM64:
> https://lkml.org/lkml/2016/5/30/468
> 
> Quirk handling relies on an idea of matching MCFG OEM ID and OEM revision
> (the ones from standard header of MCFG table). Linker section is used
> so that quirks can be registered using special macro (see patches) and
> kept self contained.
> 
> As an example, last patch presents above mechanism usage for ThunderX PEM driver.
> 
> Tomasz Nowicki (3):
>   pci, acpi: Match PCI config space accessors against platfrom specific
>     ECAM quirks.
>   arm64, pci: Start using quirks handling for ACPI based PCI host
>     controller.
>   pci, pci-thunder-pem: Add ACPI support for ThunderX PEM.
> 
>  arch/arm64/kernel/pci.c            |   7 +-
>  drivers/acpi/pci_mcfg.c            |  32 +++++++++
>  drivers/pci/host/pci-thunder-pem.c | 132 +++++++++++++++++++++++++++++++++----
>  include/asm-generic/vmlinux.lds.h  |   7 ++
>  include/linux/pci-acpi.h           |  19 ++++++
>  5 files changed, 181 insertions(+), 16 deletions(-)

Is this series superceded by Dongdong's series of 6/13 ("[RFC,V2,1/2]
ACPI/PCI: Match PCI config space accessors against platfrom specific ECAM
quirks")?

[RFC PATCH 0/3] ECAM quirks handling for ARM64 platforms

[Tomasz Nowicki]

On 19.07.2016 23:17, Bjorn Helgaas wrote:
> On Thu, Jun 02, 2016 at 10:41:00AM +0200, Tomasz Nowicki wrote:
>> This series bases on pending ACPI PCI support for ARM64:
>> https://lkml.org/lkml/2016/5/30/468
>>
>> Quirk handling relies on an idea of matching MCFG OEM ID and OEM revision
>> (the ones from standard header of MCFG table). Linker section is used
>> so that quirks can be registered using special macro (see patches) and
>> kept self contained.
>>
>> As an example, last patch presents above mechanism usage for ThunderX PEM driver.
>>
>> Tomasz Nowicki (3):
>>    pci, acpi: Match PCI config space accessors against platfrom specific
>>      ECAM quirks.
>>    arm64, pci: Start using quirks handling for ACPI based PCI host
>>      controller.
>>    pci, pci-thunder-pem: Add ACPI support for ThunderX PEM.
>>
>>   arch/arm64/kernel/pci.c            |   7 +-
>>   drivers/acpi/pci_mcfg.c            |  32 +++++++++
>>   drivers/pci/host/pci-thunder-pem.c | 132 +++++++++++++++++++++++++++++++++----
>>   include/asm-generic/vmlinux.lds.h  |   7 ++
>>   include/linux/pci-acpi.h           |  19 ++++++
>>   5 files changed, 181 insertions(+), 16 deletions(-)
>
> Is this series superceded by Dongdong's series of 6/13 ("[RFC,V2,1/2]
> ACPI/PCI: Match PCI config space accessors against platfrom specific ECAM
> quirks")?
>

Yes this series had two another versions (v2,v3) posted by someone else. 
However, I posted another v4 which is the latest one:
[RFC PATCH v4 0/5] ECAM quirks handling for ARM64 platforms
https://lkml.org/lkml/2016/6/28/165

In v4 there is one minor thing to be fixed. Do you want me to resend it 
as v5 including mentioned fix ?

Thanks,
Tomasz