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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Thu, Jun 30, 2022 at 03:29:33PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 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.
> 

Okay, please mention this in the commit message.

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

Okay. The DBI access patch is not included in this series, so you should
mention it in the commit message.

Thanks,
Mani

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

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