[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