Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
 wrote:
> Hi Duc,
>
> On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>>>  wrote:
 On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki 
>
> pci_generic_ecam_ops is used by default. Since there are platforms
> which have non-compliant ECAM space we need to overwrite these
> accessors prior to PCI buses enumeration. In order to do that
> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> we can use proper PCI config space accessors and bus_shift.
>
> pci_generic_ecam_ops is still used for platforms free from quirks.
>
> Signed-off-by: Tomasz Nowicki 
> ---
>  arch/arm64/kernel/pci.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 94cd43c..a891bda 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> *root)
>   struct pci_config_window *cfg;
>   struct resource cfgres;
>   unsigned int bsz;
> + struct pci_ecam_ops *ops;
>
>   /* Use address from _CBA if present, otherwise lookup MCFG */
>   if (!root->mcfg_addr)
> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> *root)
>   return NULL;
>   }
>
> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
> + ops = pci_mcfg_get_ops(root);
> + bsz = 1 << ops->bus_shift;
>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>   cfgres.flags = IORESOURCE_MEM;
> - cfg = pci_ecam_create(>device->dev, , bus_res,
> -   _generic_ecam_ops);
> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);

 Arnd pointed this out already, I think that's the only pending question
 here.

 pci_ecam_create() maps ECAM space for config regions retrieved from
 the MCFG, which are *supposed* to be ECAM compliant.

 Do we think that's *always* correct/safe regardless of the kind
 of quirk we are currently fixing up ?

 Or we do think that configuration space regions should come from
 a different resource declared in the ACPI namespace if the regions
 are not MCFG/ECAM compliant (ie config space is not defined through
 MCFG at all - possibly through a _CRS method for a vendor specific
 _HID under the PNP0A03 node ?)
>>>
>>> Hi Lorenzo,
>>>
>>> For X-Gene: the ECAM space is used to access the configuration space
>>> of PCIe devices, with additional help from controller register to
>>> specify the bus, device and function number. Below is the RFC patch
>>> that implements ECAM fixup for X-Gene PCIe controller on top of this
>>> RFC ECAM quirk v3 for your and others reference.
>>
>> Yes, you have an additional resource in the PNP0A03 _CRS to describe
>> your register that is a deliberate abuse of the ACPI standard in
>> that the _CRS is meant to describe resources that are passed on
>> to secondary buses
>
> A potential alternative came up in an off-list discussion: Would it be
> better to hard code the information in the quirk workaround than look it
> up from a repurposed ACPI resource?

Hi Chris, Lorenzo,

Thanks for looking into this.

Yes, I am open for this approach and I think it may work. I can
+ check the pci_config_window resource start address (cfg->res.start)
to figure out the controller and then get the fixed controller
register address
or
+ using the domain number to identify the controller.

Regards,
Duc Dang.
>
> Supporting quirk workarounds for early, non-compliant hardware is
> helpful and perhaps necessary for bootstrapping the ecosystem in a
> timely manner. But we don't really want to provide an expandable or
> reusable interface that would make it easy for new hardware to remain
> non-compliant.
>
> Regards,
> Cov
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
 wrote:
> Hi Duc,
>
> On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>>>  wrote:
 On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki 
>
> pci_generic_ecam_ops is used by default. Since there are platforms
> which have non-compliant ECAM space we need to overwrite these
> accessors prior to PCI buses enumeration. In order to do that
> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> we can use proper PCI config space accessors and bus_shift.
>
> pci_generic_ecam_ops is still used for platforms free from quirks.
>
> Signed-off-by: Tomasz Nowicki 
> ---
>  arch/arm64/kernel/pci.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 94cd43c..a891bda 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> *root)
>   struct pci_config_window *cfg;
>   struct resource cfgres;
>   unsigned int bsz;
> + struct pci_ecam_ops *ops;
>
>   /* Use address from _CBA if present, otherwise lookup MCFG */
>   if (!root->mcfg_addr)
> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> *root)
>   return NULL;
>   }
>
> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
> + ops = pci_mcfg_get_ops(root);
> + bsz = 1 << ops->bus_shift;
>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>   cfgres.flags = IORESOURCE_MEM;
> - cfg = pci_ecam_create(>device->dev, , bus_res,
> -   _generic_ecam_ops);
> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);

 Arnd pointed this out already, I think that's the only pending question
 here.

 pci_ecam_create() maps ECAM space for config regions retrieved from
 the MCFG, which are *supposed* to be ECAM compliant.

 Do we think that's *always* correct/safe regardless of the kind
 of quirk we are currently fixing up ?

 Or we do think that configuration space regions should come from
 a different resource declared in the ACPI namespace if the regions
 are not MCFG/ECAM compliant (ie config space is not defined through
 MCFG at all - possibly through a _CRS method for a vendor specific
 _HID under the PNP0A03 node ?)
>>>
>>> Hi Lorenzo,
>>>
>>> For X-Gene: the ECAM space is used to access the configuration space
>>> of PCIe devices, with additional help from controller register to
>>> specify the bus, device and function number. Below is the RFC patch
>>> that implements ECAM fixup for X-Gene PCIe controller on top of this
>>> RFC ECAM quirk v3 for your and others reference.
>>
>> Yes, you have an additional resource in the PNP0A03 _CRS to describe
>> your register that is a deliberate abuse of the ACPI standard in
>> that the _CRS is meant to describe resources that are passed on
>> to secondary buses
>
> A potential alternative came up in an off-list discussion: Would it be
> better to hard code the information in the quirk workaround than look it
> up from a repurposed ACPI resource?

Hi Chris, Lorenzo,

Thanks for looking into this.

Yes, I am open for this approach and I think it may work. I can
+ check the pci_config_window resource start address (cfg->res.start)
to figure out the controller and then get the fixed controller
register address
or
+ using the domain number to identify the controller.

Regards,
Duc Dang.
>
> Supporting quirk workarounds for early, non-compliant hardware is
> helpful and perhaps necessary for bootstrapping the ecosystem in a
> timely manner. But we don't really want to provide an expandable or
> reusable interface that would make it easy for new hardware to remain
> non-compliant.
>
> Regards,
> Cov
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Mon, Jun 20, 2016 at 12:12 PM, Duc Dang  wrote:
> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
>  wrote:
>> Hi Duc,
>>
>> On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>>> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
 On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
  wrote:
> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>> From: Tomasz Nowicki 
>>
>> pci_generic_ecam_ops is used by default. Since there are platforms
>> which have non-compliant ECAM space we need to overwrite these
>> accessors prior to PCI buses enumeration. In order to do that
>> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>> we can use proper PCI config space accessors and bus_shift.
>>
>> pci_generic_ecam_ops is still used for platforms free from quirks.
>>
>> Signed-off-by: Tomasz Nowicki 
>> ---
>>  arch/arm64/kernel/pci.c | 7 ---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index 94cd43c..a891bda 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> *root)
>>   struct pci_config_window *cfg;
>>   struct resource cfgres;
>>   unsigned int bsz;
>> + struct pci_ecam_ops *ops;
>>
>>   /* Use address from _CBA if present, otherwise lookup MCFG */
>>   if (!root->mcfg_addr)
>> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> *root)
>>   return NULL;
>>   }
>>
>> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
>> + ops = pci_mcfg_get_ops(root);
>> + bsz = 1 << ops->bus_shift;
>>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>>   cfgres.flags = IORESOURCE_MEM;
>> - cfg = pci_ecam_create(>device->dev, , bus_res,
>> -   _generic_ecam_ops);
>> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>
> Arnd pointed this out already, I think that's the only pending question
> here.
>
> pci_ecam_create() maps ECAM space for config regions retrieved from
> the MCFG, which are *supposed* to be ECAM compliant.
>
> Do we think that's *always* correct/safe regardless of the kind
> of quirk we are currently fixing up ?
>
> Or we do think that configuration space regions should come from
> a different resource declared in the ACPI namespace if the regions
> are not MCFG/ECAM compliant (ie config space is not defined through
> MCFG at all - possibly through a _CRS method for a vendor specific
> _HID under the PNP0A03 node ?)

 Hi Lorenzo,

 For X-Gene: the ECAM space is used to access the configuration space
 of PCIe devices, with additional help from controller register to
 specify the bus, device and function number. Below is the RFC patch
 that implements ECAM fixup for X-Gene PCIe controller on top of this
 RFC ECAM quirk v3 for your and others reference.
>>>
>>> Yes, you have an additional resource in the PNP0A03 _CRS to describe
>>> your register that is a deliberate abuse of the ACPI standard in
>>> that the _CRS is meant to describe resources that are passed on
>>> to secondary buses
>>
>> A potential alternative came up in an off-list discussion: Would it be
>> better to hard code the information in the quirk workaround than look it
>> up from a repurposed ACPI resource?
>
> Hi Chris, Lorenzo,
>
> Thanks for looking into this.
>
> Yes, I am open for this approach and I think it may work. I can
> + check the pci_config_window resource start address (cfg->res.start)
> to figure out the controller and then get the fixed controller
> register address
> or
> + using the domain number to identify the controller.

Hi Chris, Lorenzo,

I reworked the patch to use fix-ed controller address. Please let me know
if you have further comment/concern.

---
[PATCH 1/1] pci: xgene: ECAM fix-up code for X-Gene PCIe

X-Gene PCIe controller does not fully support ECAM.
This patch adds required ECAM fixup to allow X-Gene
PCIe controller to be functional in ACPI boot mode.

Signed-off-by: Duc Dang 
---
v2 changes:
 - Using ECAM base address to identify fixed controller address

 drivers/pci/host/Makefile |   2 +-
 drivers/pci/host/pci-xgene-ecam.c | 291 ++
 2 files changed, 292 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/host/pci-xgene-ecam.c

diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 9c8698e..3480696 100644
--- a/drivers/pci/host/Makefile
+++ 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Mon, Jun 20, 2016 at 12:12 PM, Duc Dang  wrote:
> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
>  wrote:
>> Hi Duc,
>>
>> On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>>> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
 On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
  wrote:
> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>> From: Tomasz Nowicki 
>>
>> pci_generic_ecam_ops is used by default. Since there are platforms
>> which have non-compliant ECAM space we need to overwrite these
>> accessors prior to PCI buses enumeration. In order to do that
>> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>> we can use proper PCI config space accessors and bus_shift.
>>
>> pci_generic_ecam_ops is still used for platforms free from quirks.
>>
>> Signed-off-by: Tomasz Nowicki 
>> ---
>>  arch/arm64/kernel/pci.c | 7 ---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index 94cd43c..a891bda 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> *root)
>>   struct pci_config_window *cfg;
>>   struct resource cfgres;
>>   unsigned int bsz;
>> + struct pci_ecam_ops *ops;
>>
>>   /* Use address from _CBA if present, otherwise lookup MCFG */
>>   if (!root->mcfg_addr)
>> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> *root)
>>   return NULL;
>>   }
>>
>> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
>> + ops = pci_mcfg_get_ops(root);
>> + bsz = 1 << ops->bus_shift;
>>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>>   cfgres.flags = IORESOURCE_MEM;
>> - cfg = pci_ecam_create(>device->dev, , bus_res,
>> -   _generic_ecam_ops);
>> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>
> Arnd pointed this out already, I think that's the only pending question
> here.
>
> pci_ecam_create() maps ECAM space for config regions retrieved from
> the MCFG, which are *supposed* to be ECAM compliant.
>
> Do we think that's *always* correct/safe regardless of the kind
> of quirk we are currently fixing up ?
>
> Or we do think that configuration space regions should come from
> a different resource declared in the ACPI namespace if the regions
> are not MCFG/ECAM compliant (ie config space is not defined through
> MCFG at all - possibly through a _CRS method for a vendor specific
> _HID under the PNP0A03 node ?)

 Hi Lorenzo,

 For X-Gene: the ECAM space is used to access the configuration space
 of PCIe devices, with additional help from controller register to
 specify the bus, device and function number. Below is the RFC patch
 that implements ECAM fixup for X-Gene PCIe controller on top of this
 RFC ECAM quirk v3 for your and others reference.
