On Thu Apr 17, 2025 at 10:29 AM CEST, Shawn Lin wrote:
> 在 2025/04/17 星期四 16:17, Diederik de Haas 写道:
>> 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@xxxxxxxxxxxxxx>
>>> ---
>>>
>>> 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);
I just noticed another inconsistency:
In ``rockchip_pcie_probe`` from line 657 I see this:
```
deinit_clk:
clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
deinit_phy:
rockchip_pcie_phy_deinit(rockchip);
disable_regulator:
if (rockchip->vpcie3v3)
regulator_disable(rockchip->vpcie3v3);
```
https://elixir.bootlin.com/linux/v6.15-rc1/source/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L657
Which is not in the same sequence as the code in this patch.
Shouldn't those be in the same sequence?
>>> +
>>> + 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. :)
If you have no objections, I'd still like to send a patch to fix it.
Cheers,
Diederik
>>
>>> +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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Attachment:
signature.asc
Description: PGP signature
