On Mon, Jun 20, 2016 at 12:12:24PM -0700, Duc Dang wrote: > On Mon, Jun 20, 2016 at 10:17 AM, Christopher Covington > <c...@codeaurora.org> 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 > >>> <lorenzo.pieral...@arm.com> wrote: > >>>> 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(&root->device->dev, &cfgres, bus_res, > >>>>> - &pci_generic_ecam_ops); > >>>>> + cfg = pci_ecam_create(&root->device->dev, &cfgres, 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 >