LinuxLists.cc - [PATCH 01/12] pcie: qcom: add missing ipq806x clocks in pcie driver

[permalink] [raw]

Subject: [PATCH 03/12] pcie: qcom: change duplicate pci reset to phy reset

From: Abhishek Sahu <[email protected]>

The deinit issues reset_control_assert for pci twice and
does not contain phy reset.

Signed-off-by: Abhishek Sahu <[email protected]>
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/pci/controller/dwc/pcie-qcom.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index f958c535de6e..1fcc7fed8443 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -284,7 +284,7 @@ static void qcom_pcie_deinit_2_1_0(struct qcom_pcie *pcie)
reset_control_assert(res->axi_reset);
reset_control_assert(res->ahb_reset);
reset_control_assert(res->por_reset);
- reset_control_assert(res->pci_reset);
+ reset_control_assert(res->phy_reset);
clk_disable_unprepare(res->iface_clk);
clk_disable_unprepare(res->core_clk);
clk_disable_unprepare(res->phy_clk);
--
2.25.1

2020-03-20 18:36:14

[permalink] [raw]

Subject: [PATCH 04/12] pcie: qcom: Fixed pcie_phy_clk branch issue

Following backtraces are observed in PCIe deinit operation.

Hardware name: Qualcomm (Flattened Device Tree)
(unwind_backtrace) from [] (show_stack+0x10/0x14)
(show_stack) from [] (dump_stack+0x84/0x98)
(dump_stack) from [] (warn_slowpath_common+0x9c/0xb8)
(warn_slowpath_common) from [] (warn_slowpath_fmt+0x30/0x40)
(warn_slowpath_fmt) from [] (clk_branch_wait+0x114/0x120)
(clk_branch_wait) from [] (clk_core_disable+0xd0/0x1f4)
(clk_core_disable) from [] (clk_disable+0x24/0x30)
(clk_disable) from [] (qcom_pcie_deinit_v0+0x6c/0xb8)
(qcom_pcie_deinit_v0) from [] (qcom_pcie_host_init+0xe0/0xe8)
(qcom_pcie_host_init) from [] (dw_pcie_host_init+0x3b0/0x538)
(dw_pcie_host_init) from [] (qcom_pcie_probe+0x20c/0x2e4)

pcie_phy_clk is generated for PCIe controller itself and the
GCC controls its branch operation. This error is coming since
the assert operations turn off the parent clock before branch
clock. Now this patch moves clk_disable_unprepare before assert
operations.

Similarly, during probe function, the clock branch operation
should be done after dessert operation. Currently, it does not
generate any error since bootloader enables the pcie_phy_clk
but the same error is coming during probe, if bootloader
disables pcie_phy_clk.

Signed-off-by: Abhishek Sahu <[email protected]>
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/pci/controller/dwc/pcie-qcom.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 1fcc7fed8443..596731b54728 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -280,6 +280,7 @@ static void qcom_pcie_deinit_2_1_0(struct qcom_pcie *pcie)
{
struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0;

+ clk_disable_unprepare(res->phy_clk);
reset_control_assert(res->pci_reset);
reset_control_assert(res->axi_reset);
reset_control_assert(res->ahb_reset);
@@ -287,7 +288,6 @@ static void qcom_pcie_deinit_2_1_0(struct qcom_pcie *pcie)
reset_control_assert(res->phy_reset);
clk_disable_unprepare(res->iface_clk);
clk_disable_unprepare(res->core_clk);
- clk_disable_unprepare(res->phy_clk);
clk_disable_unprepare(res->aux_clk);
clk_disable_unprepare(res->ref_clk);
regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
@@ -325,12 +325,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
goto err_clk_core;
}

- ret = clk_prepare_enable(res->phy_clk);
- if (ret) {
- dev_err(dev, "cannot prepare/enable phy clock\n");
- goto err_clk_phy;
- }
-
ret = clk_prepare_enable(res->aux_clk);
if (ret) {
dev_err(dev, "cannot prepare/enable aux clock\n");
@@ -383,6 +377,12 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
return ret;
}

+ ret = clk_prepare_enable(res->phy_clk);
+ if (ret) {
+ dev_err(dev, "cannot prepare/enable phy clock\n");
+ goto err_deassert_ahb;
+ }
+
/* wait for clock acquisition */
usleep_range(1000, 1500);

@@ -400,8 +400,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
err_clk_ref:
clk_disable_unprepare(res->aux_clk);
err_clk_aux:
- clk_disable_unprepare(res->phy_clk);
-err_clk_phy:
clk_disable_unprepare(res->core_clk);
err_clk_core:
clk_disable_unprepare(res->iface_clk);
--
2.25.1

2020-03-20 18:36:22

[permalink] [raw]

Subject: [PATCH 06/12] devicetree: bindings: pci: add ext reset to qcom,pcie

Document ext reset used in ipq806x soc by qcom pcie driver

Signed-off-by: Ansuel Smith <[email protected]>
---
Documentation/devicetree/bindings/pci/qcom,pcie.txt | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index becdbdc0fffa..6efcef040741 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -179,6 +179,7 @@
- "pwr" PWR reset
- "ahb" AHB reset
- "phy_ahb" PHY AHB reset
+ - "ext" EXT reset

- reset-names:
Usage: required for ipq8074
@@ -287,8 +288,9 @@
<&gcc PCIE_HCLK_RESET>,
<&gcc PCIE_POR_RESET>,
<&gcc PCIE_PCI_RESET>,
- <&gcc PCIE_PHY_RESET>;
- reset-names = "axi", "ahb", "por", "pci", "phy";
+ <&gcc PCIE_PHY_RESET>,
+ <&gcc PCIE_EXT_RESET>;
+ reset-names = "axi", "ahb", "por", "pci", "phy", "ext";
pinctrl-0 = <&pcie_pins_default>;
pinctrl-names = "default";
};
--
2.25.1

2020-03-20 18:36:30

[permalink] [raw]

Subject: [PATCH 12/12] pcie: qcom: Set PCIE MRRS and MPS to 256B

From: Sriram Palanisamy <[email protected]>

Set Max Read Request Size and Max Payload Size to 256 bytes,
per chip team recommendation.

Signed-off-by: Gokul Sriram Palanisamy <[email protected]>
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/pci/controller/dwc/pcie-qcom.c | 37 ++++++++++++++++++++++++++
1 file changed, 37 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 03130a3206b4..ad437c6f49a0 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -125,6 +125,14 @@

#define PCIE20_LNK_CONTROL2_LINK_STATUS2 0xA0

+#define __set(v, a, b) (((v) << (b)) & GENMASK(a, b))
+#define __mask(a, b) (((1 << ((a) + 1)) - 1) & ~((1 << (b)) - 1))
+#define PCIE20_DEV_CAS 0x78
+#define PCIE20_MRRS_MASK __mask(14, 12)
+#define PCIE20_MRRS(x) __set(x, 14, 12)
+#define PCIE20_MPS_MASK __mask(7, 5)
+#define PCIE20_MPS(x) __set(x, 7, 5)
+
#define DEVICE_TYPE_RC 0x4

