[PATCH v4] PCI: rockchip: Support property to specify the link capability

[PATCH v4] PCI: rockchip: Support property to specify the link capability

[Shawn Lin]

From: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

rk3399 supports PCIe 2.x link speeds marginally at best, and on some
boards, the link won't train at 5 GT/s at all. Rather than sacrifice 500
ms waiting for training that will never happen, let's add a property
from devicetree to specify link capability.

Signed-off-by: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

---

Changes in v4:
- define link_gen as u32
- elaborate more for rockchip,max-link-speed on doc

Changes in v3:
- Cast a warning for invalid max link speed and use gen1 for it.
  That looks better than v2. (Suggested by Brian)

Changes in v2:
- rename the property to rockchip,max-link-speed according to
  Bjorn's recommendation and take some bits from imx6q-pcie to
  make this requirement more consisent.

 .../devicetree/bindings/pci/rockchip-pcie.txt      |  4 ++
 drivers/pci/host/pcie-rockchip.c                   | 63 ++++++++++++++--------
 2 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
index ba67b39..9bb29de 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
@@ -42,6 +42,10 @@ Required properties:
 Optional Property:
 - ep-gpios: contain the entry for pre-reset gpio
 - num-lanes: number of lanes to use
+- rockchip,max-link-speed: Specify PCI gen for link capability. Must
+	be '2' for gen2, and '1' for gen1, otherwise will default to gen1.
+	For backward compatibility, if this property isn't assigned, we
+	use gen2 by default.
 - vpcie3v3-supply: The phandle to the 3.3v regulator to use for PCIe.
 - vpcie1v8-supply: The phandle to the 1.8v regulator to use for PCIe.
 - vpcie0v9-supply: The phandle to the 0.9v regulator to use for PCIe.
diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 35591b1..6dde999 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -53,6 +53,7 @@
 #define   PCIE_CLIENT_ARI_ENABLE	  HIWORD_UPDATE_BIT(0x0008)
 #define   PCIE_CLIENT_CONF_LANE_NUM(x)	  HIWORD_UPDATE(0x0030, ENCODE_LANES(x))
 #define   PCIE_CLIENT_MODE_RC		  HIWORD_UPDATE_BIT(0x0040)
+#define   PCIE_CLIENT_GEN_SEL_1		  HIWORD_UPDATE(0x0080, 0)
 #define   PCIE_CLIENT_GEN_SEL_2		  HIWORD_UPDATE_BIT(0x0080)
 #define PCIE_CLIENT_BASIC_STATUS1	(PCIE_CLIENT_BASE + 0x48)
 #define   PCIE_CLIENT_LINK_STATUS_UP		0x00300000
@@ -205,6 +206,7 @@ struct rockchip_pcie {
 	struct	gpio_desc *ep_gpio;
 	u32	lanes;
 	u8	root_bus_nr;
+	u32	link_gen;
 	struct	device *dev;
 	struct	irq_domain *irq_domain;
 };
@@ -443,14 +445,19 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 		return err;
 	}
 
+	if (rockchip->link_gen == 2)
+		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_2,
+				    PCIE_CLIENT_CONFIG);
+	else
+		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1,
+				    PCIE_CLIENT_CONFIG);
+
 	rockchip_pcie_write(rockchip,
 			    PCIE_CLIENT_CONF_ENABLE |
 			    PCIE_CLIENT_LINK_TRAIN_ENABLE |
 			    PCIE_CLIENT_ARI_ENABLE |
 			    PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes) |
-			    PCIE_CLIENT_MODE_RC |
-			    PCIE_CLIENT_GEN_SEL_2,
-				PCIE_CLIENT_CONFIG);
+			    PCIE_CLIENT_MODE_RC, PCIE_CLIENT_CONFIG);
 
 	err = phy_power_on(rockchip->phy);
 	if (err) {
@@ -550,29 +557,31 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 		msleep(20);
 	}
 
