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

[Shawn Lin <shawn.lin@rock-chips.com>]

在 2025/04/17 星期四 16:17, Diederik de Haas 写道:
> Hi,
> 
> On Fri Apr 11, 2025 at 8:14 AM CEST, 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 exist code.
> 
> s/exist/existing/ ?
> 
>> ...
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>> Changes in v2:
>> - Use NOIRQ_SYSTEM_SLEEP_PM_OPS
>>
>>   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 185 +++++++++++++++++++++++---
>>   1 file changed, 169 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> index 56acfea..7246a49 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"
>>   ...
>>   
>> +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);
> 
> You're using ``rockchip_pcie_phy_deinit(rockchip)`` here ...
> 
>> +	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;
>> +}
>> +
>> +static int rockchip_pcie_resume(struct device *dev)
>> +{
>> +	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>> +	struct dw_pcie *pci = &rockchip->pci;
>> +	int ret;
>> +
>> +	reset_control_assert(rockchip->rst);
>> +
>> +	ret = clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks);
>> +	if (ret) {
>> +		dev_err(dev, "clock init failed\n");
>> +		goto err_clk;
>> +	}
>> +
>> +	if (rockchip->vpcie3v3) {
>> +		ret = regulator_enable(rockchip->vpcie3v3);
>> +		if (ret)
>> +			goto err_power;
>> +	}
>> +
>> +	ret = phy_init(rockchip->phy);
>> +	if (ret) {
>> +		dev_err(dev, "fail to init phy\n");
>> +		goto err_phy_init;
>> +	}
>> +
>> +	ret = phy_power_on(rockchip->phy);
>> +	if (ret) {
>> +		dev_err(dev, "fail to power on phy\n");
>> +		goto err_phy_on;
>> +	}
> 
> ... would it be possible to reuse ``rockchip_pcie_phy_init(rockchip)``
> here ?
> 

yeah, will do.

> otherwise, ``s/fail/failed/`` in the error messages
> 
>> +
>> +	reset_control_deassert(rockchip->rst);
>> +
>> +	rockchip_pcie_writel_apb(rockchip, HIWORD_UPDATE(0xffff, rockchip->intx),
>> +				 PCIE_CLIENT_INTR_MASK_LEGACY);
>> +
>> +	rockchip_pcie_ltssm_enable_control_mode(rockchip, PCIE_CLIENT_RC_MODE);
>> +	rockchip_pcie_unmask_dll_indicator(rockchip);
>> +
>> +	ret = dw_pcie_resume_noirq(pci);
>> +	if (ret) {
>> +		dev_err(dev, "fail to resume\n");
>> +		goto err_resume;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_resume:
>> +	phy_power_off(rockchip->phy);
>> +err_phy_on:
>> +	phy_exit(rockchip->phy);
> 
> I initially thought this sequence was incorrect as I looked at the
> ``rockchip_pcie_phy_deinit`` function:
> 
> 	phy_exit(rockchip->phy);
> 	phy_power_off(rockchip->phy);
> 
> https://elixir.bootlin.com/linux/v6.15-rc1/source/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L411
> 
> But the ``phy_exit`` function docs says "Must be called after phy_power_off()."
> https://elixir.bootlin.com/linux/v6.15-rc1/source/drivers/phy/phy-core.c#L264
> 
> So it seems the code/sequence in this patch is correct, but
> ``rockchip_pcie_phy_deinit`` has it wrong?

Right, it's wrong in rockchip_pcie_phy_deinit() although it doesn't
matter, as the PHY drivers used by Rockchip PCIe actually don't provide
.power_off callback. :)


> 
>> +err_phy_init:
>> +	if (rockchip->vpcie3v3)
>> +		regulator_disable(rockchip->vpcie3v3);
>> +err_power:
>> +	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
>> +err_clk:
>> +	reset_control_deassert(rockchip->rst);
>> +	return ret;
>> +}
>> +
>>   static const struct rockchip_pcie_of_data rockchip_pcie_rc_of_data_rk3568 = {
>>   	.mode = DW_PCIE_RC_TYPE,
>>   };
> 
> Cheers,
>    Diederik

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip