From: Dongdong Liu <hidden> Date: 2016-06-13 12:50:03
This series bases on pending ACPI PCI support for ARM64:
https://lkml.org/lkml/2016/6/10/706
Quirk handling relies on an idea of matching MCFG OEM ID and OEM TABLE ID
(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.
v1 -> v2
- use oem_id and oem_table_id to replace oem_id and oem_revision to make
a difference between different platforms.
- change pci_cfg_fixup __mcfg_fixup_##system##dom##bus to
__mcfg_fixup_##oem_id##oem_table_id##dom##bus. With oem_id and oem_table_id
is to ensure that the object is unique.
Tomasz Nowicki (2):
ACPI/PCI: Match PCI config space accessors against platfrom specific
ECAM quirks
ARM64/PCI: Start using quirks handling for ACPI based PCI host
controller
arch/arm64/kernel/pci.c | 7 ++++---
drivers/acpi/pci_mcfg.c | 41 ++++++++++++++++++++++++++++++++++++---
include/asm-generic/vmlinux.lds.h | 7 +++++++
include/linux/pci-acpi.h | 20 +++++++++++++++++++
4 files changed, 69 insertions(+), 6 deletions(-)
--
1.9.1
From: Dongdong Liu <hidden> Date: 2016-06-13 12:50:14
From: Tomasz Nowicki <redacted>
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 <redacted>
---
arch/arm64/kernel/pci.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
From: Dongdong Liu <hidden> Date: 2016-06-13 12:50:44
From: Tomasz Nowicki <redacted>
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_table_id, domain, bus number> tuple and returns corresponding
PCI config ops. oem_id and oem_table_id 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_table_id>, <domain_nr>, <bus_nr>);
Signed-off-by: Tomasz Nowicki <redacted>
Signed-off-by: Dongdong Liu <redacted>
---
drivers/acpi/pci_mcfg.c | 41 ++++++++++++++++++++++++++++++++++++---
include/asm-generic/vmlinux.lds.h | 7 +++++++
include/linux/pci-acpi.h | 20 +++++++++++++++++++
3 files changed, 65 insertions(+), 3 deletions(-)
@@ -22,6 +22,10 @@#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 */+staticstructacpi_table_mcfg*mcfg_table;/* Structure to hold entries from the MCFG table */structmcfg_entry{
@@ -35,6 +39,38 @@ struct mcfg_entry {/* List to save mcfg entries */staticLIST_HEAD(pci_mcfg_list);+externstructpci_cfg_fixup__start_acpi_mcfg_fixups[];+externstructpci_cfg_fixup__end_acpi_mcfg_fixups[];++structpci_ecam_ops*pci_mcfg_get_ops(structacpi_pci_root*root)+{+intbus_num=root->secondary.start;+intdomain=root->segment;+structpci_cfg_fixup*f;++if(!mcfg_table)+return&pci_generic_ecam_ops;++/*+*Matchagainstplatformspecificquirksandreturncorresponding+*CAMops.+*+*FirstmatchagainstPCItopology<domain:bus>thenuseOEMIDand+*OEMrevisionfromMCFGtablestandardheader.+*/+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))&&+(!strncmp(f->oem_table_id,mcfg_table->header.oem_table_id,+ACPI_OEM_TABLE_ID_SIZE)))+returnf->ops;+}+/* No quirks, use ECAM */+return&pci_generic_ecam_ops;+}+phys_addr_tpci_mcfg_lookup(u16seg,structresource*bus_res){structmcfg_entry*e;
-----Original Message-----
From: liudongdong (C)
Sent: 13 June 2016 14:02
To: 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;
tn at semihalf.com
Cc: robert.richter at caviumnetworks.com; mw at semihalf.com;
Liviu.Dudau at arm.com; ddaney at caviumnetworks.com; Wangyijing;
Suravee.Suthikulpanit at amd.com; msalter at redhat.com; linux-
pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-
acpi at vger.kernel.org; linux-kernel at vger.kernel.org; linaro-
acpi at lists.linaro.org; jcm at redhat.com; andrea.gallo at linaro.org;
dhdang at apm.com; jeremy.linton at arm.com; liudongdong (C);
cov at codeaurora.org; Gabriele Paoloni; Chenxin (Charles); Linuxarm
Subject: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space accessors
against platfrom specific ECAM quirks
From: Tomasz Nowicki <redacted>
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_table_id, domain, bus number> tuple and returns
corresponding
PCI config ops. oem_id and oem_table_id 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_table_id>,
<domain_nr>, <bus_nr>);
Signed-off-by: Tomasz Nowicki <redacted>
Signed-off-by: Dongdong Liu <redacted>
---
drivers/acpi/pci_mcfg.c | 41
++++++++++++++++++++++++++++++++++++---
include/asm-generic/vmlinux.lds.h | 7 +++++++
include/linux/pci-acpi.h | 20 +++++++++++++++++++
3 files changed, 65 insertions(+), 3 deletions(-)
@@ -22,6 +22,10 @@#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 */+staticstructacpi_table_mcfg*mcfg_table;/* Structure to hold entries from the MCFG table */structmcfg_entry{
@@ -35,6 +39,38 @@ struct mcfg_entry {/* List to save mcfg entries */staticLIST_HEAD(pci_mcfg_list);+externstructpci_cfg_fixup__start_acpi_mcfg_fixups[];+externstructpci_cfg_fixup__end_acpi_mcfg_fixups[];++structpci_ecam_ops*pci_mcfg_get_ops(structacpi_pci_root*root)+{+intbus_num=root->secondary.start;+intdomain=root->segment;+structpci_cfg_fixup*f;++if(!mcfg_table)+return&pci_generic_ecam_ops;++/*+*Matchagainstplatformspecificquirksandreturn
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)) &&
+ (!strncmp(f->oem_table_id, mcfg_table-
quoted
header.oem_table_id,
+ ACPI_OEM_TABLE_ID_SIZE)))
As you can see here Liudongdong has replaced oem_revision with
oem_table_id.
Now it seems that there are some platforms that have already shipped
using a matching based on the oem_revision (right Jon?)
However I guess that if in FW they have defined oem_table_id properly
they should be able to use this mechanism without needing to a FW update.
Can these vendors confirm this?
Tomasz do you think this can work for Cavium Thunder?
Thanks
Gab
From: Sinan Kaya <hidden> Date: 2016-06-13 14:03:09
On 6/13/2016 9:54 AM, Gabriele Paoloni wrote:
As you can see here Liudongdong has replaced oem_revision with
oem_table_id.
Now it seems that there are some platforms that have already shipped
using a matching based on the oem_revision (right Jon?)
However I guess that if in FW they have defined oem_table_id properly
they should be able to use this mechanism without needing to a FW update.
Can these vendors confirm this?
Tomasz do you think this can work for Cavium Thunder?
Thanks
Gab
Why not have all three of them?
The initial approach was OEM id and revision id.
Jeff Hugo indicated that addition (not removing any other fields) of table id
would make more sense.
--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
-----Original Message-----
From: Sinan Kaya [mailto:okaya at codeaurora.org]
Sent: 13 June 2016 15:03
To: Gabriele Paoloni; liudongdong (C); 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;
jchandra at broadcom.com; tn at semihalf.com
Cc: robert.richter at caviumnetworks.com; mw at semihalf.com;
Liviu.Dudau at arm.com; ddaney at caviumnetworks.com; Wangyijing;
Suravee.Suthikulpanit at amd.com; msalter at redhat.com; linux-
pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-
acpi at vger.kernel.org; linux-kernel at vger.kernel.org; linaro-
acpi at lists.linaro.org; jcm at redhat.com; andrea.gallo at linaro.org;
dhdang at apm.com; jeremy.linton at arm.com; cov at codeaurora.org; Chenxin
(Charles); Linuxarm
Subject: Re: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space
accessors against platfrom specific ECAM quirks
On 6/13/2016 9:54 AM, Gabriele Paoloni wrote:
quoted
As you can see here Liudongdong has replaced oem_revision with
oem_table_id.
Now it seems that there are some platforms that have already shipped
using a matching based on the oem_revision (right Jon?)
However I guess that if in FW they have defined oem_table_id properly
they should be able to use this mechanism without needing to a FW
update.
quoted
Can these vendors confirm this?
Tomasz do you think this can work for Cavium Thunder?
Thanks
Gab
Why not have all three of them?
The initial approach was OEM id and revision id.
Jeff Hugo indicated that addition (not removing any other fields) of
table id
would make more sense.
Mmm from last email of Jeff Hugo on "[RFC PATCH 1/3] pci, acpi: Match
PCI config space accessors against platfrom specific ECAM quirks."
I quote:
"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."
So it looks to me that he pointed out that using the OEM revision field
is wrong...and this is why I have asked if replacing it with the table
id can work for other vendors....
Thanks
Gab
--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center,
Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
-----Original Message-----
From: Sinan Kaya [mailto:okaya at codeaurora.org]
Sent: 13 June 2016 15:03
To: Gabriele Paoloni; liudongdong (C); 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;
jchandra at broadcom.com; tn at semihalf.com
Cc: robert.richter at caviumnetworks.com; mw at semihalf.com;
Liviu.Dudau at arm.com; ddaney at caviumnetworks.com; Wangyijing;
Suravee.Suthikulpanit at amd.com; msalter at redhat.com; linux-
pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-
acpi at vger.kernel.org; linux-kernel at vger.kernel.org; linaro-
acpi at lists.linaro.org; jcm at redhat.com; andrea.gallo at linaro.org;
dhdang at apm.com; jeremy.linton at arm.com; cov at codeaurora.org; Chenxin
(Charles); Linuxarm
Subject: Re: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space
accessors against platfrom specific ECAM quirks
On 6/13/2016 9:54 AM, Gabriele Paoloni wrote:
quoted
As you can see here Liudongdong has replaced oem_revision with
oem_table_id.
Now it seems that there are some platforms that have already shipped
using a matching based on the oem_revision (right Jon?)
However I guess that if in FW they have defined oem_table_id properly
they should be able to use this mechanism without needing to a FW
update.
quoted
Can these vendors confirm this?
Tomasz do you think this can work for Cavium Thunder?
Thanks
Gab
Why not have all three of them?
The initial approach was OEM id and revision id.
Jeff Hugo indicated that addition (not removing any other fields) of
table id
would make more sense.
Mmm from last email of Jeff Hugo on "[RFC PATCH 1/3] pci, acpi: Match
PCI config space accessors against platfrom specific ECAM quirks."
I quote:
"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."
So it looks to me that he pointed out that using the OEM revision field
is wrong...and this is why I have asked if replacing it with the table
id can work for other vendors....
Thanks
Gab
I had an internal discussion with jeff and cov before posting on the
maillist.
I think there is missing info in the email.
Usage of oem id + table id + revision is ok.
Usage of oem id + revision is not ok as one oem can build multiple chips
with the same oem id and revision id but different table id. Otherwise,
we can run out of revisions very quickly.
quoted
--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center,
Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
@@ -22,6 +22,10 @@#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 */+staticstructacpi_table_mcfg*mcfg_table;/* Structure to hold entries from the MCFG table */structmcfg_entry{
@@ -35,6 +39,38 @@ struct mcfg_entry {/* List to save mcfg entries */staticLIST_HEAD(pci_mcfg_list);+externstructpci_cfg_fixup__start_acpi_mcfg_fixups[];+externstructpci_cfg_fixup__end_acpi_mcfg_fixups[];++structpci_ecam_ops*pci_mcfg_get_ops(structacpi_pci_root*root)+{+intbus_num=root->secondary.start;+intdomain=root->segment;+structpci_cfg_fixup*f;++if(!mcfg_table)+return&pci_generic_ecam_ops;++/*+*Matchagainstplatformspecificquirksandreturncorresponding+*CAMops.+*+*FirstmatchagainstPCItopology<domain:bus>thenuseOEMIDand+*OEMrevisionfromMCFGtablestandardheader.+*/+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))&&+(!strncmp(f->oem_table_id,mcfg_table->header.oem_table_id,+ACPI_OEM_TABLE_ID_SIZE)))
This would just be a small convenience, but if the character count used here were
min(strlen(f->oem_id), ACPI_OEM_ID_SIZE)
then the parameters to DECLARE_ACPI_MCFG_FIXUP macro could be substrings and
wouldn't need to be padded out to the full length.
+ return f->ops;
+ }
+ /* No quirks, use ECAM */
+ return &pci_generic_ecam_ops;
+}
@@ -72,6 +73,25 @@ struct acpi_pci_root_ops {int(*prepare_resources)(structacpi_pci_root_info*info);};+structpci_cfg_fixup{+structpci_ecam_ops*ops;+char*oem_id;+char*oem_table_id;+intdomain;+intbus_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, oem_table_id, dom, bus) \+staticconststructpci_cfg_fixup\+__mcfg_fixup_##oem_id##oem_table_id##dom##bus\
I'm not entirely sure that this is the right fix--I'm pretty blindly
following a GCC documentation suggestion [1]--but removing the first two
preprocessor concatenation operators "##" solved the following build error
for me.
include/linux/pci-acpi.h:90:2: error: pasting "__mcfg_fixup_" and ""QCOM"" does not give a valid preprocessing token
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
arch/arm64/kernel/pci.c:225:1: note: in expansion of macro ?DECLARE_ACPI_MCFG_FIXUP?
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432", PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
arch/arm64/kernel/pci.c:225:44: error: pasting ""QCOM"" and ""QDF2432"" does not give a valid preprocessing token
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432", PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:17: note: in definition of macro ?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
arch/arm64/kernel/pci.c:225:52: error: pasting ""QDF2432"" and "PCI_MCFG_DOMAIN_ANY" does not give a valid preprocessi
ng token
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432", PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:25: note: in definition of macro ?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
arch/arm64/kernel/pci.c:225:44: error: expected ?=?, ?,?, ?;?, ?asm? or ?__attribute__? before string constant
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432", PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:17: note: in definition of macro ?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
make[1]: *** [arch/arm64/kernel/pci.o] Error 1
make: *** [arch/arm64/kernel] Error 2
1. https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html#Concatenation
Thanks,
Cov
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
From: Jeffrey Hugo <hidden> Date: 2016-06-13 15:59:43
On 6/13/2016 9:12 AM, okaya at codeaurora.org wrote:
On 2016-06-13 10:29, Gabriele Paoloni wrote:
quoted
Hi Sinan
quoted
-----Original Message-----
From: Sinan Kaya [mailto:okaya at codeaurora.org]
Sent: 13 June 2016 15:03
To: Gabriele Paoloni; liudongdong (C); 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;
jchandra at broadcom.com; tn at semihalf.com
Cc: robert.richter at caviumnetworks.com; mw at semihalf.com;
Liviu.Dudau at arm.com; ddaney at caviumnetworks.com; Wangyijing;
Suravee.Suthikulpanit at amd.com; msalter at redhat.com; linux-
pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-
acpi at vger.kernel.org; linux-kernel at vger.kernel.org; linaro-
acpi at lists.linaro.org; jcm at redhat.com; andrea.gallo at linaro.org;
dhdang at apm.com; jeremy.linton at arm.com; cov at codeaurora.org; Chenxin
(Charles); Linuxarm
Subject: Re: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space
accessors against platfrom specific ECAM quirks
On 6/13/2016 9:54 AM, Gabriele Paoloni wrote:
quoted
As you can see here Liudongdong has replaced oem_revision with
oem_table_id.
Now it seems that there are some platforms that have already shipped
using a matching based on the oem_revision (right Jon?)
However I guess that if in FW they have defined oem_table_id properly
they should be able to use this mechanism without needing to a FW
update.
quoted
Can these vendors confirm this?
Tomasz do you think this can work for Cavium Thunder?
Thanks
Gab
Why not have all three of them?
The initial approach was OEM id and revision id.
Jeff Hugo indicated that addition (not removing any other fields) of
table id
would make more sense.
Mmm from last email of Jeff Hugo on "[RFC PATCH 1/3] pci, acpi: Match
PCI config space accessors against platfrom specific ECAM quirks."
I quote:
"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."
So it looks to me that he pointed out that using the OEM revision field
is wrong...and this is why I have asked if replacing it with the table
id can work for other vendors....
Thanks
Gab
I had an internal discussion with jeff and cov before posting on the
maillist.
I think there is missing info in the email.
Usage of oem id + table id + revision is ok.
Usage of oem id + revision is not ok as one oem can build multiple chips
with the same oem id and revision id but different table id. Otherwise,
we can run out of revisions very quickly.
Agreed.
I'm sorry for the confusion. My intent was to point out that revision
alone appeared insufficient to address all the identified problems, but
I believe there is still a case for using revision. Table id is useful
for differentiating between platforms/chips. Revision is useful for
differentiation between different versions of a single platform/chip
assuming the silicon is respun or some other fix is applied. Both solve
different scenarios, and I'm not aware of a reason why they could not be
used together to solve all currently identified cases.
quoted
quoted
--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center,
Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
--
Jeffrey Hugo
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
@@ -22,6 +22,10 @@#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 */+staticstructacpi_table_mcfg*mcfg_table;/* Structure to hold entries from the MCFG table */structmcfg_entry{
@@ -35,6 +39,38 @@ struct mcfg_entry {/* List to save mcfg entries */staticLIST_HEAD(pci_mcfg_list);+externstructpci_cfg_fixup__start_acpi_mcfg_fixups[];+externstructpci_cfg_fixup__end_acpi_mcfg_fixups[];++structpci_ecam_ops*pci_mcfg_get_ops(structacpi_pci_root*root)+{+intbus_num=root->secondary.start;+intdomain=root->segment;+structpci_cfg_fixup*f;++if(!mcfg_table)+return&pci_generic_ecam_ops;++/*+*Matchagainstplatformspecificquirksandreturncorresponding+*CAMops.+*+*FirstmatchagainstPCItopology<domain:bus>thenuseOEMIDand+*OEMrevisionfromMCFGtablestandardheader.+*/+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))&&+(!strncmp(f->oem_table_id,mcfg_table->header.oem_table_id,+ACPI_OEM_TABLE_ID_SIZE)))
This would just be a small convenience, but if the character count used here were
min(strlen(f->oem_id), ACPI_OEM_ID_SIZE)
then the parameters to DECLARE_ACPI_MCFG_FIXUP macro could be substrings and
wouldn't need to be padded out to the full length.
quoted
+ return f->ops;
+ }
+ /* No quirks, use ECAM */
+ return &pci_generic_ecam_ops;
+}
@@ -72,6 +73,25 @@ struct acpi_pci_root_ops {int(*prepare_resources)(structacpi_pci_root_info*info);};+structpci_cfg_fixup{+structpci_ecam_ops*ops;+char*oem_id;+char*oem_table_id;+intdomain;+intbus_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, oem_table_id, dom, bus) \+staticconststructpci_cfg_fixup\+__mcfg_fixup_##oem_id##oem_table_id##dom##bus\
I'm not entirely sure that this is the right fix--I'm pretty blindly
following a GCC documentation suggestion [1]--but removing the first two
preprocessor concatenation operators "##" solved the following build error
for me.
include/linux/pci-acpi.h:90:2: error: pasting "__mcfg_fixup_" and ""QCOM"" does not give a valid preprocessing token
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
I think the problem is gcc is not happy with quoted string when
processing these tokens
(""QCOM"", the extra "" are added by gcc). So should we not concat
string tokens and
use the fixup definition in v1 of this RFC:
/* 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 };
Regards,
Duc Dang.
^
arch/arm64/kernel/pci.c:225:1: note: in expansion of macro ?DECLARE_ACPI_MCFG_FIXUP?
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432", PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
arch/arm64/kernel/pci.c:225:44: error: pasting ""QCOM"" and ""QDF2432"" does not give a valid preprocessing token
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432", PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:17: note: in definition of macro ?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
arch/arm64/kernel/pci.c:225:52: error: pasting ""QDF2432"" and "PCI_MCFG_DOMAIN_ANY" does not give a valid preprocessi
ng token
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432", PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:25: note: in definition of macro ?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
arch/arm64/kernel/pci.c:225:44: error: expected ?=?, ?,?, ?;?, ?asm? or ?__attribute__? before string constant
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432", PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:17: note: in definition of macro ?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
make[1]: *** [arch/arm64/kernel/pci.o] Error 1
make: *** [arch/arm64/kernel] Error 2
1. https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html#Concatenation
Thanks,
Cov
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Mon, Jun 13, 2016 at 8:59 AM, Jeffrey Hugo [off-list ref] wrote:
On 6/13/2016 9:12 AM, okaya at codeaurora.org wrote:
quoted
On 2016-06-13 10:29, Gabriele Paoloni wrote:
quoted
Hi Sinan
quoted
-----Original Message-----
From: Sinan Kaya [mailto:okaya at codeaurora.org]
Sent: 13 June 2016 15:03
To: Gabriele Paoloni; liudongdong (C); 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;
jchandra at broadcom.com; tn at semihalf.com
Cc: robert.richter at caviumnetworks.com; mw at semihalf.com;
Liviu.Dudau at arm.com; ddaney at caviumnetworks.com; Wangyijing;
Suravee.Suthikulpanit at amd.com; msalter at redhat.com; linux-
pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-
acpi at vger.kernel.org; linux-kernel at vger.kernel.org; linaro-
acpi at lists.linaro.org; jcm at redhat.com; andrea.gallo at linaro.org;
dhdang at apm.com; jeremy.linton at arm.com; cov at codeaurora.org; Chenxin
(Charles); Linuxarm
Subject: Re: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space
accessors against platfrom specific ECAM quirks
On 6/13/2016 9:54 AM, Gabriele Paoloni wrote:
quoted
As you can see here Liudongdong has replaced oem_revision with
oem_table_id.
Now it seems that there are some platforms that have already shipped
using a matching based on the oem_revision (right Jon?)
However I guess that if in FW they have defined oem_table_id properly
they should be able to use this mechanism without needing to a FW
update.
quoted
Can these vendors confirm this?
Tomasz do you think this can work for Cavium Thunder?
Thanks
Gab
Why not have all three of them?
The initial approach was OEM id and revision id.
Jeff Hugo indicated that addition (not removing any other fields) of
table id
would make more sense.
Mmm from last email of Jeff Hugo on "[RFC PATCH 1/3] pci, acpi: Match
PCI config space accessors against platfrom specific ECAM quirks."
I quote:
"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."
So it looks to me that he pointed out that using the OEM revision field
is wrong...and this is why I have asked if replacing it with the table
id can work for other vendors....
Thanks
Gab
I had an internal discussion with jeff and cov before posting on the
maillist.
I think there is missing info in the email.
Usage of oem id + table id + revision is ok.
Usage of oem id + revision is not ok as one oem can build multiple chips
with the same oem id and revision id but different table id. Otherwise,
we can run out of revisions very quickly.
Agreed.
I'm sorry for the confusion. My intent was to point out that revision alone
appeared insufficient to address all the identified problems, but I believe
there is still a case for using revision. Table id is useful for
differentiating between platforms/chips. Revision is useful for
differentiation between different versions of a single platform/chip
assuming the silicon is respun or some other fix is applied. Both solve
different scenarios, and I'm not aware of a reason why they could not be
used together to solve all currently identified cases.
Using OEM ID + Table ID + Revision will work for X-Gene platforms as well.
Regards,
Duc Dang.
quoted
quoted
quoted
--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center,
Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
--
Jeffrey Hugo
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project
@@ -22,6 +22,10 @@#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 */+staticstructacpi_table_mcfg*mcfg_table;/* Structure to hold entries from the MCFG table */structmcfg_entry{
@@ -35,6 +39,38 @@ struct mcfg_entry {/* List to save mcfg entries */staticLIST_HEAD(pci_mcfg_list);+externstructpci_cfg_fixup__start_acpi_mcfg_fixups[];+externstructpci_cfg_fixup__end_acpi_mcfg_fixups[];++structpci_ecam_ops*pci_mcfg_get_ops(structacpi_pci_root*root)+{+intbus_num=root->secondary.start;+intdomain=root->segment;+structpci_cfg_fixup*f;++if(!mcfg_table)+return&pci_generic_ecam_ops;++/*+*Matchagainstplatformspecificquirksandreturncorresponding+*CAMops.+*+*FirstmatchagainstPCItopology<domain:bus>thenuseOEMIDand+*OEMrevisionfromMCFGtablestandardheader.+*/+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))&&+(!strncmp(f->oem_table_id,mcfg_table->header.oem_table_id,+ACPI_OEM_TABLE_ID_SIZE)))
This would just be a small convenience, but if the character count used here were
min(strlen(f->oem_id), ACPI_OEM_ID_SIZE)
then the parameters to DECLARE_ACPI_MCFG_FIXUP macro could be substrings and
wouldn't need to be padded out to the full length.
quoted
+ return f->ops;
+ }
+ /* No quirks, use ECAM */
+ return &pci_generic_ecam_ops;
+}
@@ -72,6 +73,25 @@ struct acpi_pci_root_ops {int(*prepare_resources)(structacpi_pci_root_info*info);};+structpci_cfg_fixup{+structpci_ecam_ops*ops;+char*oem_id;+char*oem_table_id;+intdomain;+intbus_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, oem_table_id, dom, bus) \+staticconststructpci_cfg_fixup\+__mcfg_fixup_##oem_id##oem_table_id##dom##bus\
I'm not entirely sure that this is the right fix--I'm pretty blindly
following a GCC documentation suggestion [1]--but removing the first two
preprocessor concatenation operators "##" solved the following build error
for me.
include/linux/pci-acpi.h:90:2: error: pasting "__mcfg_fixup_" and ""QCOM"" does not give a valid preprocessing token
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
I think the problem is gcc is not happy with quoted string when
processing these tokens
(""QCOM"", the extra "" are added by gcc). So should we not concat
string tokens and
use the fixup definition in v1 of this RFC:
/* 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 };
V1 fixup exist the redefinition error when compiling mutiple DECLARE_ACPI_MCFG_FIXUP
with the same PCI_MCFG_DOMAIN_ANY and PCI_MCFG_BUS_ANY.
#define EFI_ACPI_HISI_OEM_ID "HISI"
#define EFI_ACPI_HISI_D02_OEM_TABLE_ID "HISI-D02"
#define EFI_ACPI_HISI_D03_OEM_TABLE_ID "HISI-D03"
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
EFI_ACPI_HISI_D02_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
EFI_ACPI_HISI_D03_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
In file included from drivers/pci/host/pcie-hisi-acpi.c:15:0:
include/linux/pci-acpi.h:98:43: error: redefinition of '__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY'
static const struct pci_cfg_fixup __mcfg_fixup_##system##dom##bus\
^
drivers/pci/host/pcie-hisi-acpi.c:215:1: note: in expansion of macro 'DECLARE_ACPI_MCFG_FIXUP'
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
^
include/linux/pci-acpi.h:98:43: note: previous definition of '__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY' was here
static const struct pci_cfg_fixup __mcfg_fixup_##system##dom##bus\
^
drivers/pci/host/pcie-hisi-acpi.c:212:1: note: in expansion of macro 'DECLARE_ACPI_MCFG_FIXUP'
V2 fixup can resolve the redefinition error, but need to use macro.
We can see that the name of macro is not replace with it's value in
"__mcfg_fixup_EFI_ACPI_HISI_OEM_IDEFI_ACPI_HISI_D03_OEM_TABLE_IDPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY".
Any good idea is appreciated.
Thanks
Dongdong
Regards,
Duc Dang.
quoted
^
arch/arm64/kernel/pci.c:225:1: note: in expansion of macro ?DECLARE_ACPI_MCFG_FIXUP?
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432", PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
arch/arm64/kernel/pci.c:225:44: error: pasting ""QCOM"" and ""QDF2432"" does not give a valid preprocessing token
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432", PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:17: note: in definition of macro ?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
arch/arm64/kernel/pci.c:225:52: error: pasting ""QDF2432"" and "PCI_MCFG_DOMAIN_ANY" does not give a valid preprocessi
ng token
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432", PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:25: note: in definition of macro ?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
arch/arm64/kernel/pci.c:225:44: error: expected ?=?, ?,?, ?;?, ?asm? or ?__attribute__? before string constant
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432", PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:17: note: in definition of macro ?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
make[1]: *** [arch/arm64/kernel/pci.o] Error 1
make: *** [arch/arm64/kernel] Error 2
1. https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html#Concatenation
Thanks,
Cov
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
@@ -22,6 +22,10 @@#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 */+staticstructacpi_table_mcfg*mcfg_table;/* Structure to hold entries from the MCFG table */structmcfg_entry{
@@ -35,6 +39,38 @@ struct mcfg_entry {/* List to save mcfg entries */staticLIST_HEAD(pci_mcfg_list);+externstructpci_cfg_fixup__start_acpi_mcfg_fixups[];+externstructpci_cfg_fixup__end_acpi_mcfg_fixups[];++structpci_ecam_ops*pci_mcfg_get_ops(structacpi_pci_root*root)+{+intbus_num=root->secondary.start;+intdomain=root->segment;+structpci_cfg_fixup*f;++if(!mcfg_table)+return&pci_generic_ecam_ops;++/*+*Matchagainstplatformspecificquirksandreturncorresponding+*CAMops.+*+*FirstmatchagainstPCItopology<domain:bus>thenuseOEMID
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)) &&
+ (!strncmp(f->oem_table_id,
mcfg_table->header.oem_table_id,
+ ACPI_OEM_TABLE_ID_SIZE)))
This would just be a small convenience, but if the character count used
here were
min(strlen(f->oem_id), ACPI_OEM_ID_SIZE)
then the parameters to DECLARE_ACPI_MCFG_FIXUP macro could be substrings
and
wouldn't need to be padded out to the full length.
quoted
+ return f->ops;
+ }
+ /* No quirks, use ECAM */
+ return &pci_generic_ecam_ops;
+}
@@ -72,6 +73,25 @@ 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;+ char *oem_table_id;+ 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, oem_table_id, dom, bus) \+ static const struct pci_cfg_fixup \+ __mcfg_fixup_##oem_id##oem_table_id##dom##bus \
I'm not entirely sure that this is the right fix--I'm pretty blindly
following a GCC documentation suggestion [1]--but removing the first two
preprocessor concatenation operators "##" solved the following build
error
for me.
include/linux/pci-acpi.h:90:2: error: pasting "__mcfg_fixup_" and
""QCOM"" does not give a valid preprocessing token
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
I think the problem is gcc is not happy with quoted string when
processing these tokens
(""QCOM"", the extra "" are added by gcc). So should we not concat
string tokens and
use the fixup definition in v1 of this RFC:
/* 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 };
V1 fixup exist the redefinition error when compiling mutiple
DECLARE_ACPI_MCFG_FIXUP
with the same PCI_MCFG_DOMAIN_ANY and PCI_MCFG_BUS_ANY.
#define EFI_ACPI_HISI_OEM_ID "HISI"
#define EFI_ACPI_HISI_D02_OEM_TABLE_ID "HISI-D02"
#define EFI_ACPI_HISI_D03_OEM_TABLE_ID "HISI-D03"
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
EFI_ACPI_HISI_D02_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY,
PCI_MCFG_BUS_ANY);
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
EFI_ACPI_HISI_D03_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY,
PCI_MCFG_BUS_ANY);
In file included from drivers/pci/host/pcie-hisi-acpi.c:15:0:
include/linux/pci-acpi.h:98:43: error: redefinition of
'__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY'
static const struct pci_cfg_fixup __mcfg_fixup_##system##dom##bus\
^
drivers/pci/host/pcie-hisi-acpi.c:215:1: note: in expansion of macro
'DECLARE_ACPI_MCFG_FIXUP'
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
^
include/linux/pci-acpi.h:98:43: note: previous definition of
'__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY' was here
static const struct pci_cfg_fixup __mcfg_fixup_##system##dom##bus\
^
drivers/pci/host/pcie-hisi-acpi.c:212:1: note: in expansion of macro
'DECLARE_ACPI_MCFG_FIXUP'
V2 fixup can resolve the redefinition error, but need to use macro.
We can see that the name of macro is not replace with it's value in
"__mcfg_fixup_EFI_ACPI_HISI_OEM_IDEFI_ACPI_HISI_D03_OEM_TABLE_IDPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY".
Any good idea is appreciated.
Hmmm.
I was testing # op and using min_t to get the min-len when doing
strncmp similar to Chris' suggestion (using min_t avoids type
warnings)
/* Designate a routine to fix up buggy MCFG */
#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, rev, dom, bus) \
static const struct pci_cfg_fixup
__mcfg_fixup##oem_id##oem_table_id##rev##dom##bus\
__used __attribute__((__section__(".acpi_fixup_mcfg"), \
aligned((sizeof(void *))))) = \
{ ops, #oem_id, #oem_table_id, rev, dom, bus };
My fixup definition:
DECLARE_ACPI_MCFG_FIXUP(&xgene_pcie_ecam_ops, APM, XGENE, 2,
PCI_MCFG_DOMAIN_ANY,
PCI_MCFG_BUS_ANY);
My match condition is:
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->header.oem_id,
min_t(size_t, strlen(f->oem_id),
ACPI_OEM_ID_SIZE))) &&
(!strncmp(f->oem_table_id, mcfg->header.oem_table_id,
min_t(size_t, strlen(f->oem_table_id),
ACPI_OEM_TABLE_ID_SIZE))) &&
(f->oem_revision == mcfg->header.oem_revision)) {
return f->ops;
}
But this still does not work for your case as having HISI-D02 as
oem_table_id will cause error.
Thanks
Dongdong
quoted
Regards,
Duc Dang.
quoted
^
arch/arm64/kernel/pci.c:225:1: note: in expansion of macro
?DECLARE_ACPI_MCFG_FIXUP?
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432",
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
arch/arm64/kernel/pci.c:225:44: error: pasting ""QCOM"" and ""QDF2432""
does not give a valid preprocessing token
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432",
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:17: note: in definition of macro
?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
arch/arm64/kernel/pci.c:225:52: error: pasting ""QDF2432"" and
"PCI_MCFG_DOMAIN_ANY" does not give a valid preprocessi
ng token
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432",
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:25: note: in definition of macro
?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
arch/arm64/kernel/pci.c:225:44: error: expected ?=?, ?,?, ?;?, ?asm? or
?__attribute__? before string constant
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432",
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:17: note: in definition of macro
?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
make[1]: *** [arch/arm64/kernel/pci.o] Error 1
make: *** [arch/arm64/kernel] Error 2
1. https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html#Concatenation
Thanks,
Cov
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
@@ -22,6 +22,10 @@#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 */+staticstructacpi_table_mcfg*mcfg_table;/* Structure to hold entries from the MCFG table */structmcfg_entry{
@@ -35,6 +39,38 @@ struct mcfg_entry {/* List to save mcfg entries */staticLIST_HEAD(pci_mcfg_list);+externstructpci_cfg_fixup__start_acpi_mcfg_fixups[];+externstructpci_cfg_fixup__end_acpi_mcfg_fixups[];++structpci_ecam_ops*pci_mcfg_get_ops(structacpi_pci_root*root)+{+intbus_num=root->secondary.start;+intdomain=root->segment;+structpci_cfg_fixup*f;++if(!mcfg_table)+return&pci_generic_ecam_ops;++/*+*Matchagainstplatformspecificquirksandreturncorresponding+*CAMops.+*+*FirstmatchagainstPCItopology<domain:bus>thenuseOEMID
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)) &&
+ (!strncmp(f->oem_table_id,
mcfg_table->header.oem_table_id,
+ ACPI_OEM_TABLE_ID_SIZE)))
This would just be a small convenience, but if the character count used
here were
min(strlen(f->oem_id), ACPI_OEM_ID_SIZE)
then the parameters to DECLARE_ACPI_MCFG_FIXUP macro could be substrings
and
wouldn't need to be padded out to the full length.
quoted
+ return f->ops;
+ }
+ /* No quirks, use ECAM */
+ return &pci_generic_ecam_ops;
+}
@@ -72,6 +73,25 @@ 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;+ char *oem_table_id;+ 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, oem_table_id, dom, bus) \+ static const struct pci_cfg_fixup \+ __mcfg_fixup_##oem_id##oem_table_id##dom##bus \
I'm not entirely sure that this is the right fix--I'm pretty blindly
following a GCC documentation suggestion [1]--but removing the first two
preprocessor concatenation operators "##" solved the following build
error
for me.
include/linux/pci-acpi.h:90:2: error: pasting "__mcfg_fixup_" and
""QCOM"" does not give a valid preprocessing token
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
I think the problem is gcc is not happy with quoted string when
processing these tokens
(""QCOM"", the extra "" are added by gcc). So should we not concat
string tokens and
use the fixup definition in v1 of this RFC:
/* 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 };
V1 fixup exist the redefinition error when compiling mutiple
DECLARE_ACPI_MCFG_FIXUP
with the same PCI_MCFG_DOMAIN_ANY and PCI_MCFG_BUS_ANY.
#define EFI_ACPI_HISI_OEM_ID "HISI"
#define EFI_ACPI_HISI_D02_OEM_TABLE_ID "HISI-D02"
#define EFI_ACPI_HISI_D03_OEM_TABLE_ID "HISI-D03"
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
EFI_ACPI_HISI_D02_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY,
PCI_MCFG_BUS_ANY);
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
EFI_ACPI_HISI_D03_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY,
PCI_MCFG_BUS_ANY);
In file included from drivers/pci/host/pcie-hisi-acpi.c:15:0:
include/linux/pci-acpi.h:98:43: error: redefinition of
'__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY'
static const struct pci_cfg_fixup __mcfg_fixup_##system##dom##bus\
^
drivers/pci/host/pcie-hisi-acpi.c:215:1: note: in expansion of macro
'DECLARE_ACPI_MCFG_FIXUP'
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
^
include/linux/pci-acpi.h:98:43: note: previous definition of
'__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY' was here
static const struct pci_cfg_fixup __mcfg_fixup_##system##dom##bus\
^
drivers/pci/host/pcie-hisi-acpi.c:212:1: note: in expansion of macro
'DECLARE_ACPI_MCFG_FIXUP'
V2 fixup can resolve the redefinition error, but need to use macro.
We can see that the name of macro is not replace with it's value in
"__mcfg_fixup_EFI_ACPI_HISI_OEM_IDEFI_ACPI_HISI_D03_OEM_TABLE_IDPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY".
Any good idea is appreciated.
Hmmm.
I was testing # op and using min_t to get the min-len when doing
strncmp similar to Chris' suggestion (using min_t avoids type
warnings)
/* Designate a routine to fix up buggy MCFG */
#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, rev, dom, bus) \
static const struct pci_cfg_fixup
__mcfg_fixup##oem_id##oem_table_id##rev##dom##bus\
__used __attribute__((__section__(".acpi_fixup_mcfg"), \
aligned((sizeof(void *))))) = \
{ ops, #oem_id, #oem_table_id, rev, dom, bus };
This should change to { ops, oem_id, oem_table_id, rev, dom, bus };
?#? is not need.
My fixup definition:
DECLARE_ACPI_MCFG_FIXUP(&xgene_pcie_ecam_ops, APM, XGENE, 2,
PCI_MCFG_DOMAIN_ANY,
PCI_MCFG_BUS_ANY);
My match condition is:
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->header.oem_id,
min_t(size_t, strlen(f->oem_id),
ACPI_OEM_ID_SIZE))) &&
(!strncmp(f->oem_table_id, mcfg->header.oem_table_id,
min_t(size_t, strlen(f->oem_table_id),
ACPI_OEM_TABLE_ID_SIZE))) &&
(f->oem_revision == mcfg->header.oem_revision)) {
return f->ops;
}
But this still does not work for your case as having HISI-D02 as
oem_table_id will cause error.
In my case, I have tested and it worked ok.
Could you show the detail error information that you met?
Thanks
Dongdong
quoted
Thanks
Dongdong
quoted
Regards,
Duc Dang.
quoted
^
arch/arm64/kernel/pci.c:225:1: note: in expansion of macro
?DECLARE_ACPI_MCFG_FIXUP?
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432",
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
arch/arm64/kernel/pci.c:225:44: error: pasting ""QCOM"" and ""QDF2432""
does not give a valid preprocessing token
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432",
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:17: note: in definition of macro
?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
arch/arm64/kernel/pci.c:225:52: error: pasting ""QDF2432"" and
"PCI_MCFG_DOMAIN_ANY" does not give a valid preprocessi
ng token
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432",
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:25: note: in definition of macro
?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
arch/arm64/kernel/pci.c:225:44: error: expected ?=?, ?,?, ?;?, ?asm? or
?__attribute__? before string constant
DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432",
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
^
include/linux/pci-acpi.h:90:17: note: in definition of macro
?DECLARE_ACPI_MCFG_FIXUP?
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
^
make[1]: *** [arch/arm64/kernel/pci.o] Error 1
make: *** [arch/arm64/kernel] Error 2
1. https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html#Concatenation
Thanks,
Cov
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
@@ -22,6 +22,10 @@#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 */+staticstructacpi_table_mcfg*mcfg_table;/* Structure to hold entries from the MCFG table */structmcfg_entry{
@@ -35,6 +39,38 @@ struct mcfg_entry {/* List to save mcfg entries */staticLIST_HEAD(pci_mcfg_list);+externstructpci_cfg_fixup__start_acpi_mcfg_fixups[];+externstructpci_cfg_fixup__end_acpi_mcfg_fixups[];++structpci_ecam_ops*pci_mcfg_get_ops(structacpi_pci_root*root)+{+intbus_num=root->secondary.start;+intdomain=root->segment;+structpci_cfg_fixup*f;++if(!mcfg_table)+return&pci_generic_ecam_ops;++/*+*Matchagainstplatformspecificquirksandreturn
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)) &&
+ (!strncmp(f->oem_table_id,
mcfg_table->header.oem_table_id,
+ ACPI_OEM_TABLE_ID_SIZE)))
This would just be a small convenience, but if the character count
used
here were
min(strlen(f->oem_id), ACPI_OEM_ID_SIZE)
then the parameters to DECLARE_ACPI_MCFG_FIXUP macro could be
substrings
and
wouldn't need to be padded out to the full length.
quoted
+ return f->ops;
+ }
+ /* No quirks, use ECAM */
+ return &pci_generic_ecam_ops;
+}
I'm not entirely sure that this is the right fix--I'm pretty blindly
following a GCC documentation suggestion [1]--but removing the
first two
preprocessor concatenation operators "##" solved the following build
error
for me.
include/linux/pci-acpi.h:90:2: error: pasting "__mcfg_fixup_" and
""QCOM"" does not give a valid preprocessing token
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
I think the problem is gcc is not happy with quoted string when
processing these tokens
(""QCOM"", the extra "" are added by gcc). So should we not concat
string tokens and
use the fixup definition in v1 of this RFC:
/* 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 };
V1 fixup exist the redefinition error when compiling mutiple
DECLARE_ACPI_MCFG_FIXUP
with the same PCI_MCFG_DOMAIN_ANY and PCI_MCFG_BUS_ANY.
#define EFI_ACPI_HISI_OEM_ID "HISI"
#define EFI_ACPI_HISI_D02_OEM_TABLE_ID "HISI-D02"
#define EFI_ACPI_HISI_D03_OEM_TABLE_ID "HISI-D03"
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
EFI_ACPI_HISI_D02_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY,
PCI_MCFG_BUS_ANY);
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
EFI_ACPI_HISI_D03_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY,
PCI_MCFG_BUS_ANY);
In file included from drivers/pci/host/pcie-hisi-acpi.c:15:0:
include/linux/pci-acpi.h:98:43: error: redefinition of
'__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY'
static const struct pci_cfg_fixup
__mcfg_fixup_##system##dom##bus\
^
drivers/pci/host/pcie-hisi-acpi.c:215:1: note: in expansion of macro
'DECLARE_ACPI_MCFG_FIXUP'
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
^
include/linux/pci-acpi.h:98:43: note: previous definition of
'__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY' was here
static const struct pci_cfg_fixup
__mcfg_fixup_##system##dom##bus\
^
drivers/pci/host/pcie-hisi-acpi.c:212:1: note: in expansion of macro
'DECLARE_ACPI_MCFG_FIXUP'
V2 fixup can resolve the redefinition error, but need to use macro.
We can see that the name of macro is not replace with it's value in
"__mcfg_fixup_EFI_ACPI_HISI_OEM_IDEFI_ACPI_HISI_D03_OEM_TABLE_IDPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY".
Any good idea is appreciated.
Hmmm.
I was testing # op and using min_t to get the min-len when doing
strncmp similar to Chris' suggestion (using min_t avoids type
warnings)
/* Designate a routine to fix up buggy MCFG */
#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, rev, dom,
bus) \
static const struct pci_cfg_fixup
__mcfg_fixup##oem_id##oem_table_id##rev##dom##bus\
__used
__attribute__((__section__(".acpi_fixup_mcfg"), \
aligned((sizeof(void *)))))
= \
quoted
{ ops, #oem_id, #oem_table_id, rev, dom, bus };
This should change to { ops, oem_id, oem_table_id, rev, dom, bus };
?#? is not need.
Both solutions are OK.
1. This works when we use macros as OEM ID and OEM table ID:
#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, rev, dom, bus)\
static const struct pci_cfg_fixup \
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
__used __attribute__((__section__(".acpi_fixup_mcfg"), \
aligned((sizeof(void *))))) = \
{ ops, oem_id, oem_table_id, rev, dom, bus };
#define OEM_ID "XXXXXX"
#define OEM_TABLE_ID "YYYYYYYY"
DECLARE_ACPI_MCFG_FIXUP(&quirk_ops, OEM_ID, OEM_TABLE_ID, 1,
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
2. This works w/o macro which means we need to define OEM ID and OEM as
string w/o quotation marks:
#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, rev, dom, bus)\
static const struct pci_cfg_fixup \
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
__used __attribute__((__section__(".acpi_fixup_mcfg"), \
aligned((sizeof(void *))))) = \
{ ops, #oem_id, #oem_table_id, rev, dom, bus };
DECLARE_ACPI_MCFG_FIXUP(&quirk_ops, XXXXXX, YYYYYYYY, 1,
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
Personally I think that (2) is better, no need for macro definitions.
Tomasz
@@ -22,6 +22,10 @@#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 */+staticstructacpi_table_mcfg*mcfg_table;/* Structure to hold entries from the MCFG table */structmcfg_entry{
@@ -35,6 +39,38 @@ struct mcfg_entry {/* List to save mcfg entries */staticLIST_HEAD(pci_mcfg_list);+externstructpci_cfg_fixup__start_acpi_mcfg_fixups[];+externstructpci_cfg_fixup__end_acpi_mcfg_fixups[];++structpci_ecam_ops*pci_mcfg_get_ops(structacpi_pci_root*root)+{+intbus_num=root->secondary.start;+intdomain=root->segment;+structpci_cfg_fixup*f;++if(!mcfg_table)+return&pci_generic_ecam_ops;++/*+*Matchagainstplatformspecificquirksandreturn
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)) &&
+ (!strncmp(f->oem_table_id,
mcfg_table->header.oem_table_id,
+ ACPI_OEM_TABLE_ID_SIZE)))
This would just be a small convenience, but if the character count
used
here were
min(strlen(f->oem_id), ACPI_OEM_ID_SIZE)
then the parameters to DECLARE_ACPI_MCFG_FIXUP macro could be
substrings
and
wouldn't need to be padded out to the full length.
quoted
+ return f->ops;
+ }
+ /* No quirks, use ECAM */
+ return &pci_generic_ecam_ops;
+}
I'm not entirely sure that this is the right fix--I'm pretty blindly
following a GCC documentation suggestion [1]--but removing the
first two
preprocessor concatenation operators "##" solved the following build
error
for me.
include/linux/pci-acpi.h:90:2: error: pasting "__mcfg_fixup_" and
""QCOM"" does not give a valid preprocessing token
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
I think the problem is gcc is not happy with quoted string when
processing these tokens
(""QCOM"", the extra "" are added by gcc). So should we not concat
string tokens and
use the fixup definition in v1 of this RFC:
/* 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 };
V1 fixup exist the redefinition error when compiling mutiple
DECLARE_ACPI_MCFG_FIXUP
with the same PCI_MCFG_DOMAIN_ANY and PCI_MCFG_BUS_ANY.
#define EFI_ACPI_HISI_OEM_ID "HISI"
#define EFI_ACPI_HISI_D02_OEM_TABLE_ID "HISI-D02"
#define EFI_ACPI_HISI_D03_OEM_TABLE_ID "HISI-D03"
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
EFI_ACPI_HISI_D02_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY,
PCI_MCFG_BUS_ANY);
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
EFI_ACPI_HISI_D03_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY,
PCI_MCFG_BUS_ANY);
In file included from drivers/pci/host/pcie-hisi-acpi.c:15:0:
include/linux/pci-acpi.h:98:43: error: redefinition of
'__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY'
static const struct pci_cfg_fixup
__mcfg_fixup_##system##dom##bus\
^
drivers/pci/host/pcie-hisi-acpi.c:215:1: note: in expansion of macro
'DECLARE_ACPI_MCFG_FIXUP'
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
^
include/linux/pci-acpi.h:98:43: note: previous definition of
'__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY' was here
static const struct pci_cfg_fixup
__mcfg_fixup_##system##dom##bus\
^
drivers/pci/host/pcie-hisi-acpi.c:212:1: note: in expansion of macro
'DECLARE_ACPI_MCFG_FIXUP'
V2 fixup can resolve the redefinition error, but need to use macro.
We can see that the name of macro is not replace with it's value in
"__mcfg_fixup_EFI_ACPI_HISI_OEM_IDEFI_ACPI_HISI_D03_OEM_TABLE_IDPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY".
Any good idea is appreciated.
Hmmm.
I was testing # op and using min_t to get the min-len when doing
strncmp similar to Chris' suggestion (using min_t avoids type
warnings)
/* Designate a routine to fix up buggy MCFG */
#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, rev, dom,
bus) \
static const struct pci_cfg_fixup
__mcfg_fixup##oem_id##oem_table_id##rev##dom##bus\
__used
__attribute__((__section__(".acpi_fixup_mcfg"), \
aligned((sizeof(void *)))))
= \
quoted
{ ops, #oem_id, #oem_table_id, rev, dom, bus };
This should change to { ops, oem_id, oem_table_id, rev, dom, bus };
?#? is not need.
Both solutions are OK.
1. This works when we use macros as OEM ID and OEM table ID:
#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, rev, dom, bus)\
static const struct pci_cfg_fixup \
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
__used __attribute__((__section__(".acpi_fixup_mcfg"), \
aligned((sizeof(void *))))) = \
{ ops, oem_id, oem_table_id, rev, dom, bus };
#define OEM_ID "XXXXXX"
#define OEM_TABLE_ID "YYYYYYYY"
DECLARE_ACPI_MCFG_FIXUP(&quirk_ops, OEM_ID, OEM_TABLE_ID, 1,
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
2. This works w/o macro which means we need to define OEM ID and OEM as
string w/o quotation marks:
#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, rev, dom, bus)\
static const struct pci_cfg_fixup \
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
__used __attribute__((__section__(".acpi_fixup_mcfg"), \
aligned((sizeof(void *))))) = \
{ ops, #oem_id, #oem_table_id, rev, dom, bus };
DECLARE_ACPI_MCFG_FIXUP(&quirk_ops, XXXXXX, YYYYYYYY, 1,
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
In case of oem_id and oem_table_id have special characters ("HISI-D02"
as an example), #2 will have problem unless we use macro definitions
for them.
Personally I think that (2) is better, no need for macro definitions.
Tomasz
From: Jon Masters <hidden> Date: 2016-06-16 06:32:13
On 06/13/2016 09:54 AM, Gabriele Paoloni wrote:
Hi Tomasz, Jon
Hi Gab,
Sorry for the lag in following up.
<snip>
As you can see here Liudongdong has replaced oem_revision with
oem_table_id.
Now it seems that there are some platforms that have already shipped
using a matching based on the oem_revision (right Jon?)
Actually, it turns out (Cov is correct) that we can just match on OEM
Table ID. The revision should not generally be needed and the vendors
will just need to make sure that they change OEM Table ID in future
silicon. An example from two shipping platforms:
1). AppliedMicro Mustang:
[000h 0000 4] Signature : "MCFG" [Memory Mapped
Configuration table]
[004h 0004 4] Table Length : 0000003C
[008h 0008 1] Revision : 01
[009h 0009 1] Checksum : 4A
[00Ah 0010 6] Oem ID : "APM "
[010h 0016 8] Oem Table ID : "XGENE "
[018h 0024 4] Oem Revision : 00000002
[01Ch 0028 4] Asl Compiler ID : "INTL"
[020h 0032 4] Asl Compiler Revision : 20140724
2). HP(E[0]) Moonshot:
[000h 0000 4] Signature : "MCFG" [Memory Mapped
Configuration table]
[004h 0004 4] Table Length : 0000003C
[008h 0008 1] Revision : 01
[009h 0009 1] Checksum : 48
[00Ah 0010 6] Oem ID : "APM "
[010h 0016 8] Oem Table ID : "XGENE "
[018h 0024 4] Oem Revision : 00000001
[01Ch 0028 4] Asl Compiler ID : "HP "
[020h 0032 4] Asl Compiler Revision : 00000001
I have pinged the semiconductor (Applied) and insisted upon a written
plan (which I have now) for handling the first affect generation(s) and
future chip roadmap stuff, along with how they plan to upstream this
immediately as part of this thread. I have also done similar with each
of the other vendors (this is something ARM or Linaro should be doing).
But I particularly care about X-Gene because I want it to be loved as
shipping silicon in production systems (Moonshot) that are sitting and
waiting in the Fedora Phoenix datacenter in large quantity to come
online if only an upstream kernel would actually boot on them :)
However I guess that if in FW they have defined oem_table_id properly
they should be able to use this mechanism without needing to a FW update.
Correct.
Can these vendors confirm this?
I've checked all current shipping silicon and prototypes.
Tomasz do you think this can work for Cavium Thunder?
Will let the vendors followup directly as well.
Jon.
[0] Fortunately that name change doesn't factor when using semiconductor
matching...hopefully none of the non-complaint IP companies in gen1
stuff get bought and change their table names. In the unlikely event
that that does happen, I will preemptively beat them up and ensure that
something crazy doesn't happen with table contents.
--
Computer Architect | Sent from my Fedora powered laptop
On Wed, Jun 15, 2016 at 11:31 PM, Jon Masters [off-list ref] wrote:
On 06/13/2016 09:54 AM, Gabriele Paoloni wrote:
quoted
Hi Tomasz, Jon
Hi Gab,
Sorry for the lag in following up.
<snip>
quoted
As you can see here Liudongdong has replaced oem_revision with
oem_table_id.
Now it seems that there are some platforms that have already shipped
using a matching based on the oem_revision (right Jon?)
Actually, it turns out (Cov is correct) that we can just match on OEM
Table ID. The revision should not generally be needed and the vendors
will just need to make sure that they change OEM Table ID in future
silicon. An example from two shipping platforms:
1). AppliedMicro Mustang:
[000h 0000 4] Signature : "MCFG" [Memory Mapped
Configuration table]
[004h 0004 4] Table Length : 0000003C
[008h 0008 1] Revision : 01
[009h 0009 1] Checksum : 4A
[00Ah 0010 6] Oem ID : "APM "
[010h 0016 8] Oem Table ID : "XGENE "
[018h 0024 4] Oem Revision : 00000002
[01Ch 0028 4] Asl Compiler ID : "INTL"
[020h 0032 4] Asl Compiler Revision : 20140724
2). HP(E[0]) Moonshot:
[000h 0000 4] Signature : "MCFG" [Memory Mapped
Configuration table]
[004h 0004 4] Table Length : 0000003C
[008h 0008 1] Revision : 01
[009h 0009 1] Checksum : 48
[00Ah 0010 6] Oem ID : "APM "
[010h 0016 8] Oem Table ID : "XGENE "
[018h 0024 4] Oem Revision : 00000001
[01Ch 0028 4] Asl Compiler ID : "HP "
[020h 0032 4] Asl Compiler Revision : 00000001
I have pinged the semiconductor (Applied) and insisted upon a written
plan (which I have now) for handling the first affect generation(s) and
future chip roadmap stuff, along with how they plan to upstream this
immediately as part of this thread. I have also done similar with each
of the other vendors (this is something ARM or Linaro should be doing).
But I particularly care about X-Gene because I want it to be loved as
shipping silicon in production systems (Moonshot) that are sitting and
waiting in the Fedora Phoenix datacenter in large quantity to come
online if only an upstream kernel would actually boot on them :)
Thanks for the MCFG information on Moonshot system, Jon.
I will make sure the posted quirk for X-Gene takes care for HPE
Moonshot system as well.
Regards,
Duc Dang.
quoted
However I guess that if in FW they have defined oem_table_id properly
they should be able to use this mechanism without needing to a FW update.
Correct.
quoted
Can these vendors confirm this?
I've checked all current shipping silicon and prototypes.
quoted
Tomasz do you think this can work for Cavium Thunder?
Will let the vendors followup directly as well.
Jon.
[0] Fortunately that name change doesn't factor when using semiconductor
matching...hopefully none of the non-complaint IP companies in gen1
stuff get bought and change their table names. In the unlikely event
that that does happen, I will preemptively beat them up and ensure that
something crazy doesn't happen with table contents.
--
Computer Architect | Sent from my Fedora powered laptop
From: Jon Masters <hidden> Date: 2016-06-16 07:55:41
:) We should be good with a match on XGENE (but be careful with substring matching) as long as future platforms change that to eg XGENE3 (which is a publicly announced future chip). What you want to avoid is a shorter match later triggering on a future generation.
There are a lot of folks eagerly awaiting Moonshot running an upstream kernel in F25, both for 64 and 32-bit (VMs running 32-bit can replace older builders). I can picture a wonderful world in which this whole ARM server ecosystem works properly with folks doing what they should have three years ago and development happening on upstream kernels with ACPI. Then things become incredibly dull just like x86 - do the dev against upstream, pull into an enterprise distro after it is tested out in a Fedora cycle...and no random nonsense patches. Next thing we know there will be media reports covering silicon that won't end with a rant about how they couldn't get the right firmware and hacked up Ubuntu kernel booting.
Best, and goodnight :)
Jon.
--
Computer Architect | Sent from my 64-bit #ARM Powered phone
On Jun 16, 2016, at 03:46, Duc Dang [off-list ref] wrote:
quoted
On Wed, Jun 15, 2016 at 11:31 PM, Jon Masters [off-list ref] wrote:
quoted
On 06/13/2016 09:54 AM, Gabriele Paoloni wrote:
Hi Tomasz, Jon
Hi Gab,
Sorry for the lag in following up.
<snip>
quoted
As you can see here Liudongdong has replaced oem_revision with
oem_table_id.
Now it seems that there are some platforms that have already shipped
using a matching based on the oem_revision (right Jon?)
Actually, it turns out (Cov is correct) that we can just match on OEM
Table ID. The revision should not generally be needed and the vendors
will just need to make sure that they change OEM Table ID in future
silicon. An example from two shipping platforms:
1). AppliedMicro Mustang:
[000h 0000 4] Signature : "MCFG" [Memory Mapped
Configuration table]
[004h 0004 4] Table Length : 0000003C
[008h 0008 1] Revision : 01
[009h 0009 1] Checksum : 4A
[00Ah 0010 6] Oem ID : "APM "
[010h 0016 8] Oem Table ID : "XGENE "
[018h 0024 4] Oem Revision : 00000002
[01Ch 0028 4] Asl Compiler ID : "INTL"
[020h 0032 4] Asl Compiler Revision : 20140724
2). HP(E[0]) Moonshot:
[000h 0000 4] Signature : "MCFG" [Memory Mapped
Configuration table]
[004h 0004 4] Table Length : 0000003C
[008h 0008 1] Revision : 01
[009h 0009 1] Checksum : 48
[00Ah 0010 6] Oem ID : "APM "
[010h 0016 8] Oem Table ID : "XGENE "
[018h 0024 4] Oem Revision : 00000001
[01Ch 0028 4] Asl Compiler ID : "HP "
[020h 0032 4] Asl Compiler Revision : 00000001
I have pinged the semiconductor (Applied) and insisted upon a written
plan (which I have now) for handling the first affect generation(s) and
future chip roadmap stuff, along with how they plan to upstream this
immediately as part of this thread. I have also done similar with each
of the other vendors (this is something ARM or Linaro should be doing).
But I particularly care about X-Gene because I want it to be loved as
shipping silicon in production systems (Moonshot) that are sitting and
waiting in the Fedora Phoenix datacenter in large quantity to come
online if only an upstream kernel would actually boot on them :)
Thanks for the MCFG information on Moonshot system, Jon.
I will make sure the posted quirk for X-Gene takes care for HPE
Moonshot system as well.
Regards,
Duc Dang.
quoted
quoted
However I guess that if in FW they have defined oem_table_id properly
they should be able to use this mechanism without needing to a FW update.
Correct.
quoted
Can these vendors confirm this?
I've checked all current shipping silicon and prototypes.
quoted
Tomasz do you think this can work for Cavium Thunder?
Will let the vendors followup directly as well.
Jon.
[0] Fortunately that name change doesn't factor when using semiconductor
matching...hopefully none of the non-complaint IP companies in gen1
stuff get bought and change their table names. In the unlikely event
that that does happen, I will preemptively beat them up and ensure that
something crazy doesn't happen with table contents.
--
Computer Architect | Sent from my Fedora powered laptop