-	/*
-	 * Enable retrain for gen2. This should be configured only after
-	 * gen1 finished.
-	 */
-	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
-	status |= PCIE_RC_CONFIG_LCS_RETRAIN_LINK;
-	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
+	if (rockchip->link_gen == 2) {
+		/*
+		 * Enable retrain for gen2. This should be configured only after
+		 * gen1 finished.
+		 */
+		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
+		status |= PCIE_RC_CONFIG_LCS_RETRAIN_LINK;
+		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
+
+		timeout = jiffies + msecs_to_jiffies(500);
+		for (;;) {
+			status = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL);
+			if ((status & PCIE_CORE_PL_CONF_SPEED_MASK) ==
+					PCIE_CORE_PL_CONF_SPEED_5G) {
+				dev_dbg(dev, "PCIe link training gen2 pass!\n");
+				break;
+			}
 
-	timeout = jiffies + msecs_to_jiffies(500);
-	for (;;) {
-		status = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL);
-		if ((status & PCIE_CORE_PL_CONF_SPEED_MASK) ==
-		    PCIE_CORE_PL_CONF_SPEED_5G) {
-			dev_dbg(dev, "PCIe link training gen2 pass!\n");
-			break;
-		}
+			if (time_after(jiffies, timeout)) {
+				dev_dbg(dev, "PCIe link training gen2 timeout, fall back to gen1!\n");
+				break;
+			}
 
-		if (time_after(jiffies, timeout)) {
-			dev_dbg(dev, "PCIe link training gen2 timeout, fall back to gen1!\n");
-			break;
+			msleep(20);
 		}
-
-		msleep(20);
 	}
 
 	/* Check the final link width from negotiated lane counter from MGMT */
@@ -781,6 +790,14 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
 		rockchip->lanes = 1;
 	}
 
+	rockchip->link_gen = 2;
+	err = of_property_read_u32(node, "rockchip,max-link-speed",
+				   &rockchip->link_gen);
+	if (!err && rockchip->link_gen != 1 && rockchip->link_gen != 2) {
+		dev_warn(dev, "invalid max-link-speed, default to use gen1\n");
+		rockchip->link_gen = 1;
+	}
+
 	rockchip->core_rst = devm_reset_control_get(dev, "core");
 	if (IS_ERR(rockchip->core_rst)) {
 		if (PTR_ERR(rockchip->core_rst) != -EPROBE_DEFER)
-- 
2.3.7

[PATCH v4] PCI: rockchip: Support property to specify the link capability

[Shawn Lin]

From: Brian Norris <briannorris@chromium.org>

rk3399 supports PCIe 2.x link speeds marginally at best, and on some
boards, the link won't train at 5 GT/s at all. Rather than sacrifice 500
ms waiting for training that will never happen, let's add a property
from devicetree to specify link capability.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v4:
- define link_gen as u32
- elaborate more for rockchip,max-link-speed on doc

Changes in v3:
- Cast a warning for invalid max link speed and use gen1 for it.
  That looks better than v2. (Suggested by Brian)

Changes in v2:
- rename the property to rockchip,max-link-speed according to
  Bjorn's recommendation and take some bits from imx6q-pcie to
  make this requirement more consisent.

 .../devicetree/bindings/pci/rockchip-pcie.txt      |  4 ++
 drivers/pci/host/pcie-rockchip.c                   | 63 ++++++++++++++--------
 2 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
index ba67b39..9bb29de 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
@@ -42,6 +42,10 @@ Required properties:
 Optional Property:
 - ep-gpios: contain the entry for pre-reset gpio
 - num-lanes: number of lanes to use
+- rockchip,max-link-speed: Specify PCI gen for link capability. Must
+	be '2' for gen2, and '1' for gen1, otherwise will default to gen1.
+	For backward compatibility, if this property isn't assigned, we
+	use gen2 by default.
 - vpcie3v3-supply: The phandle to the 3.3v regulator to use for PCIe.
 - vpcie1v8-supply: The phandle to the 1.8v regulator to use for PCIe.
 - vpcie0v9-supply: The phandle to the 0.9v regulator to use for PCIe.
diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 35591b1..6dde999 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -53,6 +53,7 @@
 #define   PCIE_CLIENT_ARI_ENABLE	  HIWORD_UPDATE_BIT(0x0008)
 #define   PCIE_CLIENT_CONF_LANE_NUM(x)	  HIWORD_UPDATE(0x0030, ENCODE_LANES(x))
 #define   PCIE_CLIENT_MODE_RC		  HIWORD_UPDATE_BIT(0x0040)
+#define   PCIE_CLIENT_GEN_SEL_1		  HIWORD_UPDATE(0x0080, 0)
 #define   PCIE_CLIENT_GEN_SEL_2		  HIWORD_UPDATE_BIT(0x0080)
 #define PCIE_CLIENT_BASIC_STATUS1	(PCIE_CLIENT_BASE + 0x48)
 #define   PCIE_CLIENT_LINK_STATUS_UP		0x00300000
@@ -205,6 +206,7 @@ struct rockchip_pcie {
 	struct	gpio_desc *ep_gpio;
 	u32	lanes;
 	u8	root_bus_nr;
+	u32	link_gen;
 	struct	device *dev;
 	struct	irq_domain *irq_domain;
 };
@@ -443,14 +445,19 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 		return err;
 	}
 
