Re: [PATCH 3/7] imx8mp: power-domain: Add PCIe support

[Marek Vasut]

On 2/20/24 14:10, Sumit Garg wrote:
Pre-requisite to enable PCIe support on iMX8MP SoC.

This commit message is useless, write a proper one.

Signed-off-by: Sumit Garg <sumit.g...@linaro.org>
---
  drivers/power/domain/imx8mp-hsiomix.c | 50 +++++++++++++++++++++++++--
  1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/power/domain/imx8mp-hsiomix.c 
b/drivers/power/domain/imx8mp-hsiomix.c
index e2d772c5ec7..62145e0261b 100644
--- a/drivers/power/domain/imx8mp-hsiomix.c
+++ b/drivers/power/domain/imx8mp-hsiomix.c
@@ -16,14 +16,19 @@
  #define GPR_REG0              0x0
  #define  PCIE_CLOCK_MODULE_EN BIT(0)
  #define  USB_CLOCK_MODULE_EN  BIT(1)
+#define  PCIE_PHY_APB_RST      BIT(4)
+#define  PCIE_PHY_INIT_RST     BIT(5)
  
  struct imx8mp_hsiomix_priv {
        void __iomem *base;
        struct clk clk_usb;
+       struct clk clk_pcie;
        struct power_domain pd_bus;
        struct power_domain pd_usb;
+       struct power_domain pd_pcie;
        struct power_domain pd_usb_phy1;
        struct power_domain pd_usb_phy2;
+       struct power_domain pd_pcie_phy;
  };
  
  static int imx8mp_hsiomix_on(struct power_domain *power_domain)
@@ -43,6 +48,10 @@ static int imx8mp_hsiomix_on(struct power_domain 
*power_domain)
                domain = &priv->pd_usb_phy1;
        } else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) {
                domain = &priv->pd_usb_phy2;
+       } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
+               domain = &priv->pd_pcie;
+       } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
+               domain = &priv->pd_pcie_phy;
        } else {
                ret = -EINVAL;
                goto err_pd;
@@ -54,14 +63,25 @@ static int imx8mp_hsiomix_on(struct power_domain 
*power_domain)
  
  	ret = clk_enable(&priv->clk_usb);
        if (ret)
-               goto err_clk;
+               goto err_clk_usb;
+
+       ret = clk_enable(&priv->clk_pcie);
+       if (ret)
+               goto err_clk_pcie;

Does this mean that when USB power domains get enabled, PCIe clock are 
also enabled ? Why ?

What if the PCIe clock enable fails, do USB clock remain enabled ?

        if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
                setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
+       else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
+               setbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
+       else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
+               setbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
+                                                   PCIE_PHY_INIT_RST);

Shouldn't the reset bits be cleared here ?

[...]