Received-SPF: pass (google.com: domain of linux-kernel+bounces-72313-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <39f28d21-e178-68df-c7b6-eef30f0584e3@quicinc.com>
Date: Tue, 20 Feb 2024 09:42:41 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH v2 3/3] PCI: qcom: properly implement RC shutdown/power up
Content-Language: en-US
To: Konrad Dybcio <konrad.dybcio@linaro.org>,
        Bjorn Andersson
	<andersson@kernel.org>,
        Manivannan Sadhasivam
	<manivannan.sadhasivam@linaro.org>,
        Lorenzo Pieralisi
	<lpieralisi@kernel.org>,
        =?UTF-8?Q?Krzysztof_Wilczy=c5=84ski?=
	<kw@linux.com>,
        Rob Herring <robh@kernel.org>, Bjorn Helgaas
	<bhelgaas@google.com>,
        Philipp Zabel <p.zabel@pengutronix.de>
CC: <linux-arm-msm@vger.kernel.org>, <linux-pci@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, Johan Hovold <johan+linaro@kernel.org>,
        "Bjorn Andersson" <quic_bjorande@quicinc.com>
References: <20240210-topic-8280_pcie-v2-0-1cef4b606883@linaro.org>
 <20240210-topic-8280_pcie-v2-3-1cef4b606883@linaro.org>
