diff mbox

[V2,22/23] pci, acpi: Match PCI config space accessors against platfrom specific quirks.

Message ID 1450278993-12664-23-git-send-email-tn@semihalf.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Nowicki Dec. 16, 2015, 3:16 p.m. UTC
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 before PCI buses enumeration. Algorithm that overwrite accessors
matches against platform ID (DMI), domain and bus number, hopefully
enough for all cases. All quirks can be defined using:
DECLARE_ACPI_MCFG_FIXUP() and keep self contained.

example:

static const struct dmi_system_id yyy[] = {
	{
		.ident = "<Platform ident string>",
		.callback = <handler>,
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
			DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
			DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
		},
	},
	{ }
};

static struct pci_ops ecam_pci_ops = {
	.map_bus = pci_mcfg_dev_base,
	.read = xxxx_ecam_config_read,
	.write = xxxx_ecam_config_write,
};
DECLARE_ACPI_MCFG_FIXUP(yyy, &ecam_pci_ops, <domain_nr>, <bus_nr>);

Note, that more custom actions can be done via DMI callback hook.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 drivers/acpi/mcfg.c               | 35 +++++++++++++++++++++++++++++++++--
 include/asm-generic/vmlinux.lds.h |  7 +++++++
 include/linux/ecam.h              | 17 +++++++++++++++++
 3 files changed, 57 insertions(+), 2 deletions(-)

Comments

Gabriele Paoloni Dec. 21, 2015, 11:47 a.m. UTC | #1
Hi Tomasz

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Tomasz Nowicki
> Sent: 16 December 2015 15:17
> To: bhelgaas@google.com; arnd@arndb.de; will.deacon@arm.com;
> catalin.marinas@arm.com; rjw@rjwysocki.net; hanjun.guo@linaro.org;
> Lorenzo.Pieralisi@arm.com; okaya@codeaurora.org;
> jiang.liu@linux.intel.com; Stefano.Stabellini@eu.citrix.com
> Cc: robert.richter@caviumnetworks.com; mw@semihalf.com;
> Liviu.Dudau@arm.com; ddaney@caviumnetworks.com; tglx@linutronix.de;
> Wangyijing; Suravee.Suthikulpanit@amd.com; msalter@redhat.com; linux-
> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linaro-
> acpi@lists.linaro.org; jchandra@broadcom.com; jcm@redhat.com; Tomasz
> Nowicki
> Subject: [PATCH V2 22/23] pci, acpi: Match PCI config space accessors
> against platfrom specific quirks.
> 
> 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 before PCI buses enumeration. Algorithm that overwrite
> accessors matches against platform ID (DMI), domain and bus number,
> hopefully enough for all cases. All quirks can be defined using:
> DECLARE_ACPI_MCFG_FIXUP() and keep self contained.

I've got a couple of comments/questions about this patch..

1) So according to this mechanism quirks would be supported only by 
   vendors whose BIOS are SMBIOS compliant. Now personally I am ok
   with this but I don't know if this is OK in general as it would 
   narrow down the number of platforms that would be able to define
   the quirks...
   Lorenzo, Arnd what is your opinion here?

2) In the quirk mechanism you proposed, I see that the callback function
   allows to do some preparation work for the host bridge. For example in
   Hisilicon hip05 case we would need to read some values from the ACPI 
   table (see acpi_pci_root_hisi_add() function in 
   https://lkml.org/lkml/2015/12/3/426). 
   I am quite new to ACPI and I wonder if it is OK to add such "Packages"
   to the  PCI host bridge ACPI device...or maybe we need to declare a new
   one...?

Many Thanks

Gab

> 
> example:
> 
> static const struct dmi_system_id yyy[] = {
> 	{
> 		.ident = "<Platform ident string>",
> 		.callback = <handler>,
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
> 			DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
> 		},
> 	},
> 	{ }
> };
> 
> static struct pci_ops ecam_pci_ops = {
> 	.map_bus = pci_mcfg_dev_base,
> 	.read = xxxx_ecam_config_read,
> 	.write = xxxx_ecam_config_write,
> };
> DECLARE_ACPI_MCFG_FIXUP(yyy, &ecam_pci_ops, <domain_nr>, <bus_nr>);
> 
> Note, that more custom actions can be done via DMI callback hook.
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>  drivers/acpi/mcfg.c               | 35
> +++++++++++++++++++++++++++++++++--
>  include/asm-generic/vmlinux.lds.h |  7 +++++++
>  include/linux/ecam.h              | 17 +++++++++++++++++
>  3 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/mcfg.c b/drivers/acpi/mcfg.c index
> a9b2231..6d0194d 100644
> --- a/drivers/acpi/mcfg.c
> +++ b/drivers/acpi/mcfg.c
> @@ -8,6 +8,7 @@
>   */
> 
>  #include <linux/acpi.h>
> +#include <linux/dmi.h>
>  #include <linux/ecam.h>
>  #include <linux/pci.h>
>  #include <linux/pci-acpi.h>
> @@ -34,6 +35,31 @@ int __weak raw_pci_write(unsigned int domain,
> unsigned int bus,
>  	return PCIBIOS_DEVICE_NOT_FOUND;
>  }
> 
> +extern struct pci_mcfg_fixup __start_acpi_mcfg_fixups[]; extern struct
> +pci_mcfg_fixup __end_acpi_mcfg_fixups[];
> +
> +static struct pci_ops *pci_mcfg_check_quirks(int domain, int
> +bus_number) {
> +	struct pci_mcfg_fixup *fixup;
> +
> +	fixup = __start_acpi_mcfg_fixups;
> +	while (fixup < __end_acpi_mcfg_fixups) {
> +		if (dmi_check_system(fixup->system) &&
> +		    (fixup->domain == domain ||
> +		     fixup->domain == PCI_MCFG_DOMAIN_ANY) &&
> +		    (fixup->bus_number == bus_number ||
> +		     fixup->bus_number == PCI_MCFG_BUS_ANY)) {
> +			pr_info(PREFIX "Fixup applied: Platform [%s] domain
> [%d] bus number [%d]\n",
> +				fixup->system->ident, fixup->domain,
> +				fixup->bus_number);
> +			return fixup->ops;
> +		}
> +		++fixup;
> +	}
> +
> +	return NULL;
> +}
> +
>  void __iomem *
>  pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
> { @@ -56,10 +82,15 @@ static struct pci_ops default_pci_mcfg_ops = {
> 
>  struct pci_ops *pci_mcfg_get_ops(int domain, int bus)  {
> +	struct pci_ops *pci_mcfg_ops_quirk;
> +
>  	/*
> -	 * TODO: Match against platform specific quirks and return
> -	 * corresponding PCI config space accessor set.
> +	 * Match against platform specific quirks and return
> corresponding
> +	 * PCI config space accessor set.
>  	 */
> +	pci_mcfg_ops_quirk = pci_mcfg_check_quirks(domain, bus);
> +	if (pci_mcfg_ops_quirk)
> +		return pci_mcfg_ops_quirk;
> 
>  	return &default_pci_mcfg_ops;
>  }
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-
> generic/vmlinux.lds.h
> index c4bd0e2..c93fc97 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -298,6 +298,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/ecam.h b/include/linux/ecam.h index
> e0f322e..1319fa8 100644
> --- a/include/linux/ecam.h
> +++ b/include/linux/ecam.h
> @@ -20,6 +20,23 @@ struct pci_mmcfg_region {
>  	bool hot_added;
>  };
> 
> +struct pci_mcfg_fixup {
> +	const struct dmi_system_id *system;
> +	struct pci_ops *ops;
> +	int domain;
> +	int bus_number;
> +};
> +
> +#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(system, ops, dom, bus)
> 	\
> +	static const struct pci_mcfg_fixup
> __mcfg_fixup_##system##dom##bus\
> +	 __used	__attribute__((__section__(".acpi_fixup_mcfg"),
> 	\
> +				aligned((sizeof(void *))))) =		\
> +	{ system, ops, dom, bus };
> +
>  struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus);
> struct pci_mmcfg_region *pci_mmconfig_alloc(int segment, int start,
>  						   int end, u64 addr);
> --
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
> in the body of a message to majordomo@vger.kernel.org More majordomo
> info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
Arnd Bergmann Dec. 21, 2015, 2:10 p.m. UTC | #2
On Monday 21 December 2015, Gabriele Paoloni wrote:
> > -----Original Message-----
> > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> > owner@vger.kernel.org] On Behalf Of 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 before PCI buses enumeration. Algorithm that overwrite
> > accessors matches against platform ID (DMI), domain and bus number,
> > hopefully enough for all cases. All quirks can be defined using:
> > DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
> 
> I've got a couple of comments/questions about this patch..
> 
> 1) So according to this mechanism quirks would be supported only by 
>    vendors whose BIOS are SMBIOS compliant. Now personally I am ok
>    with this but I don't know if this is OK in general as it would 
>    narrow down the number of platforms that would be able to define
>    the quirks...
>    Lorenzo, Arnd what is your opinion here?

