From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751785AbcFWBfe (ORCPT ); Wed, 22 Jun 2016 21:35:34 -0400 Received: from mail-pa0-f50.google.com ([209.85.220.50]:34319 "EHLO mail-pa0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751312AbcFWBfb (ORCPT ); Wed, 22 Jun 2016 21:35:31 -0400 Date: Wed, 22 Jun 2016 18:35:27 -0700 From: Brian Norris To: Shawn Lin Cc: Bjorn Helgaas , devicetree@vger.kernel.org, Heiko Stuebner , Arnd Bergmann , Marc Zyngier , linux-pci@vger.kernel.org, Wenrui Li , linux-kernel@vger.kernel.org, Doug Anderson , linux-rockchip@lists.infradead.org, Rob Herring Subject: Re: [PATCH v3 2/2] PCI: Rockchip: Add Rockchip PCIe controller support Message-ID: <20160623013527.GA46876@google.com> References: <1466041835-1695-1-git-send-email-shawn.lin@rock-chips.com> <20160623002919.GA21157@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Thu, Jun 23, 2016 at 09:09:46AM +0800, Shawn Lin wrote: > 在 2016/6/23 8:29, Brian Norris 写道: > >On Thu, Jun 16, 2016 at 09:50:35AM +0800, Shawn Lin wrote: [...] > >>+ /* 500ms timeout value should be enough for gen1/2 taining */ > >>+ timeout = jiffies + msecs_to_jiffies(500); > >>+ > >>+ err = -ETIMEDOUT; > >>+ while (time_before(jiffies, timeout)) { > >>+ status = pcie_read(port, PCIE_CLIENT_BASIC_STATUS1); > >>+ if (((status >> PCIE_CLIENT_LINK_STATUS_SHIFT) & > >>+ PCIE_CLIENT_LINK_STATUS_MASK) == > >>+ PCIE_CLIENT_LINK_STATUS_UP) { > >>+ dev_dbg(port->dev, "pcie link training gen1 pass!\n"); > >>+ err = 0; > >>+ break; > >>+ } > >>+ msleep(20); > >>+ } > > > >Technically, the above timeout loop is not quite correct. Possible error > >case: we can fail with a timeout after 500 ms if the training completes > >between the 480-500 ms time window. This can happen because you're > >doing: > > > >(1) read register: if complete, then terminate successfully > >(2) delay > >(3) check for timeout: if time is up, return error > > > >You actually need: > > > >(1) read register: if complete, then terminate successfully > >(2) delay > >(3) check for timeout: if time is up, repeat (1), and then report error > > > >You can examine the logic for readx_poll_timeout() in > >include/linux/iopoll.h to see an example of a proper timeout loop. You > >could even try to use one of the helpers there, except that your > >pcie_read() takes 2 args. > > I see, thanks. > > > > >>+ if (err) { > >>+ dev_err(port->dev, "pcie link training gen1 timeout!\n"); > >>+ return err; > >>+ } > >>+ > >>+ /* > >>+ * Enable retrain for gen2. This should be configured only after > >>+ * gen1 finished. > >>+ */ > >>+ status = pcie_read(port, > >>+ PCIR_RC_CONFIG_LCS + PCIE_RC_CONFIG_BASE); > >>+ status |= PCIE_CORE_LCSR_RETAIN_LINK; > >>+ pcie_write(port, status, > >>+ PCIR_RC_CONFIG_LCS + PCIE_RC_CONFIG_BASE); > > > >I'm not really an expert on this, but how are you actually "retraining > >for gen2"? Is that just the behavior of the core, that it retries at the > >higher rate on the 2nd training attempt? I'm just confused, since you > >set PCIE_CLIENT_GEN_SEL_2 above, so you've already allowed either gen1 > >or gen2 negotiation AFAICT, and so seemingly you might already have > >negotiated at gen2. > > > Not really. I allow the core to enable gen2, but it needs a extra > setting of retrain after finishing gen1. It's not so strange as it > depends on the vendor's design. So I have to say it fits the > designer's expectation. OK. > >>+ err = -ETIMEDOUT; > >>+ > >>+ while (time_before(jiffies, timeout)) { > > > >You never reset 'timeout' when starting this loop. So you only have a > >cumulative 500 ms to do both the gen1 and gen2 loops. Is that > >intentional? (I feel like this comment was made on an earlier revision > >of this driver, though I haven't read everything thoroughly.) > > yes, I don't have any docs to let me know how long should I wait for > gen1/2 to be finished. Maybe someday it will be augmented to a larger > value if finding a device actually need a longer time. But the only > thing I can say is that it's from my test for many pcie devices > currently. > > > Do you agree? I'm not suggesting increasing the timeout, exactly; I'm suggesting that you should set some minimum timeout for each training loop, instead of reusing the same deadline. i.e., something like this before the second loop: /* Reset the deadline for gen2 */ timeout = jiffies + msecs_to_jiffies(500); As it stands, if the first loop takes 490 ms, that leaves you only 10 ms for the second loop. And I think it'd be confusing if we ever see the first loop take a "long" time, and then we time out on the second -- we'd be blaming the gen2 training for gen1's slowness. > >>+ status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE); > >>+ if (((status >> PCIE_CORE_PL_CONF_SPEED_SHIFT) & > >>+ PCIE_CORE_PL_CONF_SPEED_MASK) == > >>+ PCIE_CORE_PL_CONF_SPEED_50G) { > >>+ dev_dbg(port->dev, "pcie link training gen2 pass!\n"); > >>+ err = 0; > >>+ break; > >>+ } > >>+ msleep(20); > >>+ } > > > >Similar problem with your timeout loop, as mentioned above. Although I > >confused about what you do in the error case here... > > > >>+ if (err) > >>+ dev_dbg(port->dev, "pcie link training gen2 timeout, force to gen1!\n"); > > > >... how are you forcing gen1? I don't see any code that indicates this. > >Should you at least be checking that there aren't some kind of training > >errors, and that we settled back on gen1/2.5G? > > yes, when failing gen2, my pcie controller will fallback to gen1 > automatically. OK. Well maybe the text should say something like "falling back" instead of "force"? > if we pass the gen1 then fail to train gen2, a dbg msg here is enough > here to let we know that we should check the HW signal. Of course we > should make sure that this device supports gen2. OK. [...] Brian