Hi Lorenzo and Tomasz Many Thanks for looking at this > -----Original Message----- > From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@xxxxxxx] > Sent: 15 September 2016 11:59 > To: liudongdong (C) > Cc: Tomasz Nowicki; helgaas@xxxxxxxxxx; will.deacon@xxxxxxx; > catalin.marinas@xxxxxxx; rafael@xxxxxxxxxx; arnd@xxxxxxxx; > hanjun.guo@xxxxxxxxxx; okaya@xxxxxxxxxxxxxx; jchandra@xxxxxxxxxxxx; > cov@xxxxxxxxxxxxxx; dhdang@xxxxxxx; ard.biesheuvel@xxxxxxxxxx; > robert.richter@xxxxxxxxxxxxxxxxxx; mw@xxxxxxxxxxxx; > Liviu.Dudau@xxxxxxx; ddaney@xxxxxxxxxxxxxxxxxx; Wangyijing; > msalter@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; linaro-acpi@xxxxxxxxxxxxxxxx; > jcm@xxxxxxxxxx; andrea.gallo@xxxxxxxxxx; jeremy.linton@xxxxxxx; > Gabriele Paoloni; jhugo@xxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [PATCH V6 2/5] PCI/ACPI: Check platform specific ECAM > quirks > > On Tue, Sep 13, 2016 at 07:38:39PM +0800, Dongdong Liu wrote: > > [...] > > > Our host bridge is non ECAM only for the RC bus config space; > > for any other bus underneath the root bus we support ECAM access. > > > > RC config resource with hardcode as DEFINE_RES_MEM(0xb0070000, > SZ_4K), > > EP config resource we get it from MCFG table. > > So we need to override ops, but config resource we only need to > hardcode with RC config resource. > > > > Our host controller ACPI support patch can be found: > > https://lkml.org/lkml/2016/8/31/340 > > Sorry I misread your code. IIUC you create an array of resources that > represent non-ECAM config space (and incidentally contain debug > registers to check the link status - that you need to check for every > given config access (?)), but you still need to have an MCFG entry that The link status check is inherited from the designware framework (see http://lxr.free-electrons.com/source/drivers/pci/host/pcie-designware.c#L678) However I think that in this case we can just check the link status in our init function and return an error if the link is down > covers the bus number subject to quirk to make sure this mechanism > works. Correct ? Well we need the quirks for the root bus numbers but if read this v6 quirk mechanism correctly even if we do not specify an mcfg entry for bus 0 oci_mcfg_match_quirks() is called anyway and we can set our special configuration space addresses for the root buses (i.e. I think we can have a clean MCFG table with entries only for those bus ranges that are really ECAM) > > This also means that, with the MCFG tables you have and current > mainline kernel you are able to probe a root bridge (because the MCFG > table covers the bus number that is not ECAM), with enumeration > going haywire because it is trying to carry out ECAM accesses on > non-ECAM space. Yes correct, we cannot access the host controller configuration space with our current MCFG table and current Linux mainline > > Is my reading correct ? > > Anyway, that's not stricly related to this discussion, it is time we > converge on this patchset, we can add a domain range if that > simplifies things. IMO it would be better to have the domain range to avoid a very large and repetitive static quirk array Thanks Gab > > Thanks, > Lorenzo > > > This patch is based on RFC V5 quirk mechanism. > > > > Based on V6 quirk mechanism, we have to change it as below: > > > > #ifdef CONFIG_PCI_HISI_ACPI > > { "HISI ", "HIP05 ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_hip05_ops, > > MCFG_RES_EMPTY}, > > { "HISI ", "HIP05 ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_hip05_ops, > > MCFG_RES_EMPTY}, > > { "HISI ", "HIP05 ", 0, 2, MCFG_BUS_ANY, &hisi_pcie_hip05_ops, > > MCFG_RES_EMPTY}, > > { "HISI ", "HIP05 ", 0, 3, MCFG_BUS_ANY, &hisi_pcie_hip05_ops, > > MCFG_RES_EMPTY}, > > { "HISI ", "HIP06 ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_hip06_ops, > > MCFG_RES_EMPTY}, > > { "HISI ", "HIP06 ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_hip06_ops, > > MCFG_RES_EMPTY}, > > { "HISI ", "HIP06 ", 0, 2, MCFG_BUS_ANY, &hisi_pcie_hip06_ops, > > MCFG_RES_EMPTY}, > > { "HISI ", "HIP06 ", 0, 3, MCFG_BUS_ANY, &hisi_pcie_hip06_ops, > > MCFG_RES_EMPTY}, > > { "HISI ", "HIP07 ", 0, 0, MCFG_BUS_ANY, &hisi_pcie_hip07_ops, > > MCFG_RES_EMPTY}, > > { "HISI ", "HIP07 ", 0, 1, MCFG_BUS_ANY, &hisi_pcie_hip07_ops, > > MCFG_RES_EMPTY}, > > .... > > > > { "HISI ", "HIP07 ", 0, 15, MCFG_BUS_ANY, &hisi_pcie_hip07_ops, > > MCFG_RES_EMPTY}, > > > > #endif > > > > struct pci_ecam_ops hisi_pci_hip05_ops = { > > .bus_shift = 20, > > .init = hisi_pci_hip05_init, > > .pci_ops = { > > .map_bus = pci_ecam_map_bus, > > .read = hisi_pcie_acpi_rd_conf, > > .write = hisi_pcie_acpi_wr_conf, > > } > > }; > > > > struct pci_ecam_ops hisi_pci_hip06_ops = { > > .bus_shift = 20, > > .init = hisi_pci_hip06_init, > > .pci_ops = { > > .map_bus = pci_ecam_map_bus, > > .read = hisi_pcie_acpi_rd_conf, > > .write = hisi_pcie_acpi_wr_conf, > > } > > }; > > > > hisi_pci_hipxx_init function is used to get RC config resource with > hardcode. > > ..... > > > > So I hope we can use MCFG_DOM_RANGE, Then I can change it as below. > > > > #ifdef CONFIG_PCI_HISI_ACPI > > { "HISI ", "HIP05 ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY, > > &hisi_pcie_hip05_ops, MCFG_RES_EMPTY}, > > { "HISI ", "HIP06 ", 0, MCFG_DOM_RANGE(0, 3), MCFG_BUS_ANY, > > &hisi_pcie_hip06_ops, MCFG_RES_EMPTY}, > > { "HISI ", "HIP07 ", 0, MCFG_DOM_RANGE(0, 15), MCFG_BUS_ANY, > > &hisi_pcie_hip07_ops, MCFG_RES_EMPTY}, > > #endif > > > > Thanks > > Dongdong > > > > > >Thanks, > > >Tomasz > > > > > >. > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