I'd rather not see the quirks in mainline at all, and only support
SBSA compliant machines, or require the BIOS to work around the hardware
quirks differently (e.g. by trapping config space access through secure
firmware, or going through an AML method to be defined). I'm certainly
ok with making it depend on SMBIOS if we are going to use something like this.

	Arnd
David Daney Dec. 21, 2015, 5:29 p.m. UTC | #3
On 12/21/2015 06:10 AM, Arnd Bergmann wrote:
> On Monday 21 December 2015, Gabriele Paoloni wrote:
>>> -----Original Message-----
>>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>>> owner@vger.kernel.org] On Behalf Of 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 before PCI buses enumeration. Algorithm that overwrite
>>> accessors matches against platform ID (DMI), domain and bus number,
>>> hopefully enough for all cases. All quirks can be defined using:
>>> DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
>>
>> I've got a couple of comments/questions about this patch..
>>
>> 1) So according to this mechanism quirks would be supported only by
>>     vendors whose BIOS are SMBIOS compliant. Now personally I am ok
>>     with this but I don't know if this is OK in general as it would
>>     narrow down the number of platforms that would be able to define
>>     the quirks...
>>     Lorenzo, Arnd what is your opinion here?
>
> I'd rather not see the quirks in mainline at all, and only support
> SBSA compliant machines,

There seems to exist a class of systems that were intended to be SBSA 
compliant, but after they were manufactured turn out to not be fully 
complaint.  It would be nice to have Linux kernel support for some of 
these systems

There also seems to be historical precedent for quirk frameworks in 
various kernel subsystems to handle systems and devices that are not 
fully "Spec. Compliant".

> or require the BIOS to work around the hardware
> quirks differently (e.g. by trapping config space access through secure
> firmware, or going through an AML method to be defined).

Some systems don't seem to have this capability.  For example, in ARMv8 
(A.K.A. arm64), I haven't been able to figure out how to trap these 
accesses to EL3 for emulation.  The specification is 5700 pages long 
though, so perhaps I missed that bit.

> I'm certainly
> ok with making it depend on SMBIOS if we are going to use something like this.
>
> 	Arnd
>
Arnd Bergmann Dec. 21, 2015, 10:42 p.m. UTC | #4
On Monday 21 December 2015, David Daney wrote:
> On 12/21/2015 06:10 AM, Arnd Bergmann wrote:
> > On Monday 21 December 2015, Gabriele Paoloni wrote:
> 
> > or require the BIOS to work around the hardware
> > quirks differently (e.g. by trapping config space access through secure
> > firmware, or going through an AML method to be defined).
> 
> Some systems don't seem to have this capability.  For example, in ARMv8 
> (A.K.A. arm64), I haven't been able to figure out how to trap these 
> accesses to EL3 for emulation.  The specification is 5700 pages long 
> though, so perhaps I missed that bit.

How about using AML then? This would be similar to what CHRP used with
RTAS calls to do PCI config space access.

	Arnd
Jon Masters Dec. 21, 2015, 11:10 p.m. UTC | #5
Sorry for top-posting. A quick note that SMBIOS3 is required by SBBR so it can be presumed that compliant platforms will provide quirks via DMI.
Jon Masters Dec. 21, 2015, 11:24 p.m. UTC | #6
On 12/21/2015 05:42 PM, Arnd Bergmann wrote:
> On Monday 21 December 2015, David Daney wrote:
>> On 12/21/2015 06:10 AM, Arnd Bergmann wrote:
>>> On Monday 21 December 2015, Gabriele Paoloni wrote:
>>
>>> or require the BIOS to work around the hardware
>>> quirks differently (e.g. by trapping config space access through secure
>>> firmware, or going through an AML method to be defined).
>>
>> Some systems don't seem to have this capability.  For example, in ARMv8 
>> (A.K.A. arm64), I haven't been able to figure out how to trap these 
>> accesses to EL3 for emulation.  The specification is 5700 pages long 
>> though, so perhaps I missed that bit.