From: Krishna Chaitanya Chundru <quic_krichai@quicinc.com>
In-Reply-To: <20240210-topic-8280_pcie-v2-3-1cef4b606883@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 2/10/2024 10:40 PM, Konrad Dybcio wrote:
> Currently, we've only been minimizing the power draw while keeping the
> RC up at all times. This is suboptimal, as it draws a whole lot of power
> and prevents the SoC from power collapsing.
> 
> Implement full shutdown and re-initialization to allow for powering off
> the controller.
> 
> This is mainly indended for SC8280XP with a broken power rail setup,
> which requires a full RC shutdown/reinit in order to reach SoC-wide
> power collapse, but sleeping is generally better than not sleeping and
> less destructive suspend can be implemented later for platforms that
> support it.
> 
> Co-developed-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/pci/controller/dwc/Kconfig     |   1 +
>   drivers/pci/controller/dwc/pcie-qcom.c | 159 ++++++++++++++++++++++++++-------
>   2 files changed, 126 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 8afacc90c63b..4ce266951cb6 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -268,6 +268,7 @@ config PCIE_DW_PLAT_EP
>   config PCIE_QCOM
>   	bool "Qualcomm PCIe controller (host mode)"
>   	depends on OF && (ARCH_QCOM || COMPILE_TEST)
> +	depends on QCOM_COMMAND_DB || QCOM_COMMAND_DB=n
>   	depends on PCI_MSI
>   	select PCIE_DW_HOST
>   	select CRC8
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 6a469ed213ce..c807833ee4a7 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -30,13 +30,18 @@
>   #include <linux/reset.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> +#include <soc/qcom/cmd-db.h>
>   
>   #include "../../pci.h"
>   #include "pcie-designware.h"
>   
> +#include <dt-bindings/interconnect/qcom,icc.h>
> +#include <dt-bindings/interconnect/qcom,rpm-icc.h>
> +
>   /* PARF registers */
>   #define PARF_SYS_CTRL				0x00
>   #define PARF_PM_CTRL				0x20
> +#define PARF_PM_STTS				0x24
>   #define PARF_PCS_DEEMPH				0x34
>   #define PARF_PCS_SWING				0x38
>   #define PARF_PHY_CTRL				0x40
> @@ -80,7 +85,10 @@
>   #define L1_CLK_RMV_DIS				BIT(1)
>   
>   /* PARF_PM_CTRL register fields */
> -#define REQ_NOT_ENTR_L1				BIT(5)
> +#define REQ_NOT_ENTR_L1				BIT(5) /* "Prevent L0->L1" */
> +
> +/* PARF_PM_STTS register fields */
> +#define PM_ENTER_L23				BIT(5)
>   
>   /* PARF_PCS_DEEMPH register fields */
>   #define PCS_DEEMPH_TX_DEEMPH_GEN1(x)		FIELD_PREP(GENMASK(21, 16), x)
> @@ -122,6 +130,7 @@
>   
>   /* ELBI_SYS_CTRL register fields */
>   #define ELBI_SYS_CTRL_LT_ENABLE			BIT(0)
> +#define ELBI_SYS_CTRL_PME_TURNOFF_MSG		BIT(4)
>   
>   /* AXI_MSTR_RESP_COMP_CTRL0 register fields */
>   #define CFG_REMOTE_RD_REQ_BRIDGE_SIZE_2K	0x4
> @@ -243,6 +252,7 @@ struct qcom_pcie {
>   	const struct qcom_pcie_cfg *cfg;
>   	struct dentry *debugfs;
>   	bool suspended;
> +	bool soc_is_rpmh;
>   };
>   
>   #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> @@ -272,6 +282,24 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
>   	return 0;
>   }
>   
> +static int qcom_pcie_stop_link(struct dw_pcie *pci)
> +{
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +	u32 ret_l23, val;
> +
> +	writel(ELBI_SYS_CTRL_PME_TURNOFF_MSG, pcie->elbi + ELBI_SYS_CTRL);
> +	readl(pcie->elbi + ELBI_SYS_CTRL);
> +
> +	ret_l23 = readl_poll_timeout(pcie->parf + PARF_PM_STTS, val,
> +				     val & PM_ENTER_L23, 10000, 100000);
> +	if (ret_l23) {
> +		dev_err(pci->dev, "Failed to enter L2/L3\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
>   static void qcom_pcie_clear_hpc(struct dw_pcie *pci)
>   {
>   	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> @@ -987,9 +1015,19 @@ static void qcom_pcie_host_post_init_2_7_0(struct qcom_pcie *pcie)
>   static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>   {
>   	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +	u32 val;
> +
> +	/* Disable PCIe clocks and resets */
> +	val = readl(pcie->parf + PARF_PHY_CTRL);
> +	val |= PHY_TEST_PWR_DOWN;
> +	writel(val, pcie->parf + PARF_PHY_CTRL);
> +	readl(pcie->parf + PARF_PHY_CTRL);
>   
>   	clk_bulk_disable_unprepare(res->num_clks, res->clks);
>   
> +	reset_control_assert(res->rst);
> +	usleep_range(2000, 2500);
> +
>   	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>   }
>   
> @@ -1545,6 +1583,9 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>   		goto err_phy_exit;
>   	}
>   
> +	/* If the soc features RPMh, cmd_db must have been prepared by now */
> +	pcie->soc_is_rpmh = !cmd_db_ready();
> +
>   	qcom_pcie_icc_update(pcie);
>   
>   	if (pcie->mhi)
> @@ -1561,58 +1602,108 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>   	return ret;
>   }
>   
> -static int qcom_pcie_suspend_noirq(struct device *dev)
> +static int qcom_pcie_resume_noirq(struct device *dev)
>   {
>   	struct qcom_pcie *pcie = dev_get_drvdata(dev);
>   	int ret;
>   
> -	/*
> -	 * Set minimum bandwidth required to keep data path functional during
> -	 * suspend.
> -	 */
> -	ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
> -	if (ret) {
> -		dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
> -		return ret;
> +	if (pcie->soc_is_rpmh) {
> +		/*
> +		 * Undo the tag change from qcom_pcie_suspend_noirq first in
> +		 * case RPMh spontaneously decides to power collapse the
> +		 * platform based on other inputs.
> +		 */
> +		icc_set_tag(pcie->icc_mem, QCOM_ICC_TAG_ALWAYS);
> +
> +		/* Flush the tag change */
> +		ret = icc_enable(pcie->icc_mem);
> +		if (ret) {
> +			dev_err(pcie->pci->dev, "failed to icc_enable: %d", ret);
> +			return ret;
> +		}
>   	}
>   
> -	/*
> -	 * Turn OFF the resources only for controllers without active PCIe
> -	 * devices. For controllers with active devices, the resources are kept
> -	 * ON and the link is expected to be in L0/L1 (sub)states.
> -	 *
> -	 * Turning OFF the resources for controllers with active PCIe devices
> -	 * will trigger access violation during the end of the suspend cycle,
> -	 * as kernel tries to access the PCIe devices config space for masking
> -	 * MSIs.
> -	 *
> -	 * Also, it is not desirable to put the link into L2/L3 state as that
> -	 * implies VDD supply will be removed and the devices may go into
> -	 * powerdown state. This will affect the lifetime of the storage devices
> -	 * like NVMe.
> -	 */
> -	if (!dw_pcie_link_up(pcie->pci)) {
> -		qcom_pcie_host_deinit(&pcie->pci->pp);
> -		pcie->suspended = true;
> -	}
> +	/* Only check this now to make sure the icc tag has been set. */
> +	if (!pcie->suspended)
> +		return 0;
> +
> +	ret = qcom_pcie_host_init(&pcie->pci->pp);
> +	if (ret)
> +		goto revert_icc_tag;
> +
> +	dw_pcie_setup_rc(&pcie->pci->pp);
> +
> +	ret = qcom_pcie_start_link(pcie->pci);
> +	if (ret)
> +		goto deinit_host;
> +
> +	/* Ignore the retval, the devices may come up later. */
> +	dw_pcie_wait_for_link(pcie->pci);
> +
> +	qcom_pcie_icc_update(pcie);
> +
> +	pcie->suspended = false;
>   
>   	return 0;
> +
> +deinit_host:
> +	qcom_pcie_host_deinit(&pcie->pci->pp);
> +revert_icc_tag:
> +	if (pcie->soc_is_rpmh) {
> +		icc_set_tag(pcie->icc_mem, QCOM_ICC_TAG_WAKE);
> +
> +		/* Ignore the retval, failing here would be tragic anyway.. */
> +		icc_enable(pcie->icc_mem);
> +	}
> +
> +	return ret;
>   }
>   
> -static int qcom_pcie_resume_noirq(struct device *dev)
> +static int qcom_pcie_suspend_noirq(struct device *dev)
>   {
>   	struct qcom_pcie *pcie = dev_get_drvdata(dev);
>   	int ret;
>   
> -	if (pcie->suspended) {
> -		ret = qcom_pcie_host_init(&pcie->pci->pp);
> +	if (pcie->suspended)
> +		return 0;
> +
> +	if (dw_pcie_link_up(pcie->pci)) {
> +		ret = qcom_pcie_stop_link(pcie->pci);
>   		if (ret)
>   			return ret;
> +	}
>   
> -		pcie->suspended = false;
> +	qcom_pcie_host_deinit(&pcie->pci->pp);
> +
> +	if (pcie->soc_is_rpmh) {
> +		/*
> +		 * The PCIe RC may be covertly accessed by the secure firmware
> +		 * on sleep exit. Use the WAKE bucket to let RPMh pull the plug
> +		 * on PCIe in sleep, but guarantee it comes back up for resume.
> +		 */
> +		icc_set_tag(pcie->icc_mem, QCOM_ICC_TAG_WAKE);
> +
> +		/* Flush the tag change */
> +		ret = icc_enable(pcie->icc_mem);
> +		if (ret) {
> +			dev_err(pcie->pci->dev, "failed to icc_enable %d\n", ret);
> +
> +			/* Revert everything and pray icc calls succeed */
> +			return qcom_pcie_resume_noirq(dev);
> +		}
> +	} else {
> +		/*
> +		 * Set minimum bandwidth required to keep data path functional
> +		 * during suspend.
> +		 */
calling qcom_pcie_host_deinit(&pcie->pci->pp) above will turn off all 
the resources, setting BW to 1Kbps will not make sense here.

- Krishna Chaitanya.
> +		ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
> +		if (ret) {
> +			dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
> +			return ret;
> +		}
>   	}
>   
> -	qcom_pcie_icc_update(pcie);
> +	pcie->suspended = true;
>   
>   	return 0;
>   }
>