#define QCOM_PCIE_2_1_0_MAX_SUPPLY 3
@@ -1595,6 +1603,35 @@ static int qcom_pcie_probe(struct platform_device *pdev)
return ret;
}

+static void qcom_pcie_fixup_final(struct pci_dev *pcidev)
+{
+ int cap, err;
+ u16 ctl, reg_val;
+
+ cap = pci_pcie_cap(pcidev);
+ if (!cap)
+ return;
+
+ err = pci_read_config_word(pcidev, cap + PCI_EXP_DEVCTL, &ctl);
+
+ if (err)
+ return;
+
+ reg_val = ctl;
+
+ if (((reg_val & PCIE20_MRRS_MASK) >> 12) > 1)
+ reg_val = (reg_val & ~(PCIE20_MRRS_MASK)) | PCIE20_MRRS(0x1);
+
+ if (((ctl & PCIE20_MPS_MASK) >> 5) > 1)
+ reg_val = (reg_val & ~(PCIE20_MPS_MASK)) | PCIE20_MPS(0x1);
+
+ err = pci_write_config_word(pcidev, cap + PCI_EXP_DEVCTL, reg_val);
+
+ if (err)
+ dev_err(&pcidev->dev, "pcie config write failed %d\n", err);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, qcom_pcie_fixup_final);
+
static const struct of_device_id qcom_pcie_match[] = {
{ .compatible = "qcom,pcie-apq8084", .data = &ops_1_0_0 },
{ .compatible = "qcom,pcie-ipq8064", .data = &ops_2_1_0 },
--
2.25.1

2020-03-20 18:36:38

[permalink] [raw]

Subject: [PATCH 11/12] devicetree: bindings: pci: add force_gen1 for qcom,pcie

Document force_gen1 optional definition to limit pcie
line to GEN1 speed

Signed-off-by: Ansuel Smith <[email protected]>
---
Documentation/devicetree/bindings/pci/qcom,pcie.txt | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 8c1d014f37b0..766876465c42 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -260,6 +260,11 @@
Definition: If not defined is 0. In ipq806x is set to 7. In newer
revision (v2.0) the offset is zero.

+- force_gen1:
+ Usage: optional
+ Value type: <u32>
+ Definition: Set 1 to force the pcie line to GEN1
+
* Example for ipq/apq8064
pcie@1b500000 {
compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";
--
2.25.1

2020-03-20 18:36:42

[permalink] [raw]

Subject: [PATCH 09/12] pcie: qcom: Programming the PCIE iATU for IPQ806x

From: Sham Muthayyan <[email protected]>

Resolved PCIE EP detection errors caused due to missing iATU programming.

Signed-off-by: Sham Muthayyan <[email protected]>
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/pci/controller/dwc/pcie-qcom.c | 78 ++++++++++++++++++++++++++
1 file changed, 78 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 8009e3117765..e26ba8f63d4f 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -77,6 +77,30 @@
#define PCIE20_CAP_LINK_1 (PCIE20_CAP + 0x14)
#define PCIE_CAP_LINK1_VAL 0x2FD7F

+#define PCIE20_CAP_LINKCTRLSTATUS (PCIE20_CAP + 0x10)
+
+#define PCIE20_AXI_MSTR_RESP_COMP_CTRL0 0x818
+#define PCIE20_AXI_MSTR_RESP_COMP_CTRL1 0x81c
+
+#define PCIE20_PLR_IATU_VIEWPORT 0x900
+#define PCIE20_PLR_IATU_REGION_OUTBOUND (0x0 << 31)
+#define PCIE20_PLR_IATU_REGION_INDEX(x) (x << 0)
+
+#define PCIE20_PLR_IATU_CTRL1 0x904
+#define PCIE20_PLR_IATU_TYPE_CFG0 (0x4 << 0)
+#define PCIE20_PLR_IATU_TYPE_MEM (0x0 << 0)
+
+#define PCIE20_PLR_IATU_CTRL2 0x908
+#define PCIE20_PLR_IATU_ENABLE BIT(31)
+
+#define PCIE20_PLR_IATU_LBAR 0x90C
+#define PCIE20_PLR_IATU_UBAR 0x910
+#define PCIE20_PLR_IATU_LAR 0x914
+#define PCIE20_PLR_IATU_LTAR 0x918
+#define PCIE20_PLR_IATU_UTAR 0x91c
+
+#define MSM_PCIE_DEV_CFG_ADDR 0x01000000
+
#define PCIE20_PARF_Q2A_FLUSH 0x1AC

#define PCIE20_MISC_CONTROL_1_REG 0x8BC
@@ -251,6 +275,57 @@ static void qcom_pcie_2_1_0_ltssm_enable(struct qcom_pcie *pcie)
writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL);
}

