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

On 2/26/24 9:04 AM, Sumit Garg wrote:

Add support for GPCv2 power domains and clock handling for PCIe and PCIe PHY.

Signed-off-by: Sumit Garg sumit.garg@linaro.org

drivers/power/domain/imx8mp-hsiomix.c | 101 ++++++++++++++++++++------ 1 file changed, 78 insertions(+), 23 deletions(-)

diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c index e2d772c5ec78..58cc3f63bb56 100644 --- a/drivers/power/domain/imx8mp-hsiomix.c +++ b/drivers/power/domain/imx8mp-hsiomix.c @@ -16,48 +16,73 @@ #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) { struct udevice *dev = power_domain->dev; struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);

• struct power_domain *domain;

• struct power_domain *domain = NULL;
• struct clk *clk = NULL;
• u32 gpr_reg0_bits = 0; int ret;

• ret = power_domain_on(&priv->pd_bus);
• if (ret)

• return ret;
  

• if (power_domain->id == IMX8MP_HSIOBLK_PD_USB) {

• switch (power_domain->id) {
• case IMX8MP_HSIOBLK_PD_USB: domain = &priv->pd_usb;

• } else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY1) {

• clk = &priv->clk_usb;
  

• gpr_reg0_bits |= USB_CLOCK_MODULE_EN;
  

• break;
  

• case IMX8MP_HSIOBLK_PD_USB_PHY1: domain = &priv->pd_usb_phy1;

• } else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) {

• break;
  

• case IMX8MP_HSIOBLK_PD_USB_PHY2: domain = &priv->pd_usb_phy2;

• } else {

• ret = -EINVAL;
  

• goto err_pd;
  

• break;
  

• case IMX8MP_HSIOBLK_PD_PCIE:

• domain = &priv->pd_pcie;
  

• clk = &priv->clk_pcie;
  

• gpr_reg0_bits |= PCIE_CLOCK_MODULE_EN;
  

• break;
  

• case IMX8MP_HSIOBLK_PD_PCIE_PHY:

• domain = &priv->pd_pcie_phy;
  

• /* Bits to deassert PCIe PHY reset */
  

• gpr_reg0_bits |= PCIE_PHY_APB_RST | PCIE_PHY_INIT_RST;
  

• break;
  

• default:

• dev_err(dev, "unknown power domain id: %ld\n",
  

• 	power_domain->id);
  

• return -EINVAL;
  

  }

• ret = power_domain_on(&priv->pd_bus);

• if (ret)

• return ret;
  

• ret = power_domain_on(domain); if (ret) goto err_pd;

• ret = clk_enable(&priv->clk_usb);
• if (ret)

• goto err_clk;
  

• if (clk) {

• ret = clk_enable(clk);
  

• if (ret)
  

• 	goto err_clk;
  

• }

• if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)

• setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
  

• setbits_le32(priv->base + GPR_REG0, gpr_reg0_bits);

Are you sure it is OK to do this unconditionally ?

Why not this:

if (gpr_reg0_bits) setbits_le32(priv->base + GPR_REG0, gpr_reg0_bits);

return 0;

@@ -73,17 +98,31 @@ static int imx8mp_hsiomix_off(struct power_domain *power_domain) struct udevice *dev = power_domain->dev; struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);

• if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)

• switch (power_domain->id) {
• case IMX8MP_HSIOBLK_PD_USB: clrbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);

• clk_disable(&priv->clk_usb);
• if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)

• clk_disable(&priv->clk_usb);
  

  power_domain_off(&priv->pd_usb);

• else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY1)

• break;
  

• case IMX8MP_HSIOBLK_PD_USB_PHY1: power_domain_off(&priv->pd_usb_phy1);

• else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2)

• break;
  

• case IMX8MP_HSIOBLK_PD_USB_PHY2: power_domain_off(&priv->pd_usb_phy2);

• break;
  

• case IMX8MP_HSIOBLK_PD_PCIE:

Maybe we need some function ... imx8mp_hsiomix_id_to_pd() to avoid this duplicate switch statement:

pd = imx8mp_hsiomix_id_to_pd(...) ... power_domain_off(fd);

• clrbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
  

• clk_disable(&priv->clk_pcie);
  

• power_domain_off(&priv->pd_pcie);
  

• break;
  

• case IMX8MP_HSIOBLK_PD_PCIE_PHY:

• clrbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
  

• 				    PCIE_PHY_INIT_RST);
  

• power_domain_off(&priv->pd_pcie_phy);
  

Is it OK to turn off the bus PD after this ? Please double-check if power_domain_on/off() is refcounted .

• break;
  

• default:

• break;
  

• }

  power_domain_off(&priv->pd_bus);

@@ -109,6 +148,10 @@ static int imx8mp_hsiomix_probe(struct udevice *dev) if (ret < 0) return ret;

• ret = clk_get_by_name(dev, "pcie", &priv->clk_pcie);
• if (ret < 0)

• return ret;
  

• ret = power_domain_get_by_name(dev, &priv->pd_bus, "bus"); if (ret < 0) return ret;

Some clk_put() seems to be missing for clk_pcie .