[PATCH v2 1/1] PCI: layerscape: Add power management support

[PATCH v2 1/1] PCI: layerscape: Add power management support

[Frank Li]

From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

Add PME_Turn_Off/PME_TO_Ack handshake sequence to PCIe devices, such as
NVME or wifi module, and finally put the PCIe controller into D3 state
after the L2/L3 ready state transition process completion.

However, it's important to note that not all devices may be able to
tolerate the PME_Turn_Off command. In general, fixed PCIe devices
connected to Layerscape, such as NXP wifi devices, are able to handle
this command.

Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v1 to v2
- fixed Bjorn Helgaas's comments
- remove ls1043 and ls1021, which PME is not self cleaned. After check
spec, there are PME interrupt to indicate PEM_TO_ACK command. but I
have not these platform to debug it.  I just can test ls1028 platform
now. 

 drivers/pci/controller/dwc/pci-layerscape.c  | 219 ++++++++++++++++++-
 drivers/pci/controller/dwc/pcie-designware.h |   1 +
 2 files changed, 211 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
index ed5fb492fe08..5fbc9151ff82 100644
--- a/drivers/pci/controller/dwc/pci-layerscape.c
+++ b/drivers/pci/controller/dwc/pci-layerscape.c
@@ -8,9 +8,11 @@
  * Author: Minghuan Lian <Minghuan.Lian@freescale.com>
  */
 
+#include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/interrupt.h>
 #include <linux/init.h>
+#include <linux/iopoll.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
 #include <linux/of_address.h>
@@ -29,10 +31,40 @@
 
 #define PCIE_IATU_NUM		6
 
+/* PF Message Command Register */
+#define LS_PCIE_PF_MCR		0x2c
+#define PF_MCR_PTOMR		BIT(0)
+#define PF_MCR_EXL2S		BIT(1)
+
+#define LS_PCIE_IS_L2(v)	\
+	(((v) & PORT_LOGIC_LTSSM_STATE_MASK) == PORT_LOGIC_LTSSM_STATE_L2)
+
+struct ls_pcie;
+
+struct ls_pcie_host_pm_ops {
+	int (*pm_init)(struct ls_pcie *pcie);
+	void (*send_turn_off_message)(struct ls_pcie *pcie);
+	void (*exit_from_l2)(struct ls_pcie *pcie);
+};
+
+struct ls_pcie_drvdata {
+	const u32 pf_off;
+	const u32 lut_off;
+	const struct ls_pcie_host_pm_ops *pm_ops;
+};
+
 struct ls_pcie {
 	struct dw_pcie *pci;
+	const struct ls_pcie_drvdata *drvdata;
+	void __iomem *pf_base;
+	void __iomem *lut_base;
+	bool big_endian;
+	bool pm_support;
+	struct regmap *scfg;
+	int index;
 };
 
+#define ls_pcie_pf_readl_addr(addr)	ls_pcie_pf_readl(pcie, addr)
 #define to_ls_pcie(x)	dev_get_drvdata((x)->dev)
 
 static bool ls_pcie_is_bridge(struct ls_pcie *pcie)
@@ -73,6 +105,69 @@ static void ls_pcie_fix_error_response(struct ls_pcie *pcie)
 	iowrite32(PCIE_ABSERR_SETTING, pci->dbi_base + PCIE_ABSERR);
 }
 
+static u32 ls_pcie_pf_readl(struct ls_pcie *pcie, u32 off)
+{
+	if (pcie->big_endian)
+		return ioread32be(pcie->pf_base + off);
+
+	return ioread32(pcie->pf_base + off);
+}
+
+static void ls_pcie_pf_writel(struct ls_pcie *pcie, u32 off, u32 val)
+{
+	if (pcie->big_endian)
+		return iowrite32be(val, pcie->pf_base + off);
+
+	return iowrite32(val, pcie->pf_base + off);
+}
+
+static void ls_pcie_send_turnoff_msg(struct ls_pcie *pcie)
+{
+	u32 val;
+	int ret;
+
+	val = ls_pcie_pf_readl(pcie, LS_PCIE_PF_MCR);
+	val |= PF_MCR_PTOMR;
+	ls_pcie_pf_writel(pcie, LS_PCIE_PF_MCR, val);
+
+	ret = readx_poll_timeout(ls_pcie_pf_readl_addr, LS_PCIE_PF_MCR,
+				 val, !(val & PF_MCR_PTOMR), 100, 10000);
+	if (ret)
+		dev_warn(pcie->pci->dev, "poll turn off message timeout\n");
+}
+
+static void ls_pcie_exit_from_l2(struct ls_pcie *pcie)
+{
+	u32 val;
+	int ret;
+
+	val = ls_pcie_pf_readl(pcie, LS_PCIE_PF_MCR);
+	val |= PF_MCR_EXL2S;
+	ls_pcie_pf_writel(pcie, LS_PCIE_PF_MCR, val);
+
+	ret = readx_poll_timeout(ls_pcie_pf_readl_addr, LS_PCIE_PF_MCR,
+				 val, !(val & PF_MCR_EXL2S), 100, 10000);
+	if (ret)
+		dev_warn(pcie->pci->dev, "poll exit L2 state timeout\n");
+}
+
+static int ls_pcie_pm_init(struct ls_pcie *pcie)
+{
+	return 0;
+}
+
+static void ls_pcie_set_dstate(struct ls_pcie *pcie, u32 dstate)
+{
+	struct dw_pcie *pci = pcie->pci;
+	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_PM);
+	u32 val;
+
+	val = dw_pcie_readw_dbi(pci, offset + PCI_PM_CTRL);
+	val &= ~PCI_PM_CTRL_STATE_MASK;
+	val |= dstate;
+	dw_pcie_writew_dbi(pci, offset + PCI_PM_CTRL, val);
+}
+
 static int ls_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -86,23 +181,46 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
 
 	ls_pcie_drop_msg_tlp(pcie);
 
+	if (pcie->drvdata->pm_ops && pcie->drvdata->pm_ops->pm_init &&
+	    !pcie->drvdata->pm_ops->pm_init(pcie))
+		pcie->pm_support = true;
+
 	return 0;
 }
 
+static struct ls_pcie_host_pm_ops ls_pcie_host_pm_ops = {
+	.pm_init = &ls_pcie_pm_init,
+	.send_turn_off_message = &ls_pcie_send_turnoff_msg,
+	.exit_from_l2 = &ls_pcie_exit_from_l2,
+};
+
 static const struct dw_pcie_host_ops ls_pcie_host_ops = {
 	.host_init = ls_pcie_host_init,
 };
 
