Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Thierry Reding <thierry.reding@gmail.com>
To: Manikanta Maddireddy <mmaddireddy@nvidia.com>
Cc: bhelgaas@google.com, robh+dt@kernel.org, mark.rutland@arm.com,
	jonathanh@nvidia.com, lorenzo.pieralisi@arm.com,
	vidyas@nvidia.com, linux-tegra@vger.kernel.org,
	linux-pci@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 04/30] PCI: tegra: Add PCIe Gen2 link speed support
Date: Mon, 15 Apr 2019 13:21:08 +0200
Message-ID: <20190415112108.GE29254@ulmo> (raw)
In-Reply-To: <20190411170355.6882-5-mmaddireddy@nvidia.com>

[-- Attachment #1: Type: text/plain, Size: 5219 bytes --]

On Thu, Apr 11, 2019 at 10:33:29PM +0530, Manikanta Maddireddy wrote:
> Tegra124, 132, 210 and 186 support Gen2 link speed. After PCIe link is up
> in Gen1, set target link speed as Gen2 and retrain link. Link switches to
> Gen2 speed if Gen2 capable end point is connected, else link stays in Gen1.
> 
> Per PCIe 4.0r0.9 sec 7.6.3.7 implementation note, driver need to wait for
> PCIe LTSSM to come back from recovery before retraining the link.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
>  drivers/pci/controller/pci-tegra.c | 61 ++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index a61ce9d475b4..6ccda82735f8 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -191,6 +191,8 @@
>  #define  RP_LINK_CONTROL_STATUS_DL_LINK_ACTIVE	0x20000000
>  #define  RP_LINK_CONTROL_STATUS_LINKSTAT_MASK	0x3fff0000
>  
> +#define RP_LINK_CONTROL_STATUS_2		0x000000b0
> +
>  #define PADS_CTL_SEL		0x0000009c
>  
>  #define PADS_CTL		0x000000a0
> @@ -2096,6 +2098,62 @@ static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
>  		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
>  }
>  
> +#define LINK_RETRAIN_TIMEOUT 100000

This is oddly placed. I think this should go somewhere near the top of
the file. We already have PME_ACK_TIMEOUT there.

But to be honest, I wouldn't even bother with the #define. This is used
exactly twice and is much longer to type than the actual number.

> +
> +static void tegra_pcie_change_link_speed(struct tegra_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	struct tegra_pcie_port *port, *tmp;
> +	ktime_t deadline;
> +	u32 val;

The driver uses u32 value for register values elsewhere. It'd be good to
stay consistent with that convention.

> +
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +		/*
> +		 * Link Capabilities 2 register is hardwired to 0 in Tegra,
> +		 * so no need to read it before setting target speed.
> +		 */
> +		val = readl(port->base + RP_LINK_CONTROL_STATUS_2);
> +		val &= ~PCI_EXP_LNKSTA_CLS;
> +		val |= PCI_EXP_LNKSTA_CLS_5_0GB;
> +		writel(val, port->base + RP_LINK_CONTROL_STATUS_2);

The comment says there's no need to read the register, but then the code
goes on and reads it before modifying it.

That's the first thing that came to my mind. Then I realized that the
code doesn't actually do anything with the Link Capabilities 2 register
at all. So what's the deal here? Is it that the Link Capabilities 2
register being hardwired to 0 means that we can change the target speed?
Your comment needs to explain more clearly how it relates to the code.

> +
> +		/*
> +		 * Poll until link comes back from recovery to avoid race
> +		 * condition.
> +		 */
> +		deadline = ktime_add_us(ktime_get(), LINK_RETRAIN_TIMEOUT);
> +		for (;;) {
> +			val = readl(port->base + RP_LINK_CONTROL_STATUS);
> +			if (!(val & PCI_EXP_LNKSTA_LT))
> +				break;
> +			if (ktime_after(ktime_get(), deadline))
> +				break;
> +			usleep_range(2000, 3000);
> +		}

This would be more compact when written as a while loop. Also I think
it's more readable to make the !(...) an explicit comparison. Finally,
use whitespace to improve readability. The above looks very cluttered
and, in my opinion, makes the code difficult to read. Something like
the below is much easier to read, in my opinion:

		while (ktime_before(ktime_get(), deadline)) {
			value = readl(port->base + RP_LINK_CONTROL_STATUS);
			if ((value & PCI_EXP_LNKSTA_LT) == 0)
				break;

			usleep_range(2000, 3000);
		}

> +		if (val & PCI_EXP_LNKSTA_LT)
> +			dev_err(dev, "PCIe port %u link is still in recovery\n",
> +				port->index);

Since you're continuing execution, perhaps make this dev_warn()?

> +
> +		/* Retrain the link */
> +		val = readl(port->base + RP_LINK_CONTROL_STATUS);
> +		val |= PCI_EXP_LNKCTL_RL;
> +		writel(val, port->base + RP_LINK_CONTROL_STATUS);
> +
> +		deadline = ktime_add_us(ktime_get(), LINK_RETRAIN_TIMEOUT);
> +		for (;;) {
> +			val = readl(port->base + RP_LINK_CONTROL_STATUS);
> +			if (!(val & PCI_EXP_LNKSTA_LT))
> +				break;
> +			if (ktime_after(ktime_get(), deadline))
> +				break;
> +			usleep_range(2000, 3000);
> +		}

Same comments as above.

> +		if (val & PCI_EXP_LNKSTA_LT)
> +			dev_err(dev, "link retrain of PCIe port %u failed\n",
> +				port->index);
> +	}

Most of the error messages in this file are of the form:

	"failed to ..."

Perhaps make this:

	"failed to retrain link of port %u\n"

for consistency?

Thierry

> +}
> +
>  static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
> @@ -2122,6 +2180,9 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>  		tegra_pcie_port_disable(port);
>  		tegra_pcie_port_free(port);
>  	}
> +
> +	if (pcie->soc->has_gen2)
> +		tegra_pcie_change_link_speed(pcie);
>  }
>  
>  static void tegra_pcie_disable_ports(struct tegra_pcie *pcie)
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply index

Thread overview: 106+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-11 17:03 [PATCH 00/30] Enable Tegra PCIe root port features Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 01/30] soc/tegra: pmc: Export tegra_powergate_power_on() Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 02/30] PCI: tegra: Fix PCIe host power up sequence Manikanta Maddireddy
2019-04-15 11:01   ` Thierry Reding
2019-04-15 14:11     ` Manikanta Maddireddy
2019-04-15 14:30       ` Thierry Reding
2019-04-15 18:14         ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 03/30] PCI: tegra: Move REFCLK pad settings out of phy_power_on() Manikanta Maddireddy
2019-04-15 11:06   ` Thierry Reding
2019-04-15 14:20     ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 04/30] PCI: tegra: Add PCIe Gen2 link speed support Manikanta Maddireddy
2019-04-15 11:21   ` Thierry Reding [this message]
2019-04-15 14:47     ` Manikanta Maddireddy
2019-04-15 15:36       ` Thierry Reding
2019-04-15 15:53         ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 05/30] PCI: tegra: Advertise PCIe Advanced Error Reporting (AER) capability Manikanta Maddireddy
2019-04-15 11:23   ` Thierry Reding
2019-04-15 14:49     ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 06/30] PCI: tegra: Program UPHY electrical settings for Tegra210 Manikanta Maddireddy
2019-04-15 11:29   ` Thierry Reding
2019-04-15 14:55     ` Manikanta Maddireddy
2019-04-15 15:38       ` Thierry Reding
2019-04-11 17:03 ` [PATCH 07/30] PCI: tegra: Enable opportunistic update FC and ACK Manikanta Maddireddy
2019-04-15 11:30   ` Thierry Reding
2019-04-11 17:03 ` [PATCH 08/30] PCI: tegra: Disable AFI dynamic clock gating Manikanta Maddireddy
2019-04-15 11:32   ` Thierry Reding
2019-04-11 17:03 ` [PATCH 09/30] PCI: tegra: Process pending DLL transactions before entering L1 or L2 Manikanta Maddireddy
2019-04-15 11:33   ` Thierry Reding
2019-04-11 17:03 ` [PATCH 10/30] PCI: tegra: Enable PCIe xclk clock clamping Manikanta Maddireddy
2019-04-15 11:37   ` Thierry Reding
2019-04-15 14:58     ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 11/30] PCI: tegra: Increase the deskew retry time Manikanta Maddireddy
2019-04-15 11:39   ` Thierry Reding
2019-04-15 14:58     ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 12/30] PCI: tegra: Add SW fixup for RAW violations Manikanta Maddireddy
2019-04-11 20:01   ` Bjorn Helgaas
2019-04-12  5:59     ` Manikanta Maddireddy
2019-04-15 11:41       ` Thierry Reding
2019-04-15 11:45   ` Thierry Reding
2019-04-15 15:02     ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 13/30] PCI: tegra: Update flow control threshold in Tegra210 Manikanta Maddireddy
2019-04-15 11:47   ` Thierry Reding
2019-04-15 15:05     ` Manikanta Maddireddy
2019-04-23  9:27       ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 14/30] PCI: tegra: Set target speed as Gen1 before link up Manikanta Maddireddy
2019-04-11 20:04   ` Bjorn Helgaas
2019-04-12  6:44     ` Manikanta Maddireddy
2019-04-12 14:35       ` Bjorn Helgaas
2019-04-15 10:43         ` Manikanta Maddireddy
2019-04-15 11:52   ` Thierry Reding
2019-04-15 15:12     ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 15/30] PCI: tegra: Fix PLLE powerdown issue due to CLKREQ# signal Manikanta Maddireddy
2019-04-15 13:17   ` Thierry Reding
2019-04-15 15:14     ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 16/30] PCI: tegra: Program AFI_CACHE* registers only for Tegra20 Manikanta Maddireddy
2019-04-15 13:20   ` Thierry Reding
2019-04-16 10:47     ` Manikanta Maddireddy
2019-04-16 16:11       ` Thierry Reding
2019-04-11 17:03 ` [PATCH 17/30] PCI: tegra: Use switch statements in tegra_pcie_isr() Manikanta Maddireddy
2019-04-15 13:25   ` Thierry Reding
2019-04-15 15:25     ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 18/30] PCI: tegra: Change PRSNT_SENSE irq log to debug Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 19/30] PCI: tegra: Use legacy irq for port service drivers Manikanta Maddireddy
2019-04-15 13:35   ` Thierry Reding
2019-04-11 17:03 ` [PATCH 20/30] PCI: tegra: Add AFI_PEX2_CTRL reg offset as part of soc struct Manikanta Maddireddy
2019-04-15 13:31   ` Thierry Reding
2019-04-11 17:03 ` [PATCH 21/30] PCI: tegra: Add "pci" type check before parsing child device tree node Manikanta Maddireddy
2019-04-15 13:37   ` Thierry Reding
2019-04-15 15:30     ` Manikanta Maddireddy
2019-04-15 15:42       ` Thierry Reding
2019-04-11 17:03 ` [PATCH 22/30] PCI: tegra: Access endpoint config only if PCIe link is up Manikanta Maddireddy
2019-04-11 20:15   ` Bjorn Helgaas
2019-04-12  7:00     ` Manikanta Maddireddy
2019-04-12 14:50       ` Bjorn Helgaas
2019-04-15 11:36         ` Manikanta Maddireddy
2019-04-15 13:45           ` Thierry Reding
2019-04-15 13:52             ` Thierry Reding
2019-04-15 14:04           ` Bjorn Helgaas
2019-04-15 15:43             ` Manikanta Maddireddy
2019-04-23 20:24               ` Bjorn Helgaas
2019-04-11 17:03 ` [PATCH 23/30] dt-bindings: pci: tegra: Document PCIe DPD pinctrl optional prop Manikanta Maddireddy
2019-04-15 14:07   ` Thierry Reding
2019-04-15 15:48     ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 24/30] arm64: tegra: Add PEX DPD states as pinctrl properties Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 25/30] PCI: tegra: Put PEX CLK & BIAS pads in DPD mode Manikanta Maddireddy
2019-04-15 14:11   ` Thierry Reding
2019-04-11 17:03 ` [PATCH 26/30] dt-bindings: pci: tegra: Document nvidia,plat-gpios optional prop Manikanta Maddireddy
2019-04-11 20:18   ` Bjorn Helgaas
2019-04-12  7:01     ` Manikanta Maddireddy
2019-04-15 14:16   ` Thierry Reding
2019-04-15 17:58     ` Manikanta Maddireddy
2019-04-16 15:34       ` Thierry Reding
2019-04-17 11:22         ` Manikanta Maddireddy
2019-04-17 15:19           ` Thierry Reding
2019-04-17 18:26             ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 27/30] PCI: tegra: Add support to configure platform GPIOs Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 28/30] dt-bindings: pci: tegra: Document nvidia,rst-gpio optional prop Manikanta Maddireddy
2019-04-15 14:20   ` Thierry Reding
2019-04-15 18:01     ` Manikanta Maddireddy
2019-04-29 18:33     ` Rob Herring
2019-04-11 17:03 ` [PATCH 29/30] PCI: tegra: Add support for GPIO based PCIe reset Manikanta Maddireddy
2019-04-15 14:20   ` Thierry Reding
2019-04-15 18:03     ` Manikanta Maddireddy
2019-04-11 17:03 ` [PATCH 30/30] PCI: tegra: Change link retry log level to INFO Manikanta Maddireddy
2019-04-15 14:23   ` Thierry Reding
2019-04-15 18:05     ` Manikanta Maddireddy

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190415112108.GE29254@ulmo \
    --to=thierry.reding@gmail.com \
    --cc=bhelgaas@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jonathanh@nvidia.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=mmaddireddy@nvidia.com \
    --cc=robh+dt@kernel.org \
    --cc=vidyas@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org linux-pci@archiver.kernel.org
	public-inbox-index linux-pci


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/ public-inbox