Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752600AbcKCFML (ORCPT ); Thu, 3 Nov 2016 01:12:11 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:50441 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750846AbcKCFMJ (ORCPT ); Thu, 3 Nov 2016 01:12:09 -0400 Subject: Re: [PATCH V3 2/2] PCI/ACPI: hisi: Add ACPI support for HiSilicon SoCs Host Controllers To: Bjorn Helgaas References: <1476933034-46044-1-git-send-email-liudongdong3@huawei.com> <1476933034-46044-3-git-send-email-liudongdong3@huawei.com> <20161102234053.GC10528@bhelgaas-glaptop.roam.corp.google.com> CC: , , , , , , , , , , , , , From: Dongdong Liu Message-ID: <5380c7fd-8351-1ce3-e1fb-54c649adf7ce@huawei.com> Date: Thu, 3 Nov 2016 13:05:11 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161102234053.GC10528@bhelgaas-glaptop.roam.corp.google.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.63.141.93] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12023 Lines: 366 Hi Bjorn Thanks for your review. 在 2016/11/3 7:40, Bjorn Helgaas 写道: > On Thu, Oct 20, 2016 at 11:10:34AM +0800, Dongdong Liu wrote: >> PCIe controller in Hip05/HIP06/HIP07 SoCs is not ECAM compliant. >> It is non ECAM only for the RC bus config space;for any other bus >> underneath the root bus we support ECAM access. >> Add specific quirks for PCI config space accessors.This involves: >> 1. New initialization call hisi_pcie_init() to obtain rc base >> addresses from PNP0C02 as subdevice of PNP0A03. >> 2. New entry in common quirk array. >> >> Signed-off-by: Dongdong Liu >> Signed-off-by: Gabriele Paoloni >> --- >> MAINTAINERS | 1 + >> drivers/acpi/pci_mcfg.c | 15 ++++ >> drivers/pci/host/Kconfig | 8 ++ >> drivers/pci/host/Makefile | 1 + >> drivers/pci/host/pcie-hisi-acpi.c | 170 ++++++++++++++++++++++++++++++++++++++ >> include/linux/pci-ecam.h | 4 + >> 6 files changed, 199 insertions(+) >> create mode 100755 drivers/pci/host/pcie-hisi-acpi.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 1cd38a7..b224caa 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -9358,6 +9358,7 @@ L: linux-pci@vger.kernel.org >> S: Maintained >> F: Documentation/devicetree/bindings/pci/hisilicon-pcie.txt >> F: drivers/pci/host/pcie-hisi.c >> +F: drivers/pci/host/pcie-hisi-acpi.c >> >> PCIE DRIVER FOR ROCKCHIP >> M: Shawn Lin >> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c >> index bb2c508..135a9b4 100644 >> --- a/drivers/acpi/pci_mcfg.c >> +++ b/drivers/acpi/pci_mcfg.c >> @@ -96,6 +96,21 @@ struct mcfg_fixup { >> THUNDER_ECAM_MCFG(2, 12), >> THUNDER_ECAM_MCFG(2, 13), >> #endif >> +#define PCI_ACPI_QUIRK_QUAD_DOM(table_id, seg, ops) \ >> + { "HISI ", table_id, 0, seg + 0, MCFG_BUS_ANY, ops }, \ >> + { "HISI ", table_id, 0, seg + 1, MCFG_BUS_ANY, ops }, \ >> + { "HISI ", table_id, 0, seg + 2, MCFG_BUS_ANY, ops }, \ >> + { "HISI ", table_id, 0, seg + 3, MCFG_BUS_ANY, ops } >> + >> +#ifdef CONFIG_PCI_HISI_ACPI >> + PCI_ACPI_QUIRK_QUAD_DOM("HIP05 ", 0, &hisi_pcie_ops), >> + PCI_ACPI_QUIRK_QUAD_DOM("HIP06 ", 0, &hisi_pcie_ops), >> + PCI_ACPI_QUIRK_QUAD_DOM("HIP07 ", 0, &hisi_pcie_ops), >> + PCI_ACPI_QUIRK_QUAD_DOM("HIP07 ", 4, &hisi_pcie_ops), >> + PCI_ACPI_QUIRK_QUAD_DOM("HIP07 ", 8, &hisi_pcie_ops), >> + PCI_ACPI_QUIRK_QUAD_DOM("HIP07 ", 12, &hisi_pcie_ops), >> +#endif >> + >> }; >> >> static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; >> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig >> index ae98644..9ff2bcd 100644 >> --- a/drivers/pci/host/Kconfig >> +++ b/drivers/pci/host/Kconfig >> @@ -227,6 +227,14 @@ config PCI_HISI >> Say Y here if you want PCIe controller support on HiSilicon >> Hip05 and Hip06 and Hip07 SoCs >> >> +config PCI_HISI_ACPI >> + depends on ACPI && ARM64 >> + bool "HiSilicon Hip05 and Hip06 and Hip07 SoCs ACPI PCIe controllers" >> + select PNP >> + help >> + Say Y here if you want ACPI PCIe controller support on HiSilicon >> + Hip05 and Hip06 and Hip07 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 084cb49..9402858 100644 >> --- a/drivers/pci/host/Makefile >> +++ b/drivers/pci/host/Makefile >> @@ -26,6 +26,7 @@ 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 >> +obj-$(CONFIG_PCI_HISI_ACPI) += pcie-hisi-acpi.o >> obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o >> obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o >> obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o >> diff --git a/drivers/pci/host/pcie-hisi-acpi.c b/drivers/pci/host/pcie-hisi-acpi.c >> new file mode 100755 >> index 0000000..5650d91 >> --- /dev/null >> +++ b/drivers/pci/host/pcie-hisi-acpi.c >> @@ -0,0 +1,170 @@ >> +/* >> + * PCIe host controller driver for HiSilicon HipXX SoCs >> + * >> + * Copyright (C) 2016 HiSilicon Co., Ltd. http://www.hisilicon.com >> + * >> + * Author: Dongdong Liu >> + * Gabriele Paoloni >> + * >> + * 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 >> +#include >> +#include >> + >> +#define DEBUG0 0x728 >> +#define PCIE_LTSSM_LINKUP_STATE 0x11 >> +#define PCIE_LTSSM_STATE_MASK 0x3F >> + >> +static const struct acpi_device_id hisi_pcie_rc_res_ids[] = { >> + {"HISI0081", 0}, >> + {"", 0}, >> +}; >> + >> +static int hisi_pcie_link_up_acpi(struct pci_config_window *cfg) >> +{ >> + u32 val; >> + void __iomem *reg_base = cfg->priv; >> + >> + val = readl(reg_base + DEBUG0); >> + return ((val & PCIE_LTSSM_STATE_MASK) == PCIE_LTSSM_LINKUP_STATE); >> +} > > Maybe reorder this so hisi_pcie_link_up_acpi() is next to the caller, > without the config accessors in between? Yes, will fix it. > >> +static int hisi_pcie_acpi_rd_conf(struct pci_bus *bus, u32 devfn, int where, >> + int size, u32 *val) >> +{ >> + struct pci_config_window *cfg = bus->sysdata; >> + int dev = PCI_SLOT(devfn); >> + >> + if (bus->number == cfg->busr.start) { >> + /* access only one slot on each root port */ >> + if (dev > 0) >> + return PCIBIOS_DEVICE_NOT_FOUND; >> + else >> + return pci_generic_config_read32(bus, devfn, 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 pci_config_window *cfg = bus->sysdata; >> + int dev = PCI_SLOT(devfn); >> + >> + if (bus->number == cfg->busr.start) { >> + /* access only one slot on each root port */ >> + if (dev > 0) >> + return PCIBIOS_DEVICE_NOT_FOUND; >> + else >> + return pci_generic_config_write32(bus, devfn, where, >> + size, val); >> + } >> + >> + return pci_generic_config_write(bus, devfn, where, size, val); >> +} > > Seems like we could *almost* get rid of these and use only the generic > 32-bit accessors: > > struct pci_ecam_ops hisi_pcie_ops = { > .bus_shift = 20, > .init = hisi_pcie_init, > .pci_ops = { > .map_bus = hisi_pcie_map_bus, > .read = pci_generic_config_read32, > .write = pci_generic_config_write32, > } > } > > We'd be using the 32-bit accessors on the root bus when it's not > strictly necessary. And we'd give up the "dev > 0" check. Not sure > how important those are, though. If "dev > 0" accesses just fail > naturally, there'd be no need to do the check. Our host controller only has one root port. If we'd give up the "dev > 0" check, will enumerate 32 root ports, so we need to do the check. root@(none)$ lspci 10:00.0 Class 0604: 19e5:1610 10:01.0 Class 0604: 19e5:1610 10:02.0 Class 0604: 19e5:1610 ..... > >> +static void __iomem *hisi_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, >> + int where) >> +{ >> + struct pci_config_window *cfg = bus->sysdata; >> + void __iomem *reg_base = cfg->priv; >> + >> + if (bus->number == cfg->busr.start) >> + return reg_base + where; >> + else >> + return pci_ecam_map_bus(bus, devfn, where); >> +} >> + >> +/* >> + * Retrieve RC base and size from sub-device under the RC >> + * Device (RES1) >> + * { >> + * Name (_HID, "HISI0081") >> + * Name (_CID, "PNP0C02") >> + * Name (_CRS, ResourceTemplate (){ >> + * Memory32Fixed (ReadWrite, 0xb0080000, 0x10000) >> + * }) >> + * } >> + */ >> +static int hisi_pcie_rc_addr_get(struct acpi_device *adev, >> + void __iomem **addr) >> +{ >> + struct acpi_device *child_adev; >> + struct list_head list; >> + struct resource *res; >> + struct resource_entry *entry; >> + unsigned long flags; >> + int ret; >> + >> + list_for_each_entry(child_adev, &adev->children, node) { >> + ret = acpi_match_device_ids(child_adev, hisi_pcie_rc_res_ids); >> + if (ret) >> + continue; >> + >> + INIT_LIST_HEAD(&list); >> + flags = IORESOURCE_MEM; >> + ret = acpi_dev_get_resources(child_adev, &list, >> + acpi_dev_filter_resource_type_cb, >> + (void *)flags); >> + if (ret < 0) { >> + dev_err(&child_adev->dev, >> + "failed to parse _CRS method, error code %d\n", >> + ret); >> + return ret; >> + } else if (ret == 0) { >> + dev_err(&child_adev->dev, >> + "no IO and memory resources present in _CRS\n"); >> + return -EINVAL; >> + } >> + >> + entry = list_first_entry(&list, struct resource_entry, node); >> + res = entry->res; >> + *addr = devm_ioremap(&child_adev->dev, >> + res->start, resource_size(res)); >> + acpi_dev_free_resource_list(&list); >> + if (IS_ERR(*addr)) { >> + dev_err(&child_adev->dev, "error with ioremap\n"); >> + return -ENOMEM; >> + } >> + >> + return 0; >> + } >> + >> + return -EINVAL; >> +} >> + >> +static int hisi_pcie_init(struct pci_config_window *cfg) >> +{ >> + int ret; >> + struct acpi_device *adev = to_acpi_device(cfg->parent); >> + void __iomem *reg_base; >> + >> + ret = hisi_pcie_rc_addr_get(adev, ®_base); >> + if (ret) { >> + dev_err(&adev->dev, "can't get rc base address"); >> + return ret; >> + } >> + >> + cfg->priv = reg_base; >> + if (!hisi_pcie_link_up_acpi(cfg)) { >> + dev_err(&adev->dev, "link status is down\n"); >> + return -EINVAL; >> + } > > How necessary is this link_up check?The link can go down > asynchronously anyway, at any time after we do this check, so why > bother doing it here? Yes, this is not relly necessary, If we do link_up check,and the link is down(e.g not insert a card), we will return early and not continue to enumerate devices. > > If we don't need to do it, we don't really need to know the register > addresses here, do we? Maybe we can just fall back to the standard > x86 solution of a PNP0C02 device at the ACPI root that has _CRS to > cover the host bridge registers? Yes , we don't really need to know link status register, but we still need to know the RC base address. this reg_base is not only used to get the link status but also to access RC config space. see hisi_pcie_map_bus() static void __iomem *hisi_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, int where) { struct pci_config_window *cfg = bus->sysdata; void __iomem *reg_base = cfg->priv; ^^^^^^^ if (bus->number == cfg->busr.start) return reg_base + where; ^^^^^^^ else return pci_ecam_map_bus(bus, devfn, where); } Thanks Dongdong > > It'd be nice if we didn't have to grub around in the ACPI devices > looking for a corresponding device. That's sort of a new solution to > a problem that will go away soon anyway (assuming future hardware is > more spec-conforming as far as ECAM). > >> + return 0; >> +} >> + >> +struct pci_ecam_ops hisi_pcie_ops = { >> + .bus_shift = 20, >> + .init = hisi_pcie_init, >> + .pci_ops = { >> + .map_bus = hisi_pcie_map_bus, >> + .read = hisi_pcie_acpi_rd_conf, >> + .write = hisi_pcie_acpi_wr_conf, >> + } >> +}; >> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h >> index 35f0e81..2db4db8 100644 >> --- a/include/linux/pci-ecam.h >> +++ b/include/linux/pci-ecam.h >> @@ -66,6 +66,10 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn, >> extern struct pci_ecam_ops pci_thunder_ecam_ops; >> #endif >> >> +#ifdef CONFIG_PCI_HISI_ACPI >> +extern struct pci_ecam_ops hisi_pcie_ops; >> +#endif >> + >> #ifdef CONFIG_PCI_HOST_GENERIC >> /* for DT-based PCI controllers that support ECAM */ >> int pci_host_common_probe(struct platform_device *pdev, >> -- >> 1.9.1 >> > > . >