diff mbox series

[v8,6/7] PCI: mediatek-gen3: Add system PM support

Message ID 20210224061132.26526-7-jianjun.wang@mediatek.com (mailing list archive)
State New, archived
Headers show
Series PCI: mediatek: Add new generation controller support | expand

Commit Message

Jianjun Wang (王建军) Feb. 24, 2021, 6:11 a.m. UTC
Add suspend_noirq and resume_noirq callback functions to implement
PM system suspend hooks for MediaTek Gen3 PCIe controller.

When system suspend, trigger the PCIe link to L2 state and pull down
the PERST# pin, gating the clocks of MAC layer and power off the
physical layer for the sake of power saving.

When system resum, the PCIe link should be re-established and the
related control register values should be restored.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 84 +++++++++++++++++++++
 1 file changed, 84 insertions(+)

Comments

Krzysztof Wilczyński Feb. 24, 2021, 2:10 p.m. UTC | #1
Hi Jianjun,

> Add suspend_noirq and resume_noirq callback functions to implement
> PM system suspend hooks for MediaTek Gen3 PCIe controller.

So, "systems suspend" and "resume" hooks, correct?

> When system suspend, trigger the PCIe link to L2 state and pull down

It probably would be "the system suspends".

[...]
> When system resum, the PCIe link should be re-established and the
> related control register values should be restored.

Similarly to the above: "the system resumes".

[...]
> +	if (err) {
> +		dev_err(port->dev, "can not enter L2 state\n");
> +		return err;
> +	}

Most likely you want "cannot" or "can't" in the above error message.

> +	/* Pull down the PERST# pin */
> +	val = readl_relaxed(port->base + PCIE_RST_CTRL_REG);
> +	val |= PCIE_PE_RSTB;
> +	writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
> +
> +	dev_dbg(port->dev, "enter L2 state success");

Just a nitpick.  What about "entered L2 states successfully"?

[...]
> +	if (err) {
> +		dev_err(port->dev, "resume failed\n");
> +		return err;
> +	}

This error message does not quite convey that the mtk_pcie_startup_port()
was the function that failed, which is only a part of what you have to do
to successfully resume.

> +	dev_dbg(port->dev, "resume done\n");

A nitpick.  Probably not needed, as lack of error message would mean
that the device resumed successfully after being suspended.

Krzysztof
Jianjun Wang (王建军) Feb. 25, 2021, 3:34 a.m. UTC | #2
Hi Krzysztof,

Thanks for your review,

On Wed, 2021-02-24 at 15:10 +0100, Krzysztof Wilczyński wrote:
> Hi Jianjun,
> 
> > Add suspend_noirq and resume_noirq callback functions to implement
> > PM system suspend hooks for MediaTek Gen3 PCIe controller.
> 
> So, "systems suspend" and "resume" hooks, correct?

The callback functions is suspend_noirq and resume_noirq, should I use
"systems suspend" and "resume" in the commit message?

> 
> > When system suspend, trigger the PCIe link to L2 state and pull down
> 
> It probably would be "the system suspends".
> 
> [...]
> > When system resum, the PCIe link should be re-established and the
> > related control register values should be restored.
> 
> Similarly to the above: "the system resumes".
> 
> [...]
> > +	if (err) {
> > +		dev_err(port->dev, "can not enter L2 state\n");
> > +		return err;
> > +	}
> 
> Most likely you want "cannot" or "can't" in the above error message.
> 
> > +	/* Pull down the PERST# pin */
> > +	val = readl_relaxed(port->base + PCIE_RST_CTRL_REG);
> > +	val |= PCIE_PE_RSTB;
> > +	writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
> > +
> > +	dev_dbg(port->dev, "enter L2 state success");
> 
> Just a nitpick.  What about "entered L2 states successfully"?
> 
> [...]
> > +	if (err) {
> > +		dev_err(port->dev, "resume failed\n");
> > +		return err;
> > +	}
> 
> This error message does not quite convey that the mtk_pcie_startup_port()
> was the function that failed, which is only a part of what you have to do
> to successfully resume.
> 
> > +	dev_dbg(port->dev, "resume done\n");
> 
> A nitpick.  Probably not needed, as lack of error message would mean
> that the device resumed successfully after being suspended.
> 
> Krzysztof

Thanks.
Krzysztof Wilczyński Feb. 25, 2021, 10 p.m. UTC | #3
Hi Jianjun,

[...]
> Thanks for your review,

Thank YOU for all the work here!
 
[...]
> > > Add suspend_noirq and resume_noirq callback functions to implement
> > > PM system suspend hooks for MediaTek Gen3 PCIe controller.
> > 
> > So, "systems suspend" and "resume" hooks, correct?
> 
> The callback functions is suspend_noirq and resume_noirq, should I use
> "systems suspend" and "resume" in the commit message?
[...]


What I meant was something along these lines:

  Add suspend_noirq and resume_noirq callback functions to implement PM
  system suspend and resume hooks for the MediaTek Gen3 PCIe controller.
  
  When the system suspends, trigger the PCIe link to enter the L2 state
  and pull down the PERST# pin, gating the clocks of the MAC layer, and
  then power-off the physical layer to provide power-saving.
  
  When the system resumes, the PCIe link should be re-established and the
  related control register values should be restored.

The above is just a suggestion, thus feel tree to ignore it completely,
and it's heavily based on your original commit message.

