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

[Marek Vasut]

On 2/21/24 07:01, Sumit Garg wrote:
> 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.

The second sentence is redundant.

>>> 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

Please add this ^ comment into the code.