| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <wcxyba2xxpsfgffa5ds7ctgekt5fanp6udeivh3kf7x7pr6cw4@3i27f7etqqt3>
Date: Wed, 26 Jun 2024 09:31:55 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: Krishna chaitanya chundru <quic_krichai@...cinc.com>
Cc: Bartosz Golaszewski <brgl@...ev.pl>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>, Rob Herring <robh@...nel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>,
Jingoo Han <jingoohan1@...il.com>, quic_vbadigan@...cinc.com, quic_skananth@...cinc.com,
quic_nitegupt@...cinc.com, linux-arm-msm@...r.kernel.org, linux-pci@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 6/7] pci: qcom: Add support for start_link() &
stop_link()
On Wed, Jun 26, 2024 at 06:07:54PM GMT, Krishna chaitanya chundru wrote:
Please start your commit message with a description of the problem that
you're solving, then followed by the technical description of your
solution (which you have here).
Also, uppercase "PCI:" in your subject prefix.
> In the stop_link() if the PCIe link is not up, disable LTSSM enable
> bit to stop link training otherwise keep the link in D3cold.
> And in the start_link() the enable LTSSM bit if the resources are
> turned on other wise do the all the initialization and then start
> the link.
>
> Introduce ltssm_disable function op to stop the link training.
>
> Use a flag 'pci_pwrctl_turned_off" to indicate the resources are
> turned off by the pci pwrctl framework.
>
> If the link is stopped using the stop_link() then just return with
> doing anything in suspend and resume.
And an empty line between commit message and the tags.
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@...cinc.com>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 108 +++++++++++++++++++++++++++++----
> 1 file changed, 97 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 14772edcf0d3..1ab3ffdb3914 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -37,6 +37,7 @@
> /* 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
> @@ -83,6 +84,9 @@
> /* PARF_PM_CTRL register fields */
> #define REQ_NOT_ENTR_L1 BIT(5)
>
> +/* 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)
> #define PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(x) FIELD_PREP(GENMASK(13, 8), x)
> @@ -126,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
> @@ -228,6 +233,7 @@ struct qcom_pcie_ops {
> void (*host_post_init)(struct qcom_pcie *pcie);
> void (*deinit)(struct qcom_pcie *pcie);
> void (*ltssm_enable)(struct qcom_pcie *pcie);
> + void (*ltssm_disable)(struct qcom_pcie *pcie);
> int (*config_sid)(struct qcom_pcie *pcie);
> };
>
> @@ -248,10 +254,13 @@ struct qcom_pcie {
> const struct qcom_pcie_cfg *cfg;
> struct dentry *debugfs;
> bool suspended;
> + bool pci_pwrctl_turned_off;
> };
>
> #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
>
> +static void qcom_pcie_icc_update(struct qcom_pcie *pcie);
> +
> static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> {
> gpiod_set_value_cansleep(pcie->reset, 1);
> @@ -266,17 +275,6 @@ static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> }
>
> -static int qcom_pcie_start_link(struct dw_pcie *pci)
> -{
> - struct qcom_pcie *pcie = to_qcom_pcie(pci);
> -
> - /* Enable Link Training state machine */
> - if (pcie->cfg->ops->ltssm_enable)
> - pcie->cfg->ops->ltssm_enable(pcie);
> -
> - return 0;
> -}
> -
> static void qcom_pcie_clear_aspm_l0s(struct dw_pcie *pci)
> {
> struct qcom_pcie *pcie = to_qcom_pcie(pci);
> @@ -556,6 +554,15 @@ static int qcom_pcie_post_init_1_0_0(struct qcom_pcie *pcie)
> return 0;
> }
>
> +static void qcom_pcie_2_3_2_ltssm_disable(struct qcom_pcie *pcie)
> +{
> + u32 val;
> +
> + val = readl(pcie->parf + PARF_LTSSM);
> + val &= ~LTSSM_EN;
> + writel(val, pcie->parf + PARF_LTSSM);
> +}
> +
> static void qcom_pcie_2_3_2_ltssm_enable(struct qcom_pcie *pcie)
> {
> u32 val;
> @@ -1336,6 +1343,7 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
> .post_init = qcom_pcie_post_init_2_7_0,
> .deinit = qcom_pcie_deinit_2_7_0,
> .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> + .ltssm_disable = qcom_pcie_2_3_2_ltssm_disable,
> };
>
> /* Qcom IP rev.: 1.9.0 */
> @@ -1346,6 +1354,7 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
> .host_post_init = qcom_pcie_host_post_init_2_7_0,
> .deinit = qcom_pcie_deinit_2_7_0,
> .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> + .ltssm_disable = qcom_pcie_2_3_2_ltssm_disable,
> .config_sid = qcom_pcie_config_sid_1_9_0,
> };
>
> @@ -1395,9 +1404,81 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
> .no_l0s = true,
> };
>
> +static int qcom_pcie_turnoff_link(struct dw_pcie *pci)
> +{
> + struct qcom_pcie *pcie = to_qcom_pcie(pci);
> + u32 ret_l23, val, ret;
> +
> + if (!dw_pcie_link_up(pcie->pci)) {
> + if (pcie->cfg->ops->ltssm_disable)
> + pcie->cfg->ops->ltssm_disable(pcie);
> + } else {
> + writel(ELBI_SYS_CTRL_PME_TURNOFF_MSG, 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;
> + }
> +
> + qcom_pcie_host_deinit(&pcie->pci->pp);
> +
> + ret = icc_disable(pcie->icc_mem);
> + if (ret)
> + dev_err(pci->dev, "Failed to disable PCIe-MEM interconnect path: %d\n",
> + ret);
> +
> + pcie->pci_pwrctl_turned_off = true;
> + }
> +
> + return 0;
> +}
> +
> +static int qcom_pcie_turnon_link(struct dw_pcie *pci)
> +{
> + struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +
> + if (pcie->pci_pwrctl_turned_off) {
> + qcom_pcie_host_init(&pcie->pci->pp);
> +
> + dw_pcie_setup_rc(&pcie->pci->pp);
> + }
> +
> + if (pcie->cfg->ops->ltssm_enable)
> + pcie->cfg->ops->ltssm_enable(pcie);
> +
> + /* Ignore the retval, the devices may come up later. */
> + dw_pcie_wait_for_link(pcie->pci);
> +
> + qcom_pcie_icc_update(pcie);
> +
> + pcie->pci_pwrctl_turned_off = false;
> +
> + return 0;
> +}
> +
> +static int qcom_pcie_start_link(struct dw_pcie *pci)
> +{
> + return qcom_pcie_turnon_link(pci);
If you inline qcom_pcie_turnon_link() here, you don't need to have two
different words for "start"/"turnon".
> +}
> +
> +static void qcom_pcie_stop_link(struct dw_pcie *pci)
> +{
> + struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +
> + if (!dw_pcie_link_up(pcie->pci)) {
Unless I'm reading it wrong, qcom_pcie_turnoff_link() has exactly the
same prologue, so you should be able to just inline
qcom_pcie_turnoff_link() in this function and avoid the "stop"/"turnoff"
naming.
> + if (pcie->cfg->ops->ltssm_disable)
> + pcie->cfg->ops->ltssm_disable(pcie);
> + } else {
> + qcom_pcie_turnoff_link(pci);
> + }
> +}
> +
> static const struct dw_pcie_ops dw_pcie_ops = {
> .link_up = qcom_pcie_link_up,
> .start_link = qcom_pcie_start_link,
> + .stop_link = qcom_pcie_stop_link,
> };
>
> static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> @@ -1604,6 +1685,8 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> struct qcom_pcie *pcie = dev_get_drvdata(dev);
> int ret;
>
This deserves a comment.
> + if (pcie->pci_pwrctl_turned_off)
> + return 0;
> /*
> * Set minimum bandwidth required to keep data path functional during
> * suspend.
> @@ -1642,6 +1725,9 @@ static int qcom_pcie_resume_noirq(struct device *dev)
> struct qcom_pcie *pcie = dev_get_drvdata(dev);
> int ret;
>
Ditto.
> + if (pcie->pci_pwrctl_turned_off)
> + return 0;
> +
Regards,
Bjorn
> if (pcie->suspended) {
> ret = qcom_pcie_host_init(&pcie->pci->pp);
> if (ret)
>
> --
> 2.42.0
>
Powered by blists - more mailing lists