All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Robert Marko <robimarko@gmail.com>
Cc: svarbanov@mm-sol.com, agross@kernel.org,
	bjorn.andersson@linaro.org, lpieralisi@kernel.org,
	robh@kernel.org, kw@linux.com, bhelgaas@google.com,
	p.zabel@pengutronix.de, jingoohan1@gmail.com,
	linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org, johan+linaro@kernel.org,
	dmitry.baryshkov@linaro.org
Subject: Re: [PATCH v3 2/2] PCI: qcom: move register accesses to .post_init
Date: Thu, 23 Jun 2022 16:55:31 -0500	[thread overview]
Message-ID: <20220623215531.GA1479475@bhelgaas> (raw)
In-Reply-To: <20220623155004.688090-2-robimarko@gmail.com>

On Thu, Jun 23, 2022 at 05:50:04PM +0200, Robert Marko wrote:
> Move register accesses from .init to .post_init callbacks to maintain
> consinstency for all IP since IPQ8074 specifically requires PHY-s to be
> powered on before register access and its accesses have been moved to
> .post_init.

This doesn't do the corresponding move for qcom_pcie_init_2_7_0().  Is
that intentional or an oversight?

> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 171 ++++++++++++++-----------
>  1 file changed, 97 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 24708d5d817d..1aa11f12c069 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -348,8 +348,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;
> -	struct device_node *node = dev->of_node;
> -	u32 val;
>  	int ret;
>  
>  	/* reset the PCIe interface as uboot can leave it undefined state */
> @@ -360,8 +358,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
>  	reset_control_assert(res->ext_reset);
>  	reset_control_assert(res->phy_reset);
>  
> -	writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> -
>  	ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
>  	if (ret < 0) {
>  		dev_err(dev, "cannot enable regulators\n");
> @@ -408,6 +404,35 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
>  	if (ret)
>  		goto err_clks;
>  
> +	return 0;
> +
> +err_clks:
> +	reset_control_assert(res->axi_reset);
> +err_deassert_axi:
> +	reset_control_assert(res->por_reset);
> +err_deassert_por:
> +	reset_control_assert(res->pci_reset);
> +err_deassert_pci:
> +	reset_control_assert(res->phy_reset);
> +err_deassert_phy:
> +	reset_control_assert(res->ext_reset);
> +err_deassert_ext:
> +	reset_control_assert(res->ahb_reset);
> +err_deassert_ahb:
> +	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> +
> +	return ret;
> +}
> +
> +static int qcom_pcie_post_init_2_1_0(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	struct device *dev = pci->dev;
> +	struct device_node *node = dev->of_node;
> +	u32 val;
> +
> +	writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> +
>  	/* enable PCIe clocks and resets */
>  	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
>  	val &= ~BIT(0);
> @@ -451,23 +476,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
>  	       pci->dbi_base + PCIE20_AXI_MSTR_RESP_COMP_CTRL1);
>  
>  	return 0;
> -
> -err_clks:
> -	reset_control_assert(res->axi_reset);
> -err_deassert_axi:
> -	reset_control_assert(res->por_reset);
> -err_deassert_por:
> -	reset_control_assert(res->pci_reset);
> -err_deassert_pci:
> -	reset_control_assert(res->phy_reset);
> -err_deassert_phy:
> -	reset_control_assert(res->ext_reset);
> -err_deassert_ext:
> -	reset_control_assert(res->ahb_reset);
> -err_deassert_ahb:
> -	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> -
> -	return ret;
>  }
>  
>  static int qcom_pcie_get_resources_1_0_0(struct qcom_pcie *pcie)
> @@ -555,16 +563,6 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
>  		goto err_slave;
>  	}
>  
> -	/* change DBI base address */
> -	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> -
> -	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> -		u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> -
> -		val |= BIT(31);
> -		writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> -	}
> -
>  	return 0;
>  err_slave:
>  	clk_disable_unprepare(res->slave_bus);
> @@ -580,6 +578,22 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
>  	return ret;
>  }
>  
> +static int qcom_pcie_post_init_1_0_0(struct qcom_pcie *pcie)
> +{
> +
> +	/* change DBI base address */
> +	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> +
> +		val |= BIT(31);
> +		writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> +	}
> +
> +	return 0;
> +}
> +
>  static void qcom_pcie_2_3_2_ltssm_enable(struct qcom_pcie *pcie)
>  {
>  	u32 val;
> @@ -648,7 +662,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
>  	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);
> @@ -681,27 +694,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
>  		goto err_slave_clk;
>  	}
>  
> -	/* enable PCIe clocks and resets */
> -	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> -	val &= ~BIT(0);
> -	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> -
> -	/* change DBI base address */
> -	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> -
> -	/* MAC PHY_POWERDOWN MUX DISABLE  */
> -	val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
> -	val &= ~BIT(29);
> -	writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
> -
> -	val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> -	val |= BIT(4);
> -	writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> -
> -	val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> -	val |= BIT(31);
> -	writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> -
>  	return 0;
>  
>  err_slave_clk:
> @@ -722,8 +714,30 @@ static int qcom_pcie_post_init_2_3_2(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
> +	u32 val;
>  	int ret;
>  
> +	/* enable PCIe clocks and resets */
> +	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> +	val &= ~BIT(0);
> +	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> +
> +	/* change DBI base address */
> +	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> +
> +	/* MAC PHY_POWERDOWN MUX DISABLE  */
> +	val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
> +	val &= ~BIT(29);
> +	writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
> +
> +	val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> +	val |= BIT(4);
> +	writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> +
> +	val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> +	val |= BIT(31);
> +	writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> +
>  	ret = clk_prepare_enable(res->pipe_clk);
>  	if (ret) {
>  		dev_err(dev, "cannot prepare/enable pipe clock\n");
> @@ -837,7 +851,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
> -	u32 val;
>  	int ret;
>  
>  	ret = reset_control_assert(res->axi_m_reset);
> @@ -962,6 +975,33 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>  	if (ret)
>  		goto err_clks;
>  
> +	return 0;
> +
> +err_clks:
> +	reset_control_assert(res->ahb_reset);
> +err_rst_ahb:
> +	reset_control_assert(res->pwr_reset);
> +err_rst_pwr:
> +	reset_control_assert(res->axi_s_reset);
> +err_rst_axi_s:
> +	reset_control_assert(res->axi_m_sticky_reset);
> +err_rst_axi_m_sticky:
> +	reset_control_assert(res->axi_m_reset);
> +err_rst_axi_m:
> +	reset_control_assert(res->pipe_sticky_reset);
> +err_rst_pipe_sticky:
> +	reset_control_assert(res->pipe_reset);
> +err_rst_pipe:
> +	reset_control_assert(res->phy_reset);
> +err_rst_phy:
> +	reset_control_assert(res->phy_ahb_reset);
> +	return ret;
> +}
> +
> +static int qcom_pcie_post_init_2_4_0(struct qcom_pcie *pcie)
> +{
> +	u32 val;
> +
>  	/* enable PCIe clocks and resets */
>  	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
>  	val &= ~BIT(0);
> @@ -984,26 +1024,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>  	writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
>  
>  	return 0;
> -
> -err_clks:
> -	reset_control_assert(res->ahb_reset);
> -err_rst_ahb:
> -	reset_control_assert(res->pwr_reset);
> -err_rst_pwr:
> -	reset_control_assert(res->axi_s_reset);
> -err_rst_axi_s:
> -	reset_control_assert(res->axi_m_sticky_reset);
> -err_rst_axi_m_sticky:
> -	reset_control_assert(res->axi_m_reset);
> -err_rst_axi_m:
> -	reset_control_assert(res->pipe_sticky_reset);
> -err_rst_pipe_sticky:
> -	reset_control_assert(res->pipe_reset);
> -err_rst_pipe:
> -	reset_control_assert(res->phy_reset);
> -err_rst_phy:
> -	reset_control_assert(res->phy_ahb_reset);
> -	return ret;
>  }
>  
>  static int qcom_pcie_get_resources_2_3_3(struct qcom_pcie *pcie)
> @@ -1569,6 +1589,7 @@ static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
>  static const struct qcom_pcie_ops ops_2_1_0 = {
>  	.get_resources = qcom_pcie_get_resources_2_1_0,
>  	.init = qcom_pcie_init_2_1_0,
> +	.post_init = qcom_pcie_post_init_2_1_0,
>  	.deinit = qcom_pcie_deinit_2_1_0,
>  	.ltssm_enable = qcom_pcie_2_1_0_ltssm_enable,
>  };
> @@ -1577,6 +1598,7 @@ static const struct qcom_pcie_ops ops_2_1_0 = {
>  static const struct qcom_pcie_ops ops_1_0_0 = {
>  	.get_resources = qcom_pcie_get_resources_1_0_0,
>  	.init = qcom_pcie_init_1_0_0,
> +	.post_init = qcom_pcie_post_init_1_0_0,
>  	.deinit = qcom_pcie_deinit_1_0_0,
>  	.ltssm_enable = qcom_pcie_2_1_0_ltssm_enable,
>  };
> @@ -1595,6 +1617,7 @@ static const struct qcom_pcie_ops ops_2_3_2 = {
>  static const struct qcom_pcie_ops ops_2_4_0 = {
>  	.get_resources = qcom_pcie_get_resources_2_4_0,
>  	.init = qcom_pcie_init_2_4_0,
> +	.post_init = qcom_pcie_post_init_2_4_0,
>  	.deinit = qcom_pcie_deinit_2_4_0,
>  	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>  };
> -- 
> 2.36.1
> 

  reply	other threads:[~2022-06-23 21:55 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-23 15:50 [PATCH v3 1/2] PCI: qcom: fix IPQ8074 Gen2 support Robert Marko
2022-06-23 15:50 ` [PATCH v3 2/2] PCI: qcom: move register accesses to .post_init Robert Marko
2022-06-23 21:55   ` Bjorn Helgaas [this message]
2022-06-24 10:36     ` Robert Marko
2022-06-24 10:46       ` Robert Marko
2022-06-24 17:06         ` Bjorn Helgaas
2022-06-23 16:03 ` [PATCH v3 1/2] PCI: qcom: fix IPQ8074 Gen2 support Bjorn Helgaas
2022-06-23 16:17   ` Robert Marko

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=20220623215531.GA1479475@bhelgaas \
    --to=helgaas@kernel.org \
    --cc=agross@kernel.org \
    --cc=bhelgaas@google.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=dmitry.baryshkov@linaro.org \
    --cc=jingoohan1@gmail.com \
    --cc=johan+linaro@kernel.org \
    --cc=kw@linux.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lpieralisi@kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=robh@kernel.org \
    --cc=robimarko@gmail.com \
    --cc=svarbanov@mm-sol.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.