diff mbox

[V6,2/5] PCI/ACPI: Check platform specific ECAM quirks

Message ID 1473449047-10499-3-git-send-email-tn@semihalf.com
State Changes Requested
Headers show

Commit Message

Tomasz Nowicki Sept. 9, 2016, 7:24 p.m. UTC
Some platforms may not be fully compliant with generic set of PCI config
accessors. For these cases we implement the way to overwrite 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 <tn@semihalf.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
Signed-off-by: Christopher Covington <cov@codeaurora.org>
---
 drivers/acpi/pci_mcfg.c | 80 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 6 deletions(-)

Comments

Duc Dang Sept. 12, 2016, 10:24 p.m. UTC | #1
On Fri, Sep 9, 2016 at 12:24 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
>
> 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 <tn@semihalf.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> ---
>  drivers/acpi/pci_mcfg.c | 80 +++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 74 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index ffcc651..2b8acc7 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -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
> +        * 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.

> +                   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
Regards,
Duc Dang.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Duc Dang Sept. 12, 2016, 10:47 p.m. UTC | #2
On Mon, Sep 12, 2016 at 3:24 PM, Duc Dang <dhdang@apm.com> wrote:
> On Fri, Sep 9, 2016 at 12:24 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
>>
>> 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 <tn@semihalf.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>> ---
>>  drivers/acpi/pci_mcfg.c | 80 +++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 74 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index ffcc651..2b8acc7 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -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
>> +        * 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.
>
>> +                   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;

I think this is a typo, it should be:
               mcfg_oem_revision = header->oem_revision;

>> +
>>         pr_info("MCFG table detected, %d entries\n", n);
>>         return 0;
>>  }
>> --
>> 1.9.1
> Regards,
> Duc Dang.
Regards,
Duc Dang.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dongdong Liu Sept. 13, 2016, 2:36 a.m. UTC | #3
Hi Tomasz

在 2016/9/10 3:24, 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 <tn@semihalf.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> ---
>   drivers/acpi/pci_mcfg.c | 80 +++++++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 74 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index ffcc651..2b8acc7 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -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
> +	 * 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 &&

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
};

> +		    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;
>   }
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Nowicki Sept. 13, 2016, 5:58 a.m. UTC | #4
On 13.09.2016 00:47, Duc Dang wrote:
> On Mon, Sep 12, 2016 at 3:24 PM, Duc Dang <dhdang@apm.com> wrote:
>> On Fri, Sep 9, 2016 at 12:24 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
>>>
>>> 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 <tn@semihalf.com>
>>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>>> ---
>>>  drivers/acpi/pci_mcfg.c | 80 +++++++++++++++++++++++++++++++++++++++++++++----
>>>  1 file changed, 74 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>>> index ffcc651..2b8acc7 100644
>>> --- a/drivers/acpi/pci_mcfg.c
>>> +++ b/drivers/acpi/pci_mcfg.c
>>> @@ -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
>>> +        * 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.
>>
>>> +                   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;
>
> I think this is a typo, it should be:
>                mcfg_oem_revision = header->oem_revision;

You are right, header->oem_revision should be assigned.

Thanks!
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Nowicki Sept. 13, 2016, 6:32 a.m. UTC | #5
Hi Liu,

On 13.09.2016 04:36, Dongdong Liu wrote:
> Hi Tomasz
>
> 在 2016/9/10 3:24, 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 <tn@semihalf.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>> ---
>>   drivers/acpi/pci_mcfg.c | 80
>> +++++++++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 74 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index ffcc651..2b8acc7 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -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
>> +     * 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 &&
>
> 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
> };

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Nowicki Sept. 13, 2016, 6:37 a.m. UTC | #6
On 13.09.2016 00:24, Duc Dang wrote:
> On Fri, Sep 9, 2016 at 12:24 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
>>
>> 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 <tn@semihalf.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>> ---
>>  drivers/acpi/pci_mcfg.c | 80 +++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 74 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index ffcc651..2b8acc7 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -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
>> +        * 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).

MCFG_DOM_ANY makes sense only if we want to use the same ops for segment 
range, but not for CFG range manipulation. So we can add MCFG_DOM_ANY 
back here in the hope it will be used properly.

Thanks,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dongdong Liu Sept. 13, 2016, 11:38 a.m. UTC | #7
Hi Tomasz