Krzysztof
Jianjun Wang (王建军) Feb. 26, 2021, 10:06 a.m. UTC | #4
Hi Krzysztof,

Thanks for your suggestion, I will fix it in the next version.

On Thu, 2021-02-25 at 23:00 +0100, Krzysztof Wilczyński wrote:
> Hi Jianjun,
> 
> [...]
> > Thanks for your review,
> 
> Thank YOU for all the work here!
>  
> [...]
> > > > Add suspend_noirq and resume_noirq callback functions to implement
> > > > PM system suspend hooks for MediaTek Gen3 PCIe controller.
> > > 
> > > So, "systems suspend" and "resume" hooks, correct?
> > 
> > The callback functions is suspend_noirq and resume_noirq, should I use
> > "systems suspend" and "resume" in the commit message?
> [...]
> 
> 
> What I meant was something along these lines:
> 
>   Add suspend_noirq and resume_noirq callback functions to implement PM
>   system suspend and resume hooks for the MediaTek Gen3 PCIe controller.
>   
>   When the system suspends, trigger the PCIe link to enter the L2 state
>   and pull down the PERST# pin, gating the clocks of the MAC layer, and
>   then power-off the physical layer to provide power-saving.
>   
>   When the system resumes, the PCIe link should be re-established and the
>   related control register values should be restored.
> 
> The above is just a suggestion, thus feel tree to ignore it completely,
> and it's heavily based on your original commit message.
> 
> Krzysztof

Thanks.
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index fde9de591888..fd13540d37fe 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -45,6 +45,9 @@ 
 #define PCIE_PE_RSTB			BIT(3)
 
 #define PCIE_LTSSM_STATUS_REG		0x150
+#define PCIE_LTSSM_STATE_MASK		GENMASK(28, 24)
+#define PCIE_LTSSM_STATE(val)		((val & PCIE_LTSSM_STATE_MASK) >> 24)
+#define PCIE_LTSSM_STATE_L2_IDLE	0x14
 
 #define PCIE_LINK_STATUS_REG		0x154
 #define PCIE_PORT_LINKUP		BIT(8)
@@ -73,6 +76,9 @@ 
 #define PCIE_MSI_SET_ADDR_HI_BASE	0xc80
 #define PCIE_MSI_SET_ADDR_HI_OFFSET	0x04
 
+#define PCIE_ICMD_PM_REG		0x198
+#define PCIE_TURN_OFF_LINK		BIT(4)
+
 #define PCIE_TRANS_TABLE_BASE_REG	0x800
 #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
 #define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8
@@ -892,6 +898,83 @@  static int mtk_pcie_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused mtk_pcie_turn_off_link(struct mtk_pcie_port *port)
+{
+	u32 val;
+
+	val = readl_relaxed(port->base + PCIE_ICMD_PM_REG);
+	val |= PCIE_TURN_OFF_LINK;
+	writel_relaxed(val, port->base + PCIE_ICMD_PM_REG);
+
+	/* Check the link is L2 */
+	return readl_poll_timeout(port->base + PCIE_LTSSM_STATUS_REG, val,
+				  (PCIE_LTSSM_STATE(val) ==
+				   PCIE_LTSSM_STATE_L2_IDLE), 20,
+				   50 * USEC_PER_MSEC);
+}
+
+static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)
+{
+	struct mtk_pcie_port *port = dev_get_drvdata(dev);
+	int err;
+	u32 val;
+
+	/* Trigger link to L2 state */
+	err = mtk_pcie_turn_off_link(port);
+	if (err) {
+		dev_err(port->dev, "can not enter L2 state\n");
+		return err;
+	}
+
+	/* Pull down the PERST# pin */
+	val = readl_relaxed(port->base + PCIE_RST_CTRL_REG);
+	val |= PCIE_PE_RSTB;
+	writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
+
+	dev_dbg(port->dev, "enter L2 state success");
+
+	clk_bulk_disable_unprepare(port->num_clks, port->clks);
+
+	reset_control_assert(port->mac_reset);
+
+	phy_power_off(port->phy);
+	reset_control_assert(port->phy_reset);
+
+	return 0;
+}
+
+static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
+{
+	struct mtk_pcie_port *port = dev_get_drvdata(dev);
+	int err;
+
+	reset_control_deassert(port->phy_reset);
+	phy_power_on(port->phy);
+
+	reset_control_deassert(port->mac_reset);
+
+	err = clk_bulk_prepare_enable(port->num_clks, port->clks);
+	if (err) {
+		dev_dbg(dev, "failed to enable PCIe clocks\n");
+		return err;
+	}
+
+	err = mtk_pcie_startup_port(port);
+	if (err) {
+		dev_err(port->dev, "resume failed\n");
+		return err;
+	}
+
+	dev_dbg(port->dev, "resume done\n");
+
+	return 0;
+}
+
+static const struct dev_pm_ops mtk_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,
+				      mtk_pcie_resume_noirq)
+};
+
 static const struct of_device_id mtk_pcie_of_match[] = {
 	{ .compatible = "mediatek,mt8192-pcie" },
 	{},
@@ -903,6 +986,7 @@  static struct platform_driver mtk_pcie_driver = {
 	.driver = {
 		.name = "mtk-pcie",
 		.of_match_table = mtk_pcie_of_match,
+		.pm = &mtk_pcie_pm_ops,
 	},
 };