ECAM quirks handling for ARM64 platforms

Some platforms may not be fully compliant with generic set of PCI config accessors. For these cases we implement the way to overwrite CFG accessors set and configuration space range.

In first place pci_mcfg_parse() saves machine's IDs and revision number (these come from MCFG header) in order to match against known quirk entries. Then the algorithm traverses available quirk list (static array), matches against <oem_id, oem_table_id, rev, domain, bus number range> and returns custom PCI config ops and/or CFG resource structure.

When adding new quirk there are two possibilities:
1. Override default pci_generic_ecam_ops ops but CFG resource comes from MCFG { "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &foo_ops, MCFG_RES_EMPTY },
2. Override default pci_generic_ecam_ops ops and CFG resource. For this case it is also allowed get CFG resource from quirk entry w/o having it in MCFG. { "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &boo_ops, DEFINE_RES_MEM(START, SIZE) },

pci_generic_ecam_ops and MCFG entries will be used for platforms free from quirks.

Signed-off-by Tomasz Nowicki <[email protected]>

Signed-off-by Dongdong Liu <[email protected]>

Signed-off-by Christopher Covington <[email protected]>

---
 drivers/acpi/pci_mcfg.c | 80 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 6 deletions(-)

drivers/acpi/pci_mcfg.c +28 -0

index ffcc651..2b8acc7 100644
@@ -32,6 +32,59 @@ struct mcfg_entry {
        u8                      bus_start;
        u8                      bus_end;
 };
+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;
+       struct resource bus_range;
+       struct pci_ecam_ops *ops;
+       struct resource cfgres;
+};
+
+#define MCFG_DOM_ANY                   (-1)
+#define MCFG_BUS_RANGE(start, end)     DEFINE_RES_NAMED((start),       \
+                                               ((end) - (start) + 1),  \
+                                               NULL, IORESOURCE_BUS)
+#define MCFG_BUS_ANY           MCFG_BUS_RANGE(0x0, 0xff)
+#define MCFG_RES_EMPTY         DEFINE_RES_NAMED(0, 0, NULL, 0)
+
+static struct mcfg_fixup mcfg_quirks[] = {
+/*     { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */
+};
+
+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,
+                                 struct resource *cfgres,
+                                 struct pci_ecam_ops **ecam_ops)

drivers/acpi/pci_mcfg.c +36 -7

+                   resource_contains(&f->bus_range, &root->secondary) &&
+                   !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) {
+                       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);
+                       return;
+               }
+       }
+}
 
 /* List to save MCFG entries */
 static LIST_HEAD(pci_mcfg_list);

@@ -61,14 +114,24 @@ int pci_mcfg_lookup(struct acpi_pci_root *root, struct resource *cfgres,
 
        }
 
-       if (!root->mcfg_addr)
-               return -ENXIO;
-
 skip_lookup:
        memset(&res, 0, sizeof(res));
-       res.start = root->mcfg_addr + (bus_res->start << 20);
-       res.end = res.start + (resource_size(bus_res) << 20) - 1;
-       res.flags = IORESOURCE_MEM;
+       if (root->mcfg_addr) {
+               res.start = root->mcfg_addr + (bus_res->start << 20);
+               res.end = res.start + (resource_size(bus_res) << 20) - 1;
+               res.flags = IORESOURCE_MEM;
+       }
+
+       /*
+        * Let to override default ECAM ops and CFG resource range.
+        * Also, this might even retrieve CFG resource range in case 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);
+       if (!res.start)
+               return -ENXIO;
+
        *cfgres = res;
        *ecam_ops = ops;
        return 0;

@@ -101,6 +164,11 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
                list_add(&e->list, &pci_mcfg_list);
        }
 
+       /* Save MCFG IDs and revision for quirks matching */
+       memcpy(mcfg_oem_id, header->oem_id, ACPI_OEM_ID_SIZE);
+       memcpy(mcfg_oem_table_id, header->oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
+       mcfg_oem_revision = header->revision;
+
        pr_info("MCFG table detected, %d entries\n", n);
        return 0;
 }
--

1.9.1

Quoted 65 lines · 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 CFG accessors set and configuration space range.

In first place pci_mcfg_parse() saves machine's IDs and revision number (these come from MCFG header) in order to match against known quirk entries. Then the algorithm traverses available quirk list (static array), matches against <oem_id, oem_table_id, rev, domain, bus number range> and returns custom PCI config ops and/or CFG resource structure.

When adding new quirk there are two possibilities:
1. Override default pci_generic_ecam_ops ops but CFG resource comes from MCFG { "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &foo_ops, MCFG_RES_EMPTY },
2. Override default pci_generic_ecam_ops ops and CFG resource. For this case it is also allowed get CFG resource from quirk entry w/o having it in MCFG. { "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &boo_ops, DEFINE_RES_MEM(START, SIZE) },

pci_generic_ecam_ops and MCFG entries will be used for platforms free from quirks.

Signed-off-by Tomasz Nowicki <[email protected]>

Signed-off-by Dongdong Liu <[email protected]>

Signed-off-by Christopher Covington <[email protected]>

---
 drivers/acpi/pci_mcfg.c | 80 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 6 deletions(-)

drivers/acpi/pci_mcfg.c +28 -0

index ffcc651..2b8acc7 100644
@@ -32,6 +32,59 @@ struct mcfg_entry {
        u8                      bus_start;
        u8                      bus_end;
 };
+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;
+       struct resource bus_range;
+       struct pci_ecam_ops *ops;
+       struct resource cfgres;
+};
+
+#define MCFG_DOM_ANY                   (-1)
+#define MCFG_BUS_RANGE(start, end)     DEFINE_RES_NAMED((start),       \
+                                               ((end) - (start) + 1),  \
+                                               NULL, IORESOURCE_BUS)
+#define MCFG_BUS_ANY           MCFG_BUS_RANGE(0x0, 0xff)
+#define MCFG_RES_EMPTY         DEFINE_RES_NAMED(0, 0, NULL, 0)
+
+static struct mcfg_fixup mcfg_quirks[] = {
+/*     { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */
+};
+
+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,
+                                 struct resource *cfgres,
+                                 struct pci_ecam_ops **ecam_ops)

drivers/acpi/pci_mcfg.c struct mcfg_entry { · line 72

Quoted 10 lines

drivers/acpi/pci_mcfg.c +10 -0

+{
+       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 (f->seg == root->segment &&

Is dropping the comparison with MCFG_DOM_ANY intended? It is useful if all the controllers (segs) can use the same quirk (X-Gene case). If you decide to drop it, then we can remove MCFG_DOM_ANY definition as well.

Quoted 53 lines

drivers/acpi/pci_mcfg.c +31 -6

+                   resource_contains(&f->bus_range, &root->secondary) &&
+                   !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) {
+                       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);
+                       return;
+               }
+       }
+}
 
 /* List to save MCFG entries */
 static LIST_HEAD(pci_mcfg_list);

@@ -61,14 +114,24 @@ int pci_mcfg_lookup(struct acpi_pci_root *root, struct resource *cfgres,
 
        }
 
-       if (!root->mcfg_addr)
-               return -ENXIO;
-
 skip_lookup:
        memset(&res, 0, sizeof(res));
-       res.start = root->mcfg_addr + (bus_res->start << 20);
-       res.end = res.start + (resource_size(bus_res) << 20) - 1;
-       res.flags = IORESOURCE_MEM;
+       if (root->mcfg_addr) {
+               res.start = root->mcfg_addr + (bus_res->start << 20);
+               res.end = res.start + (resource_size(bus_res) << 20) - 1;
+               res.flags = IORESOURCE_MEM;
+       }
+
+       /*
+        * Let to override default ECAM ops and CFG resource range.
+        * Also, this might even retrieve CFG resource range in case 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);
+       if (!res.start)
+               return -ENXIO;
+
        *cfgres = res;
        *ecam_ops = ops;
        return 0;

@@ -101,6 +164,11 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
                list_add(&e->list, &pci_mcfg_list);
        }
 

drivers/acpi/pci_mcfg.c static __init int pci_mcfg_parse(struct acpi_table_header *header) · line 170

Quoted 4 lines

drivers/acpi/pci_mcfg.c +4 -0

+       /* Save MCFG IDs and revision for quirks matching */
+       memcpy(mcfg_oem_id, header->oem_id, ACPI_OEM_ID_SIZE);
+       memcpy(mcfg_oem_table_id, header->oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
+       mcfg_oem_revision = header->revision;

Quoted 6 lines

drivers/acpi/pci_mcfg.c +1 -1

+
        pr_info("MCFG table detected, %d entries\n", n);
        return 0;
 }
--

1.9.1

Regards,
Duc Dang.

Quoted 134 lines · Duc Dang

Quoted 65 lines · 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 CFG accessors set and configuration space range.

In first place pci_mcfg_parse() saves machine's IDs and revision number (these come from MCFG header) in order to match against known quirk entries. Then the algorithm traverses available quirk list (static array), matches against <oem_id, oem_table_id, rev, domain, bus number range> and returns custom PCI config ops and/or CFG resource structure.

When adding new quirk there are two possibilities:
1. Override default pci_generic_ecam_ops ops but CFG resource comes from MCFG { "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &foo_ops, MCFG_RES_EMPTY },
2. Override default pci_generic_ecam_ops ops and CFG resource. For this case it is also allowed get CFG resource from quirk entry w/o having it in MCFG. { "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &boo_ops, DEFINE_RES_MEM(START, SIZE) },

pci_generic_ecam_ops and MCFG entries will be used for platforms free from quirks.

Signed-off-by Tomasz Nowicki <[email protected]>

Signed-off-by Dongdong Liu <[email protected]>

Signed-off-by Christopher Covington <[email protected]>

---
 drivers/acpi/pci_mcfg.c | 80 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 6 deletions(-)

drivers/acpi/pci_mcfg.c +28 -0

index ffcc651..2b8acc7 100644
@@ -32,6 +32,59 @@ struct mcfg_entry {
        u8                      bus_start;
        u8                      bus_end;
 };
+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;
+       struct resource bus_range;
+       struct pci_ecam_ops *ops;
+       struct resource cfgres;
+};
+
+#define MCFG_DOM_ANY                   (-1)
+#define MCFG_BUS_RANGE(start, end)     DEFINE_RES_NAMED((start),       \
+                                               ((end) - (start) + 1),  \
+                                               NULL, IORESOURCE_BUS)
+#define MCFG_BUS_ANY           MCFG_BUS_RANGE(0x0, 0xff)
+#define MCFG_RES_EMPTY         DEFINE_RES_NAMED(0, 0, NULL, 0)
+
+static struct mcfg_fixup mcfg_quirks[] = {
+/*     { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */
+};
+
+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,
+                                 struct resource *cfgres,
+                                 struct pci_ecam_ops **ecam_ops)

drivers/acpi/pci_mcfg.c struct mcfg_entry { · line 72

Quoted 10 lines

drivers/acpi/pci_mcfg.c +10 -0

+{
+       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 (f->seg == root->segment &&

Is dropping the comparison with MCFG_DOM_ANY intended? It is useful if all the controllers (segs) can use the same quirk (X-Gene case). If you decide to drop it, then we can remove MCFG_DOM_ANY definition as well.

Quoted 53 lines

drivers/acpi/pci_mcfg.c +31 -6

+                   resource_contains(&f->bus_range, &root->secondary) &&
+                   !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) {
+                       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);
+                       return;
+               }
+       }
+}
 
 /* List to save MCFG entries */
 static LIST_HEAD(pci_mcfg_list);

@@ -61,14 +114,24 @@ int pci_mcfg_lookup(struct acpi_pci_root *root, struct resource *cfgres,
 
        }
 
-       if (!root->mcfg_addr)
-               return -ENXIO;
-
 skip_lookup:
        memset(&res, 0, sizeof(res));
-       res.start = root->mcfg_addr + (bus_res->start << 20);
-       res.end = res.start + (resource_size(bus_res) << 20) - 1;
-       res.flags = IORESOURCE_MEM;
+       if (root->mcfg_addr) {
+               res.start = root->mcfg_addr + (bus_res->start << 20);
+               res.end = res.start + (resource_size(bus_res) << 20) - 1;
+               res.flags = IORESOURCE_MEM;
+       }
+
+       /*
+        * Let to override default ECAM ops and CFG resource range.
+        * Also, this might even retrieve CFG resource range in case 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);
+       if (!res.start)
+               return -ENXIO;
+
        *cfgres = res;
        *ecam_ops = ops;
        return 0;

@@ -101,6 +164,11 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
                list_add(&e->list, &pci_mcfg_list);
        }
 

Quoted 4 lines

drivers/acpi/pci_mcfg.c static __init int pci_mcfg_parse(struct acpi_table_header *header) · line 170

Quoted 4 lines

drivers/acpi/pci_mcfg.c +4 -0

+       /* Save MCFG IDs and revision for quirks matching */
+       memcpy(mcfg_oem_id, header->oem_id, ACPI_OEM_ID_SIZE);
+       memcpy(mcfg_oem_table_id, header->oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
+       mcfg_oem_revision = header->revision;

I think this is a typo, it should be:

               mcfg_oem_revision = header->oem_revision;

Quoted 70 lines · 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 CFG accessors set and configuration space range.

In first place pci_mcfg_parse() saves machine's IDs and revision number (these come from MCFG header) in order to match against known quirk entries. Then the algorithm traverses available quirk list (static array), matches against <oem_id, oem_table_id, rev, domain, bus number range> and returns custom PCI config ops and/or CFG resource structure.

When adding new quirk there are two possibilities:
1. Override default pci_generic_ecam_ops ops but CFG resource comes from MCFG { "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &foo_ops, MCFG_RES_EMPTY },
2. Override default pci_generic_ecam_ops ops and CFG resource. For this case it is also allowed get CFG resource from quirk entry w/o having it in MCFG. { "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &boo_ops, DEFINE_RES_MEM(START, SIZE) },

pci_generic_ecam_ops and MCFG entries will be used for platforms free from quirks.

Signed-off-by Tomasz Nowicki <[email protected]>

Signed-off-by Dongdong Liu <[email protected]>

Signed-off-by Christopher Covington <[email protected]>

---
 drivers/acpi/pci_mcfg.c | 80 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 6 deletions(-)

drivers/acpi/pci_mcfg.c +33 -0

index ffcc651..2b8acc7 100644
@@ -32,6 +32,59 @@ struct mcfg_entry {
        u8                      bus_start;
        u8                      bus_end;
 };
+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;
+       struct resource bus_range;
+       struct pci_ecam_ops *ops;
+       struct resource cfgres;
+};
+
+#define MCFG_DOM_ANY                   (-1)
+#define MCFG_BUS_RANGE(start, end)     DEFINE_RES_NAMED((start),       \
+                                               ((end) - (start) + 1),  \
+                                               NULL, IORESOURCE_BUS)
+#define MCFG_BUS_ANY           MCFG_BUS_RANGE(0x0, 0xff)
+#define MCFG_RES_EMPTY         DEFINE_RES_NAMED(0, 0, NULL, 0)
+
+static struct mcfg_fixup mcfg_quirks[] = {
+/*     { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */
+};
+
+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,
+                                 struct resource *cfgres,
+                                 struct pci_ecam_ops **ecam_ops)
+{
+       struct mcfg_fixup *f;
+       int i;
+
+       /*

drivers/acpi/pci_mcfg.c struct mcfg_entry { · line 72

Quoted 5 lines

drivers/acpi/pci_mcfg.c +5 -0

+        * 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 (f->seg == root->segment &&

Is dropping the comparison with MCFG_DOM_ANY intended? It is useful if all the controllers (segs) can use the same quirk (X-Gene case).

Some platforms may not be fully compliant with generic set of PCI config accessors. For these cases we implement the way to overwrite CFG accessors set and configuration space range.

In first place pci_mcfg_parse() saves machine's IDs and revision number (these come from MCFG header) in order to match against known quirk entries. Then the algorithm traverses available quirk list (static array), matches against <oem_id, oem_table_id, rev, domain, bus number range> and returns custom PCI config ops and/or CFG resource structure.

When adding new quirk there are two possibilities:
1. Override default pci_generic_ecam_ops ops but CFG resource comes from MCFG { "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &foo_ops, MCFG_RES_EMPTY },
2. Override default pci_generic_ecam_ops ops and CFG resource. For this case it is also allowed get CFG resource from quirk entry w/o having it in MCFG. { "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &boo_ops, DEFINE_RES_MEM(START, SIZE) },

pci_generic_ecam_ops and MCFG entries will be used for platforms free from quirks.

Signed-off-by Tomasz Nowicki <[email protected]>

Signed-off-by Dongdong Liu <[email protected]>

Signed-off-by Christopher Covington <[email protected]>

---
  drivers/acpi/pci_mcfg.c | 80 +++++++++++++++++++++++++++++++++++++++++++++----
  1 file changed, 74 insertions(+), 6 deletions(-)

drivers/acpi/pci_mcfg.c +28 -0

index ffcc651..2b8acc7 100644
@@ -32,6 +32,59 @@ struct mcfg_entry {
  	u8			bus_start;
  	u8			bus_end;
  };
+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;
+	struct resource bus_range;
+	struct pci_ecam_ops *ops;
+	struct resource cfgres;
+};
+
+#define MCFG_DOM_ANY			(-1)
+#define MCFG_BUS_RANGE(start, end)	DEFINE_RES_NAMED((start),	\
+						((end) - (start) + 1),	\
+						NULL, IORESOURCE_BUS)
+#define MCFG_BUS_ANY		MCFG_BUS_RANGE(0x0, 0xff)
+#define MCFG_RES_EMPTY		DEFINE_RES_NAMED(0, 0, NULL, 0)
+
+static struct mcfg_fixup mcfg_quirks[] = {
+/*	{ OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */
+};
+
+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,
+				  struct resource *cfgres,
+				  struct pci_ecam_ops **ecam_ops)