>>>
>>> Yes, you have an additional resource in the PNP0A03 _CRS to describe
>>> your register that is a deliberate abuse of the ACPI standard in
>>> that the _CRS is meant to describe resources that are passed on
>>> to secondary buses
>>
>> A potential alternative came up in an off-list discussion: Would it be
>> better to hard code the information in the quirk workaround than look it
>> up from a repurposed ACPI resource?
>
> Hi Chris, Lorenzo,
>
> Thanks for looking into this.
>
> Yes, I am open for this approach and I think it may work. I can
> + check the pci_config_window resource start address (cfg->res.start)
> to figure out the controller and then get the fixed controller
> register address
> or
> + using the domain number to identify the controller.

Hi Chris, Lorenzo,

I reworked the patch to use fix-ed controller address. Please let me know
if you have further comment/concern.

---
[PATCH 1/1] pci: xgene: ECAM fix-up code for X-Gene PCIe

X-Gene PCIe controller does not fully support ECAM.
This patch adds required ECAM fixup to allow X-Gene
PCIe controller to be functional in ACPI boot mode.

Signed-off-by: Duc Dang 
---
v2 changes:
 - Using ECAM base address to identify fixed controller address

 drivers/pci/host/Makefile |   2 +-
 drivers/pci/host/pci-xgene-ecam.c | 291 ++
 2 files changed, 292 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/host/pci-xgene-ecam.c

diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 9c8698e..3480696 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -14,7 +14,7 @@ obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Tue, Jun 21, 2016 at 2:26 AM, Lorenzo Pieralisi
 wrote:
> On Mon, Jun 20, 2016 at 12:12:24PM -0700, Duc Dang wrote:
>> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
>>  wrote:
>> > Hi Duc,
>> >
>> > On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>> >> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>> >>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>> >>>  wrote:
>>  On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>> > From: Tomasz Nowicki 
>> >
>> > pci_generic_ecam_ops is used by default. Since there are platforms
>> > which have non-compliant ECAM space we need to overwrite these
>> > accessors prior to PCI buses enumeration. In order to do that
>> > we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>> > we can use proper PCI config space accessors and bus_shift.
>> >
>> > pci_generic_ecam_ops is still used for platforms free from quirks.
>> >
>> > Signed-off-by: Tomasz Nowicki 
>> > ---
>> >  arch/arm64/kernel/pci.c | 7 ---
>> >  1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> > index 94cd43c..a891bda 100644
>> > --- a/arch/arm64/kernel/pci.c
>> > +++ b/arch/arm64/kernel/pci.c
>> > @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> > *root)
>> >   struct pci_config_window *cfg;
>> >   struct resource cfgres;
>> >   unsigned int bsz;
>> > + struct pci_ecam_ops *ops;
>> >
>> >   /* Use address from _CBA if present, otherwise lookup MCFG */
>> >   if (!root->mcfg_addr)
>> > @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> > *root)
>> >   return NULL;
>> >   }
>> >
>> > - bsz = 1 << pci_generic_ecam_ops.bus_shift;
>> > + ops = pci_mcfg_get_ops(root);
>> > + bsz = 1 << ops->bus_shift;
>> >   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>> >   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>> >   cfgres.flags = IORESOURCE_MEM;
>> > - cfg = pci_ecam_create(>device->dev, , bus_res,
>> > -   _generic_ecam_ops);
>> > + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>> 
>>  Arnd pointed this out already, I think that's the only pending question
>>  here.
>> 
>>  pci_ecam_create() maps ECAM space for config regions retrieved from
>>  the MCFG, which are *supposed* to be ECAM compliant.
>> 
>>  Do we think that's *always* correct/safe regardless of the kind
>>  of quirk we are currently fixing up ?
>> 
>>  Or we do think that configuration space regions should come from
>>  a different resource declared in the ACPI namespace if the regions
>>  are not MCFG/ECAM compliant (ie config space is not defined through
>>  MCFG at all - possibly through a _CRS method for a vendor specific
>>  _HID under the PNP0A03 node ?)
>> >>>
>> >>> Hi Lorenzo,
>> >>>
>> >>> For X-Gene: the ECAM space is used to access the configuration space
>> >>> of PCIe devices, with additional help from controller register to
>> >>> specify the bus, device and function number. Below is the RFC patch
>> >>> that implements ECAM fixup for X-Gene PCIe controller on top of this
>> >>> RFC ECAM quirk v3 for your and others reference.
>> >>
>> >> Yes, you have an additional resource in the PNP0A03 _CRS to describe
>> >> your register that is a deliberate abuse of the ACPI standard in
>> >> that the _CRS is meant to describe resources that are passed on
>> >> to secondary buses
>> >
>> > A potential alternative came up in an off-list discussion: Would it be
>> > better to hard code the information in the quirk workaround than look it
>> > up from a repurposed ACPI resource?
>>
>> Hi Chris, Lorenzo,
>>
>> Thanks for looking into this.
>>
>> Yes, I am open for this approach and I think it may work. I can +
>> check the pci_config_window resource start address (cfg->res.start) to
>> figure out the controller and then get the fixed controller register
>> address or + using the domain number to identify the controller.
>
> First thing to do is to remove config space entry from PNP0A03
> _CRS, that's a FW bug.

Yes, I will remove it in the next version of firmware release.

>
> You could use the {bus-range, domain} to get that register address,
> anyway, that's not the main concern here.
>
> Thanks,
> Lorenzo
>
>> Regards,
>> Duc Dang.
>> >
>> > Supporting quirk workarounds for early, non-compliant hardware is
>> > helpful and perhaps necessary for bootstrapping the ecosystem in a
>> > timely manner. But we don't really want to provide an expandable or
>> > reusable interface that would make it easy 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Tue, Jun 21, 2016 at 2:26 AM, Lorenzo Pieralisi
 wrote:
> On Mon, Jun 20, 2016 at 12:12:24PM -0700, Duc Dang wrote:
>> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
>>  wrote:
>> > Hi Duc,
>> >
>> > On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>> >> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>> >>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>> >>>  wrote:
>>  On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>> > From: Tomasz Nowicki 
>> >
>> > pci_generic_ecam_ops is used by default. Since there are platforms
>> > which have non-compliant ECAM space we need to overwrite these
>> > accessors prior to PCI buses enumeration. In order to do that
>> > we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>> > we can use proper PCI config space accessors and bus_shift.
>> >
>> > pci_generic_ecam_ops is still used for platforms free from quirks.
>> >
>> > Signed-off-by: Tomasz Nowicki 
>> > ---
>> >  arch/arm64/kernel/pci.c | 7 ---
>> >  1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> > index 94cd43c..a891bda 100644
>> > --- a/arch/arm64/kernel/pci.c
>> > +++ b/arch/arm64/kernel/pci.c
>> > @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> > *root)
>> >   struct pci_config_window *cfg;
>> >   struct resource cfgres;
>> >   unsigned int bsz;
>> > + struct pci_ecam_ops *ops;
>> >
>> >   /* Use address from _CBA if present, otherwise lookup MCFG */
>> >   if (!root->mcfg_addr)
>> > @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> > *root)
>> >   return NULL;
>> >   }
>> >
>> > - bsz = 1 << pci_generic_ecam_ops.bus_shift;
>> > + ops = pci_mcfg_get_ops(root);
>> > + bsz = 1 << ops->bus_shift;
>> >   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>> >   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>> >   cfgres.flags = IORESOURCE_MEM;
>> > - cfg = pci_ecam_create(>device->dev, , bus_res,
>> > -   _generic_ecam_ops);
>> > + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>> 
>>  Arnd pointed this out already, I think that's the only pending question
>>  here.
>> 
>>  pci_ecam_create() maps ECAM space for config regions retrieved from
>>  the MCFG, which are *supposed* to be ECAM compliant.
>> 
>>  Do we think that's *always* correct/safe regardless of the kind
>>  of quirk we are currently fixing up ?
>> 
>>  Or we do think that configuration space regions should come from
>>  a different resource declared in the ACPI namespace if the regions
>>  are not MCFG/ECAM compliant (ie config space is not defined through
>>  MCFG at all - possibly through a _CRS method for a vendor specific
>>  _HID under the PNP0A03 node ?)
>> >>>
>> >>> Hi Lorenzo,
>> >>>
>> >>> For X-Gene: the ECAM space is used to access the configuration space
>> >>> of PCIe devices, with additional help from controller register to
>> >>> specify the bus, device and function number. Below is the RFC patch
>> >>> that implements ECAM fixup for X-Gene PCIe controller on top of this
>> >>> RFC ECAM quirk v3 for your and others reference.
>> >>
>> >> Yes, you have an additional resource in the PNP0A03 _CRS to describe
>> >> your register that is a deliberate abuse of the ACPI standard in
>> >> that the _CRS is meant to describe resources that are passed on
>> >> to secondary buses
>> >
>> > A potential alternative came up in an off-list discussion: Would it be
>> > better to hard code the information in the quirk workaround than look it
>> > up from a repurposed ACPI resource?
>>
>> Hi Chris, Lorenzo,
>>
>> Thanks for looking into this.
>>
>> Yes, I am open for this approach and I think it may work. I can +
>> check the pci_config_window resource start address (cfg->res.start) to
>> figure out the controller and then get the fixed controller register
>> address or + using the domain number to identify the controller.
>
> First thing to do is to remove config space entry from PNP0A03
> _CRS, that's a FW bug.

Yes, I will remove it in the next version of firmware release.

>
> You could use the {bus-range, domain} to get that register address,
> anyway, that's not the main concern here.
>
> Thanks,
> Lorenzo
>
>> Regards,
>> Duc Dang.
>> >
>> > Supporting quirk workarounds for early, non-compliant hardware is
>> > helpful and perhaps necessary for bootstrapping the ecosystem in a
>> > timely manner. But we don't really want to provide an expandable or
>> > reusable interface that would make it easy for new hardware to remain
>> > non-compliant.
>> >
>> > Regards,
>> > Cov
>> >
>> > --
>> > Qualcomm Innovation 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Lorenzo Pieralisi]

