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

[Krishna Chaitanya Chundru]

On 6/30/2022 10:04 AM, Manivannan Sadhasivam wrote:
> 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?

with aspm the link will automatically go to L1ss. for L2/L3 entry we 
need to explicitly send

PME turn off which we are not doing now. So we are checking only for L1ss.

> > 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.
Sure will change in the next patch.
> >   };
> >   
> >   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?

We are using this flag in the resume function to check whether we 
suspended and disabled

the clocks in the suspend path. And we want to use this flag to control 
the access to dbi etc

after suspend.

> >   };
> >   
> >   #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
Will update in the next patch.
> > +};
> > +
> >   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,
will update in the next patch.
> 		.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),
>
> Thanks,
> Mani
>
> >   		.suppress_bind_attrs = true,
> >   		.of_match_table = qcom_pcie_match,
> >   	},
> > --
> > 2.7.4