diff mbox

[V6,0/5] ECAM quirks handling for ARM64 platforms

Message ID 20160922230806.GB1514@localhost (mailing list archive)
State New, archived
Headers show

Commit Message

Bjorn Helgaas Sept. 22, 2016, 11:08 p.m. UTC
On Wed, Sep 21, 2016 at 06:40:47PM -0400, Christopher Covington wrote:
> Hi Bjorn,
> 
> On 09/21/2016 09:11 AM, Bjorn Helgaas wrote:
> > On Tue, Sep 20, 2016 at 09:15:14PM -0400, cov@codeaurora.org wrote:
> 
> >>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> >>> index eb14f74..bb3b8ad 100644
> >>> --- a/drivers/acpi/pci_mcfg.c
> >>> +++ b/drivers/acpi/pci_mcfg.c
> >>> @@ -42,86 +42,59 @@ struct mcfg_fixup {
> >>> 	struct resource cfgres;
> >>> };
> >>>
> >>> -#define MCFG_DOM_ANY			(-1)
> >>
> >> Did you delete this because there were no current users, because you'd
> >> prefer users just use "-1", or for some other reason?
> > 
> > I removed it because there were no users of it and, more importantly,
> > the code doesn't implement support for it.
> 
> It looks like a stale "First match against PCI topology <domain:bus>..."
> comment remains.

Yep.  I removed the comment since it's sort of obvious from the code.
I also renamed a few things and pulled the match out into a helper
function.

I also changed the dmesg note: I think the actual resource and the
name of the pci_ecam_ops is more interesting than the table IDs (which
I think are already elsewhere in the dmesg log).

Here's the incremental diff, which I can't really test:

Comments

Christopher Covington Sept. 23, 2016, 6:41 p.m. UTC | #1
On 09/22/2016 07:08 PM, Bjorn Helgaas wrote:
> On Wed, Sep 21, 2016 at 06:40:47PM -0400, Christopher Covington wrote:
>> Hi Bjorn,
>>
>> On 09/21/2016 09:11 AM, Bjorn Helgaas wrote:
>>> On Tue, Sep 20, 2016 at 09:15:14PM -0400, cov@codeaurora.org wrote:
>>
>>>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>>>>> index eb14f74..bb3b8ad 100644
>>>>> --- a/drivers/acpi/pci_mcfg.c
>>>>> +++ b/drivers/acpi/pci_mcfg.c
>>>>> @@ -42,86 +42,59 @@ struct mcfg_fixup {
>>>>> 	struct resource cfgres;
>>>>> };
>>>>>
>>>>> -#define MCFG_DOM_ANY			(-1)
>>>>
>>>> Did you delete this because there were no current users, because you'd
>>>> prefer users just use "-1", or for some other reason?
>>>
>>> I removed it because there were no users of it and, more importantly,
>>> the code doesn't implement support for it.
>>
>> It looks like a stale "First match against PCI topology <domain:bus>..."
>> comment remains.
> 
> Yep.  I removed the comment since it's sort of obvious from the code.
> I also renamed a few things and pulled the match out into a helper
> function.
> 
> I also changed the dmesg note: I think the actual resource and the
> name of the pci_ecam_ops is more interesting than the table IDs (which
> I think are already elsewhere in the dmesg log).

It looks like the resource is already being printed from
drivers/pci/ecam.c:102.

> Here's the incremental diff, which I can't really test:

Here's what it looks like for me:

ACPI: PCI Root Bridge [PCI2] (domain 0002 [bus 00-1f])
acpi PNP0A08:02: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
acpi PNP0A08:02: _OSC: platform does not support [PCIeHotplug]
acpi PNP0A08:02: _OSC: OS now controls [PME AER PCIeCapability]
acpi PNP0A08:02: MCFG quirk: ECAM space for [bus 00-1f] at [mem 0xa0000000000-0xa0001ffffff] with pci_3
acpi PNP0A08:02: ECAM at [mem 0xa0000000000-0xa0001ffffff] for [bus 00-1f]
Remapped I/O 0x00000affffff0000 to [io  0x10000-0x1ffff window]
PCI host bridge to bus 0002:00

> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index 245b79f..0b36bc5 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -36,7 +36,7 @@ struct mcfg_fixup {
>  	char oem_id[ACPI_OEM_ID_SIZE + 1];
>  	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
>  	u32 oem_revision;
> -	u16 seg;
> +	u16 segment;
>  	struct resource bus_range;
>  	struct pci_ecam_ops *ops;
>  	struct resource cfgres;
> @@ -102,30 +102,37 @@ static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
>  static char mcfg_oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
>  static u32 mcfg_oem_revision;
>  
> -static void pci_mcfg_match_quirks(struct acpi_pci_root *root,
> +static int pci_mcfg_quirk_matches(struct mcfg_fixup *f, u16 segment,
> +				  struct resource *bus_range)
> +{
> +	if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) &&
> +	    !memcmp(f->oem_table_id, mcfg_oem_table_id,
> +	            ACPI_OEM_TABLE_ID_SIZE) &&
> +	    f->oem_revision == mcfg_oem_revision &&
> +	    f->segment == segment &&
> +	    resource_contains(&f->bus_range, bus_range))
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
>  				  struct resource *cfgres,
>  				  struct pci_ecam_ops **ecam_ops)
>  {
> +	u16 segment = root->segment;
> +	struct resource *bus_range = &root->secondary;
>  	struct mcfg_fixup *f;
>  	int i;
>  
> -	/*
> -	 * First match against PCI topology <domain:bus> then use OEM ID, OEM
> -	 * table ID, and OEM revision from MCFG table standard header.
> -	 */
>  	for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
> -		if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) &&
> -		    !memcmp(f->oem_table_id, mcfg_oem_table_id,
> -		            ACPI_OEM_TABLE_ID_SIZE) &&
> -		    f->oem_revision == mcfg_oem_revision &&
> -		    f->seg == root->segment &&
> -		    resource_contains(&f->bus_range, &root->secondary)) {
> +		if (pci_mcfg_quirk_matches(f, segment, bus_range)) {
>  			if (f->cfgres.start)
>  				*cfgres = f->cfgres;
>  			if (f->ops)
>  				*ecam_ops =  f->ops;
> -			dev_info(&root->device->dev, "Applying PCI MCFG quirks for %s %s rev: %d\n",
> -				 f->oem_id, f->oem_table_id, f->oem_revision);
> +			dev_info(&root->device->dev, "MCFG quirk: ECAM space for %pR at %pR with %ps\n",
> +				 bus_range, cfgres, *ecam_ops);
>  			return;
>  		}
>  	}
> @@ -173,7 +180,7 @@ skip_lookup:
>  	 * MCFG does not have it.  Invalid CFG start address means MCFG
>  	 * firmware bug or we need another quirk in array.
>  	 */
> -	pci_mcfg_match_quirks(root, &res, &ops);
> +	pci_mcfg_apply_quirks(root, &res, &ops);
>  	if (!res.start)
>  		return -ENXIO;
>  
>
Bjorn Helgaas Sept. 23, 2016, 7:17 p.m. UTC | #2
On Fri, Sep 23, 2016 at 02:41:39PM -0400, Christopher Covington wrote:
> On 09/22/2016 07:08 PM, Bjorn Helgaas wrote:
> > On Wed, Sep 21, 2016 at 06:40:47PM -0400, Christopher Covington wrote:
> >> Hi Bjorn,
> >>
> >> On 09/21/2016 09:11 AM, Bjorn Helgaas wrote:
> >>> On Tue, Sep 20, 2016 at 09:15:14PM -0400, cov@codeaurora.org wrote:
> >>
> >>>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> >>>>> index eb14f74..bb3b8ad 100644
> >>>>> --- a/drivers/acpi/pci_mcfg.c
> >>>>> +++ b/drivers/acpi/pci_mcfg.c
> >>>>> @@ -42,86 +42,59 @@ struct mcfg_fixup {
> >>>>> 	struct resource cfgres;
> >>>>> };
> >>>>>
> >>>>> -#define MCFG_DOM_ANY			(-1)
> >>>>
> >>>> Did you delete this because there were no current users, because you'd
> >>>> prefer users just use "-1", or for some other reason?
> >>>
> >>> I removed it because there were no users of it and, more importantly,
> >>> the code doesn't implement support for it.
> >>
> >> It looks like a stale "First match against PCI topology <domain:bus>..."
> >> comment remains.
> > 
> > Yep.  I removed the comment since it's sort of obvious from the code.
> > I also renamed a few things and pulled the match out into a helper
> > function.
> > 
> > I also changed the dmesg note: I think the actual resource and the
> > name of the pci_ecam_ops is more interesting than the table IDs (which
> > I think are already elsewhere in the dmesg log).
> 
> It looks like the resource is already being printed from
> drivers/pci/ecam.c:102.

