From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754571AbcITJpr (ORCPT ); Tue, 20 Sep 2016 05:45:47 -0400 Received: from lhrrgout.huawei.com ([194.213.3.17]:3451 "EHLO lhrrgout.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752145AbcITJpn (ORCPT ); Tue, 20 Sep 2016 05:45:43 -0400 From: Gabriele Paoloni To: Arnd Bergmann , "liudongdong (C)" CC: "helgaas@kernel.org" , "rafael@kernel.org" , "Lorenzo.Pieralisi@arm.com" , "tn@semihalf.com" , "Wangzhou (B)" , "pratyush.anand@gmail.com" , "linux-pci@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "jcm@redhat.com" , "Chenxin (Charles)" , "hanjun.guo@linaro.org" , Linuxarm Subject: RE: [RFC PATCH V2 1/3] PCI: hisi: re-architect Hip05/Hip06 controllers driver to preapare for ACPI Thread-Topic: [RFC PATCH V2 1/3] PCI: hisi: re-architect Hip05/Hip06 controllers driver to preapare for ACPI Thread-Index: AQHSA3uwE98qoTe6Ok6YN71SzlNtK6Bi4pYAgADwToCAAF3CgIAAVN+AgAAVp4CAHaNEUA== Date: Tue, 20 Sep 2016 09:45:13 +0000 Message-ID: References: <1472644094-82731-1-git-send-email-liudongdong3@huawei.com> <5913196.I3zIbc2qla@wuerfel> <57C822C1.9000203@huawei.com> <6532722.cVEMfv9Kqj@wuerfel> In-Reply-To: <6532722.cVEMfv9Kqj@wuerfel> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.203.181.151] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020205.57E10538.0039,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 95f116a612ac893318a22f6cc09cb52e Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u8K9juRo031814 Hi Arnd, thanks for your comments > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 01 September 2016 15:02 > To: liudongdong (C) > Cc: helgaas@kernel.org; rafael@kernel.org; Lorenzo.Pieralisi@arm.com; > tn@semihalf.com; Wangzhou (B); pratyush.anand@gmail.com; linux- > pci@vger.kernel.org; linux-acpi@vger.kernel.org; linux- > kernel@vger.kernel.org; jcm@redhat.com; Gabriele Paoloni; Chenxin > (Charles); hanjun.guo@linaro.org; Linuxarm > Subject: Re: [RFC PATCH V2 1/3] PCI: hisi: re-architect Hip05/Hip06 > controllers driver to preapare for ACPI > > 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. Not sure about this... >>From my perspective having the shared code would make clear that the two drivers (ACPI and DT) are kind of related... For example see the reply from Bjorn to the xgene driver: https://lkml.org/lkml/2016/9/19/749 I know in our case the duplication isn't much but as I said I am a bit reluctant to rework this... Thx Gab > > ARnd