Re: [PATCH v6 07/10] PCI: dw-rockchip: Add pme_turn_off support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incorrect use of FIELD_PREP_WM16() on a Write-1-to-Clear (W1C) interrupt status register, causing unintended clearing of bit 25.
- [Low] Redundant macro definition for the Power Management Control Register.
--

commit 10a270e398b67f78bf174b063ed14cff0ce4e43b
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

    PCI: dw-rockchip: Add pme_turn_off support

This commit prepares the Rockchip PCIe controller for system suspend support
by adding the PME turn off operation.

> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
[ ... ]
> @@ -64,6 +65,11 @@
>  
>  /* Interrupt Mask Register Related to Miscellaneous Operation */
>  #define PCIE_CLIENT_INTR_MASK_MISC	0x24
> +#define PCIE_CLIENT_POWER		0x2c

[Severity: Low]
Does this code introduce a redundant macro definition? It appears that
PCIE_CLIENT_POWER_CON is already defined as 0x2c immediately below
this addition.

> +#define PCIE_CLIENT_MSG_GEN		0x34
> +#define PME_READY_ENTER_L23		BIT(3)
> +#define PME_TURN_OFF			FIELD_PREP_WM16(BIT(4), 1)
> +#define PME_TO_ACK			FIELD_PREP_WM16(BIT(9), 1)
>  
>  /* Power Management Control Register */
>  #define PCIE_CLIENT_POWER_CON		0x2c
[ ... ]
> @@ -452,8 +458,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)
> +{
[ ... ]
> +	/* 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);

[Severity: Medium]
Since PME_TO_ACK uses FIELD_PREP_WM16(), it sets the write mask in the
upper 16 bits (asserting bit 25). If PCIE_CLIENT_INTR_STATUS_MSG_RX is
a Write-1-to-Clear interrupt status register, will writing this WM16
formatted value unintentionally clear the interrupt status for bit 25?
If bit 25 corresponds to an active interrupt, it could be silently lost.

> +	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");
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-rockchip-pcie-system-suspend-v6-0-fbabcca9921b@collabora.com?part=7