diff mbox series

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

Message ID 20240220131056.2962331-4-sumit.garg@linaro.org
State New
Headers show
Series imx8mp: Enable PCIe/NVMe support | expand

Commit Message

Sumit Garg Feb. 20, 2024, 1:10 p.m. UTC 
Pre-requisite to enable PCIe support on iMX8MP SoC.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/power/domain/imx8mp-hsiomix.c | 50 +++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

Comments

Marek Vasut Feb. 20, 2024, 3:14 p.m. UTC | #1 
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.

> Signed-off-by: Sumit Garg <sumit.garg@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 ?

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

[...]
Sumit Garg Feb. 21, 2024, 6:01 a.m. UTC | #2 
On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@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@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
Marek Vasut Feb. 21, 2024, 9:32 a.m. UTC | #3 
On 2/21/24 07:01, Sumit Garg wrote:
> On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@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@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.
diff mbox series

Patch

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;
 
 	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);
 
 	return 0;
 
-err_clk:
+err_clk_pcie:
+	clk_disable(&priv->clk_usb);
+err_clk_usb:
 	power_domain_off(domain);
 err_pd:
 	power_domain_off(&priv->pd_bus);
@@ -75,8 +95,14 @@  static int imx8mp_hsiomix_off(struct power_domain *power_domain)
 
 	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
 		clrbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
+	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
+		clrbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
+	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
+		clrbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
+						    PCIE_PHY_INIT_RST);
 
 	clk_disable(&priv->clk_usb);
+	clk_disable(&priv->clk_pcie);
 
 	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
 		power_domain_off(&priv->pd_usb);
@@ -84,6 +110,10 @@  static int imx8mp_hsiomix_off(struct power_domain *power_domain)
 		power_domain_off(&priv->pd_usb_phy1);
 	else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2)
 		power_domain_off(&priv->pd_usb_phy2);
+	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
+		power_domain_off(&priv->pd_usb_phy2);
+	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
+		power_domain_off(&priv->pd_usb_phy2);
 
 	power_domain_off(&priv->pd_bus);
 
@@ -109,6 +139,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 +159,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: