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

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.garg@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 ?

[...]