| 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: <YrypE5KlICZgmEi0@google.com>
Date: Wed, 29 Jun 2022 12:33:39 -0700
From: Matthias Kaehlcke <mka@...omium.org>
To: Krishna chaitanya chundru <quic_krichai@...cinc.com>
Cc: helgaas@...nel.org, linux-pci@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
quic_vbadigan@...cinc.com, quic_hemantk@...cinc.com,
quic_nitegupt@...cinc.com, quic_skananth@...cinc.com,
quic_ramkri@...cinc.com, manivannan.sadhasivam@...aro.org,
swboyd@...omium.org, dmitry.baryshkov@...aro.org,
Stanimir Varbanov <svarbanov@...sol.com>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Rob Herring <robh@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH v2 1/2] PCI: qcom: Add system PM support
On Wed, Jun 29, 2022 at 03:03:33PM +0530, Krishna chaitanya chundru wrote:
> Add suspend and resume pm callbacks.
>
> When system suspends, and if the link is in L1ss, disable the clocks
> so that system enters into low power state to save the maximum power.
> And when the system resumes, enable the clocks back if they are
> disabled in the suspend path.
>
> Changes since v1:
> - Fixed compilation errors.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@...cinc.com>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 81 ++++++++++++++++++++++++++++++++++
> 1 file changed, 81 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 6ab9089..8e9ef37 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -41,6 +41,9 @@
> #define L23_CLK_RMV_DIS BIT(2)
> #define L1_CLK_RMV_DIS BIT(1)
>
> +#define PCIE20_PARF_PM_STTS 0x24
> +#define PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB BIT(8)
> +
> #define PCIE20_PARF_PHY_CTRL 0x40
> #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK GENMASK(20, 16)
> #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x) ((x) << 16)
> @@ -190,6 +193,8 @@ struct qcom_pcie_ops {
> void (*post_deinit)(struct qcom_pcie *pcie);
> void (*ltssm_enable)(struct qcom_pcie *pcie);
> int (*config_sid)(struct qcom_pcie *pcie);
> + int (*enable_clks)(struct qcom_pcie *pcie);
> + int (*disable_clks)(struct qcom_pcie *pcie);
> };
>
> struct qcom_pcie_cfg {
> @@ -199,6 +204,7 @@ struct qcom_pcie_cfg {
> unsigned int has_ddrss_sf_tbu_clk:1;
> unsigned int has_aggre0_clk:1;
> unsigned int has_aggre1_clk:1;
> + unsigned int support_pm_ops:1;
nit: s/support_pm_ops/supports_system_suspend/ ?
It's not really the ops that are supported, but system suspend. 'supports'
(with an 's' at the end) to specify a characteristic of the
driver/controller (similar to the 'has_<something>'s above), rather than
an imperative of what the driver should do.
In any case, it's just a nit :)
> };
>
> struct qcom_pcie {
> @@ -209,6 +215,7 @@ struct qcom_pcie {
> struct phy *phy;
> struct gpio_desc *reset;
> const struct qcom_pcie_cfg *cfg;
> + unsigned int is_suspended:1;
> };
>
> #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
> @@ -1308,6 +1315,23 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
> clk_disable_unprepare(res->pipe_clk);
> }
>
> +static int qcom_pcie_enable_clks_2_7_0(struct qcom_pcie *pcie)
> +{
> + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +
> + return clk_bulk_prepare_enable(res->num_clks, res->clks);
> +}
> +
> +static int qcom_pcie_disable_clks_2_7_0(struct qcom_pcie *pcie)
> +{
> + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +
> + clk_bulk_disable_unprepare(res->num_clks, res->clks);
> +
> + return 0;
> +}
> +
> +
nit: delete one empty line
> static int qcom_pcie_link_up(struct dw_pcie *pci)
> {
> u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> @@ -1485,6 +1509,8 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
> .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> .post_init = qcom_pcie_post_init_2_7_0,
> .post_deinit = qcom_pcie_post_deinit_2_7_0,
> + .enable_clks = qcom_pcie_enable_clks_2_7_0,
> + .disable_clks = qcom_pcie_disable_clks_2_7_0,
> };
>
> /* Qcom IP rev.: 1.9.0 */
> @@ -1548,6 +1574,7 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
> .ops = &ops_1_9_0,
> .has_tbu_clk = true,
> .pipe_clk_need_muxing = true,
> + .support_pm_ops = true,
> };
>
> static const struct dw_pcie_ops dw_pcie_ops = {
> @@ -1591,6 +1618,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>
> pcie->cfg = pcie_cfg;
>
> + pcie->is_suspended = false;
not strictly necessary because of kzalloc, but does no harm either.
> +
> pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> if (IS_ERR(pcie->reset)) {
> ret = PTR_ERR(pcie->reset);
> @@ -1645,6 +1674,57 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> return ret;
> }
>
> +static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
> +{
> + struct qcom_pcie *pcie = dev_get_drvdata(dev);
> + u32 val;
> +
> + if (!pcie->cfg->support_pm_ops)
> + return 0;
> +
> + /* if the link is not in l1ss don't turn off clocks */
> + val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> + if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> + dev_err(dev, "Link is not in L1ss\n");
If this is an error, should the function return an error? Otherwise maybe it
should be a warning.
> + return 0;
> + }
> +
> + if (pcie->cfg->ops->disable_clks)
> + pcie->cfg->ops->disable_clks(pcie);
> +
> + if (pcie->cfg->ops->post_deinit)
> + pcie->cfg->ops->post_deinit(pcie);
> +
> + pcie->is_suspended = true;
> +
> + return 0;
> +}
> +
> +static int __maybe_unused qcom_pcie_pm_resume(struct device *dev)
> +{
> + struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +
> + if (!pcie->cfg->support_pm_ops)
> + return 0;
> +
> + if (!pcie->is_suspended)
> + return 0;
> +
> + if (pcie->cfg->ops->enable_clks)
> + pcie->cfg->ops->enable_clks(pcie);
> +
> + if (pcie->cfg->ops->post_init)
> + pcie->cfg->ops->post_init(pcie);
> +
> + pcie->is_suspended = false;
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
> +};
> +
> static const struct of_device_id qcom_pcie_match[] = {
> { .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
> { .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
> @@ -1679,6 +1759,7 @@ static struct platform_driver qcom_pcie_driver = {
> .probe = qcom_pcie_probe,
> .driver = {
> .name = "qcom-pcie",
> + .pm = &qcom_pcie_pm_ops,
> .suppress_bind_attrs = true,
> .of_match_table = qcom_pcie_match,
> },
> --
> 2.7.4
>
Powered by blists - more mailing lists