[PATCH v2 3/8] imx8mp: power-domain: Add PCIe support
Sumit Garg
sumit.garg at linaro.org
Mon Feb 26 12:46:18 CET 2024
On Mon, 26 Feb 2024 at 14:24, Marek Vasut <marex at 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 at 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
More information about the U-Boot
mailing list