diff mbox series

[v2,07/10] PCIe: qcom: fix init problem with missing PARF programming

Message ID 20200402121148.1767-8-ansuelsmth@gmail.com
State New
Headers show
Series Multiple fixes in PCIe qcom driver | expand

Commit Message

Ansuel Smith April 2, 2020, 12:11 p.m. UTC
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 <ansuelsmth@gmail.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 48 +++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 9 deletions(-)

Comments

Stanimir Varbanov April 8, 2020, 8:50 a.m. UTC | #1
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 <ansuelsmth@gmail.com>
> ---
>  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) {
>
Ansuel Smith April 8, 2020, 12:38 p.m. UTC | #2
> 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 <ansuelsmth@gmail.com>
> > ---
> >  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
Stanimir Varbanov April 8, 2020, 1:18 p.m. UTC | #3
Hi Ansuel,

On 4/8/20 3:38 PM, ansuelsmth@gmail.com 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.
diff mbox series

Patch

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) {