| Message ID | 20240226080433.3307154-4-sumit.garg@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | imx8mp: Enable PCIe/NVMe support | expand |
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 .
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/c9309f40a6831b1ac5cd0a7227b5c3717d34c812 -Sumit
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); 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: + 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); + 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; @@ -125,8 +168,20 @@ static int imx8mp_hsiomix_probe(struct udevice *dev) if (ret < 0) goto err_pd_usb_phy2; + ret = power_domain_get_by_name(dev, &priv->pd_pcie, "pcie"); + if (ret < 0) + goto err_pd_pcie; + + ret = power_domain_get_by_name(dev, &priv->pd_pcie_phy, "pcie-phy"); + if (ret < 0) + goto err_pd_pcie_phy; + return 0; +err_pd_pcie_phy: + power_domain_free(&priv->pd_pcie); +err_pd_pcie: + power_domain_free(&priv->pd_usb_phy2); err_pd_usb_phy2: power_domain_free(&priv->pd_usb_phy1); err_pd_usb_phy1:
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(-)