From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932996AbcHXSkj (ORCPT ); Wed, 24 Aug 2016 14:40:39 -0400 Received: from mail-pa0-f49.google.com ([209.85.220.49]:36559 "EHLO mail-pa0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932846AbcHXSkf (ORCPT ); Wed, 24 Aug 2016 14:40:35 -0400 Subject: Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only To: Bjorn Helgaas , Ley Foon Tan References: <1471595078-26297-1-git-send-email-lftan@altera.com> <20160822154738.GB18628@localhost> <20160824175431.GC23914@localhost> Cc: Bjorn Helgaas , "linux-kernel@vger.kernel.org" , linux-pci , Ray Jui , Scott Branden , Jon Mason , bcm-kernel-feedback-list@broadcom.com From: Scott Branden Message-ID: <4975a3ff-e04e-a1ac-e6cb-6d1b3e87f377@broadcom.com> Date: Wed, 24 Aug 2016 11:40:16 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160824175431.GC23914@localhost> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Bjorn, Ray is off this week and will likely have some comments next week. On 16-08-24 10:54 AM, Bjorn Helgaas wrote: > [+cc Ray, Scott, Jon, bcm-kernel-feedback-list] > > On Wed, Aug 24, 2016 at 03:07:52PM +0800, Ley Foon Tan wrote: >> On Mon, Aug 22, 2016 at 11:47 PM, Bjorn Helgaas wrote: >>> On Fri, Aug 19, 2016 at 04:24:38PM +0800, Ley Foon Tan wrote: >>>> Altera PCIe IP can be configured as rootport or device and they might have >>>> same vendor ID. It will cause the system hang issue if Altera PCIe is in >>>> endpoint mode and work with other PCIe rootport that from other vendors. >>>> So, add the rootport mode checking in link retrain fixup function. >>>> >>>> Signed-off-by: Ley Foon Tan >>>> --- >>>> v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT >>>> --- >>>> drivers/pci/host/pcie-altera.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c >>>> index 58eef99..33b6968 100644 >>>> --- a/drivers/pci/host/pcie-altera.c >>>> +++ b/drivers/pci/host/pcie-altera.c >>>> @@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct pci_dev *dev) >>>> u16 linkcap, linkstat; >>>> struct altera_pcie *pcie = dev->bus->sysdata; >>>> >>>> + if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) >>>> + return; >>>> + >>>> if (!altera_pcie_link_is_up(pcie)) >>>> return; >>> >>> Instead of making this a PCI fixup, can you make an >>> altera_pcie_host_init() function, call it from altera_pcie_probe(), >>> and do the link retrain there? Then you wouldn't need to worry about >>> whether this is a Root Port or an Endpoint, plus it would make the >>> altera driver structure more like the other drivers. >>> >>> You would call altera_pcie_host_init() before pci_scan_root_bus(), so >>> you wouldn't have a pci_dev yet, so you wouldn't be able to use >>> pcie_capability_set_word() to set the PCI_EXP_LNKCTL_RL bit. But I >>> assume there's some device-dependent way to access it using >>> cra_writel()? >> We can't use cra_write() to set PCI_EXP_LNKCTL_RL bit. > > Why not? I don't mean it has to be cra_write(), but isn't there some > way you can write that bit before we scan the root bus? It doesn't > make sense that we have to scan the bus before we can train the link. > > We want to be able to tell the PCI core "all the device-specific root > complex initialization has been done, here are the config accessors > you need, please scan for devices." I want to keep device-specific > things like this quirk directly in the driver and out of the > enumeration process. > >> We can use >> pci_bus_find_capability() and pci_bus_read_config_word() with struct >> pci_bus instead. >> But this only can be called after pci_scan_root_bus(). > >> Found >> iproc_pcie_check_link() have similar implementation. > > You're right, and I don't like iproc_pcie_check_link() either, for the > same reasons. > > The iproc_pcie_check_link() is a little better because it's called > before enumeration: > > pci_create_root_bus() > iproc_pcie_check_link() > pci_scan_child_bus() > > But it would be a lot better if iproc_pcie_check_link() were done > first, before pci_create_root_bus(). Then it would be more like the > structure of other drivers, and we could use pci_scan_root_bus() > instead. > > Comments, iproc folks? > > Bjorn > Regards, Scott