On Mon, Jun 20, 2016 at 12:12:24PM -0700, Duc Dang wrote:
> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
>  wrote:
> > Hi Duc,
> >
> > On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
> >> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
> >>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
> >>>  wrote:
>  On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> > From: Tomasz Nowicki 
> >
> > pci_generic_ecam_ops is used by default. Since there are platforms
> > which have non-compliant ECAM space we need to overwrite these
> > accessors prior to PCI buses enumeration. In order to do that
> > we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> > we can use proper PCI config space accessors and bus_shift.
> >
> > pci_generic_ecam_ops is still used for platforms free from quirks.
> >
> > Signed-off-by: Tomasz Nowicki 
> > ---
> >  arch/arm64/kernel/pci.c | 7 ---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index 94cd43c..a891bda 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> > *root)
> >   struct pci_config_window *cfg;
> >   struct resource cfgres;
> >   unsigned int bsz;
> > + struct pci_ecam_ops *ops;
> >
> >   /* Use address from _CBA if present, otherwise lookup MCFG */
> >   if (!root->mcfg_addr)
> > @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> > *root)
> >   return NULL;
> >   }
> >
> > - bsz = 1 << pci_generic_ecam_ops.bus_shift;
> > + ops = pci_mcfg_get_ops(root);
> > + bsz = 1 << ops->bus_shift;
> >   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
> >   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
> >   cfgres.flags = IORESOURCE_MEM;
> > - cfg = pci_ecam_create(>device->dev, , bus_res,
> > -   _generic_ecam_ops);
> > + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
> 
>  Arnd pointed this out already, I think that's the only pending question
>  here.
> 
>  pci_ecam_create() maps ECAM space for config regions retrieved from
>  the MCFG, which are *supposed* to be ECAM compliant.
> 
>  Do we think that's *always* correct/safe regardless of the kind
>  of quirk we are currently fixing up ?
> 
>  Or we do think that configuration space regions should come from
>  a different resource declared in the ACPI namespace if the regions
>  are not MCFG/ECAM compliant (ie config space is not defined through
>  MCFG at all - possibly through a _CRS method for a vendor specific
>  _HID under the PNP0A03 node ?)
> >>>
> >>> Hi Lorenzo,
> >>>
> >>> For X-Gene: the ECAM space is used to access the configuration space
> >>> of PCIe devices, with additional help from controller register to
> >>> specify the bus, device and function number. Below is the RFC patch
> >>> that implements ECAM fixup for X-Gene PCIe controller on top of this
> >>> RFC ECAM quirk v3 for your and others reference.
> >>
> >> Yes, you have an additional resource in the PNP0A03 _CRS to describe
> >> your register that is a deliberate abuse of the ACPI standard in
> >> that the _CRS is meant to describe resources that are passed on
> >> to secondary buses
> >
> > A potential alternative came up in an off-list discussion: Would it be
> > better to hard code the information in the quirk workaround than look it
> > up from a repurposed ACPI resource?
> 
> Hi Chris, Lorenzo,
> 
> Thanks for looking into this.
> 
> Yes, I am open for this approach and I think it may work. I can +
> check the pci_config_window resource start address (cfg->res.start) to
> figure out the controller and then get the fixed controller register
> address or + using the domain number to identify the controller.

First thing to do is to remove config space entry from PNP0A03
_CRS, that's a FW bug.

You could use the {bus-range, domain} to get that register address,
anyway, that's not the main concern here.

Thanks,
Lorenzo

> Regards,
> Duc Dang.
> >
> > Supporting quirk workarounds for early, non-compliant hardware is
> > helpful and perhaps necessary for bootstrapping the ecosystem in a
> > timely manner. But we don't really want to provide an expandable or
> > reusable interface that would make it easy for new hardware to remain
> > non-compliant.
> >
> > Regards,
> > Cov
> >
> > --
> > Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Lorenzo Pieralisi]

On Mon, Jun 20, 2016 at 12:12:24PM -0700, Duc Dang wrote:
> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
>  wrote:
> > Hi Duc,
> >
> > On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
> >> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
> >>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
> >>>  wrote:
>  On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> > From: Tomasz Nowicki 
> >
> > pci_generic_ecam_ops is used by default. Since there are platforms
> > which have non-compliant ECAM space we need to overwrite these
> > accessors prior to PCI buses enumeration. In order to do that
> > we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> > we can use proper PCI config space accessors and bus_shift.
> >
> > pci_generic_ecam_ops is still used for platforms free from quirks.
> >
> > Signed-off-by: Tomasz Nowicki 
> > ---
> >  arch/arm64/kernel/pci.c | 7 ---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index 94cd43c..a891bda 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> > *root)
> >   struct pci_config_window *cfg;
> >   struct resource cfgres;
> >   unsigned int bsz;
> > + struct pci_ecam_ops *ops;
> >
> >   /* Use address from _CBA if present, otherwise lookup MCFG */
> >   if (!root->mcfg_addr)
> > @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> > *root)
> >   return NULL;
> >   }
> >
> > - bsz = 1 << pci_generic_ecam_ops.bus_shift;
> > + ops = pci_mcfg_get_ops(root);
> > + bsz = 1 << ops->bus_shift;
> >   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
> >   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
> >   cfgres.flags = IORESOURCE_MEM;
> > - cfg = pci_ecam_create(>device->dev, , bus_res,
> > -   _generic_ecam_ops);
> > + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
> 
>  Arnd pointed this out already, I think that's the only pending question
>  here.
> 
>  pci_ecam_create() maps ECAM space for config regions retrieved from
>  the MCFG, which are *supposed* to be ECAM compliant.
> 
>  Do we think that's *always* correct/safe regardless of the kind
>  of quirk we are currently fixing up ?
> 
>  Or we do think that configuration space regions should come from
>  a different resource declared in the ACPI namespace if the regions
>  are not MCFG/ECAM compliant (ie config space is not defined through
>  MCFG at all - possibly through a _CRS method for a vendor specific
>  _HID under the PNP0A03 node ?)
> >>>
> >>> Hi Lorenzo,
> >>>
> >>> For X-Gene: the ECAM space is used to access the configuration space
> >>> of PCIe devices, with additional help from controller register to
> >>> specify the bus, device and function number. Below is the RFC patch
> >>> that implements ECAM fixup for X-Gene PCIe controller on top of this
> >>> RFC ECAM quirk v3 for your and others reference.
> >>
> >> Yes, you have an additional resource in the PNP0A03 _CRS to describe
> >> your register that is a deliberate abuse of the ACPI standard in
> >> that the _CRS is meant to describe resources that are passed on
> >> to secondary buses
> >
> > A potential alternative came up in an off-list discussion: Would it be
> > better to hard code the information in the quirk workaround than look it
> > up from a repurposed ACPI resource?
> 
> Hi Chris, Lorenzo,
> 
> Thanks for looking into this.
> 
> Yes, I am open for this approach and I think it may work. I can +
> check the pci_config_window resource start address (cfg->res.start) to
> figure out the controller and then get the fixed controller register
> address or + using the domain number to identify the controller.

First thing to do is to remove config space entry from PNP0A03
_CRS, that's a FW bug.

You could use the {bus-range, domain} to get that register address,
anyway, that's not the main concern here.

Thanks,
Lorenzo

> Regards,
> Duc Dang.
> >
> > Supporting quirk workarounds for early, non-compliant hardware is
> > helpful and perhaps necessary for bootstrapping the ecosystem in a
> > timely manner. But we don't really want to provide an expandable or
> > reusable interface that would make it easy for new hardware to remain
> > non-compliant.
> >
> > Regards,
> > Cov
> >
> > --
> > Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Tue, Jun 21, 2016 at 2:26 AM, Lorenzo Pieralisi
 wrote:
> On Mon, Jun 20, 2016 at 12:12:24PM -0700, Duc Dang wrote:
>> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
>>  wrote:
>> > Hi Duc,
>> >
>> > On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>> >> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>> >>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>> >>>  wrote:
>>  On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>> > From: Tomasz Nowicki 
>> >
>> > pci_generic_ecam_ops is used by default. Since there are platforms
>> > which have non-compliant ECAM space we need to overwrite these
>> > accessors prior to PCI buses enumeration. In order to do that
>> > we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>> > we can use proper PCI config space accessors and bus_shift.
>> >
>> > pci_generic_ecam_ops is still used for platforms free from quirks.
>> >
>> > Signed-off-by: Tomasz Nowicki 
>> > ---
>> >  arch/arm64/kernel/pci.c | 7 ---
>> >  1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> > index 94cd43c..a891bda 100644
>> > --- a/arch/arm64/kernel/pci.c
>> > +++ b/arch/arm64/kernel/pci.c
>> > @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> > *root)
>> >   struct pci_config_window *cfg;
>> >   struct resource cfgres;
>> >   unsigned int bsz;
>> > + struct pci_ecam_ops *ops;
>> >
>> >   /* Use address from _CBA if present, otherwise lookup MCFG */
>> >   if (!root->mcfg_addr)
>> > @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> > *root)
>> >   return NULL;
>> >   }
>> >
>> > - bsz = 1 << pci_generic_ecam_ops.bus_shift;
>> > + ops = pci_mcfg_get_ops(root);
>> > + bsz = 1 << ops->bus_shift;
>> >   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>> >   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>> >   cfgres.flags = IORESOURCE_MEM;
>> > - cfg = pci_ecam_create(>device->dev, , bus_res,
>> > -   _generic_ecam_ops);
>> > + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>> 
>>  Arnd pointed this out already, I think that's the only pending question
>>  here.
>> 
>>  pci_ecam_create() maps ECAM space for config regions retrieved from
>>  the MCFG, which are *supposed* to be ECAM compliant.
>> 
>>  Do we think that's *always* correct/safe regardless of the kind
>>  of quirk we are currently fixing up ?
>> 
>>  Or we do think that configuration space regions should come from
>>  a different resource declared in the ACPI namespace if the regions
>>  are not MCFG/ECAM compliant (ie config space is not defined through
>>  MCFG at all - possibly through a _CRS method for a vendor specific
>>  _HID under the PNP0A03 node ?)
>> >>>
>> >>> Hi Lorenzo,
>> >>>
>> >>> For X-Gene: the ECAM space is used to access the configuration space
>> >>> of PCIe devices, with additional help from controller register to
>> >>> specify the bus, device and function number. Below is the RFC patch
>> >>> that implements ECAM fixup for X-Gene PCIe controller on top of this
>> >>> RFC ECAM quirk v3 for your and others reference.
>> >>
>> >> Yes, you have an additional resource in the PNP0A03 _CRS to describe
>> >> your register that is a deliberate abuse of the ACPI standard in
>> >> that the _CRS is meant to describe resources that are passed on
>> >> to secondary buses
>> >
>> > A potential alternative came up in an off-list discussion: Would it be
>> > better to hard code the information in the quirk workaround than look it
>> > up from a repurposed ACPI resource?
>>
>> Hi Chris, Lorenzo,
>>
>> Thanks for looking into this.
>>
>> Yes, I am open for this approach and I think it may work. I can +
>> check the pci_config_window resource start address (cfg->res.start) to
>> figure out the controller and then get the fixed controller register
>> address or + using the domain number to identify the controller.
>
> First thing to do is to remove config space entry from PNP0A03
> _CRS, that's a FW bug.

Yes, I will remove it in the next version of firmware release.

>
> You could use the {bus-range, domain} to get that register address,
> anyway, that's not the main concern here.
>
> Thanks,
> Lorenzo
>
>> Regards,
>> Duc Dang.
>> >
>> > Supporting quirk workarounds for early, non-compliant hardware is
>> > helpful and perhaps necessary for bootstrapping the ecosystem in a
>> > timely manner. But we don't really want to provide an expandable or
>> > reusable interface that would make it easy 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Tue, Jun 21, 2016 at 2:26 AM, Lorenzo Pieralisi
 wrote:
