From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753518AbcDUPyE (ORCPT ); Thu, 21 Apr 2016 11:54:04 -0400 Received: from lhrrgout.huawei.com ([194.213.3.17]:2075 "EHLO lhrrgout.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752053AbcDUPyB convert rfc822-to-8bit (ORCPT ); Thu, 21 Apr 2016 11:54:01 -0400 From: Gabriele Paoloni To: Bjorn Helgaas CC: Jisheng Zhang , "jingoohan1@gmail.com" , "pratyush.anand@gmail.com" , "bhelgaas@google.com" , "linux-pci@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" Subject: RE: [PATCH v2] PCI: designware: move remaining rc setup code to dw_pcie_setup_rc() Thread-Topic: [PATCH v2] PCI: designware: move remaining rc setup code to dw_pcie_setup_rc() Thread-Index: AQHRf3lGVLGDTqXebkC5RA6YylAqxZ981kjQgAEG4wCAAG7IQP//9QGAgAAqAHCAADKGAIABki0QgAAgiICABeh3MIAOgicAgAARpyA= Date: Thu, 21 Apr 2016 15:53:29 +0000 Message-ID: References: <1458128433-3020-1-git-send-email-jszhang@marvell.com> <20160407103734.55e72da7@xhacker> <20160407163443.291fbd49@xhacker> <20160407140551.GA2648@localhost> <20160408160145.GA10565@localhost> <20160421154832.GC32739@localhost> In-Reply-To: <20160421154832.GC32739@localhost> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.203.181.152] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090205.5718F780.00C1,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: c8c52022f55fc318e1635d1364311b83 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Bjorn > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: 21 April 2016 16:49 > To: Gabriele Paoloni > Cc: Jisheng Zhang; jingoohan1@gmail.com; pratyush.anand@gmail.com; > bhelgaas@google.com; linux-pci@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code > to dw_pcie_setup_rc() > > On Tue, Apr 12, 2016 at 09:43:32AM +0000, Gabriele Paoloni wrote: > > Hi Bjorn > > > > [...] > > > > > > > > > > > > What's the hisi plan for resuming after suspend-to-RAM? How > does > > > the > > > > > RC get reprogrammed after it loses all its state? > > > > > > > > PM is not part of the driver yet. This is planned for near > > > > future release so haven't made such considerations yet > > > > > > > > > > What would break if hisi did call dw_pcie_setup_rc()? I know > you > > > said > > > > > it would overwrite what the bootloader already did, which is > true. > > > > > > > > I am try to figure this out now with our HW team. > > > > > > > > > > > > > > But hisi does call dw_pcie_host_init(), so it reads pp->mem > (which > > > > > determines pp->mem_base) and pp->lanes from the DT. Other > drivers > > > > > then call dw_pcie_setup_rc() which programs the RC based on > > > > > pp->mem_base and pp->lanes. So hisi assumes UEFI programmed > the RC > > > to > > > > > match the DT, while the other drivers read the DT and program > the > > > RC > > > > > to match. The latter seems more robust because it enforces the > > > > > consistency rather than relying on it. > > > > > > > > Yes I agree with you, however we have preferred to move RC config > to > > > > BIOS to have a single driver to support multiple versions of the > > > > same SoC. > > > > > > I think there are two reasonable approaches: > > > > > > 1) A single generic driver that doesn't have any knowledge about > the > > > chipset registers; it uses run-time firmware interfaces to manage > > > the bridge. The ACPI pci_root.c driver is the best example so > far > > > and works very well. It supports basically all x86 and ia64 > > > chipsets and requires no kernel work for new ones. > > > > > > 2) Native drivers specific to each chipset. These may get > > > configuration information from DT, but they do their own > > > register-level programming of the device without run-time help > from > > > firmware. > > > > > > I think hisi is a native driver because it uses hip05/hip06 > registers > > > to check link state and perform config operations. And apparently > you > > > rely on the ATU, BAR, class, and link width programming currently > done > > > in dw_pcie_host_init(). But you want to rely on pre-boot firmware > to > > > set up the link. That doesn't make sense to me -- if the driver > wants > > > to twiddle the registers, it should know how to do it all. I don't > > > see how you can reasonably manage this half-way approach. > > > > > > > The patch I proposed above does the same job as the original > patch > > > > proposed by Jisheng and also allows hisi driver to call the moved > > > > code. > > > > > > > > Do you see anything wrong with it? > > > > > > Only that it makes the structure more complicated and we haven't > > > identified a corresponding benefit yet. > > > > Finally I have checked that assigning .host_init function pointer > > in our driver to call dw_pcie_setup_rc() will not affect the values > > already set by BIOS. > > > > Also I agree with you that a hybrid approach is not ideal. > > > > So I will update the driver to call dw_pcie_setup_rc() from > > .host_init and ask the BIOS team to update the firmware for next > > releases (the driver will be backward compatible anyway). > > Am I right in assuming that the patch currently in my tree: > > https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h= > pci/host-designware&id=1488aefa37a4033080942c860294d13c613ec829 > > will work for you? I'm going to assume so unless I hear otherwise. Yes you are right. I thought it was clear by the last conclusion. Sorry if it was not explicit. Many Thanks and Regards Gab > > Bjorn From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Gabriele Paoloni To: Bjorn Helgaas CC: Jisheng Zhang , "jingoohan1@gmail.com" , "pratyush.anand@gmail.com" , "bhelgaas@google.com" , "linux-pci@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" Subject: RE: [PATCH v2] PCI: designware: move remaining rc setup code to dw_pcie_setup_rc() Date: Thu, 21 Apr 2016 15:53:29 +0000 Message-ID: References: <1458128433-3020-1-git-send-email-jszhang@marvell.com> <20160407103734.55e72da7@xhacker> <20160407163443.291fbd49@xhacker> <20160407140551.GA2648@localhost> <20160408160145.GA10565@localhost> <20160421154832.GC32739@localhost> In-Reply-To: <20160421154832.GC32739@localhost> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: Hi Bjorn > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: 21 April 2016 16:49 > To: Gabriele Paoloni > Cc: Jisheng Zhang; jingoohan1@gmail.com; pratyush.anand@gmail.com; > bhelgaas@google.com; linux-pci@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code > to dw_pcie_setup_rc() > > On Tue, Apr 12, 2016 at 09:43:32AM +0000, Gabriele Paoloni wrote: > > Hi Bjorn > > > > [...] > > > > > > > > > > > > What's the hisi plan for resuming after suspend-to-RAM? How > does > > > the > > > > > RC get reprogrammed after it loses all its state? > > > > > > > > PM is not part of the driver yet. This is planned for near > > > > future release so haven't made such considerations yet > > > > > > > > > > What would break if hisi did call dw_pcie_setup_rc()? I know > you > > > said > > > > > it would overwrite what the bootloader already did, which is > true. > > > > > > > > I am try to figure this out now with our HW team. > > > > > > > > > > > > > > But hisi does call dw_pcie_host_init(), so it reads pp->mem > (which > > > > > determines pp->mem_base) and pp->lanes from the DT. Other > drivers > > > > > then call dw_pcie_setup_rc() which programs the RC based on > > > > > pp->mem_base and pp->lanes. So hisi assumes UEFI programmed > the RC > > > to > > > > > match the DT, while the other drivers read the DT and program > the > > > RC > > > > > to match. The latter seems more robust because it enforces the > > > > > consistency rather than relying on it. > > > > > > > > Yes I agree with you, however we have preferred to move RC config > to > > > > BIOS to have a single driver to support multiple versions of the > > > > same SoC. > > > > > > I think there are two reasonable approaches: > > > > > > 1) A single generic driver that doesn't have any knowledge about > the > > > chipset registers; it uses run-time firmware interfaces to manage > > > the bridge. The ACPI pci_root.c driver is the best example so > far > > > and works very well. It supports basically all x86 and ia64 > > > chipsets and requires no kernel work for new ones. > > > > > > 2) Native drivers specific to each chipset. These may get > > > configuration information from DT, but they do their own > > > register-level programming of the device without run-time help > from > > > firmware. > > > > > > I think hisi is a native driver because it uses hip05/hip06 > registers > > > to check link state and perform config operations. And apparently > you > > > rely on the ATU, BAR, class, and link width programming currently > done > > > in dw_pcie_host_init(). But you want to rely on pre-boot firmware > to > > > set up the link. That doesn't make sense to me -- if the driver > wants > > > to twiddle the registers, it should know how to do it all. I don't > > > see how you can reasonably manage this half-way approach. > > > > > > > The patch I proposed above does the same job as the original > patch > > > > proposed by Jisheng and also allows hisi driver to call the moved > > > > code. > > > > > > > > Do you see anything wrong with it? > > > > > > Only that it makes the structure more complicated and we haven't > > > identified a corresponding benefit yet. > > > > Finally I have checked that assigning .host_init function pointer > > in our driver to call dw_pcie_setup_rc() will not affect the values > > already set by BIOS. > > > > Also I agree with you that a hybrid approach is not ideal. > > > > So I will update the driver to call dw_pcie_setup_rc() from > > .host_init and ask the BIOS team to update the firmware for next > > releases (the driver will be backward compatible anyway). > > Am I right in assuming that the patch currently in my tree: > > https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h= > pci/host-designware&id=1488aefa37a4033080942c860294d13c613ec829 > > will work for you? I'm going to assume so unless I hear otherwise. Yes you are right. I thought it was clear by the last conclusion. Sorry if it was not explicit. Many Thanks and Regards Gab > > Bjorn From mboxrd@z Thu Jan 1 00:00:00 1970 From: gabriele.paoloni@huawei.com (Gabriele Paoloni) Date: Thu, 21 Apr 2016 15:53:29 +0000 Subject: [PATCH v2] PCI: designware: move remaining rc setup code to dw_pcie_setup_rc() In-Reply-To: <20160421154832.GC32739@localhost> References: <1458128433-3020-1-git-send-email-jszhang@marvell.com> <20160407103734.55e72da7@xhacker> <20160407163443.291fbd49@xhacker> <20160407140551.GA2648@localhost> <20160408160145.GA10565@localhost> <20160421154832.GC32739@localhost> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Bjorn > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas at kernel.org] > Sent: 21 April 2016 16:49 > To: Gabriele Paoloni > Cc: Jisheng Zhang; jingoohan1 at gmail.com; pratyush.anand at gmail.com; > bhelgaas at google.com; linux-pci at vger.kernel.org; linux- > kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org > Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code > to dw_pcie_setup_rc() > > On Tue, Apr 12, 2016 at 09:43:32AM +0000, Gabriele Paoloni wrote: > > Hi Bjorn > > > > [...] > > > > > > > > > > > > What's the hisi plan for resuming after suspend-to-RAM? How > does > > > the > > > > > RC get reprogrammed after it loses all its state? > > > > > > > > PM is not part of the driver yet. This is planned for near > > > > future release so haven't made such considerations yet > > > > > > > > > > What would break if hisi did call dw_pcie_setup_rc()? I know > you > > > said > > > > > it would overwrite what the bootloader already did, which is > true. > > > > > > > > I am try to figure this out now with our HW team. > > > > > > > > > > > > > > But hisi does call dw_pcie_host_init(), so it reads pp->mem > (which > > > > > determines pp->mem_base) and pp->lanes from the DT. Other > drivers > > > > > then call dw_pcie_setup_rc() which programs the RC based on > > > > > pp->mem_base and pp->lanes. So hisi assumes UEFI programmed > the RC > > > to > > > > > match the DT, while the other drivers read the DT and program > the > > > RC > > > > > to match. The latter seems more robust because it enforces the > > > > > consistency rather than relying on it. > > > > > > > > Yes I agree with you, however we have preferred to move RC config > to > > > > BIOS to have a single driver to support multiple versions of the > > > > same SoC. > > > > > > I think there are two reasonable approaches: > > > > > > 1) A single generic driver that doesn't have any knowledge about > the > > > chipset registers; it uses run-time firmware interfaces to manage > > > the bridge. The ACPI pci_root.c driver is the best example so > far > > > and works very well. It supports basically all x86 and ia64 > > > chipsets and requires no kernel work for new ones. > > > > > > 2) Native drivers specific to each chipset. These may get > > > configuration information from DT, but they do their own > > > register-level programming of the device without run-time help > from > > > firmware. > > > > > > I think hisi is a native driver because it uses hip05/hip06 > registers > > > to check link state and perform config operations. And apparently > you > > > rely on the ATU, BAR, class, and link width programming currently > done > > > in dw_pcie_host_init(). But you want to rely on pre-boot firmware > to > > > set up the link. That doesn't make sense to me -- if the driver > wants > > > to twiddle the registers, it should know how to do it all. I don't > > > see how you can reasonably manage this half-way approach. > > > > > > > The patch I proposed above does the same job as the original > patch > > > > proposed by Jisheng and also allows hisi driver to call the moved > > > > code. > > > > > > > > Do you see anything wrong with it? > > > > > > Only that it makes the structure more complicated and we haven't > > > identified a corresponding benefit yet. > > > > Finally I have checked that assigning .host_init function pointer > > in our driver to call dw_pcie_setup_rc() will not affect the values > > already set by BIOS. > > > > Also I agree with you that a hybrid approach is not ideal. > > > > So I will update the driver to call dw_pcie_setup_rc() from > > .host_init and ask the BIOS team to update the firmware for next > > releases (the driver will be backward compatible anyway). > > Am I right in assuming that the patch currently in my tree: > > https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h= > pci/host-designware&id=1488aefa37a4033080942c860294d13c613ec829 > > will work for you? I'm going to assume so unless I hear otherwise. Yes you are right. I thought it was clear by the last conclusion. Sorry if it was not explicit. Many Thanks and Regards Gab > > Bjorn