Hello Shawn,
On Fri, Apr 18, 2025 at 09:45:59AM +0800, Shawn Lin wrote:
> This patch adds system PM support for Rockchip platforms by adding
> .pme_turn_off and .get_ltssm hook and tries to reuse possible existing
> code.
>
> It's tested on RK3576 EVB1 board with Some NVMes 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
>
> Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
> ---
>
> 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))
> #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)
> +{
> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
> +
> + return rockchip_pcie_get_ltssm(rockchip) & PCIE_LTSSM_STATUS_MASK;
> +}
The name rockchip_pcie_get_pure_ltssm() is quite confusing.
I think what makes most sense is that:
The current rockchip_pcie_get_ltssm() function is renamed to something like:
rockchip_pcie_get_ltssm_status_reg()
or
rockchip_pcie_get_ltssm_status_reg_raw()
Since some of the users of this function want the some other bits in the
ltssm_status register, that is not the LTSSM state itself.
(This can be done in patch 1/4.)
The new callback / function pointer, .get_ltssm(), is initialized to a
function called:
rockchip_pcie_get_ltssm()
or
rockchip_pcie_get_ltssm_state()
(This can be done in the patch that adds suspend/resume itself (patch 4/4).)
> +
> 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;
> +
> + /* 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) {
> + 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);
> +}
I think that the name of this function is misleading.
The comment:
/* LTSSM enable control mode */
represents:
the field app_ltssm_enable_enhance
i.e. step 9) in the TRM:
"Set the app_ltssm_enable_enhance to enable enhance control mode of
app_ltssm_enable"
See also:
"app_ltssm_enable_enhance is a new way to control the glue behavior of the
app_ltssm_enable. It’s advised set app_ltssm_enable_enhance to “1” when the
version >= 0x50600. The mechanisms of delaying the Hot reset are available
only in app_ltssm_enable_enhance mode."
So I think it is a bit confusing that this new function: rockchip_pcie_ltssm_enable_control_mode()
is doing that _and_ setting the mode to RC mode / EP mode.
Perhaps:
Add a function that adds:
rockchip_pcie_enable_enhanced_ltssm_control_mode()
which does:
+ /* Enable the enhanced control mode of signal app_ltssm_enable */
+ val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
+ rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
(This can be done in patch 2/4.)
Then add a that adds:
rockchip_pcie_set_controller_mode()
which does:
rockchip_pcie_writel_apb(rockchip, mode, PCIE_CLIENT_GENERAL_CONTROL);
(This can be done in patch 3/4.)
Kind regards,
Niklas