> On Mon, Jun 20, 2016 at 12:12:24PM -0700, Duc Dang wrote:
>> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
>>  wrote:
>> > Hi Duc,
>> >
>> > On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>> >> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>> >>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>> >>>  wrote:
>>  On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>> > From: Tomasz Nowicki 
>> >
>> > pci_generic_ecam_ops is used by default. Since there are platforms
>> > which have non-compliant ECAM space we need to overwrite these
>> > accessors prior to PCI buses enumeration. In order to do that
>> > we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>> > we can use proper PCI config space accessors and bus_shift.
>> >
>> > pci_generic_ecam_ops is still used for platforms free from quirks.
>> >
>> > Signed-off-by: Tomasz Nowicki 
>> > ---
>> >  arch/arm64/kernel/pci.c | 7 ---
>> >  1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> > index 94cd43c..a891bda 100644
>> > --- a/arch/arm64/kernel/pci.c
>> > +++ b/arch/arm64/kernel/pci.c
>> > @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> > *root)
>> >   struct pci_config_window *cfg;
>> >   struct resource cfgres;
>> >   unsigned int bsz;
>> > + struct pci_ecam_ops *ops;
>> >
>> >   /* Use address from _CBA if present, otherwise lookup MCFG */
>> >   if (!root->mcfg_addr)
>> > @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> > *root)
>> >   return NULL;
>> >   }
>> >
>> > - bsz = 1 << pci_generic_ecam_ops.bus_shift;
>> > + ops = pci_mcfg_get_ops(root);
>> > + bsz = 1 << ops->bus_shift;
>> >   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>> >   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>> >   cfgres.flags = IORESOURCE_MEM;
>> > - cfg = pci_ecam_create(>device->dev, , bus_res,
>> > -   _generic_ecam_ops);
>> > + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>> 
>>  Arnd pointed this out already, I think that's the only pending question
>>  here.
>> 
>>  pci_ecam_create() maps ECAM space for config regions retrieved from
>>  the MCFG, which are *supposed* to be ECAM compliant.
>> 
>>  Do we think that's *always* correct/safe regardless of the kind
>>  of quirk we are currently fixing up ?
>> 
>>  Or we do think that configuration space regions should come from
>>  a different resource declared in the ACPI namespace if the regions
>>  are not MCFG/ECAM compliant (ie config space is not defined through
>>  MCFG at all - possibly through a _CRS method for a vendor specific
>>  _HID under the PNP0A03 node ?)
>> >>>
>> >>> Hi Lorenzo,
>> >>>
>> >>> For X-Gene: the ECAM space is used to access the configuration space
>> >>> of PCIe devices, with additional help from controller register to
>> >>> specify the bus, device and function number. Below is the RFC patch
>> >>> that implements ECAM fixup for X-Gene PCIe controller on top of this
>> >>> RFC ECAM quirk v3 for your and others reference.
>> >>
>> >> Yes, you have an additional resource in the PNP0A03 _CRS to describe
>> >> your register that is a deliberate abuse of the ACPI standard in
>> >> that the _CRS is meant to describe resources that are passed on
>> >> to secondary buses
>> >
>> > A potential alternative came up in an off-list discussion: Would it be
>> > better to hard code the information in the quirk workaround than look it
>> > up from a repurposed ACPI resource?
>>
>> Hi Chris, Lorenzo,
>>
>> Thanks for looking into this.
>>
>> Yes, I am open for this approach and I think it may work. I can +
>> check the pci_config_window resource start address (cfg->res.start) to
>> figure out the controller and then get the fixed controller register
>> address or + using the domain number to identify the controller.
>
> First thing to do is to remove config space entry from PNP0A03
> _CRS, that's a FW bug.

Yes, I will remove it in the next version of firmware release.

>
> You could use the {bus-range, domain} to get that register address,
> anyway, that's not the main concern here.
>
> Thanks,
> Lorenzo
>
>> Regards,
>> Duc Dang.
>> >
>> > Supporting quirk workarounds for early, non-compliant hardware is
>> > helpful and perhaps necessary for bootstrapping the ecosystem in a
>> > timely manner. But we don't really want to provide an expandable or
>> > reusable interface that would make it easy for new hardware to remain
>> > non-compliant.
>> >
>> > Regards,
>> > Cov
>> >
>> > --
>> > Qualcomm Innovation 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Mon, Jun 20, 2016 at 12:12 PM, Duc Dang  wrote:
> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
>  wrote:
>> Hi Duc,
>>
>> On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>>> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
 On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
  wrote:
> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>> From: Tomasz Nowicki 
>>
>> pci_generic_ecam_ops is used by default. Since there are platforms
>> which have non-compliant ECAM space we need to overwrite these
>> accessors prior to PCI buses enumeration. In order to do that
>> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>> we can use proper PCI config space accessors and bus_shift.
>>
>> pci_generic_ecam_ops is still used for platforms free from quirks.
>>
>> Signed-off-by: Tomasz Nowicki 
>> ---
>>  arch/arm64/kernel/pci.c | 7 ---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index 94cd43c..a891bda 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> *root)
>>   struct pci_config_window *cfg;
>>   struct resource cfgres;
>>   unsigned int bsz;
>> + struct pci_ecam_ops *ops;
>>
>>   /* Use address from _CBA if present, otherwise lookup MCFG */
>>   if (!root->mcfg_addr)
>> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> *root)
>>   return NULL;
>>   }
>>
>> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
>> + ops = pci_mcfg_get_ops(root);
>> + bsz = 1 << ops->bus_shift;
>>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>>   cfgres.flags = IORESOURCE_MEM;
>> - cfg = pci_ecam_create(>device->dev, , bus_res,
>> -   _generic_ecam_ops);
>> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>
> Arnd pointed this out already, I think that's the only pending question
> here.
>
> pci_ecam_create() maps ECAM space for config regions retrieved from
> the MCFG, which are *supposed* to be ECAM compliant.
>
> Do we think that's *always* correct/safe regardless of the kind
> of quirk we are currently fixing up ?
>
> Or we do think that configuration space regions should come from
> a different resource declared in the ACPI namespace if the regions
> are not MCFG/ECAM compliant (ie config space is not defined through
> MCFG at all - possibly through a _CRS method for a vendor specific
> _HID under the PNP0A03 node ?)

 Hi Lorenzo,

 For X-Gene: the ECAM space is used to access the configuration space
 of PCIe devices, with additional help from controller register to
 specify the bus, device and function number. Below is the RFC patch
 that implements ECAM fixup for X-Gene PCIe controller on top of this
 RFC ECAM quirk v3 for your and others reference.
>>>
>>> Yes, you have an additional resource in the PNP0A03 _CRS to describe
>>> your register that is a deliberate abuse of the ACPI standard in
>>> that the _CRS is meant to describe resources that are passed on
>>> to secondary buses
>>
>> A potential alternative came up in an off-list discussion: Would it be
>> better to hard code the information in the quirk workaround than look it
>> up from a repurposed ACPI resource?
>
> Hi Chris, Lorenzo,
>
> Thanks for looking into this.
>
> Yes, I am open for this approach and I think it may work. I can
> + check the pci_config_window resource start address (cfg->res.start)
> to figure out the controller and then get the fixed controller
> register address
> or
> + using the domain number to identify the controller.

Hi Chris, Lorenzo,

I reworked the patch to use fix-ed controller address. Please let me know
if you have further comment/concern.

---
[PATCH 1/1] pci: xgene: ECAM fix-up code for X-Gene PCIe

X-Gene PCIe controller does not fully support ECAM.
This patch adds required ECAM fixup to allow X-Gene
PCIe controller to be functional in ACPI boot mode.

Signed-off-by: Duc Dang 
---
v2 changes:
 - Using ECAM base address to identify fixed controller address

 drivers/pci/host/Makefile |   2 +-
 drivers/pci/host/pci-xgene-ecam.c | 291 ++
 2 files changed, 292 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/host/pci-xgene-ecam.c

diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 9c8698e..3480696 100644
--- a/drivers/pci/host/Makefile
+++ 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Mon, Jun 20, 2016 at 12:12 PM, Duc Dang  wrote:
> On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
>  wrote:
>> Hi Duc,
>>
>> On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>>> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
 On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
  wrote:
> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>> From: Tomasz Nowicki 
>>
>> pci_generic_ecam_ops is used by default. Since there are platforms
>> which have non-compliant ECAM space we need to overwrite these
>> accessors prior to PCI buses enumeration. In order to do that
>> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>> we can use proper PCI config space accessors and bus_shift.
>>
>> pci_generic_ecam_ops is still used for platforms free from quirks.
>>
>> Signed-off-by: Tomasz Nowicki 
>> ---
>>  arch/arm64/kernel/pci.c | 7 ---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index 94cd43c..a891bda 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> *root)
>>   struct pci_config_window *cfg;
>>   struct resource cfgres;
>>   unsigned int bsz;
>> + struct pci_ecam_ops *ops;
>>
>>   /* Use address from _CBA if present, otherwise lookup MCFG */
>>   if (!root->mcfg_addr)
>> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> *root)
>>   return NULL;
>>   }
>>
>> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
>> + ops = pci_mcfg_get_ops(root);
>> + bsz = 1 << ops->bus_shift;
>>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>>   cfgres.flags = IORESOURCE_MEM;
>> - cfg = pci_ecam_create(>device->dev, , bus_res,
>> -   _generic_ecam_ops);
>> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>
> Arnd pointed this out already, I think that's the only pending question
> here.
>
> pci_ecam_create() maps ECAM space for config regions retrieved from
> the MCFG, which are *supposed* to be ECAM compliant.
>
> Do we think that's *always* correct/safe regardless of the kind
> of quirk we are currently fixing up ?
>
> Or we do think that configuration space regions should come from
> a different resource declared in the ACPI namespace if the regions
> are not MCFG/ECAM compliant (ie config space is not defined through
> MCFG at all - possibly through a _CRS method for a vendor specific
> _HID under the PNP0A03 node ?)

 Hi Lorenzo,

 For X-Gene: the ECAM space is used to access the configuration space
 of PCIe devices, with additional help from controller register to
 specify the bus, device and function number. Below is the RFC patch
 that implements ECAM fixup for X-Gene PCIe controller on top of this
 RFC ECAM quirk v3 for your and others reference.
>>>
>>> Yes, you have an additional resource in the PNP0A03 _CRS to describe
>>> your register that is a deliberate abuse of the ACPI standard in
>>> that the _CRS is meant to describe resources that are passed on
>>> to secondary buses
>>
>> A potential alternative came up in an off-list discussion: Would it be
>> better to hard code the information in the quirk workaround than look it
>> up from a repurposed ACPI resource?
>
> Hi Chris, Lorenzo,
>
> Thanks for looking into this.
>
> Yes, I am open for this approach and I think it may work. I can
> + check the pci_config_window resource start address (cfg->res.start)
> to figure out the controller and then get the fixed controller
> register address
> or
> + using the domain number to identify the controller.

Hi Chris, Lorenzo,

I reworked the patch to use fix-ed controller address. Please let me know
if you have further comment/concern.

---
[PATCH 1/1] pci: xgene: ECAM fix-up code for X-Gene PCIe

X-Gene PCIe controller does not fully support ECAM.
This patch adds required ECAM fixup to allow X-Gene
PCIe controller to be functional in ACPI boot mode.