There isn't a way to directly trap to EL3 for emulation (caveat - there
might be some nasty hack with an SMMU that wouldn't be supportable). I
requested the implementation of a generic mechanism for LPC type
emulation (complete with "SMI" traps to EL3 for fixups) about 4 years
ago. That wouldn't have helped with this situation, but this was to be
on the radar afterward. However, on ARM, it is still early days with
respect to transparently trapping and emulating hardware workarounds.

> How about using AML then? This would be similar to what CHRP used with
> RTAS calls to do PCI config space access.

The best way to do it for now (IMO) is via a DMI quirk match and a
special method for the early SoCs that aren't implementing MMCONFIG
correctly. An effort is underway to correct that in third party IP, and
similar directly with the partners for future generations. So this
should not get "much" more out of hand than it sadly is so far. Once we
have a good upstream solution (which is vital) then it will be an error
and a pre-tapeout bug to not be able to boot an upstream kernel with
stock ACPI hostbridge working sans nasty quirks. Therefore, the sooner
this is upstream, the better it will be for everyone involved.

Jon.
Tomasz Nowicki Dec. 22, 2015, 8:45 a.m. UTC | #7
On 22.12.2015 00:10, Jon Masters wrote:
> Sorry for top-posting. A quick note that SMBIOS3 is required by SBBR so it can be presumed that compliant platforms will provide quirks via DMI.
>

Thanks Jon for confirmation.

Tomasz
Gabriele Paoloni Dec. 22, 2015, 9:29 a.m. UTC | #8
Hi Jon, thanks for replying

> -----Original Message-----
> From: Jon Masters [mailto:jcm@redhat.com]
> Sent: 21 December 2015 23:11
> To: Arnd Bergmann
> Cc: Gabriele Paoloni; Tomasz Nowicki; bhelgaas@google.com;
> will.deacon@arm.com; catalin.marinas@arm.com; rjw@rjwysocki.net;
> hanjun.guo@linaro.org; Lorenzo.Pieralisi@arm.com; okaya@codeaurora.org;
> jiang.liu@linux.intel.com; Stefano.Stabellini@eu.citrix.com;
> robert.richter@caviumnetworks.com; mw@semihalf.com; Liviu.Dudau@arm.com;
> ddaney@caviumnetworks.com; tglx@linutronix.de; Wangyijing;
> Suravee.Suthikulpanit@amd.com; msalter@redhat.com; linux-
> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linaro-
> acpi@lists.linaro.org; jchandra@broadcom.com
> Subject: Re: [PATCH V2 22/23] pci, acpi: Match PCI config space
> accessors against platfrom specific quirks.
> 
> Sorry for top-posting. A quick note that SMBIOS3 is required by SBBR so
> it can be presumed that compliant platforms will provide quirks via DMI.

Ok so you completely clarified my question 1). Many Thanks for this

Gab

> 
> --
> Computer Architect | Sent from my 64-bit #ARM Powered phone
> 
> > On Dec 21, 2015, at 09:11, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Monday 21 December 2015, Gabriele Paoloni wrote:
> >>> -----Original Message-----
> >>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> >>> owner@vger.kernel.org] On Behalf Of 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 before PCI buses enumeration. Algorithm that
> overwrite
> >>> accessors matches against platform ID (DMI), domain and bus number,
> >>> hopefully enough for all cases. All quirks can be defined using:
> >>> DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
> >>
> >> I've got a couple of comments/questions about this patch..
> >>
> >> 1) So according to this mechanism quirks would be supported only by
> >>   vendors whose BIOS are SMBIOS compliant. Now personally I am ok
> >>   with this but I don't know if this is OK in general as it would
> >>   narrow down the number of platforms that would be able to define
> >>   the quirks...
> >>   Lorenzo, Arnd what is your opinion here?
> >
> > I'd rather not see the quirks in mainline at all, and only support
> > SBSA compliant machines, or require the BIOS to work around the
> > hardware quirks differently (e.g. by trapping config space access
> > through secure firmware, or going through an AML method to be
> > defined). I'm certainly ok with making it depend on SMBIOS if we are
> going to use something like this.
> >
> >    Arnd
Tomasz Nowicki Dec. 22, 2015, 10:20 a.m. UTC | #9
On 21.12.2015 12:47, Gabriele Paoloni wrote:
> 2) In the quirk mechanism you proposed, I see that the callback function
>     allows to do some preparation work for the host bridge. For example in
>     Hisilicon hip05 case we would need to read some values from the ACPI
>     table (see acpi_pci_root_hisi_add() function in
>     https://lkml.org/lkml/2015/12/3/426).
>     I am quite new to ACPI and I wonder if it is OK to add such "Packages"
>     to the  PCI host bridge ACPI device...or maybe we need to declare a new
>     one...?

I may miss sth so please correct me in that case.

https://lkml.org/lkml/2015/12/3/426 shows that you need special handling 
for root->secondary.start bus number only, right? So how about creating 
special MCFG region <rc-base:rc-base+rc-size> only for <segment,bus>. 
Like that:

[0008]                       Base Address : <rc-base>
[0002]               Segment Group Number : <segment>
[0001]                   Start Bus Number : <root->secondary.start>
[0001]                     End Bus Number : <root->secondary.start>
[0004]                           Reserved : 00000000


static const struct dmi_system_id hisi_quirk[] = {
	{
		.ident = "HiSi...",
		.matches = {
			DMI_MATCH(<whatever you need to match your platform>),
		},
	},
	{ }
};

static struct pci_ops hisi_ecam_pci_ops = {
	.map_bus = pci_mcfg_dev_base,
	.read = hisi_pcie_cfg_read,
	.write = hisi_pcie_cfg_write,
};

DECLARE_ACPI_MCFG_FIXUP(hisi_quirk, &hisi_ecam_pci_ops,
			<segment>, <bus>);

With above code you can use your custom PCI config accessor only for 
that region.

Let me know if that is not enough for you.

Tomasz
Gabriele Paoloni Dec. 22, 2015, 2:48 p.m. UTC | #10
Hi Tomasz

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Tomasz Nowicki
> Sent: 22 December 2015 10:20
> To: Gabriele Paoloni; bhelgaas@google.com; arnd@arndb.de;
> will.deacon@arm.com; catalin.marinas@arm.com; rjw@rjwysocki.net;
> hanjun.guo@linaro.org; Lorenzo.Pieralisi@arm.com; okaya@codeaurora.org;
> jiang.liu@linux.intel.com; Stefano.Stabellini@eu.citrix.com
> Cc: robert.richter@caviumnetworks.com; mw@semihalf.com;
> Liviu.Dudau@arm.com; ddaney@caviumnetworks.com; tglx@linutronix.de;
> Wangyijing; Suravee.Suthikulpanit@amd.com; msalter@redhat.com; linux-
> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linaro-
> acpi@lists.linaro.org; jchandra@broadcom.com; jcm@redhat.com
> Subject: Re: [PATCH V2 22/23] pci, acpi: Match PCI config space
> accessors against platfrom specific quirks.
> 
> On 21.12.2015 12:47, Gabriele Paoloni wrote:
> > 2) In the quirk mechanism you proposed, I see that the callback
> function
> >     allows to do some preparation work for the host bridge. For
> example in
> >     Hisilicon hip05 case we would need to read some values from the
> ACPI
> >     table (see acpi_pci_root_hisi_add() function in
> >     https://lkml.org/lkml/2015/12/3/426).
> >     I am quite new to ACPI and I wonder if it is OK to add such
> "Packages"
> >     to the  PCI host bridge ACPI device...or maybe we need to declare
> a new
> >     one...?
> 
> I may miss sth so please correct me in that case.
> 
> https://lkml.org/lkml/2015/12/3/426 shows that you need special
> handling for root->secondary.start bus number only, right? So how about
> creating special MCFG region <rc-base:rc-base+rc-size> only for
> <segment,bus>.
> Like that:
> 
> [0008]                       Base Address : <rc-base>
> [0002]               Segment Group Number : <segment>
> [0001]                   Start Bus Number : <root->secondary.start>
> [0001]                     End Bus Number : <root->secondary.start>
> [0004]                           Reserved : 00000000
> 
> 
> static const struct dmi_system_id hisi_quirk[] = {
> 	{
> 		.ident = "HiSi...",
> 		.matches = {
> 			DMI_MATCH(<whatever you need to match your platform>),
> 		},
> 	},
> 	{ }
> };
> 
> static struct pci_ops hisi_ecam_pci_ops = {
> 	.map_bus = pci_mcfg_dev_base,
> 	.read = hisi_pcie_cfg_read,
> 	.write = hisi_pcie_cfg_write,
> };
> 
> DECLARE_ACPI_MCFG_FIXUP(hisi_quirk, &hisi_ecam_pci_ops,
> 			<segment>, <bus>);
> 
> With above code you can use your custom PCI config accessor only for
> that region.
> 
> Let me know if that is not enough for you.

In principle I think it can work...

Liudongdong, Guo Hanjun what is your opinion about?

Thanks

Gab

> 
> Tomasz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
> in the body of a message to majordomo@vger.kernel.org More majordomo
> info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
Jon Masters Dec. 22, 2015, 4:36 p.m. UTC | #11
On 12/22/2015 04:29 AM, Gabriele Paoloni wrote:
> Hi Jon, thanks for replying
> 
>> -----Original Message-----
>> From: Jon Masters [mailto:jcm@redhat.com]
>> Sent: 21 December 2015 23:11
>> To: Arnd Bergmann
>> Cc: Gabriele Paoloni; Tomasz Nowicki; bhelgaas@google.com;
>> will.deacon@arm.com; catalin.marinas@arm.com; rjw@rjwysocki.net;
>> hanjun.guo@linaro.org; Lorenzo.Pieralisi@arm.com; okaya@codeaurora.org;
>> jiang.liu@linux.intel.com; Stefano.Stabellini@eu.citrix.com;
>> robert.richter@caviumnetworks.com; mw@semihalf.com; Liviu.Dudau@arm.com;
>> ddaney@caviumnetworks.com; tglx@linutronix.de; Wangyijing;
>> Suravee.Suthikulpanit@amd.com; msalter@redhat.com; linux-
>> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linaro-
>> acpi@lists.linaro.org; jchandra@broadcom.com
>> Subject: Re: [PATCH V2 22/23] pci, acpi: Match PCI config space
>> accessors against platfrom specific quirks.
>>
>> Sorry for top-posting. A quick note that SMBIOS3 is required by SBBR so
>> it can be presumed that compliant platforms will provide quirks via DMI.
> 
> Ok so you completely clarified my question 1). Many Thanks for this

No problem. One of the (many) reasons I pushed for requiring SMBIOS/DMI
in SBBR (I was lead author of one of the early drafts of that document)
was to make the experience ultimately equivalent across architectures.
We already know how to do quirks and handle platform deviations, and the
major Operating System vendors did not want to reinvent the wheel.

Jon.
Jon Masters Dec. 22, 2015, 4:45 p.m. UTC | #12
On 12/22/2015 11:36 AM, Jon Masters wrote:
> On 12/22/2015 04:29 AM, Gabriele Paoloni wrote:
>> Hi Jon, thanks for replying
>>
>>> -----Original Message-----
>>> From: Jon Masters [mailto:jcm@redhat.com]
>>> Sent: 21 December 2015 23:11
>>> To: Arnd Bergmann
>>> Cc: Gabriele Paoloni; Tomasz Nowicki; bhelgaas@google.com;
>>> will.deacon@arm.com; catalin.marinas@arm.com; rjw@rjwysocki.net;
>>> hanjun.guo@linaro.org; Lorenzo.Pieralisi@arm.com; okaya@codeaurora.org;
>>> jiang.liu@linux.intel.com; Stefano.Stabellini@eu.citrix.com;
>>> robert.richter@caviumnetworks.com; mw@semihalf.com; Liviu.Dudau@arm.com;
>>> ddaney@caviumnetworks.com; tglx@linutronix.de; Wangyijing;
>>> Suravee.Suthikulpanit@amd.com; msalter@redhat.com; linux-
>>> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>>> acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linaro-
>>> acpi@lists.linaro.org; jchandra@broadcom.com
>>> Subject: Re: [PATCH V2 22/23] pci, acpi: Match PCI config space
>>> accessors against platfrom specific quirks.
>>>
>>> Sorry for top-posting. A quick note that SMBIOS3 is required by SBBR so
>>> it can be presumed that compliant platforms will provide quirks via DMI.
>>
>> Ok so you completely clarified my question 1). Many Thanks for this
> 
> No problem. One of the (many) reasons I pushed for requiring SMBIOS/DMI
> in SBBR (I was lead author of one of the early drafts of that document)
> was to make the experience ultimately equivalent across architectures.
> We already know how to do quirks and handle platform deviations, and the
> major Operating System vendors did not want to reinvent the wheel.