Yes, but I want a hint that a quirk has overridden it because that's a
clue that there's something wonky about the platform or the firmware.

But I guess it'd be nice to mirror the format of the existing info
(mem first, then bus range).

> > Here's the incremental diff, which I can't really test:
> 
> Here's what it looks like for me:
> 
> ACPI: PCI Root Bridge [PCI2] (domain 0002 [bus 00-1f])
> acpi PNP0A08:02: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
> acpi PNP0A08:02: _OSC: platform does not support [PCIeHotplug]
> acpi PNP0A08:02: _OSC: OS now controls [PME AER PCIeCapability]
> acpi PNP0A08:02: MCFG quirk: ECAM space for [bus 00-1f] at [mem 0xa0000000000-0xa0001ffffff] with pci_3

Is "pci_3" really the entire name?  If not, what happened to the rest?
I was hoping for a symbol we could grep for.

> acpi PNP0A08:02: ECAM at [mem 0xa0000000000-0xa0001ffffff] for [bus 00-1f]
> Remapped I/O 0x00000affffff0000 to [io  0x10000-0x1ffff window]
> PCI host bridge to bus 0002:00
> 
> > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> > index 245b79f..0b36bc5 100644
> > --- a/drivers/acpi/pci_mcfg.c
> > +++ b/drivers/acpi/pci_mcfg.c
> > @@ -36,7 +36,7 @@ struct mcfg_fixup {
> >  	char oem_id[ACPI_OEM_ID_SIZE + 1];
> >  	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> >  	u32 oem_revision;
> > -	u16 seg;
> > +	u16 segment;
> >  	struct resource bus_range;
> >  	struct pci_ecam_ops *ops;
> >  	struct resource cfgres;
> > @@ -102,30 +102,37 @@ static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
> >  static char mcfg_oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
> >  static u32 mcfg_oem_revision;
> >  
> > -static void pci_mcfg_match_quirks(struct acpi_pci_root *root,
> > +static int pci_mcfg_quirk_matches(struct mcfg_fixup *f, u16 segment,
> > +				  struct resource *bus_range)
> > +{
> > +	if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) &&
> > +	    !memcmp(f->oem_table_id, mcfg_oem_table_id,
> > +	            ACPI_OEM_TABLE_ID_SIZE) &&
> > +	    f->oem_revision == mcfg_oem_revision &&
> > +	    f->segment == segment &&
> > +	    resource_contains(&f->bus_range, bus_range))
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
> >  				  struct resource *cfgres,
> >  				  struct pci_ecam_ops **ecam_ops)
> >  {
> > +	u16 segment = root->segment;
> > +	struct resource *bus_range = &root->secondary;
> >  	struct mcfg_fixup *f;
> >  	int i;
> >  
> > -	/*
> > -	 * First match against PCI topology <domain:bus> then use OEM ID, OEM
> > -	 * table ID, and OEM revision from MCFG table standard header.
> > -	 */
> >  	for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
> > -		if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) &&
> > -		    !memcmp(f->oem_table_id, mcfg_oem_table_id,
> > -		            ACPI_OEM_TABLE_ID_SIZE) &&
> > -		    f->oem_revision == mcfg_oem_revision &&
> > -		    f->seg == root->segment &&
> > -		    resource_contains(&f->bus_range, &root->secondary)) {
> > +		if (pci_mcfg_quirk_matches(f, segment, bus_range)) {
> >  			if (f->cfgres.start)
> >  				*cfgres = f->cfgres;
> >  			if (f->ops)
> >  				*ecam_ops =  f->ops;
> > -			dev_info(&root->device->dev, "Applying PCI MCFG quirks for %s %s rev: %d\n",
> > -				 f->oem_id, f->oem_table_id, f->oem_revision);
> > +			dev_info(&root->device->dev, "MCFG quirk: ECAM space for %pR at %pR with %ps\n",
> > +				 bus_range, cfgres, *ecam_ops);
> >  			return;
> >  		}
> >  	}
> > @@ -173,7 +180,7 @@ skip_lookup:
> >  	 * MCFG does not have it.  Invalid CFG start address means MCFG
> >  	 * firmware bug or we need another quirk in array.
> >  	 */
> > -	pci_mcfg_match_quirks(root, &res, &ops);
> > +	pci_mcfg_apply_quirks(root, &res, &ops);
> >  	if (!res.start)
> >  		return -ENXIO;
> >  
> > 
> 
> 
> -- 
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
> Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code
> Aurora Forum, a Linux Foundation Collaborative Project.
Christopher Covington Sept. 23, 2016, 7:22 p.m. UTC | #3
On 09/23/2016 03:17 PM, Bjorn Helgaas wrote:
> On Fri, Sep 23, 2016 at 02:41:39PM -0400, Christopher Covington wrote:
>> On 09/22/2016 07:08 PM, Bjorn Helgaas wrote:
>>> On Wed, Sep 21, 2016 at 06:40:47PM -0400, Christopher Covington wrote:
>>>> Hi Bjorn,
>>>>
>>>> On 09/21/2016 09:11 AM, Bjorn Helgaas wrote:
>>>>> On Tue, Sep 20, 2016 at 09:15:14PM -0400, cov@codeaurora.org wrote:
>>>>
>>>>>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>>>>>>> index eb14f74..bb3b8ad 100644
>>>>>>> --- a/drivers/acpi/pci_mcfg.c
>>>>>>> +++ b/drivers/acpi/pci_mcfg.c
>>>>>>> @@ -42,86 +42,59 @@ struct mcfg_fixup {
>>>>>>> 	struct resource cfgres;
>>>>>>> };
>>>>>>>
>>>>>>> -#define MCFG_DOM_ANY			(-1)
>>>>>>
>>>>>> Did you delete this because there were no current users, because you'd
>>>>>> prefer users just use "-1", or for some other reason?
>>>>>
>>>>> I removed it because there were no users of it and, more importantly,
>>>>> the code doesn't implement support for it.
>>>>
>>>> It looks like a stale "First match against PCI topology <domain:bus>..."
>>>> comment remains.
>>>
>>> Yep.  I removed the comment since it's sort of obvious from the code.
>>> I also renamed a few things and pulled the match out into a helper
>>> function.
>>>
>>> I also changed the dmesg note: I think the actual resource and the
>>> name of the pci_ecam_ops is more interesting than the table IDs (which
>>> I think are already elsewhere in the dmesg log).
>>
>> It looks like the resource is already being printed from
>> drivers/pci/ecam.c:102.
> 
> Yes, but I want a hint that a quirk has overridden it because that's a
> clue that there's something wonky about the platform or the firmware.
> 
> But I guess it'd be nice to mirror the format of the existing info
> (mem first, then bus range).
> 
>>> Here's the incremental diff, which I can't really test:
>>
>> Here's what it looks like for me:
>>
>> ACPI: PCI Root Bridge [PCI2] (domain 0002 [bus 00-1f])
>> acpi PNP0A08:02: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
>> acpi PNP0A08:02: _OSC: platform does not support [PCIeHotplug]
>> acpi PNP0A08:02: _OSC: OS now controls [PME AER PCIeCapability]
>> acpi PNP0A08:02: MCFG quirk: ECAM space for [bus 00-1f] at [mem 0xa0000000000-0xa0001ffffff] with pci_3
> 
> Is "pci_3" really the entire name?  If not, what happened to the rest?
> I was hoping for a symbol we could grep for.