+	if (rockchip->link_gen == 2)
+		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_2,
+				    PCIE_CLIENT_CONFIG);
+	else
+		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1,
+				    PCIE_CLIENT_CONFIG);
+
 	rockchip_pcie_write(rockchip,
 			    PCIE_CLIENT_CONF_ENABLE |
 			    PCIE_CLIENT_LINK_TRAIN_ENABLE |
 			    PCIE_CLIENT_ARI_ENABLE |
 			    PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes) |
-			    PCIE_CLIENT_MODE_RC |
-			    PCIE_CLIENT_GEN_SEL_2,
-				PCIE_CLIENT_CONFIG);
+			    PCIE_CLIENT_MODE_RC, PCIE_CLIENT_CONFIG);
 
 	err = phy_power_on(rockchip->phy);
 	if (err) {
@@ -550,29 +557,31 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 		msleep(20);
 	}
 
-	/*
-	 * Enable retrain for gen2. This should be configured only after
-	 * gen1 finished.
-	 */
-	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
-	status |= PCIE_RC_CONFIG_LCS_RETRAIN_LINK;
-	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
+	if (rockchip->link_gen == 2) {
+		/*
+		 * Enable retrain for gen2. This should be configured only after
+		 * gen1 finished.
+		 */
+		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
+		status |= PCIE_RC_CONFIG_LCS_RETRAIN_LINK;
+		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
+
+		timeout = jiffies + msecs_to_jiffies(500);
+		for (;;) {
+			status = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL);
+			if ((status & PCIE_CORE_PL_CONF_SPEED_MASK) ==
+					PCIE_CORE_PL_CONF_SPEED_5G) {
+				dev_dbg(dev, "PCIe link training gen2 pass!\n");
+				break;
+			}
 
-	timeout = jiffies + msecs_to_jiffies(500);
-	for (;;) {
-		status = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL);
-		if ((status & PCIE_CORE_PL_CONF_SPEED_MASK) ==
-		    PCIE_CORE_PL_CONF_SPEED_5G) {
-			dev_dbg(dev, "PCIe link training gen2 pass!\n");
-			break;
-		}
+			if (time_after(jiffies, timeout)) {
+				dev_dbg(dev, "PCIe link training gen2 timeout, fall back to gen1!\n");
+				break;
+			}
 
-		if (time_after(jiffies, timeout)) {
-			dev_dbg(dev, "PCIe link training gen2 timeout, fall back to gen1!\n");
-			break;
+			msleep(20);
 		}
-
-		msleep(20);
 	}
 
 	/* Check the final link width from negotiated lane counter from MGMT */
@@ -781,6 +790,14 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
 		rockchip->lanes = 1;
 	}
 
+	rockchip->link_gen = 2;
+	err = of_property_read_u32(node, "rockchip,max-link-speed",
+				   &rockchip->link_gen);
+	if (!err && rockchip->link_gen != 1 && rockchip->link_gen != 2) {
+		dev_warn(dev, "invalid max-link-speed, default to use gen1\n");
+		rockchip->link_gen = 1;
+	}
+
 	rockchip->core_rst = devm_reset_control_get(dev, "core");
 	if (IS_ERR(rockchip->core_rst)) {
 		if (PTR_ERR(rockchip->core_rst) != -EPROBE_DEFER)
-- 
2.3.7

Re: [PATCH v4] PCI: rockchip: Support property to specify the link capability

[Brian Norris]

On Thu, Oct 06, 2016 at 04:50:00PM +0800, Shawn Lin wrote:
> From: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> rk3399 supports PCIe 2.x link speeds marginally at best, and on some
> boards, the link won't train at 5 GT/s at all. Rather than sacrifice 500
> ms waiting for training that will never happen, let's add a property
> from devicetree to specify link capability.
> 
> Signed-off-by: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> 
> ---
> 
> Changes in v4:
> - define link_gen as u32
> - elaborate more for rockchip,max-link-speed on doc
> 
> Changes in v3:
> - Cast a warning for invalid max link speed and use gen1 for it.
>   That looks better than v2. (Suggested by Brian)
> 
> Changes in v2:
> - rename the property to rockchip,max-link-speed according to
>   Bjorn's recommendation and take some bits from imx6q-pcie to
>   make this requirement more consisent.
> 
>  .../devicetree/bindings/pci/rockchip-pcie.txt      |  4 ++
>  drivers/pci/host/pcie-rockchip.c                   | 63 ++++++++++++++--------
>  2 files changed, 44 insertions(+), 23 deletions(-)

Not sure if it means anything much, since I wrote half of this, but:

Reviewed-by: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] PCI: rockchip: Support property to specify the link capability