+static void qcom_pcie_prog_viewport_cfg0(struct qcom_pcie *pcie, u32 busdev)
+{
+ struct pcie_port *pp = &pcie->pci->pp;
+
+ /*
+ * program and enable address translation region 0 (device config
+ * address space); region type config;
+ * axi config address range to device config address range
+ */
+ writel(PCIE20_PLR_IATU_REGION_OUTBOUND |
+ PCIE20_PLR_IATU_REGION_INDEX(0),
+ pcie->pci->dbi_base + PCIE20_PLR_IATU_VIEWPORT);
+
+ writel(PCIE20_PLR_IATU_TYPE_CFG0, pcie->pci->dbi_base + PCIE20_PLR_IATU_CTRL1);
+ writel(PCIE20_PLR_IATU_ENABLE, pcie->pci->dbi_base + PCIE20_PLR_IATU_CTRL2);
+ writel(pp->cfg0_base, pcie->pci->dbi_base + PCIE20_PLR_IATU_LBAR);
+ writel((pp->cfg0_base >> 32), pcie->pci->dbi_base + PCIE20_PLR_IATU_UBAR);
+ writel((pp->cfg0_base + pp->cfg0_size - 1),
+ pcie->pci->dbi_base + PCIE20_PLR_IATU_LAR);
+ writel(busdev, pcie->pci->dbi_base + PCIE20_PLR_IATU_LTAR);
+ writel(0, pcie->pci->dbi_base + PCIE20_PLR_IATU_UTAR);
+}
+
+static void qcom_pcie_prog_viewport_mem2_outbound(struct qcom_pcie *pcie)
+{
+ struct pcie_port *pp = &pcie->pci->pp;
+
+ /*
+ * program and enable address translation region 2 (device resource
+ * address space); region type memory;
+ * axi device bar address range to device bar address range
+ */
+ writel(PCIE20_PLR_IATU_REGION_OUTBOUND |
+ PCIE20_PLR_IATU_REGION_INDEX(2),
+ pcie->pci->dbi_base + PCIE20_PLR_IATU_VIEWPORT);
+
+ writel(PCIE20_PLR_IATU_TYPE_MEM, pcie->pci->dbi_base + PCIE20_PLR_IATU_CTRL1);
+ writel(PCIE20_PLR_IATU_ENABLE, pcie->pci->dbi_base + PCIE20_PLR_IATU_CTRL2);
+ writel(pp->mem_base, pcie->pci->dbi_base + PCIE20_PLR_IATU_LBAR);
+ writel((pp->mem_base >> 32), pcie->pci->dbi_base + PCIE20_PLR_IATU_UBAR);
+ writel(pp->mem_base + pp->mem_size - 1,
+ pcie->pci->dbi_base + PCIE20_PLR_IATU_LAR);
+ writel(pp->mem_bus_addr, pcie->pci->dbi_base + PCIE20_PLR_IATU_LTAR);
+ writel(upper_32_bits(pp->mem_bus_addr),
+ pcie->pci->dbi_base + PCIE20_PLR_IATU_UTAR);
+
+ /* 256B PCIE buffer setting */
+ writel(0x1, pcie->pci->dbi_base + PCIE20_AXI_MSTR_RESP_COMP_CTRL0);
+ writel(0x1, pcie->pci->dbi_base + PCIE20_AXI_MSTR_RESP_COMP_CTRL1);
+}
+
static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
{
struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0;
@@ -448,6 +523,9 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
writel(CFG_BRIDGE_SB_INIT,
pci->dbi_base + PCIE20_AXI_MSTR_RESP_COMP_CTRL1);

+ qcom_pcie_prog_viewport_cfg0(pcie, MSM_PCIE_DEV_CFG_ADDR);
+ qcom_pcie_prog_viewport_mem2_outbound(pcie);
+
return 0;

err_deassert_ahb:
--
2.25.1

2020-03-20 18:36:57

[permalink] [raw]

Subject: [PATCH 08/12] devicetree: bindings: pci: add phy-tx0-term-offset to qcom,pcie

Document phy-tx0-term-offset propriety to qcom pcie driver

Signed-off-by: Ansuel Smith <[email protected]>
---
Documentation/devicetree/bindings/pci/qcom,pcie.txt | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 6efcef040741..8c1d014f37b0 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -254,6 +254,12 @@
- "perst-gpios" PCIe endpoint reset signal line
- "wake-gpios" PCIe endpoint wake signal line

+- phy-tx0-term-offset:
+ Usage: optional
+ Value type: <u32>
+ Definition: If not defined is 0. In ipq806x is set to 7. In newer
+ revision (v2.0) the offset is zero.
+
* Example for ipq/apq8064
pcie@1b500000 {
compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";
@@ -293,6 +299,7 @@
reset-names = "axi", "ahb", "por", "pci", "phy", "ext";
pinctrl-0 = <&pcie_pins_default>;
pinctrl-names = "default";
+ phy-tx0-term-offset = <7>;
};

* Example for apq8084
--
2.25.1

2020-03-20 18:37:09

[permalink] [raw]

Subject: [PATCH 05/12] pcie: qcom: add missing reset for ipq806x

Add missing ext reset used by ipq806x soc in
pcie qcom driver

Signed-off-by: Sham Muthayyan <[email protected]>
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/pci/controller/dwc/pcie-qcom.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 596731b54728..ecc22fd27ea6 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -95,6 +95,7 @@ struct qcom_pcie_resources_2_1_0 {
struct reset_control *ahb_reset;
struct reset_control *por_reset;
struct reset_control *phy_reset;
+ struct reset_control *ext_reset;
struct regulator_bulk_data supplies[QCOM_PCIE_2_1_0_MAX_SUPPLY];
};

@@ -272,6 +273,10 @@ static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
if (IS_ERR(res->por_reset))
return PTR_ERR(res->por_reset);

+ res->ext_reset = devm_reset_control_get(dev, "ext");
+ if (IS_ERR(res->ext_reset))
+ return PTR_ERR(res->ext_reset);
+
res->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
return PTR_ERR_OR_ZERO(res->phy_reset);
}
@@ -285,6 +290,7 @@ static void qcom_pcie_deinit_2_1_0(struct qcom_pcie *pcie)
reset_control_assert(res->axi_reset);
reset_control_assert(res->ahb_reset);
reset_control_assert(res->por_reset);
+ reset_control_assert(res->ext_reset);
reset_control_assert(res->phy_reset);
clk_disable_unprepare(res->iface_clk);
clk_disable_unprepare(res->core_clk);
@@ -301,18 +307,18 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
u32 val;
int ret;

+ ret = reset_control_assert(res->ahb_reset);
+ if (ret) {
+ dev_err(dev, "cannot assert ahb reset\n");
+ return ret;
+ }
+
ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
if (ret < 0) {
dev_err(dev, "cannot enable regulators\n");
return ret;
}

- ret = reset_control_assert(res->ahb_reset);
- if (ret) {
- dev_err(dev, "cannot assert ahb reset\n");
- goto err_assert_ahb;
- }
-
ret = clk_prepare_enable(res->iface_clk);
if (ret) {
dev_err(dev, "cannot prepare/enable iface clock\n");
@@ -343,6 +349,12 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
goto err_deassert_ahb;
}

+ ret = reset_control_deassert(res->ext_reset);
+ if (ret) {
+ dev_err(dev, "cannot assert ext reset\n");
+ goto err_deassert_ahb;
+ }
+
/* enable PCIe clocks and resets */
val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
val &= ~BIT(0);
--
2.25.1

2020-03-20 18:37:27

[permalink] [raw]

Subject: [PATCH 07/12] pcie: qcom: add tx term offset support

From: Sham Muthayyan <[email protected]>

Add tx term offset support to pcie qcom driver
need in some revision of the ipq806x soc

Signed-off-by: Sham Muthayyan <[email protected]>
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/pci/controller/dwc/pcie-qcom.c | 61 ++++++++++++++++++++++----
1 file changed, 52 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index ecc22fd27ea6..8009e3117765 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -45,7 +45,13 @@
#define PCIE_CAP_CPL_TIMEOUT_DISABLE 0x10

#define PCIE20_PARF_PHY_CTRL 0x40
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK (0x1f << 16)
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x) (x << 16)
+
#define PCIE20_PARF_PHY_REFCLK 0x4C
+#define REF_SSP_EN BIT(16)
+#define REF_USE_PAD BIT(12)
+
#define PCIE20_PARF_DBI_BASE_ADDR 0x168
#define PCIE20_PARF_SLV_ADDR_SPACE_SIZE 0x16C
#define PCIE20_PARF_MHI_CLOCK_RESET_CTRL 0x174
@@ -77,6 +83,18 @@
#define DBI_RO_WR_EN 1

#define PERST_DELAY_US 1000
+/* PARF registers */
+#define PCIE20_PARF_PCS_DEEMPH 0x34
+#define PCS_DEEMPH_TX_DEEMPH_GEN1(x) (x << 16)
+#define PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(x) (x << 8)
+#define PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(x) (x << 0)
+
+#define PCIE20_PARF_PCS_SWING 0x38
+#define PCS_SWING_TX_SWING_FULL(x) (x << 8)
+#define PCS_SWING_TX_SWING_LOW(x) (x << 0)
+
+#define PCIE20_PARF_CONFIG_BITS 0x50
+#define PHY_RX0_EQ(x) (x << 24)