Signed-off-by: Duc Dang 
---
v2 changes:
 - Using ECAM base address to identify fixed controller address

 drivers/pci/host/Makefile |   2 +-
 drivers/pci/host/pci-xgene-ecam.c | 291 ++
 2 files changed, 292 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/host/pci-xgene-ecam.c

diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 9c8698e..3480696 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -14,7 +14,7 @@ obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
 wrote:
> Hi Duc,
>
> On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>>>  wrote:
 On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki 
>
> pci_generic_ecam_ops is used by default. Since there are platforms
> which have non-compliant ECAM space we need to overwrite these
> accessors prior to PCI buses enumeration. In order to do that
> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> we can use proper PCI config space accessors and bus_shift.
>
> pci_generic_ecam_ops is still used for platforms free from quirks.
>
> Signed-off-by: Tomasz Nowicki 
> ---
>  arch/arm64/kernel/pci.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 94cd43c..a891bda 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> *root)
>   struct pci_config_window *cfg;
>   struct resource cfgres;
>   unsigned int bsz;
> + struct pci_ecam_ops *ops;
>
>   /* Use address from _CBA if present, otherwise lookup MCFG */
>   if (!root->mcfg_addr)
> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> *root)
>   return NULL;
>   }
>
> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
> + ops = pci_mcfg_get_ops(root);
> + bsz = 1 << ops->bus_shift;
>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>   cfgres.flags = IORESOURCE_MEM;
> - cfg = pci_ecam_create(>device->dev, , bus_res,
> -   _generic_ecam_ops);
> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);

 Arnd pointed this out already, I think that's the only pending question
 here.

 pci_ecam_create() maps ECAM space for config regions retrieved from
 the MCFG, which are *supposed* to be ECAM compliant.

 Do we think that's *always* correct/safe regardless of the kind
 of quirk we are currently fixing up ?

 Or we do think that configuration space regions should come from
 a different resource declared in the ACPI namespace if the regions
 are not MCFG/ECAM compliant (ie config space is not defined through
 MCFG at all - possibly through a _CRS method for a vendor specific
 _HID under the PNP0A03 node ?)
>>>
>>> Hi Lorenzo,
>>>
>>> For X-Gene: the ECAM space is used to access the configuration space
>>> of PCIe devices, with additional help from controller register to
>>> specify the bus, device and function number. Below is the RFC patch
>>> that implements ECAM fixup for X-Gene PCIe controller on top of this
>>> RFC ECAM quirk v3 for your and others reference.
>>
>> Yes, you have an additional resource in the PNP0A03 _CRS to describe
>> your register that is a deliberate abuse of the ACPI standard in
>> that the _CRS is meant to describe resources that are passed on
>> to secondary buses
>
> A potential alternative came up in an off-list discussion: Would it be
> better to hard code the information in the quirk workaround than look it
> up from a repurposed ACPI resource?

Hi Chris, Lorenzo,

Thanks for looking into this.

Yes, I am open for this approach and I think it may work. I can
+ check the pci_config_window resource start address (cfg->res.start)
to figure out the controller and then get the fixed controller
register address
or
+ using the domain number to identify the controller.

Regards,
Duc Dang.
>
> Supporting quirk workarounds for early, non-compliant hardware is
> helpful and perhaps necessary for bootstrapping the ecosystem in a
> timely manner. But we don't really want to provide an expandable or
> reusable interface that would make it easy for new hardware to remain
> non-compliant.
>
> Regards,
> Cov
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington
 wrote:
> Hi Duc,
>
> On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
>> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>>>  wrote:
 On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki 
>
> pci_generic_ecam_ops is used by default. Since there are platforms
> which have non-compliant ECAM space we need to overwrite these
> accessors prior to PCI buses enumeration. In order to do that
> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> we can use proper PCI config space accessors and bus_shift.
>
> pci_generic_ecam_ops is still used for platforms free from quirks.
>
> Signed-off-by: Tomasz Nowicki 
> ---
>  arch/arm64/kernel/pci.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 94cd43c..a891bda 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> *root)
>   struct pci_config_window *cfg;
>   struct resource cfgres;
>   unsigned int bsz;
> + struct pci_ecam_ops *ops;
>
>   /* Use address from _CBA if present, otherwise lookup MCFG */
>   if (!root->mcfg_addr)
> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> *root)
>   return NULL;
>   }
>
> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
> + ops = pci_mcfg_get_ops(root);
> + bsz = 1 << ops->bus_shift;
>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>   cfgres.flags = IORESOURCE_MEM;
> - cfg = pci_ecam_create(>device->dev, , bus_res,
> -   _generic_ecam_ops);
> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);

 Arnd pointed this out already, I think that's the only pending question
 here.

 pci_ecam_create() maps ECAM space for config regions retrieved from
 the MCFG, which are *supposed* to be ECAM compliant.

 Do we think that's *always* correct/safe regardless of the kind
 of quirk we are currently fixing up ?

 Or we do think that configuration space regions should come from
 a different resource declared in the ACPI namespace if the regions
 are not MCFG/ECAM compliant (ie config space is not defined through
 MCFG at all - possibly through a _CRS method for a vendor specific
 _HID under the PNP0A03 node ?)
>>>
>>> Hi Lorenzo,
>>>
>>> For X-Gene: the ECAM space is used to access the configuration space
>>> of PCIe devices, with additional help from controller register to
>>> specify the bus, device and function number. Below is the RFC patch
>>> that implements ECAM fixup for X-Gene PCIe controller on top of this
>>> RFC ECAM quirk v3 for your and others reference.
>>
>> Yes, you have an additional resource in the PNP0A03 _CRS to describe
>> your register that is a deliberate abuse of the ACPI standard in
>> that the _CRS is meant to describe resources that are passed on
>> to secondary buses
>
> A potential alternative came up in an off-list discussion: Would it be
> better to hard code the information in the quirk workaround than look it
> up from a repurposed ACPI resource?

Hi Chris, Lorenzo,

Thanks for looking into this.

Yes, I am open for this approach and I think it may work. I can
+ check the pci_config_window resource start address (cfg->res.start)
to figure out the controller and then get the fixed controller
register address
or
+ using the domain number to identify the controller.

Regards,
Duc Dang.
>
> Supporting quirk workarounds for early, non-compliant hardware is
> helpful and perhaps necessary for bootstrapping the ecosystem in a
> timely manner. But we don't really want to provide an expandable or
> reusable interface that would make it easy for new hardware to remain
> non-compliant.
>
> Regards,
> Cov
>
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Christopher Covington]

Hi Duc,

On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>>  wrote:
>>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
 From: Tomasz Nowicki 

 pci_generic_ecam_ops is used by default. Since there are platforms
 which have non-compliant ECAM space we need to overwrite these
 accessors prior to PCI buses enumeration. In order to do that
 we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
 we can use proper PCI config space accessors and bus_shift.

 pci_generic_ecam_ops is still used for platforms free from quirks.

 Signed-off-by: Tomasz Nowicki 
 ---
  arch/arm64/kernel/pci.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

 diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
 index 94cd43c..a891bda 100644
 --- a/arch/arm64/kernel/pci.c
 +++ b/arch/arm64/kernel/pci.c
 @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
   struct pci_config_window *cfg;
   struct resource cfgres;
   unsigned int bsz;
 + struct pci_ecam_ops *ops;

   /* Use address from _CBA if present, otherwise lookup MCFG */
   if (!root->mcfg_addr)
 @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
 *root)
   return NULL;
   }

 - bsz = 1 << pci_generic_ecam_ops.bus_shift;
 + ops = pci_mcfg_get_ops(root);
 + bsz = 1 << ops->bus_shift;
   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
   cfgres.flags = IORESOURCE_MEM;
 - cfg = pci_ecam_create(>device->dev, , bus_res,
 -   _generic_ecam_ops);
 + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>>>
>>> Arnd pointed this out already, I think that's the only pending question
>>> here.
>>>
>>> pci_ecam_create() maps ECAM space for config regions retrieved from
>>> the MCFG, which are *supposed* to be ECAM compliant.
>>>
>>> Do we think that's *always* correct/safe regardless of the kind
>>> of quirk we are currently fixing up ?
>>>
>>> Or we do think that configuration space regions should come from
>>> a different resource declared in the ACPI namespace if the regions
>>> are not MCFG/ECAM compliant (ie config space is not defined through
>>> MCFG at all - possibly through a _CRS method for a vendor specific
>>> _HID under the PNP0A03 node ?)
>>
>> Hi Lorenzo,
>>
>> For X-Gene: the ECAM space is used to access the configuration space
>> of PCIe devices, with additional help from controller register to
>> specify the bus, device and function number. Below is the RFC patch
>> that implements ECAM fixup for X-Gene PCIe controller on top of this
>> RFC ECAM quirk v3 for your and others reference.
> 
> Yes, you have an additional resource in the PNP0A03 _CRS to describe
> your register that is a deliberate abuse of the ACPI standard in
> that the _CRS is meant to describe resources that are passed on
> to secondary buses

A potential alternative came up in an off-list discussion: Would it be
better to hard code the information in the quirk workaround than look it
up from a repurposed ACPI resource?

Supporting quirk workarounds for early, non-compliant hardware is
helpful and perhaps necessary for bootstrapping the ecosystem in a
timely manner. But we don't really want to provide an expandable or
reusable interface that would make it easy for new hardware to remain
non-compliant.

Regards,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Christopher Covington]

Hi Duc,

On 06/20/2016 05:42 AM, Lorenzo Pieralisi wrote:
> On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
>> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>>  wrote:
>>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
 From: Tomasz Nowicki 

 pci_generic_ecam_ops is used by default. Since there are platforms
 which have non-compliant ECAM space we need to overwrite these
 accessors prior to PCI buses enumeration. In order to do that
 we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
 we can use proper PCI config space accessors and bus_shift.

 pci_generic_ecam_ops is still used for platforms free from quirks.

 Signed-off-by: Tomasz Nowicki 
 ---
  arch/arm64/kernel/pci.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

 diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
 index 94cd43c..a891bda 100644
 --- a/arch/arm64/kernel/pci.c
 +++ b/arch/arm64/kernel/pci.c
 @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
   struct pci_config_window *cfg;
   struct resource cfgres;
   unsigned int bsz;
 + struct pci_ecam_ops *ops;

   /* Use address from _CBA if present, otherwise lookup MCFG */
   if (!root->mcfg_addr)
 @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
 *root)
   return NULL;
   }

 - bsz = 1 << pci_generic_ecam_ops.bus_shift;
 + ops = pci_mcfg_get_ops(root);
 + bsz = 1 << ops->bus_shift;
   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
   cfgres.flags = IORESOURCE_MEM;
 - cfg = pci_ecam_create(>device->dev, , bus_res,
 -   _generic_ecam_ops);
 + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>>>