[Brian Norris]

On Thu, Oct 06, 2016 at 04:50:00PM +0800, Shawn Lin wrote:
> From: Brian Norris <briannorris@chromium.org>
> 
> rk3399 supports PCIe 2.x link speeds marginally at best, and on some
> boards, the link won't train at 5 GT/s at all. Rather than sacrifice 500
> ms waiting for training that will never happen, let's add a property
> from devicetree to specify link capability.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
> Changes in v4:
> - define link_gen as u32
> - elaborate more for rockchip,max-link-speed on doc
> 
> Changes in v3:
> - Cast a warning for invalid max link speed and use gen1 for it.
>   That looks better than v2. (Suggested by Brian)
> 
> Changes in v2:
> - rename the property to rockchip,max-link-speed according to
>   Bjorn's recommendation and take some bits from imx6q-pcie to
>   make this requirement more consisent.
> 
>  .../devicetree/bindings/pci/rockchip-pcie.txt      |  4 ++
>  drivers/pci/host/pcie-rockchip.c                   | 63 ++++++++++++++--------
>  2 files changed, 44 insertions(+), 23 deletions(-)

Not sure if it means anything much, since I wrote half of this, but:

Reviewed-by: Brian Norris <briannorris@chromium.org>

Re: [PATCH v4] PCI: rockchip: Support property to specify the link capability

[Rob Herring]

On Thu, Oct 06, 2016 at 04:50:00PM +0800, Shawn Lin wrote:
> From: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> rk3399 supports PCIe 2.x link speeds marginally at best, and on some
> boards, the link won't train at 5 GT/s at all. Rather than sacrifice 500
> ms waiting for training that will never happen, let's add a property
> from devicetree to specify link capability.
> 
> Signed-off-by: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> 
> ---
> 
> Changes in v4:
> - define link_gen as u32
> - elaborate more for rockchip,max-link-speed on doc
> 
> Changes in v3:
> - Cast a warning for invalid max link speed and use gen1 for it.
>   That looks better than v2. (Suggested by Brian)
> 
> Changes in v2:
> - rename the property to rockchip,max-link-speed according to
>   Bjorn's recommendation and take some bits from imx6q-pcie to
>   make this requirement more consisent.
> 
>  .../devicetree/bindings/pci/rockchip-pcie.txt      |  4 ++
>  drivers/pci/host/pcie-rockchip.c                   | 63 ++++++++++++++--------
>  2 files changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> index ba67b39..9bb29de 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> @@ -42,6 +42,10 @@ Required properties:
>  Optional Property:
>  - ep-gpios: contain the entry for pre-reset gpio
>  - num-lanes: number of lanes to use
> +- rockchip,max-link-speed: Specify PCI gen for link capability. Must
> +	be '2' for gen2, and '1' for gen1, otherwise will default to gen1.
> +	For backward compatibility, if this property isn't assigned, we
> +	use gen2 by default.

Defaults to gen1 or gen2?

Let's drop rockchip and make this a common property. 

>  - vpcie3v3-supply: The phandle to the 3.3v regulator to use for PCIe.
>  - vpcie1v8-supply: The phandle to the 1.8v regulator to use for PCIe.
>  - vpcie0v9-supply: The phandle to the 0.9v regulator to use for PCIe.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] PCI: rockchip: Support property to specify the link capability

[Rob Herring]

