Re: [PATCH v8 7/9] PCI: dw-rockchip: Add pme_turn_off support

[Manivannan Sadhasivam]

On 2026-11-05 08:51 +0800, Shawn Lin wrote:
> 在 2026/11/05 星期四 7:47, Sebastian Reichel 写道:
> > Prepare Rockchip PCIe controller for system suspend support by
> > adding the PME turn off operation.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> > ---
> >   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 47 +++++++++++++++++++++++++++
> >   1 file changed, 47 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > index 0e26abc50920..7933c50df340 100644
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -47,6 +47,10 @@
> >   #define  PCIE_CLIENT_ENABLE_LTSSM	FIELD_PREP_WM16(BIT(2), 1)
> >   #define  PCIE_CLIENT_DISABLE_LTSSM	FIELD_PREP_WM16(BIT(2), 0)
> >   
> > +/* Interrupt Status Register for Received Messages */
> > +#define PCIE_CLIENT_INTR_STATUS_MSG_RX	0x04
> > +#define  PME_TO_ACK			BIT(9)
> > +
> >   /* Interrupt Status Register Related to Legacy Interrupt */
> >   #define PCIE_CLIENT_INTR_STATUS_LEGACY	0x8
> >   
> > @@ -71,6 +75,11 @@
> >   #define  PCIE_CLKREQ_READY		FIELD_PREP_WM16(BIT(0), 1)
> >   #define  PCIE_CLKREQ_NOT_READY		FIELD_PREP_WM16(BIT(0), 0)
> >   #define  PCIE_CLKREQ_PULL_DOWN		FIELD_PREP_WM16(GENMASK(13, 12), 1)
> > +#define  PME_READY_ENTER_L23		BIT(3)
> > +
> > +/* Message Request Register */
> > +#define PCIE_CLIENT_MSG_GEN		0x34
> > +#define  PME_TURN_OFF			FIELD_PREP_WM16(BIT(4), 1)
> >   
> >   /* RASDES TBA information */
> >   #define PCIE_CLIENT_CDM_RASDES_TBA_INFO_CMN	0x154
> > @@ -439,8 +448,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;
> > +	u32 status;
> > +	int ret;
> > +
> > +	/* 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_CON,
> > +				 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");
> > +}
> 
> 
> This is a question for the DWC core: should we change
> dw_pcie_host_ops::pme_turn_off() to return a value instead of silently
> ignoring a failure?
> 
> Looking at pcie-designware-host.c, we have:
> 
> if (pci->pp.ops->pme_turn_off) {
>      pci->pp.ops->pme_turn_off(&pci->pp);
> } else {
>      ret = dw_pcie_pme_turn_off(pci);
>      if (ret)
>          return ret;
> }
> 
> The handling feels asymmetric. If ->pme_turn_off fails, shouldn't we 
> abort dw_pcie_suspend_noirq as well?
> 

Sure. The initial callback implementations were just broadcasing PME_Turn_off
message and didn't check for PME_To_Ack. So they ended up being void. But for
PME_To_Ack and L2/L3 Ready timeouts, we should definitely return failure and
abort suspend as the link may not enter L2/L3.

> Other than that, this patch looks good to me in current shape,
> Reviewed-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
> 
> We can further discuss the callback's return value issue on top.
> 

It can be done as a separate series.

- Mani