drivers/acpi/pci_mcfg.c +36 -6

+		    resource_contains(&f->bus_range, &root->secondary) &&
+		    !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) {
+			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);
+			return;
+		}
+	}
+}
 
  /* List to save MCFG entries */
  static LIST_HEAD(pci_mcfg_list);

@@ -61,14 +114,24 @@ int pci_mcfg_lookup(struct acpi_pci_root *root, struct resource *cfgres,
 
  	}
 
-	if (!root->mcfg_addr)
-		return -ENXIO;
-
  skip_lookup:
  	memset(&res, 0, sizeof(res));
-	res.start = root->mcfg_addr + (bus_res->start << 20);
-	res.end = res.start + (resource_size(bus_res) << 20) - 1;
-	res.flags = IORESOURCE_MEM;
+	if (root->mcfg_addr) {
+		res.start = root->mcfg_addr + (bus_res->start << 20);
+		res.end = res.start + (resource_size(bus_res) << 20) - 1;
+		res.flags = IORESOURCE_MEM;
+	}
+
+	/*
+	 * Let to override default ECAM ops and CFG resource range.
+	 * Also, this might even retrieve CFG resource range in case 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);
+	if (!res.start)
+		return -ENXIO;
+
  	*cfgres = res;
  	*ecam_ops = ops;
  	return 0;

@@ -101,6 +164,11 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
  		list_add(&e->list, &pci_mcfg_list);
  	}
 
+	/* Save MCFG IDs and revision for quirks matching */
+	memcpy(mcfg_oem_id, header->oem_id, ACPI_OEM_ID_SIZE);
+	memcpy(mcfg_oem_table_id, header->oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
+	mcfg_oem_revision = header->revision;
+
  	pr_info("MCFG table detected, %d entries\n", n);
  	return 0;
  }

Hi Tomasz

Quoted 81 lines · 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 CFG accessors
set and configuration space range.

In first place pci_mcfg_parse() saves machine's IDs and revision number (these come from MCFG header) in order to match against known quirk entries.
Then the algorithm traverses available quirk list (static array), matches against <oem_id, oem_table_id, rev, domain, bus number range> and returns custom PCI config ops and/or CFG resource structure.

