Re: [PATCH v2 1/2] PCI: qcom: Add system PM support

From: Manivannan Sadhasivam
Date: Thu Jun 30 2022 - 00:34:31 EST


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.
>

Why only during L1ss and not L2/L3?

> Changes since v1:
> - Fixed compilation errors.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@xxxxxxxxxxx>
> ---
> 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);

I think these could vary between platforms. Like some other platform may try to
disable regulators etc... So use names such as suspend and resume.

> };
>
> 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;
> };
>
> 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;

Why do you need this flag? Is suspend going to happen multiple times in
an out-of-order manner?

> };
>
> #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 const struct dev_pm_ops qcom_pcie_pm_ops = {
> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)

Use the new macro: NOIRQ_SYSTEM_SLEEP_PM_OPS

> +};
> +
> 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,

There will be warnings when CONFIG_PM_SLEEP is not set. So use below,

.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),

Thanks,
Mani

> .suppress_bind_attrs = true,
> .of_match_table = qcom_pcie_match,
> },
> --
> 2.7.4
>

--
மணிவண்ணன் சதாசிவம்