Re: [PATCH v7 10/10] PCI: dw-rockchip: Clear debug buffer before entering L2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PCI: dw-rockchip: ineffective debug register clear in rockchip_pcie_suspend()
--

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

PCI: dw-rockchip: Clear debug buffer before entering L2

This commit adds rockchip_pcie_link_status_clear() to write 0x0 to
PCIE_CLIENT_GENERAL_DEBUG during system suspend in rockchip_pcie_suspend() so
that post-resume diagnostics reflect the new link bring-up.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 75b40091bb953..66b3d31d7bc9c 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -81,6 +81,9 @@
>  #define PCIE_CLIENT_MSG_GEN		0x34
>  #define  PME_TURN_OFF			FIELD_PREP_WM16(BIT(4), 1)
>  
> +/* General Debug Register */
> +#define PCIE_CLIENT_GENERAL_DEBUG	0x104
> +
>  /* RASDES TBA information */
>  #define PCIE_CLIENT_CDM_RASDES_TBA_INFO_CMN	0x154
>  #define  PCIE_CLIENT_CDM_RASDES_TBA_L1_1	BIT(4)
> @@ -1039,6 +1042,11 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
>  	return ret;
>  }
>  
> +static void rockchip_pcie_link_status_clear(struct rockchip_pcie *rockchip)
> +{
> +	rockchip_pcie_writel_apb(rockchip, 0x0, PCIE_CLIENT_GENERAL_DEBUG);
> +}
> +
>  static int rockchip_pcie_suspend(struct device *dev)
>  {
>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> @@ -1050,6 +1058,8 @@ static int rockchip_pcie_suspend(struct device *dev)
>  
>  	rockchip->intx = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_MASK_LEGACY);
>  
> +	rockchip_pcie_link_status_clear(rockchip);

[Severity: Medium]
Does clearing PCIE_CLIENT_GENERAL_DEBUG at this point in
rockchip_pcie_suspend() have any effect on link or LTSSM diagnostics in the
upstream driver?

In drivers/pci/controller/dwc/pcie-dw-rockchip.c, PCIE_CLIENT_GENERAL_DEBUG
(0x104) is never written during link bring-up and is never read by any status,
diagnostic, or trace path. Live LTSSM state is read from
PCIE_CLIENT_LTSSM_STATUS and PCIE_CLIENT_CDM_RASDES_TBA_INFO_CMN in
rockchip_pcie_get_ltssm(), while hardware LTSSM state transition history is
recorded in the debug FIFO at PCIE_CLIENT_DBG_FIFO_MODE_CON through
PCIE_CLIENT_DBG_FIFO_STATUS managed by rockchip_pcie_ltssm_trace() in
rockchip_pcie_start_link() and rockchip_pcie_stop_link(). (In the downstream
vendor kernel, PCIE_CLIENT_GENERAL_DEBUG is used as a software scratchpad
register written by vendor-specific link-up code that is not present
upstream.)

Furthermore, even if PCIE_CLIENT_GENERAL_DEBUG latched hardware LTSSM
transitions, rockchip_pcie_link_status_clear() is called before
dw_pcie_suspend_noirq() while the link is still active:

rockchip_pcie_suspend()
  rockchip_pcie_link_status_clear()
  dw_pcie_suspend_noirq()
    rockchip_pcie_pme_turn_off()
    dw_pcie_get_ltssm() /* polls for L2_IDLE */
    dw_pcie_stop_link()
  ...
  clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
  reset_control_assert(rockchip->rst);

Because dw_pcie_suspend_noirq() subsequently broadcasts PME_Turn_Off,
transitions LTSSM to L2_IDLE, and stops the link, and rockchip_pcie_suspend()
then disables clocks and asserts reset_control_assert(rockchip->rst) (which
resets the APB client registers across suspend/resume, requiring
rockchip->intx to be saved and restored), is this write to
PCIE_CLIENT_GENERAL_DEBUG needed or intended to target a different mechanism?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-rockchip-pcie-system-suspend-v7-0-6ae6875939a2@collabora.com?part=10