Additional: it is clear that more prescription is required to get the
vendors onto the bandwagon that we have with other architectures (e.g.
that other one). So there will be a Red Hat "ARM server whitepaper"
coming in the early new year that will include the kind of "server 101"
material we want to make sure people know. Things like making sure you
implement and test PCIe correctly, handle backward compatibility (you
will build hardware in the future that runs my existing OS release),
design the hardware to allow for workarounds later, etc. I expect some
other Operating System vendors to be involved in reviewing that.

Ultimately my objective is to make this whole thing dull and boring. You
will get RHEL(SA)/upstream kernels and it will either boot or it will
not. If it does not boot, vendor X screwed up their hardware. We know
this story, it's been this way for over a decade already, and that is
exactly how it is going to be with ARM servers shortly.

Jon.
Gabriele Paoloni Dec. 22, 2015, 5:49 p.m. UTC | #13
> -----Original Message-----
> From: Jon Masters [mailto:jcm@redhat.com]
> Sent: 22 December 2015 16:45
> To: Gabriele Paoloni; Arnd Bergmann
> Cc: Tomasz Nowicki; bhelgaas@google.com; will.deacon@arm.com;
> catalin.marinas@arm.com; rjw@rjwysocki.net; hanjun.guo@linaro.org;
> Lorenzo.Pieralisi@arm.com; okaya@codeaurora.org;
> jiang.liu@linux.intel.com; Stefano.Stabellini@eu.citrix.com;
> robert.richter@caviumnetworks.com; mw@semihalf.com; Liviu.Dudau@arm.com;
> ddaney@caviumnetworks.com; tglx@linutronix.de; Wangyijing;
> Suravee.Suthikulpanit@amd.com; msalter@redhat.com; linux-
> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linaro-
> acpi@lists.linaro.org; jchandra@broadcom.com
> Subject: Re: [PATCH V2 22/23] pci, acpi: Match PCI config space
> accessors against platfrom specific quirks.
> 
> On 12/22/2015 11:36 AM, Jon Masters wrote:
> > On 12/22/2015 04:29 AM, Gabriele Paoloni wrote:
> >> Hi Jon, thanks for replying
> >>
> >>> -----Original Message-----
> >>> From: Jon Masters [mailto:jcm@redhat.com]
> >>> Sent: 21 December 2015 23:11
> >>> To: Arnd Bergmann
> >>> Cc: Gabriele Paoloni; Tomasz Nowicki; bhelgaas@google.com;
> >>> will.deacon@arm.com; catalin.marinas@arm.com; rjw@rjwysocki.net;
> >>> hanjun.guo@linaro.org; Lorenzo.Pieralisi@arm.com;
> >>> okaya@codeaurora.org; jiang.liu@linux.intel.com;
> >>> Stefano.Stabellini@eu.citrix.com; robert.richter@caviumnetworks.com;
> >>> mw@semihalf.com; Liviu.Dudau@arm.com; ddaney@caviumnetworks.com;
> >>> tglx@linutronix.de; Wangyijing; Suravee.Suthikulpanit@amd.com;
> >>> msalter@redhat.com; linux- pci@vger.kernel.org;
> >>> linux-arm-kernel@lists.infradead.org; linux- acpi@vger.kernel.org;
> >>> linux-kernel@vger.kernel.org; linaro- acpi@lists.linaro.org;
> >>> jchandra@broadcom.com
> >>> Subject: Re: [PATCH V2 22/23] pci, acpi: Match PCI config space
> >>> accessors against platfrom specific quirks.
> >>>
> >>> Sorry for top-posting. A quick note that SMBIOS3 is required by
> SBBR
> >>> so it can be presumed that compliant platforms will provide quirks
> via DMI.
> >>
> >> Ok so you completely clarified my question 1). Many Thanks for this
> >
> > No problem. One of the (many) reasons I pushed for requiring
> > SMBIOS/DMI in SBBR (I was lead author of one of the early drafts of
> > that document) was to make the experience ultimately equivalent
> across architectures.
> > We already know how to do quirks and handle platform deviations, and
> > the major Operating System vendors did not want to reinvent the wheel.
> 
> Additional: it is clear that more prescription is required to get the
> vendors onto the bandwagon that we have with other architectures (e.g.
> that other one). So there will be a Red Hat "ARM server whitepaper"
> coming in the early new year that will include the kind of "server 101"
> material we want to make sure people know. Things like making sure you
> implement and test PCIe correctly, handle backward compatibility (you
> will build hardware in the future that runs my existing OS release),
> design the hardware to allow for workarounds later, etc. I expect some
> other Operating System vendors to be involved in reviewing that.
> 
> Ultimately my objective is to make this whole thing dull and boring.
> You will get RHEL(SA)/upstream kernels and it will either boot or it
> will not. If it does not boot, vendor X screwed up their hardware. We
> know this story, it's been this way for over a decade already, and that
> is exactly how it is going to be with ARM servers shortly.

Ok got it.

Many Thanks

Gab