#define PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE 0x358
#define SLV_ADDR_SPACE_SZ 0x10000000
@@ -97,6 +115,7 @@ struct qcom_pcie_resources_2_1_0 {
struct reset_control *phy_reset;
struct reset_control *ext_reset;
struct regulator_bulk_data supplies[QCOM_PCIE_2_1_0_MAX_SUPPLY];
+ uint8_t phy_tx0_term_offset;
};

struct qcom_pcie_resources_1_0_0 {
@@ -184,6 +203,16 @@ struct qcom_pcie {

#define to_qcom_pcie(x) dev_get_drvdata((x)->dev)

+static inline void
+writel_masked(void __iomem *addr, u32 clear_mask, u32 set_mask)
+{
+ u32 val = readl(addr);
+
+ val &= ~clear_mask;
+ val |= set_mask;
+ writel(val, addr);
+}
+
static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
{
gpiod_set_value_cansleep(pcie->reset, 1);
@@ -277,6 +306,10 @@ static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
if (IS_ERR(res->ext_reset))
return PTR_ERR(res->ext_reset);

+ if (of_property_read_u8(dev->of_node, "phy-tx0-term-offset",
+ &res->phy_tx0_term_offset))
+ res->phy_tx0_term_offset = 0;
+
res->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
return PTR_ERR_OR_ZERO(res->phy_reset);
}
@@ -304,7 +337,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0;
struct dw_pcie *pci = pcie->pci;
struct device *dev = pci->dev;
- u32 val;
int ret;

ret = reset_control_assert(res->ahb_reset);
@@ -355,15 +387,26 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
goto err_deassert_ahb;
}

- /* enable PCIe clocks and resets */
- val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
- val &= ~BIT(0);
- writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+ writel_masked(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
+
+ /* Set Tx termination offset */
+ writel_masked(pcie->parf + PCIE20_PARF_PHY_CTRL,
+ PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK,
+ PHY_CTRL_PHY_TX0_TERM_OFFSET(res->phy_tx0_term_offset));
+
+ /* PARF programming */
+ writel(PCS_DEEMPH_TX_DEEMPH_GEN1(0x18) |
+ PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(0x18) |
+ PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(0x22),
+ pcie->parf + PCIE20_PARF_PCS_DEEMPH);
+ writel(PCS_SWING_TX_SWING_FULL(0x78) |
+ PCS_SWING_TX_SWING_LOW(0x78),
+ pcie->parf + PCIE20_PARF_PCS_SWING);
+ writel(PHY_RX0_EQ(0x4), pcie->parf + PCIE20_PARF_CONFIG_BITS);

- /* enable external reference clock */
- val = readl(pcie->parf + PCIE20_PARF_PHY_REFCLK);
- val |= BIT(16);
- writel(val, pcie->parf + PCIE20_PARF_PHY_REFCLK);
+ /* Enable reference clock */
+ writel_masked(pcie->parf + PCIE20_PARF_PHY_REFCLK,
+ REF_USE_PAD, REF_SSP_EN);

ret = reset_control_deassert(res->phy_reset);
if (ret) {
--
2.25.1

2020-03-20 18:37:29

[permalink] [raw]

Subject: [PATCH 02/12] devicetree: bindings: pci: add missing clks to qcom,pcie

Document missing clks used in ipq806x soc

Signed-off-by: Ansuel Smith <[email protected]>
---
Documentation/devicetree/bindings/pci/qcom,pcie.txt | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 981b4de12807..becdbdc0fffa 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -90,6 +90,8 @@
Definition: Should contain the following entries
- "core" Clocks the pcie hw block
- "phy" Clocks the pcie PHY block
+ - "aux" Clocks the pcie AUX block
+ - "ref" Clocks the pcie ref block
- clock-names:
Usage: required for apq8084/ipq4019
Value type: <stringlist>
@@ -277,8 +279,10 @@
<0 0 0 4 &intc 0 39 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
clocks = <&gcc PCIE_A_CLK>,
<&gcc PCIE_H_CLK>,
- <&gcc PCIE_PHY_CLK>;
- clock-names = "core", "iface", "phy";
+ <&gcc PCIE_PHY_CLK>,
+ <&gcc PCIE_AUX_CLK>,
+ <&gcc PCIE_ALT_REF_CLK>;
+ clock-names = "core", "iface", "phy", "aux", "ref";
resets = <&gcc PCIE_ACLK_RESET>,
<&gcc PCIE_HCLK_RESET>,
<&gcc PCIE_POR_RESET>,
--
2.25.1

2020-03-20 18:37:33

[permalink] [raw]

Subject: [PATCH 10/12] pcie: qcom: add Force GEN1 support

From: Sham Muthayyan <[email protected]>

Add Force GEN1 support needed in some ipq806x board
that needs to limit some pcie line to gen1 for some
hardware limitation

Signed-off-by: Sham Muthayyan <[email protected]>
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/pci/controller/dwc/pcie-qcom.c | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index e26ba8f63d4f..03130a3206b4 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -123,6 +123,8 @@
#define PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE 0x358
#define SLV_ADDR_SPACE_SZ 0x10000000

+#define PCIE20_LNK_CONTROL2_LINK_STATUS2 0xA0
+
#define DEVICE_TYPE_RC 0x4

#define QCOM_PCIE_2_1_0_MAX_SUPPLY 3
@@ -223,6 +225,7 @@ struct qcom_pcie {
struct phy *phy;
struct gpio_desc *reset;
const struct qcom_pcie_ops *ops;
+ uint32_t force_gen1;
};

#define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
@@ -515,6 +518,11 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)

/* wait for clock acquisition */
usleep_range(1000, 1500);
+ if (pcie->force_gen1) {
+ writel_relaxed((readl_relaxed(
+ pcie->pci->dbi_base + PCIE20_LNK_CONTROL2_LINK_STATUS2) | 1),
+ pcie->pci->dbi_base + PCIE20_LNK_CONTROL2_LINK_STATUS2);
+ }

/* Set the Max TLP size to 2K, instead of using default of 4K */
@@ -1487,6 +1495,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
struct dw_pcie *pci;
struct qcom_pcie *pcie;
int ret;
+ uint32_t force_gen1 = 0;
+ struct device_node *np = pdev->dev.of_node;

pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
if (!pcie)
@@ -1517,6 +1527,9 @@ static int qcom_pcie_probe(struct platform_device *pdev)
goto err_pm_runtime_put;
}

+ of_property_read_u32(np, "force_gen1", &force_gen1);
+ pcie->force_gen1 = force_gen1;
+
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "parf");
pcie->parf = devm_ioremap_resource(dev, res);
if (IS_ERR(pcie->parf)) {
--
2.25.1

2020-03-20 18:48:24

[permalink] [raw]

Subject: Re: [PATCH 01/12] pcie: qcom: add missing ipq806x clocks in pcie driver

Please add a cover letter with the patches as responses to it.

Make your subjects match in capitalization, verb tense, etc.:

$ git log --oneline drivers/pci/controller/dwc/pcie-qcom.c
ed8cc3b1fc84 PCI: qcom: Add support for SDM845 PCIe controller
64adde31c8e9 PCI: qcom: Ensure that PERST is asserted for at least 100 ms
67021ae0bbe9 PCI: qcom: Add QCS404 PCIe controller support
5aa180974e4d PCI: qcom: Use clk bulk API for 2.4.0 controllers
322f03436692 PCI: qcom: Use default config space read function
02b485e31d98 PCI: qcom: Don't deassert reset GPIO during probe
6e5da6f7d824 PCI: qcom: Fix error handling in runtime PM support
739cd35918b7 PCI: qcom: Drop unnecessary root_bus_nr setting

On Fri, Mar 20, 2020 at 07:34:43PM +0100, Ansuel Smith wrote:
> Aux and Ref clk are missing in pcie qcom driver.
> Add support in the driver to fix pcie inizialization
> in ipq806x

s/pcie/PCIe/ in English text like commit logs and comments.
s/inizialization/initialization/

It'd be useful to have enough context to know what "ipq806x" is.

Add period at end of sentence.

2020-03-20 18:53:28

[permalink] [raw]

Subject: Re: [PATCH 05/12] pcie: qcom: add missing reset for ipq806x

On Fri, Mar 20, 2020 at 07:34:47PM +0100, Ansuel Smith wrote:
> Add missing ext reset used by ipq806x soc in
> pcie qcom driver

You say "missing" -- does that mean this is a *new* requirement for
this ipq806x device, and previous devices work correctly without this
patch?

Or does this fix an omission and previous devices actually didn't work
correctly?

s/soc/SoC/
s/pcie/PCIe/

Period at end of sentence. Please wrap to fill 75 columns.

2020-03-20 19:23:29

[permalink] [raw]

Subject: Re: [PATCH 07/12] pcie: qcom: add tx term offset support

On Fri, Mar 20, 2020 at 07:34:49PM +0100, Ansuel Smith wrote:
> From: Sham Muthayyan <[email protected]>
>
> Add tx term offset support to pcie qcom driver
> need in some revision of the ipq806x soc

The usual (s/pcie/PCIe/, s/soc/SoC/, wrap to fill 75 columns, add
period at end). Apply to all patches.

> +#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK (0x1f << 16)
> +#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x) (x << 16)

Since the rest of the file uses BIT(), I think it would make sense to
use GENMASK() here. And below, of course.

> +static inline void
> +writel_masked(void __iomem *addr, u32 clear_mask, u32 set_mask)

Follow coding style of rest of file, i.e.,

static inline void writel_masked(...)

The name "writel_masked" suggests that we're writing "val & mask" to a
register, but that's not what's happening. This is really a "clear
some bits and set others" operation.

The names are wordy, but we do have pci_clear_and_set_dword(),
pcie_capability_clear_and_set_word(), etc., functions that work
similarly.

> +{
> + u32 val = readl(addr);
> +
> + val &= ~clear_mask;
> + val |= set_mask;
> + writel(val, addr);
> +}

> + /* Set Tx termination offset */

Capitalize consistently with other comments in the file. Below also.
Hmm, I see the file is already a bit of a mess in that regard. So
just do what you can.

2020-03-20 19:26:36

[permalink] [raw]

Subject: Re: [PATCH 09/12] pcie: qcom: Programming the PCIE iATU for IPQ806x

s/Programming/Program/

Capitalize IPQ806x consistently (other patches mention "ipq806x").

On Fri, Mar 20, 2020 at 07:34:51PM +0100, Ansuel Smith wrote:
> From: Sham Muthayyan <[email protected]>
>
> Resolved PCIE EP detection errors caused due to missing iATU programming.

s/Resolved/Resolve/
s/PCIE/PCIe/

Is this a bug fix? If so, can you cite the commit that introduced the
bug? This is useful when backporting fixes. Same question applies to
the other patches as well.

2020-03-20 19:38:29

[permalink] [raw]

Subject: Re: [PATCH 10/12] pcie: qcom: add Force GEN1 support

On Fri, Mar 20, 2020 at 07:34:52PM +0100, Ansuel Smith wrote:
> From: Sham Muthayyan <[email protected]>
>
> Add Force GEN1 support needed in some ipq806x board
> that needs to limit some pcie line to gen1 for some
> hardware limitation

Usual commit log comments.

> + uint32_t force_gen1;

unsigned int force_gen1 : 1

> + of_property_read_u32(np, "force_gen1", &force_gen1);
> + pcie->force_gen1 = force_gen1;

I think there's a more or less standard property you can use for this
instead of inventing a new one specific to this device.

Documentation/devicetree/bindings/pci/pci.txt:
"max-link-speed"

2020-03-20 19:47:17

[permalink] [raw]

Subject: Re: [PATCH 12/12] pcie: qcom: Set PCIE MRRS and MPS to 256B

On Fri, Mar 20, 2020 at 07:34:54PM +0100, Ansuel Smith wrote:
> From: Sriram Palanisamy <[email protected]>
>
> Set Max Read Request Size and Max Payload Size to 256 bytes,
> per chip team recommendation.

Is this to avoid a device defect or to optimize performance?

This should not be done in an individual driver for performance
reasons because these parameters need to be managed at the system
level.

If this is to work around a device defect, we probably need to think
about a quirk that changes the device capabilities advertised by this
bridge and then changes to the PCI core code to take that into
account.

> Signed-off-by: Gokul Sriram Palanisamy <[email protected]>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 37 ++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 03130a3206b4..ad437c6f49a0 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -125,6 +125,14 @@
>
> #define PCIE20_LNK_CONTROL2_LINK_STATUS2 0xA0
>
> +#define __set(v, a, b) (((v) << (b)) & GENMASK(a, b))
> +#define __mask(a, b) (((1 << ((a) + 1)) - 1) & ~((1 << (b)) - 1))
> +#define PCIE20_DEV_CAS 0x78
> +#define PCIE20_MRRS_MASK __mask(14, 12)
> +#define PCIE20_MRRS(x) __set(x, 14, 12)
> +#define PCIE20_MPS_MASK __mask(7, 5)
> +#define PCIE20_MPS(x) __set(x, 7, 5)

These should all be the generic PCI_EXP_DEVCTL_READRQ and similar
#defines, since you use them on values from PCI_EXP_DEVCTL.

> #define DEVICE_TYPE_RC 0x4
>
> #define QCOM_PCIE_2_1_0_MAX_SUPPLY 3
> @@ -1595,6 +1603,35 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> return ret;
> }
>
> +static void qcom_pcie_fixup_final(struct pci_dev *pcidev)
> +{
> + int cap, err;
> + u16 ctl, reg_val;
> +
> + cap = pci_pcie_cap(pcidev);
> + if (!cap)
> + return;
> +
> + err = pci_read_config_word(pcidev, cap + PCI_EXP_DEVCTL, &ctl);
> +
> + if (err)
> + return;
> +
> + reg_val = ctl;
> +
> + if (((reg_val & PCIE20_MRRS_MASK) >> 12) > 1)
> + reg_val = (reg_val & ~(PCIE20_MRRS_MASK)) | PCIE20_MRRS(0x1);
> +
> + if (((ctl & PCIE20_MPS_MASK) >> 5) > 1)
> + reg_val = (reg_val & ~(PCIE20_MPS_MASK)) | PCIE20_MPS(0x1);
> +
> + err = pci_write_config_word(pcidev, cap + PCI_EXP_DEVCTL, reg_val);
> +
> + if (err)
> + dev_err(&pcidev->dev, "pcie config write failed %d\n", err);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, qcom_pcie_fixup_final);
> +
> static const struct of_device_id qcom_pcie_match[] = {
> { .compatible = "qcom,pcie-apq8084", .data = &ops_1_0_0 },
> { .compatible = "qcom,pcie-ipq8064", .data = &ops_2_1_0 },
> --
> 2.25.1
>

2020-03-23 06:08:54

by Philipp Zabel

[permalink] [raw]

Subject: Re: [PATCH 05/12] pcie: qcom: add missing reset for ipq806x

Hi Ansuel,

On Fri, Mar 20, 2020 at 07:34:47PM +0100, Ansuel Smith wrote:
> Add missing ext reset used by ipq806x soc in
> pcie qcom driver
>
> Signed-off-by: Sham Muthayyan <[email protected]>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 24 ++++++++++++++++++------
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 596731b54728..ecc22fd27ea6 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -95,6 +95,7 @@ struct qcom_pcie_resources_2_1_0 {
> struct reset_control *ahb_reset;
> struct reset_control *por_reset;
> struct reset_control *phy_reset;
> + struct reset_control *ext_reset;
> struct regulator_bulk_data supplies[QCOM_PCIE_2_1_0_MAX_SUPPLY];
> };
>
> @@ -272,6 +273,10 @@ static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
> if (IS_ERR(res->por_reset))
> return PTR_ERR(res->por_reset);
>
> + res->ext_reset = devm_reset_control_get(dev, "ext");

Please use devm_reset_control_get_exclusive() instead.

> + if (IS_ERR(res->ext_reset))
> + return PTR_ERR(res->ext_reset);
> +
> res->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
> return PTR_ERR_OR_ZERO(res->phy_reset);
> }
> @@ -285,6 +290,7 @@ static void qcom_pcie_deinit_2_1_0(struct qcom_pcie *pcie)
> reset_control_assert(res->axi_reset);
> reset_control_assert(res->ahb_reset);
> reset_control_assert(res->por_reset);
> + reset_control_assert(res->ext_reset);
> reset_control_assert(res->phy_reset);
> clk_disable_unprepare(res->iface_clk);
> clk_disable_unprepare(res->core_clk);
> @@ -301,18 +307,18 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> u32 val;
> int ret;
>
> + ret = reset_control_assert(res->ahb_reset);
> + if (ret) {
> + dev_err(dev, "cannot assert ahb reset\n");
> + return ret;
> + }
> +
> ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> if (ret < 0) {
> dev_err(dev, "cannot enable regulators\n");
> return ret;
> }
>
> - ret = reset_control_assert(res->ahb_reset);
> - if (ret) {
> - dev_err(dev, "cannot assert ahb reset\n");
> - goto err_assert_ahb;
> - }
> -

