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
From: Pankaj Dubey <[email protected]>
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 <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
Signed-off-by: Shradha Todi <[email protected]>
Cc: Jingoo Han <[email protected]>
Cc: Gustavo Pimentel <[email protected]>
Cc: Lorenzo Pieralisi <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Bjorn Helgaas <[email protected]>
---
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
From: Pankaj Dubey <[email protected]>
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 <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
Signed-off-by: Shradha Todi <[email protected]>
Cc: Kishon Vijay Abraham I <[email protected]>
Cc: Vinod Koul <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
Signed-off-by: Shradha Todi <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Kishon Vijay Abraham I <[email protected]>
Cc: Vidya Sagar <[email protected]>
---
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
From: Pankaj Dubey <[email protected]>
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 <[email protected]>
Signed-off-by: Pankaj Dubey <[email protected]>
Signed-off-by: Shradha Todi <[email protected]>
Cc: Lorenzo Pieralisi <[email protected]>
Cc: Andrew Murray <[email protected]>
Cc: Bjorn Helgaas <[email protected]>
Cc: Vidya Sagar <[email protected]>
Cc: Jonathan Hunter <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Shradha Todi <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Jonathan Hunter <[email protected]>
Cc: Vidya Sagar <[email protected]>
To: [email protected]
To: [email protected]
---
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
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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Pankaj Dubey <[email protected]>
> Signed-off-by: Shradha Todi <[email protected]>
> Cc: Jingoo Han <[email protected]>
> Cc: Gustavo Pimentel <[email protected]>
> Cc: Lorenzo Pieralisi <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> ---
> 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
>
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 <[email protected]>
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/[email protected]
> -----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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Pankaj Dubey <[email protected]>
> > Signed-off-by: Shradha Todi <[email protected]>
> > Cc: Jingoo Han <[email protected]>
> > Cc: Gustavo Pimentel <[email protected]>
> > Cc: Lorenzo Pieralisi <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Bjorn Helgaas <[email protected]>
> > ---
> > 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
> >