linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stanimir Varbanov <svarbanov@mm-sol.com>
To: Ansuel Smith <ansuelsmth@gmail.com>, Andy Gross <agross@kernel.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Andrew Murray <amurray@thegoodpenguin.co.uk>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 07/10] PCIe: qcom: fix init problem with missing PARF programming
Date: Wed, 8 Apr 2020 11:50:43 +0300	[thread overview]
Message-ID: <fea9cfd1-2bc7-0141-444e-9c781877ad02@mm-sol.com> (raw)
In-Reply-To: <20200402121148.1767-8-ansuelsmth@gmail.com>

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

-- 
regards,
Stan

  reply	other threads:[~2020-04-08  8:51 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-02 12:11 [PATCH v2 00/10] Multiple fixes in PCIe qcom driver Ansuel Smith
2020-04-02 12:11 ` [PATCH v2 01/10] PCIe: qcom: add missing ipq806x clocks in PCIe driver Ansuel Smith
2020-04-08  8:50   ` Stanimir Varbanov
2020-04-08 12:36     ` R: " ansuelsmth
2020-04-08 12:48       ` Stanimir Varbanov
2020-04-08 12:55         ` R: " ansuelsmth
2020-04-08 13:06           ` Stanimir Varbanov
2020-04-02 12:11 ` [PATCH v2 02/10] devicetree: bindings: pci: add missing clks to qcom,pcie Ansuel Smith
2020-04-02 12:11 ` [PATCH v2 03/10] PCIe: qcom: change duplicate PCI reset to phy reset Ansuel Smith
2020-04-02 12:11 ` [PATCH v2 04/10] PCIe: qcom: Fixed pcie_phy_clk branch issue Ansuel Smith
2020-04-02 12:11 ` [PATCH v2 05/10] PCIe: qcom: add missing reset for ipq806x Ansuel Smith
2020-04-02 12:11 ` [PATCH v2 06/10] devicetree: bindings: pci: add ext reset to qcom,pcie Ansuel Smith
2020-04-02 12:11 ` [PATCH v2 07/10] PCIe: qcom: fix init problem with missing PARF programming Ansuel Smith
2020-04-08  8:50   ` Stanimir Varbanov [this message]
2020-04-08 12:38     ` R: " ansuelsmth
2020-04-08 13:18       ` Stanimir Varbanov
2020-04-02 12:11 ` [PATCH v2 08/10] PCIe: qcom: add ipq8064 rev2 variant and set tx term offset Ansuel Smith
2020-04-02 12:11 ` [PATCH v2 09/10] devicetree: bindings: pci: add ipq8064 rev 2 variant to qcom,pcie Ansuel Smith
2020-04-14 17:07   ` Rob Herring
2020-04-02 12:11 ` [PATCH v2 10/10] PCIe: qcom: add Force GEN1 support Ansuel Smith
2020-04-03  9:01   ` Stanimir Varbanov
2020-04-14 17:09     ` Rob Herring
2020-04-03  9:01 ` [PATCH v2 00/10] Multiple fixes in PCIe qcom driver Stanimir Varbanov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fea9cfd1-2bc7-0141-444e-9c781877ad02@mm-sol.com \
    --to=svarbanov@mm-sol.com \
    --cc=agross@kernel.org \
    --cc=amurray@thegoodpenguin.co.uk \
    --cc=ansuelsmth@gmail.com \
    --cc=bhelgaas@google.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=p.zabel@pengutronix.de \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).