This change is not described in the commit message.

regards
Philipp

2020-03-31 17:31:24

[permalink] [raw]

Subject: Re: [PATCH 02/12] devicetree: bindings: pci: add missing clks to qcom,pcie

On Fri, Mar 20, 2020 at 07:34:44PM +0100, Ansuel Smith wrote:
> Document missing clks used in ipq806x soc
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/qcom,pcie.txt | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)

What a mess the clocks are for this binding...

Oh well,

Acked-by: Rob Herring <[email protected]>

>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 981b4de12807..becdbdc0fffa 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -90,6 +90,8 @@
> Definition: Should contain the following entries
> - "core" Clocks the pcie hw block
> - "phy" Clocks the pcie PHY block
> + - "aux" Clocks the pcie AUX block
> + - "ref" Clocks the pcie ref block
> - clock-names:
> Usage: required for apq8084/ipq4019
> Value type: <stringlist>
> @@ -277,8 +279,10 @@
> <0 0 0 4 &intc 0 39 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> clocks = <&gcc PCIE_A_CLK>,
> <&gcc PCIE_H_CLK>,
> - <&gcc PCIE_PHY_CLK>;
> - clock-names = "core", "iface", "phy";
> + <&gcc PCIE_PHY_CLK>,
> + <&gcc PCIE_AUX_CLK>,
> + <&gcc PCIE_ALT_REF_CLK>;
> + clock-names = "core", "iface", "phy", "aux", "ref";
> resets = <&gcc PCIE_ACLK_RESET>,
> <&gcc PCIE_HCLK_RESET>,
> <&gcc PCIE_POR_RESET>,
> --
> 2.25.1
>

