2020-04-02 12:13:47

by Christian Marangi

[permalink] [raw]
Subject: [PATCH v2 00/10] Multiple fixes in PCIe qcom driver

This contains multiple fix for PCIe qcom driver.
Some optional reset and clocks were missing.
Fix a problem with no PARF programming that cause kernel lock on load.
Add support to force gen 1 speed if needed. (due to hardware limitation)
Add ipq8064 rev 2 support that use a different tx termination offset.

v2:
* Drop iATU programming (already done in pcie init)
* Use max-link-speed instead of force-gen1 custom definition
* Drop MRRS to 256B (Can't find a realy reason why this was suggested)
* Introduce a new variant for different revision of ipq8064

Abhishek Sahu (1):
PCIe: qcom: change duplicate PCI reset to phy reset

Ansuel Smith (7):
PCIe: qcom: add missing ipq806x clocks in PCIe driver
devicetree: bindings: pci: add missing clks to qcom,pcie
PCIe: qcom: Fixed pcie_phy_clk branch issue
PCIe: qcom: add missing reset for ipq806x
devicetree: bindings: pci: add ext reset to qcom,pcie
PCIe: qcom: fix init problem with missing PARF programming
devicetree: bindings: pci: add ipq8064 rev 2 variant to qcom,pcie

Sham Muthayyan (2):
PCIe: qcom: add ipq8064 rev2 variant and set tx term offset
PCIe: qcom: add Force GEN1 support

.../devicetree/bindings/pci/qcom,pcie.txt | 56 +++++++-
drivers/pci/controller/dwc/pcie-qcom.c | 134 +++++++++++++++---
2 files changed, 167 insertions(+), 23 deletions(-)

--
2.25.1


2020-04-02 12:13:51

by Christian Marangi

[permalink] [raw]
Subject: [PATCH v2 07/10] PCIe: qcom: fix init problem with missing PARF programming

PARF programming was missing and this cause initilizzation problem on
some ipq806x based device (Netgear R7800 for example). This cause a
total lock of the system on kernel load.

Fixes: 82a82383 PCI: qcom: Add Qualcomm PCIe controller driver
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/pci/controller/dwc/pcie-qcom.c | 48 +++++++++++++++++++++-----
1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 211a1aa7d0f1..77b1ab7e23a3 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -46,6 +46,9 @@

#define PCIE20_PARF_PHY_CTRL 0x40
#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 +80,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
@@ -184,6 +199,16 @@ struct qcom_pcie {

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

+static inline void qcom_clear_and_set_dword(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);
@@ -304,7 +329,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 = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
@@ -355,15 +379,21 @@ 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);
+ qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
+
+ /* 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 */
+ qcom_clear_and_set_dword(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-02 12:14:43

by Christian Marangi

[permalink] [raw]
Subject: [PATCH v2 05/10] PCIe: qcom: add missing reset for ipq806x

Add missing ext reset used by ipq806x SoC in PCIe qcom driver.

Fixes: 82a82383 PCI: qcom: Add Qualcomm PCIe controller driver
Signed-off-by: Sham Muthayyan <[email protected]>
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/pci/controller/dwc/pcie-qcom.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 596731b54728..211a1aa7d0f1 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_exclusive(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);
@@ -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-04-02 12:17:05

by Christian Marangi

[permalink] [raw]
Subject: [PATCH v2 02/10] devicetree: bindings: pci: add missing clks to qcom,pcie

Document missing clks used in ipq806x soc.

Signed-off-by: Ansuel Smith <[email protected]>
Acked-by: Rob Herring <[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-04-03 09:03:01

by Stanimir Varbanov

[permalink] [raw]
Subject: Re: [PATCH v2 00/10] Multiple fixes in PCIe qcom driver

Hi Ansuel,

Please run "checkpatch --strict" for the next version.

On 4/2/20 3:11 PM, Ansuel Smith wrote:
> This contains multiple fix for PCIe qcom driver.
> Some optional reset and clocks were missing.
> Fix a problem with no PARF programming that cause kernel lock on load.
> Add support to force gen 1 speed if needed. (due to hardware limitation)
> Add ipq8064 rev 2 support that use a different tx termination offset.
>
> v2:
> * Drop iATU programming (already done in pcie init)
> * Use max-link-speed instead of force-gen1 custom definition
> * Drop MRRS to 256B (Can't find a realy reason why this was suggested)
> * Introduce a new variant for different revision of ipq8064
>
> Abhishek Sahu (1):
> PCIe: qcom: change duplicate PCI reset to phy reset
>
> Ansuel Smith (7):
> PCIe: qcom: add missing ipq806x clocks in PCIe driver
> devicetree: bindings: pci: add missing clks to qcom,pcie
> PCIe: qcom: Fixed pcie_phy_clk branch issue
> PCIe: qcom: add missing reset for ipq806x
> devicetree: bindings: pci: add ext reset to qcom,pcie
> PCIe: qcom: fix init problem with missing PARF programming
> devicetree: bindings: pci: add ipq8064 rev 2 variant to qcom,pcie
>
> Sham Muthayyan (2):
> PCIe: qcom: add ipq8064 rev2 variant and set tx term offset
> PCIe: qcom: add Force GEN1 support
>
> .../devicetree/bindings/pci/qcom,pcie.txt | 56 +++++++-
> drivers/pci/controller/dwc/pcie-qcom.c | 134 +++++++++++++++---
> 2 files changed, 167 insertions(+), 23 deletions(-)
>

--
regards,
Stan

2020-04-08 09:42:32

by Stanimir Varbanov

[permalink] [raw]
Subject: Re: [PATCH v2 07/10] PCIe: qcom: fix init problem with missing PARF programming

Hi Ansuel,

Please fix the patch subject for all patches in the series per Bjorn H.
request.

PCI: qcom: Fix init problem with missing PARF programming

Also the patch subject is misleading to me. Actually you change few phy
parameters: Tx De-Emphasis, Tx Swing and Rx equalization. On the other
side I guess those parameters are board specific and I'm not sure how
this change will reflect on apq8064 boards.

On 4/2/20 3:11 PM, Ansuel Smith wrote:
> PARF programming was missing and this cause initilizzation problem on
> some ipq806x based device (Netgear R7800 for example). This cause a
> total lock of the system on kernel load.
>
> Fixes: 82a82383 PCI: qcom: Add Qualcomm PCIe controller driver
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 48 +++++++++++++++++++++-----
> 1 file changed, 39 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 211a1aa7d0f1..77b1ab7e23a3 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -46,6 +46,9 @@
>
> #define PCIE20_PARF_PHY_CTRL 0x40
> #define PCIE20_PARF_PHY_REFCLK 0x4C
> +#define REF_SSP_EN BIT(16)
> +#define REF_USE_PAD BIT(12)

Could you rename this to:

PHY_REFCLK_SSP_EN
PHY_REFCLK_USE_PAD

> +
> #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 +80,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
> @@ -184,6 +199,16 @@ struct qcom_pcie {
>
> #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
>
> +static inline void qcom_clear_and_set_dword(void __iomem *addr,

drop 'inline' the compiler is smart enough to decide.

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

If you add such function you should introduce it in a separate patch and
use it in the whole driver where it is applicable. After that we can see
what is the benefit of it.

> static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> {
> gpiod_set_value_cansleep(pcie->reset, 1);
> @@ -304,7 +329,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 = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> @@ -355,15 +379,21 @@ 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);
> + qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);

please keep the comment.

> +
> + /* PARF programming */

pointless comment, please drop it.

> + 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 */

Why you dropped 'external' ?

> + qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_REFCLK,
> + REF_USE_PAD, REF_SSP_EN);
>
> ret = reset_control_deassert(res->phy_reset);
> if (ret) {
>

--
regards,
Stan

2020-04-08 13:49:03

by Christian Marangi

[permalink] [raw]
Subject: R: [PATCH v2 07/10] PCIe: qcom: fix init problem with missing PARF programming

> PARF programming
>
> Hi Ansuel,
>
> Please fix the patch subject for all patches in the series per Bjorn H.
> request.
>
> PCI: qcom: Fix init problem with missing PARF programming
>
> Also the patch subject is misleading to me. Actually you change few phy
> parameters: Tx De-Emphasis, Tx Swing and Rx equalization. On the other
> side I guess those parameters are board specific and I'm not sure how
> this change will reflect on apq8064 boards.
>

I also think that this would brake apq8064, on ipq8064 this is needed or
the system doesn't boot.
Should I move this to the dts and set this params only if they are present
in dts or also here check for compatible and set accordingly?

> On 4/2/20 3:11 PM, Ansuel Smith wrote:
> > PARF programming was missing and this cause initilizzation problem on
> > some ipq806x based device (Netgear R7800 for example). This cause a
> > total lock of the system on kernel load.
> >
> > Fixes: 82a82383 PCI: qcom: Add Qualcomm PCIe controller driver
> > Signed-off-by: Ansuel Smith <[email protected]>
> > ---
> > drivers/pci/controller/dwc/pcie-qcom.c | 48 +++++++++++++++++++++--
> ---
> > 1 file changed, 39 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c
> b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 211a1aa7d0f1..77b1ab7e23a3 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -46,6 +46,9 @@
> >
> > #define PCIE20_PARF_PHY_CTRL 0x40
> > #define PCIE20_PARF_PHY_REFCLK 0x4C
> > +#define REF_SSP_EN BIT(16)
> > +#define REF_USE_PAD BIT(12)
>
> Could you rename this to:
>
> PHY_REFCLK_SSP_EN
> PHY_REFCLK_USE_PAD
>
> > +
> > #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 +80,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
> > @@ -184,6 +199,16 @@ struct qcom_pcie {
> >
> > #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
> >
> > +static inline void qcom_clear_and_set_dword(void __iomem *addr,
>
> drop 'inline' the compiler is smart enough to decide.
>
> > + u32 clear_mask, u32 set_mask)
> > +{
> > + u32 val = readl(addr);
> > +
> > + val &= ~clear_mask;
> > + val |= set_mask;
> > + writel(val, addr);
> > +}
> > +
>
> If you add such function you should introduce it in a separate patch and
> use it in the whole driver where it is applicable. After that we can see
> what is the benefit of it.
>
> > static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> > {
> > gpiod_set_value_cansleep(pcie->reset, 1);
> > @@ -304,7 +329,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 = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res-
> >supplies);
> > @@ -355,15 +379,21 @@ 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);
> > + qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL,
> BIT(0), 0);
>
> please keep the comment.
>
> > +
> > + /* PARF programming */
>
> pointless comment, please drop it.
>
> > + 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 */
>
> Why you dropped 'external' ?
>
> > + qcom_clear_and_set_dword(pcie->parf +
> PCIE20_PARF_PHY_REFCLK,
> > + REF_USE_PAD, REF_SSP_EN);
> >
> > ret = reset_control_deassert(res->phy_reset);
> > if (ret) {
> >
>
> --
> regards,
> Stan

2020-04-08 13:51:11

by Stanimir Varbanov

[permalink] [raw]
Subject: Re: R: [PATCH v2 07/10] PCIe: qcom: fix init problem with missing PARF programming

Hi Ansuel,

On 4/8/20 3:38 PM, [email protected] wrote:
>> PARF programming
>>
>> Hi Ansuel,
>>
>> Please fix the patch subject for all patches in the series per Bjorn H.
>> request.
>>
>> PCI: qcom: Fix init problem with missing PARF programming
>>
>> Also the patch subject is misleading to me. Actually you change few phy
>> parameters: Tx De-Emphasis, Tx Swing and Rx equalization. On the other
>> side I guess those parameters are board specific and I'm not sure how
>> this change will reflect on apq8064 boards.
>>
>
> I also think that this would brake apq8064, on ipq8064 this is needed or
> the system doesn't boot.
> Should I move this to the dts and set this params only if they are present
> in dts or also here check for compatible and set accordingly?
>

I also think that these phy params should be per board (and they are
tunable).

Maybe you can propose those as generic phy params in pci.txt binding
document and then we can ask DT maintainers for opinion. If they refuse
such generic bindings, we could switch to custom qcom,phy properties.

--
regards,
Stan