+static const struct ls_pcie_drvdata ls1021a_drvdata = {
+};
+
+static const struct ls_pcie_drvdata ls1043a_drvdata = {
+	.lut_off = 0x10000,
+};
+
+static const struct ls_pcie_drvdata layerscape_drvdata = {
+	.lut_off = 0x80000,
+	.pf_off = 0xc0000,
+	.pm_ops = &ls_pcie_host_pm_ops,
+};
+
 static const struct of_device_id ls_pcie_of_match[] = {
-	{ .compatible = "fsl,ls1012a-pcie", },
-	{ .compatible = "fsl,ls1021a-pcie", },
-	{ .compatible = "fsl,ls1028a-pcie", },
-	{ .compatible = "fsl,ls1043a-pcie", },
-	{ .compatible = "fsl,ls1046a-pcie", },
-	{ .compatible = "fsl,ls2080a-pcie", },
-	{ .compatible = "fsl,ls2085a-pcie", },
-	{ .compatible = "fsl,ls2088a-pcie", },
-	{ .compatible = "fsl,ls1088a-pcie", },
+	{ .compatible = "fsl,ls1012a-pcie", .data = &layerscape_drvdata },
+	{ .compatible = "fsl,ls1021a-pcie", .data = &ls1021a_drvdata },
+	{ .compatible = "fsl,ls1028a-pcie", .data = &layerscape_drvdata },
+	{ .compatible = "fsl,ls1043a-pcie", .data = &ls1043a_drvdata },
+	{ .compatible = "fsl,ls1046a-pcie", .data = &layerscape_drvdata },
+	{ .compatible = "fsl,ls2080a-pcie", .data = &layerscape_drvdata },
+	{ .compatible = "fsl,ls2085a-pcie", .data = &layerscape_drvdata },
+	{ .compatible = "fsl,ls2088a-pcie", .data = &layerscape_drvdata },
+	{ .compatible = "fsl,ls1088a-pcie", .data = &layerscape_drvdata },
 	{ },
 };
 
@@ -121,6 +239,8 @@ static int ls_pcie_probe(struct platform_device *pdev)
 	if (!pci)
 		return -ENOMEM;
 
+	pcie->drvdata = of_device_get_match_data(dev);
+
 	pci->dev = dev;
 	pci->pp.ops = &ls_pcie_host_ops;
 
@@ -131,6 +251,14 @@ static int ls_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(pci->dbi_base))
 		return PTR_ERR(pci->dbi_base);
 
+	pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
+
+	if (pcie->drvdata->lut_off)
+		pcie->lut_base = pci->dbi_base + pcie->drvdata->lut_off;
+
+	if (pcie->drvdata->pf_off)
+		pcie->pf_base = pci->dbi_base + pcie->drvdata->pf_off;
+
 	if (!ls_pcie_is_bridge(pcie))
 		return -ENODEV;
 
@@ -139,12 +267,85 @@ static int ls_pcie_probe(struct platform_device *pdev)
 	return dw_pcie_host_init(&pci->pp);
 }
 