> 
> Jon.
Hanjun Guo Dec. 23, 2015, 9:38 a.m. UTC | #14
On 12/22/2015 10:48 PM, Gabriele Paoloni wrote:
> Hi Tomasz
>
>> -----Original Message-----
>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>> owner@vger.kernel.org] On Behalf Of Tomasz Nowicki
>> Sent: 22 December 2015 10:20
>> To: Gabriele Paoloni; bhelgaas@google.com; arnd@arndb.de;
>> will.deacon@arm.com; catalin.marinas@arm.com; rjw@rjwysocki.net;
>> hanjun.guo@linaro.org; Lorenzo.Pieralisi@arm.com; okaya@codeaurora.org;
>> jiang.liu@linux.intel.com; Stefano.Stabellini@eu.citrix.com
>> Cc: robert.richter@caviumnetworks.com; mw@semihalf.com;
>> Liviu.Dudau@arm.com; ddaney@caviumnetworks.com; tglx@linutronix.de;
>> Wangyijing; Suravee.Suthikulpanit@amd.com; msalter@redhat.com; linux-
>> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linaro-
>> acpi@lists.linaro.org; jchandra@broadcom.com; jcm@redhat.com
>> Subject: Re: [PATCH V2 22/23] pci, acpi: Match PCI config space
>> accessors against platfrom specific quirks.
>>
>> On 21.12.2015 12:47, Gabriele Paoloni wrote:
>>> 2) In the quirk mechanism you proposed, I see that the callback
>> function
>>>      allows to do some preparation work for the host bridge. For
>> example in
>>>      Hisilicon hip05 case we would need to read some values from the
>> ACPI
>>>      table (see acpi_pci_root_hisi_add() function in
>>>      https://lkml.org/lkml/2015/12/3/426).
>>>      I am quite new to ACPI and I wonder if it is OK to add such
>> "Packages"
>>>      to the  PCI host bridge ACPI device...or maybe we need to declare
>> a new
>>>      one...?
>>
>> I may miss sth so please correct me in that case.
>>
>> https://lkml.org/lkml/2015/12/3/426 shows that you need special
>> handling for root->secondary.start bus number only, right? So how about
>> creating special MCFG region <rc-base:rc-base+rc-size> only for
>> <segment,bus>.
>> Like that:
>>
>> [0008]                       Base Address : <rc-base>
>> [0002]               Segment Group Number : <segment>
>> [0001]                   Start Bus Number : <root->secondary.start>
>> [0001]                     End Bus Number : <root->secondary.start>
>> [0004]                           Reserved : 00000000
>>
>>
>> static const struct dmi_system_id hisi_quirk[] = {
>> 	{
>> 		.ident = "HiSi...",
>> 		.matches = {
>> 			DMI_MATCH(<whatever you need to match your platform>),
>> 		},
>> 	},
>> 	{ }
>> };
>>
>> static struct pci_ops hisi_ecam_pci_ops = {
>> 	.map_bus = pci_mcfg_dev_base,
>> 	.read = hisi_pcie_cfg_read,
>> 	.write = hisi_pcie_cfg_write,
>> };
>>
>> DECLARE_ACPI_MCFG_FIXUP(hisi_quirk, &hisi_ecam_pci_ops,
>> 			<segment>, <bus>);
>>
>> With above code you can use your custom PCI config accessor only for
>> that region.
>>
>> Let me know if that is not enough for you.
>
> In principle I think it can work...
>
> Liudongdong, Guo Hanjun what is your opinion about?

Let me and Dongdong prepare a patch for Hip05 and then will
back to this discussion to see if we met some problems.

Thanks
Hanjun
Mark Salter Jan. 8, 2016, 2:16 p.m. UTC | #15
On Wed, 2015-12-16 at 16:16 +0100, 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 before PCI buses enumeration. Algorithm that overwrite accessors
> matches against platform ID (DMI), domain and bus number, hopefully
> enough for all cases. All quirks can be defined using:
> DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
> 
> example:
> 
> static const struct dmi_system_id yyy[] = {
>         {
>                 .ident = "<Platform ident string>",
>                 .callback = <handler>,
>                 .matches = {
>                         DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
>                         DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
>                         DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
>                 },
>         },
>         { }
> };
> 

This seems awkward to me in the case where the quirk is SoC-based and there
may be multiple platforms affected. Needing a DECLARE_ACPI_MCFG_FIXUP for
each platform using such a SoC (i.e. Mustang and Moonshot) doesn't seem
right. In that case, I think it'd be better to check CPUID and possibly
some SoC register to cover all platforms affected.

Also, there doesn't seem to be a way to connect a given quirk check to the
MCFG/device requesting the ops. So if there is a platform with multiple PCIE
roots and not all of them have quirks, how does one no whether to override the
default ecam ops?
Tomasz Nowicki Jan. 8, 2016, 2:36 p.m. UTC | #16
On 08.01.2016 15:16, Mark Salter wrote:
> On Wed, 2015-12-16 at 16:16 +0100, 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 before PCI buses enumeration. Algorithm that overwrite accessors
>> matches against platform ID (DMI), domain and bus number, hopefully
>> enough for all cases. All quirks can be defined using:
>> DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
>>
>> example:
>>
>> static const struct dmi_system_id yyy[] = {
>>          {
>>                  .ident = "<Platform ident string>",
>>                  .callback = <handler>,
>>                  .matches = {
>>                          DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
>>                          DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
>>                          DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
>>                  },
>>          },
>>          { }
>> };
>>
>
> This seems awkward to me in the case where the quirk is SoC-based and there
> may be multiple platforms affected. Needing a DECLARE_ACPI_MCFG_FIXUP for
> each platform using such a SoC (i.e. Mustang and Moonshot) doesn't seem
> right. In that case, I think it'd be better to check CPUID and possibly
> some SoC register to cover all platforms affected.

Right, my next version already has alternative to DMI match handler, so 
there will be two ways to match:
1. DMI, like in this patch set
2. int (*match)(struct pci_mcfg_fixup *) where you can read CPUID, and 
whatever is necessary.

>
> Also, there doesn't seem to be a way to connect a given quirk check to the
> MCFG/device requesting the ops. So if there is a platform with multiple PCIE
> roots and not all of them have quirks, how does one no whether to override the
> default ecam ops?
>

Then we can identify them using <domain:bus>. I was wondering to pass 
acpi device handler to match handler for the case where we need e.g. 
extra properties from related DSDT device descriptor. Does it make sense 
to you?

Tomasz
Jeremy Linton Jan. 8, 2016, 2:42 p.m. UTC | #17
On 01/08/2016 08:16 AM, Mark Salter wrote:
> On Wed, 2015-12-16 at 16:16 +0100, 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 before PCI buses enumeration. Algorithm that overwrite accessors
>> matches against platform ID (DMI), domain and bus number, hopefully
>> enough for all cases. All quirks can be defined using:
>> DECLARE_ACPI_MCFG_FIXUP() and keep self contained.

...

> This seems awkward to me in the case where the quirk is SoC-based and there
> may be multiple platforms affected. Needing a DECLARE_ACPI_MCFG_FIXUP for
> each platform using such a SoC (i.e. Mustang and Moonshot) doesn't seem
> right. In that case, I think it'd be better to check CPUID and possibly
> some SoC register to cover all platforms affected.
>
> Also, there doesn't seem to be a way to connect a given quirk check to the
> MCFG/device requesting the ops. So if there is a platform with multiple PCIE
> roots and not all of them have quirks, how does one no whether to override the
> default ecam ops?

That was the thinking that lead me to quirk the xgene based m400 using
the root bridge VID/DID and DECLARE_PCI_FIXUP_EARLY(). A solution which
works because the default config space accessors work sufficiently to
read the VID/DID.

I think that solution might work for at least one other vendor as well,
and IMHO is better than using DMI for the reasons you list.

I will rebase/post those patches RSN...
















(please ignore the message below, i'm working getting that taken care of
as well)



IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Mark Salter Jan. 8, 2016, 2:51 p.m. UTC | #18
On Fri, 2016-01-08 at 15:36 +0100, Tomasz Nowicki wrote:
> On 08.01.2016 15:16, Mark Salter wrote:
> > On Wed, 2015-12-16 at 16:16 +0100, 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 before PCI buses enumeration. Algorithm that overwrite accessors
> > > matches against platform ID (DMI), domain and bus number, hopefully
> > > enough for all cases. All quirks can be defined using:
> > > DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
> > > 
> > > example:
> > > 
> > > static const struct dmi_system_id yyy[] = {
> > >          {
> > >                  .ident = "<Platform ident string>",
> > >                  .callback = <handler>,
> > >                  .matches = {
> > >                          DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
> > >                          DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
> > >                          DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
> > >                  },
> > >          },
> > >          { }
> > > };
> > > 
> > 
> > This seems awkward to me in the case where the quirk is SoC-based and there
> > may be multiple platforms affected. Needing a DECLARE_ACPI_MCFG_FIXUP for
> > each platform using such a SoC (i.e. Mustang and Moonshot) doesn't seem
> > right. In that case, I think it'd be better to check CPUID and possibly
> > some SoC register to cover all platforms affected.
> 
> Right, my next version already has alternative to DMI match handler, so 
> there will be two ways to match:
> 1. DMI, like in this patch set
> 2. int (*match)(struct pci_mcfg_fixup *) where you can read CPUID, and 
> whatever is necessary.
> 

Great. Thanks.

> > 
> > Also, there doesn't seem to be a way to connect a given quirk check to the
> > MCFG/device requesting the ops. So if there is a platform with multiple PCIE
> > roots and not all of them have quirks, how does one no whether to override the
> > default ecam ops?
> > 
> 
> Then we can identify them using <domain:bus>. I was wondering to pass 
> acpi device handler to match handler for the case where we need e.g. 
> extra properties from related DSDT device descriptor. Does it make sense 
> to you?
> 

The thing with domain:bus is that it is really a firmware setting. So on one
platform domain 0 may be associated with hwdev X but on another platform it
may be associated with hwdev Y. But yes, an acpi handle would make it much
easier for the match handler to find out which hw dev it is being asked to
match.
Mark Rutland Jan. 8, 2016, 3:01 p.m. UTC | #19
On Fri, Jan 08, 2016 at 09:16:21AM -0500, Mark Salter wrote:
> On Wed, 2015-12-16 at 16:16 +0100, 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 before PCI buses enumeration. Algorithm that overwrite accessors
> > matches against platform ID (DMI), domain and bus number, hopefully
> > enough for all cases. All quirks can be defined using:
> > DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
> > 
> > example:
> > 
> > static const struct dmi_system_id yyy[] = {
> >         {
> >                 .ident = "<Platform ident string>",
> >                 .callback = <handler>,
> >                 .matches = {
> >                         DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
> >                         DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
> >                         DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
> >                 },
> >         },
> >         { }
> > };
> > 
> 
> This seems awkward to me in the case where the quirk is SoC-based and there
> may be multiple platforms affected. Needing a DECLARE_ACPI_MCFG_FIXUP for
> each platform using such a SoC (i.e. Mustang and Moonshot) doesn't seem
> right. In that case, I think it'd be better to check CPUID and possibly
> some SoC register to cover all platforms affected.

CPUs get reused across SoCs, so as you've implicitly noted, the CPUID
alone is insufficient.

Given that IP blocks get moved around between SoC variants, I don't
think you can check "some SoC register" based on the CPU ID -- you can
end up bringing the board down at that point.

If the CPU ID alone is insufficient to tell you about a component, it
cannot give you enough information about a component you can use to
query more information from.

If your platform requires a quirk, it's always going to be painful (and
to some extent, rightfulyl so). We should aim for correctness here with
explicit matching.

