subject:"Re\: \[PATCH 3\/7\] imx8mp\: power\-domain\: Add PCIe support"

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

2024-02-21 Thread Marek Vasut


On 2/21/24 07:01, Sumit Garg wrote:

On Tue, 20 Feb 2024 at 21:02, Marek Vasut  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 
---
   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_REG00x0
   #define  PCIE_CLOCK_MODULE_EN   BIT(0)
   #define  USB_CLOCK_MODULE_ENBIT(1)
+#define  PCIE_PHY_APB_RSTBIT(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 = >pd_usb_phy1;
   } else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) {
   domain = >pd_usb_phy2;
+ } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
+ domain = >pd_pcie;
+ } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
+ domain = >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(>clk_usb);
   if (ret)
- goto err_clk;
+ goto err_clk_usb;
+
+ ret = clk_enable(>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.

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

2024-02-20 Thread Sumit Garg

On Tue, 20 Feb 2024 at 21:02, Marek Vasut  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 
> > ---
> >   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_REG00x0
> >   #define  PCIE_CLOCK_MODULE_EN   BIT(0)
> >   #define  USB_CLOCK_MODULE_ENBIT(1)
> > +#define  PCIE_PHY_APB_RSTBIT(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 = >pd_usb_phy1;
> >   } else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) {
> >   domain = >pd_usb_phy2;
> > + } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
> > + domain = >pd_pcie;
> > + } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
> > + domain = >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(>clk_usb);
> >   if (ret)
> > - goto err_clk;
> > + goto err_clk_usb;
> > +
> > + ret = clk_enable(>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

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

2024-02-20 Thread Marek Vasut


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 
---
  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 = >pd_usb_phy1;
} else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) {
domain = >pd_usb_phy2;
+   } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
+   domain = >pd_pcie;
+   } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
+   domain = >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(>clk_usb);

if (ret)
-   goto err_clk;
+   goto err_clk_usb;
+
+   ret = clk_enable(>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 ?

[...]

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information