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

Hi Lorenzo and Tomasz

Many Thanks for looking at this

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

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

[...]

quoted

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

RC config resource with hardcode as DEFINE_RES_MEM(0xb0070000,

SZ_4K),

quoted

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

hardcode with RC config resource.

quoted

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

quoted

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.

quoted

.....

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

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

Thanks
Dongdong

quoted

Thanks,
Tomasz

.