[PATCH v7 0/5] Add support to handle ZRX-DC Compliant PHYs

[PATCH v7 0/5] Add support to handle ZRX-DC Compliant PHYs

[Shradha Todi]

According the PCI Express base specification when PHY does not meet ZRX-DC
specification, after every 100ms timeout the link should transition to
recovery state when the link is in low power states. 

Ports that meet the ZRX-DC specification for 2.5 GT/s while in the L1.Idle
state and are therefore not required to implement the 100 ms timeout and
transition to Recovery should avoid implementing it, since it will reduce
the power savings expected from the L1 state.

DesignWare controller provides GEN3_ZRXDC_NONCOMPL field in
GEN3_RELATED_OFF to specify about ZRX-DC compliant PHY.

We need to get the PHY property in controller driver. So, we are proposing
a new method phy_property_present() in the phy driver.

PCIe controller platform drivers should populate the phy_zrxdc_compliant
flag, which will be used by generic DesignWare driver.

pci->phy_zrxdc_compliant = phy_property_present(xxxx_ctrl->phy, "phy-zrxdc-compliant");

Patchset v2 can be found at:
- 1/2: https://lkml.org/lkml/2019/11/11/672
- 2/2: https://lkml.org/lkml/2019/10/28/285

Changes w.r.t v2:
- Addressed review comments
- Rebased on latest linus/master

Changes w.r.t v3:
- Added linux-pci@xxxxxxxxxxxxxxx as pointed by Gustavo, Sorry for annoying.

Changes w.r.t v4:
- Addressed review comments from Andrew Murray
- Rebased on latest linus/master

Changes w.r.t v5:
- Added check for NULL pointer

Changes w.r.t v6:
- Rebased on latest linus/master
- Used this feature in nvidia tegra files

Pankaj Dubey (3):
  phy: core: add phy_property_present method
  PCI: dwc: add support to handle ZRX-DC Compliant PHYs
  PCI: tegra: Remove platform driver support for ZRX-DC compliant PHY

Shradha Todi (2):
  dt-bindings: PHY: P2U: Add binding for ZRX-DC PHY property
  arm64: tegra: Add support for ZRX DC PHY property

 .../devicetree/bindings/phy/phy-tegra194-p2u.txt     |  4 ++++
 arch/arm64/boot/dts/nvidia/tegra194.dtsi             | 20 ++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.c         |  6 ++++++
 drivers/pci/controller/dwc/pcie-designware.h         |  4 ++++
 drivers/pci/controller/dwc/pcie-tegra194.c           | 17 ++++++++---------
 drivers/phy/phy-core.c                               | 20 ++++++++++++++++++++
 include/linux/phy/phy.h                              |  6 ++++++
 7 files changed, 68 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH v7 1/5] phy: core: add phy_property_present method

[Shradha Todi]

From: Pankaj Dubey <pankaj.dubey@samsung.com>

In some platforms, we need information of phy properties in the controller
drivers. This patch adds a new phy_property_present() method which can be
used to check if some property exists in PHY or not.

In case of DesignWare PCIe controller, we need to write into controller
register to specify about ZRX-DC compliance property of the PHY, which
reduces the power consumption during lower power states.

