From patchwork Fri Jun 12 20:20:08 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 483758 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 63718140497 for ; Sat, 13 Jun 2015 06:20:15 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.b=nGBpCZjc; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752092AbbFLUUN (ORCPT ); Fri, 12 Jun 2015 16:20:13 -0400 Received: from mail-ig0-f180.google.com ([209.85.213.180]:38504 "EHLO mail-ig0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752215AbbFLUUM (ORCPT ); Fri, 12 Jun 2015 16:20:12 -0400 Received: by igblz2 with SMTP id lz2so17728684igb.1 for ; Fri, 12 Jun 2015 13:20:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=1ZI7gtk7o8HNS969yrKM4zim9+6LU07CHZQT4AH2rY0=; b=nGBpCZjclyWdeyLNmIuSy7+6XAVyIklCwyDP4/21sVVBb+ZEkFWkCkNS29qLih0+c7 Q0CCaOJ6wi1QC1MV8c0HZ4qOiP53U9tAoR1ACgbah5FMI/b2s32W9nvskDH5M9yGURNj MoWSH1t5Jrm2eGr7MLVvbSyQk6FNwa80+mbW84/P7Icar8JtZO6WJlw8HtVVIRIoigk2 awVYW6VjGf1gBPIVceJ4nBbsKumYgWhFEs1Wgu6OX5U6CMhJW4CBlCbNQMlmGCArV2gx hoXmpUum4Y0XmpuVZpJecwjObevh2DKEOYefhMgON8BBSwTra7r6mGZNGaC9PgZ2vmK8 8WGg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=1ZI7gtk7o8HNS969yrKM4zim9+6LU07CHZQT4AH2rY0=; b=GGk11X8MCYYq7M96PfFoPNq32ZS8quC/7TZRKiq+MjA44qg191rok0Day7b2S/VqHf OkI6uaHEBRwt0NaPBURaXaeHMo8S+CLqBJrrfo83l7lCz26hvKJczqh2dqAjkg9CxwpS nYFmyIOpEGCCes/oYw1mBUiuYTHztiDxtqNNlpIhEQH2JcA7VFoTC6qZdE5PCTl05oes OeGtZVDXdu4aeWzB/fG7uC1+uAebWM+gd2WT11zqKH7Q3lOvr4JbDyJe7xgPpSVPE069 WdRF+ENrznvpNJHa+oTWD/E7k4YM5+m2hgIMPEimaNt6nlOeX9IK5LBnrMwyNY/PjKST IQ7Q== X-Gm-Message-State: ALoCoQmmz1qG+jYolX4FFKxNOTRPj9CA7v9hiw/ENtVIJKQXXTG3mCNMd8/zt7uF60Lbl9KC3fg/ X-Received: by 10.42.194.66 with SMTP id dx2mr18396340icb.39.1434140411703; Fri, 12 Jun 2015 13:20:11 -0700 (PDT) Received: from google.com ([69.71.1.1]) by mx.google.com with ESMTPSA id h7sm1857652igt.3.2015.06.12.13.20.10 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 12 Jun 2015 13:20:10 -0700 (PDT) Date: Fri, 12 Jun 2015 15:20:08 -0500 From: Bjorn Helgaas To: Troy Kisky Cc: festevam@gmail.com, marex@denx.de, linux-pci@vger.kernel.org, r65037@freescale.com, l.stach@pengutronix.de, tharvey@gateworks.com Subject: Re: [PATCH 1/1] pci-imx6: add speed change timeout message Message-ID: <20150612202008.GI23119@google.com> References: <1433543864-7252-1-git-send-email-troy.kisky@boundarydevices.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1433543864-7252-1-git-send-email-troy.kisky@boundarydevices.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org Hi Troy, On Fri, Jun 05, 2015 at 03:37:44PM -0700, Troy Kisky wrote: > Currently, the timeout is never detected as count > has a value of -1 if a timeout happens, but the code is checking > for 0. Also, this patch removes the unneeded final wait > if a timeout occurs. > > Signed-off-by: Troy Kisky > --- > drivers/pci/host/pci-imx6.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index fdb9536..51be92c 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -398,20 +398,22 @@ static int imx6_pcie_start_link(struct pcie_port *pp) > writel(tmp, pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL); > > count = 200; > - while (count--) { > + while (1) { > tmp = readl(pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL); > /* Test if the speed change finished. */ > - if (!(tmp & PORT_LOGIC_SPEED_CHANGE)) > + if (!(tmp & PORT_LOGIC_SPEED_CHANGE)) { > + /* Make sure link training is finished as well! */ > + ret = imx6_pcie_wait_for_link(pp); > + break; > + } > + if (count-- == 0) { > + dev_err(pp->dev, "Speed change timeout\n"); > + ret = -EINVAL; > break; > + } > usleep_range(100, 1000); > } > > - /* Make sure link training is finished as well! */ > - if (count) > - ret = imx6_pcie_wait_for_link(pp); > - else > - ret = -EINVAL; > - > if (ret) { > dev_err(pp->dev, "Failed to bring link up!\n"); > } else { This looks fine functionally, but I propose the following slight rearrangement because I have another patch that makes all the "wait_for_link" timeout loops look similar, and this loop might as well be similar, too. Unrelated to your patch, but noticed while doing this: what's the magic constant 0x80 here? + tmp = readl(pp->dbi_base + 0x80); Is that correct? Can we add a symbolic name for it? Bjorn commit 24533d23e8fa Author: Troy Kisky Date: Fri Jun 12 14:30:16 2015 -0500 PCI: imx6: Add speed change timeout message Currently, the timeout is never detected as count has a value of -1 if a timeout happens, but the code is checking for 0. Also, this patch removes the unneeded final wait if a timeout occurs. [bhelgaas: reworked starting from http://lkml.kernel.org/r/1433543864-7252-1-git-send-email-troy.kisky@boundarydevices.com] Signed-off-by: Troy Kisky Signed-off-by: Bjorn Helgaas --- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c index fdb9536..2bbd38d 100644 --- a/drivers/pci/host/pci-imx6.c +++ b/drivers/pci/host/pci-imx6.c @@ -352,6 +352,23 @@ static int imx6_pcie_wait_for_link(struct pcie_port *pp) return 0; } +static int imx6_pcie_wait_for_speed_change(struct pcie_port *pp) +{ + uint32_t tmp; + unsigned int retries; + + for (retries = 0; retries < 200; retries++) + tmp = readl(pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL); + /* Test if the speed change finished. */ + if (!(tmp & PORT_LOGIC_SPEED_CHANGE)) + return 0; + usleep_range(100, 1000); + } + + dev_err(pp->dev, "Speed change timeout\n"); + return -EINVAL; +} + static irqreturn_t imx6_pcie_msi_handler(int irq, void *arg) { struct pcie_port *pp = arg; @@ -363,7 +380,7 @@ static int imx6_pcie_start_link(struct pcie_port *pp) { struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp); uint32_t tmp; - int ret, count; + int ret; /* * Force Gen1 operation when starting the link. In case the link is @@ -397,29 +414,22 @@ static int imx6_pcie_start_link(struct pcie_port *pp) tmp |= PORT_LOGIC_SPEED_CHANGE; writel(tmp, pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL); - count = 200; - while (count--) { - tmp = readl(pp->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL); - /* Test if the speed change finished. */ - if (!(tmp & PORT_LOGIC_SPEED_CHANGE)) - break; - usleep_range(100, 1000); + ret = imx6_pcie_wait_for_speed_change(pp); + if (ret) { + dev_err(pp->dev, "Failed to bring link up!\n"); + return ret; } /* Make sure link training is finished as well! */ - if (count) - ret = imx6_pcie_wait_for_link(pp); - else - ret = -EINVAL; - + ret = imx6_pcie_wait_for_link(pp); if (ret) { dev_err(pp->dev, "Failed to bring link up!\n"); - } else { - tmp = readl(pp->dbi_base + 0x80); - dev_dbg(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf); + return ret; } - return ret; + tmp = readl(pp->dbi_base + 0x80); + dev_dbg(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf); + return 0; } static void imx6_pcie_host_init(struct pcie_port *pp)