在 2016/9/13 14:32, Tomasz Nowicki 写道:
> Hi Liu,
>
> On 13.09.2016 04:36, Dongdong Liu wrote:
>> Hi Tomasz
>>
>> 在 2016/9/10 3:24, 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 <tn@semihalf.com>
>>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>>> ---
>>>   drivers/acpi/pci_mcfg.c | 80
>>> +++++++++++++++++++++++++++++++++++++++++++++----
>>>   1 file changed, 74 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>>> index ffcc651..2b8acc7 100644
>>> --- a/drivers/acpi/pci_mcfg.c
>>> +++ b/drivers/acpi/pci_mcfg.c
>>> @@ -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
>>> +     * 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 &&
>>
>> 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
>> };
>
> 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.

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
>
> Thanks,
> Tomasz
>
> .
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi Sept. 14, 2016, 12:40 p.m. UTC | #8
On Tue, Sep 13, 2016 at 07:38:39PM +0800, Dongdong Liu wrote:
> Hi Tomasz
> 
> ?? 2016/9/13 14:32, Tomasz Nowicki ????:
> >Hi Liu,
> >
> >On 13.09.2016 04:36, Dongdong Liu wrote:
> >>Hi Tomasz
> >>
> >>?? 2016/9/10 3:24, 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 <tn@semihalf.com>
> >>>Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> >>>Signed-off-by: Christopher Covington <cov@codeaurora.org>
> >>>---
> >>>  drivers/acpi/pci_mcfg.c | 80
> >>>+++++++++++++++++++++++++++++++++++++++++++++----
> >>>  1 file changed, 74 insertions(+), 6 deletions(-)
> >>>
> >>>diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> >>>index ffcc651..2b8acc7 100644
> >>>--- a/drivers/acpi/pci_mcfg.c
> >>>+++ b/drivers/acpi/pci_mcfg.c
> >>>@@ -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
> >>>+     * 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 &&
> >>
> >>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
> >>};
> >
> >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.
> 
> 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:

That's because you are hacking around the quirk mechanism to define
resources that have nothing to do with PCI config regions (that you
ioremap and use to check the PCI link status) and of top of that you are
*still* using the MCFG to describe config regions that are NOT ECAM
compliant, which is the exact opposite of what this patchset is meant
to achieve.

If a config region is not ECAM compliant having it defined in the MCFG
is a firmware bug and the way you are using this patchset is not what
we designed it for, please correct me if I am wrong.

Thanks,
Lorenzo

> 
> #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
> >
> >Thanks,
> >Tomasz
> >
> >.
> >
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi Sept. 15, 2016, 10:58 a.m. UTC | #9
On Tue, Sep 13, 2016 at 07:38:39PM +0800, Dongdong Liu wrote:

[...]

> 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

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

> 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
> >
> >Thanks,
> >Tomasz
> >
> >.
> >
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gabriele Paoloni Sept. 16, 2016, 9:02 a.m. UTC | #10
Hi Lorenzo and Tomasz

Many Thanks for looking at this

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> Sent: 15 September 2016 11:59
> To: liudongdong (C)
> Cc: Tomasz Nowicki; helgaas@kernel.org; will.deacon@arm.com;
> catalin.marinas@arm.com; rafael@kernel.org; arnd@arndb.de;
> hanjun.guo@linaro.org; okaya@codeaurora.org; jchandra@broadcom.com;
> cov@codeaurora.org; dhdang@apm.com; ard.biesheuvel@linaro.org;
> robert.richter@caviumnetworks.com; mw@semihalf.com;
> Liviu.Dudau@arm.com; ddaney@caviumnetworks.com; Wangyijing;
> msalter@redhat.com; linux-pci@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linaro-acpi@lists.linaro.org;
> jcm@redhat.com; andrea.gallo@linaro.org; jeremy.linton@arm.com;
> Gabriele Paoloni; jhugo@codeaurora.org; linux-acpi@vger.kernel.org;
> linux-kernel@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:
> 
> [...]
> 
> > 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
> 
> 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

> 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)  

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

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

Thanks

Gab