>>> Arnd pointed this out already, I think that's the only pending question
>>> here.
>>>
>>> pci_ecam_create() maps ECAM space for config regions retrieved from
>>> the MCFG, which are *supposed* to be ECAM compliant.
>>>
>>> Do we think that's *always* correct/safe regardless of the kind
>>> of quirk we are currently fixing up ?
>>>
>>> Or we do think that configuration space regions should come from
>>> a different resource declared in the ACPI namespace if the regions
>>> are not MCFG/ECAM compliant (ie config space is not defined through
>>> MCFG at all - possibly through a _CRS method for a vendor specific
>>> _HID under the PNP0A03 node ?)
>>
>> Hi Lorenzo,
>>
>> For X-Gene: the ECAM space is used to access the configuration space
>> of PCIe devices, with additional help from controller register to
>> specify the bus, device and function number. Below is the RFC patch
>> that implements ECAM fixup for X-Gene PCIe controller on top of this
>> RFC ECAM quirk v3 for your and others reference.
> 
> Yes, you have an additional resource in the PNP0A03 _CRS to describe
> your register that is a deliberate abuse of the ACPI standard in
> that the _CRS is meant to describe resources that are passed on
> to secondary buses

A potential alternative came up in an off-list discussion: Would it be
better to hard code the information in the quirk workaround than look it
up from a repurposed ACPI resource?

Supporting quirk workarounds for early, non-compliant hardware is
helpful and perhaps necessary for bootstrapping the ecosystem in a
timely manner. But we don't really want to provide an expandable or
reusable interface that would make it easy for new hardware to remain
non-compliant.

Regards,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Lorenzo Pieralisi]

On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>  wrote:
> > On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> >> From: Tomasz Nowicki 
> >>
> >> pci_generic_ecam_ops is used by default. Since there are platforms
> >> which have non-compliant ECAM space we need to overwrite these
> >> accessors prior to PCI buses enumeration. In order to do that
> >> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> >> we can use proper PCI config space accessors and bus_shift.
> >>
> >> pci_generic_ecam_ops is still used for platforms free from quirks.
> >>
> >> Signed-off-by: Tomasz Nowicki 
> >> ---
> >>  arch/arm64/kernel/pci.c | 7 ---
> >>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> >> index 94cd43c..a891bda 100644
> >> --- a/arch/arm64/kernel/pci.c
> >> +++ b/arch/arm64/kernel/pci.c
> >> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
> >>   struct pci_config_window *cfg;
> >>   struct resource cfgres;
> >>   unsigned int bsz;
> >> + struct pci_ecam_ops *ops;
> >>
> >>   /* Use address from _CBA if present, otherwise lookup MCFG */
> >>   if (!root->mcfg_addr)
> >> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> >> *root)
> >>   return NULL;
> >>   }
> >>
> >> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
> >> + ops = pci_mcfg_get_ops(root);
> >> + bsz = 1 << ops->bus_shift;
> >>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
> >>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
> >>   cfgres.flags = IORESOURCE_MEM;
> >> - cfg = pci_ecam_create(>device->dev, , bus_res,
> >> -   _generic_ecam_ops);
> >> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
> >
> > Arnd pointed this out already, I think that's the only pending question
> > here.
> >
> > pci_ecam_create() maps ECAM space for config regions retrieved from
> > the MCFG, which are *supposed* to be ECAM compliant.
> >
> > Do we think that's *always* correct/safe regardless of the kind
> > of quirk we are currently fixing up ?
> >
> > Or we do think that configuration space regions should come from
> > a different resource declared in the ACPI namespace if the regions
> > are not MCFG/ECAM compliant (ie config space is not defined through
> > MCFG at all - possibly through a _CRS method for a vendor specific
> > _HID under the PNP0A03 node ?)
> 
> Hi Lorenzo,
> 
> For X-Gene: the ECAM space is used to access the configuration space
> of PCIe devices, with additional help from controller register to
> specify the bus, device and function number. Below is the RFC patch
> that implements ECAM fixup for X-Gene PCIe controller on top of this
> RFC ECAM quirk v3 for your and others reference.

Yes, you have an additional resource in the PNP0A03 _CRS to describe
your register that is a deliberate abuse of the ACPI standard in
that the _CRS is meant to describe resources that are passed on
to secondary buses

> 
> ---
> From 7a72c122e06aab024497c90fb31790110bebb666 Mon Sep 17 00:00:00 2001
> From: Duc Dang 
> Date: Wed, 4 May 2016 09:54:00 -0700
> Subject: [PATCH 1/1] pci: xgene: ECAM fix-up code for X-Gene PCIe
> 
> X-Gene PCIe controller does not fully support ECAM.
> This patch adds required ECAM fixup to allow X-Gene
> PCIe controller to be functional in ACPI boot mode.
> 
> Signed-off-by: Duc Dang 
> ---
>  drivers/pci/host/Makefile |   2 +-
>  drivers/pci/host/pci-xgene-ecam.c | 198 
> ++
>  2 files changed, 199 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pci/host/pci-xgene-ecam.c
> 
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 9c8698e..3480696 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -14,7 +14,7 @@ obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
>  obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
>  obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
> -obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
> +obj-$(CONFIG_PCI_XGENE) += pci-xgene.o pci-xgene-ecam.o
>  obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
>  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>  obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
> diff --git a/drivers/pci/host/pci-xgene-ecam.c
> b/drivers/pci/host/pci-xgene-ecam.c
> new file mode 100644
> index 000..253a5c5
> --- /dev/null
> +++ b/drivers/pci/host/pci-xgene-ecam.c
> @@ -0,0 +1,198 @@
> +/*
> + * APM X-Gene PCIe ECAM fixup driver
> + *
> + * Copyright (c) 2016, Applied Micro Circuits Corporation
> + * Author:
> + *Duc Dang 
> + *
> + * This program is free software; you can 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Lorenzo Pieralisi]

On Fri, Jun 17, 2016 at 02:37:02PM -0700, Duc Dang wrote:
> On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
>  wrote:
> > On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> >> From: Tomasz Nowicki 
> >>
> >> pci_generic_ecam_ops is used by default. Since there are platforms
> >> which have non-compliant ECAM space we need to overwrite these
> >> accessors prior to PCI buses enumeration. In order to do that
> >> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> >> we can use proper PCI config space accessors and bus_shift.
> >>
> >> pci_generic_ecam_ops is still used for platforms free from quirks.
> >>
> >> Signed-off-by: Tomasz Nowicki 
> >> ---
> >>  arch/arm64/kernel/pci.c | 7 ---
> >>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> >> index 94cd43c..a891bda 100644
> >> --- a/arch/arm64/kernel/pci.c
> >> +++ b/arch/arm64/kernel/pci.c
> >> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
> >>   struct pci_config_window *cfg;
> >>   struct resource cfgres;
> >>   unsigned int bsz;
> >> + struct pci_ecam_ops *ops;
> >>
> >>   /* Use address from _CBA if present, otherwise lookup MCFG */
> >>   if (!root->mcfg_addr)
> >> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
> >> *root)
> >>   return NULL;
> >>   }
> >>
> >> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
> >> + ops = pci_mcfg_get_ops(root);
> >> + bsz = 1 << ops->bus_shift;
> >>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
> >>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
> >>   cfgres.flags = IORESOURCE_MEM;
> >> - cfg = pci_ecam_create(>device->dev, , bus_res,
> >> -   _generic_ecam_ops);
> >> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
> >
> > Arnd pointed this out already, I think that's the only pending question
> > here.
> >
> > pci_ecam_create() maps ECAM space for config regions retrieved from
> > the MCFG, which are *supposed* to be ECAM compliant.
> >
> > Do we think that's *always* correct/safe regardless of the kind
> > of quirk we are currently fixing up ?
> >
> > Or we do think that configuration space regions should come from
> > a different resource declared in the ACPI namespace if the regions
> > are not MCFG/ECAM compliant (ie config space is not defined through
> > MCFG at all - possibly through a _CRS method for a vendor specific
> > _HID under the PNP0A03 node ?)
> 
> Hi Lorenzo,
> 
> For X-Gene: the ECAM space is used to access the configuration space
> of PCIe devices, with additional help from controller register to
> specify the bus, device and function number. Below is the RFC patch
> that implements ECAM fixup for X-Gene PCIe controller on top of this
> RFC ECAM quirk v3 for your and others reference.

Yes, you have an additional resource in the PNP0A03 _CRS to describe
your register that is a deliberate abuse of the ACPI standard in
that the _CRS is meant to describe resources that are passed on
to secondary buses

> 
> ---
> From 7a72c122e06aab024497c90fb31790110bebb666 Mon Sep 17 00:00:00 2001
> From: Duc Dang 
> Date: Wed, 4 May 2016 09:54:00 -0700
> Subject: [PATCH 1/1] pci: xgene: ECAM fix-up code for X-Gene PCIe
> 
> X-Gene PCIe controller does not fully support ECAM.
> This patch adds required ECAM fixup to allow X-Gene
> PCIe controller to be functional in ACPI boot mode.
> 
> Signed-off-by: Duc Dang 
> ---
>  drivers/pci/host/Makefile |   2 +-
>  drivers/pci/host/pci-xgene-ecam.c | 198 
> ++
>  2 files changed, 199 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pci/host/pci-xgene-ecam.c
> 
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 9c8698e..3480696 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -14,7 +14,7 @@ obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
>  obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
>  obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
> -obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
> +obj-$(CONFIG_PCI_XGENE) += pci-xgene.o pci-xgene-ecam.o
>  obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
>  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>  obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
> diff --git a/drivers/pci/host/pci-xgene-ecam.c
> b/drivers/pci/host/pci-xgene-ecam.c
> new file mode 100644
> index 000..253a5c5
> --- /dev/null
> +++ b/drivers/pci/host/pci-xgene-ecam.c
> @@ -0,0 +1,198 @@
> +/*
> + * APM X-Gene PCIe ECAM fixup driver
> + *
> + * Copyright (c) 2016, Applied Micro Circuits Corporation
> + * Author:
> + *Duc Dang 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
 wrote:
> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>> From: Tomasz Nowicki 
>>
>> pci_generic_ecam_ops is used by default. Since there are platforms
>> which have non-compliant ECAM space we need to overwrite these
>> accessors prior to PCI buses enumeration. In order to do that
>> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>> we can use proper PCI config space accessors and bus_shift.
>>
>> pci_generic_ecam_ops is still used for platforms free from quirks.
>>
>> Signed-off-by: Tomasz Nowicki 
>> ---
>>  arch/arm64/kernel/pci.c | 7 ---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index 94cd43c..a891bda 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>>   struct pci_config_window *cfg;
>>   struct resource cfgres;
>>   unsigned int bsz;
>> + struct pci_ecam_ops *ops;
>>
>>   /* Use address from _CBA if present, otherwise lookup MCFG */
>>   if (!root->mcfg_addr)
>> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>>   return NULL;
>>   }
>>
>> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
>> + ops = pci_mcfg_get_ops(root);
>> + bsz = 1 << ops->bus_shift;
>>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>>   cfgres.flags = IORESOURCE_MEM;
>> - cfg = pci_ecam_create(>device->dev, , bus_res,
>> -   _generic_ecam_ops);
>> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>
> Arnd pointed this out already, I think that's the only pending question
> here.
>
> pci_ecam_create() maps ECAM space for config regions retrieved from
> the MCFG, which are *supposed* to be ECAM compliant.
>
> Do we think that's *always* correct/safe regardless of the kind
> of quirk we are currently fixing up ?
>
> Or we do think that configuration space regions should come from
> a different resource declared in the ACPI namespace if the regions
> are not MCFG/ECAM compliant (ie config space is not defined through
> MCFG at all - possibly through a _CRS method for a vendor specific
> _HID under the PNP0A03 node ?)

Hi Lorenzo,