Signed-off-by: Anvesh Salveru <anvesh.salveru@gmail.com>
Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Shradha Todi <shradha.t@samsung.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>
Cc: Vinod Koul <vkoul@kernel.org>
---
 drivers/phy/phy-core.c  | 20 ++++++++++++++++++++
 include/linux/phy/phy.h |  6 ++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 71cb108..e4ecd41 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -420,6 +420,26 @@ int phy_calibrate(struct phy *phy)
 EXPORT_SYMBOL_GPL(phy_calibrate);
 
 /**
+ * phy_property_present() - checks if the property is present in PHY
+ * @phy: the phy returned by phy_get()
+ * @property: name of the property to check
+ *
+ * Used to check if the given property is present in PHY.
+ * Searches for the given property in the phy device tree
+ * node.
+ *
+ * Returns: true if property exists, false otherwise
+ */
+bool phy_property_present(struct phy *phy, const char *property)
+{
+	if (!phy)
+		return false;
+
+	return of_property_read_bool(phy->dev.of_node, property);
+}
+EXPORT_SYMBOL_GPL(phy_property_present);
+
+/**
  * phy_configure() - Changes the phy parameters
  * @phy: the phy returned by phy_get()
  * @opts: New configuration to apply
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index e435bdb..cdecb07 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -225,6 +225,7 @@ static inline enum phy_mode phy_get_mode(struct phy *phy)
 }
 int phy_reset(struct phy *phy);
 int phy_calibrate(struct phy *phy);
+bool phy_property_present(struct phy *phy, const char *property);
 static inline int phy_get_bus_width(struct phy *phy)
 {
 	return phy->attrs.bus_width;
@@ -363,6 +364,11 @@ static inline int phy_calibrate(struct phy *phy)
 	return -ENOSYS;
 }
 
+static inline bool phy_property_present(struct phy *phy, const char *property)
+{
+	return false;
+}
+
 static inline int phy_configure(struct phy *phy,
 				union phy_configure_opts *opts)
 {
-- 
2.7.4

[PATCH v7 2/5] PCI: dwc: add support to handle ZRX-DC Compliant PHYs

[Shradha Todi]

From: Pankaj Dubey <pankaj.dubey@samsung.com>

Many platforms use DesignWare controller but the PHY can be different in
different platforms. If the PHY is compliant is to ZRX-DC specification it
helps in low power consumption during power states.

If current data rate is 8.0 GT/s or higher and PHY is not compliant to
ZRX-DC specification, then after every 100ms link should transition to
recovery state during the low power states.

DesignWare controller provides GEN3_ZRXDC_NONCOMPL field in
GEN3_RELATED_OFF to specify about ZRX-DC compliant PHY.

Platforms with ZRX-DC compliant PHY can set phy_zrxdc_compliant variable to
specify this property to the controller.

Signed-off-by: Anvesh Salveru <anvesh.salveru@gmail.com>
Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Shradha Todi <shradha.t@samsung.com>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/dwc/pcie-designware.c | 6 ++++++
 drivers/pci/controller/dwc/pcie-designware.h | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 645fa18..74590c7 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -722,4 +722,10 @@ void dw_pcie_setup(struct dw_pcie *pci)
 		       PCIE_PL_CHK_REG_CHK_REG_START;
 		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
 	}
+
+	if (pci->phy_zrxdc_compliant) {
+		val = dw_pcie_readl_dbi(pci, PCIE_PORT_GEN3_RELATED);
+		val &= ~PORT_LOGIC_GEN3_ZRXDC_NONCOMPL;
+		dw_pcie_writel_dbi(pci, PCIE_PORT_GEN3_RELATED, val);
+	}
 }
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 0207840..8b905a2 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -74,6 +74,9 @@
 #define PCIE_MSI_INTR0_MASK		0x82C
 #define PCIE_MSI_INTR0_STATUS		0x830
 
+#define PCIE_PORT_GEN3_RELATED		0x890
+#define PORT_LOGIC_GEN3_ZRXDC_NONCOMPL	BIT(0)
+
 #define PCIE_PORT_MULTI_LANE_CTRL	0x8C0
 #define PORT_MLTI_UPCFG_SUPPORT		BIT(7)
 
@@ -273,6 +276,7 @@ struct dw_pcie {
 	u8			n_fts[2];
 	bool			iatu_unroll_enabled: 1;
 	bool			io_cfg_atu_shared: 1;
+	bool			phy_zrxdc_compliant;
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
-- 
2.7.4

[PATCH v7 3/5] dt-bindings: PHY: P2U: Add binding for ZRX-DC PHY property

[Shradha Todi]

Add support for ZRX-DC compliant PHYs. If PHY is not compliant to ZRX-DC
specification, then after every 100ms link should transition to recovery
state during the low power states which increases power consumption.

Platforms with ZRX-DC compliant PHY can use "phy-zrxdc-compliant" property
in PCIe PHY DT node.

Signed-off-by: Anvesh Salveru <anvesh.salveru@gmail.com>
Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Shradha Todi <shradha.t@samsung.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Kishon Vijay Abraham I <kishon@ti.com>
Cc: Vidya Sagar <vidyas@nvidia.com>
---
 Documentation/devicetree/bindings/phy/phy-tegra194-p2u.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/phy-tegra194-p2u.txt b/Documentation/devicetree/bindings/phy/phy-tegra194-p2u.txt
index d23ff90..73f2fa0 100644
--- a/Documentation/devicetree/bindings/phy/phy-tegra194-p2u.txt
+++ b/Documentation/devicetree/bindings/phy/phy-tegra194-p2u.txt
@@ -15,6 +15,10 @@ Required properties:
 Required properties for PHY port node:
 - #phy-cells: Defined by generic PHY bindings.  Must be 0.
 
+Optional properties for other PHY features:
+- phy-zrxdc-compliant: This property is needed if phy complies with the
+		       ZRX-DC specification.
+
 Refer to phy/phy-bindings.txt for the generic PHY binding properties.
 
 Example:
-- 
2.7.4

[PATCH v7 4/5] PCI: tegra: Remove platform driver support for ZRX-DC compliant PHY

[Shradha Todi]

From: Pankaj Dubey <pankaj.dubey@samsung.com>

As part of dw_pcie_setup(), PHYs which are compliant to ZRX-DC
specification are already handled based on "phy-zrxdc-compliant" property
in PCIe PHY DT node. So, instead of handling ZRX-DC compliant settings in
each platform driver, remove this driver specific code.

Signed-off-by: Anvesh Salveru <anvesh.salveru@gmail.com>
Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Shradha Todi <shradha.t@samsung.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Andrew Murray <andrew.murray@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Vidya Sagar <vidyas@nvidia.com>
Cc: Jonathan Hunter <jonathanh@nvidia.com>
---
 drivers/pci/controller/dwc/pcie-tegra194.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 6fa216e..50e85e5 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -194,7 +194,6 @@
 #define GEN3_EQ_CONTROL_OFF_FB_MODE_MASK	GENMASK(3, 0)
 
 #define GEN3_RELATED_OFF			0x890
-#define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
 #define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
 #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
 #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
@@ -899,10 +898,6 @@ static int tegra_pcie_dw_host_init(struct pcie_port *pp)
 		disable_aspm_l12(pcie);
 	}
 
-	val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
-	val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
-	dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
-
 	if (pcie->update_fc_fixup) {
 		val = dw_pcie_readl_dbi(pci, CFG_TIMER_CTRL_MAX_FUNC_NUM_OFF);
 		val |= 0x1 << CFG_TIMER_CTRL_ACK_NAK_SHIFT;
@@ -1752,10 +1747,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
 		disable_aspm_l12(pcie);
 	}
 
-	val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
-	val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
-	dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
-
 	pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci,
 						      PCI_CAP_ID_EXP);
 	clk_set_rate(pcie->core_clk, GEN4_CORE_CLK_FREQ);
@@ -1958,6 +1949,7 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
 {
 	const struct tegra_pcie_dw_of_data *data;
 	struct device *dev = &pdev->dev;
+	unsigned int phy_zrxdc_count;
 	struct resource *atu_dma_res;
 	struct tegra_pcie_dw *pcie;
 	struct pcie_port *pp;
@@ -2066,8 +2058,15 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
 				dev_err(dev, "Failed to get PHY: %d\n", ret);
 			return ret;
 		}
+		if (phy_property_present(phys[i], "phy-zrxdc-compliant"))
+			phy_zrxdc_count++;
 	}
 
+	if ((pcie->phy_count) && (pcie->phy_count == phy_zrxdc_count))
+		pci->phy_zrxdc_compliant = true;
+	else
+		pci->phy_zrxdc_compliant = false;
+
 	pcie->phys = phys;
 
 	atu_dma_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-- 
2.7.4

[PATCH v7 5/5] arm64: tegra: Add support for ZRX DC PHY property

[Shradha Todi]

DesignWare controller driver provides the support to handle the PHYs which
are compliant to ZRX-DC specification based on "phy-zrxdc-compliant" DT
property. So, add "phy-zrxdc-compliant" property in tegra PCIe PHY DT
nodes.

Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Shradha Todi <shradha.t@samsung.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Jonathan Hunter <jonathanh@nvidia.com>
Cc: Vidya Sagar <vidyas@nvidia.com>
To: devicetree@vger.kernel.org
To: linux-tegra@vger.kernel.org
---
 arch/arm64/boot/dts/nvidia/tegra194.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index 25f36d6..9d91006 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -1006,6 +1006,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_hsio_1: phy@3e20000 {
@@ -1014,6 +1015,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_hsio_2: phy@3e30000 {
@@ -1022,6 +1024,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_hsio_3: phy@3e40000 {
@@ -1030,6 +1033,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_hsio_4: phy@3e50000 {
@@ -1038,6 +1042,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_hsio_5: phy@3e60000 {
@@ -1046,6 +1051,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_hsio_6: phy@3e70000 {
@@ -1054,6 +1060,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_hsio_7: phy@3e80000 {
@@ -1062,6 +1069,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_hsio_8: phy@3e90000 {
@@ -1070,6 +1078,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_hsio_9: phy@3ea0000 {
@@ -1078,6 +1087,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_nvhs_0: phy@3eb0000 {
@@ -1086,6 +1096,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_nvhs_1: phy@3ec0000 {
@@ -1094,6 +1105,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_nvhs_2: phy@3ed0000 {
@@ -1102,6 +1114,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_nvhs_3: phy@3ee0000 {
@@ -1110,6 +1123,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_nvhs_4: phy@3ef0000 {
@@ -1118,6 +1132,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_nvhs_5: phy@3f00000 {
@@ -1126,6 +1141,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_nvhs_6: phy@3f10000 {
@@ -1134,6 +1150,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_nvhs_7: phy@3f20000 {
@@ -1142,6 +1159,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_hsio_10: phy@3f30000 {
@@ -1150,6 +1168,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		p2u_hsio_11: phy@3f40000 {
@@ -1158,6 +1177,7 @@
 			reg-names = "ctl";
 
 			#phy-cells = <0>;
+			phy-zrxdc-compliant;
 		};
 
 		hsp_aon: hsp@c150000 {
-- 
2.7.4

Re: [PATCH v7 2/5] PCI: dwc: add support to handle ZRX-DC Compliant PHYs

[Bjorn Helgaas]

Capitalize subject to match the rest of the series.

"Add support to handle .." is redundant; "Add support for ..." would
be equivalent and shorter.

But this patch actually doesn't add anything at all by itself, since
it checks pci->phy_zrxdc_compliant but never sets it.

On Thu, Jan 07, 2021 at 08:58:40PM +0530, Shradha Todi wrote:
> From: Pankaj Dubey <pankaj.dubey@samsung.com>
> 
> Many platforms use DesignWare controller but the PHY can be different in
> different platforms. If the PHY is compliant is to ZRX-DC specification it
> helps in low power consumption during power states.

s/is to/to/

Even with that, this sentence doesn't quite parse correctly.  Do you
mean something like this?

  If the PHY is compliant to the ZRX-DC specification, it reduces
  power consumption in low power Link states.

(I assume this is related to Link power states (L0, L1, etc), not
device power states (D0, D3hot, etc)).

> If current data rate is 8.0 GT/s or higher and PHY is not compliant to
> ZRX-DC specification, then after every 100ms link should transition to
> recovery state during the low power states.

Not sure this makes sense.  If the Link is in a low power state for 10
seconds, it must transition to the Recovery state every 100ms during
that 10 seconds, i.e., 100 times?

> DesignWare controller provides GEN3_ZRXDC_NONCOMPL field in
> GEN3_RELATED_OFF to specify about ZRX-DC compliant PHY.
> 
> Platforms with ZRX-DC compliant PHY can set phy_zrxdc_compliant variable to
> specify this property to the controller.

If this is a DesignWare-generic register and the "phy-zrxdc-compliant"
property can be used by any DesignWare-based driver, why isn't the
code to look for it in the DesignWare-generic part?

Is there a link to the ZRX-DC specification you can mention somewhere
in this series?

> Signed-off-by: Anvesh Salveru <anvesh.salveru@gmail.com>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 6 ++++++
>  drivers/pci/controller/dwc/pcie-designware.h | 4 ++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 645fa18..74590c7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -722,4 +722,10 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  		       PCIE_PL_CHK_REG_CHK_REG_START;
>  		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
>  	}
> +
> +	if (pci->phy_zrxdc_compliant) {
> +		val = dw_pcie_readl_dbi(pci, PCIE_PORT_GEN3_RELATED);
> +		val &= ~PORT_LOGIC_GEN3_ZRXDC_NONCOMPL;
> +		dw_pcie_writel_dbi(pci, PCIE_PORT_GEN3_RELATED, val);
> +	}
>  }
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 0207840..8b905a2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -74,6 +74,9 @@
>  #define PCIE_MSI_INTR0_MASK		0x82C
>  #define PCIE_MSI_INTR0_STATUS		0x830
>  
> +#define PCIE_PORT_GEN3_RELATED		0x890
> +#define PORT_LOGIC_GEN3_ZRXDC_NONCOMPL	BIT(0)
> +
>  #define PCIE_PORT_MULTI_LANE_CTRL	0x8C0
>  #define PORT_MLTI_UPCFG_SUPPORT		BIT(7)
>  
> @@ -273,6 +276,7 @@ struct dw_pcie {
>  	u8			n_fts[2];
>  	bool			iatu_unroll_enabled: 1;
>  	bool			io_cfg_atu_shared: 1;
> +	bool			phy_zrxdc_compliant;

I raise my eyebrows a little at "bool xx : 1".  I think it's probably
*correct*, but "unsigned int xx : 1" is the overwhelming favorite and
I doubt bool gives any advantage.

  $ git grep -E "int\s+\S+\s*:\s*1" | egrep "^\S*\.[ch]" | wc -l
  3129
  $ git grep -E "bool\s+\S+\s*:\s*1" | egrep "^\S*\.[ch]" | wc -l
  637

pcie-designware.h is the only user in drivers/pci.  But you're
following the existing style in the file, which is good.

>  };
>  
>  #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
> -- 
> 2.7.4
> 

Re: [PATCH v7 4/5] PCI: tegra: Remove platform driver support for ZRX-DC compliant PHY

[kernel test robot]

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

Hi Shradha,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pci/next]
[also build test WARNING on linus/master v5.11-rc2 next-20210104]
[cannot apply to robh/for-next linux/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Shradha-Todi/Add-support-to-handle-ZRX-DC-Compliant-PHYs/20210108-001626
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-a005-20210107 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 5c951623bc8965fa1e89660f2f5f4a2944e4981a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/a626b81436de9ab5c0c0bc9785fbc64549ea0f3a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Shradha-Todi/Add-support-to-handle-ZRX-DC-Compliant-PHYs/20210108-001626
        git checkout a626b81436de9ab5c0c0bc9785fbc64549ea0f3a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/pci/controller/dwc/pcie-tegra194.c:2063:4: warning: variable 'phy_zrxdc_count' is uninitialized when used here [-Wuninitialized]
                           phy_zrxdc_count++;
                           ^~~~~~~~~~~~~~~
   drivers/pci/controller/dwc/pcie-tegra194.c:1953:30: note: initialize the variable 'phy_zrxdc_count' to silence this warning
           unsigned int phy_zrxdc_count;
                                       ^
                                        = 0
   1 warning generated.


vim +/phy_zrxdc_count +2063 drivers/pci/controller/dwc/pcie-tegra194.c

  1948	
  1949	static int tegra_pcie_dw_probe(struct platform_device *pdev)
  1950	{
  1951		const struct tegra_pcie_dw_of_data *data;
  1952		struct device *dev = &pdev->dev;
  1953		unsigned int phy_zrxdc_count;
  1954		struct resource *atu_dma_res;
  1955		struct tegra_pcie_dw *pcie;
  1956		struct pcie_port *pp;
  1957		struct dw_pcie *pci;
  1958		struct phy **phys;
  1959		char *name;
  1960		int ret;
  1961		u32 i;
  1962	
  1963		data = of_device_get_match_data(dev);
  1964	
  1965		pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
  1966		if (!pcie)
  1967			return -ENOMEM;
  1968	
  1969		pci = &pcie->pci;
  1970		pci->dev = &pdev->dev;
  1971		pci->ops = &tegra_dw_pcie_ops;
  1972		pci->n_fts[0] = N_FTS_VAL;
  1973		pci->n_fts[1] = FTS_VAL;
  1974		pci->version = 0x490A;
  1975	
  1976		pp = &pci->pp;
  1977		pp->num_vectors = MAX_MSI_IRQS;
  1978		pcie->dev = &pdev->dev;
  1979		pcie->mode = (enum dw_pcie_device_mode)data->mode;
  1980	
  1981		ret = tegra_pcie_dw_parse_dt(pcie);
  1982		if (ret < 0) {
  1983			const char *level = KERN_ERR;
  1984	
  1985			if (ret == -EPROBE_DEFER)
  1986				level = KERN_DEBUG;
  1987	
  1988			dev_printk(level, dev,
  1989				   dev_fmt("Failed to parse device tree: %d\n"),
  1990				   ret);
  1991			return ret;
  1992		}
  1993	
  1994		ret = tegra_pcie_get_slot_regulators(pcie);
  1995		if (ret < 0) {
  1996			const char *level = KERN_ERR;
  1997	
  1998			if (ret == -EPROBE_DEFER)
  1999				level = KERN_DEBUG;
  2000	
  2001			dev_printk(level, dev,
  2002				   dev_fmt("Failed to get slot regulators: %d\n"),
  2003				   ret);
  2004			return ret;
  2005		}
  2006	
  2007		if (pcie->pex_refclk_sel_gpiod)
  2008			gpiod_set_value(pcie->pex_refclk_sel_gpiod, 1);
  2009	
  2010		pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl");
  2011		if (IS_ERR(pcie->pex_ctl_supply)) {
  2012			ret = PTR_ERR(pcie->pex_ctl_supply);
  2013			if (ret != -EPROBE_DEFER)
  2014				dev_err(dev, "Failed to get regulator: %ld\n",
  2015					PTR_ERR(pcie->pex_ctl_supply));
  2016			return ret;
  2017		}
  2018	
  2019		pcie->core_clk = devm_clk_get(dev, "core");
  2020		if (IS_ERR(pcie->core_clk)) {
  2021			dev_err(dev, "Failed to get core clock: %ld\n",
  2022				PTR_ERR(pcie->core_clk));
  2023			return PTR_ERR(pcie->core_clk);
  2024		}
  2025	
  2026		pcie->appl_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
  2027							      "appl");
  2028		if (!pcie->appl_res) {
  2029			dev_err(dev, "Failed to find \"appl\" region\n");
  2030			return -ENODEV;
  2031		}
  2032	
  2033		pcie->appl_base = devm_ioremap_resource(dev, pcie->appl_res);
  2034		if (IS_ERR(pcie->appl_base))
  2035			return PTR_ERR(pcie->appl_base);
  2036	
  2037		pcie->core_apb_rst = devm_reset_control_get(dev, "apb");
  2038		if (IS_ERR(pcie->core_apb_rst)) {
  2039			dev_err(dev, "Failed to get APB reset: %ld\n",
  2040				PTR_ERR(pcie->core_apb_rst));
  2041			return PTR_ERR(pcie->core_apb_rst);
  2042		}
  2043	
  2044		phys = devm_kcalloc(dev, pcie->phy_count, sizeof(*phys), GFP_KERNEL);
  2045		if (!phys)
  2046			return -ENOMEM;
  2047	
  2048		for (i = 0; i < pcie->phy_count; i++) {
  2049			name = kasprintf(GFP_KERNEL, "p2u-%u", i);
  2050			if (!name) {
  2051				dev_err(dev, "Failed to create P2U string\n");
  2052				return -ENOMEM;
  2053			}
  2054			phys[i] = devm_phy_get(dev, name);
  2055			kfree(name);
  2056			if (IS_ERR(phys[i])) {
  2057				ret = PTR_ERR(phys[i]);
  2058				if (ret != -EPROBE_DEFER)
  2059					dev_err(dev, "Failed to get PHY: %d\n", ret);
  2060				return ret;
  2061			}
  2062			if (phy_property_present(phys[i], "phy-zrxdc-compliant"))
> 2063				phy_zrxdc_count++;
  2064		}
  2065	
  2066		if ((pcie->phy_count) && (pcie->phy_count == phy_zrxdc_count))
  2067			pci->phy_zrxdc_compliant = true;
  2068		else
  2069			pci->phy_zrxdc_compliant = false;
  2070	
  2071		pcie->phys = phys;
  2072	
  2073		atu_dma_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
  2074							   "atu_dma");
  2075		if (!atu_dma_res) {
  2076			dev_err(dev, "Failed to find \"atu_dma\" region\n");
  2077			return -ENODEV;
  2078		}
  2079		pcie->atu_dma_res = atu_dma_res;
  2080	
  2081		pci->atu_size = resource_size(atu_dma_res);
  2082		pci->atu_base = devm_ioremap_resource(dev, atu_dma_res);
  2083		if (IS_ERR(pci->atu_base))
  2084			return PTR_ERR(pci->atu_base);
  2085	
  2086		pcie->core_rst = devm_reset_control_get(dev, "core");
  2087		if (IS_ERR(pcie->core_rst)) {
  2088			dev_err(dev, "Failed to get core reset: %ld\n",
  2089				PTR_ERR(pcie->core_rst));
  2090			return PTR_ERR(pcie->core_rst);
  2091		}
  2092	
  2093		pp->irq = platform_get_irq_byname(pdev, "intr");
  2094		if (pp->irq < 0)
  2095			return pp->irq;
  2096	
  2097		pcie->bpmp = tegra_bpmp_get(dev);
  2098		if (IS_ERR(pcie->bpmp))
  2099			return PTR_ERR(pcie->bpmp);
  2100	
  2101		platform_set_drvdata(pdev, pcie);
  2102	
  2103		switch (pcie->mode) {
  2104		case DW_PCIE_RC_TYPE:
  2105			ret = devm_request_irq(dev, pp->irq, tegra_pcie_rp_irq_handler,
  2106					       IRQF_SHARED, "tegra-pcie-intr", pcie);
  2107			if (ret) {
  2108				dev_err(dev, "Failed to request IRQ %d: %d\n", pp->irq,
  2109					ret);
  2110				goto fail;
  2111			}
  2112	
  2113			ret = tegra_pcie_config_rp(pcie);
  2114			if (ret && ret != -ENOMEDIUM)
  2115				goto fail;
  2116			else
  2117				return 0;
  2118			break;
  2119	
  2120		case DW_PCIE_EP_TYPE:
  2121			ret = devm_request_threaded_irq(dev, pp->irq,
  2122							tegra_pcie_ep_hard_irq,
  2123							tegra_pcie_ep_irq_thread,
  2124							IRQF_SHARED | IRQF_ONESHOT,
  2125							"tegra-pcie-ep-intr", pcie);
  2126			if (ret) {
  2127				dev_err(dev, "Failed to request IRQ %d: %d\n", pp->irq,
  2128					ret);
  2129				goto fail;
  2130			}
  2131	
  2132			ret = tegra_pcie_config_ep(pcie, pdev);
  2133			if (ret < 0)
  2134				goto fail;
  2135			break;
  2136	
  2137		default:
  2138			dev_err(dev, "Invalid PCIe device type %d\n", pcie->mode);
  2139		}
  2140	
  2141	fail:
  2142		tegra_bpmp_put(pcie->bpmp);
  2143		return ret;
  2144	}
  2145	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 47045 bytes --]

RE: [PATCH v7 2/5] PCI: dwc: add support to handle ZRX-DC Compliant PHYs

[Pankaj Dubey]

> -----Original Message-----
<snip>
> Subject: Re: [PATCH v7 2/5] PCI: dwc: add support to handle ZRX-DC
> Compliant PHYs
> 
> Capitalize subject to match the rest of the series.
> 
> "Add support to handle .." is redundant; "Add support for ..." would be
> equivalent and shorter.

OK 

> 
> But this patch actually doesn't add anything at all by itself, since it
checks pci-
> >phy_zrxdc_compliant but never sets it.

OK, we will reword the commit message as  "configure controller to handle
ZRX-DC compliant PHYs"

> 
> On Thu, Jan 07, 2021 at 08:58:40PM +0530, Shradha Todi wrote:
> > From: Pankaj Dubey <pankaj.dubey@samsung.com>
> >
> > Many platforms use DesignWare controller but the PHY can be different
> > in different platforms. If the PHY is compliant is to ZRX-DC
> > specification it helps in low power consumption during power states.
> 
> s/is to/to/

OK

> 
> Even with that, this sentence doesn't quite parse correctly.  Do you mean
> something like this?
> 
>   If the PHY is compliant to the ZRX-DC specification, it reduces
>   power consumption in low power Link states.
> 
> (I assume this is related to Link power states (L0, L1, etc), not device
power
> states (D0, D3hot, etc)).
> 

Yes, we are talking about Link power states. We rephrase the commit
description to make it more clear.

> > If current data rate is 8.0 GT/s or higher and PHY is not compliant to
> > ZRX-DC specification, then after every 100ms link should transition to
> > recovery state during the low power states.
> 
> Not sure this makes sense.  If the Link is in a low power state for 10
seconds,
> it must transition to the Recovery state every 100ms during that 10
seconds,
> i.e., 100 times?
> 

According to SNPS DesignWare data-book, the link will transition into
recovery state every 100ms, which means that yes, 100 times in 10 seconds.
But what we are trying to say here is that if the PHY is ZRX-DC compliant,
then the controller does not need to do this and we can thus save power
consumption.

As per the DesignWare data-book, the controller keeps this bit set to '1' by
default ("This bit enables a 100ms timer which can trigger exit from L1.")
and we are trying to reset this bit to '0' in order to not perform the
constant recovery and hence save power.

> > DesignWare controller provides GEN3_ZRXDC_NONCOMPL field in
> > GEN3_RELATED_OFF to specify about ZRX-DC compliant PHY.
> >
> > Platforms with ZRX-DC compliant PHY can set phy_zrxdc_compliant
> > variable to specify this property to the controller.
> 
> If this is a DesignWare-generic register and the "phy-zrxdc-compliant"
> property can be used by any DesignWare-based driver, why isn't the code to
> look for it in the DesignWare-generic part?
> 

Do you mean why this property is part of PHY node instead of DesignWare
controller?

> Is there a link to the ZRX-DC specification you can mention somewhere in
this
> series?
> 

I don't know if there is any separate ZRX-DC specification exists which can
be pointed out here, but we have implementation note in PCIe specification
Rev 4.0 which says as:
"Ports that meet the ZRX-DC specification for 2.5 GT/s while in the L1.Idle
state and are therefore not required to implement the 100 ms timeout and
transition to Recovery should avoid implementing it, since it will reduce
the power savings expected from the L1 state." 

We have captured same in cover-letter of this patch series.

> > Signed-off-by: Anvesh Salveru <anvesh.salveru@gmail.com>
> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> > Cc: Jingoo Han <jingoohan1@gmail.com>
> > Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 6 ++++++
> > drivers/pci/controller/dwc/pcie-designware.h | 4 ++++
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> > b/drivers/pci/controller/dwc/pcie-designware.c
> > index 645fa18..74590c7 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -722,4 +722,10 @@ void dw_pcie_setup(struct dw_pcie *pci)
> >  		       PCIE_PL_CHK_REG_CHK_REG_START;
> >  		dw_pcie_writel_dbi(pci,
> PCIE_PL_CHK_REG_CONTROL_STATUS, val);
> >  	}
> > +
> > +	if (pci->phy_zrxdc_compliant) {
> > +		val = dw_pcie_readl_dbi(pci, PCIE_PORT_GEN3_RELATED);
> > +		val &= ~PORT_LOGIC_GEN3_ZRXDC_NONCOMPL;
> > +		dw_pcie_writel_dbi(pci, PCIE_PORT_GEN3_RELATED, val);
> > +	}
> >  }
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > b/drivers/pci/controller/dwc/pcie-designware.h
> > index 0207840..8b905a2 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -74,6 +74,9 @@
> >  #define PCIE_MSI_INTR0_MASK		0x82C
> >  #define PCIE_MSI_INTR0_STATUS		0x830
> >
> > +#define PCIE_PORT_GEN3_RELATED		0x890
> > +#define PORT_LOGIC_GEN3_ZRXDC_NONCOMPL	BIT(0)
> > +
> >  #define PCIE_PORT_MULTI_LANE_CTRL	0x8C0
> >  #define PORT_MLTI_UPCFG_SUPPORT		BIT(7)
> >
> > @@ -273,6 +276,7 @@ struct dw_pcie {
> >  	u8			n_fts[2];
> >  	bool			iatu_unroll_enabled: 1;
> >  	bool			io_cfg_atu_shared: 1;
> > +	bool			phy_zrxdc_compliant;
> 
> I raise my eyebrows a little at "bool xx : 1".  I think it's probably
*correct*, but
> "unsigned int xx : 1" is the overwhelming favorite and I doubt bool gives
any
> advantage.
> 
>   $ git grep -E "int\s+\S+\s*:\s*1" | egrep "^\S*\.[ch]" | wc -l
>   3129
>   $ git grep -E "bool\s+\S+\s*:\s*1" | egrep "^\S*\.[ch]" | wc -l
>   637
> 
> pcie-designware.h is the only user in drivers/pci.  But you're following
the
> existing style in the file, which is good.

No, we didn't follow existing style, we will update this in next version.

Thanks for review.

Pankaj Dubey
> 
> >  };
> >
> >  #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie,
> > pp)
> > --
> > 2.7.4
> >