| Message ID | 1455039260-6040-4-git-send-email-gabriele.paoloni@huawei.com |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- > owner@vger.kernel.org] On Behalf Of Mark Rutland > Sent: 10 February 2016 11:13 > To: Gabriele Paoloni > Cc: Guohanjun (Hanjun Guo); Wangzhou (B); liudongdong (C); Linuxarm; > qiujiang; bhelgaas@google.com; arnd@arndb.de; > Lorenzo.Pieralisi@arm.com; tn@semihalf.com; linux-pci@vger.kernel.org; > linux-kernel@vger.kernel.org; xuwei (O); linux-acpi@vger.kernel.org; > jcm@redhat.com; zhangjukuo; Liguozhu (Kenneth); linux-arm- > kernel@lists.infradead.org > Subject: Re: [RFC PATCH v3 3/3] PCI/ACPI: hisi: Add ACPI support for > HiSilicon SoCs Host Controllers > > On Wed, Feb 10, 2016 at 09:52:36AM +0000, Gabriele Paoloni wrote: > > Hi Mark > > > > > On Tue, Feb 09, 2016 at 05:34:20PM +0000, Gabriele Paoloni wrote: > > > > From: gabriele paoloni <gabriele.paoloni@huawei.com> > > > > +/* > > > > + * Retrieve rc_dbi base and size from _DSD > > > > + * Name (_DSD, Package () { > > > > + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > > > + * Package () { > > > > + * Package () {"rc-dbi", Package () { 0x0, 0xb0080000, 0x0, > 0x10000 > > > }}, > > > > + * } > > > > + * }) > > > > + */ > > > > > > As above, this does not look right. ACPI has standard mechanisms > for > > > describing addresses. Making something up like this is not a good > idea. > > > > I am quite new to ACPI, may I ask you to explain a bit? > > ACPI has standard mechanisms for describing certain resources, and > these > should not be described in _DSD. Memory or IO address regions are such > resources (in _CRS, IIRC), and should not be described in _DSD. Hi Mark, In my case I think in need to look into the MCFG object as the problem I have is RC using a different range than the rest of the hierarchy. I'll investigate this and try to come with a solution in v4 Many Thanks Gab > > Thanks, > Mark.
Hi Bjorn, Lorenzo Many Thanks for your replies and suggestions > -----Original Message----- > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > owner@vger.kernel.org] On Behalf Of Bjorn Helgaas > Sent: 25 February 2016 19:59 > To: Lorenzo Pieralisi > Cc: Gabriele Paoloni; 'Mark Rutland'; Guohanjun (Hanjun Guo); Wangzhou > (B); liudongdong (C); Linuxarm; qiujiang; 'bhelgaas@google.com'; > 'arnd@arndb.de'; 'tn@semihalf.com'; 'linux-pci@vger.kernel.org'; > 'linux-kernel@vger.kernel.org'; xuwei (O); 'linux- > acpi@vger.kernel.org'; 'jcm@redhat.com'; zhangjukuo; Liguozhu > (Kenneth); 'linux-arm-kernel@lists.infradead.org' > Subject: Re: [RFC PATCH v3 3/3] PCI/ACPI: hisi: Add ACPI support for > HiSilicon SoCs Host Controllers > > On Thu, Feb 25, 2016 at 12:07:50PM +0000, Lorenzo Pieralisi wrote: > > On Thu, Feb 25, 2016 at 03:01:19AM +0000, Gabriele Paoloni wrote: > > > > [...] > > > > > > I think the relevant spec is the PCI Firmware Spec, r3.0, sec > 4.1.2. > > > > Note 2 in that section says the address range of an MMCFG region > > > > must be reserved by declaring a motherboard resource, i.e., > included > > > > in the _CRS of a PNP0C02 or similar device. > > > > > > I had a look a this. So yes the specs says that we should use the > > > PNP0C02 device if MCFG is not supported. > > > > AFAIK, PNP0C02 is a resource reservation mechanism, the spec says > that > > MCFG regions must be reserved using PNP0C02, even though its > > current usage on x86 is a bit unfathomable to me, in particular > > in relation to MCFG resources retrieved for hotpluggable bridges (ie > > through _CBA, which I think consider the MCFG region as reserved > > by default, regardless of PNP0c02): > > > > see pci_mmcfg_check_reserved() arch/x86/pci/mmconfig-shared.c Yes I checked this and it seems to check if an area of memory from MCFG is overlapping with any area of memory specified by PNP0C02 _CRS... However (maybe I am wrong) it looks to me that this part works independently of the PNP0c02 driver. It seems that goes directly to walk the ACPI namespace and look for PNP0C02 HID; as it finds it, it checks the range of memory specified in the _CRS method and see if it overlaps with the MCFG resource...am I missing something? If my interpretation is correct, couldn't we just modify pci_mmconfig_map_resource() in the latest Nowicki patchset and add a similar check before insert_resource_conflict() is called? On the other side HiSilicon host bridge quirks could use the address retrieved by the _CRS method of PNP0C02 for our root complex config rd/wr...? > > I don't know how _CBA-related resources would be reserved. I haven't > personally worked with any host bridges that supply _CBA, so I don't > know whether or how they handled it. > > I think the spec intent was that the Consumer/Producer bit (Extended > Address Space Descriptor, General Flags Bit[0], see ACPI spec sec > 6.4.3.5.4) would be used. Resources such as ECAM areas would be > marked "Consumer", meaning the bridge consumed that space itself, and > windows passed down to the PCI bus would be marked "Producer". > > But BIOSes didn't use that bit consistently, so we couldn't rely on > it. I verified experimentally that Windows didn't pay attention to > that bit either, at least for DWord descriptors: > https://bugzilla.kernel.org/show_bug.cgi?id=15701 > > It's conceivable that we could still use that bit in Extended Address > Space descriptors, or maybe some hack like pay attention if the bridge > has _CBA, or some such. Or maybe a BIOS could add a PNP0C02 device > along with the PNP0A03 device that uses _CBA, with the PNP0C02 _CRS > describing the ECAM area referenced by _CBA. Seeems hacky no matter > how we slice it. Well about this I don't know much but, having looked at the bugzilla and considering the current mechanism used by pci_mmcfg_check_reserved() I have the feeling that this last one is easier to implement and it seems the one currently used (in mmconfig-shared.c ) Cheers Gab > > > Have a look at drivers/pnp/system.c for PNP0c02 > > > > > So probably I can use acpi_get_devices("PNP0C02",...) to retrieve > it > > > from the quirk match function, I will look into this... > > > > > > > > > > > > On the other side, since this is an exception only for the > config > > > > > space address of our host controller (as said before all the > buses > > > > > below the root one support ECAM), I think that it is right to > put > > > > > this address as a device specific data (in fact the rest of the > > > > > config space addresses will be parsed from MCFG). > > > > > > > > A kernel with no support for your host controller (and thus no > > > > knowledge of its _DSD) should still be able to operate the rest > of the > > > > system correctly. That means we must have a generic way to learn > what > > > > address space is consumed by the host controller so we don't try > to > > > > assign it to other devices. > > > > > > This is something I don't understand much... > > > Are you talking about a scenario where we have a Kernel image > compiled > > > without our host controller support and running on our platform? > > > > I *think* the point here is that your host controller config space > should be > > reserved through PNP0c02 so that the kernel will reserve it through > the > > generic PNP0c02 driver even if your host controller driver (and > related > > _DSD) is not supported in the kernel. > > Right. Assume you have two top-level devices: > > PNP0A03 PCI host bridge > _CRS describes windows > ???? describes ECAM space consumed > PNPxxxx another ACPI device, currently disabled > _PRS specifies possible resource settings, may specify no > restrictions > _SRS assign resources and enable device > _CRS empty until device enabled > > When the OS enables PNPxxxx, it must first assign resources to it > using _PRS and _SRS. We evaluate _PRS to find out what the addresses > PNPxxxx can support. This tells us things like how wide the address > decoder is, the size of the region required, and any alignment > restrictions -- basically the same information we get by sizing a PCI > BAR. > > Now, how do we assign space for PNPxxxx? In a few cases, _PRS has > only a few specific possibilities, e.g., an x86 legacy serial port > that can be at 0x3f8 or 0x2f8. But in general, _PRS need not impose > any restrictions. > > So in general the OS can use any space that can be routed to PNPxxxx. > If there's an upstream bridge, it may define windows that restrict the > possibilities. But in this case, there *is* no upstream bridge, so > the possible choices are the entire physical address space of the > platform, except for other things that are already allocated: RAM, the > _CRS settings for other ACPI devices, things reserved by the E820 > table (at least on x86), etc. > > If PNP0A03 consumes address space for ECAM, that space must be > reported *somewhere* so the OS knows not to place PNPxxxx there. This > reporting must be generic (not device-specific like _DSD). The ACPI > core (not drivers) is responsible for managing this address space > because: > > a) the OS is not guaranteed to have drivers for all devices, and > > b) even it *did* have drivers for all devices, the PNPxxxx space may > be assigned before drivers are initialized. > > > I do not understand how PNP0c02 works, currently, by the way. > > > > If I read x86 code correctly, the unassigned PCI bus resources are > > assigned in arch/x86/pci/i386.c (?) > fs_initcall(pcibios_assign_resources), > > with a comment: > > > > /** > > * called in fs_initcall (one below subsys_initcall), > > * give a chance for motherboard reserve resources > > */ > > > > Problem is, motherboard resources are requested through (?): > > > > drivers/pnp/system.c > > > > which is also initialized at fs_initcall, so it might be called after > > core x86 code reassign resources, defeating the purpose PNP0c02 was > > designed for, namely, request motherboard regions before resources > > are assigned, am I wrong ? > > I think you're right. This is a long-standing screwup in Linux. > IMHO, ACPI resources should be parsed and reserved by the ACPI core, > before any PCI resource management (since PCI host bridges are > represented in ACPI). But historically PCI devices have enumerated > before ACPI got involved. And the ACPI core doesn't really pay > attention to _CRS for most devices (with the exception of PNP0C02). > > IMO the PNP0C02 code in drivers/pnp/system.c should really be done in > the ACPI core for all ACPI devices, similar to the way the PCI core > reserves BAR space for all PCI devices, even if we don't have drivers > for them. I've tried to fix this in the past, but it is really a > nightmare to unravel everything. > > Because the ACPI core doesn't reserve resources for the _CRS of all > ACPI devices, we're already vulnerable to the problem of placing a > device on top of another ACPI device. We don't see problems because > on x86, at least, most ACPI devices are already configured by the BIOS > to be enabled and non-overlapping. But x86 has the advantage of > having extensive test coverage courtesy of Windows, and as long as > _CRS has the right stuff in it, we at least have the potential of > fixing problems in Linux. > > If the platform doesn't report resource usage correctly on ARM, we may > not find problems (because we don't have the Windows test suite) and > if we have resource assignment problems because _CRS is lacking, we'll > have no way to fix them. > > > As per last Tomasz's patchset, we claim and assign unassigned PCI > > resources upon ACPI PCI host bridge probing (which happens at > > subsys_initcall time, courtesy of ACPI current code); at that time > the > > kernel did not even register the PNP0c02 driver > (drivers/pnp/system.c) > > (it does that at fs_initcall). On the other hand, we insert MCFG > > regions into the resource tree upon MCFG parsing, so I do not > > see why we need to rely on PNP0c02 to do that for us (granted, the > > mechanism is part of the PCI fw specs, which are x86 centric anyway > > ie we can't certainly rely on Int15 e820 to detect reserved memory > > on ARM :D) > > > > There is lots of legacy x86 here and Bjorn definitely has more > > visibility into that than I have, the ARM world must understand > > how this works to make sure we have an agreement. > > As you say, there is lots of unpleasant x86 legacy here. Possibly ARM > has a chance to clean this up and do it more sanely; I'm not sure > whether it's feasible to reverse the ACPI/PCI init order there or not. > > Rafael, any thoughts on this whole thing? > > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/MAINTAINERS b/MAINTAINERS index d69f436..f184c3e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8412,6 +8412,7 @@ F: Documentation/devicetree/bindings/pci/hisilicon-pcie.txt F: drivers/pci/host/pcie-hisi.h F: drivers/pci/host/pcie-hisi.c F: drivers/pci/host/pcie-hisi-common.c +F: drivers/pci/host/pcie-hisi-acpi.c PCIE DRIVER FOR QUALCOMM MSM M: Stanimir Varbanov <svarbanov@mm-sol.com> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig index 75a6054..65b1add 100644 --- a/drivers/pci/host/Kconfig +++ b/drivers/pci/host/Kconfig @@ -181,6 +181,14 @@ config PCI_HISI Say Y here if you want PCIe controller support on HiSilicon Hip05 and Hip06 SoCs +config PCI_HISI_ACPI + depends on ACPI + bool "HiSilicon Hip05 and Hip06 SoCs ACPI PCIe controllers" + select ACPI_PCI_HOST_GENERIC + help + Say Y here if you want ACPI PCIe controller support on HiSilicon + Hip05 and Hip06 SoCs + config PCIE_QCOM bool "Qualcomm PCIe controller" depends on ARCH_QCOM && OF diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile index 8c93c0f..57e4379 100644 --- a/drivers/pci/host/Makefile +++ b/drivers/pci/host/Makefile @@ -21,4 +21,5 @@ obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o obj-$(CONFIG_PCIE_ALTERA_MSI) += pcie-altera-msi.o obj-$(CONFIG_PCI_HISI) += pcie-hisi.o pcie-hisi-common.o +obj-$(CONFIG_PCI_HISI_ACPI) += pcie-hisi-acpi.o pcie-hisi-common.o obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o diff --git a/drivers/pci/host/pcie-hisi-acpi.c b/drivers/pci/host/pcie-hisi-acpi.c new file mode 100644 index 0000000..3605260 --- /dev/null +++ b/drivers/pci/host/pcie-hisi-acpi.c @@ -0,0 +1,188 @@ +/* + * PCIe host controller driver for HiSilicon HipXX SoCs + * + * Copyright (C) 2016 HiSilicon Co., Ltd. http://www.hisilicon.com + * + * Author: Dongdong Liu <liudongdong3@huawei.com> + * Gabriele Paoloni <gabriele.paoloni@huawei.com> + * + * 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. + */ +#include <linux/interrupt.h> +#include <linux/acpi.h> +#include <linux/ecam.h> +#include <linux/pci.h> +#include <linux/pci-acpi.h> +#include "pcie-hisi.h" + +#define GET_PCIE_LINK_STATUS 0x0 + +/* uuid 6d30f553-836c-408e-b6ad-45bccc957949 */ +const u8 hisi_pcie_acpi_dsm_uuid[] = { + 0x53, 0xf5, 0x30, 0x6d, 0x6c, 0x83, 0x8e, 0x40, + 0xb6, 0xad, 0x45, 0xbc, 0xcc, 0x95, 0x79, 0x49 +}; + +static const struct acpi_device_id hisi_pcie_ids[] = { + {"HISI0080", 0}, + {"", 0}, +}; + +static int hisi_pcie_get_addr(struct acpi_pci_root *root, const char *name, + void __iomem **addr) +{ + struct acpi_device *device; + u64 base; + u64 size; + u32 buf[4]; + int ret; + + device = root->device; + ret = fwnode_property_read_u32_array(&device->fwnode, name, + buf, ARRAY_SIZE(buf)); + if (ret) { + dev_err(&device->dev, "can't get %s\n", name); + return ret; + } + + base = ((u64)buf[0] << 32) | buf[1]; + size = ((u64)buf[2] << 32) | buf[3]; + *addr = devm_ioremap(&device->dev, base, size); + if (!(*addr)) { + dev_err(&device->dev, "error with ioremap\n"); + return -ENOMEM; + } + + return 0; +} + + +static int hisi_pcie_link_up_acpi(struct acpi_pci_root *root) +{ + u32 val; + struct acpi_device *device; + union acpi_object *obj; + + device = root->device; + obj = acpi_evaluate_dsm(device->handle, + hisi_pcie_acpi_dsm_uuid, 0, + GET_PCIE_LINK_STATUS, NULL); + + if (!obj || obj->type != ACPI_TYPE_INTEGER) { + dev_err(&device->dev, "can't get link status from _DSM\n"); + return 0; + } + val = obj->integer.value; + + return ((val & PCIE_LTSSM_STATE_MASK) == PCIE_LTSSM_LINKUP_STATE); + +} + +/* + * Retrieve rc_dbi base and size from _DSD + * Name (_DSD, Package () { + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + * Package () { + * Package () {"rc-dbi", Package () { 0x0, 0xb0080000, 0x0, 0x10000 }}, + * } + * }) + */ +static int hisi_pcie_init(struct acpi_pci_root *root) +{ + int ret; + struct acpi_device *device; + void __iomem *reg_base; + + device = root->device; + ret = hisi_pcie_get_addr(root, "rc-dbi", ®_base); + if (ret) { + dev_err(&device->dev, "can't get rc-dbi\n"); + return ret; + } + + root->sysdata = reg_base; + return 0; +} + +static int hisi_pcie_match(struct pci_mcfg_fixup *fixup, + struct acpi_pci_root *root) +{ + int ret; + struct acpi_device *device; + + device = root->device; + ret = acpi_match_device_ids(device, hisi_pcie_ids); + if (ret) + return 0; + + ret = hisi_pcie_init(root); + if (ret) + dev_warn(&device->dev, "hisi pcie init fail\n"); + + return 1; +} + +static int hisi_pcie_acpi_valid_config(struct acpi_pci_root *root, + struct pci_bus *bus, int dev) +{ + /* If there is no link, then there is no device */ + if (bus->number != root->secondary.start) { + if (!hisi_pcie_link_up_acpi(root)) + return 0; + } + + /* access only one slot on each root port */ + if (bus->number == root->secondary.start && dev > 0) + return 0; + + /* + * do not read more than one device on the bus directly attached + * to RC's (Virtual Bridge's) DS side. + */ + if (bus->primary == root->secondary.start && dev > 0) + return 0; + + return 1; +} + +static int hisi_pcie_acpi_rd_conf(struct pci_bus *bus, u32 devfn, int where, + int size, u32 *val) +{ + struct acpi_pci_root *root = bus->sysdata; + void __iomem *reg_base = root->sysdata; + + if (hisi_pcie_acpi_valid_config(root, bus, PCI_SLOT(devfn)) == 0) + return PCIBIOS_DEVICE_NOT_FOUND; + + if (bus->number == root->secondary.start) + return hisi_pcie_common_cfg_read(reg_base, where, size, val); + + return pci_generic_config_read(bus, devfn, where, size, val); +} + +static int hisi_pcie_acpi_wr_conf(struct pci_bus *bus, u32 devfn, + int where, int size, u32 val) +{ + struct acpi_pci_root *root = bus->sysdata; + void __iomem *reg_base = root->sysdata; + + if (hisi_pcie_acpi_valid_config(root, bus, PCI_SLOT(devfn)) == 0) + return PCIBIOS_DEVICE_NOT_FOUND; + + if (bus->number == root->secondary.start) + return hisi_pcie_common_cfg_write(reg_base, where, size, val); + + return pci_generic_config_write(bus, devfn, where, size, val); +} + +struct pci_ops hisi_pcie_acpi_ops = { + .map_bus = pci_mcfg_dev_base, + .read = hisi_pcie_acpi_rd_conf, + .write = hisi_pcie_acpi_wr_conf, +}; + + +DECLARE_ACPI_MCFG_FIXUP(NULL, hisi_pcie_match, &hisi_pcie_acpi_ops, + PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY); diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c index d3e2047..d1ef346 100644 --- a/drivers/pci/host/pcie-hisi.c +++ b/drivers/pci/host/pcie-hisi.c @@ -23,8 +23,6 @@ #include "pcie-designware.h" #include "pcie-hisi.h" -#define PCIE_LTSSM_LINKUP_STATE 0x11 -#define PCIE_LTSSM_STATE_MASK 0x3F #define PCIE_SUBCTRL_SYS_STATE4_REG 0x6818 #define PCIE_SYS_STATE4 0x31c #define PCIE_HIP06_CTRL_OFF 0x1000 diff --git a/drivers/pci/host/pcie-hisi.h b/drivers/pci/host/pcie-hisi.h index 29e0790..45cf0fd 100644 --- a/drivers/pci/host/pcie-hisi.h +++ b/drivers/pci/host/pcie-hisi.h @@ -14,6 +14,8 @@ #ifndef PCIE_HISI_H_ #define PCIE_HISI_H_ +#define PCIE_LTSSM_LINKUP_STATE 0x11 +#define PCIE_LTSSM_STATE_MASK 0x3F int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size, u32 *val);