2020-03-31 17:32:55

[permalink] [raw]

Subject: Re: [PATCH 06/12] devicetree: bindings: pci: add ext reset to qcom,pcie

On Fri, 20 Mar 2020 19:34:48 +0100, Ansuel Smith wrote:
> Document ext reset used in ipq806x soc by qcom pcie driver
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/qcom,pcie.txt | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>

Acked-by: Rob Herring <[email protected]>

2020-03-31 17:34:32

[permalink] [raw]

Subject: Re: [PATCH 11/12] devicetree: bindings: pci: add force_gen1 for qcom,pcie

On Fri, Mar 20, 2020 at 07:34:53PM +0100, Ansuel Smith wrote:
> Document force_gen1 optional definition to limit pcie
> line to GEN1 speed
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/qcom,pcie.txt | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 8c1d014f37b0..766876465c42 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -260,6 +260,11 @@
> Definition: If not defined is 0. In ipq806x is set to 7. In newer
> revision (v2.0) the offset is zero.
>
> +- force_gen1:
> + Usage: optional
> + Value type: <u32>
> + Definition: Set 1 to force the pcie line to GEN1
> +

I believe we have a standard property 'link-speed' for this purpose.

> * Example for ipq/apq8064
> pcie@1b500000 {
> compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";
> --
> 2.25.1
>

2020-03-31 17:34:40

[permalink] [raw]

Subject: Re: [PATCH 08/12] devicetree: bindings: pci: add phy-tx0-term-offset to qcom,pcie

On Fri, Mar 20, 2020 at 07:34:50PM +0100, Ansuel Smith wrote:
> Document phy-tx0-term-offset propriety to qcom pcie driver

propriety?

>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/qcom,pcie.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 6efcef040741..8c1d014f37b0 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -254,6 +254,12 @@
> - "perst-gpios" PCIe endpoint reset signal line
> - "wake-gpios" PCIe endpoint wake signal line
>
> +- phy-tx0-term-offset:

Needs a vendor prefix.

> + Usage: optional
> + Value type: <u32>
> + Definition: If not defined is 0. In ipq806x is set to 7. In newer
> + revision (v2.0) the offset is zero.
> +
> * Example for ipq/apq8064
> pcie@1b500000 {
> compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";
> @@ -293,6 +299,7 @@
> reset-names = "axi", "ahb", "por", "pci", "phy", "ext";
> pinctrl-0 = <&pcie_pins_default>;
> pinctrl-names = "default";
> + phy-tx0-term-offset = <7>;
> };
>
> * Example for apq8084
> --
> 2.25.1
>

2020-04-01 12:09:51

[permalink] [raw]

Subject: R: [PATCH 11/12] devicetree: bindings: pci: add force_gen1 for qcom,pcie

> -----Messaggio originale-----
> Da: Rob Herring <[email protected]>
> Inviato: marted? 31 marzo 2020 19:34
> A: Ansuel Smith <[email protected]>
> Cc: Stanimir Varbanov <[email protected]>; Andy Gross
> <[email protected]>; Bjorn Andersson <[email protected]>;
> Bjorn Helgaas <[email protected]>; Mark Rutland
> <[email protected]>; Lorenzo Pieralisi <[email protected]>;
> Andrew Murray <[email protected]>; Philipp Zabel
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]
> Oggetto: Re: [PATCH 11/12] devicetree: bindings: pci: add force_gen1 for
> qcom,pcie
>
> On Fri, Mar 20, 2020 at 07:34:53PM +0100, Ansuel Smith wrote:
> > Document force_gen1 optional definition to limit pcie
> > line to GEN1 speed
> >
> > Signed-off-by: Ansuel Smith <[email protected]>
> > ---
> > Documentation/devicetree/bindings/pci/qcom,pcie.txt | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > index 8c1d014f37b0..766876465c42 100644
> > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > @@ -260,6 +260,11 @@
> > Definition: If not defined is 0. In ipq806x is set to 7. In newer
> > revision (v2.0) the offset is zero.
> >
> > +- force_gen1:
> > + Usage: optional
> > + Value type: <u32>
> > + Definition: Set 1 to force the pcie line to GEN1
> > +
>
> I believe we have a standard property 'link-speed' for this purpose.
>