When adding new quirk there are two possibilities:
1. Override default pci_generic_ecam_ops ops but CFG resource comes from MCFG
{ "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &foo_ops, MCFG_RES_EMPTY },
2. Override default pci_generic_ecam_ops ops and CFG resource. For this case
it is also allowed get CFG resource from quirk entry w/o having it in MCFG.
{ "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &boo_ops,
DEFINE_RES_MEM(START, SIZE) },

pci_generic_ecam_ops and MCFG entries will be used for platforms free from quirks.

Signed-off-by Tomasz Nowicki <[email protected]>

Signed-off-by Dongdong Liu <[email protected]>

Signed-off-by Christopher Covington <[email protected]>

---
  drivers/acpi/pci_mcfg.c | 80
+++++++++++++++++++++++++++++++++++++++++++++----
  1 file changed, 74 insertions(+), 6 deletions(-)

drivers/acpi/pci_mcfg.c +34 -0

index ffcc651..2b8acc7 100644
@@ -32,6 +32,59 @@ struct mcfg_entry {
      u8            bus_start;
      u8            bus_end;
  };
+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;
+    struct resource bus_range;
+    struct pci_ecam_ops *ops;
+    struct resource cfgres;
+};
+
+#define MCFG_DOM_ANY            (-1)
+#define MCFG_BUS_RANGE(start, end)    DEFINE_RES_NAMED((start),    \
+                        ((end) - (start) + 1),    \
+                        NULL, IORESOURCE_BUS)
+#define MCFG_BUS_ANY        MCFG_BUS_RANGE(0x0, 0xff)
+#define MCFG_RES_EMPTY        DEFINE_RES_NAMED(0, 0, NULL, 0)
+
+static struct mcfg_fixup mcfg_quirks[] = {
+/*    { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */
+};
+
+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,
+                  struct resource *cfgres,
+                  struct pci_ecam_ops **ecam_ops)
+{
+    struct mcfg_fixup *f;
+    int i;
+
+    /*
+     * First match against PCI topology <domain:bus> then use OEM ID,

OEM

drivers/acpi/pci_mcfg.c +3 -0

+     * table ID, and OEM revision from MCFG table standard header.
+     */
+    for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++,

Quoted 2 lines

f++) {
+ if (f->seg == root->segment &&

why not use MCFG_DOM_RANGE, I think MCFG_DOM_RANGE is better. if drop MCFG_DOM_RANGE, mcfg_quirks[] will be more complex.

static struct mcfg_fixup mcfg_quirks[] = {
/* { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */

#ifdef CONFIG_PCI_HOST_THUNDER_ECAM
    /* SoC pass1.x */
    { "CAVIUM", "THUNDERX", 2, 0, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 1, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 2, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 3, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 10, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 11, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 12, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 13, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},

#endif
.....
};

As PATCH v5 we only need define mcfg_quirks as below, It looks better. static struct pci_cfg_fixup mcfg_quirks[] __initconst = {
/* { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, pci_ops, init_hook
}, */

#ifdef CONFIG_PCI_HOST_THUNDER_PEM
    /* Pass2.0 */
    { "CAVIUM", "THUNDERX", 1, MCFG_DOM_RANGE(4, 9), MCFG_BUS_ANY, NULL,
      thunder_pem_cfg_init },
    { "CAVIUM", "THUNDERX", 1, MCFG_DOM_RANGE(14, 19), MCFG_BUS_ANY, NULL,
      thunder_pem_cfg_init },
#endif
#ifdef CONFIG_PCI_HISI_ACPI
    { "HISI  ", "HIP05   ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY,
      NULL, hisi_pcie_acpi_hip05_init},
    { "HISI  ", "HIP06   ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY,
      NULL, hisi_pcie_acpi_hip06_init},
    { "HISI  ", "HIP07   ", 0, MCFG_DOM_RANGE(0, 15), MCFG_BUS_ANY,
      NULL, hisi_pcie_acpi_hip07_init},

#endif
};

Hi Liu,

Quoted 132 lines · Dongdong Liu

Hi Tomasz

Quoted 81 lines · 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 CFG accessors
set and configuration space range.

In first place pci_mcfg_parse() saves machine's IDs and revision number (these come from MCFG header) in order to match against known quirk entries.
Then the algorithm traverses available quirk list (static array), matches against <oem_id, oem_table_id, rev, domain, bus number range> and returns custom PCI config ops and/or CFG resource structure.

When adding new quirk there are two possibilities:
1. Override default pci_generic_ecam_ops ops but CFG resource comes from MCFG
{ "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &foo_ops, MCFG_RES_EMPTY },
2. Override default pci_generic_ecam_ops ops and CFG resource. For this case
it is also allowed get CFG resource from quirk entry w/o having it in MCFG.
{ "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &boo_ops,
DEFINE_RES_MEM(START, SIZE) },

pci_generic_ecam_ops and MCFG entries will be used for platforms free from quirks.

Signed-off-by Tomasz Nowicki <[email protected]>

Signed-off-by Dongdong Liu <[email protected]>

Signed-off-by Christopher Covington <[email protected]>

---
  drivers/acpi/pci_mcfg.c | 80
+++++++++++++++++++++++++++++++++++++++++++++----
  1 file changed, 74 insertions(+), 6 deletions(-)

drivers/acpi/pci_mcfg.c +34 -0

index ffcc651..2b8acc7 100644
@@ -32,6 +32,59 @@ struct mcfg_entry {
      u8            bus_start;
      u8            bus_end;
  };
+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;
+    struct resource bus_range;
+    struct pci_ecam_ops *ops;
+    struct resource cfgres;
+};
+
+#define MCFG_DOM_ANY            (-1)
+#define MCFG_BUS_RANGE(start, end)    DEFINE_RES_NAMED((start),    \
+                        ((end) - (start) + 1),    \
+                        NULL, IORESOURCE_BUS)
+#define MCFG_BUS_ANY        MCFG_BUS_RANGE(0x0, 0xff)
+#define MCFG_RES_EMPTY        DEFINE_RES_NAMED(0, 0, NULL, 0)
+
+static struct mcfg_fixup mcfg_quirks[] = {
+/*    { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */
+};
+
+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,
+                  struct resource *cfgres,
+                  struct pci_ecam_ops **ecam_ops)
+{
+    struct mcfg_fixup *f;
+    int i;
+
+    /*
+     * First match against PCI topology <domain:bus> then use OEM ID,

OEM

drivers/acpi/pci_mcfg.c +3 -0

+     * table ID, and OEM revision from MCFG table standard header.
+     */
+    for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++,

Quoted 2 lines

f++) {
+ if (f->seg == root->segment &&

why not use MCFG_DOM_RANGE, I think MCFG_DOM_RANGE is better. if drop MCFG_DOM_RANGE, mcfg_quirks[] will be more complex.

static struct mcfg_fixup mcfg_quirks[] = {
/* { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */

#ifdef CONFIG_PCI_HOST_THUNDER_ECAM
    /* SoC pass1.x */
    { "CAVIUM", "THUNDERX", 2, 0, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 1, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 2, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 3, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 10, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 11, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 12, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},
    { "CAVIUM", "THUNDERX", 2, 13, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
      MCFG_RES_EMPTY},

#endif
.....
};

As PATCH v5 we only need define mcfg_quirks as below, It looks better. static struct pci_cfg_fixup mcfg_quirks[] __initconst = {
/* { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, pci_ops, init_hook
}, */

#ifdef CONFIG_PCI_HOST_THUNDER_PEM
    /* Pass2.0 */
    { "CAVIUM", "THUNDERX", 1, MCFG_DOM_RANGE(4, 9), MCFG_BUS_ANY, NULL,
      thunder_pem_cfg_init },
    { "CAVIUM", "THUNDERX", 1, MCFG_DOM_RANGE(14, 19), MCFG_BUS_ANY, NULL,
      thunder_pem_cfg_init },
#endif
#ifdef CONFIG_PCI_HISI_ACPI
    { "HISI  ", "HIP05   ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY,
      NULL, hisi_pcie_acpi_hip05_init},
    { "HISI  ", "HIP06   ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY,
      NULL, hisi_pcie_acpi_hip06_init},
    { "HISI  ", "HIP07   ", 0, MCFG_DOM_RANGE(0, 15), MCFG_BUS_ANY,
      NULL, hisi_pcie_acpi_hip07_init},

Quoted 2 lines

#endif
};

Note this series disallow hisi_pcie_acpi_hip07_init() call. According to the Bjorn suggestion I rework quirk code to override ops and CFG resources only. Giving that I do not see the way to use MCFG_DOM_RANGE macro. For HISI you would need to get CFG range for each possible case:

#ifdef CONFIG_PCI_HISI_ACPI
     { "HISI  ", "HIP05   ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_ops,
       DEFINE_RES_MEM(start0, size0)},
     { "HISI  ", "HIP05   ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_ops,
       DEFINE_RES_MEM(start1, size1)},
     { "HISI  ", "HIP05   ", 0, 2, MCFG_BUS_ANY, &hisi_pcie_ops,
       DEFINE_RES_MEM(start2, size2)},
     { "HISI  ", "HIP05   ", 0, 3, MCFG_BUS_ANY, &hisi_pcie_ops,
       DEFINE_RES_MEM(start3, size3)},

[...]
#endif

Indeed there are more entries here but you do not have to define the same resource array in driver.

Thanks,
Tomasz

.

Hi Tomasz

?? 2016/9/13 14:32, Tomasz Nowicki ????:

Quoted 149 lines

Hi Liu,

Quoted 132 lines · Dongdong Liu

Hi Tomasz

?? 2016/9/10 3:24, Tomasz Nowicki ????:

Quoted 81 lines

Some platforms may not be fully compliant with generic set of PCI config accessors. For these cases we implement the way to overwrite CFG accessors
set and configuration space range.

In first place pci_mcfg_parse() saves machine's IDs and revision number (these come from MCFG header) in order to match against known quirk entries.
Then the algorithm traverses available quirk list (static array), matches against <oem_id, oem_table_id, rev, domain, bus number range> and returns custom PCI config ops and/or CFG resource structure.

When adding new quirk there are two possibilities:
1. Override default pci_generic_ecam_ops ops but CFG resource comes from MCFG
{ "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &foo_ops, MCFG_RES_EMPTY },
2. Override default pci_generic_ecam_ops ops and CFG resource. For this case
it is also allowed get CFG resource from quirk entry w/o having it in MCFG.
{ "OEM_ID", "OEM_TABLE_ID", <REV>, <DOMAIN>, <BUS_NR>, &boo_ops,
DEFINE_RES_MEM(START, SIZE) },

pci_generic_ecam_ops and MCFG entries will be used for platforms free from quirks.

Signed-off-by Tomasz Nowicki <[email protected]>

Signed-off-by Dongdong Liu <[email protected]>

Signed-off-by Christopher Covington <[email protected]>

---
 drivers/acpi/pci_mcfg.c | 80
+++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 6 deletions(-)

drivers/acpi/pci_mcfg.c +34 -0

index ffcc651..2b8acc7 100644
@@ -32,6 +32,59 @@ struct mcfg_entry {
     u8            bus_start;
     u8            bus_end;
 };
+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;
+    struct resource bus_range;
+    struct pci_ecam_ops *ops;
+    struct resource cfgres;
+};
+
+#define MCFG_DOM_ANY            (-1)
+#define MCFG_BUS_RANGE(start, end)    DEFINE_RES_NAMED((start),    \
+                        ((end) - (start) + 1),    \
+                        NULL, IORESOURCE_BUS)
+#define MCFG_BUS_ANY        MCFG_BUS_RANGE(0x0, 0xff)
+#define MCFG_RES_EMPTY        DEFINE_RES_NAMED(0, 0, NULL, 0)
+
+static struct mcfg_fixup mcfg_quirks[] = {
+/*    { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */
+};
+
+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,
+                  struct resource *cfgres,
+                  struct pci_ecam_ops **ecam_ops)
+{
+    struct mcfg_fixup *f;
+    int i;
+
+    /*
+     * First match against PCI topology <domain:bus> then use OEM ID,

OEM

drivers/acpi/pci_mcfg.c +3 -0

+     * table ID, and OEM revision from MCFG table standard header.
+     */
+    for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++,

Quoted 2 lines

f++) {
+ if (f->seg == root->segment &&

why not use MCFG_DOM_RANGE, I think MCFG_DOM_RANGE is better. if drop MCFG_DOM_RANGE, mcfg_quirks[] will be more complex.

static struct mcfg_fixup mcfg_quirks[] = {
/* { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, cfgres, ops }, */

#ifdef CONFIG_PCI_HOST_THUNDER_ECAM
   /* SoC pass1.x */
   { "CAVIUM", "THUNDERX", 2, 0, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
     MCFG_RES_EMPTY},
   { "CAVIUM", "THUNDERX", 2, 1, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
     MCFG_RES_EMPTY},
   { "CAVIUM", "THUNDERX", 2, 2, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
     MCFG_RES_EMPTY},
   { "CAVIUM", "THUNDERX", 2, 3, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
     MCFG_RES_EMPTY},
   { "CAVIUM", "THUNDERX", 2, 10, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
     MCFG_RES_EMPTY},
   { "CAVIUM", "THUNDERX", 2, 11, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
     MCFG_RES_EMPTY},
   { "CAVIUM", "THUNDERX", 2, 12, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
     MCFG_RES_EMPTY},
   { "CAVIUM", "THUNDERX", 2, 13, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
     MCFG_RES_EMPTY},

#endif
.....
};

As PATCH v5 we only need define mcfg_quirks as below, It looks better. static struct pci_cfg_fixup mcfg_quirks[] __initconst = {
/* { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, pci_ops, init_hook
}, */

#ifdef CONFIG_PCI_HOST_THUNDER_PEM
   /* Pass2.0 */
   { "CAVIUM", "THUNDERX", 1, MCFG_DOM_RANGE(4, 9), MCFG_BUS_ANY, NULL,
     thunder_pem_cfg_init },
   { "CAVIUM", "THUNDERX", 1, MCFG_DOM_RANGE(14, 19), MCFG_BUS_ANY, NULL,
     thunder_pem_cfg_init },
#endif
#ifdef CONFIG_PCI_HISI_ACPI
   { "HISI  ", "HIP05   ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY,
     NULL, hisi_pcie_acpi_hip05_init},
   { "HISI  ", "HIP06   ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY,
     NULL, hisi_pcie_acpi_hip06_init},
   { "HISI  ", "HIP07   ", 0, MCFG_DOM_RANGE(0, 15), MCFG_BUS_ANY,
     NULL, hisi_pcie_acpi_hip07_init},

Quoted 2 lines

#endif
};

Note this series disallow hisi_pcie_acpi_hip07_init() call. According to the Bjorn suggestion I rework quirk code to override ops and CFG resources only. Giving that I do not see the way to use MCFG_DOM_RANGE macro. For HISI you would need to get CFG range for each possible case:

#ifdef CONFIG_PCI_HISI_ACPI
    { "HISI  ", "HIP05   ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_ops,
      DEFINE_RES_MEM(start0, size0)},
    { "HISI  ", "HIP05   ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_ops,
      DEFINE_RES_MEM(start1, size1)},
    { "HISI  ", "HIP05   ", 0, 2, MCFG_BUS_ANY, &hisi_pcie_ops,
      DEFINE_RES_MEM(start2, size2)},
    { "HISI  ", "HIP05   ", 0, 3, MCFG_BUS_ANY, &hisi_pcie_ops,
      DEFINE_RES_MEM(start3, size3)},

Quoted 4 lines

[...]
#endif

Indeed there are more entries here but you do not have to define the same resource array in driver.

Our host bridge is non ECAM only for the RC bus config space; for any other bus underneath the root bus we support ECAM access.

RC config resource with hardcode as DEFINE_RES_MEM(0xb0070000, SZ_4K),
EP config resource we get it from MCFG table.
So we need to override ops, but config resource we only need to hardcode with RC config resource.

Our host controller ACPI support patch can be found:
https://lkml.org/lkml/2016/8/31/340
This patch is based on RFC V5 quirk mechanism.

Based on V6 quirk mechanism, we have to change it as below:

#ifdef CONFIG_PCI_HISI_ACPI
	{ "HISI ", "HIP05 ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_hip05_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP05 ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_hip05_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP05 ", 0, 2, MCFG_BUS_ANY, &hisi_pcie_hip05_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP05 ", 0, 3, MCFG_BUS_ANY, &hisi_pcie_hip05_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP06 ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_hip06_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP06 ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_hip06_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP06 ", 0, 2, MCFG_BUS_ANY, &hisi_pcie_hip06_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP06 ", 0, 3, MCFG_BUS_ANY, &hisi_pcie_hip06_ops,
         MCFG_RES_EMPTY},
	{ "HISI ", "HIP07 ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_hip07_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP07 ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_hip07_ops,
	  MCFG_RES_EMPTY},
	....
	
	{ "HISI ", "HIP07 ", 0, 15, MCFG_BUS_ANY, &hisi_pcie_hip07_ops,
	  MCFG_RES_EMPTY},

#endif

struct pci_ecam_ops hisi_pci_hip05_ops = {
	.bus_shift	= 20,
	.init		=  hisi_pci_hip05_init,
	.pci_ops	= {
		.map_bus	= pci_ecam_map_bus,
		.read		= hisi_pcie_acpi_rd_conf,
		.write		= hisi_pcie_acpi_wr_conf,
	}
};

struct pci_ecam_ops hisi_pci_hip06_ops = {
	.bus_shift = 20,
	.init = hisi_pci_hip06_init,
	.pci_ops = {
		.map_bus = pci_ecam_map_bus,
		.read = hisi_pcie_acpi_rd_conf,
		.write = hisi_pcie_acpi_wr_conf,
	}
};

hisi_pci_hipxx_init function is used to get RC config resource with hardcode.
.....

So I hope we can use MCFG_DOM_RANGE, Then I can change it as below.

#ifdef CONFIG_PCI_HISI_ACPI
	{ "HISI  ", "HIP05   ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY,
	 &hisi_pcie_hip05_ops, MCFG_RES_EMPTY},
	{ "HISI  ", "HIP06   ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY,
	 &hisi_pcie_hip06_ops, MCFG_RES_EMPTY},
	{ "HISI  ", "HIP07   ", 0, MCFG_DOM_RANGE(0, 15), MCFG_BUS_ANY,
	  &hisi_pcie_hip07_ops, MCFG_RES_EMPTY},

#endif

Thanks
Dongdong

Quoted 6 lines

Thanks,
Tomasz

.

Our host bridge is non ECAM only for the RC bus config space; for any other bus underneath the root bus we support ECAM access.

RC config resource with hardcode as DEFINE_RES_MEM(0xb0070000, SZ_4K),
EP config resource we get it from MCFG table.
So we need to override ops, but config resource we only need to hardcode with RC config resource.

Our host controller ACPI support patch can be found:
https://lkml.org/lkml/2016/8/31/340

This patch is based on RFC V5 quirk mechanism.

Based on V6 quirk mechanism, we have to change it as below:

#ifdef CONFIG_PCI_HISI_ACPI
	{ "HISI ", "HIP05 ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_hip05_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP05 ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_hip05_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP05 ", 0, 2, MCFG_BUS_ANY, &hisi_pcie_hip05_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP05 ", 0, 3, MCFG_BUS_ANY, &hisi_pcie_hip05_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP06 ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_hip06_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP06 ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_hip06_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP06 ", 0, 2, MCFG_BUS_ANY, &hisi_pcie_hip06_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP06 ", 0, 3, MCFG_BUS_ANY, &hisi_pcie_hip06_ops,
         MCFG_RES_EMPTY},
	{ "HISI ", "HIP07 ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_hip07_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP07 ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_hip07_ops,
	  MCFG_RES_EMPTY},
	....
	
	{ "HISI ", "HIP07 ", 0, 15, MCFG_BUS_ANY, &hisi_pcie_hip07_ops,
	  MCFG_RES_EMPTY},

#endif

struct pci_ecam_ops hisi_pci_hip05_ops = {
	.bus_shift	= 20,
	.init		=  hisi_pci_hip05_init,
	.pci_ops	= {
		.map_bus	= pci_ecam_map_bus,
		.read		= hisi_pcie_acpi_rd_conf,
		.write		= hisi_pcie_acpi_wr_conf,
	}
};

struct pci_ecam_ops hisi_pci_hip06_ops = {
	.bus_shift = 20,
	.init = hisi_pci_hip06_init,
	.pci_ops = {
		.map_bus = pci_ecam_map_bus,
		.read = hisi_pcie_acpi_rd_conf,
		.write = hisi_pcie_acpi_wr_conf,
	}
};

hisi_pci_hipxx_init function is used to get RC config resource with hardcode.
.....

So I hope we can use MCFG_DOM_RANGE, Then I can change it as below.

#ifdef CONFIG_PCI_HISI_ACPI
	{ "HISI  ", "HIP05   ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY,
	 &hisi_pcie_hip05_ops, MCFG_RES_EMPTY},
	{ "HISI  ", "HIP06   ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY,
	 &hisi_pcie_hip06_ops, MCFG_RES_EMPTY},
	{ "HISI  ", "HIP07   ", 0, MCFG_DOM_RANGE(0, 15), MCFG_BUS_ANY,
	  &hisi_pcie_hip07_ops, MCFG_RES_EMPTY},

#endif

Thanks
Dongdong

Quoted 6 lines

Thanks,
Tomasz

.

-----Original Message-----
From: Lorenzo Pieralisi [mailto:[email protected]]
Sent: 15 September 2016 11:59
To: liudongdong (C)
Cc: Tomasz Nowicki; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Wangyijing; [email protected]; [email protected]; linux-arm- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Gabriele Paoloni; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH V6 2/5] PCI/ACPI: Check platform specific ECAM quirks

On Tue, Sep 13, 2016 at 07:38:39PM +0800, Dongdong Liu wrote:

[...]

Quoted 4 lines

Our host bridge is non ECAM only for the RC bus config space; for any other bus underneath the root bus we support ECAM access.

RC config resource with hardcode as DEFINE_RES_MEM(0xb0070000,

SZ_4K),

Quoted 2 lines

EP config resource we get it from MCFG table.
So we need to override ops, but config resource we only need to

hardcode with RC config resource.

Quoted 1 line

Quoted 2 lines

Our host controller ACPI support patch can be found:
https://lkml.org/lkml/2016/8/31/340

Sorry I misread your code. IIUC you create an array of resources that represent non-ECAM config space (and incidentally contain debug registers to check the link status - that you need to check for every given config access (?)), but you still need to have an MCFG entry that

covers the bus number subject to quirk to make sure this mechanism works. Correct ?

This also means that, with the MCFG tables you have and current mainline kernel you are able to probe a root bridge (because the MCFG table covers the bus number that is not ECAM), with enumeration going haywire because it is trying to carry out ECAM accesses on non-ECAM space.

Is my reading correct ?

Anyway, that's not stricly related to this discussion, it is time we converge on this patchset, we can add a domain range if that simplifies things.

Thanks,
Lorenzo

Quoted 52 lines

This patch is based on RFC V5 quirk mechanism.

Based on V6 quirk mechanism, we have to change it as below:

#ifdef CONFIG_PCI_HISI_ACPI
	{ "HISI ", "HIP05 ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_hip05_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP05 ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_hip05_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP05 ", 0, 2, MCFG_BUS_ANY, &hisi_pcie_hip05_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP05 ", 0, 3, MCFG_BUS_ANY, &hisi_pcie_hip05_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP06 ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_hip06_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP06 ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_hip06_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP06 ", 0, 2, MCFG_BUS_ANY, &hisi_pcie_hip06_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP06 ", 0, 3, MCFG_BUS_ANY, &hisi_pcie_hip06_ops,
         MCFG_RES_EMPTY},
	{ "HISI ", "HIP07 ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_hip07_ops,
	  MCFG_RES_EMPTY},
	{ "HISI ", "HIP07 ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_hip07_ops,
	  MCFG_RES_EMPTY},
	....

	{ "HISI ", "HIP07 ", 0, 15, MCFG_BUS_ANY, &hisi_pcie_hip07_ops,
	  MCFG_RES_EMPTY},

#endif

struct pci_ecam_ops hisi_pci_hip05_ops = {
	.bus_shift	= 20,
	.init		=  hisi_pci_hip05_init,
	.pci_ops	= {
		.map_bus	= pci_ecam_map_bus,
		.read		= hisi_pcie_acpi_rd_conf,
		.write		= hisi_pcie_acpi_wr_conf,
	}
};

struct pci_ecam_ops hisi_pci_hip06_ops = {
	.bus_shift = 20,
	.init = hisi_pci_hip06_init,
	.pci_ops = {
		.map_bus = pci_ecam_map_bus,
		.read = hisi_pcie_acpi_rd_conf,
		.write = hisi_pcie_acpi_wr_conf,
	}
};

Quoted 1 line

hisi_pci_hipxx_init function is used to get RC config resource with

hardcode.

Quoted 22 lines

.....

So I hope we can use MCFG_DOM_RANGE, Then I can change it as below.

#ifdef CONFIG_PCI_HISI_ACPI
	{ "HISI  ", "HIP05   ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY,
	 &hisi_pcie_hip05_ops, MCFG_RES_EMPTY},
	{ "HISI  ", "HIP06   ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY,
	 &hisi_pcie_hip06_ops, MCFG_RES_EMPTY},
	{ "HISI  ", "HIP07   ", 0, MCFG_DOM_RANGE(0, 15), MCFG_BUS_ANY,
	  &hisi_pcie_hip07_ops, MCFG_RES_EMPTY},

#endif

Thanks
Dongdong

Quoted 6 lines

Thanks,
Tomasz

.

Hi Lorenzo and Tomasz

Many Thanks for looking at this

Quoted 31 lines

-----Original Message-----
From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
Sent: 15 September 2016 11:59
To: liudongdong (C)
Cc: Tomasz Nowicki; helgaas at kernel.org; will.deacon at arm.com; catalin.marinas at arm.com; rafael at kernel.org; arnd at arndb.de; hanjun.guo at linaro.org; okaya at codeaurora.org; jchandra at broadcom.com; cov at codeaurora.org; dhdang at apm.com; ard.biesheuvel at linaro.org; robert.richter at caviumnetworks.com; mw at semihalf.com; Liviu.Dudau at arm.com; ddaney at caviumnetworks.com; Wangyijing; msalter at redhat.com; linux-pci at vger.kernel.org; linux-arm- kernel at lists.infradead.org; linaro-acpi at lists.linaro.org; jcm at redhat.com; andrea.gallo at linaro.org; jeremy.linton at arm.com; Gabriele Paoloni; jhugo at codeaurora.org; linux-acpi at vger.kernel.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH V6 2/5] PCI/ACPI: Check platform specific ECAM quirks

On Tue, Sep 13, 2016 at 07:38:39PM +0800, Dongdong Liu wrote:

[...]

Quoted 4 lines

Our host bridge is non ECAM only for the RC bus config space; for any other bus underneath the root bus we support ECAM access.

RC config resource with hardcode as DEFINE_RES_MEM(0xb0070000,

SZ_4K),

Quoted 2 lines

EP config resource we get it from MCFG table.
So we need to override ops, but config resource we only need to

hardcode with RC config resource.

Quoted 1 line

Quoted 7 lines

Quoted 2 lines

Our host controller ACPI support patch can be found:
https://lkml.org/lkml/2016/8/31/340

Sorry I misread your code. IIUC you create an array of resources that represent non-ECAM config space (and incidentally contain debug registers to check the link status - that you need to check for every given config access (?)), but you still need to have an MCFG entry that

