[PATCH 3/7] imx8mp: power-domain: Add PCIe support

[Sumit Garg]

On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex at denx.de> wrote:
>
> On 2/20/24 14:10, Sumit Garg wrote:
> > Pre-requisite to enable PCIe support on iMX8MP SoC.
>
> This commit message is useless, write a proper one.
>

How about the following?

    Add support for GPCv2 power domains and clock handling
    for PCIe and PCIe PHY. It is required to enable PCIe support
    on the iMX8MP SoC.

> > Signed-off-by: Sumit Garg <sumit.garg at linaro.org>
> > ---
> >   drivers/power/domain/imx8mp-hsiomix.c | 50 +++++++++++++++++++++++++--
> >   1 file changed, 48 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c
> > index e2d772c5ec7..62145e0261b 100644
> > --- a/drivers/power/domain/imx8mp-hsiomix.c
> > +++ b/drivers/power/domain/imx8mp-hsiomix.c
> > @@ -16,14 +16,19 @@
> >   #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)
> > @@ -43,6 +48,10 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
> >               domain = &priv->pd_usb_phy1;
> >       } else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) {
> >               domain = &priv->pd_usb_phy2;
> > +     } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
> > +             domain = &priv->pd_pcie;
> > +     } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
> > +             domain = &priv->pd_pcie_phy;
> >       } else {
> >               ret = -EINVAL;
> >               goto err_pd;
> > @@ -54,14 +63,25 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
> >
> >       ret = clk_enable(&priv->clk_usb);
> >       if (ret)
> > -             goto err_clk;
> > +             goto err_clk_usb;
> > +
> > +     ret = clk_enable(&priv->clk_pcie);
> > +     if (ret)
> > +             goto err_clk_pcie;
>
> Does this mean that when USB power domains get enabled, PCIe clock are
> also enabled ? Why ?
>
> What if the PCIe clock enable fails, do USB clock remain enabled ?

Let me gate them behind corresponding power domain IDs.

>
> >       if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
> >               setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
> > +     else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
> > +             setbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
> > +     else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
> > +             setbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
> > +                                                 PCIE_PHY_INIT_RST);
>
> Shouldn't the reset bits be cleared here ?
>

Although I can't find their reference in the TRM, as per Linux commit
[1], setting the reset bit is actually deassertion of PCIe PHY reset.
You can think of it like an active low signal.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5506018d3dec41e6678efb92b836586e9ee1d628

-Sumit