[PATCH v3] PCI: dw-rockchip: Add system PM support
Manivannan Sadhasivam
manivannan.sadhasivam at linaro.org
Sun Apr 27 02:53:25 PDT 2025
On Fri, Apr 18, 2025 at 09:45:59AM +0800, Shawn Lin wrote:
> This patch adds system PM support for Rockchip platforms by adding
Both subject and commit message should mention that PM support is added only
for RC mode.
> .pme_turn_off and .get_ltssm hook and tries to reuse possible existing
> code.
>
> It's tested on RK3576 EVB1 board with Some NVMes
Oh, really? How did they survive once you turn off the power during suspend()?
NVMe driver expects the device to be in D0 even during suspend to avoid resume
latency.
> and PCIe-2-SATA/XHCI
> devices. And check the PCIe protocol analyzer to make sure the L2 process
> fits the spec.
>
> nvme nvme0: missing or invalid SUBNQN field.
> nvme nvme0: allocated 64 MiB host memory buffer (16 segments).
> nvme nvme0: 8/0/0 default/read/poll queues
> nvme nvme0: Ignoring bogus Namespace Identifiers
>
> echo N > /sys/module/printk/parameters/console_suspend
> echo core > /sys/power/pm_test
> echo mem > /sys/power/state
>
> PM: suspend entry (deep)
> Filesystems sync: 0.000 seconds
> Freezing user space processes
> Freezing user space processes completed (elapsed 0.001 seconds)
> OOM killer disabled.
> Freezing remaining freezable tasks
> Freezing remaining freezable tasks completed (elapsed 0.000 seconds)
>
> ...
>
> rockchip-dw-pcie 22400000.pcie: PCIe Gen.2 x1 link up
> OOM killer enabled.
> Restarting tasks ... done.
> random: crng reseeded on system resumption
> PM: suspend exit
> nvme nvme0: 8/0/0 default/read/poll queues
> nvme nvme0: Ignoring bogus Namespace Identifiers
>
None of these logs are relevant to be mentioned in patch description.
> Signed-off-by: Shawn Lin <shawn.lin at rock-chips.com>
> ---
>
> Changes in v3:
> - amend the commit msg suggested by Bjorn
> - reuse more code suggested by Diederik
> - bail out EP case suggested by Niklas
>
> Changes in v2:
> - Use NOIRQ_SYSTEM_SLEEP_PM_OPS
>
> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 197 +++++++++++++++++++++++---
> 1 file changed, 177 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 56acfea..4bcd4006 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -21,6 +21,7 @@
> #include <linux/regmap.h>
> #include <linux/reset.h>
>
> +#include "../../pci.h"
> #include "pcie-designware.h"
>
> /*
> @@ -37,8 +38,14 @@
> #define PCIE_CLIENT_EP_MODE HIWORD_UPDATE(0xf0, 0x0)
> #define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc)
> #define PCIE_CLIENT_DISABLE_LTSSM HIWORD_UPDATE(0x0c, 0x8)
> +#define PCIE_CLIENT_INTR_STATUS_MSG_RX 0x04
> #define PCIE_CLIENT_INTR_STATUS_MISC 0x10
> #define PCIE_CLIENT_INTR_MASK_MISC 0x24
> +#define PCIE_CLIENT_POWER 0x2c
> +#define PCIE_CLIENT_MSG_GEN 0x34
> +#define PME_READY_ENTER_L23 BIT(3)
> +#define PME_TURN_OFF (BIT(4) | BIT(20))
> +#define PME_TO_ACK (BIT(9) | BIT(25))
Why two bitfields are combined? Is it possible to add definitions for each of
them?
> #define PCIE_SMLH_LINKUP BIT(16)
> #define PCIE_RDLH_LINKUP BIT(17)
> #define PCIE_LINKUP (PCIE_SMLH_LINKUP | PCIE_RDLH_LINKUP)
> @@ -63,6 +70,7 @@ struct rockchip_pcie {
> struct gpio_desc *rst_gpio;
> struct regulator *vpcie3v3;
> struct irq_domain *irq_domain;
> + u32 intx;
> const struct rockchip_pcie_of_data *data;
> };
>
> @@ -159,6 +167,13 @@ static u32 rockchip_pcie_get_ltssm(struct rockchip_pcie *rockchip)
> return rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS);
> }
>
> +static u32 rockchip_pcie_get_pure_ltssm(struct dw_pcie *pci)
What does 'pure_ltssm' mean? You are just masking out some bits from
LTSSM_STATUS. What do they correspond to?
> +{
> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
> +
> + return rockchip_pcie_get_ltssm(rockchip) & PCIE_LTSSM_STATUS_MASK;
> +}
> +
> static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip)
> {
> rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_ENABLE_LTSSM,
> @@ -248,8 +263,46 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
> return 0;
> }
>
> +static void rockchip_pcie_pme_turn_off(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
> + struct device *dev = rockchip->pci.dev;
> + int ret;
> + u32 status;
Reverse Xmas order please (for possible variables).
> +
> + /* 1. Broadcast PME_Turn_Off Message, bit 4 self-clear once done */
> + rockchip_pcie_writel_apb(rockchip, PME_TURN_OFF, PCIE_CLIENT_MSG_GEN);
> + ret = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_MSG_GEN,
> + status, !(status & BIT(4)), PCIE_PME_TO_L2_TIMEOUT_US / 10,
> + PCIE_PME_TO_L2_TIMEOUT_US);
> + if (ret) {
> + dev_warn(dev, "Failed to send PME_Turn_Off\n");
> + return;
> + }
> +
> + /* 2. Wait for PME_TO_Ack, bit 9 will be set once received */
> + ret = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_INTR_STATUS_MSG_RX,
> + status, status & BIT(9), PCIE_PME_TO_L2_TIMEOUT_US / 10,
> + PCIE_PME_TO_L2_TIMEOUT_US);
> + if (ret) {
> + dev_warn(dev, "Failed to receive PME_TO_Ack\n");
> + return;
> + }
> +
> + /* 3. Clear PME_TO_Ack and Wait for ready to enter L23 message */
> + rockchip_pcie_writel_apb(rockchip, PME_TO_ACK, PCIE_CLIENT_INTR_STATUS_MSG_RX);
> + ret = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_POWER,
> + status, status & PME_READY_ENTER_L23,
> + PCIE_PME_TO_L2_TIMEOUT_US / 10,
> + PCIE_PME_TO_L2_TIMEOUT_US);
> + if (ret)
> + dev_err(dev, "Failed to get ready to enter L23 message\n");
> +}
> +
> static const struct dw_pcie_host_ops rockchip_pcie_host_ops = {
> .init = rockchip_pcie_host_init,
> + .pme_turn_off = rockchip_pcie_pme_turn_off,
> };
>
> /*
> @@ -404,10 +457,12 @@ static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip)
> struct device *dev = rockchip->pci.dev;
> int ret;
>
> - rockchip->phy = devm_phy_get(dev, "pcie-phy");
> - if (IS_ERR(rockchip->phy))
> - return dev_err_probe(dev, PTR_ERR(rockchip->phy),
> - "missing PHY\n");
> + if (!rockchip->phy) {
To avoid this, could you please move both devm_phy_get() and
devm_clk_bulk_get_all() to rockchip_pcie_resource_get()? This ensures that all
resource get code are contained in a single function and that function gets
called only during probe().
You should do it in a separate patch before this one.
> + rockchip->phy = devm_phy_get(dev, "pcie-phy");
> + if (IS_ERR(rockchip->phy))
> + return dev_err_probe(dev, PTR_ERR(rockchip->phy),
> + "missing PHY\n");
> + }
>
> ret = phy_init(rockchip->phy);
> if (ret < 0)
> @@ -430,6 +485,7 @@ static const struct dw_pcie_ops dw_pcie_ops = {
> .link_up = rockchip_pcie_link_up,
> .start_link = rockchip_pcie_start_link,
> .stop_link = rockchip_pcie_stop_link,
> + .get_ltssm = rockchip_pcie_get_pure_ltssm,
> };
>
> static irqreturn_t rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
> @@ -489,13 +545,32 @@ static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
> return IRQ_HANDLED;
> }
>
> +static void rockchip_pcie_ltssm_enable_control_mode(struct rockchip_pcie *rockchip, u32 mode)
> +{
> + u32 val;
> +
> + /* LTSSM enable control mode */
> + val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
> + rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
> +
> + rockchip_pcie_writel_apb(rockchip, mode, PCIE_CLIENT_GENERAL_CONTROL);
> +}
> +
> +static void rockchip_pcie_unmask_dll_indicator(struct rockchip_pcie *rockchip)
> +{
> + u32 val;
> +
> + /* unmask DLL up/down indicator */
> + val = HIWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED, 0);
> + rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
> +}
> +
> static int rockchip_pcie_configure_rc(struct platform_device *pdev,
> struct rockchip_pcie *rockchip)
> {
> struct device *dev = &pdev->dev;
> struct dw_pcie_rp *pp;
> int irq, ret;
> - u32 val;
>
> if (!IS_ENABLED(CONFIG_PCIE_ROCKCHIP_DW_HOST))
> return -ENODEV;
> @@ -512,12 +587,7 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
> return ret;
> }
>
> - /* LTSSM enable control mode */
> - val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
> - rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
> -
> - rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE,
> - PCIE_CLIENT_GENERAL_CONTROL);
> + rockchip_pcie_ltssm_enable_control_mode(rockchip, PCIE_CLIENT_RC_MODE);
>
> pp = &rockchip->pci.pp;
> pp->ops = &rockchip_pcie_host_ops;
> @@ -529,9 +599,7 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
> return ret;
> }
>
> - /* unmask DLL up/down indicator */
> - val = HIWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED, 0);
> - rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
> + rockchip_pcie_unmask_dll_indicator(rockchip);
>
> return ret;
> }
> @@ -558,12 +626,7 @@ static int rockchip_pcie_configure_ep(struct platform_device *pdev,
> return ret;
> }
>
> - /* LTSSM enable control mode */
> - val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
> - rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
> -
> - rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_EP_MODE,
> - PCIE_CLIENT_GENERAL_CONTROL);
> + rockchip_pcie_ltssm_enable_control_mode(rockchip, PCIE_CLIENT_EP_MODE);
>
> rockchip->pci.ep.ops = &rockchip_pcie_ep_ops;
> rockchip->pci.ep.page_size = SZ_64K;
> @@ -677,6 +740,94 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> return ret;
> }
>
> +static int rockchip_pcie_suspend(struct device *dev)
> +{
> + struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> + struct dw_pcie *pci = &rockchip->pci;
> + int ret;
> +
> + if (rockchip->data->mode == DW_PCIE_EP_TYPE) {
> + dev_err(dev, "suspend is not supported in EP mode\n");
> + return -EOPNOTSUPP;
One of the reasons why I hate clubbing both RC and EP in a single driver. But
fine for now.
> + }
> +
> + rockchip->intx = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_MASK_LEGACY);
> +
> + ret = dw_pcie_suspend_noirq(pci);
> + if (ret) {
> + dev_err(dev, "failed to suspend\n");
Please print the errno also. Or better drop the error message and rely on PM
core.
> + return ret;
> + }
> +
> + rockchip_pcie_phy_deinit(rockchip);
> + clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> + reset_control_assert(rockchip->rst);
> + if (rockchip->vpcie3v3)
> + regulator_disable(rockchip->vpcie3v3);
> + gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
PERST# should be asserted in advance of power being switched off, not after.
> +
> + return 0;
> +}
> +
> +static int rockchip_pcie_resume(struct device *dev)
> +{
> + struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> + struct dw_pcie *pci = &rockchip->pci;
> + int ret;
> +
> + if (rockchip->data->mode == DW_PCIE_EP_TYPE) {
> + dev_err(dev, "resume is not supported in EP mode\n");
> + return -EOPNOTSUPP;
> + }
> +
> + reset_control_assert(rockchip->rst);
> +
Why? reset is supposed to be asserted in suspend() itself.
> + ret = clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks);
> + if (ret) {
> + dev_err(dev, "clock init failed\n");
As said above, print the errno.
> + goto err_clk;
> + }
> +
> + if (rockchip->vpcie3v3) {
> + ret = regulator_enable(rockchip->vpcie3v3);
> + if (ret)
> + goto err_power;
Please name the goto labels as per their purpose, not per the point of failure.
Like, 'err_disablk_clk'.
> + }
> +
> + ret = rockchip_pcie_phy_init(rockchip);
> + if (ret) {
> + dev_err(dev, "phy init failed\n");
> + goto err_phy_init;
> + }
> +
> + reset_control_deassert(rockchip->rst);
> +
> + rockchip_pcie_writel_apb(rockchip, HIWORD_UPDATE(0xffff, rockchip->intx),
> + PCIE_CLIENT_INTR_MASK_LEGACY);
> +
> + rockchip_pcie_ltssm_enable_control_mode(rockchip, PCIE_CLIENT_RC_MODE);
> + rockchip_pcie_unmask_dll_indicator(rockchip);
> +
> + ret = dw_pcie_resume_noirq(pci);
I had to check the driver to see where PERST# is getting deasserted and it
happens in start_link() callback. Since you assert it in suspend(), you should
explicitly do it in resume() to maintain symmetry.
> + if (ret) {
> + dev_err(dev, "fail to resume\n");
> + goto err_resume;
> + }
> +
> + return 0;
> +
> +err_resume:
> + rockchip_pcie_phy_deinit(rockchip);
> +err_phy_init:
> + if (rockchip->vpcie3v3)
> + regulator_disable(rockchip->vpcie3v3);
> +err_power:
> + clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> +err_clk:
> + reset_control_deassert(rockchip->rst);
Why do you want to deassert the reset in the case of failure? If you get rid of
the assert at the start as I suggested, this label can be removed altogether.
- Mani
--
மணிவண்ணன் சதாசிவம்
More information about the Linux-rockchip
mailing list