Thanks,
Mark.
Mark Rutland Jan. 8, 2016, 3:12 p.m. UTC | #20
On Fri, Jan 08, 2016 at 03:01:37PM +0000, Mark Rutland wrote:
> On Fri, Jan 08, 2016 at 09:16:21AM -0500, Mark Salter wrote:
> > On Wed, 2015-12-16 at 16:16 +0100, 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 before PCI buses enumeration. Algorithm that overwrite accessors
> > > matches against platform ID (DMI), domain and bus number, hopefully
> > > enough for all cases. All quirks can be defined using:
> > > DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
> > > 
> > > example:
> > > 
> > > static const struct dmi_system_id yyy[] = {
> > >         {
> > >                 .ident = "<Platform ident string>",
> > >                 .callback = <handler>,
> > >                 .matches = {
> > >                         DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
> > >                         DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
> > >                         DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
> > >                 },
> > >         },
> > >         { }
> > > };
> > > 
> > 
> > This seems awkward to me in the case where the quirk is SoC-based and there
> > may be multiple platforms affected. Needing a DECLARE_ACPI_MCFG_FIXUP for
> > each platform using such a SoC (i.e. Mustang and Moonshot) doesn't seem
> > right. In that case, I think it'd be better to check CPUID and possibly
> > some SoC register to cover all platforms affected.
> 
> CPUs get reused across SoCs, so as you've implicitly noted, the CPUID
> alone is insufficient.
> 
> Given that IP blocks get moved around between SoC variants, I don't
> think you can check "some SoC register" based on the CPU ID -- you can
> end up bringing the board down at that point.
> 
> If the CPU ID alone is insufficient to tell you about a component, it
> cannot give you enough information about a component you can use to
> query more information from.
> 
> If your platform requires a quirk, it's always going to be painful (and
> to some extent, rightfulyl so). We should aim for correctness here with
> explicit matching.

