From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933661AbcIAOC7 convert rfc822-to-8bit (ORCPT ); Thu, 1 Sep 2016 10:02:59 -0400 Received: from mout.kundenserver.de ([212.227.126.130]:62278 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932705AbcIAOCz (ORCPT ); Thu, 1 Sep 2016 10:02:55 -0400 From: Arnd Bergmann To: Dongdong Liu Cc: helgaas@kernel.org, rafael@kernel.org, Lorenzo.Pieralisi@arm.com, tn@semihalf.com, wangzhou1@hisilicon.com, pratyush.anand@gmail.com, linux-pci@vger.kernel.org, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, jcm@redhat.com, gabriele.paoloni@huawei.com, charles.chenxin@huawei.com, hanjun.guo@linaro.org, linuxarm@huawei.com Subject: Re: [RFC PATCH V2 1/3] PCI: hisi: re-architect Hip05/Hip06 controllers driver to preapare for ACPI Date: Thu, 01 Sep 2016 16:02:19 +0200 Message-ID: <6532722.cVEMfv9Kqj@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <57C822C1.9000203@huawei.com> References: <1472644094-82731-1-git-send-email-liudongdong3@huawei.com> <5913196.I3zIbc2qla@wuerfel> <57C822C1.9000203@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT Content-Type: text/plain; charset="UTF-8" X-Provags-ID: V03:K0:vsn6Gwdt1hsye+tiN/mBwKHPHVBd+rWxWiOtPuwJzOQI6kWR7gT wfWdVZp5Hg922zG8qcSjdpffUn31mwv46j+QG3XdESobQmwqV9I1kj7eMxiUfdn8wnQhmE2 Q4pm2HPlon4AB2VClV+vojJ/w0n5MIh6HSQaOisuKJBz3RBRQRRB2yNLPZrhdEYprsDx4va 4s2T2jCEmEECPunaISpGg== X-UI-Out-Filterresults: notjunk:1;V01:K0:JbUTIuVO1IQ=:eMXcVvF1FvfNWHQ/4yY4Kr rRvFxN8FI78/ZUAzBQ9Ar1ZpSTp5ZYcZyP/aHx0B0aLgXRbJsjcIK81J8LvvMzEEsS+d3mVYA fKkOuJPjxfs5prsq/VbB+8+PIJ0fzN6mh2QMSS69Fves8lSnC8bzRaVmMyTYeGCHwwVAqaBpS YXsJsoX25SzhZVe5FVl23BdkSBOfQkBbB8AqQhSTcM1J7ahHp/fWwUMnopf6ZEGnjfD+Op7X2 ZmZEzRhHXfAdJQO6vrIDTTLr5MQuCod8wy+k1rrdbaXEfCChSKcVre0NhvpnF45Qa/ZsXKwGQ 8BX2udeq4WceOK4mn3PcONdzmN3WbfmSpsSZij+Ii9w/TBOa2xfEC0cGwVSwFD+jDBi0jTOau TRZ3FK8PzfjQiegflOrs6ONbrMqF6p8t1kecf3iIwZDqE/M8tXZtk54E765wHKzmOrt47r1y6 SNBOACs7q6lGoto0PG2dMRYHRyBj7REJIZVZPZdMpSIuFj6nxQqWfwj7BZCRZTLxxj9/fB/lf 6+SntREiRFUs+MFJd6HK+c3GzwlShUSmnlaN9e4deq4Y0zRGJF8rNQkYd8RBv6WlF0aWsPqJ2 zIetPO27vbeuO3eSTTSamJmq9Lw9NSEcODbGoj+2ZXt0UgKWt3zBxmjNlCBR3PsSYYPQXMeK7 OaRjB61RbqBAgajuL+X+ddmxypML8E621NLr05h0rqAkqDywRpqriLXMzXc8Mip6sOdA+a7GM hPgE3SwbqZGza3UV Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thursday, September 1, 2016 8:44:49 PM CEST Dongdong Liu wrote: > 在 2016/9/1 15:41, Arnd Bergmann 写道: > > On Thursday, September 1, 2016 10:05:29 AM CEST Dongdong Liu wrote: > >> 在 2016/8/31 19:45, Arnd Bergmann 写道: > >>> On Wednesday, August 31, 2016 7:48:12 PM CEST Dongdong Liu wrote: > I know your point. > > 1. For our host bridge , ".map_bus = pci_ecam_map_bus" is only suitable for > accessing the EP config space. > pci_generic_config_read32() need to call "addr = bus->ops->map_bus(bus, devfn, where & ~0x3);", > > drivers/pci/host/pcie-hisi-acpi.c > static struct pci_ops hisi_pcie_ops = { > .map_bus = pci_ecam_map_bus, > .read = hisi_pcie_acpi_rd_conf, > .write = hisi_pcie_acpi_wr_conf, > }; > > Yes, we can change ".map_bus = pci_ecam_map_bus" to ".map_bus = hisi_pci_map_bus", and implentment hisi_pci_map_bus as below, > then we will not need to call hisi_pcie_common_cfg_read(). > > void __iomem *hisi_pci_map_bus(struct pci_bus *bus, unsigned int devfn, int where) > { > struct pci_config_window *cfg = bus->sysdata; > void __iomem *reg_base = cfg->priv; > > /* for RC config access*/ > if (bus->number == cfg->busr.start) > return reg_base + (where & ~0x3); > else > /* for EP config access */ > return pci_ecam_map_bus(bus, devfn, where); > } > > and hisi_pcie_acpi_rd_conf() need to change as below. > 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; > > if (hisi_pcie_acpi_valid_config(cfg, bus, PCI_SLOT(devfn)) == 0) > return PCIBIOS_DEVICE_NOT_FOUND; > > /* access RC config space */ > if (bus->number == cfg->busr.start) > return pci_generic_config_read32(bus, devfn, where, size, val); > > /* access EP config space */ > return pci_generic_config_read(bus, devfn, where, size, val); > } Right, this is what I had in mind. > 2. We need to backward compatible with the old dt way config access as below code, > so we have to call hisi_pcie_common_cfg_read() when accessing the RC config space. > For this, we have to call hisi_pcie_common_cfg_read(). > > drivers/pci/host/pcie-hisi.c > static inline int hisi_pcie_cfg_read(struct pcie_port *pp, int where, > int size, u32 *val) > { > struct hisi_pcie *pcie = to_hisi_pcie(pp); > > return hisi_pcie_common_cfg_read(pcie->reg_base, where, size, val); > } > > static struct pcie_host_ops hisi_pcie_host_ops = { > .rd_own_conf = hisi_pcie_cfg_read, > .wr_own_conf = hisi_pcie_cfg_write, > .link_up = hisi_pcie_link_up, > }; I think this would be easier if you separate the ACPI code from the DT code and not try to have a common file used for both. Sharing the config space accessors really isn't worth it when both variants are fairly simple to do, but they don't fit in a common model because one is called from the ACPI quirks and the other is called from the dw-pcie driver with completely different calling conventions. ARnd