Yes this will be dropped in v2

> > * Example for ipq/apq8064
> > pcie@1b500000 {
> > compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064",
> "snps,dw-pcie";
> > --
> > 2.25.1
> >

2020-04-01 12:11:05

[permalink] [raw]

Subject: R: [PATCH 08/12] devicetree: bindings: pci: add phy-tx0-term-offset to qcom,pcie

> -----Messaggio originale-----
> Da: Rob Herring <[email protected]>
> Inviato: marted? 31 marzo 2020 19:33
> A: Ansuel Smith <[email protected]>
> Cc: Stanimir Varbanov <[email protected]>; Andy Gross
> <[email protected]>; Bjorn Andersson <[email protected]>;
> Bjorn Helgaas <[email protected]>; Mark Rutland
> <[email protected]>; Lorenzo Pieralisi <[email protected]>;
> Andrew Murray <[email protected]>; Philipp Zabel
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]
> Oggetto: Re: [PATCH 08/12] devicetree: bindings: pci: add phy-tx0-term-
> offset to qcom,pcie
>
> On Fri, Mar 20, 2020 at 07:34:50PM +0100, Ansuel Smith wrote:
> > Document phy-tx0-term-offset propriety to qcom pcie driver
>
> propriety?
>
> >
> > Signed-off-by: Ansuel Smith <[email protected]>
> > ---
> > Documentation/devicetree/bindings/pci/qcom,pcie.txt | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > index 6efcef040741..8c1d014f37b0 100644
> > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > @@ -254,6 +254,12 @@
> > - "perst-gpios" PCIe endpoint reset signal line
> > - "wake-gpios" PCIe endpoint wake signal line
> >
> > +- phy-tx0-term-offset:
>
> Needs a vendor prefix.
>

So I should change to qcom,phy-tx0-term-offset right?

> > + Usage: optional
> > + Value type: <u32>
> > + Definition: If not defined is 0. In ipq806x is set to 7. In newer
> > + revision (v2.0) the offset is zero.
> > +
> > * Example for ipq/apq8064
> > pcie@1b500000 {
> > compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064",
> "snps,dw-pcie";
> > @@ -293,6 +299,7 @@
> > reset-names = "axi", "ahb", "por", "pci", "phy", "ext";
> > pinctrl-0 = <&pcie_pins_default>;
> > pinctrl-names = "default";
> > + phy-tx0-term-offset = <7>;
> > };
> >
> > * Example for apq8084
> > --
> > 2.25.1
> >

2020-04-01 13:02:49

by Stanimir Varbanov

[permalink] [raw]

Subject: Re: [PATCH 01/12] pcie: qcom: add missing ipq806x clocks in pcie driver

Hi Ansuel,

As Bjorn already mentioned please make cover-letter in next version of
the patchset so that we know what is the purpose of the patches.

I've the impression that you want to use pcie-qcom platform driver as an
endpoint, am I wrong?

I'll review the patches these days.

On 3/20/20 8:34 PM, Ansuel Smith wrote:
> Aux and Ref clk are missing in pcie qcom driver.
> Add support in the driver to fix pcie inizialization
> in ipq806x
>
> Signed-off-by: Sham Muthayyan <[email protected]>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 38 ++++++++++++++++++++++----
> 1 file changed, 33 insertions(+), 5 deletions(-)

--
regards,
Stan

2020-04-01 13:19:11

by Stanimir Varbanov

[permalink] [raw]

Subject: Re: [PATCH 11/12] devicetree: bindings: pci: add force_gen1 for qcom,pcie

Hi Ansuel,

Before inventing new DT property I'd suggest you to consult with [1].
There is already property max-link-speed for that purpose.

On 3/20/20 8:34 PM, Ansuel Smith wrote:
> Document force_gen1 optional definition to limit pcie
> line to GEN1 speed
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/qcom,pcie.txt | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 8c1d014f37b0..766876465c42 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -260,6 +260,11 @@
> Definition: If not defined is 0. In ipq806x is set to 7. In newer
> revision (v2.0) the offset is zero.
>
> +- force_gen1:
> + Usage: optional
> + Value type: <u32>
> + Definition: Set 1 to force the pcie line to GEN1
> +
> * Example for ipq/apq8064
> pcie@1b500000 {
> compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";
>

--
regards,
Stan

[1] Documentation/devicetree/bindings/pci/pci.txt

2020-04-01 13:21:56

by Stanimir Varbanov

[permalink] [raw]

Subject: Re: [PATCH 09/12] pcie: qcom: Programming the PCIE iATU for IPQ806x

Hi Ansuel,

On 3/20/20 8:34 PM, Ansuel Smith wrote:
> From: Sham Muthayyan <[email protected]>
>
> Resolved PCIE EP detection errors caused due to missing iATU programming.

NACK, the iATU programing is not belonging here. Did you check what
pcie-designware-*.c is doing with iATU?

If you want to support endpoint mode in pcie-qcom driver you have to see
how the other drivers is doing that.

>
> Signed-off-by: Sham Muthayyan <[email protected]>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 78 ++++++++++++++++++++++++++
> 1 file changed, 78 insertions(+)
>

--
regards,
Stan

2020-04-01 20:50:08

by Bjorn Andersson

[permalink] [raw]

Subject: Re: [PATCH 08/12] devicetree: bindings: pci: add phy-tx0-term-offset to qcom,pcie

On Fri 20 Mar 11:34 PDT 2020, Ansuel Smith wrote:

> Document phy-tx0-term-offset propriety to qcom pcie driver
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/qcom,pcie.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 6efcef040741..8c1d014f37b0 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -254,6 +254,12 @@
> - "perst-gpios" PCIe endpoint reset signal line
> - "wake-gpios" PCIe endpoint wake signal line
>
> +- phy-tx0-term-offset:

If I understand your implementation correctly this difference in
hardware revision should be encoded in the compatible string.

Regards,
Bjorn

> + Usage: optional
> + Value type: <u32>
> + Definition: If not defined is 0. In ipq806x is set to 7. In newer
> + revision (v2.0) the offset is zero.
> +
> * Example for ipq/apq8064
> pcie@1b500000 {
> compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";
> @@ -293,6 +299,7 @@
> reset-names = "axi", "ahb", "por", "pci", "phy", "ext";
> pinctrl-0 = <&pcie_pins_default>;
> pinctrl-names = "default";
> + phy-tx0-term-offset = <7>;
> };
>
> * Example for apq8084
> --
> 2.25.1
>

2020-04-01 20:50:27

by Bjorn Andersson

[permalink] [raw]

Subject: Re: [PATCH 07/12] pcie: qcom: add tx term offset support

On Fri 20 Mar 11:34 PDT 2020, Ansuel Smith wrote:

> From: Sham Muthayyan <[email protected]>
>
> Add tx term offset support to pcie qcom driver
> need in some revision of the ipq806x soc
>
> Signed-off-by: Sham Muthayyan <[email protected]>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 61 ++++++++++++++++++++++----
> 1 file changed, 52 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index ecc22fd27ea6..8009e3117765 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -45,7 +45,13 @@
> #define PCIE_CAP_CPL_TIMEOUT_DISABLE 0x10
>
> #define PCIE20_PARF_PHY_CTRL 0x40
> +#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK (0x1f << 16)
> +#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x) (x << 16)
> +
> #define PCIE20_PARF_PHY_REFCLK 0x4C
> +#define REF_SSP_EN BIT(16)
> +#define REF_USE_PAD BIT(12)
> +
> #define PCIE20_PARF_DBI_BASE_ADDR 0x168
> #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE 0x16C
> #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL 0x174
> @@ -77,6 +83,18 @@
> #define DBI_RO_WR_EN 1
>
> #define PERST_DELAY_US 1000
> +/* PARF registers */
> +#define PCIE20_PARF_PCS_DEEMPH 0x34
> +#define PCS_DEEMPH_TX_DEEMPH_GEN1(x) (x << 16)
> +#define PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(x) (x << 8)
> +#define PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(x) (x << 0)
> +
> +#define PCIE20_PARF_PCS_SWING 0x38
> +#define PCS_SWING_TX_SWING_FULL(x) (x << 8)
> +#define PCS_SWING_TX_SWING_LOW(x) (x << 0)
> +
> +#define PCIE20_PARF_CONFIG_BITS 0x50
> +#define PHY_RX0_EQ(x) (x << 24)
>
> #define PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE 0x358
> #define SLV_ADDR_SPACE_SZ 0x10000000
> @@ -97,6 +115,7 @@ struct qcom_pcie_resources_2_1_0 {
> struct reset_control *phy_reset;
> struct reset_control *ext_reset;
> struct regulator_bulk_data supplies[QCOM_PCIE_2_1_0_MAX_SUPPLY];
> + uint8_t phy_tx0_term_offset;
> };
>
> struct qcom_pcie_resources_1_0_0 {
> @@ -184,6 +203,16 @@ struct qcom_pcie {
>
> #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
>
> +static inline void
> +writel_masked(void __iomem *addr, u32 clear_mask, u32 set_mask)
> +{
> + u32 val = readl(addr);
> +
> + val &= ~clear_mask;
> + val |= set_mask;
> + writel(val, addr);
> +}
> +
> static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> {
> gpiod_set_value_cansleep(pcie->reset, 1);
> @@ -277,6 +306,10 @@ static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
> if (IS_ERR(res->ext_reset))
> return PTR_ERR(res->ext_reset);
>
> + if (of_property_read_u8(dev->of_node, "phy-tx0-term-offset",
> + &res->phy_tx0_term_offset))
> + res->phy_tx0_term_offset = 0;

The appropriate way is to encode differences in hardware is to use
different compatibles for the two different versions of the hardware.

Regards,
Bjorn

> +
> res->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
> return PTR_ERR_OR_ZERO(res->phy_reset);
> }
> @@ -304,7 +337,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0;
> struct dw_pcie *pci = pcie->pci;
> struct device *dev = pci->dev;
> - u32 val;
> int ret;
>
> ret = reset_control_assert(res->ahb_reset);
> @@ -355,15 +387,26 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> goto err_deassert_ahb;
> }
>
> - /* enable PCIe clocks and resets */
> - val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> - val &= ~BIT(0);
> - writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> + writel_masked(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
> +
> + /* Set Tx termination offset */
> + writel_masked(pcie->parf + PCIE20_PARF_PHY_CTRL,
> + PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK,
> + PHY_CTRL_PHY_TX0_TERM_OFFSET(res->phy_tx0_term_offset));
> +
> + /* PARF programming */
> + writel(PCS_DEEMPH_TX_DEEMPH_GEN1(0x18) |
> + PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(0x18) |
> + PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(0x22),
> + pcie->parf + PCIE20_PARF_PCS_DEEMPH);
> + writel(PCS_SWING_TX_SWING_FULL(0x78) |
> + PCS_SWING_TX_SWING_LOW(0x78),
> + pcie->parf + PCIE20_PARF_PCS_SWING);
> + writel(PHY_RX0_EQ(0x4), pcie->parf + PCIE20_PARF_CONFIG_BITS);
>
> - /* enable external reference clock */
> - val = readl(pcie->parf + PCIE20_PARF_PHY_REFCLK);
> - val |= BIT(16);
> - writel(val, pcie->parf + PCIE20_PARF_PHY_REFCLK);
> + /* Enable reference clock */
> + writel_masked(pcie->parf + PCIE20_PARF_PHY_REFCLK,
> + REF_USE_PAD, REF_SSP_EN);
>
> ret = reset_control_deassert(res->phy_reset);
> if (ret) {
> --
> 2.25.1
>

2020-04-01 21:55:56