For X-Gene: the ECAM space is used to access the configuration space of PCIe
devices, with additional help from controller register to specify the
bus, device and
function number. Below is the RFC patch that implements ECAM fixup for X-Gene
PCIe controller on top of this RFC ECAM quirk v3 for your and others reference.

---
>From 7a72c122e06aab024497c90fb31790110bebb666 Mon Sep 17 00:00:00 2001
From: Duc Dang 
Date: Wed, 4 May 2016 09:54:00 -0700
Subject: [PATCH 1/1] pci: xgene: ECAM fix-up code for X-Gene PCIe

X-Gene PCIe controller does not fully support ECAM.
This patch adds required ECAM fixup to allow X-Gene
PCIe controller to be functional in ACPI boot mode.

Signed-off-by: Duc Dang 
---
 drivers/pci/host/Makefile |   2 +-
 drivers/pci/host/pci-xgene-ecam.c | 198 ++
 2 files changed, 199 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/host/pci-xgene-ecam.c

diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 9c8698e..3480696 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -14,7 +14,7 @@ obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
 obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
 obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
 obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
-obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
+obj-$(CONFIG_PCI_XGENE) += pci-xgene.o pci-xgene-ecam.o
 obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
 obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
diff --git a/drivers/pci/host/pci-xgene-ecam.c
b/drivers/pci/host/pci-xgene-ecam.c
new file mode 100644
index 000..253a5c5
--- /dev/null
+++ b/drivers/pci/host/pci-xgene-ecam.c
@@ -0,0 +1,198 @@
+/*
+ * APM X-Gene PCIe ECAM fixup driver
+ *
+ * Copyright (c) 2016, Applied Micro Circuits Corporation
+ * Author:
+ *Duc Dang 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Duc Dang]

On Thu, Jun 16, 2016 at 10:48 AM, Lorenzo Pieralisi
 wrote:
> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>> From: Tomasz Nowicki 
>>
>> pci_generic_ecam_ops is used by default. Since there are platforms
>> which have non-compliant ECAM space we need to overwrite these
>> accessors prior to PCI buses enumeration. In order to do that
>> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>> we can use proper PCI config space accessors and bus_shift.
>>
>> pci_generic_ecam_ops is still used for platforms free from quirks.
>>
>> Signed-off-by: Tomasz Nowicki 
>> ---
>>  arch/arm64/kernel/pci.c | 7 ---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index 94cd43c..a891bda 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>>   struct pci_config_window *cfg;
>>   struct resource cfgres;
>>   unsigned int bsz;
>> + struct pci_ecam_ops *ops;
>>
>>   /* Use address from _CBA if present, otherwise lookup MCFG */
>>   if (!root->mcfg_addr)
>> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>>   return NULL;
>>   }
>>
>> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
>> + ops = pci_mcfg_get_ops(root);
>> + bsz = 1 << ops->bus_shift;
>>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>>   cfgres.flags = IORESOURCE_MEM;
>> - cfg = pci_ecam_create(>device->dev, , bus_res,
>> -   _generic_ecam_ops);
>> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>
> Arnd pointed this out already, I think that's the only pending question
> here.
>
> pci_ecam_create() maps ECAM space for config regions retrieved from
> the MCFG, which are *supposed* to be ECAM compliant.
>
> Do we think that's *always* correct/safe regardless of the kind
> of quirk we are currently fixing up ?
>
> Or we do think that configuration space regions should come from
> a different resource declared in the ACPI namespace if the regions
> are not MCFG/ECAM compliant (ie config space is not defined through
> MCFG at all - possibly through a _CRS method for a vendor specific
> _HID under the PNP0A03 node ?)

Hi Lorenzo,

For X-Gene: the ECAM space is used to access the configuration space of PCIe
devices, with additional help from controller register to specify the
bus, device and
function number. Below is the RFC patch that implements ECAM fixup for X-Gene
PCIe controller on top of this RFC ECAM quirk v3 for your and others reference.

---
>From 7a72c122e06aab024497c90fb31790110bebb666 Mon Sep 17 00:00:00 2001
From: Duc Dang 
Date: Wed, 4 May 2016 09:54:00 -0700
Subject: [PATCH 1/1] pci: xgene: ECAM fix-up code for X-Gene PCIe

X-Gene PCIe controller does not fully support ECAM.
This patch adds required ECAM fixup to allow X-Gene
PCIe controller to be functional in ACPI boot mode.

Signed-off-by: Duc Dang 
---
 drivers/pci/host/Makefile |   2 +-
 drivers/pci/host/pci-xgene-ecam.c | 198 ++
 2 files changed, 199 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/host/pci-xgene-ecam.c

diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 9c8698e..3480696 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -14,7 +14,7 @@ obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
 obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
 obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
 obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
-obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
+obj-$(CONFIG_PCI_XGENE) += pci-xgene.o pci-xgene-ecam.o
 obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
 obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
diff --git a/drivers/pci/host/pci-xgene-ecam.c
b/drivers/pci/host/pci-xgene-ecam.c
new file mode 100644
index 000..253a5c5
--- /dev/null
+++ b/drivers/pci/host/pci-xgene-ecam.c
@@ -0,0 +1,198 @@
+/*
+ * APM X-Gene PCIe ECAM fixup driver
+ *
+ * Copyright (c) 2016, Applied Micro Circuits Corporation
+ * Author:
+ *Duc Dang 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Christopher Covington]

On 06/17/2016 04:01 AM, Gabriele Paoloni wrote:
> Hi Lorenzo and All
> 
>> -Original Message-
>> From: Lorenzo Pieralisi [mailto:lorenzo.pieral...@arm.com]
>> Sent: 16 June 2016 18:49
>> To: Christopher Covington
>> Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo;
>> Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit;
>> Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm-
>> ker...@lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas;
>> Ganapatrao Kulkarni; linux-kernel@vger.kernel.org
>> Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling
>> for ACPI based PCI host controller
>>
>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>>> From: Tomasz Nowicki <t...@semihalf.com>
>>>
>>> pci_generic_ecam_ops is used by default. Since there are platforms
>>> which have non-compliant ECAM space we need to overwrite these
>>> accessors prior to PCI buses enumeration. In order to do that
>>> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>>> we can use proper PCI config space accessors and bus_shift.
>>>
>>> pci_generic_ecam_ops is still used for platforms free from quirks.
>>>
>>> Signed-off-by: Tomasz Nowicki <t...@semihalf.com>
>>> ---
>>>  arch/arm64/kernel/pci.c | 7 ---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>>> index 94cd43c..a891bda 100644
>>> --- a/arch/arm64/kernel/pci.c
>>> +++ b/arch/arm64/kernel/pci.c
>>> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root
>> *root)
>>> struct pci_config_window *cfg;
>>> struct resource cfgres;
>>> unsigned int bsz;
>>> +   struct pci_ecam_ops *ops;
>>>
>>> /* Use address from _CBA if present, otherwise lookup MCFG */
>>> if (!root->mcfg_addr)
>>> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct
>> acpi_pci_root *root)
>>> return NULL;
>>> }
>>>
>>> -   bsz = 1 << pci_generic_ecam_ops.bus_shift;
>>> +   ops = pci_mcfg_get_ops(root);
>>> +   bsz = 1 << ops->bus_shift;
>>> cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>>> cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>>> cfgres.flags = IORESOURCE_MEM;
>>> -   cfg = pci_ecam_create(>device->dev, , bus_res,
>>> - _generic_ecam_ops);
>>> +   cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>>
>> Arnd pointed this out already, I think that's the only pending question
>> here.
>>
>> pci_ecam_create() maps ECAM space for config regions retrieved from
>> the MCFG, which are *supposed* to be ECAM compliant.
>>
>> Do we think that's *always* correct/safe regardless of the kind
>> of quirk we are currently fixing up ?

> From my perspective I think the easiest solution is to keep this
> quirk mechanism in place and then review vendor by vendor solution
> as they are pushed to the mailing list; if some vendors are abusing
> of some addresses/resources then they can be rejected...

Using pci_ecam_create() works well for QDF2432:

https://us.codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-rc3-testing=ca7439a54ddd8612b435c797ffc54f4e19f03e2b

Cheers,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Christopher Covington]

On 06/17/2016 04:01 AM, Gabriele Paoloni wrote:
> Hi Lorenzo and All
> 
>> -Original Message-
>> From: Lorenzo Pieralisi [mailto:lorenzo.pieral...@arm.com]
>> Sent: 16 June 2016 18:49
>> To: Christopher Covington
>> Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo;
>> Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit;
>> Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm-
>> ker...@lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas;
>> Ganapatrao Kulkarni; linux-kernel@vger.kernel.org
>> Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling
>> for ACPI based PCI host controller
>>
>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>>> From: Tomasz Nowicki 
>>>
>>> pci_generic_ecam_ops is used by default. Since there are platforms
>>> which have non-compliant ECAM space we need to overwrite these
>>> accessors prior to PCI buses enumeration. In order to do that
>>> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>>> we can use proper PCI config space accessors and bus_shift.
>>>
>>> pci_generic_ecam_ops is still used for platforms free from quirks.
>>>
>>> Signed-off-by: Tomasz Nowicki 
>>> ---
>>>  arch/arm64/kernel/pci.c | 7 ---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>>> index 94cd43c..a891bda 100644
>>> --- a/arch/arm64/kernel/pci.c
>>> +++ b/arch/arm64/kernel/pci.c
>>> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root
>> *root)
>>> struct pci_config_window *cfg;
>>> struct resource cfgres;
>>> unsigned int bsz;
>>> +   struct pci_ecam_ops *ops;
>>>
>>> /* Use address from _CBA if present, otherwise lookup MCFG */
>>> if (!root->mcfg_addr)
>>> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct
>> acpi_pci_root *root)
>>> return NULL;
>>> }
>>>
>>> -   bsz = 1 << pci_generic_ecam_ops.bus_shift;
>>> +   ops = pci_mcfg_get_ops(root);
>>> +   bsz = 1 << ops->bus_shift;
>>> cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>>> cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>>> cfgres.flags = IORESOURCE_MEM;
>>> -   cfg = pci_ecam_create(>device->dev, , bus_res,
>>> - _generic_ecam_ops);
>>> +   cfg = pci_ecam_create(>device->dev, , bus_res, ops);
>>
>> Arnd pointed this out already, I think that's the only pending question
>> here.
>>
>> pci_ecam_create() maps ECAM space for config regions retrieved from
>> the MCFG, which are *supposed* to be ECAM compliant.
>>
>> Do we think that's *always* correct/safe regardless of the kind
>> of quirk we are currently fixing up ?

> From my perspective I think the easiest solution is to keep this
> quirk mechanism in place and then review vendor by vendor solution
> as they are pushed to the mailing list; if some vendors are abusing
> of some addresses/resources then they can be rejected...

Using pci_ecam_create() works well for QDF2432:

https://us.codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-rc3-testing=ca7439a54ddd8612b435c797ffc54f4e19f03e2b

Cheers,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Gabriele Paoloni]

Hi Lorenzo and All