The full name is pci_32b_ops. The print overflowed my tmux pane.

>> acpi PNP0A08:02: ECAM at [mem 0xa0000000000-0xa0001ffffff] for [bus 00-1f]
>> Remapped I/O 0x00000affffff0000 to [io  0x10000-0x1ffff window]
>> PCI host bridge to bus 0002:00

Thanks,
Cov
diff mbox

Patch

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index 245b79f..0b36bc5 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -36,7 +36,7 @@  struct mcfg_fixup {
 	char oem_id[ACPI_OEM_ID_SIZE + 1];
 	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
 	u32 oem_revision;
-	u16 seg;
+	u16 segment;
 	struct resource bus_range;
 	struct pci_ecam_ops *ops;
 	struct resource cfgres;
@@ -102,30 +102,37 @@  static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
 static char mcfg_oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
 static u32 mcfg_oem_revision;
 
-static void pci_mcfg_match_quirks(struct acpi_pci_root *root,
+static int pci_mcfg_quirk_matches(struct mcfg_fixup *f, u16 segment,
+				  struct resource *bus_range)
+{
+	if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) &&
+	    !memcmp(f->oem_table_id, mcfg_oem_table_id,
+	            ACPI_OEM_TABLE_ID_SIZE) &&
+	    f->oem_revision == mcfg_oem_revision &&
+	    f->segment == segment &&
+	    resource_contains(&f->bus_range, bus_range))
+		return 1;
+
+	return 0;
+}
+
+static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
 				  struct resource *cfgres,
 				  struct pci_ecam_ops **ecam_ops)
 {
+	u16 segment = root->segment;
+	struct resource *bus_range = &root->secondary;
 	struct mcfg_fixup *f;
 	int i;
 
-	/*
-	 * First match against PCI topology <domain:bus> then use OEM ID, OEM
-	 * table ID, and OEM revision from MCFG table standard header.
-	 */
 	for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
-		if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) &&
-		    !memcmp(f->oem_table_id, mcfg_oem_table_id,
-		            ACPI_OEM_TABLE_ID_SIZE) &&
-		    f->oem_revision == mcfg_oem_revision &&
-		    f->seg == root->segment &&
-		    resource_contains(&f->bus_range, &root->secondary)) {
+		if (pci_mcfg_quirk_matches(f, segment, bus_range)) {
 			if (f->cfgres.start)
 				*cfgres = f->cfgres;
 			if (f->ops)
 				*ecam_ops =  f->ops;
-			dev_info(&root->device->dev, "Applying PCI MCFG quirks for %s %s rev: %d\n",
-				 f->oem_id, f->oem_table_id, f->oem_revision);
+			dev_info(&root->device->dev, "MCFG quirk: ECAM space for %pR at %pR with %ps\n",
+				 bus_range, cfgres, *ecam_ops);
 			return;
 		}
 	}
@@ -173,7 +180,7 @@  skip_lookup:
 	 * MCFG does not have it.  Invalid CFG start address means MCFG
 	 * firmware bug or we need another quirk in array.
 	 */
-	pci_mcfg_match_quirks(root, &res, &ops);
+	pci_mcfg_apply_quirks(root, &res, &ops);
 	if (!res.start)
 		return -ENXIO;