On Mon, 26 Feb 2024 at 14:24, Marek Vasut marex@denx.de wrote:
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 ?
Although setbits_le32(addr, 0) should be a nop but...
Why not this:
if (gpr_reg0_bits) setbits_le32(priv->base + GPR_REG0, gpr_reg0_bits);
...if we can avoid that then it's better. I will make it conditional.
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);
But still we need another switch case for clocks and GPR_REG0 bits to be configured.
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 .
I can't see refcounting being implemented in imx8m-power-domain.c. This seems to be an existing issue with USB support too.
break;default:break;} power_domain_off(&priv->pd_bus);
I suppose we should just be fine to drop bus PD off from here.
@@ -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 .
I can't see clk_put() as any generic API. However, there used to be clk_free() which was dropped by this commit [1].
[1] https://source.denx.de/u-boot/u-boot/-/commit/c9309f40a6831b1ac5cd0a7227b5c3...
-Sumit