Re: [PATCH] pci: rockchip: Release resources on failing probe

[Kever Yang]

On 2023/7/12 07:13, Jonas Karlman wrote:

The PCIe driver for RK3399 is affected by a similar issue that was fixed
for RK35xx in the commit e04b67a7f4c1 ("pci: pcie_dw_rockchip: release
resources on failing probe").

Resources are not released on failing probe, e.g. regulators may be left
enabled and the ep-gpio may be left in a requested state.

Change to use regulator_set_enable_if_allowed and disable regulators
after failure to keep regulator enable count balanced, ep-gpio is also
released on regulator failure.

Also add support for the vpcie12v-supply, remove unused include and
check return value from dev_read_addr_name.

Signed-off-by: Jonas Karlman <jo...@kwiboo.se>

Reviewed-by: Kever Yang <kever.y...@rock-chips.com>

Thanks,
- Kever

---
  drivers/pci/pcie_rockchip.c | 108 +++++++++++++++++++-----------------
  1 file changed, 57 insertions(+), 51 deletions(-)

diff --git a/drivers/pci/pcie_rockchip.c b/drivers/pci/pcie_rockchip.c
index 72b41398f27b..624841e9d8b8 100644
--- a/drivers/pci/pcie_rockchip.c
+++ b/drivers/pci/pcie_rockchip.c
@@ -12,23 +12,15 @@
   */

#include <common.h>

-#include <clk.h>
  #include <dm.h>
-#include <asm/global_data.h>
  #include <dm/device_compat.h>
  #include <generic-phy.h>
  #include <pci.h>
-#include <power-domain.h>
  #include <power/regulator.h>
  #include <reset.h>
-#include <syscon.h>
-#include <asm/io.h>
  #include <asm-generic/gpio.h>
-#include <asm/arch-rockchip/clock.h>
  #include <linux/iopoll.h>

-DECLARE_GLOBAL_DATA_PTR;

-
  #define HIWORD_UPDATE(mask, val)        (((mask) << 16) | (val))
  #define HIWORD_UPDATE_BIT(val)          HIWORD_UPDATE(val, val)

@@ -383,41 +375,38 @@ static int rockchip_pcie_set_vpcie(struct udevice *dev)

        struct rockchip_pcie *priv = dev_get_priv(dev);
        int ret;

- if (priv->vpcie3v3) {

-               ret = regulator_set_enable(priv->vpcie3v3, true);
-               if (ret) {
-                       dev_err(dev, "failed to enable vpcie3v3 (ret=%d)\n",
-                               ret);
-                       return ret;
-               }
+       ret = regulator_set_enable_if_allowed(priv->vpcie12v, true);
+       if (ret && ret != -ENOSYS) {
+               dev_err(dev, "failed to enable vpcie12v (ret=%d)\n", ret);
+               return ret;
        }

- if (priv->vpcie1v8) {

-               ret = regulator_set_enable(priv->vpcie1v8, true);
-               if (ret) {
-                       dev_err(dev, "failed to enable vpcie1v8 (ret=%d)\n",
-                               ret);
-                       goto err_disable_3v3;
-               }
+       ret = regulator_set_enable_if_allowed(priv->vpcie3v3, true);
+       if (ret && ret != -ENOSYS) {
+               dev_err(dev, "failed to enable vpcie3v3 (ret=%d)\n", ret);
+               goto err_disable_12v;
        }

- if (priv->vpcie0v9) {

-               ret = regulator_set_enable(priv->vpcie0v9, true);
-               if (ret) {
-                       dev_err(dev, "failed to enable vpcie0v9 (ret=%d)\n",
-                               ret);
-                       goto err_disable_1v8;
-               }
+       ret = regulator_set_enable_if_allowed(priv->vpcie1v8, true);
+       if (ret && ret != -ENOSYS) {
+               dev_err(dev, "failed to enable vpcie1v8 (ret=%d)\n", ret);
+               goto err_disable_3v3;
+       }
+
+       ret = regulator_set_enable_if_allowed(priv->vpcie0v9, true);
+       if (ret && ret != -ENOSYS) {
+               dev_err(dev, "failed to enable vpcie0v9 (ret=%d)\n", ret);
+               goto err_disable_1v8;
        }

return 0; err_disable_1v8:

-       if (priv->vpcie1v8)
-               regulator_set_enable(priv->vpcie1v8, false);
+       regulator_set_enable_if_allowed(priv->vpcie1v8, false);
  err_disable_3v3:
-       if (priv->vpcie3v3)
-               regulator_set_enable(priv->vpcie3v3, false);
+       regulator_set_enable_if_allowed(priv->vpcie3v3, false);
+err_disable_12v:
+       regulator_set_enable_if_allowed(priv->vpcie12v, false);
        return ret;
  }

@@ -427,19 +416,12 @@ static int rockchip_pcie_parse_dt(struct udevice *dev)

        int ret;

priv->axi_base = dev_read_addr_name(dev, "axi-base");

-       if (!priv->axi_base)
-               return -ENODEV;
+       if (priv->axi_base == FDT_ADDR_T_NONE)
+               return -EINVAL;

priv->apb_base = dev_read_addr_name(dev, "apb-base");

-       if (!priv->axi_base)
-               return -ENODEV;
-
-       ret = gpio_request_by_name(dev, "ep-gpios", 0,
-                                  &priv->ep_gpio, GPIOD_IS_OUT);
-       if (ret) {
-               dev_err(dev, "failed to find ep-gpios property\n");
-               return ret;
-       }
+       if (priv->apb_base == FDT_ADDR_T_NONE)
+               return -EINVAL;

ret = reset_get_by_name(dev, "core", &priv->core_rst);

        if (ret) {
@@ -483,6 +465,13 @@ static int rockchip_pcie_parse_dt(struct udevice *dev)
                return ret;
        }

+ ret = device_get_supply_regulator(dev, "vpcie12v-supply",

+                                         &priv->vpcie12v);
+       if (ret && ret != -ENOENT) {
+               dev_err(dev, "failed to get vpcie12v supply (ret=%d)\n", ret);
+               return ret;
+       }
+
        ret = device_get_supply_regulator(dev, "vpcie3v3-supply",
                                          &priv->vpcie3v3);
        if (ret && ret != -ENOENT) {
@@ -510,6 +499,13 @@ static int rockchip_pcie_parse_dt(struct udevice *dev)
                return ret;
        }

+ ret = gpio_request_by_name(dev, "ep-gpios", 0,

+                                  &priv->ep_gpio, GPIOD_IS_OUT);
+       if (ret) {
+               dev_err(dev, "failed to find ep-gpios property\n");
+               return ret;
+       }
+
        return 0;
  }

@@ -529,16 +525,26 @@ static int rockchip_pcie_probe(struct udevice *dev) ret = rockchip_pcie_set_vpcie(dev);

        if (ret)
-               return ret;
+               goto err_gpio_free;

ret = rockchip_pcie_init_port(dev);

        if (ret)
-               return ret;
+               goto err_disable_vpcie;

dev_info(dev, "PCIE-%d: Link up (Bus%d)\n",

                 dev_seq(dev), hose->first_busno);

return 0;

+
+err_disable_vpcie:
+       regulator_set_enable_if_allowed(priv->vpcie0v9, false);
+       regulator_set_enable_if_allowed(priv->vpcie1v8, false);
+       regulator_set_enable_if_allowed(priv->vpcie3v3, false);
+       regulator_set_enable_if_allowed(priv->vpcie12v, false);
+err_gpio_free:
+       if (dm_gpio_is_valid(&priv->ep_gpio))
+               dm_gpio_free(dev, &priv->ep_gpio);
+       return ret;
  }

static const struct dm_pci_ops rockchip_pcie_ops = {

@@ -552,10 +558,10 @@ static const struct udevice_id rockchip_pcie_ids[] = {
  };

U_BOOT_DRIVER(rockchip_pcie) = {

-       .name                   = "rockchip_pcie",
-       .id                     = UCLASS_PCI,
-       .of_match               = rockchip_pcie_ids,
-       .ops                    = &rockchip_pcie_ops,
-       .probe                  = rockchip_pcie_probe,
+       .name           = "rockchip_pcie",
+       .id             = UCLASS_PCI,
+       .of_match       = rockchip_pcie_ids,
+       .ops            = &rockchip_pcie_ops,
+       .probe          = rockchip_pcie_probe,
        .priv_auto      = sizeof(struct rockchip_pcie),
  };