On Thu, Oct 06, 2016 at 04:50:00PM +0800, Shawn Lin wrote:
> From: Brian Norris <briannorris@chromium.org>
> 
> rk3399 supports PCIe 2.x link speeds marginally at best, and on some
> boards, the link won't train at 5 GT/s at all. Rather than sacrifice 500
> ms waiting for training that will never happen, let's add a property
> from devicetree to specify link capability.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
> Changes in v4:
> - define link_gen as u32
> - elaborate more for rockchip,max-link-speed on doc
> 
> Changes in v3:
> - Cast a warning for invalid max link speed and use gen1 for it.
>   That looks better than v2. (Suggested by Brian)
> 
> Changes in v2:
> - rename the property to rockchip,max-link-speed according to
>   Bjorn's recommendation and take some bits from imx6q-pcie to
>   make this requirement more consisent.
> 
>  .../devicetree/bindings/pci/rockchip-pcie.txt      |  4 ++
>  drivers/pci/host/pcie-rockchip.c                   | 63 ++++++++++++++--------
>  2 files changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> index ba67b39..9bb29de 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> @@ -42,6 +42,10 @@ Required properties:
>  Optional Property:
>  - ep-gpios: contain the entry for pre-reset gpio
>  - num-lanes: number of lanes to use
> +- rockchip,max-link-speed: Specify PCI gen for link capability. Must
> +	be '2' for gen2, and '1' for gen1, otherwise will default to gen1.
> +	For backward compatibility, if this property isn't assigned, we
> +	use gen2 by default.

Defaults to gen1 or gen2?

Let's drop rockchip and make this a common property. 

>  - vpcie3v3-supply: The phandle to the 3.3v regulator to use for PCIe.
>  - vpcie1v8-supply: The phandle to the 1.8v regulator to use for PCIe.
>  - vpcie0v9-supply: The phandle to the 0.9v regulator to use for PCIe.

Re: [PATCH v4] PCI: rockchip: Support property to specify the link capability

[Brian Norris]

Hi Rob,

On Mon, Oct 10, 2016 at 09:16:39AM -0500, Rob Herring wrote:
> On Thu, Oct 06, 2016 at 04:50:00PM +0800, Shawn Lin wrote:
> > From: Brian Norris <briannorris@chromium.org>
> > 
> > rk3399 supports PCIe 2.x link speeds marginally at best, and on some
> > boards, the link won't train at 5 GT/s at all. Rather than sacrifice 500
> > ms waiting for training that will never happen, let's add a property
> > from devicetree to specify link capability.
> > 
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > 
> > ---
> > 
> > Changes in v4:
> > - define link_gen as u32
> > - elaborate more for rockchip,max-link-speed on doc
> > 
> > Changes in v3:
> > - Cast a warning for invalid max link speed and use gen1 for it.
> >   That looks better than v2. (Suggested by Brian)
> > 
> > Changes in v2:
> > - rename the property to rockchip,max-link-speed according to
> >   Bjorn's recommendation and take some bits from imx6q-pcie to
> >   make this requirement more consisent.
> > 
> >  .../devicetree/bindings/pci/rockchip-pcie.txt      |  4 ++
> >  drivers/pci/host/pcie-rockchip.c                   | 63 ++++++++++++++--------
> >  2 files changed, 44 insertions(+), 23 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > index ba67b39..9bb29de 100644
> > --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > @@ -42,6 +42,10 @@ Required properties:
> >  Optional Property:
> >  - ep-gpios: contain the entry for pre-reset gpio
> >  - num-lanes: number of lanes to use
> > +- rockchip,max-link-speed: Specify PCI gen for link capability. Must
> > +	be '2' for gen2, and '1' for gen1, otherwise will default to gen1.
> > +	For backward compatibility, if this property isn't assigned, we
> > +	use gen2 by default.
> 
> Defaults to gen1 or gen2?

The wording is a bit confusing, but I think Shawn intended for:
(a) if no property is provided, default to the maximum supported (i.e.,
    gen2), to keep backward compatibility
(b) if a property is provided, but it doesn't contain 1 or 2, just
    default to 1

I already disagreed with (b) (and suggested we make this an error
instead; we have no business accepting malformed device trees here IMO).
Alternatives to clear up the confusion Rob pointed out might include:
(1) explaining this better (i.e., what does "otherwise" mean in the
    original sentence?)
(2) changing this to uniformly default to 2 (if someone used an
    unsupported value, treat it as if the property wasn't there at all)
(3) reject incorrect values outright (so we don't have this fuzzy middle
    ground at all)

> Let's drop rockchip and make this a common property. 

As in, handle this in some generic/common way? AFAICT, there's really
no way to do that given the current structure of PCIe handling (and the
nature of these kinds of limitations).

Or, do you just mean make the name common? (i.e,. "max-link-speed"
instead of "rockchip,max-link-speed")

Also for the record, Shawn already sent v5 before your comments.
Presumably he'll need a v6 then...

Brian

> >  - vpcie3v3-supply: The phandle to the 3.3v regulator to use for PCIe.
> >  - vpcie1v8-supply: The phandle to the 1.8v regulator to use for PCIe.
> >  - vpcie0v9-supply: The phandle to the 0.9v regulator to use for PCIe.

Re: [PATCH v4] PCI: rockchip: Support property to specify the link capability

[Rob Herring]

On Mon, Oct 10, 2016 at 12:20 PM, Brian Norris <briannorris@chromium.org> wrote:
> Hi Rob,
>
> On Mon, Oct 10, 2016 at 09:16:39AM -0500, Rob Herring wrote:
>> On Thu, Oct 06, 2016 at 04:50:00PM +0800, Shawn Lin wrote:
>> > From: Brian Norris <briannorris@chromium.org>
>> >
>> > rk3399 supports PCIe 2.x link speeds marginally at best, and on some
>> > boards, the link won't train at 5 GT/s at all. Rather than sacrifice 500
>> > ms waiting for training that will never happen, let's add a property
>> > from devicetree to specify link capability.
>> >
>> > Signed-off-by: Brian Norris <briannorris@chromium.org>
>> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> >
>> > ---
>> >
>> > Changes in v4:
>> > - define link_gen as u32
>> > - elaborate more for rockchip,max-link-speed on doc
>> >
>> > Changes in v3:
>> > - Cast a warning for invalid max link speed and use gen1 for it.
>> >   That looks better than v2. (Suggested by Brian)
>> >
>> > Changes in v2:
>> > - rename the property to rockchip,max-link-speed according to
>> >   Bjorn's recommendation and take some bits from imx6q-pcie to
>> >   make this requirement more consisent.
>> >
>> >  .../devicetree/bindings/pci/rockchip-pcie.txt      |  4 ++
>> >  drivers/pci/host/pcie-rockchip.c                   | 63 ++++++++++++++--------
>> >  2 files changed, 44 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> > index ba67b39..9bb29de 100644
>> > --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> > +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> > @@ -42,6 +42,10 @@ Required properties:
>> >  Optional Property:
>> >  - ep-gpios: contain the entry for pre-reset gpio
>> >  - num-lanes: number of lanes to use
>> > +- rockchip,max-link-speed: Specify PCI gen for link capability. Must
>> > +   be '2' for gen2, and '1' for gen1, otherwise will default to gen1.
>> > +   For backward compatibility, if this property isn't assigned, we
>> > +   use gen2 by default.
>>
>> Defaults to gen1 or gen2?
>
> The wording is a bit confusing, but I think Shawn intended for:
> (a) if no property is provided, default to the maximum supported (i.e.,
>     gen2), to keep backward compatibility
> (b) if a property is provided, but it doesn't contain 1 or 2, just
>     default to 1
>
> I already disagreed with (b) (and suggested we make this an error
> instead; we have no business accepting malformed device trees here IMO).
> Alternatives to clear up the confusion Rob pointed out might include:
> (1) explaining this better (i.e., what does "otherwise" mean in the
>     original sentence?)
> (2) changing this to uniformly default to 2 (if someone used an
>     unsupported value, treat it as if the property wasn't there at all)
> (3) reject incorrect values outright (so we don't have this fuzzy middle
>     ground at all)

Sounds good.

>> Let's drop rockchip and make this a common property.
>
> As in, handle this in some generic/common way? AFAICT, there's really
> no way to do that given the current structure of PCIe handling (and the
> nature of these kinds of limitations).
>
> Or, do you just mean make the name common? (i.e,. "max-link-speed"
> instead of "rockchip,max-link-speed")

Right, just the name and document in a common spot. Could be a simple
helper function that drivers can call as well.

Rob