> -Original Message-
> From: Lorenzo Pieralisi [mailto:lorenzo.pieral...@arm.com]
> Sent: 16 June 2016 18:49
> To: Christopher Covington
> Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo;
> Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit;
> Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm-
> ker...@lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas;
> Ganapatrao Kulkarni; linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling
> for ACPI based PCI host controller
> 
> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> > From: Tomasz Nowicki <t...@semihalf.com>
> >
> > pci_generic_ecam_ops is used by default. Since there are platforms
> > which have non-compliant ECAM space we need to overwrite these
> > accessors prior to PCI buses enumeration. In order to do that
> > we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> > we can use proper PCI config space accessors and bus_shift.
> >
> > pci_generic_ecam_ops is still used for platforms free from quirks.
> >
> > Signed-off-by: Tomasz Nowicki <t...@semihalf.com>
> > ---
> >  arch/arm64/kernel/pci.c | 7 ---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index 94cd43c..a891bda 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root
> *root)
> > struct pci_config_window *cfg;
> > struct resource cfgres;
> > unsigned int bsz;
> > +   struct pci_ecam_ops *ops;
> >
> > /* Use address from _CBA if present, otherwise lookup MCFG */
> > if (!root->mcfg_addr)
> > @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct
> acpi_pci_root *root)
> > return NULL;
> > }
> >
> > -   bsz = 1 << pci_generic_ecam_ops.bus_shift;
> > +   ops = pci_mcfg_get_ops(root);
> > +   bsz = 1 << ops->bus_shift;
> > cfgres.start = root->mcfg_addr + bus_res->start * bsz;
> > cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
> > cfgres.flags = IORESOURCE_MEM;
> > -   cfg = pci_ecam_create(>device->dev, , bus_res,
> > - _generic_ecam_ops);
> > +   cfg = pci_ecam_create(>device->dev, , bus_res, ops);
> 
> Arnd pointed this out already, I think that's the only pending question
> here.
> 
> pci_ecam_create() maps ECAM space for config regions retrieved from
> the MCFG, which are *supposed* to be ECAM compliant.
> 
> Do we think that's *always* correct/safe regardless of the kind
> of quirk we are currently fixing up ?

I didn't dig into the other vendors' quirk mechanism but I will 
quickly explain what we (would like to) do in HiSilicon Hip05/Hip06
SoCs.

>From our perspective we use ECAM access mechanism for all the MCFG
buses except the root ports.

For the root ports we declare additional ACPI devices marked as
"pnp0c02" (motherboard reserved resource) i.e.:

Scope(_SB) 
{
// PCIe Root bus 
Device (PCI1) 
{ 
Name (_HID, "HISI0080") // PCI Express Root Bridge 
Name (_CID, "PNP0A03") // Compatible PCI Root Bridge 
Name(_SEG, 1) // Segment of this Root complex 
Name(_BBN, 64) // Base Bus Number 
Name(_CCA, 1)
Method (_CRS, 0, Serialized) { // Root complex resources 
Name (RBUF, ResourceTemplate () { 

[...]

)
}) // Name(RBUF)
Return (RBUF)
} // Method(_CRS)

Device (RES1)
{
Name (_HID, "HISI0081") // HiSi PCIe RC config base 
address
Name (_CID, "PNP0C02")  // Motherboard reserved resource
Name (_CRS, ResourceTemplate (){
Memory32Fixed (ReadWrite, 0xb008 , 0x1)
})
}

 [...]

} // Device(PCI1)



Therefore we declare a perfectly ECAM compliant MCFG and we "waste"
the root ports addresses as in practice for the root ports we replace
the MCFG address with the one retrieved from the ACPI device above.

In terms of "safety" I think it should be ok as in practice we are
reserving some addresses in MCFG that we do not use and the ones
that are used are reserve

RE: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Gabriele Paoloni]

Hi Lorenzo and All

> -Original Message-
> From: Lorenzo Pieralisi [mailto:lorenzo.pieral...@arm.com]
> Sent: 16 June 2016 18:49
> To: Christopher Covington
> Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo;
> Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit;
> Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm-
> ker...@lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas;
> Ganapatrao Kulkarni; linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling
> for ACPI based PCI host controller
> 
> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> > From: Tomasz Nowicki 
> >
> > pci_generic_ecam_ops is used by default. Since there are platforms
> > which have non-compliant ECAM space we need to overwrite these
> > accessors prior to PCI buses enumeration. In order to do that
> > we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> > we can use proper PCI config space accessors and bus_shift.
> >
> > pci_generic_ecam_ops is still used for platforms free from quirks.
> >
> > Signed-off-by: Tomasz Nowicki 
> > ---
> >  arch/arm64/kernel/pci.c | 7 ---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index 94cd43c..a891bda 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root
> *root)
> > struct pci_config_window *cfg;
> > struct resource cfgres;
> > unsigned int bsz;
> > +   struct pci_ecam_ops *ops;
> >
> > /* Use address from _CBA if present, otherwise lookup MCFG */
> > if (!root->mcfg_addr)
> > @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct
> acpi_pci_root *root)
> > return NULL;
> > }
> >
> > -   bsz = 1 << pci_generic_ecam_ops.bus_shift;
> > +   ops = pci_mcfg_get_ops(root);
> > +   bsz = 1 << ops->bus_shift;
> > cfgres.start = root->mcfg_addr + bus_res->start * bsz;
> > cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
> > cfgres.flags = IORESOURCE_MEM;
> > -   cfg = pci_ecam_create(>device->dev, , bus_res,
> > - _generic_ecam_ops);
> > +   cfg = pci_ecam_create(>device->dev, , bus_res, ops);
> 
> Arnd pointed this out already, I think that's the only pending question
> here.
> 
> pci_ecam_create() maps ECAM space for config regions retrieved from
> the MCFG, which are *supposed* to be ECAM compliant.
> 
> Do we think that's *always* correct/safe regardless of the kind
> of quirk we are currently fixing up ?

I didn't dig into the other vendors' quirk mechanism but I will 
quickly explain what we (would like to) do in HiSilicon Hip05/Hip06
SoCs.

>From our perspective we use ECAM access mechanism for all the MCFG
buses except the root ports.

For the root ports we declare additional ACPI devices marked as
"pnp0c02" (motherboard reserved resource) i.e.:

Scope(_SB) 
{
// PCIe Root bus 
Device (PCI1) 
{ 
Name (_HID, "HISI0080") // PCI Express Root Bridge 
Name (_CID, "PNP0A03") // Compatible PCI Root Bridge 
Name(_SEG, 1) // Segment of this Root complex 
Name(_BBN, 64) // Base Bus Number 
Name(_CCA, 1)
Method (_CRS, 0, Serialized) { // Root complex resources 
Name (RBUF, ResourceTemplate () { 

[...]

)
}) // Name(RBUF)
Return (RBUF)
} // Method(_CRS)

Device (RES1)
{
Name (_HID, "HISI0081") // HiSi PCIe RC config base 
address
Name (_CID, "PNP0C02")  // Motherboard reserved resource
Name (_CRS, ResourceTemplate (){
Memory32Fixed (ReadWrite, 0xb008 , 0x1)
})
}

 [...]

} // Device(PCI1)



Therefore we declare a perfectly ECAM compliant MCFG and we "waste"
the root ports addresses as in practice for the root ports we replace
the MCFG address with the one retrieved from the ACPI device above.

In terms of "safety" I think it should be ok as in practice we are
reserving some addresses in MCFG that we do not use and the ones
that are used are reserved as well in the ACPI namespace.

>From a general p

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Lorenzo Pieralisi]

On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki 
> 
> pci_generic_ecam_ops is used by default. Since there are platforms
> which have non-compliant ECAM space we need to overwrite these
> accessors prior to PCI buses enumeration. In order to do that
> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> we can use proper PCI config space accessors and bus_shift.
> 
> pci_generic_ecam_ops is still used for platforms free from quirks.
> 
> Signed-off-by: Tomasz Nowicki 
> ---
>  arch/arm64/kernel/pci.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 94cd43c..a891bda 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>   struct pci_config_window *cfg;
>   struct resource cfgres;
>   unsigned int bsz;
> + struct pci_ecam_ops *ops;
>  
>   /* Use address from _CBA if present, otherwise lookup MCFG */
>   if (!root->mcfg_addr)
> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>   return NULL;
>   }
>  
> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
> + ops = pci_mcfg_get_ops(root);
> + bsz = 1 << ops->bus_shift;
>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>   cfgres.flags = IORESOURCE_MEM;
> - cfg = pci_ecam_create(>device->dev, , bus_res,
> -   _generic_ecam_ops);
> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);

Arnd pointed this out already, I think that's the only pending question
here.

pci_ecam_create() maps ECAM space for config regions retrieved from
the MCFG, which are *supposed* to be ECAM compliant.

Do we think that's *always* correct/safe regardless of the kind
of quirk we are currently fixing up ?

Or we do think that configuration space regions should come from
a different resource declared in the ACPI namespace if the regions
are not MCFG/ECAM compliant (ie config space is not defined through
MCFG at all - possibly through a _CRS method for a vendor specific
_HID under the PNP0A03 node ?)

It might even be a choice we do not have anymore, but I think it
is important to make a decision and proceed accordingly.

Comments appreciated.

Thanks,
Lorenzo

>   if (IS_ERR(cfg)) {
>   dev_err(>device->dev, "%04x:%pR error %ld mapping ECAM\n",
>   seg, bus_res, PTR_ERR(cfg));
> -- 
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Lorenzo Pieralisi]

On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki 
> 
> pci_generic_ecam_ops is used by default. Since there are platforms
> which have non-compliant ECAM space we need to overwrite these
> accessors prior to PCI buses enumeration. In order to do that
> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> we can use proper PCI config space accessors and bus_shift.
> 
> pci_generic_ecam_ops is still used for platforms free from quirks.
> 
> Signed-off-by: Tomasz Nowicki 
> ---
>  arch/arm64/kernel/pci.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 94cd43c..a891bda 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>   struct pci_config_window *cfg;
>   struct resource cfgres;
>   unsigned int bsz;
> + struct pci_ecam_ops *ops;
>  
>   /* Use address from _CBA if present, otherwise lookup MCFG */
>   if (!root->mcfg_addr)
> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>   return NULL;
>   }
>  
> - bsz = 1 << pci_generic_ecam_ops.bus_shift;
> + ops = pci_mcfg_get_ops(root);
> + bsz = 1 << ops->bus_shift;
>   cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>   cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>   cfgres.flags = IORESOURCE_MEM;
> - cfg = pci_ecam_create(>device->dev, , bus_res,
> -   _generic_ecam_ops);
> + cfg = pci_ecam_create(>device->dev, , bus_res, ops);

Arnd pointed this out already, I think that's the only pending question
here.

pci_ecam_create() maps ECAM space for config regions retrieved from
the MCFG, which are *supposed* to be ECAM compliant.

Do we think that's *always* correct/safe regardless of the kind
of quirk we are currently fixing up ?

Or we do think that configuration space regions should come from
a different resource declared in the ACPI namespace if the regions
are not MCFG/ECAM compliant (ie config space is not defined through
MCFG at all - possibly through a _CRS method for a vendor specific
_HID under the PNP0A03 node ?)

It might even be a choice we do not have anymore, but I think it
is important to make a decision and proceed accordingly.

Comments appreciated.

Thanks,
Lorenzo

>   if (IS_ERR(cfg)) {
>   dev_err(>device->dev, "%04x:%pR error %ld mapping ECAM\n",
>   seg, bus_res, PTR_ERR(cfg));
> -- 
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>