+static bool ls_pcie_pm_supported(struct ls_pcie *pcie)
+{
+	if (!dw_pcie_link_up(pcie->pci)) {
+		dev_dbg(pcie->pci->dev, "Endpoint isn't present\n");
+		return false;
+	}
+
+	return pcie->pm_support;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ls_pcie_suspend_noirq(struct device *dev)
+{
+	struct ls_pcie *pcie = dev_get_drvdata(dev);
+	struct dw_pcie *pci = pcie->pci;
+	u32 val;
+	int ret;
+
+	if (!ls_pcie_pm_supported(pcie))
+		return 0;
+
+	pcie->drvdata->pm_ops->send_turn_off_message(pcie);
+
+	/* 10ms timeout to check L2 ready */
+	ret = readl_poll_timeout(pci->dbi_base + PCIE_PORT_DEBUG0,
+				 val, LS_PCIE_IS_L2(val), 100, 10000);
+	if (ret) {
+		dev_err(dev, "PCIe link enter L2 timeout! ltssm = 0x%x\n", val);
+		return ret;
+	}
+
+	ls_pcie_set_dstate(pcie, 0x3);
+
+	return 0;
+}
+
+static int ls_pcie_resume_noirq(struct device *dev)
+{
+	struct ls_pcie *pcie = dev_get_drvdata(dev);
+	struct dw_pcie *pci = pcie->pci;
+	int ret;
+
+	if (!ls_pcie_pm_supported(pcie))
+		return 0;
+
+	ls_pcie_set_dstate(pcie, 0x0);
+
+	pcie->drvdata->pm_ops->exit_from_l2(pcie);
+
+	ret = ls_pcie_host_init(&pci->pp);
+	if (ret) {
+		dev_err(dev, "PCIe host init failed! ret = 0x%x\n", ret);
+		return ret;
+	}
+
+	dw_pcie_setup_rc(&pci->pp);
+
+	ret = dw_pcie_wait_for_link(pci);
+	if (ret) {
+		dev_err(dev, "Wait link up timeout! ret = 0x%x\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static const struct dev_pm_ops ls_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(ls_pcie_suspend_noirq,
+				      ls_pcie_resume_noirq)
+};
+
 static struct platform_driver ls_pcie_driver = {
 	.probe = ls_pcie_probe,
 	.driver = {
 		.name = "layerscape-pcie",
 		.of_match_table = ls_pcie_of_match,
 		.suppress_bind_attrs = true,
+		.pm = &ls_pcie_pm_ops,
 	},
 };
 builtin_platform_driver(ls_pcie_driver);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 79713ce075cc..7de8409e2433 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -94,6 +94,7 @@
 #define PCIE_PORT_DEBUG0		0x728
 #define PORT_LOGIC_LTSSM_STATE_MASK	0x1f
 #define PORT_LOGIC_LTSSM_STATE_L0	0x11
+#define PORT_LOGIC_LTSSM_STATE_L2	0x15
 #define PCIE_PORT_DEBUG1		0x72C
 #define PCIE_PORT_DEBUG1_LINK_UP		BIT(4)
 #define PCIE_PORT_DEBUG1_LINK_IN_TRAINING	BIT(29)
-- 
2.34.1

Re: [PATCH v2 1/1] PCI: layerscape: Add power management support

[Rob Herring]

On Tue, Mar 21, 2023 at 11:02 AM Frank Li <Frank.Li@nxp.com> wrote:
>
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>
> Add PME_Turn_Off/PME_TO_Ack handshake sequence to PCIe devices, such as
> NVME or wifi module, and finally put the PCIe controller into D3 state
> after the L2/L3 ready state transition process completion.
>
> However, it's important to note that not all devices may be able to
> tolerate the PME_Turn_Off command. In general, fixed PCIe devices
> connected to Layerscape, such as NXP wifi devices, are able to handle
> this command.
>
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v1 to v2
> - fixed Bjorn Helgaas's comments
> - remove ls1043 and ls1021, which PME is not self cleaned. After check
> spec, there are PME interrupt to indicate PEM_TO_ACK command. but I
> have not these platform to debug it.  I just can test ls1028 platform
> now.
>
>  drivers/pci/controller/dwc/pci-layerscape.c  | 219 ++++++++++++++++++-
>  drivers/pci/controller/dwc/pcie-designware.h |   1 +
>  2 files changed, 211 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
> index ed5fb492fe08..5fbc9151ff82 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> @@ -8,9 +8,11 @@
>   * Author: Minghuan Lian <Minghuan.Lian@freescale.com>
>   */
>
> +#include <linux/delay.h>
>  #include <linux/kernel.h>
>  #include <linux/interrupt.h>
>  #include <linux/init.h>
> +#include <linux/iopoll.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_platform.h>
>  #include <linux/of_address.h>
> @@ -29,10 +31,40 @@
>
>  #define PCIE_IATU_NUM          6
>
> +/* PF Message Command Register */
> +#define LS_PCIE_PF_MCR         0x2c
> +#define PF_MCR_PTOMR           BIT(0)
> +#define PF_MCR_EXL2S           BIT(1)
> +
> +#define LS_PCIE_IS_L2(v)       \
> +       (((v) & PORT_LOGIC_LTSSM_STATE_MASK) == PORT_LOGIC_LTSSM_STATE_L2)
> +
> +struct ls_pcie;
> +
> +struct ls_pcie_host_pm_ops {
> +       int (*pm_init)(struct ls_pcie *pcie);
> +       void (*send_turn_off_message)(struct ls_pcie *pcie);
> +       void (*exit_from_l2)(struct ls_pcie *pcie);
> +};

Please don't create your own private layer of ops. Especially since
there is only 1 implementation.

Almost every driver has some function to set LTSSM states. Usually
that's in .start_link(). We probably need some more general DWC op
function control this.

> +
> +struct ls_pcie_drvdata {
> +       const u32 pf_off;
> +       const u32 lut_off;
> +       const struct ls_pcie_host_pm_ops *pm_ops;
> +};
> +
>  struct ls_pcie {
>         struct dw_pcie *pci;
> +       const struct ls_pcie_drvdata *drvdata;
> +       void __iomem *pf_base;
> +       void __iomem *lut_base;
> +       bool big_endian;
> +       bool pm_support;
> +       struct regmap *scfg;
> +       int index;
>  };
>
> +#define ls_pcie_pf_readl_addr(addr)    ls_pcie_pf_readl(pcie, addr)
>  #define to_ls_pcie(x)  dev_get_drvdata((x)->dev)
>
>  static bool ls_pcie_is_bridge(struct ls_pcie *pcie)
> @@ -73,6 +105,69 @@ static void ls_pcie_fix_error_response(struct ls_pcie *pcie)
>         iowrite32(PCIE_ABSERR_SETTING, pci->dbi_base + PCIE_ABSERR);
>  }
>
> +static u32 ls_pcie_pf_readl(struct ls_pcie *pcie, u32 off)
> +{
> +       if (pcie->big_endian)

You set 'big-endian' for the node, but it's only these PF registers
that are big endian?

> +               return ioread32be(pcie->pf_base + off);
> +
> +       return ioread32(pcie->pf_base + off);
> +}
> +
> +static void ls_pcie_pf_writel(struct ls_pcie *pcie, u32 off, u32 val)
> +{
> +       if (pcie->big_endian)
> +               return iowrite32be(val, pcie->pf_base + off);
> +
> +       return iowrite32(val, pcie->pf_base + off);
> +}
> +
> +static void ls_pcie_send_turnoff_msg(struct ls_pcie *pcie)
> +{
> +       u32 val;
> +       int ret;
> +
> +       val = ls_pcie_pf_readl(pcie, LS_PCIE_PF_MCR);
> +       val |= PF_MCR_PTOMR;
> +       ls_pcie_pf_writel(pcie, LS_PCIE_PF_MCR, val);
> +
> +       ret = readx_poll_timeout(ls_pcie_pf_readl_addr, LS_PCIE_PF_MCR,
> +                                val, !(val & PF_MCR_PTOMR), 100, 10000);
> +       if (ret)
> +               dev_warn(pcie->pci->dev, "poll turn off message timeout\n");
> +}
> +
> +static void ls_pcie_exit_from_l2(struct ls_pcie *pcie)
> +{
> +       u32 val;
> +       int ret;
> +
> +       val = ls_pcie_pf_readl(pcie, LS_PCIE_PF_MCR);
> +       val |= PF_MCR_EXL2S;
> +       ls_pcie_pf_writel(pcie, LS_PCIE_PF_MCR, val);
> +
> +       ret = readx_poll_timeout(ls_pcie_pf_readl_addr, LS_PCIE_PF_MCR,
> +                                val, !(val & PF_MCR_EXL2S), 100, 10000);
> +       if (ret)
> +               dev_warn(pcie->pci->dev, "poll exit L2 state timeout\n");
> +}
> +
> +static int ls_pcie_pm_init(struct ls_pcie *pcie)
> +{
> +       return 0;
> +}
> +
> +static void ls_pcie_set_dstate(struct ls_pcie *pcie, u32 dstate)
> +{
> +       struct dw_pcie *pci = pcie->pci;
> +       u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_PM);
> +       u32 val;
> +
> +       val = dw_pcie_readw_dbi(pci, offset + PCI_PM_CTRL);
> +       val &= ~PCI_PM_CTRL_STATE_MASK;
> +       val |= dstate;
> +       dw_pcie_writew_dbi(pci, offset + PCI_PM_CTRL, val);
> +}
> +
>  static int ls_pcie_host_init(struct dw_pcie_rp *pp)
>  {
>         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -86,23 +181,46 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
>
>         ls_pcie_drop_msg_tlp(pcie);
>
> +       if (pcie->drvdata->pm_ops && pcie->drvdata->pm_ops->pm_init &&
> +           !pcie->drvdata->pm_ops->pm_init(pcie))
> +               pcie->pm_support = true;

Just define 'pm_support' in the match data.

> +
>         return 0;
>  }
>
> +static struct ls_pcie_host_pm_ops ls_pcie_host_pm_ops = {
> +       .pm_init = &ls_pcie_pm_init,
> +       .send_turn_off_message = &ls_pcie_send_turnoff_msg,
> +       .exit_from_l2 = &ls_pcie_exit_from_l2,
> +};
> +
>  static const struct dw_pcie_host_ops ls_pcie_host_ops = {
>         .host_init = ls_pcie_host_init,
>  };
>
> +static const struct ls_pcie_drvdata ls1021a_drvdata = {
> +};
> +
> +static const struct ls_pcie_drvdata ls1043a_drvdata = {
> +       .lut_off = 0x10000,
> +};
> +
> +static const struct ls_pcie_drvdata layerscape_drvdata = {
> +       .lut_off = 0x80000,
> +       .pf_off = 0xc0000,
> +       .pm_ops = &ls_pcie_host_pm_ops,
> +};
> +
>  static const struct of_device_id ls_pcie_of_match[] = {
> -       { .compatible = "fsl,ls1012a-pcie", },
> -       { .compatible = "fsl,ls1021a-pcie", },
> -       { .compatible = "fsl,ls1028a-pcie", },
> -       { .compatible = "fsl,ls1043a-pcie", },
> -       { .compatible = "fsl,ls1046a-pcie", },
> -       { .compatible = "fsl,ls2080a-pcie", },
> -       { .compatible = "fsl,ls2085a-pcie", },
> -       { .compatible = "fsl,ls2088a-pcie", },
> -       { .compatible = "fsl,ls1088a-pcie", },
> +       { .compatible = "fsl,ls1012a-pcie", .data = &layerscape_drvdata },
> +       { .compatible = "fsl,ls1021a-pcie", .data = &ls1021a_drvdata },
> +       { .compatible = "fsl,ls1028a-pcie", .data = &layerscape_drvdata },
> +       { .compatible = "fsl,ls1043a-pcie", .data = &ls1043a_drvdata },
> +       { .compatible = "fsl,ls1046a-pcie", .data = &layerscape_drvdata },
> +       { .compatible = "fsl,ls2080a-pcie", .data = &layerscape_drvdata },
> +       { .compatible = "fsl,ls2085a-pcie", .data = &layerscape_drvdata },
> +       { .compatible = "fsl,ls2088a-pcie", .data = &layerscape_drvdata },
> +       { .compatible = "fsl,ls1088a-pcie", .data = &layerscape_drvdata },
>         { },
>  };
>
> @@ -121,6 +239,8 @@ static int ls_pcie_probe(struct platform_device *pdev)
>         if (!pci)
>                 return -ENOMEM;
>
> +       pcie->drvdata = of_device_get_match_data(dev);
> +
>         pci->dev = dev;
>         pci->pp.ops = &ls_pcie_host_ops;
>
> @@ -131,6 +251,14 @@ static int ls_pcie_probe(struct platform_device *pdev)
>         if (IS_ERR(pci->dbi_base))
>                 return PTR_ERR(pci->dbi_base);
>
> +       pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
> +
> +       if (pcie->drvdata->lut_off)
> +               pcie->lut_base = pci->dbi_base + pcie->drvdata->lut_off;
> +
> +       if (pcie->drvdata->pf_off)
> +               pcie->pf_base = pci->dbi_base + pcie->drvdata->pf_off;
> +
>         if (!ls_pcie_is_bridge(pcie))
>                 return -ENODEV;
>
> @@ -139,12 +267,85 @@ static int ls_pcie_probe(struct platform_device *pdev)
>         return dw_pcie_host_init(&pci->pp);
>  }
>
> +static bool ls_pcie_pm_supported(struct ls_pcie *pcie)
> +{
> +       if (!dw_pcie_link_up(pcie->pci)) {
> +               dev_dbg(pcie->pci->dev, "Endpoint isn't present\n");
> +               return false;
> +       }
> +
> +       return pcie->pm_support;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int ls_pcie_suspend_noirq(struct device *dev)
> +{
> +       struct ls_pcie *pcie = dev_get_drvdata(dev);
> +       struct dw_pcie *pci = pcie->pci;
> +       u32 val;
> +       int ret;
> +
> +       if (!ls_pcie_pm_supported(pcie))
> +               return 0;
> +
> +       pcie->drvdata->pm_ops->send_turn_off_message(pcie);
> +
> +       /* 10ms timeout to check L2 ready */
> +       ret = readl_poll_timeout(pci->dbi_base + PCIE_PORT_DEBUG0,
> +                                val, LS_PCIE_IS_L2(val), 100, 10000);
> +       if (ret) {
> +               dev_err(dev, "PCIe link enter L2 timeout! ltssm = 0x%x\n", val);
> +               return ret;
> +       }
> +
> +       ls_pcie_set_dstate(pcie, 0x3);
> +
> +       return 0;
> +}
> +
> +static int ls_pcie_resume_noirq(struct device *dev)
> +{
> +       struct ls_pcie *pcie = dev_get_drvdata(dev);
> +       struct dw_pcie *pci = pcie->pci;
> +       int ret;
> +
> +       if (!ls_pcie_pm_supported(pcie))
> +               return 0;
> +
> +       ls_pcie_set_dstate(pcie, 0x0);
> +
> +       pcie->drvdata->pm_ops->exit_from_l2(pcie);
> +
> +       ret = ls_pcie_host_init(&pci->pp);
> +       if (ret) {
> +               dev_err(dev, "PCIe host init failed! ret = 0x%x\n", ret);
> +               return ret;
> +       }
> +
> +       dw_pcie_setup_rc(&pci->pp);
> +
> +       ret = dw_pcie_wait_for_link(pci);
> +       if (ret) {
> +               dev_err(dev, "Wait link up timeout! ret = 0x%x\n", ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static const struct dev_pm_ops ls_pcie_pm_ops = {
> +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(ls_pcie_suspend_noirq,
> +                                     ls_pcie_resume_noirq)
> +};
> +
>  static struct platform_driver ls_pcie_driver = {
>         .probe = ls_pcie_probe,
>         .driver = {
>                 .name = "layerscape-pcie",
>                 .of_match_table = ls_pcie_of_match,
>                 .suppress_bind_attrs = true,
> +               .pm = &ls_pcie_pm_ops,
>         },
>  };
>  builtin_platform_driver(ls_pcie_driver);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 79713ce075cc..7de8409e2433 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -94,6 +94,7 @@
>  #define PCIE_PORT_DEBUG0               0x728
>  #define PORT_LOGIC_LTSSM_STATE_MASK    0x1f
>  #define PORT_LOGIC_LTSSM_STATE_L0      0x11
> +#define PORT_LOGIC_LTSSM_STATE_L2      0x15
>  #define PCIE_PORT_DEBUG1               0x72C
>  #define PCIE_PORT_DEBUG1_LINK_UP               BIT(4)
>  #define PCIE_PORT_DEBUG1_LINK_IN_TRAINING      BIT(29)
> --
> 2.34.1
>

RE: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add power management support

[Frank Li]

> 
> Caution: EXT Email
> 
> On Tue, Mar 21, 2023 at 11:02 AM Frank Li <Frank.Li@nxp.com> wrote:
> >
> > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> >
> > Add PME_Turn_Off/PME_TO_Ack handshake sequence to PCIe devices,
> such as
> > NVME or wifi module, and finally put the PCIe controller into D3 state
> > after the L2/L3 ready state transition process completion.
> >
> > However, it's important to note that not all devices may be able to
> > tolerate the PME_Turn_Off command. In general, fixed PCIe devices
> > connected to Layerscape, such as NXP wifi devices, are able to handle
> > this command.
> >
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > Change from v1 to v2
> > - fixed Bjorn Helgaas's comments
> > - remove ls1043 and ls1021, which PME is not self cleaned. After check
> > spec, there are PME interrupt to indicate PEM_TO_ACK command. but I
> > have not these platform to debug it.  I just can test ls1028 platform
> > now.
> >
> >  drivers/pci/controller/dwc/pci-layerscape.c  | 219 ++++++++++++++++++-
> >  drivers/pci/controller/dwc/pcie-designware.h |   1 +
> >  2 files changed, 211 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape.c
> b/drivers/pci/controller/dwc/pci-layerscape.c
> > index ed5fb492fe08..5fbc9151ff82 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> > @@ -8,9 +8,11 @@
> >   * Author: Minghuan Lian <Minghuan.Lian@freescale.com>
> >   */
> >
> > +#include <linux/delay.h>
> >  #include <linux/kernel.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/init.h>
> > +#include <linux/iopoll.h>
> >  #include <linux/of_pci.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/of_address.h>
> > @@ -29,10 +31,40 @@
> >
> >  #define PCIE_IATU_NUM          6
> >
> > +/* PF Message Command Register */
> > +#define LS_PCIE_PF_MCR         0x2c
> > +#define PF_MCR_PTOMR           BIT(0)
> > +#define PF_MCR_EXL2S           BIT(1)
> > +
> > +#define LS_PCIE_IS_L2(v)       \
> > +       (((v) & PORT_LOGIC_LTSSM_STATE_MASK) ==
> PORT_LOGIC_LTSSM_STATE_L2)
> > +
> > +struct ls_pcie;
> > +
> > +struct ls_pcie_host_pm_ops {
> > +       int (*pm_init)(struct ls_pcie *pcie);
> > +       void (*send_turn_off_message)(struct ls_pcie *pcie);
> > +       void (*exit_from_l2)(struct ls_pcie *pcie);
> > +};
> 
> Please don't create your own private layer of ops. Especially since
> there is only 1 implementation.

I removed ls1043 and ls1021 implement, because I have not platform to
test yet now. which required some logic change to detect PME_TO_ACK. 

What do you preferred?  By using if/else to handle ls1043 and ls1021 difference
In future? 

> 
> Almost every driver has some function to set LTSSM states. Usually
> that's in .start_link(). We probably need some more general DWC op
> function control this.
> 
> > +
> > +struct ls_pcie_drvdata {
> > +       const u32 pf_off;
> > +       const u32 lut_off;
> > +       const struct ls_pcie_host_pm_ops *pm_ops;
> > +};
> > +
> >  struct ls_pcie {
> >         struct dw_pcie *pci;
> > +       const struct ls_pcie_drvdata *drvdata;
> > +       void __iomem *pf_base;
> > +       void __iomem *lut_base;
> > +       bool big_endian;
> > +       bool pm_support;
> > +       struct regmap *scfg;
> > +       int index;
> >  };
> >
> > +#define ls_pcie_pf_readl_addr(addr)    ls_pcie_pf_readl(pcie, addr)
> >  #define to_ls_pcie(x)  dev_get_drvdata((x)->dev)
> >
> >  static bool ls_pcie_is_bridge(struct ls_pcie *pcie)
> > @@ -73,6 +105,69 @@ static void ls_pcie_fix_error_response(struct ls_pcie
> *pcie)
> >         iowrite32(PCIE_ABSERR_SETTING, pci->dbi_base + PCIE_ABSERR);
> >  }
> >
> > +static u32 ls_pcie_pf_readl(struct ls_pcie *pcie, u32 off)
> > +{
> > +       if (pcie->big_endian)
> 
> You set 'big-endian' for the node, but it's only these PF registers
> that are big endian?
> 
> > +               return ioread32be(pcie->pf_base + off);
> > +
> > +       return ioread32(pcie->pf_base + off);
> > +}
> > +
> > +static void ls_pcie_pf_writel(struct ls_pcie *pcie, u32 off, u32 val)
> > +{
> > +       if (pcie->big_endian)
> > +               return iowrite32be(val, pcie->pf_base + off);
> > +
> > +       return iowrite32(val, pcie->pf_base + off);
> > +}
> > +
> > +static void ls_pcie_send_turnoff_msg(struct ls_pcie *pcie)
> > +{
> > +       u32 val;
> > +       int ret;
> > +
> > +       val = ls_pcie_pf_readl(pcie, LS_PCIE_PF_MCR);
> > +       val |= PF_MCR_PTOMR;
> > +       ls_pcie_pf_writel(pcie, LS_PCIE_PF_MCR, val);
> > +
> > +       ret = readx_poll_timeout(ls_pcie_pf_readl_addr, LS_PCIE_PF_MCR,
> > +                                val, !(val & PF_MCR_PTOMR), 100, 10000);
> > +       if (ret)
> > +               dev_warn(pcie->pci->dev, "poll turn off message timeout\n");
> > +}
> > +
> > +static void ls_pcie_exit_from_l2(struct ls_pcie *pcie)
> > +{
> > +       u32 val;
> > +       int ret;
> > +
> > +       val = ls_pcie_pf_readl(pcie, LS_PCIE_PF_MCR);
> > +       val |= PF_MCR_EXL2S;
> > +       ls_pcie_pf_writel(pcie, LS_PCIE_PF_MCR, val);
> > +
> > +       ret = readx_poll_timeout(ls_pcie_pf_readl_addr, LS_PCIE_PF_MCR,
> > +                                val, !(val & PF_MCR_EXL2S), 100, 10000);
> > +       if (ret)
> > +               dev_warn(pcie->pci->dev, "poll exit L2 state timeout\n");
> > +}
> > +
> > +static int ls_pcie_pm_init(struct ls_pcie *pcie)
> > +{
> > +       return 0;
> > +}
> > +
> > +static void ls_pcie_set_dstate(struct ls_pcie *pcie, u32 dstate)
> > +{
> > +       struct dw_pcie *pci = pcie->pci;
> > +       u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_PM);
> > +       u32 val;
> > +
> > +       val = dw_pcie_readw_dbi(pci, offset + PCI_PM_CTRL);
> > +       val &= ~PCI_PM_CTRL_STATE_MASK;
> > +       val |= dstate;
> > +       dw_pcie_writew_dbi(pci, offset + PCI_PM_CTRL, val);
> > +}
> > +
> >  static int ls_pcie_host_init(struct dw_pcie_rp *pp)
> >  {
> >         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > @@ -86,23 +181,46 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
> >
> >         ls_pcie_drop_msg_tlp(pcie);
> >
> > +       if (pcie->drvdata->pm_ops && pcie->drvdata->pm_ops->pm_init &&
> > +           !pcie->drvdata->pm_ops->pm_init(pcie))
> > +               pcie->pm_support = true;
> 
> Just define 'pm_support' in the match data.
> 
> > +
> >         return 0;
> >  }
> >
> > +static struct ls_pcie_host_pm_ops ls_pcie_host_pm_ops = {
> > +       .pm_init = &ls_pcie_pm_init,
> > +       .send_turn_off_message = &ls_pcie_send_turnoff_msg,
> > +       .exit_from_l2 = &ls_pcie_exit_from_l2,
> > +};
> > +
> >  static const struct dw_pcie_host_ops ls_pcie_host_ops = {
> >         .host_init = ls_pcie_host_init,
> >  };
> >
> > +static const struct ls_pcie_drvdata ls1021a_drvdata = {
> > +};
> > +
> > +static const struct ls_pcie_drvdata ls1043a_drvdata = {
> > +       .lut_off = 0x10000,
> > +};
> > +
> > +static const struct ls_pcie_drvdata layerscape_drvdata = {
> > +       .lut_off = 0x80000,
> > +       .pf_off = 0xc0000,
> > +       .pm_ops = &ls_pcie_host_pm_ops,
> > +};
> > +
> >  static const struct of_device_id ls_pcie_of_match[] = {
> > -       { .compatible = "fsl,ls1012a-pcie", },
> > -       { .compatible = "fsl,ls1021a-pcie", },
> > -       { .compatible = "fsl,ls1028a-pcie", },
> > -       { .compatible = "fsl,ls1043a-pcie", },
> > -       { .compatible = "fsl,ls1046a-pcie", },
> > -       { .compatible = "fsl,ls2080a-pcie", },
> > -       { .compatible = "fsl,ls2085a-pcie", },
> > -       { .compatible = "fsl,ls2088a-pcie", },
> > -       { .compatible = "fsl,ls1088a-pcie", },
> > +       { .compatible = "fsl,ls1012a-pcie", .data = &layerscape_drvdata },
> > +       { .compatible = "fsl,ls1021a-pcie", .data = &ls1021a_drvdata },
> > +       { .compatible = "fsl,ls1028a-pcie", .data = &layerscape_drvdata },
> > +       { .compatible = "fsl,ls1043a-pcie", .data = &ls1043a_drvdata },
> > +       { .compatible = "fsl,ls1046a-pcie", .data = &layerscape_drvdata },
> > +       { .compatible = "fsl,ls2080a-pcie", .data = &layerscape_drvdata },
> > +       { .compatible = "fsl,ls2085a-pcie", .data = &layerscape_drvdata },
> > +       { .compatible = "fsl,ls2088a-pcie", .data = &layerscape_drvdata },
> > +       { .compatible = "fsl,ls1088a-pcie", .data = &layerscape_drvdata },
> >         { },
> >  };
> >
> > @@ -121,6 +239,8 @@ static int ls_pcie_probe(struct platform_device
> *pdev)
> >         if (!pci)
> >                 return -ENOMEM;
> >
> > +       pcie->drvdata = of_device_get_match_data(dev);
> > +
> >         pci->dev = dev;
> >         pci->pp.ops = &ls_pcie_host_ops;
> >
> > @@ -131,6 +251,14 @@ static int ls_pcie_probe(struct platform_device
> *pdev)
> >         if (IS_ERR(pci->dbi_base))
> >                 return PTR_ERR(pci->dbi_base);
> >
> > +       pcie->big_endian = of_property_read_bool(dev->of_node, "big-
> endian");
> > +
> > +       if (pcie->drvdata->lut_off)
> > +               pcie->lut_base = pci->dbi_base + pcie->drvdata->lut_off;
> > +
> > +       if (pcie->drvdata->pf_off)
> > +               pcie->pf_base = pci->dbi_base + pcie->drvdata->pf_off;
> > +
> >         if (!ls_pcie_is_bridge(pcie))
> >                 return -ENODEV;
> >
> > @@ -139,12 +267,85 @@ static int ls_pcie_probe(struct platform_device
> *pdev)
> >         return dw_pcie_host_init(&pci->pp);
> >  }
> >
> > +static bool ls_pcie_pm_supported(struct ls_pcie *pcie)
> > +{
> > +       if (!dw_pcie_link_up(pcie->pci)) {
> > +               dev_dbg(pcie->pci->dev, "Endpoint isn't present\n");
> > +               return false;
> > +       }
> > +
> > +       return pcie->pm_support;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int ls_pcie_suspend_noirq(struct device *dev)
> > +{
> > +       struct ls_pcie *pcie = dev_get_drvdata(dev);
> > +       struct dw_pcie *pci = pcie->pci;
> > +       u32 val;
> > +       int ret;
> > +
> > +       if (!ls_pcie_pm_supported(pcie))
> > +               return 0;
> > +
> > +       pcie->drvdata->pm_ops->send_turn_off_message(pcie);
> > +
> > +       /* 10ms timeout to check L2 ready */
> > +       ret = readl_poll_timeout(pci->dbi_base + PCIE_PORT_DEBUG0,
> > +                                val, LS_PCIE_IS_L2(val), 100, 10000);
> > +       if (ret) {
> > +               dev_err(dev, "PCIe link enter L2 timeout! ltssm = 0x%x\n", val);
> > +               return ret;
> > +       }
> > +
> > +       ls_pcie_set_dstate(pcie, 0x3);
> > +
> > +       return 0;
> > +}
> > +
> > +static int ls_pcie_resume_noirq(struct device *dev)
> > +{
> > +       struct ls_pcie *pcie = dev_get_drvdata(dev);
> > +       struct dw_pcie *pci = pcie->pci;
> > +       int ret;
> > +
> > +       if (!ls_pcie_pm_supported(pcie))
> > +               return 0;
> > +
> > +       ls_pcie_set_dstate(pcie, 0x0);
> > +
> > +       pcie->drvdata->pm_ops->exit_from_l2(pcie);
> > +
> > +       ret = ls_pcie_host_init(&pci->pp);
> > +       if (ret) {
> > +               dev_err(dev, "PCIe host init failed! ret = 0x%x\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       dw_pcie_setup_rc(&pci->pp);
> > +
> > +       ret = dw_pcie_wait_for_link(pci);
> > +       if (ret) {
> > +               dev_err(dev, "Wait link up timeout! ret = 0x%x\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +#endif /* CONFIG_PM_SLEEP */
> > +
> > +static const struct dev_pm_ops ls_pcie_pm_ops = {
> > +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(ls_pcie_suspend_noirq,
> > +                                     ls_pcie_resume_noirq)
> > +};
> > +
> >  static struct platform_driver ls_pcie_driver = {
> >         .probe = ls_pcie_probe,
> >         .driver = {
> >                 .name = "layerscape-pcie",
> >                 .of_match_table = ls_pcie_of_match,
> >                 .suppress_bind_attrs = true,
> > +               .pm = &ls_pcie_pm_ops,
> >         },
> >  };
> >  builtin_platform_driver(ls_pcie_driver);
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> b/drivers/pci/controller/dwc/pcie-designware.h
> > index 79713ce075cc..7de8409e2433 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -94,6 +94,7 @@
> >  #define PCIE_PORT_DEBUG0               0x728
> >  #define PORT_LOGIC_LTSSM_STATE_MASK    0x1f
> >  #define PORT_LOGIC_LTSSM_STATE_L0      0x11
> > +#define PORT_LOGIC_LTSSM_STATE_L2      0x15
> >  #define PCIE_PORT_DEBUG1               0x72C
> >  #define PCIE_PORT_DEBUG1_LINK_UP               BIT(4)
> >  #define PCIE_PORT_DEBUG1_LINK_IN_TRAINING      BIT(29)
> > --
> > 2.34.1
> >

Re: [PATCH v2 1/1] PCI: layerscape: Add power management support

[Bjorn Helgaas]

On Tue, Mar 21, 2023 at 12:02:20PM -0400, Frank Li wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> 
> Add PME_Turn_Off/PME_TO_Ack handshake sequence to PCIe devices, such as
> NVME or wifi module, and finally put the PCIe controller into D3 state
> after the L2/L3 ready state transition process completion.
> 
> However, it's important to note that not all devices may be able to
> tolerate the PME_Turn_Off command. In general, fixed PCIe devices
> connected to Layerscape, such as NXP wifi devices, are able to handle
> this command.

I know this paragraph is here because I asked whether all PCIe devices
could tolerate PME_Turn_Off.  I don't know much about that level of
the protocol, but it does look to me like PME_Turn_Off is required,
e.g., PCIe r6.0, sec 5.3.3.2.1, 5.3.3.4.

So I'm not sure this paragraph adds anything useful.  If the spec
requires it, this paragraph is like saying "it's important to note
that some PCIe devices may not follow the spec," which is pointless.

This functionality results in any downstream devices being put in
D3cold, right?  I think that *would* be worth mentioning.  There are a
few cases where we try to avoid putting devices in D3cold, e.g.,
no_d3cold, and I suspect this functionality would put them in D3cold
regardless of no_d3cold.  Those are corner cases that you would
probably never see on your platform, so a brief mention here is
probably enough.

> +static void ls_pcie_set_dstate(struct ls_pcie *pcie, u32 dstate)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_PM);
> +	u32 val;
> +
> +	val = dw_pcie_readw_dbi(pci, offset + PCI_PM_CTRL);
> +	val &= ~PCI_PM_CTRL_STATE_MASK;
> +	val |= dstate;
> +	dw_pcie_writew_dbi(pci, offset + PCI_PM_CTRL, val);

Is this a power management register for the *Root Port*, i.e., as
defined by PCIe r6.0 sec 7.5.2?

Or is it a similar register for the *Root Complex* as a whole that is
defined by a Layerscape or DesignWare spec and coincidentally uses the
same Capability ID and control register layout as the PCIe one?

The Root Port programming model is defined by the PCIe spec.  Things
like .send_turn_off_message() and .exit_from_l2() are clearly part of
the Root *Complex* programming model that is device-specific and not
defined by the PCIe spec.

I'm asking about ls_pcie_set_dstate() because it's written using the
PCIe constants (PCI_CAP_ID_PM, PCI_PM_CTRL, etc) but it's mixed in
with these Root Complex things that are *not* part of the PCIe spec.

> +static bool ls_pcie_pm_supported(struct ls_pcie *pcie)
> +{
> +	if (!dw_pcie_link_up(pcie->pci)) {
> +		dev_dbg(pcie->pci->dev, "Endpoint isn't present\n");
> +		return false;
> +	}
> +
> +	return pcie->pm_support;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int ls_pcie_suspend_noirq(struct device *dev)
> +{
> +	struct ls_pcie *pcie = dev_get_drvdata(dev);
> +	struct dw_pcie *pci = pcie->pci;
> +	u32 val;
> +	int ret;
> +
> +	if (!ls_pcie_pm_supported(pcie))
> +		return 0;
> +
> +	pcie->drvdata->pm_ops->send_turn_off_message(pcie);
> +
> +	/* 10ms timeout to check L2 ready */
> +	ret = readl_poll_timeout(pci->dbi_base + PCIE_PORT_DEBUG0,
> +				 val, LS_PCIE_IS_L2(val), 100, 10000);
> +	if (ret) {
> +		dev_err(dev, "PCIe link enter L2 timeout! ltssm = 0x%x\n", val);
> +		return ret;
> +	}
> +
> +	ls_pcie_set_dstate(pcie, 0x3);
> +
> +	return 0;
> +}
> +
> +static int ls_pcie_resume_noirq(struct device *dev)
> +{
> +	struct ls_pcie *pcie = dev_get_drvdata(dev);
> +	struct dw_pcie *pci = pcie->pci;
> +	int ret;
> +
> +	if (!ls_pcie_pm_supported(pcie))
> +		return 0;

How does this work?  You're checking whether the link is up *here*,
and if it's already up, you go on below to (I guess) set the PCIe
controller to D0, call dw_pcie_setup_rc() and dw_pcie_wait_for_link().
Most drivers call dw_pcie_setup_rc() *before* starting the link.

It looks like when you call ls_pcie_pm_supported() here,
dw_pcie_link_up() should always return false because the link isn't up
so it looks like there's no downstream device.  But I must be missing
something because if that were the case you would never wake anything
up below.

> +	ls_pcie_set_dstate(pcie, 0x0);
> +
> +	pcie->drvdata->pm_ops->exit_from_l2(pcie);
> +
> +	ret = ls_pcie_host_init(&pci->pp);
> +	if (ret) {
> +		dev_err(dev, "PCIe host init failed! ret = 0x%x\n", ret);
> +		return ret;
> +	}
> +
> +	dw_pcie_setup_rc(&pci->pp);
> +
> +	ret = dw_pcie_wait_for_link(pci);
> +	if (ret) {
> +		dev_err(dev, "Wait link up timeout! ret = 0x%x\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

RE: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add power management support

[Frank Li]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Tuesday, March 21, 2023 3:59 PM
> To: Frank Li <frank.li@nxp.com>
> Cc: bhelgaas@google.com; Leo Li <leoyang.li@nxp.com>; dl-linux-imx <linux-
> imx@nxp.com>; devicetree@vger.kernel.org;
> gustavo.pimentel@synopsys.com; kw@linux.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> pci@vger.kernel.org; lorenzo.pieralisi@arm.com; M.H. Lian
> <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>;
> robh+dt@kernel.org; Roy Zang <roy.zang@nxp.com>;
> shawnguo@kernel.org; Z.Q. Hou <zhiqiang.hou@nxp.com>
> Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add power management
> support
> 
> Caution: EXT Email
> 
> On Tue, Mar 21, 2023 at 12:02:20PM -0400, Frank Li wrote:
> > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> >
> > Add PME_Turn_Off/PME_TO_Ack handshake sequence to PCIe devices,
> such as
> > NVME or wifi module, and finally put the PCIe controller into D3 state
> > after the L2/L3 ready state transition process completion.
> >
> > However, it's important to note that not all devices may be able to
> > tolerate the PME_Turn_Off command. In general, fixed PCIe devices
> > connected to Layerscape, such as NXP wifi devices, are able to handle
> > this command.
> 
> I know this paragraph is here because I asked whether all PCIe devices
> could tolerate PME_Turn_Off.  I don't know much about that level of
> the protocol, but it does look to me like PME_Turn_Off is required,
> e.g., PCIe r6.0, sec 5.3.3.2.1, 5.3.3.4.
> 
> So I'm not sure this paragraph adds anything useful.  If the spec
> requires it, this paragraph is like saying "it's important to note
> that some PCIe devices may not follow the spec," which is pointless.
> 
> This functionality results in any downstream devices being put in
> D3cold, right?  I think that *would* be worth mentioning.  There are a
> few cases where we try to avoid putting devices in D3cold, e.g.,
> no_d3cold, and I suspect this functionality would put them in D3cold
> regardless of no_d3cold.  Those are corner cases that you would
> probably never see on your platform, so a brief mention here is
> probably enough.
> 
> > +static void ls_pcie_set_dstate(struct ls_pcie *pcie, u32 dstate)
> > +{
> > +     struct dw_pcie *pci = pcie->pci;
> > +     u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_PM);
> > +     u32 val;
> > +
> > +     val = dw_pcie_readw_dbi(pci, offset + PCI_PM_CTRL);
> > +     val &= ~PCI_PM_CTRL_STATE_MASK;
> > +     val |= dstate;
> > +     dw_pcie_writew_dbi(pci, offset + PCI_PM_CTRL, val);
> 
> Is this a power management register for the *Root Port*, i.e., as
> defined by PCIe r6.0 sec 7.5.2?
> 
> Or is it a similar register for the *Root Complex* as a whole that is
> defined by a Layerscape or DesignWare spec and coincidentally uses the
> same Capability ID and control register layout as the PCIe one?

I think it is root port. Does linux framework can do that for it automatically? 
Or need call pci_set_power_state here instead of write register directly?

> 
> The Root Port programming model is defined by the PCIe spec.  Things
> like .send_turn_off_message() and .exit_from_l2() are clearly part of
> the Root *Complex* programming model that is device-specific and not
> defined by the PCIe spec.
> 
> I'm asking about ls_pcie_set_dstate() because it's written using the
> PCIe constants (PCI_CAP_ID_PM, PCI_PM_CTRL, etc) but it's mixed in
> with these Root Complex things that are *not* part of the PCIe spec.
 > 
> > +static bool ls_pcie_pm_supported(struct ls_pcie *pcie)
> > +{
> > +     if (!dw_pcie_link_up(pcie->pci)) {
> > +             dev_dbg(pcie->pci->dev, "Endpoint isn't present\n");
> > +             return false;
> > +     }
> > +
> > +     return pcie->pm_support;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int ls_pcie_suspend_noirq(struct device *dev)
> > +{
> > +     struct ls_pcie *pcie = dev_get_drvdata(dev);
> > +     struct dw_pcie *pci = pcie->pci;
> > +     u32 val;
> > +     int ret;
> > +
> > +     if (!ls_pcie_pm_supported(pcie))
> > +             return 0;
> > +
> > +     pcie->drvdata->pm_ops->send_turn_off_message(pcie);
> > +
> > +     /* 10ms timeout to check L2 ready */
> > +     ret = readl_poll_timeout(pci->dbi_base + PCIE_PORT_DEBUG0,
> > +                              val, LS_PCIE_IS_L2(val), 100, 10000);
> > +     if (ret) {
> > +             dev_err(dev, "PCIe link enter L2 timeout! ltssm = 0x%x\n", val);
> > +             return ret;
> > +     }
> > +
> > +     ls_pcie_set_dstate(pcie, 0x3);
> > +
> > +     return 0;
> > +}
> > +
> > +static int ls_pcie_resume_noirq(struct device *dev)
> > +{
> > +     struct ls_pcie *pcie = dev_get_drvdata(dev);
> > +     struct dw_pcie *pci = pcie->pci;
> > +     int ret;
> > +
> > +     if (!ls_pcie_pm_supported(pcie))
> > +             return 0;
> 
> How does this work?  You're checking whether the link is up *here*,
> and if it's already up, you go on below to (I guess) set the PCIe
> controller to D0, call dw_pcie_setup_rc() and dw_pcie_wait_for_link().
> Most drivers call dw_pcie_setup_rc() *before* starting the link.
> 
> It looks like when you call ls_pcie_pm_supported() here,
> dw_pcie_link_up() should always return false because the link isn't up
> so it looks like there's no downstream device.  But I must be missing
> something because if that were the case you would never wake anything
> up below.
> 
> > +     ls_pcie_set_dstate(pcie, 0x0);
> > +
> > +     pcie->drvdata->pm_ops->exit_from_l2(pcie);
> > +
> > +     ret = ls_pcie_host_init(&pci->pp);
> > +     if (ret) {
> > +             dev_err(dev, "PCIe host init failed! ret = 0x%x\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     dw_pcie_setup_rc(&pci->pp);
> > +
> > +     ret = dw_pcie_wait_for_link(pci);
> > +     if (ret) {
> > +             dev_err(dev, "Wait link up timeout! ret = 0x%x\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}

Re: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add power management support

[Bjorn Helgaas]

On Tue, Mar 21, 2023 at 09:39:44PM +0000, Frank Li wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: Tuesday, March 21, 2023 3:59 PM
> > To: Frank Li <frank.li@nxp.com>
> > Cc: bhelgaas@google.com; Leo Li <leoyang.li@nxp.com>; dl-linux-imx <linux-
> > imx@nxp.com>; devicetree@vger.kernel.org;
> > gustavo.pimentel@synopsys.com; kw@linux.com; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> > pci@vger.kernel.org; lorenzo.pieralisi@arm.com; M.H. Lian
> > <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>;
> > robh+dt@kernel.org; Roy Zang <roy.zang@nxp.com>;
> > shawnguo@kernel.org; Z.Q. Hou <zhiqiang.hou@nxp.com>
> > Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add power management
> > support
> > 
> > Caution: EXT Email
> > 
> > On Tue, Mar 21, 2023 at 12:02:20PM -0400, Frank Li wrote:
> > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > >
> > > Add PME_Turn_Off/PME_TO_Ack handshake sequence to PCIe devices,
> > such as
> > > NVME or wifi module, and finally put the PCIe controller into D3 state
> > > after the L2/L3 ready state transition process completion.
> > >
> > > However, it's important to note that not all devices may be able to
> > > tolerate the PME_Turn_Off command. In general, fixed PCIe devices
> > > connected to Layerscape, such as NXP wifi devices, are able to handle
> > > this command.
> > 
> > I know this paragraph is here because I asked whether all PCIe devices
> > could tolerate PME_Turn_Off.  I don't know much about that level of
> > the protocol, but it does look to me like PME_Turn_Off is required,
> > e.g., PCIe r6.0, sec 5.3.3.2.1, 5.3.3.4.
> > 
> > So I'm not sure this paragraph adds anything useful.  If the spec
> > requires it, this paragraph is like saying "it's important to note
> > that some PCIe devices may not follow the spec," which is pointless.
> > 
> > This functionality results in any downstream devices being put in
> > D3cold, right?  I think that *would* be worth mentioning.  There are a
> > few cases where we try to avoid putting devices in D3cold, e.g.,
> > no_d3cold, and I suspect this functionality would put them in D3cold
> > regardless of no_d3cold.  Those are corner cases that you would
> > probably never see on your platform, so a brief mention here is
> > probably enough.
> > 
> > > +static void ls_pcie_set_dstate(struct ls_pcie *pcie, u32 dstate)
> > > +{
> > > +     struct dw_pcie *pci = pcie->pci;
> > > +     u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_PM);
> > > +     u32 val;
> > > +
> > > +     val = dw_pcie_readw_dbi(pci, offset + PCI_PM_CTRL);
> > > +     val &= ~PCI_PM_CTRL_STATE_MASK;
> > > +     val |= dstate;
> > > +     dw_pcie_writew_dbi(pci, offset + PCI_PM_CTRL, val);
> > 
> > Is this a power management register for the *Root Port*, i.e., as
> > defined by PCIe r6.0 sec 7.5.2?
> > 
> > Or is it a similar register for the *Root Complex* as a whole that is
> > defined by a Layerscape or DesignWare spec and coincidentally uses the
> > same Capability ID and control register layout as the PCIe one?
> 
> I think it is root port. Does linux framework can do that for it
> automatically?  Or need call pci_set_power_state here instead of
> write register directly?

Well, maybe, the linux framework might already put Root Ports in D3 if
the right conditions are satisfied.  I don't understand all of them,
but you can start at pci_dev_check_d3cold() and look at its users and
instrument things to see what actually happens.

But it might be a problem if that has to be synchronized and done in
the right order with respect to the RC things you do here, because I
don't think there's a hook for the PCI core to call your driver to do
the RC stuff.

Bjorn