The link status check is inherited from the designware framework (see
http://lxr.free-electrons.com/source/drivers/pci/host/pcie-designware.c#L678)

However I think that in this case we can just check the link status in our init function and return an error if the link is down

Quoted 2 lines

covers the bus number subject to quirk to make sure this mechanism works. Correct ?

Well we need the quirks for the root bus numbers but if read this v6 quirk mechanism correctly even if we do not specify an mcfg entry for bus 0 oci_mcfg_match_quirks() is called anyway and we can set our special configuration space addresses for the root buses (i.e. I think we can have a clean MCFG table with entries only for those bus ranges that are really ECAM)

Quoted 6 lines

This also means that, with the MCFG tables you have and current mainline kernel you are able to probe a root bridge (because the MCFG table covers the bus number that is not ECAM), with enumeration going haywire because it is trying to carry out ECAM accesses on non-ECAM space.

Yes correct, we cannot access the host controller configuration space with our current MCFG table and current Linux mainline

Quoted 1 line

Quoted 5 lines

Is my reading correct ?

Anyway, that's not stricly related to this discussion, it is time we converge on this patchset, we can add a domain range if that simplifies things.

IMO it would be better to have the domain range to avoid a very large and repetitive static quirk array

-----Original Message-----
From: Christopher Covington [mailto:cov at codeaurora.org]
Sent: 16 September 2016 13:28
To: Gabriele Paoloni; Lorenzo Pieralisi; liudongdong (C)
Cc: Tomasz Nowicki; helgaas at kernel.org; will.deacon at arm.com; catalin.marinas at arm.com; rafael at kernel.org; arnd at arndb.de; hanjun.guo at linaro.org; okaya at codeaurora.org; jchandra at broadcom.com; dhdang at apm.com; ard.biesheuvel at linaro.org;
robert.richter at caviumnetworks.com; mw at semihalf.com; Liviu.Dudau at arm.com; ddaney at caviumnetworks.com; Wangyijing; msalter at redhat.com; linux-pci at vger.kernel.org; linux-arm- kernel at lists.infradead.org; linaro-acpi at lists.linaro.org; jcm at redhat.com; andrea.gallo at linaro.org; jeremy.linton at arm.com; jhugo at codeaurora.org; linux-acpi at vger.kernel.org; linux- kernel at vger.kernel.org
Subject: Re: [PATCH V6 2/5] PCI/ACPI: Check platform specific ECAM quirks

Quoted 39 lines · Gabriele Paoloni

Hi Lorenzo and Tomasz

Many Thanks for looking at this

Quoted 35 lines

-----Original Message-----
From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
Sent: 15 September 2016 11:59
To: liudongdong (C)
Cc: Tomasz Nowicki; helgaas at kernel.org; will.deacon at arm.com; catalin.marinas at arm.com; rafael at kernel.org; arnd at arndb.de; hanjun.guo at linaro.org; okaya at codeaurora.org; jchandra at broadcom.com; cov at codeaurora.org; dhdang at apm.com; ard.biesheuvel at linaro.org; robert.richter at caviumnetworks.com; mw at semihalf.com; Liviu.Dudau at arm.com; ddaney at caviumnetworks.com; Wangyijing; msalter at redhat.com; linux-pci at vger.kernel.org; linux-arm- kernel at lists.infradead.org; linaro-acpi at lists.linaro.org; jcm at redhat.com; andrea.gallo at linaro.org; jeremy.linton at arm.com; Gabriele Paoloni; jhugo at codeaurora.org; linux-acpi at vger.kernel.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH V6 2/5] PCI/ACPI: Check platform specific ECAM quirks

On Tue, Sep 13, 2016 at 07:38:39PM +0800, Dongdong Liu wrote:

[...]

Quoted 4 lines

Our host bridge is non ECAM only for the RC bus config space; for any other bus underneath the root bus we support ECAM access.

RC config resource with hardcode as DEFINE_RES_MEM(0xb0070000,

SZ_4K),

Quoted 2 lines

EP config resource we get it from MCFG table.
So we need to override ops, but config resource we only need to

hardcode with RC config resource.

Quoted 3 lines

Our host controller ACPI support patch can be found:
https://lkml.org/lkml/2016/8/31/340

Sorry I misread your code. IIUC you create an array of resources

that

Quoted 2 lines

Quoted 2 lines

represent non-ECAM config space (and incidentally contain debug registers to check the link status - that you need to check for

every

Quoted 1 line

Quoted 1 line

given config access (?)), but you still need to have an MCFG entry

that

Quoted 3 lines

The link status check is inherited from the designware framework (see
http://lxr.free-electrons.com/source/drivers/pci/host/pcie-

designware.c#L678)

Quoted 11 lines

However I think that in this case we can just check the link status in our init function and return an error if the link is down

Quoted 2 lines

covers the bus number subject to quirk to make sure this mechanism works. Correct ?

Well we need the quirks for the root bus numbers but if read this v6 quirk mechanism correctly even if we do not specify an mcfg entry for bus 0 oci_mcfg_match_quirks() is called anyway and we can set our special configuration space addresses for the root buses (i.e. I

think

Quoted 6 lines

we can have a clean MCFG table with entries only for those bus ranges that are really ECAM)

Quoted 3 lines

This also means that, with the MCFG tables you have and current mainline kernel you are able to probe a root bridge (because the

MCFG

Quoted 14 lines

Quoted 3 lines

table covers the bus number that is not ECAM), with enumeration going haywire because it is trying to carry out ECAM accesses on non-ECAM space.

Yes correct, we cannot access the host controller configuration space with our current MCFG table and current Linux mainline

Quoted 6 lines

Is my reading correct ?

Anyway, that's not stricly related to this discussion, it is time we converge on this patchset, we can add a domain range if that simplifies things.

Quoted 2 lines

IMO it would be better to have the domain range to avoid a very large and repetitive static quirk array

The v6 framework requires what, 21 additional lines of quirk data? What if you were to define some preprocessor macros to slim it down? I think something like the following would bring it down to roughly 7 additional
lines.

#define PCI_ACPI_QUIRK_QUAD_DOM(rev, dom, ops) \
    { "HISI ", rev, 0, dom+0, MCFG_BUS_ANY, ops,
      MCFG_RES_EMPTY}, \
    { "HISI ", rev, 0, dom+1, MCFG_BUS_ANY, ops,
      MCFG_RES_EMPTY}, \
    { "HISI ", rev, 0, dom+2, MCFG_BUS_ANY, ops,
      MCFG_RES_EMPTY}, \
    { "HISI ", rev, 0, dom+3, MCFG_BUS_ANY, ops,
      MCFG_RES_EMPTY},

PCI_ACPI_QUIRK_QUAD_DOM("HIP05 ", 0, &hisi_pcie_hip05_ops) PCI_ACPI_QUIRK_QUAD_DOM("HIP06 ", 0, &hisi_pcie_hip05_ops) PCI_ACPI_QUIRK_QUAD_DOM("HIP07 ", 0, &hisi_pcie_hip06_ops) PCI_ACPI_QUIRK_QUAD_DOM("HIP07 ", 4, &hisi_pcie_hip07_ops) PCI_ACPI_QUIRK_QUAD_DOM("HIP07 ", 8, &hisi_pcie_hip07_ops) PCI_ACPI_QUIRK_QUAD_DOM("HIP07 ", 12, &hisi_pcie_hip07_ops)

Cov
--
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.

thunder-pem driver stands for being ACPI based PCI host controller. However, there is no standard way to describe its PEM-specific register ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension to obtain hardcoded addresses from static resource array. Although it is not pretty, it prevents from creating standard mechanism to handle similar cases in future.

Signed-off-by Tomasz Nowicki <[email protected]>

---
 drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 13 deletions(-)

drivers/pci/host/pci-thunder-pem.c +5 -0

index 6abaf80..b048761 100644
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-ecam.h>
 #include <linux/platform_device.h>
 

@@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
 	return pci_generic_config_write(bus, devfn, where, size, val);
 }
 
+#ifdef CONFIG_ACPI
+static struct resource thunder_pem_reg_res[] = {
+	[4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
+	[5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),

drivers/pci/host/pci-thunder-pem.c +33 -13

+};
+
+static struct resource *thunder_pem_acpi_res(struct pci_config_window *cfg)
+{
+	struct acpi_device *adev = to_acpi_device(cfg->parent);
+	struct acpi_pci_root *root = acpi_driver_data(adev);
+
+	if ((root->segment >= 4 && root->segment <= 9) ||
+	    (root->segment >= 14 && root->segment <= 19))
+		return &thunder_pem_reg_res[root->segment];
+
+	return NULL;
+}
+#else
+static struct resource *thunder_pem_acpi_res(struct pci_config_window *cfg)
+{
+	return NULL;
+}
+#endif
+
 static int thunder_pem_init(struct pci_config_window *cfg)
 {
 	struct device *dev = cfg->parent;

@@ -292,24 +327,24 @@ static int thunder_pem_init(struct pci_config_window *cfg)
 	struct thunder_pem_pci *pem_pci;
 	struct platform_device *pdev;
 
-	/* Only OF support for now */
-	if (!dev->of_node)
-		return -EINVAL;
-
 	pem_pci = devm_kzalloc(dev, sizeof(*pem_pci), GFP_KERNEL);
 	if (!pem_pci)
 		return -ENOMEM;
 
-	pdev = to_platform_device(dev);
-
-	/*
-	 * The second register range is the PEM bridge to the PCIe
-	 * bus.  It has a different config access method than those
-	 * devices behind the bridge.
-	 */
-	res_pem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (acpi_disabled) {
+		pdev = to_platform_device(dev);
+
+		/*
+		 * The second register range is the PEM bridge to the PCIe
+		 * bus.  It has a different config access method than those
+		 * devices behind the bridge.
+		 */
+		res_pem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	} else {
+		res_pem = thunder_pem_acpi_res(cfg);
+	}
 	if (!res_pem) {
-		dev_err(dev, "missing \"reg[1]\"property\n");
+		dev_err(dev, "missing configuration region\n");
 		return -EINVAL;
 	}
 

Signature · 1 line

1.9.1

Quoted 32 lines · Tomasz Nowicki

thunder-pem driver stands for being ACPI based PCI host controller. However, there is no standard way to describe its PEM-specific register ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension to obtain hardcoded addresses from static resource array. Although it is not pretty, it prevents from creating standard mechanism to handle similar cases in future.

Signed-off-by Tomasz Nowicki <[email protected]>

---
 drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 13 deletions(-)

drivers/pci/host/pci-thunder-pem.c +5 -0

index 6abaf80..b048761 100644
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-ecam.h>
 #include <linux/platform_device.h>
 

@@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
 	return pci_generic_config_write(bus, devfn, where, size, val);
 }
 
+#ifdef CONFIG_ACPI
+static struct resource thunder_pem_reg_res[] = {
+	[4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
+	[5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),

drivers/pci/host/pci-thunder-pem.c static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn, · line 301

Quoted 10 lines

drivers/pci/host/pci-thunder-pem.c +10 -0

+	[6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
+	[7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
+	[8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
+	[9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
+	[14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
+	[15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
+	[16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
+	[17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
+	[18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
+	[19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),

1) The "correct" way to discover the resources consumed by an ACPI device is to use the _CRS method. I know there are some issues there for bridges (not the fault of ThunderX!) because there's not a good way to distinguish windows from resources consumed directly by the bridge.

But we should either do this correctly, or include a comment about why we're doing it wrong, so we don't give the impression that this is the right way to do it.

I seem to recall some discussion about why we're doing it this way, but I don't remember the details. It'd be nice to include a summary here.

2) This is a little weird because here we define the resource size as 16MB, in the OF case we get the resource size from OF, in either case we ioremap 64K of it, and then as far as I can tell, we only ever access PEM_CFG_WR and PEM_CFG_RD, at offsets 0x28 and 0x30 into the space.

If the hardware actually decodes the entire 16MB, we should ioremap the whole 16MB. (Strictly speaking, drivers only need to ioremap the parts they're using, but in this case nobody claims the entire resource because of deficiencies in the ACPI and OF cores, so the driver should ioremap the entire thing to help prevent conflicts with other devices.)

It'd be nice if we didn't have the 64KB magic number. I think using devm_ioremap_resource() would be nice.

Quoted 54 lines · Bjorn Helgaas

Quoted 32 lines · Tomasz Nowicki

thunder-pem driver stands for being ACPI based PCI host controller. However, there is no standard way to describe its PEM-specific register ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension to obtain hardcoded addresses from static resource array. Although it is not pretty, it prevents from creating standard mechanism to handle similar cases in future.

Signed-off-by Tomasz Nowicki <[email protected]>

---
drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------

1 file changed, 48 insertions(+), 13 deletions(-)

drivers/pci/host/pci-thunder-pem.c +5 -0

index 6abaf80..b048761 100644
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-ecam.h>
 #include <linux/platform_device.h>
 

@@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
 	return pci_generic_config_write(bus, devfn, where, size, val);
 }
 
+#ifdef CONFIG_ACPI
+static struct resource thunder_pem_reg_res[] = {
+	[4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
+	[5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),

drivers/pci/host/pci-thunder-pem.c static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn, · line 301

Quoted 10 lines

drivers/pci/host/pci-thunder-pem.c +10 -0

+	[6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
+	[7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
+	[8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
+	[9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
+	[14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
+	[15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
+	[16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
+	[17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
+	[18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
+	[19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),

1) The "correct" way to discover the resources consumed by an ACPI device is to use the _CRS method. I know there are some issues there for bridges (not the fault of ThunderX!) because there's not a good way to distinguish windows from resources consumed directly by the bridge.

But we should either do this correctly, or include a comment about why we're doing it wrong, so we don't give the impression that this is the right way to do it.

Quoted 3 lines

I seem to recall some discussion about why we're doing it this way, but I don't remember the details. It'd be nice to include a summary here.

OK I will. The reason why we cannot use _CRS for this case is that CONSUMER flag was not use consistently for the bridge so far.

[+cc Rafael (maybe already cc'd; I didn't recognize [email protected], Duc]

Quoted 55 lines · Tomasz Nowicki

Quoted 54 lines · Bjorn Helgaas

Quoted 32 lines · Tomasz Nowicki

thunder-pem driver stands for being ACPI based PCI host controller. However, there is no standard way to describe its PEM-specific register ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension to obtain hardcoded addresses from static resource array. Although it is not pretty, it prevents from creating standard mechanism to handle similar cases in future.

Signed-off-by Tomasz Nowicki <[email protected]>

---
drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------

1 file changed, 48 insertions(+), 13 deletions(-)

drivers/pci/host/pci-thunder-pem.c +5 -0

index 6abaf80..b048761 100644
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-ecam.h>
 #include <linux/platform_device.h>
 

@@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
   return pci_generic_config_write(bus, devfn, where, size, val);
 }
 
+#ifdef CONFIG_ACPI
+static struct resource thunder_pem_reg_res[] = {
+   [4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
+   [5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),

drivers/pci/host/pci-thunder-pem.c static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn, · line 301

Quoted 10 lines

drivers/pci/host/pci-thunder-pem.c +10 -0

+   [6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
+   [7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
+   [8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
+   [9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
+   [14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
+   [15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
+   [16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
+   [17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
+   [18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
+   [19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),

1) The "correct" way to discover the resources consumed by an ACPI device is to use the _CRS method. I know there are some issues there for bridges (not the fault of ThunderX!) because there's not a good way to distinguish windows from resources consumed directly by the bridge.

But we should either do this correctly, or include a comment about why we're doing it wrong, so we don't give the impression that this is the right way to do it.

Quoted 6 lines

Quoted 3 lines

I seem to recall some discussion about why we're doing it this way, but I don't remember the details. It'd be nice to include a summary here.

OK I will. The reason why we cannot use _CRS for this case is that CONSUMER flag was not use consistently for the bridge so far.

Yes, I'm aware of that problem, but hard-coding resources into drivers is just a disaster. The PCI and ACPI cores need generic ways to learn what resources are consumed by devices. For PCI devices, that's done with BARs. For ACPI devices, it's done with _CRS. Without generic resource discovery, we can't manage resources reliably at the system level [1].

You have a PNP0A03/PNP0A08 device for the PCI host bridge. Because of the BIOS bugs in CONSUMER flag usage, we assume everything in its _CRS is a window and not consumed by the bridge itself. What if you added a companion ACPI device with a _CRS that contained the bridge resources? Then you'd have some driver ugliness to find that device, but at least the ACPI core could tell what resources were in use.

Maybe Rafael has a better idea?

Quoted 72 lines · Bjorn Helgaas

[+cc Rafael (maybe already cc'd; I didn't recognize rafael at kernel.org, Duc]

Quoted 55 lines · Tomasz Nowicki

Quoted 54 lines · Bjorn Helgaas

Quoted 32 lines · Tomasz Nowicki

thunder-pem driver stands for being ACPI based PCI host controller. However, there is no standard way to describe its PEM-specific register ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension to obtain hardcoded addresses from static resource array. Although it is not pretty, it prevents from creating standard mechanism to handle similar cases in future.

Signed-off-by Tomasz Nowicki <[email protected]>

---
drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------

1 file changed, 48 insertions(+), 13 deletions(-)

drivers/pci/host/pci-thunder-pem.c +5 -0

index 6abaf80..b048761 100644
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-ecam.h>
 #include <linux/platform_device.h>
 

@@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
   return pci_generic_config_write(bus, devfn, where, size, val);
 }
 
+#ifdef CONFIG_ACPI
+static struct resource thunder_pem_reg_res[] = {
+   [4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
+   [5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),

drivers/pci/host/pci-thunder-pem.c static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn, · line 301

Quoted 10 lines

drivers/pci/host/pci-thunder-pem.c +10 -0

+   [6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
+   [7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
+   [8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
+   [9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
+   [14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
+   [15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
+   [16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
+   [17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
+   [18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
+   [19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),

1) The "correct" way to discover the resources consumed by an ACPI device is to use the _CRS method. I know there are some issues there for bridges (not the fault of ThunderX!) because there's not a good way to distinguish windows from resources consumed directly by the bridge.

But we should either do this correctly, or include a comment about why we're doing it wrong, so we don't give the impression that this is the right way to do it.

Quoted 6 lines

Quoted 3 lines

I seem to recall some discussion about why we're doing it this way, but I don't remember the details. It'd be nice to include a summary here.

OK I will. The reason why we cannot use _CRS for this case is that CONSUMER flag was not use consistently for the bridge so far.

Yes, I'm aware of that problem, but hard-coding resources into drivers is just a disaster. The PCI and ACPI cores need generic ways to learn what resources are consumed by devices. For PCI devices, that's done with BARs. For ACPI devices, it's done with _CRS. Without generic resource discovery, we can't manage resources reliably at the system level [1].

Quoted 8 lines

You have a PNP0A03/PNP0A08 device for the PCI host bridge. Because of the BIOS bugs in CONSUMER flag usage, we assume everything in its _CRS is a window and not consumed by the bridge itself. What if you added a companion ACPI device with a _CRS that contained the bridge resources? Then you'd have some driver ugliness to find that device, but at least the ACPI core could tell what resources were in use.

Maybe Rafael has a better idea?

In the discussions leading up to this, we tried very hard to make this arm64/acpi quirks mechanism just as flexible as we need it to be to cover the current crop of incompatible hardware, but not more so. Going forward, we intend to require all arm64/acpi hardware to be spec compliant, and so any parametrization beyond what is required for the currently known broken hardware is only going to make it easier for others to ship with tweaked ACPI descriptions so that an existing quirk is triggered for hardware that it was not intended for. It also implies that we have to deal with the ACPI descriptions as they were shipped with the current hardware.

That does not mean, of course, that we should use bare constants rather than symbolic ones, but anything beyond that exceeds the desired scope of quirks handling.

Hi Ard,

Quoted 93 lines · Ard Biesheuvel

Quoted 72 lines · Bjorn Helgaas

[+cc Rafael (maybe already cc'd; I didn't recognize [email protected], Duc]

Quoted 55 lines · Tomasz Nowicki

Quoted 54 lines · Bjorn Helgaas

Quoted 32 lines · Tomasz Nowicki

thunder-pem driver stands for being ACPI based PCI host controller. However, there is no standard way to describe its PEM-specific register ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension to obtain hardcoded addresses from static resource array. Although it is not pretty, it prevents from creating standard mechanism to handle similar cases in future.

Signed-off-by Tomasz Nowicki <[email protected]>

---
drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------

1 file changed, 48 insertions(+), 13 deletions(-)

drivers/pci/host/pci-thunder-pem.c +5 -0

index 6abaf80..b048761 100644
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-ecam.h>
 #include <linux/platform_device.h>
 

@@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
   return pci_generic_config_write(bus, devfn, where, size, val);
 }
 
+#ifdef CONFIG_ACPI
+static struct resource thunder_pem_reg_res[] = {
+   [4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
+   [5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),

drivers/pci/host/pci-thunder-pem.c static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn, · line 301

Quoted 10 lines

drivers/pci/host/pci-thunder-pem.c +10 -0

+   [6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
+   [7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
+   [8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
+   [9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
+   [14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
+   [15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
+   [16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
+   [17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
+   [18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
+   [19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),

1) The "correct" way to discover the resources consumed by an ACPI device is to use the _CRS method. I know there are some issues there for bridges (not the fault of ThunderX!) because there's not a good way to distinguish windows from resources consumed directly by the bridge.

But we should either do this correctly, or include a comment about why we're doing it wrong, so we don't give the impression that this is the right way to do it.

Quoted 6 lines

Quoted 3 lines

I seem to recall some discussion about why we're doing it this way, but I don't remember the details. It'd be nice to include a summary here.

OK I will. The reason why we cannot use _CRS for this case is that CONSUMER flag was not use consistently for the bridge so far.

Yes, I'm aware of that problem, but hard-coding resources into drivers is just a disaster. The PCI and ACPI cores need generic ways to learn what resources are consumed by devices. For PCI devices, that's done with BARs. For ACPI devices, it's done with _CRS. Without generic resource discovery, we can't manage resources reliably at the system level [1].

Quoted 8 lines

You have a PNP0A03/PNP0A08 device for the PCI host bridge. Because of the BIOS bugs in CONSUMER flag usage, we assume everything in its _CRS is a window and not consumed by the bridge itself. What if you added a companion ACPI device with a _CRS that contained the bridge resources? Then you'd have some driver ugliness to find that device, but at least the ACPI core could tell what resources were in use.

Maybe Rafael has a better idea?

In the discussions leading up to this, we tried very hard to make this arm64/acpi quirks mechanism just as flexible as we need it to be to cover the current crop of incompatible hardware, but not more so. Going forward, we intend to require all arm64/acpi hardware to be spec compliant, and so any parametrization beyond what is required for the currently known broken hardware is only going to make it easier for others to ship with tweaked ACPI descriptions so that an existing quirk is triggered for hardware that it was not intended for. It also implies that we have to deal with the ACPI descriptions as they were shipped with the current hardware.

Quoted 3 lines

That does not mean, of course, that we should use bare constants rather than symbolic ones, but anything beyond that exceeds the desired scope of quirks handling.

Symbolic vs bare constants is the least of my worries. I'm pretty happy with the current quirk implementation. It's pretty simple and straightforward.

Apparently you shipped broken firmware that doesn't accurately describe system resource usage. Presumably that firmware could be updated, but maybe it's worthwhile to work around it in the kernel, depending on where it got shipped.

I'd like to step back and come up with some understanding of how non-broken firmware *should* deal with this issue. Then, if we *do* work around this particular broken firmware in the kernel, it would be nice to do it in a way that fits in with that understanding.

For example, if a companion ACPI device is the preferred solution, an ACPI quirk could fabricate a device with the required resources. That would address the problem closer to the source and make it more likely that the rest of the system will work correctly: /proc/iomem could make sense, things that look at _CRS generically would work (e.g, /sys/, an admittedly hypothetical "lsacpi", etc.)

Hard-coding stuff in drivers is a point solution that doesn't provide any guidance for future platforms and makes it likely that the hack will get copied into even more drivers.

Quoted 96 lines · Bjorn Helgaas

Hi Ard,

Quoted 93 lines · Ard Biesheuvel

Quoted 72 lines · Bjorn Helgaas

[+cc Rafael (maybe already cc'd; I didn't recognize rafael at kernel.org, Duc]

Quoted 55 lines · Tomasz Nowicki

Quoted 54 lines · Bjorn Helgaas

Quoted 32 lines · Tomasz Nowicki

thunder-pem driver stands for being ACPI based PCI host controller. However, there is no standard way to describe its PEM-specific register ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension to obtain hardcoded addresses from static resource array. Although it is not pretty, it prevents from creating standard mechanism to handle similar cases in future.

Signed-off-by Tomasz Nowicki <[email protected]>

---
drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------

1 file changed, 48 insertions(+), 13 deletions(-)

drivers/pci/host/pci-thunder-pem.c +5 -0

index 6abaf80..b048761 100644
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-ecam.h>
 #include <linux/platform_device.h>
 

@@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
   return pci_generic_config_write(bus, devfn, where, size, val);
 }
 
+#ifdef CONFIG_ACPI
+static struct resource thunder_pem_reg_res[] = {
+   [4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
+   [5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),

drivers/pci/host/pci-thunder-pem.c static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn, · line 301

Quoted 10 lines

drivers/pci/host/pci-thunder-pem.c +10 -0

+   [6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
+   [7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
+   [8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
+   [9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
+   [14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
+   [15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
+   [16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
+   [17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
+   [18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
+   [19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),

1) The "correct" way to discover the resources consumed by an ACPI device is to use the _CRS method. I know there are some issues there for bridges (not the fault of ThunderX!) because there's not a good way to distinguish windows from resources consumed directly by the bridge.

But we should either do this correctly, or include a comment about why we're doing it wrong, so we don't give the impression that this is the right way to do it.

Quoted 6 lines

Quoted 3 lines

I seem to recall some discussion about why we're doing it this way, but I don't remember the details. It'd be nice to include a summary here.

OK I will. The reason why we cannot use _CRS for this case is that CONSUMER flag was not use consistently for the bridge so far.

Yes, I'm aware of that problem, but hard-coding resources into drivers is just a disaster. The PCI and ACPI cores need generic ways to learn what resources are consumed by devices. For PCI devices, that's done with BARs. For ACPI devices, it's done with _CRS. Without generic resource discovery, we can't manage resources reliably at the system level [1].

Quoted 8 lines

You have a PNP0A03/PNP0A08 device for the PCI host bridge. Because of the BIOS bugs in CONSUMER flag usage, we assume everything in its _CRS is a window and not consumed by the bridge itself. What if you added a companion ACPI device with a _CRS that contained the bridge resources? Then you'd have some driver ugliness to find that device, but at least the ACPI core could tell what resources were in use.

Maybe Rafael has a better idea?

In the discussions leading up to this, we tried very hard to make this arm64/acpi quirks mechanism just as flexible as we need it to be to cover the current crop of incompatible hardware, but not more so. Going forward, we intend to require all arm64/acpi hardware to be spec compliant, and so any parametrization beyond what is required for the currently known broken hardware is only going to make it easier for others to ship with tweaked ACPI descriptions so that an existing quirk is triggered for hardware that it was not intended for. It also implies that we have to deal with the ACPI descriptions as they were shipped with the current hardware.

Quoted 8 lines

Quoted 3 lines

That does not mean, of course, that we should use bare constants rather than symbolic ones, but anything beyond that exceeds the desired scope of quirks handling.

Symbolic vs bare constants is the least of my worries. I'm pretty happy with the current quirk implementation. It's pretty simple and straightforward.

OK, good to know that we are on the right track here.

Quoted 5 lines

Apparently you shipped broken firmware that doesn't accurately describe system resource usage. Presumably that firmware could be updated, but maybe it's worthwhile to work around it in the kernel, depending on where it got shipped.

None of these platforms can be fixed entirely in software, and given that we will not be adding quirks for new broken hardware, we should ask ourselves whether having two versions of a quirk, i.e., one for broken hardware + currently shipping firmware, and one for the same broken hardware with fixed firmware is really an improvement over what has been proposed here.

Quoted 12 lines

I'd like to step back and come up with some understanding of how non-broken firmware *should* deal with this issue. Then, if we *do* work around this particular broken firmware in the kernel, it would be nice to do it in a way that fits in with that understanding.

For example, if a companion ACPI device is the preferred solution, an ACPI quirk could fabricate a device with the required resources. That would address the problem closer to the source and make it more likely that the rest of the system will work correctly: /proc/iomem could make sense, things that look at _CRS generically would work (e.g, /sys/, an admittedly hypothetical "lsacpi", etc.)

Quoted 4 lines

Hard-coding stuff in drivers is a point solution that doesn't provide any guidance for future platforms and makes it likely that the hack will get copied into even more drivers.

OK, I see. But the guidance for future platforms should be 'do not rely on quirks', and what I am arguing here is that the more we polish up this code and make it clean and reusable, the more likely it is that will end up getting abused by new broken hardware that we set out to reject entirely in the first place.

So of course, if the quirk involves claiming resources, let's make sure that this occurs in the cleanest and most compliant way possible. But any factoring/reuse concerns other than for the current crop of broken hardware should be avoided imo.

On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:

Quoted 6 lines

None of these platforms can be fixed entirely in software, and given that we will not be adding quirks for new broken hardware, we should ask ourselves whether having two versions of a quirk, i.e., one for broken hardware + currently shipping firmware, and one for the same broken hardware with fixed firmware is really an improvement over what has been proposed here.

We're talking about two completely different types of quirks:

1) MCFG quirks to use memory-mapped config space that doesn't quite conform to the ECAM model in the PCIe spec, and

2) Some yet-to-be-determined method to describe address space consumed by a bridge.

The first two patches of this series are a nice implementation for 1). The third patch (ThunderX-specific) is one possibility for 2), but I don't like it because there's no way for generic software like the ACPI core to discover these resources.

Quoted 23 lines

Quoted 12 lines

I'd like to step back and come up with some understanding of how non-broken firmware *should* deal with this issue. Then, if we *do* work around this particular broken firmware in the kernel, it would be nice to do it in a way that fits in with that understanding.

For example, if a companion ACPI device is the preferred solution, an ACPI quirk could fabricate a device with the required resources. That would address the problem closer to the source and make it more likely that the rest of the system will work correctly: /proc/iomem could make sense, things that look at _CRS generically would work (e.g, /sys/, an admittedly hypothetical "lsacpi", etc.)

Quoted 4 lines

Hard-coding stuff in drivers is a point solution that doesn't provide any guidance for future platforms and makes it likely that the hack will get copied into even more drivers.

OK, I see. But the guidance for future platforms should be 'do not rely on quirks', and what I am arguing here is that the more we polish up this code and make it clean and reusable, the more likely it is that will end up getting abused by new broken hardware that we set out to reject entirely in the first place.

Quoted 4 lines

So of course, if the quirk involves claiming resources, let's make sure that this occurs in the cleanest and most compliant way possible. But any factoring/reuse concerns other than for the current crop of broken hardware should be avoided imo.

If future hardware is completely ECAM-compliant and we don't need any more MCFG quirks, that would be great.

But we'll still need to describe that memory-mapped config space somewhere. If that's done with PNP0C02 or similar devices (as is done on my x86 laptop), we'd be all set.

If we need to work around firmware in the field that doesn't do that, one possibility is a PNP quirk along the lines of quirk_amd_mmconfig_area().

Quoted 1 line · Bjorn Helgaas

On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 12 lines

Quoted 6 lines

None of these platforms can be fixed entirely in software, and given that we will not be adding quirks for new broken hardware, we should ask ourselves whether having two versions of a quirk, i.e., one for broken hardware + currently shipping firmware, and one for the same broken hardware with fixed firmware is really an improvement over what has been proposed here.

We're talking about two completely different types of quirks:

1) MCFG quirks to use memory-mapped config space that doesn't quite conform to the ECAM model in the PCIe spec, and

Quoted 7 lines

2) Some yet-to-be-determined method to describe address space consumed by a bridge.

The first two patches of this series are a nice implementation for 1). The third patch (ThunderX-specific) is one possibility for 2), but I don't like it because there's no way for generic software like the ACPI core to discover these resources.

Ok, so basically this means that to implement (2) we need to assign some sort of _HID to these quirky PCI bridges (so that we know what device they represent and we can retrieve their _CRS). I take from this discussion that the goal is to make sure that all non-config resources have to be declared through _CRS device objects, which is fine but that requires a FW update (unless we can fabricate ACPI devices and corresponding _CRS in the kernel whenever we match a given MCFG table signature).

We discussed this already and I think we should make a decision:

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414722.html

Quoted 23 lines

Quoted 23 lines

Quoted 12 lines

I'd like to step back and come up with some understanding of how non-broken firmware *should* deal with this issue. Then, if we *do* work around this particular broken firmware in the kernel, it would be nice to do it in a way that fits in with that understanding.

For example, if a companion ACPI device is the preferred solution, an ACPI quirk could fabricate a device with the required resources. That would address the problem closer to the source and make it more likely that the rest of the system will work correctly: /proc/iomem could make sense, things that look at _CRS generically would work (e.g, /sys/, an admittedly hypothetical "lsacpi", etc.)

Quoted 4 lines

Hard-coding stuff in drivers is a point solution that doesn't provide any guidance for future platforms and makes it likely that the hack will get copied into even more drivers.

OK, I see. But the guidance for future platforms should be 'do not rely on quirks', and what I am arguing here is that the more we polish up this code and make it clean and reusable, the more likely it is that will end up getting abused by new broken hardware that we set out to reject entirely in the first place.

Quoted 7 lines

Quoted 4 lines

So of course, if the quirk involves claiming resources, let's make sure that this occurs in the cleanest and most compliant way possible. But any factoring/reuse concerns other than for the current crop of broken hardware should be avoided imo.

If future hardware is completely ECAM-compliant and we don't need any more MCFG quirks, that would be great.

Yes.

Quoted 3 lines

But we'll still need to describe that memory-mapped config space somewhere. If that's done with PNP0C02 or similar devices (as is done on my x86 laptop), we'd be all set.

I am not sure I understand what you mean here. Are you referring to MCFG regions reported as PNP0c02 resources through its _CRS ?

IIUC PNP0C02 is a reservation mechanism, but it does not help us associate its _CRS to a specific PCI host bridge instance, right ?

Quoted 3 lines

If we need to work around firmware in the field that doesn't do that, one possibility is a PNP quirk along the lines of quirk_amd_mmconfig_area().

You mean matching PNP0C01/PNP0c02 and create a resource (that has to hardcoded in a static array in the kernel anyway, there is no way to retrieve it otherwise) in the corresponding PNP quirk handler ?

And it is not a given we can match against PNP0c01/PNP0c02.

So it looks like the only solution is allocating an _HID for each host bridge that is not ECAM compliant to add resources to its _CRS (unless the MCFG quirk does not need any additional data/resource, eg "use different set of PCI accessorsi 32-bit vs byte-access").

For FW that is immutable I really do not see what we can do apart from hardcoding the non-config resources (consumed by a bridge), somehow.

Quoted 25 lines · Lorenzo Pieralisi

Quoted 1 line · Bjorn Helgaas

On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 12 lines

Quoted 6 lines

None of these platforms can be fixed entirely in software, and given that we will not be adding quirks for new broken hardware, we should ask ourselves whether having two versions of a quirk, i.e., one for broken hardware + currently shipping firmware, and one for the same broken hardware with fixed firmware is really an improvement over what has been proposed here.

We're talking about two completely different types of quirks:

1) MCFG quirks to use memory-mapped config space that doesn't quite conform to the ECAM model in the PCIe spec, and

Quoted 7 lines

2) Some yet-to-be-determined method to describe address space consumed by a bridge.

The first two patches of this series are a nice implementation for 1). The third patch (ThunderX-specific) is one possibility for 2), but I don't like it because there's no way for generic software like the ACPI core to discover these resources.

Quoted 8 lines

Ok, so basically this means that to implement (2) we need to assign some sort of _HID to these quirky PCI bridges (so that we know what device they represent and we can retrieve their _CRS). I take from this discussion that the goal is to make sure that all non-config resources have to be declared through _CRS device objects, which is fine but that requires a FW update (unless we can fabricate ACPI devices and corresponding _CRS in the kernel whenever we match a given MCFG table signature).

All resources consumed by ACPI devices should be declared through _CRS. If you want to fabricate ACPI devices or _CRS via kernel quirks, that's fine with me. This could be triggered via MCFG signature, DMI info, host bridge _HID, etc.

Quoted 35 lines

We discussed this already and I think we should make a decision:

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414722.html

Quoted 23 lines

Quoted 23 lines

Quoted 12 lines

I'd like to step back and come up with some understanding of how non-broken firmware *should* deal with this issue. Then, if we *do* work around this particular broken firmware in the kernel, it would be nice to do it in a way that fits in with that understanding.

For example, if a companion ACPI device is the preferred solution, an ACPI quirk could fabricate a device with the required resources. That would address the problem closer to the source and make it more likely that the rest of the system will work correctly: /proc/iomem could make sense, things that look at _CRS generically would work (e.g, /sys/, an admittedly hypothetical "lsacpi", etc.)

Quoted 4 lines

Hard-coding stuff in drivers is a point solution that doesn't provide any guidance for future platforms and makes it likely that the hack will get copied into even more drivers.

OK, I see. But the guidance for future platforms should be 'do not rely on quirks', and what I am arguing here is that the more we polish up this code and make it clean and reusable, the more likely it is that will end up getting abused by new broken hardware that we set out to reject entirely in the first place.

Quoted 7 lines

Quoted 4 lines

So of course, if the quirk involves claiming resources, let's make sure that this occurs in the cleanest and most compliant way possible. But any factoring/reuse concerns other than for the current crop of broken hardware should be avoided imo.

If future hardware is completely ECAM-compliant and we don't need any more MCFG quirks, that would be great.

Quoted 8 lines

Yes.

Quoted 3 lines

But we'll still need to describe that memory-mapped config space somewhere. If that's done with PNP0C02 or similar devices (as is done on my x86 laptop), we'd be all set.

I am not sure I understand what you mean here. Are you referring to MCFG regions reported as PNP0c02 resources through its _CRS ?

Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02 device.

Quoted 2 lines

IIUC PNP0C02 is a reservation mechanism, but it does not help us associate its _CRS to a specific PCI host bridge instance, right ?

Gab proposed a hierarchy that *would* associate a PNP0C02 device with a PCI bridge:

  Device (PCI1) {
    Name (_HID, "HISI0080") // PCI Express Root Bridge
    Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
    Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
    Device (RES0) {
      Name (_HID, "HISI0081") // HiSi PCIe RC config base address
      Name (_CID, "PNP0C02")  // Motherboard reserved resource
      Name (_CRS, ResourceTemplate () { ... }
    }
  }

That's a possibility. The PCI Firmware Spec suggests putting RES0 at the root (under \_SB), but I don't know why.

Putting it at the root means we couldn't generically associate it with a bridge, although I could imagine something like this:

  Device (RES1) {
    Name (_HID, "HISI0081") // HiSi PCIe RC config base address
    Name (_CID, "PNP0C02")  // Motherboard reserved resource
    Name (_CRS, ResourceTemplate () { ... }
    Method (BRDG) { "PCI1" }  // hand-wavy ASL
  }
  Device (PCI1) {
    Name (_HID, "HISI0080") // PCI Express Root Bridge
    Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
    Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
  }

Where you could search PNP0C02 devices for a cookie that matched the host bridge.

Quoted 7 lines

Quoted 3 lines

If we need to work around firmware in the field that doesn't do that, one possibility is a PNP quirk along the lines of quirk_amd_mmconfig_area().

You mean matching PNP0C01/PNP0c02 and create a resource (that has to hardcoded in a static array in the kernel anyway, there is no way to retrieve it otherwise) in the corresponding PNP quirk handler ?

Right. On some hardware we can read the resource out of a device-specific register, as we do in quirk_intel_mch(). But if that's not possible, it would have to be hard-coded.

Quoted 6 lines

And it is not a given we can match against PNP0c01/PNP0c02.

So it looks like the only solution is allocating an _HID for each host bridge that is not ECAM compliant to add resources to its _CRS (unless the MCFG quirk does not need any additional data/resource, eg "use different set of PCI accessorsi 32-bit vs byte-access").

It doesn't matter whether it's ECAM-compliant or not. Any memory-mapped config space should be reported via some device's _CRS.

The existing x86 practice is to use PNP0C02 devices for this purpose, and I think we should just follow that practice.

Quoted 3 lines

For FW that is immutable I really do not see what we can do apart from hardcoding the non-config resources (consumed by a bridge), somehow.

Right. Well, I assume you mean we should hard-code "non-window resources consumed directly by a bridge". If firmware in the field is broken, we should work around it, and that may mean hard-coding some resources.

My point is that the hard-coding should not be buried in a driver where it's invisible to the rest of the kernel. If we hard-code it in a quirk that adds _CRS entries, then the kernel will work just like it would if the firmware had been correct in the first place. The resource will appear in /sys/devices/pnp*/*/resources and /proc/iomem, and if we ever used _SRS to assign or move ACPI devices, we would know to avoid the bridge resource.

Bjorn

Quoted 7 lines · Bjorn Helgaas

On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:

The existing x86 practice is to use PNP0C02 devices for this purpose, and I think we should just follow that practice.

...

Quoted 7 lines

My point is that the hard-coding should not be buried in a driver where it's invisible to the rest of the kernel. If we hard-code it in a quirk that adds _CRS entries, then the kernel will work just like it would if the firmware had been correct in the first place. The resource will appear in /sys/devices/pnp*/*/resources and /proc/iomem, and if we ever used _SRS to assign or move ACPI devices, we would know to avoid the bridge resource.

Are you suggesting to add code similar to functions in linux/drivers/pnp/quirks.c to declare/attach the additional resource that the host need to have when the resource is not in MCFG table?

Quoted 16 lines · Duc Dang

Quoted 7 lines · Bjorn Helgaas

On Wed, Sep 21, 2016 at 03:05:49PM +0100, Lorenzo Pieralisi wrote:

The existing x86 practice is to use PNP0C02 devices for this purpose, and I think we should just follow that practice.

...

Quoted 7 lines

My point is that the hard-coding should not be buried in a driver where it's invisible to the rest of the kernel. If we hard-code it in a quirk that adds _CRS entries, then the kernel will work just like it would if the firmware had been correct in the first place. The resource will appear in /sys/devices/pnp*/*/resources and /proc/iomem, and if we ever used _SRS to assign or move ACPI devices, we would know to avoid the bridge resource.

Quoted 3 lines

Are you suggesting to add code similar to functions in linux/drivers/pnp/quirks.c to declare/attach the additional resource that the host need to have when the resource is not in MCFG table?

Yes, but what I'm suggesting is actually a little stronger. This has nothing to do with whether a resource is in the MCFG table or not.

I'm suggesting ACPI firmware should always describe the resource. If the firmware is defective and doesn't describe it, we should add a quirk in pnp/quirks.c to add a resource for it.

Quoted 25 lines · Lorenzo Pieralisi

Quoted 1 line · Bjorn Helgaas

On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 12 lines

Quoted 6 lines

None of these platforms can be fixed entirely in software, and given that we will not be adding quirks for new broken hardware, we should ask ourselves whether having two versions of a quirk, i.e., one for broken hardware + currently shipping firmware, and one for the same broken hardware with fixed firmware is really an improvement over what has been proposed here.

We're talking about two completely different types of quirks:

1) MCFG quirks to use memory-mapped config space that doesn't quite conform to the ECAM model in the PCIe spec, and

Quoted 7 lines

2) Some yet-to-be-determined method to describe address space consumed by a bridge.

The first two patches of this series are a nice implementation for 1). The third patch (ThunderX-specific) is one possibility for 2), but I don't like it because there's no way for generic software like the ACPI core to discover these resources.

Quoted 8 lines

Ok, so basically this means that to implement (2) we need to assign some sort of _HID to these quirky PCI bridges (so that we know what device they represent and we can retrieve their _CRS). I take from this discussion that the goal is to make sure that all non-config resources have to be declared through _CRS device objects, which is fine but that requires a FW update (unless we can fabricate ACPI devices and corresponding _CRS in the kernel whenever we match a given MCFG table signature).

All resources consumed by ACPI devices should be declared through _CRS. If you want to fabricate ACPI devices or _CRS via kernel quirks, that's fine with me. This could be triggered via MCFG signature, DMI info, host bridge _HID, etc.

Quoted 41 lines

We discussed this already and I think we should make a decision:

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414722.html

Quoted 23 lines

Quoted 23 lines

Quoted 12 lines

I'd like to step back and come up with some understanding of how non-broken firmware *should* deal with this issue. Then, if we *do* work around this particular broken firmware in the kernel, it would be nice to do it in a way that fits in with that understanding.

For example, if a companion ACPI device is the preferred solution, an ACPI quirk could fabricate a device with the required resources. That would address the problem closer to the source and make it more likely that the rest of the system will work correctly: /proc/iomem could make sense, things that look at _CRS generically would work (e.g, /sys/, an admittedly hypothetical "lsacpi", etc.)

Quoted 4 lines

Hard-coding stuff in drivers is a point solution that doesn't provide any guidance for future platforms and makes it likely that the hack will get copied into even more drivers.

OK, I see. But the guidance for future platforms should be 'do not rely on quirks', and what I am arguing here is that the more we polish up this code and make it clean and reusable, the more likely it is that will end up getting abused by new broken hardware that we set out to reject entirely in the first place.

Quoted 7 lines

Quoted 4 lines

So of course, if the quirk involves claiming resources, let's make sure that this occurs in the cleanest and most compliant way possible. But any factoring/reuse concerns other than for the current crop of broken hardware should be avoided imo.

If future hardware is completely ECAM-compliant and we don't need any more MCFG quirks, that would be great.

Yes.

Quoted 3 lines

But we'll still need to describe that memory-mapped config space somewhere. If that's done with PNP0C02 or similar devices (as is done on my x86 laptop), we'd be all set.

Quoted 2 lines

I am not sure I understand what you mean here. Are you referring to MCFG regions reported as PNP0c02 resources through its _CRS ?

Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02 device.

Quoted 2 lines

IIUC PNP0C02 is a reservation mechanism, but it does not help us associate its _CRS to a specific PCI host bridge instance, right ?

Gab proposed a hierarchy that *would* associate a PNP0C02 device with a PCI bridge:

  Device (PCI1) {
    Name (_HID, "HISI0080") // PCI Express Root Bridge
    Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
    Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
    Device (RES0) {
      Name (_HID, "HISI0081") // HiSi PCIe RC config base address
      Name (_CID, "PNP0C02")  // Motherboard reserved resource
      Name (_CRS, ResourceTemplate () { ... }
    }
  }

That's a possibility. The PCI Firmware Spec suggests putting RES0 at the root (under \_SB), but I don't know why.

Putting it at the root means we couldn't generically associate it with a bridge, although I could imagine something like this:

  Device (RES1) {
    Name (_HID, "HISI0081") // HiSi PCIe RC config base address
    Name (_CID, "PNP0C02")  // Motherboard reserved resource
    Name (_CRS, ResourceTemplate () { ... }
    Method (BRDG) { "PCI1" }  // hand-wavy ASL
  }
  Device (PCI1) {
    Name (_HID, "HISI0080") // PCI Express Root Bridge
    Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
    Method (_CRS, 0, Serialized) { // Root complex resources (windows) }
  }

Where you could search PNP0C02 devices for a cookie that matched the host bridge.o

Quoted 3 lines

For FW that is immutable I really do not see what we can do apart from hardcoding the non-config resources (consumed by a bridge), somehow.

Right. Well, I assume you mean we should hard-code "non-window resources consumed directly by a bridge". If firmware in the field is broken, we should work around it, and that may mean hard-coding some resources.

My point is that the hard-coding should not be buried in a driver where it's invisible to the rest of the kernel. If we hard-code it in a quirk that adds _CRS entries, then the kernel will work just like it would if the firmware had been correct in the first place. The resource will appear in /sys/devices/pnp*/*/resources and /proc/iomem, and if we ever used _SRS to assign or move ACPI devices, we would know to avoid the bridge resource.

-----Original Message-----
From: Lorenzo Pieralisi [mailto:[email protected]]
Sent: 22 September 2016 10:50
To: Bjorn Helgaas
Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon; Catalin Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo; Sinan Kaya; Jayachandran C; Christopher Covington; Duc Dang; Robert Richter; Marcin Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux- [email protected]; [email protected]; Linaro ACPI Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong (C); Gabriele Paoloni; Jeff Hugo; [email protected]; linux- [email protected]; Rafael J. Wysocki
Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM- specific register range for ACPI case

Quoted 7 lines · Bjorn Helgaas

Quoted 6 lines · Lorenzo Pieralisi

Quoted 1 line · Bjorn Helgaas

On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 1 line

Quoted 1 line

None of these platforms can be fixed entirely in software, and

given

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

that we will not be adding quirks for new broken hardware, we

should

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

ask ourselves whether having two versions of a quirk, i.e., one

for

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

broken hardware + currently shipping firmware, and one for the

same

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

broken hardware with fixed firmware is really an improvement

over what

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

has been proposed here.

We're talking about two completely different types of quirks:

1) MCFG quirks to use memory-mapped config space that doesn't

quite

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

conform to the ECAM model in the PCIe spec, and

2) Some yet-to-be-determined method to describe address space consumed by a bridge.

The first two patches of this series are a nice implementation

for 1).

Quoted 1 line

Quoted 1 line

Quoted 1 line

The third patch (ThunderX-specific) is one possibility for 2),

but I

Quoted 1 line

Quoted 1 line

Quoted 1 line

don't like it because there's no way for generic software like

the

Quoted 11 lines

Quoted 10 lines

Quoted 1 line

ACPI core to discover these resources.

Ok, so basically this means that to implement (2) we need to assign some sort of _HID to these quirky PCI bridges (so that we know what device they represent and we can retrieve their _CRS). I take from this discussion that the goal is to make sure that all non-config resources have to be declared through _CRS device objects, which is fine but that requires a FW update (unless we can fabricate ACPI devices and corresponding _CRS in the kernel whenever we match a given MCFG table signature).

Quoted 4 lines

All resources consumed by ACPI devices should be declared through _CRS. If you want to fabricate ACPI devices or _CRS via kernel quirks, that's fine with me. This could be triggered via MCFG signature, DMI info, host bridge _HID, etc.

I think the PNP quirk approach + PNP0c02 resource put forward by Gab is enough.

Quoted 4 lines

Quoted 4 lines

We discussed this already and I think we should make a decision:

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-

March/414722.html

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I'd like to step back and come up with some understanding of

how

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

non-broken firmware *should* deal with this issue. Then, if

we *do*

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

work around this particular broken firmware in the kernel, it

would be

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

nice to do it in a way that fits in with that understanding.

For example, if a companion ACPI device is the preferred

solution, an

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

ACPI quirk could fabricate a device with the required

resources. That

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

would address the problem closer to the source and make it

more likely

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

that the rest of the system will work correctly: /proc/iomem

could

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

make sense, things that look at _CRS generically would work

(e.g,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

/sys/, an admittedly hypothetical "lsacpi", etc.)

Hard-coding stuff in drivers is a point solution that doesn't

provide

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

any guidance for future platforms and makes it likely that

the hack

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

will get copied into even more drivers.

OK, I see. But the guidance for future platforms should be 'do

not

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

rely on quirks', and what I am arguing here is that the more we

polish

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

up this code and make it clean and reusable, the more likely it

is

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

that will end up getting abused by new broken hardware that we

set out

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

to reject entirely in the first place.

So of course, if the quirk involves claiming resources, let's

make

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

sure that this occurs in the cleanest and most compliant way

possible.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

But any factoring/reuse concerns other than for the current

crop of

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

broken hardware should be avoided imo.

If future hardware is completely ECAM-compliant and we don't need

any

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

more MCFG quirks, that would be great.

Yes.

Quoted 2 lines

But we'll still need to describe that memory-mapped config space somewhere. If that's done with PNP0C02 or similar devices (as is

done

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

on my x86 laptop), we'd be all set.

I am not sure I understand what you mean here. Are you referring to MCFG regions reported as PNP0c02 resources through its _CRS ?

Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02 device.

Ok, that's agreed. It goes without saying that since you are quoting the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device _CRS I will consider that a FW bug.

Quoted 10 lines

Quoted 2 lines

IIUC PNP0C02 is a reservation mechanism, but it does not help us associate its _CRS to a specific PCI host bridge instance, right ?

Gab proposed a hierarchy that *would* associate a PNP0C02 device with a PCI bridge:

Device (PCI1) {
Name (_HID, "HISI0080") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Method (_CRS, 0, Serialized) { // Root complex resources

(windows) }

Quoted 11 lines

    Device (RES0) {
      Name (_HID, "HISI0081") // HiSi PCIe RC config base address
      Name (_CID, "PNP0C02")  // Motherboard reserved resource
      Name (_CRS, ResourceTemplate () { ... }
    }

}

That's a possibility. The PCI Firmware Spec suggests putting RES0 at the root (under \_SB), but I don't know why.

Putting it at the root means we couldn't generically associate it

with

Quoted 12 lines

a bridge, although I could imagine something like this:

Device (RES1) {
Name (_HID, "HISI0081") // HiSi PCIe RC config base address Name (_CID, "PNP0C02") // Motherboard reserved resource Name (_CRS, ResourceTemplate () { ... }
Method (BRDG) { "PCI1" } // hand-wavy ASL
}
Device (PCI1) {
Name (_HID, "HISI0080") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Method (_CRS, 0, Serialized) { // Root complex resources

(windows) }

Quoted 4 lines

  }

Where you could search PNP0C02 devices for a cookie that matched the host bridge.o

Ok, I am fine with both and I think we are converging, but the way to solve this problem has to be uniform for all ARM partners (and not only ARM). Two points here:

1) Adding a device/subdevice allows people to add a _CRS reporting the non-window bridge resources. Fine. It also allows people to chuck in there all sorts of _DSD properties to describe their PCI host bridge as it is done with DT properties (those _DSD can contain eg clocks etc.), this may be tempting (so that they can reuse the same DT driver and do not have to update their firmware) but I want to be clear here: that must not happen. So, a subdevice with a _CRS to report resources, yes, but it will stop there.
2) It is unclear to me how to formalize the above. People should not write FW by reading the PCI mailing list, so these guidelines have
to
be written, somehow. I do not want to standardize quirks, I want to prevent random ACPI table content, which is different. Should I report this to the ACPI spec working group ? If we do not do that everyone will go solve this problem as they deem fit.

[...]

Quoted 6 lines

Quoted 3 lines

For FW that is immutable I really do not see what we can do apart from hardcoding the non-config resources (consumed by a bridge), somehow.

Right. Well, I assume you mean we should hard-code "non-window resources consumed directly by a bridge". If firmware in the field

is

Quoted 5 lines

broken, we should work around it, and that may mean hard-coding some resources.

My point is that the hard-coding should not be buried in a driver where it's invisible to the rest of the kernel. If we hard-code it

in

Quoted 1 line

a quirk that adds _CRS entries, then the kernel will work just like

it

Quoted 2 lines

would if the firmware had been correct in the first place. The resource will appear in /sys/devices/pnp*/*/resources and

/proc/iomem,

Quoted 1 line

and if we ever used _SRS to assign or move ACPI devices, we would

know

Quoted 1 line

to avoid the bridge resource.

We are in complete agreement here.

Thanks,
Lorenzo

Hi Lorenzo, Bjorn

Quoted 60 lines

-----Original Message-----
From: Lorenzo Pieralisi [mailto:[email protected]]
Sent: 22 September 2016 10:50
To: Bjorn Helgaas
Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon; Catalin Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo; Sinan Kaya; Jayachandran C; Christopher Covington; Duc Dang; Robert Richter; Marcin Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux- [email protected]; [email protected]; Linaro ACPI Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong (C); Gabriele Paoloni; Jeff Hugo; [email protected]; linux- [email protected]; Rafael J. Wysocki
Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM- specific register range for ACPI case

Quoted 7 lines · Bjorn Helgaas

Quoted 6 lines · Lorenzo Pieralisi

Quoted 1 line · Bjorn Helgaas

On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 1 line

Quoted 1 line

None of these platforms can be fixed entirely in software, and

given

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

that we will not be adding quirks for new broken hardware, we

should

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

ask ourselves whether having two versions of a quirk, i.e., one

for

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

broken hardware + currently shipping firmware, and one for the

same

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

broken hardware with fixed firmware is really an improvement

over what

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

has been proposed here.

We're talking about two completely different types of quirks:

1) MCFG quirks to use memory-mapped config space that doesn't

quite

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

conform to the ECAM model in the PCIe spec, and

2) Some yet-to-be-determined method to describe address space consumed by a bridge.

The first two patches of this series are a nice implementation

for 1).

Quoted 1 line

Quoted 1 line

Quoted 1 line

The third patch (ThunderX-specific) is one possibility for 2),

but I

Quoted 1 line

Quoted 1 line

Quoted 1 line

don't like it because there's no way for generic software like

the

Quoted 11 lines

Quoted 10 lines

Quoted 1 line

ACPI core to discover these resources.

Ok, so basically this means that to implement (2) we need to assign some sort of _HID to these quirky PCI bridges (so that we know what device they represent and we can retrieve their _CRS). I take from this discussion that the goal is to make sure that all non-config resources have to be declared through _CRS device objects, which is fine but that requires a FW update (unless we can fabricate ACPI devices and corresponding _CRS in the kernel whenever we match a given MCFG table signature).

Quoted 7 lines

Quoted 4 lines

All resources consumed by ACPI devices should be declared through _CRS. If you want to fabricate ACPI devices or _CRS via kernel quirks, that's fine with me. This could be triggered via MCFG signature, DMI info, host bridge _HID, etc.

I think the PNP quirk approach + PNP0c02 resource put forward by Gab is enough.

Great thanks as we take a final decision I will ask Dogndgong to submit another RFC based on this approach

Quoted 118 lines

Quoted 4 lines

Quoted 4 lines

We discussed this already and I think we should make a decision:

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-

March/414722.html

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I'd like to step back and come up with some understanding of

how

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

non-broken firmware *should* deal with this issue. Then, if

we *do*

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

work around this particular broken firmware in the kernel, it

would be

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

nice to do it in a way that fits in with that understanding.

For example, if a companion ACPI device is the preferred

solution, an

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

ACPI quirk could fabricate a device with the required

resources. That

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

would address the problem closer to the source and make it

more likely

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

that the rest of the system will work correctly: /proc/iomem

could

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

make sense, things that look at _CRS generically would work

(e.g,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

/sys/, an admittedly hypothetical "lsacpi", etc.)

Hard-coding stuff in drivers is a point solution that doesn't

provide

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

any guidance for future platforms and makes it likely that

the hack

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

will get copied into even more drivers.

OK, I see. But the guidance for future platforms should be 'do

not

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

rely on quirks', and what I am arguing here is that the more we

polish

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

up this code and make it clean and reusable, the more likely it

is

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

that will end up getting abused by new broken hardware that we

set out

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

to reject entirely in the first place.

So of course, if the quirk involves claiming resources, let's

make

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

sure that this occurs in the cleanest and most compliant way

possible.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

But any factoring/reuse concerns other than for the current

crop of

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

broken hardware should be avoided imo.

If future hardware is completely ECAM-compliant and we don't need

any

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

more MCFG quirks, that would be great.

Yes.

Quoted 2 lines

But we'll still need to describe that memory-mapped config space somewhere. If that's done with PNP0C02 or similar devices (as is

done

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

on my x86 laptop), we'd be all set.

I am not sure I understand what you mean here. Are you referring to MCFG regions reported as PNP0c02 resources through its _CRS ?

Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02 device.

Ok, that's agreed. It goes without saying that since you are quoting the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device _CRS I will consider that a FW bug.

Quoted 10 lines

Quoted 2 lines

IIUC PNP0C02 is a reservation mechanism, but it does not help us associate its _CRS to a specific PCI host bridge instance, right ?

Gab proposed a hierarchy that *would* associate a PNP0C02 device with a PCI bridge:

Device (PCI1) {
Name (_HID, "HISI0080") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Method (_CRS, 0, Serialized) { // Root complex resources

(windows) }

Quoted 11 lines

    Device (RES0) {
      Name (_HID, "HISI0081") // HiSi PCIe RC config base address
      Name (_CID, "PNP0C02")  // Motherboard reserved resource
      Name (_CRS, ResourceTemplate () { ... }
    }

}

That's a possibility. The PCI Firmware Spec suggests putting RES0 at the root (under \_SB), but I don't know why.

Putting it at the root means we couldn't generically associate it

with

Quoted 12 lines

a bridge, although I could imagine something like this:

Device (RES1) {
Name (_HID, "HISI0081") // HiSi PCIe RC config base address Name (_CID, "PNP0C02") // Motherboard reserved resource Name (_CRS, ResourceTemplate () { ... }
Method (BRDG) { "PCI1" } // hand-wavy ASL
}
Device (PCI1) {
Name (_HID, "HISI0080") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Method (_CRS, 0, Serialized) { // Root complex resources

(windows) }

Quoted 4 lines

  }

Where you could search PNP0C02 devices for a cookie that matched the host bridge.o

Ok, I am fine with both and I think we are converging, but the way to solve this problem has to be uniform for all ARM partners (and not only ARM). Two points here:

Quoted 16 lines

1) Adding a device/subdevice allows people to add a _CRS reporting the non-window bridge resources. Fine. It also allows people to chuck in there all sorts of _DSD properties to describe their PCI host bridge as it is done with DT properties (those _DSD can contain eg clocks etc.), this may be tempting (so that they can reuse the same DT driver and do not have to update their firmware) but I want to be clear here: that must not happen. So, a subdevice with a _CRS to report resources, yes, but it will stop there.
2) It is unclear to me how to formalize the above. People should not write FW by reading the PCI mailing list, so these guidelines have
to
be written, somehow. I do not want to standardize quirks, I want to prevent random ACPI table content, which is different. Should I report this to the ACPI spec working group ? If we do not do that everyone will go solve this problem as they deem fit.

Do we really need to formalize this?

As we discussed in the Linaro call at the moment we have few vendors that need quirks and we want to avoid promoting/accepting quirks for the future.

At the time of the call I think we decided to informally accept a set of quirks for the current platforms and reject any other quirk coming after a certain date/kernel version (this to be decided).

I am not sure if there is a way to document/formalize a temporary exception from the rule...

Thanks

Gab

Quoted 28 lines

[...]

Quoted 6 lines

Quoted 3 lines

For FW that is immutable I really do not see what we can do apart from hardcoding the non-config resources (consumed by a bridge), somehow.

Right. Well, I assume you mean we should hard-code "non-window resources consumed directly by a bridge". If firmware in the field

is

Quoted 5 lines

broken, we should work around it, and that may mean hard-coding some resources.

My point is that the hard-coding should not be buried in a driver where it's invisible to the rest of the kernel. If we hard-code it

in

Quoted 1 line

a quirk that adds _CRS entries, then the kernel will work just like

it

Quoted 2 lines

would if the firmware had been correct in the first place. The resource will appear in /sys/devices/pnp*/*/resources and

/proc/iomem,

Quoted 1 line

and if we ever used _SRS to assign or move ACPI devices, we would

know

Quoted 1 line

to avoid the bridge resource.

We are in complete agreement here.

Thanks,
Lorenzo

Quoted 214 lines · Gabriele Paoloni

Hi Lorenzo, Bjorn

Quoted 60 lines

-----Original Message-----
From: Lorenzo Pieralisi [mailto:[email protected]]
Sent: 22 September 2016 10:50
To: Bjorn Helgaas
Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon; Catalin Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo; Sinan Kaya; Jayachandran C; Christopher Covington; Duc Dang; Robert Richter; Marcin Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux- [email protected]; [email protected]; Linaro ACPI Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong (C); Gabriele Paoloni; Jeff Hugo; [email protected]; linux- [email protected]; Rafael J. Wysocki
Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM- specific register range for ACPI case

Quoted 7 lines · Bjorn Helgaas

Quoted 6 lines · Lorenzo Pieralisi

Quoted 1 line · Bjorn Helgaas

On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 1 line

Quoted 1 line

None of these platforms can be fixed entirely in software, and

given

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

that we will not be adding quirks for new broken hardware, we

should

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

ask ourselves whether having two versions of a quirk, i.e., one

for

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

broken hardware + currently shipping firmware, and one for the

same

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

broken hardware with fixed firmware is really an improvement

over what

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

has been proposed here.

We're talking about two completely different types of quirks:

1) MCFG quirks to use memory-mapped config space that doesn't

quite

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

conform to the ECAM model in the PCIe spec, and

2) Some yet-to-be-determined method to describe address space consumed by a bridge.

The first two patches of this series are a nice implementation

for 1).

Quoted 1 line

Quoted 1 line

Quoted 1 line

The third patch (ThunderX-specific) is one possibility for 2),

but I

Quoted 1 line

Quoted 1 line

Quoted 1 line

don't like it because there's no way for generic software like

the

Quoted 11 lines

Quoted 10 lines

Quoted 1 line

ACPI core to discover these resources.

Ok, so basically this means that to implement (2) we need to assign some sort of _HID to these quirky PCI bridges (so that we know what device they represent and we can retrieve their _CRS). I take from this discussion that the goal is to make sure that all non-config resources have to be declared through _CRS device objects, which is fine but that requires a FW update (unless we can fabricate ACPI devices and corresponding _CRS in the kernel whenever we match a given MCFG table signature).

Quoted 7 lines

Quoted 4 lines

All resources consumed by ACPI devices should be declared through _CRS. If you want to fabricate ACPI devices or _CRS via kernel quirks, that's fine with me. This could be triggered via MCFG signature, DMI info, host bridge _HID, etc.

I think the PNP quirk approach + PNP0c02 resource put forward by Gab is enough.

Great thanks as we take a final decision I will ask Dogndgong to submit another RFC based on this approach

Quoted 118 lines

Quoted 4 lines

Quoted 4 lines

We discussed this already and I think we should make a decision:

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-

March/414722.html

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I'd like to step back and come up with some understanding of

how

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

non-broken firmware *should* deal with this issue. Then, if

we *do*

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

work around this particular broken firmware in the kernel, it

would be

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

nice to do it in a way that fits in with that understanding.

For example, if a companion ACPI device is the preferred

solution, an

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

ACPI quirk could fabricate a device with the required

resources. That

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

would address the problem closer to the source and make it

more likely

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

that the rest of the system will work correctly: /proc/iomem

could

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

make sense, things that look at _CRS generically would work

(e.g,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

/sys/, an admittedly hypothetical "lsacpi", etc.)

Hard-coding stuff in drivers is a point solution that doesn't

provide

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

any guidance for future platforms and makes it likely that

the hack

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

will get copied into even more drivers.

OK, I see. But the guidance for future platforms should be 'do

not

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

rely on quirks', and what I am arguing here is that the more we

polish

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

up this code and make it clean and reusable, the more likely it

is

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

that will end up getting abused by new broken hardware that we

set out

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

to reject entirely in the first place.

So of course, if the quirk involves claiming resources, let's

make

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

sure that this occurs in the cleanest and most compliant way

possible.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

But any factoring/reuse concerns other than for the current

crop of

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

broken hardware should be avoided imo.

If future hardware is completely ECAM-compliant and we don't need

any

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

more MCFG quirks, that would be great.

Yes.

Quoted 2 lines

But we'll still need to describe that memory-mapped config space somewhere. If that's done with PNP0C02 or similar devices (as is

done

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

on my x86 laptop), we'd be all set.

I am not sure I understand what you mean here. Are you referring to MCFG regions reported as PNP0c02 resources through its _CRS ?

Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02 device.

Ok, that's agreed. It goes without saying that since you are quoting the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device _CRS I will consider that a FW bug.

Quoted 10 lines

Quoted 2 lines

IIUC PNP0C02 is a reservation mechanism, but it does not help us associate its _CRS to a specific PCI host bridge instance, right ?

Gab proposed a hierarchy that *would* associate a PNP0C02 device with a PCI bridge:

Device (PCI1) {
Name (_HID, "HISI0080") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Method (_CRS, 0, Serialized) { // Root complex resources

(windows) }

Quoted 11 lines

    Device (RES0) {
      Name (_HID, "HISI0081") // HiSi PCIe RC config base address
      Name (_CID, "PNP0C02")  // Motherboard reserved resource
      Name (_CRS, ResourceTemplate () { ... }
    }

}

That's a possibility. The PCI Firmware Spec suggests putting RES0 at the root (under \_SB), but I don't know why.

Putting it at the root means we couldn't generically associate it

with

Quoted 12 lines

a bridge, although I could imagine something like this:

Device (RES1) {
Name (_HID, "HISI0081") // HiSi PCIe RC config base address Name (_CID, "PNP0C02") // Motherboard reserved resource Name (_CRS, ResourceTemplate () { ... }
Method (BRDG) { "PCI1" } // hand-wavy ASL
}
Device (PCI1) {
Name (_HID, "HISI0080") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Method (_CRS, 0, Serialized) { // Root complex resources

(windows) }

Quoted 4 lines

  }

Where you could search PNP0C02 devices for a cookie that matched the host bridge.o

Ok, I am fine with both and I think we are converging, but the way to solve this problem has to be uniform for all ARM partners (and not only ARM). Two points here:

Quoted 16 lines

1) Adding a device/subdevice allows people to add a _CRS reporting the non-window bridge resources. Fine. It also allows people to chuck in there all sorts of _DSD properties to describe their PCI host bridge as it is done with DT properties (those _DSD can contain eg clocks etc.), this may be tempting (so that they can reuse the same DT driver and do not have to update their firmware) but I want to be clear here: that must not happen. So, a subdevice with a _CRS to report resources, yes, but it will stop there.
2) It is unclear to me how to formalize the above. People should not write FW by reading the PCI mailing list, so these guidelines have
to
be written, somehow. I do not want to standardize quirks, I want to prevent random ACPI table content, which is different. Should I report this to the ACPI spec working group ? If we do not do that everyone will go solve this problem as they deem fit.

Do we really need to formalize this?

As we discussed in the Linaro call at the moment we have few vendors that need quirks and we want to avoid promoting/accepting quirks for the future.

Quoted 6 lines

At the time of the call I think we decided to informally accept a set of quirks for the current platforms and reject any other quirk coming after a certain date/kernel version (this to be decided).

I am not sure if there is a way to document/formalize a temporary exception from the rule...

- (1) will be enforced.

- We do not know whether PNP0c02 can be used in non-root devices _CRS
- Are we sure (given that we are implementing this to make sure we are able to validate resources) that it is valid to have a subdevice with a _CRS whose resources are not contained in its parent _CRS address space (because that's exactly the case for these quirks) ?

That's what I mean by formalizing, I want to know how PNP0c02 should be used. We all want platforms with quirks to be enabled asap but only if we stick to the ACPI specifications. On top of that, with the bindings above, the kernel would end up creating a platform device for the "fake" device with a _CRS approach, which is questionable.

Quoted 29 lines

Quoted 28 lines

[...]

Quoted 6 lines

Quoted 3 lines

For FW that is immutable I really do not see what we can do apart from hardcoding the non-config resources (consumed by a bridge), somehow.

Right. Well, I assume you mean we should hard-code "non-window resources consumed directly by a bridge". If firmware in the field

is

Quoted 5 lines

broken, we should work around it, and that may mean hard-coding some resources.

My point is that the hard-coding should not be buried in a driver where it's invisible to the rest of the kernel. If we hard-code it

in

Quoted 1 line

a quirk that adds _CRS entries, then the kernel will work just like

it

Quoted 2 lines

would if the firmware had been correct in the first place. The resource will appear in /sys/devices/pnp*/*/resources and

/proc/iomem,

Quoted 1 line

and if we ever used _SRS to assign or move ACPI devices, we would

know

Quoted 1 line

to avoid the bridge resource.

We are in complete agreement here.

Thanks,
Lorenzo

Quoted 215 lines · Lorenzo Pieralisi

Quoted 214 lines · Gabriele Paoloni

Hi Lorenzo, Bjorn

Quoted 60 lines

-----Original Message-----
From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
Sent: 22 September 2016 10:50
To: Bjorn Helgaas
Cc: Ard Biesheuvel; Tomasz Nowicki; David Daney; Will Deacon; Catalin Marinas; Rafael Wysocki; Arnd Bergmann; Hanjun Guo; Sinan Kaya; Jayachandran C; Christopher Covington; Duc Dang; Robert Richter; Marcin Wojtas; Liviu Dudau; Wangyijing; Mark Salter; linux-
pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Linaro ACPI Mailman List; Jon Masters; Andrea Gallo; Jeremy Linton; liudongdong (C); Gabriele Paoloni; Jeff Hugo; linux-acpi at vger.kernel.org; linux- kernel at vger.kernel.org; Rafael J. Wysocki
Subject: Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM- specific register range for ACPI case

Quoted 7 lines · Bjorn Helgaas

Quoted 6 lines · Lorenzo Pieralisi

Quoted 1 line · Bjorn Helgaas

On Tue, Sep 20, 2016 at 04:09:25PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 1 line

Quoted 1 line

None of these platforms can be fixed entirely in software, and

given

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

that we will not be adding quirks for new broken hardware, we

should

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

ask ourselves whether having two versions of a quirk, i.e., one

for

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

broken hardware + currently shipping firmware, and one for the

same

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

broken hardware with fixed firmware is really an improvement

over what

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

has been proposed here.

We're talking about two completely different types of quirks:

1) MCFG quirks to use memory-mapped config space that doesn't

quite

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

conform to the ECAM model in the PCIe spec, and

2) Some yet-to-be-determined method to describe address space consumed by a bridge.

The first two patches of this series are a nice implementation

for 1).

Quoted 1 line

Quoted 1 line

Quoted 1 line

The third patch (ThunderX-specific) is one possibility for 2),

but I

Quoted 1 line

Quoted 1 line

Quoted 1 line

don't like it because there's no way for generic software like

the

Quoted 11 lines

Quoted 10 lines

Quoted 1 line

ACPI core to discover these resources.

Ok, so basically this means that to implement (2) we need to assign some sort of _HID to these quirky PCI bridges (so that we know what device they represent and we can retrieve their _CRS). I take from this discussion that the goal is to make sure that all non-config resources have to be declared through _CRS device objects, which is fine but that requires a FW update (unless we can fabricate ACPI devices and corresponding _CRS in the kernel whenever we match a given MCFG table signature).

Quoted 7 lines

Quoted 4 lines

All resources consumed by ACPI devices should be declared through _CRS. If you want to fabricate ACPI devices or _CRS via kernel quirks, that's fine with me. This could be triggered via MCFG signature, DMI info, host bridge _HID, etc.

I think the PNP quirk approach + PNP0c02 resource put forward by Gab is enough.

Great thanks as we take a final decision I will ask Dogndgong to submit another RFC based on this approach

Quoted 118 lines

Quoted 4 lines

Quoted 4 lines

We discussed this already and I think we should make a decision:

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-

March/414722.html

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I'd like to step back and come up with some understanding of

how

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

non-broken firmware *should* deal with this issue. Then, if

we *do*

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

work around this particular broken firmware in the kernel, it

would be

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

nice to do it in a way that fits in with that understanding.

For example, if a companion ACPI device is the preferred

solution, an

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

ACPI quirk could fabricate a device with the required

resources. That

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

would address the problem closer to the source and make it

more likely

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

that the rest of the system will work correctly: /proc/iomem

could

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

make sense, things that look at _CRS generically would work

(e.g,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

/sys/, an admittedly hypothetical "lsacpi", etc.)

Hard-coding stuff in drivers is a point solution that doesn't

provide

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

any guidance for future platforms and makes it likely that

the hack

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

will get copied into even more drivers.

OK, I see. But the guidance for future platforms should be 'do

not

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

rely on quirks', and what I am arguing here is that the more we

polish

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

up this code and make it clean and reusable, the more likely it

is

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

that will end up getting abused by new broken hardware that we

set out

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

to reject entirely in the first place.

So of course, if the quirk involves claiming resources, let's

make

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

sure that this occurs in the cleanest and most compliant way

possible.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

But any factoring/reuse concerns other than for the current

crop of

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

broken hardware should be avoided imo.

If future hardware is completely ECAM-compliant and we don't need

any

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

more MCFG quirks, that would be great.

Yes.

Quoted 2 lines

But we'll still need to describe that memory-mapped config space somewhere. If that's done with PNP0C02 or similar devices (as is

done

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

on my x86 laptop), we'd be all set.

I am not sure I understand what you mean here. Are you referring to MCFG regions reported as PNP0c02 resources through its _CRS ?

Yes. PCI Firmware Spec r3.0, Table 4-2, note 2 says address ranges reported via MCFG or _CBA should be reserved by _CRS of a PNP0C02 device.

Ok, that's agreed. It goes without saying that since you are quoting the PCI spec, if FW fails to report MCFG regions in a PNP0c02 device _CRS I will consider that a FW bug.

Quoted 10 lines

Quoted 2 lines

IIUC PNP0C02 is a reservation mechanism, but it does not help us associate its _CRS to a specific PCI host bridge instance, right ?

Gab proposed a hierarchy that *would* associate a PNP0C02 device with a PCI bridge:

Device (PCI1) {
Name (_HID, "HISI0080") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Method (_CRS, 0, Serialized) { // Root complex resources

(windows) }

Quoted 11 lines

    Device (RES0) {
      Name (_HID, "HISI0081") // HiSi PCIe RC config base address
      Name (_CID, "PNP0C02")  // Motherboard reserved resource
      Name (_CRS, ResourceTemplate () { ... }
    }

}

That's a possibility. The PCI Firmware Spec suggests putting RES0 at the root (under \_SB), but I don't know why.

Putting it at the root means we couldn't generically associate it

with

Quoted 12 lines

a bridge, although I could imagine something like this:

Device (RES1) {
Name (_HID, "HISI0081") // HiSi PCIe RC config base address Name (_CID, "PNP0C02") // Motherboard reserved resource Name (_CRS, ResourceTemplate () { ... }
Method (BRDG) { "PCI1" } // hand-wavy ASL
}
Device (PCI1) {
Name (_HID, "HISI0080") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Method (_CRS, 0, Serialized) { // Root complex resources

(windows) }

Quoted 4 lines

  }

Where you could search PNP0C02 devices for a cookie that matched the host bridge.o

Ok, I am fine with both and I think we are converging, but the way to solve this problem has to be uniform for all ARM partners (and not only ARM). Two points here:

Quoted 16 lines

1) Adding a device/subdevice allows people to add a _CRS reporting the non-window bridge resources. Fine. It also allows people to chuck in there all sorts of _DSD properties to describe their PCI host bridge as it is done with DT properties (those _DSD can contain eg clocks etc.), this may be tempting (so that they can reuse the same DT driver and do not have to update their firmware) but I want to be clear here: that must not happen. So, a subdevice with a _CRS to report resources, yes, but it will stop there.
2) It is unclear to me how to formalize the above. People should not write FW by reading the PCI mailing list, so these guidelines have
to
be written, somehow. I do not want to standardize quirks, I want to prevent random ACPI table content, which is different. Should I report this to the ACPI spec working group ? If we do not do that everyone will go solve this problem as they deem fit.

Do we really need to formalize this?

As we discussed in the Linaro call at the moment we have few vendors that need quirks and we want to avoid promoting/accepting quirks for the future.

Quoted 8 lines

Quoted 6 lines

At the time of the call I think we decided to informally accept a set of quirks for the current platforms and reject any other quirk coming after a certain date/kernel version (this to be decided).

I am not sure if there is a way to document/formalize a temporary exception from the rule...

- (1) will be enforced.

I'm not sure it's necessary or possible to enforce a "no future quirks" rule. For one thing, there's already a pretty strong incentive to avoid quirks: if your hardware doesn't require quirks, it works with OSes already in the field.

MCFG quirks allow us to use the generic ACPI pci_root.c driver even if the hardware doesn't support ECAM quite according to the spec.

PNP0C02 usage is a workaround for the failure of the Consumer/Producer bit. PNP0C02 quirks compensate for firmware that doesn't describe resource usage accurately. It's possible the ACPI spec folks could come up with a better Consumer/Producer workaround, if that's needed. Apparently x86 hasn't needed it yet.