Further, if there is going to be an ever-expanding set of platforms
requring quirks, then we need a standard mechanism in ACPI to enable the
platform to tell us explicitly either which specific PCI implementation
is used, or which common quirk is necessary.

Thanks,
Mark.
Mark Salter Jan. 8, 2016, 4:07 p.m. UTC | #21
On Fri, 2016-01-08 at 15:12 +0000, Mark Rutland wrote:
> On Fri, Jan 08, 2016 at 03:01:37PM +0000, Mark Rutland wrote:
> > On Fri, Jan 08, 2016 at 09:16:21AM -0500, Mark Salter wrote:
> > > On Wed, 2015-12-16 at 16:16 +0100, 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 before PCI buses enumeration. Algorithm that overwrite accessors
> > > > matches against platform ID (DMI), domain and bus number, hopefully
> > > > enough for all cases. All quirks can be defined using:
> > > > DECLARE_ACPI_MCFG_FIXUP() and keep self contained.
> > > > 
> > > > example:
> > > > 
> > > > static const struct dmi_system_id yyy[] = {
> > > >         {
> > > >                 .ident = "<Platform ident string>",
> > > >                 .callback = <handler>,
> > > >                 .matches = {
> > > >                         DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
> > > >                         DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
> > > >                         DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
> > > >                 },
> > > >         },
> > > >         { }
> > > > };
> > > > 
> > > 
> > > This seems awkward to me in the case where the quirk is SoC-based and there
> > > may be multiple platforms affected. Needing a DECLARE_ACPI_MCFG_FIXUP for
> > > each platform using such a SoC (i.e. Mustang and Moonshot) doesn't seem
> > > right. In that case, I think it'd be better to check CPUID and possibly
> > > some SoC register to cover all platforms affected.
> > 
> > CPUs get reused across SoCs, so as you've implicitly noted, the CPUID
> > alone is insufficient.
> > 
> > Given that IP blocks get moved around between SoC variants, I don't
> > think you can check "some SoC register" based on the CPU ID -- you can
> > end up bringing the board down at that point.
> > 
> > If the CPU ID alone is insufficient to tell you about a component, it
> > cannot give you enough information about a component you can use to
> > query more information from.
> > 
> > If your platform requires a quirk, it's always going to be painful (and
> > to some extent, rightfulyl so). We should aim for correctness here with
> > explicit matching.
> 
> Further, if there is going to be an ever-expanding set of platforms
> requring quirks, then we need a standard mechanism in ACPI to enable the
> platform to tell us explicitly either which specific PCI implementation
> is used, or which common quirk is necessary.
> 

No, an ever-expanding set is exactly what we don't want. I think you've convinced
me that I'm taking a wrong view of the problem. Putting something in the ACPI
standard would be going too far and I think a hard sell to the standards folk.
There really is no foolproof way to match a plug and play ACPI PCIe root to
specific hardware without considering the exact platform and/or BIOS info. So
yeah, it should be painful in order to give incentive to the silicon vendors to
get it right.
diff mbox

Patch

diff --git a/drivers/acpi/mcfg.c b/drivers/acpi/mcfg.c
index a9b2231..6d0194d 100644
--- a/drivers/acpi/mcfg.c
+++ b/drivers/acpi/mcfg.c
@@ -8,6 +8,7 @@ 
  */
 
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <linux/ecam.h>
 #include <linux/pci.h>
 #include <linux/pci-acpi.h>
@@ -34,6 +35,31 @@  int __weak raw_pci_write(unsigned int domain, unsigned int bus,
 	return PCIBIOS_DEVICE_NOT_FOUND;
 }
 
+extern struct pci_mcfg_fixup __start_acpi_mcfg_fixups[];
+extern struct pci_mcfg_fixup __end_acpi_mcfg_fixups[];
+
+static struct pci_ops *pci_mcfg_check_quirks(int domain, int bus_number)
+{
+	struct pci_mcfg_fixup *fixup;
+
+	fixup = __start_acpi_mcfg_fixups;
+	while (fixup < __end_acpi_mcfg_fixups) {
+		if (dmi_check_system(fixup->system) &&
+		    (fixup->domain == domain ||
+		     fixup->domain == PCI_MCFG_DOMAIN_ANY) &&
+		    (fixup->bus_number == bus_number ||
+		     fixup->bus_number == PCI_MCFG_BUS_ANY)) {
+			pr_info(PREFIX "Fixup applied: Platform [%s] domain [%d] bus number [%d]\n",
+				fixup->system->ident, fixup->domain,
+				fixup->bus_number);
+			return fixup->ops;
+		}
+		++fixup;
+	}
+
+	return NULL;
+}
+
 void __iomem *
 pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
 {
@@ -56,10 +82,15 @@  static struct pci_ops default_pci_mcfg_ops = {
 
 struct pci_ops *pci_mcfg_get_ops(int domain, int bus)
 {
+	struct pci_ops *pci_mcfg_ops_quirk;
+
 	/*
-	 * TODO: Match against platform specific quirks and return
-	 * corresponding PCI config space accessor set.
+	 * Match against platform specific quirks and return corresponding
+	 * PCI config space accessor set.
 	 */
+	pci_mcfg_ops_quirk = pci_mcfg_check_quirks(domain, bus);
+	if (pci_mcfg_ops_quirk)
+		return pci_mcfg_ops_quirk;
 
 	return &default_pci_mcfg_ops;
 }
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index c4bd0e2..c93fc97 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -298,6 +298,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/ecam.h b/include/linux/ecam.h
index e0f322e..1319fa8 100644
--- a/include/linux/ecam.h
+++ b/include/linux/ecam.h
@@ -20,6 +20,23 @@  struct pci_mmcfg_region {
 	bool hot_added;
 };
 
+struct pci_mcfg_fixup {
+	const struct dmi_system_id *system;
+	struct pci_ops *ops;
+	int domain;
+	int bus_number;
+};
+
+#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(system, ops, dom, bus)			\
+	static const struct pci_mcfg_fixup __mcfg_fixup_##system##dom##bus\
+	 __used	__attribute__((__section__(".acpi_fixup_mcfg"),		\
+				aligned((sizeof(void *))))) =		\
+	{ system, ops, dom, bus };
+
 struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus);
 struct pci_mmcfg_region *pci_mmconfig_alloc(int segment, int start,
 						   int end, u64 addr);