> 
> Thanks,
> Lorenzo
> 
> > 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
> > >
> > >Thanks,
> > >Tomasz
> > >
> > >.
> > >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher Covington Sept. 16, 2016, 12:27 p.m. UTC | #11
On 09/16/2016 05:02 AM, Gabriele Paoloni wrote:
> Hi Lorenzo and Tomasz
> 
> Many Thanks for looking at this
> 
>> -----Original Message-----
>> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
>> Sent: 15 September 2016 11:59
>> To: liudongdong (C)
>> Cc: Tomasz Nowicki; helgaas@kernel.org; will.deacon@arm.com;
>> catalin.marinas@arm.com; rafael@kernel.org; arnd@arndb.de;
>> hanjun.guo@linaro.org; okaya@codeaurora.org; jchandra@broadcom.com;
>> cov@codeaurora.org; dhdang@apm.com; ard.biesheuvel@linaro.org;
>> robert.richter@caviumnetworks.com; mw@semihalf.com;
>> Liviu.Dudau@arm.com; ddaney@caviumnetworks.com; Wangyijing;
>> msalter@redhat.com; linux-pci@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; linaro-acpi@lists.linaro.org;
>> jcm@redhat.com; andrea.gallo@linaro.org; jeremy.linton@arm.com;
>> Gabriele Paoloni; jhugo@codeaurora.org; linux-acpi@vger.kernel.org;
>> linux-kernel@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:
>>
>> [...]
>>
>>> 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
>>
>> 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
> 
>> 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)  
> 
>>
>> 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
> 
>>
>> 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

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
Gabriele Paoloni Sept. 16, 2016, 1:42 p.m. UTC | #12
Hi Cov

> -----Original Message-----
> From: Christopher Covington [mailto:cov@codeaurora.org]
> Sent: 16 September 2016 13:28
> To: Gabriele Paoloni; Lorenzo Pieralisi; liudongdong (C)
> Cc: Tomasz Nowicki; helgaas@kernel.org; will.deacon@arm.com;
> catalin.marinas@arm.com; rafael@kernel.org; arnd@arndb.de;
> hanjun.guo@linaro.org; okaya@codeaurora.org; jchandra@broadcom.com;
> dhdang@apm.com; ard.biesheuvel@linaro.org;
> robert.richter@caviumnetworks.com; mw@semihalf.com;
> Liviu.Dudau@arm.com; ddaney@caviumnetworks.com; Wangyijing;
> msalter@redhat.com; linux-pci@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linaro-acpi@lists.linaro.org;
> jcm@redhat.com; andrea.gallo@linaro.org; jeremy.linton@arm.com;
> jhugo@codeaurora.org; linux-acpi@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH V6 2/5] PCI/ACPI: Check platform specific ECAM
> quirks
> 
> On 09/16/2016 05:02 AM, Gabriele Paoloni wrote:
> > Hi Lorenzo and Tomasz
> >
> > Many Thanks for looking at this
> >
> >> -----Original Message-----
> >> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> >> Sent: 15 September 2016 11:59
> >> To: liudongdong (C)
> >> Cc: Tomasz Nowicki; helgaas@kernel.org; will.deacon@arm.com;
> >> catalin.marinas@arm.com; rafael@kernel.org; arnd@arndb.de;
> >> hanjun.guo@linaro.org; okaya@codeaurora.org; jchandra@broadcom.com;
> >> cov@codeaurora.org; dhdang@apm.com; ard.biesheuvel@linaro.org;
> >> robert.richter@caviumnetworks.com; mw@semihalf.com;
> >> Liviu.Dudau@arm.com; ddaney@caviumnetworks.com; Wangyijing;
> >> msalter@redhat.com; linux-pci@vger.kernel.org; linux-arm-
> >> kernel@lists.infradead.org; linaro-acpi@lists.linaro.org;
> >> jcm@redhat.com; andrea.gallo@linaro.org; jeremy.linton@arm.com;
> >> Gabriele Paoloni; jhugo@codeaurora.org; linux-acpi@vger.kernel.org;
> >> linux-kernel@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:
> >>
> >> [...]
> >>
> >>> 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
> >>
> >> 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
> >
> >> 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)
> >
> >>
> >> 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
> >
> >>
> >> 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
> 
> 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.

Thanks, yes this would work as well. To be honest this is not a big issue.
So either we go this way or we introduce domain range...

Cheers

Gab

> 
> #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.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index ffcc651..2b8acc7 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -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
+	 * 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 &&
+		    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;
 }