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@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);
> +
> + 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 ?
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?
> +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
Attachment:
signature.asc
Description: PGP signature
