Re: [PATCH] PCI: dw-rockchip: Add system PM support

[Damien Le Moal <dlemoal@kernel.org>]

On 4/11/25 11:26, Shawn Lin wrote:
> 在 2025/04/11 星期五 10:02, Damien Le Moal 写道:
>> On 4/10/25 15:50, Shawn Lin wrote:
>>> +static int rockchip_pcie_suspend(struct device *dev)
>>> +{
>>> +	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>> +	struct dw_pcie *pci = &rockchip->pci;
>>> +	int ret;
>>> +
>>> +	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");
>>> +		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);
>>> +
>>> +	return 0;
>>> +}
>>
>> This function needs a __maybe_unused in its declaration, otherwise, you get a
>> compilation warning when PM is not enabled.
>>
>> static int __maybe_unused rockchip_pcie_suspend(struct device *dev)
>>
>>
> 
> Emm.. I don't see any host drivers with system PM support under
> drivers/pci/controller/ adds these :)
> 
> #grep suspend drivers/pci/controller/ -rn | grep __maybe_unused  | wc -l
> 0
> 
> Anyway, will fix it.

If you do not add __maybe_unused, you get:

  CC      drivers/pci/controller/dwc/pcie-dw-rockchip.o
drivers/pci/controller/dwc/pcie-dw-rockchip.c:761:12: warning:
‘rockchip_pcie_resume’ defined but not used [-Wunused-function]
  761 | static int rockchip_pcie_resume(struct device *dev)
      |            ^~~~~~~~~~~~~~~~~~~~
drivers/pci/controller/dwc/pcie-dw-rockchip.c:737:12: warning:
‘rockchip_pcie_suspend’ defined but not used [-Wunused-function]
  737 | static int rockchip_pcie_suspend(struct device *dev)
      |            ^~~~~~~~~~~~~~~~~~~~~

You do not get this for other controllers because they use
NOIRQ_SYSTEM_SLEEP_PM_OPS() to set the PM ops. Your patch uses
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() which is defined as:

#ifdef CONFIG_PM_SLEEP
#define SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
	NOIRQ_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
#else
#define SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
#endif

So unlike using directly NOIRQ_SYSTEM_SLEEP_PM_OPS(), the functions names are
actually never used when CONFIG_PM_SLEEP is not enabled.

So the fix is to do like other controllers and use NOIRQ_SYSTEM_SLEEP_PM_OPS()
or use __maybe_unused.

-- 
Damien Le Moal
Western Digital Research