diff mbox series

[v2,1/7] reset: imx7: Add the iMX8MP PCIe PHY PERST support

Message ID 1646644054-24421-2-git-send-email-hongxing.zhu@nxp.com (mailing list archive)
State New, archived
Headers show
Series Add the iMX8MP PCIe support | expand

Commit Message

Hongxing Zhu March 7, 2022, 9:07 a.m. UTC
Add the i.MX8MP PCIe PHY PERST support.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/reset/reset-imx7.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Philipp Zabel April 4, 2022, 9:34 a.m. UTC | #1
Hi Richard,

On Mo, 2022-03-07 at 17:07 +0800, Richard Zhu wrote:
> Add the i.MX8MP PCIe PHY PERST support.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/reset/reset-imx7.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index 185a333df66c..d2408725eb2c 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -329,6 +329,7 @@ static int imx8mp_reset_set(struct
> reset_controller_dev *rcdev,
>                 break;
>  
>         case IMX8MP_RESET_PCIE_CTRL_APPS_EN:
> +       case IMX8MP_RESET_PCIEPHY_PERST:
>                 value = assert ? 0 : bit;
>                 break;
>         }

This doesn't do what the commit description says.

The PCIEPHY_PERST bit is already supported by the driver (albeit
incorrectly?) - this patch just inverts the bit.

Since this bit is not inverted on the other platforms, and the i.MX8MP
reference manual says nothing about this, please explicitly state why
this needs to be inverted and call it a fix in the commit description.

regards
Philipp
Lucas Stach April 14, 2022, 8:48 p.m. UTC | #2
Am Montag, dem 07.03.2022 um 17:07 +0800 schrieb Richard Zhu:
> Add the i.MX8MP PCIe PHY PERST support.

As Philipp said: please add some more description on why this is
necessary. As far as I can see the reset is already present on 8MQ, and
is low-active, like this patch claims. We just didn't handle this reset
at all on other SoCs as the power on de-asserted state was okay to get
things working.

Regards,
Lucas

> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/reset/reset-imx7.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index 185a333df66c..d2408725eb2c 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -329,6 +329,7 @@ static int imx8mp_reset_set(struct reset_controller_dev *rcdev,
>  		break;
>  
>  	case IMX8MP_RESET_PCIE_CTRL_APPS_EN:
> +	case IMX8MP_RESET_PCIEPHY_PERST:
>  		value = assert ? 0 : bit;
>  		break;
>  	}
Hongxing Zhu April 15, 2022, 7:32 a.m. UTC | #3
Hi Philipp:

> -----Original Message-----
> From: Philipp Zabel <p.zabel@pengutronix.de>
> Sent: 2022年4月4日 17:34
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; l.stach@pengutronix.de;
> bhelgaas@google.com; lorenzo.pieralisi@arm.com; robh@kernel.org;
> shawnguo@kernel.org; vkoul@kernel.org; alexander.stein@ew.tq-group.com
> Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v2 1/7] reset: imx7: Add the iMX8MP PCIe PHY PERST
> support
> 
> Hi Richard,
> 
> On Mo, 2022-03-07 at 17:07 +0800, Richard Zhu wrote:
> > Add the i.MX8MP PCIe PHY PERST support.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/reset/reset-imx7.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> > index 185a333df66c..d2408725eb2c 100644
> > --- a/drivers/reset/reset-imx7.c
> > +++ b/drivers/reset/reset-imx7.c
> > @@ -329,6 +329,7 @@ static int imx8mp_reset_set(struct
> > reset_controller_dev *rcdev,
> >                 break;
> >
> >         case IMX8MP_RESET_PCIE_CTRL_APPS_EN:
> > +       case IMX8MP_RESET_PCIEPHY_PERST:
> >                 value = assert ? 0 : bit;
> >                 break;
> >         }
> 
> This doesn't do what the commit description says.
> 
> The PCIEPHY_PERST bit is already supported by the driver (albeit
> incorrectly?) - this patch just inverts the bit.
> 
> Since this bit is not inverted on the other platforms, and the i.MX8MP
> reference manual says nothing about this, please explicitly state why this
> needs to be inverted and call it a fix in the commit description.
Thanks for your comments, and sorry for replying late.
I didn't get more details about this bit difference between i.MX8MP and others.
Let me consult with design team again, and back to you later.

Best Regards
Richard Zhu

> 
> regards
> Philipp
Hongxing Zhu April 18, 2022, 4:54 a.m. UTC | #4
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年4月15日 4:48
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; p.zabel@pengutronix.de;
> bhelgaas@google.com; lorenzo.pieralisi@arm.com; robh@kernel.org;
> shawnguo@kernel.org; vkoul@kernel.org; alexander.stein@ew.tq-group.com
> Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v2 1/7] reset: imx7: Add the iMX8MP PCIe PHY PERST
> support
> 
> Am Montag, dem 07.03.2022 um 17:07 +0800 schrieb Richard Zhu:
> > Add the i.MX8MP PCIe PHY PERST support.
> 
> As Philipp said: please add some more description on why this is necessary. As
> far as I can see the reset is already present on 8MQ, and is low-active, like this
> patch claims. We just didn't handle this reset at all on other SoCs as the power
> on de-asserted state was okay to get things working.
>
Yes, it is.
I had asking the details from design team.
Would update the description after I get the more information.

Best Regards
Richard Zhu

> Regards,
> Lucas
> 
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/reset/reset-imx7.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> > index 185a333df66c..d2408725eb2c 100644
> > --- a/drivers/reset/reset-imx7.c
> > +++ b/drivers/reset/reset-imx7.c
> > @@ -329,6 +329,7 @@ static int imx8mp_reset_set(struct
> reset_controller_dev *rcdev,
> >  		break;
> >
> >  	case IMX8MP_RESET_PCIE_CTRL_APPS_EN:
> > +	case IMX8MP_RESET_PCIEPHY_PERST:
> >  		value = assert ? 0 : bit;
> >  		break;
> >  	}
>
Hongxing Zhu April 26, 2022, 3:27 a.m. UTC | #5
Hi Philipp:

> -----Original Message-----
> From: Hongxing Zhu
> Sent: 2022年4月15日 15:33
> To: Philipp Zabel <p.zabel@pengutronix.de>; l.stach@pengutronix.de;
> bhelgaas@google.com; lorenzo.pieralisi@arm.com; robh@kernel.org;
> shawnguo@kernel.org; vkoul@kernel.org; alexander.stein@ew.tq-group.com
> Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: RE: [PATCH v2 1/7] reset: imx7: Add the iMX8MP PCIe PHY PERST
> support
> 
> Hi Philipp:
> 
> > -----Original Message-----
> > From: Philipp Zabel <p.zabel@pengutronix.de>
> > Sent: 2022年4月4日 17:34
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>; l.stach@pengutronix.de;
> > bhelgaas@google.com; lorenzo.pieralisi@arm.com; robh@kernel.org;
> > shawnguo@kernel.org; vkoul@kernel.org; alexander.stein@ew.tq-group.com
> > Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v2 1/7] reset: imx7: Add the iMX8MP PCIe PHY PERST
> > support
> >
> > Hi Richard,
> >
> > On Mo, 2022-03-07 at 17:07 +0800, Richard Zhu wrote:
> > > Add the i.MX8MP PCIe PHY PERST support.
> > >
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > ---
> > >  drivers/reset/reset-imx7.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> > > index 185a333df66c..d2408725eb2c 100644
> > > --- a/drivers/reset/reset-imx7.c
> > > +++ b/drivers/reset/reset-imx7.c
> > > @@ -329,6 +329,7 @@ static int imx8mp_reset_set(struct
> > > reset_controller_dev *rcdev,
> > >                 break;
> > >
> > >         case IMX8MP_RESET_PCIE_CTRL_APPS_EN:
> > > +       case IMX8MP_RESET_PCIEPHY_PERST:
> > >                 value = assert ? 0 : bit;
> > >                 break;
> > >         }
> >
> > This doesn't do what the commit description says.
> >
> > The PCIEPHY_PERST bit is already supported by the driver (albeit
> > incorrectly?) - this patch just inverts the bit.
> >
> > Since this bit is not inverted on the other platforms, and the i.MX8MP
> > reference manual says nothing about this, please explicitly state why
> > this needs to be inverted and call it a fix in the commit description.
> Thanks for your comments, and sorry for replying late.
> I didn't get more details about this bit difference between i.MX8MP and
> others.
> Let me consult with design team again, and back to you later.

I got some details of this PERST bit(BIT3) of SRC_PCIEPHY_RCR.
The initialized default value of this bit is 1b'1 on i.MX7/iMX8MM/iMX8MQ.
But unfortunately, the i.MX8MP has one inverted default value 1b'0 of this bit.

And this bit should be kept 1b'1 after power and clocks are stable.
So, I assert/de-assert this bit on i.MX8MP only.

Best Regards
Richard Zhu

> 
> Best Regards
> Richard Zhu
> 
> >
> > regards
> > Philipp
diff mbox series

Patch

diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
index 185a333df66c..d2408725eb2c 100644
--- a/drivers/reset/reset-imx7.c
+++ b/drivers/reset/reset-imx7.c
@@ -329,6 +329,7 @@  static int imx8mp_reset_set(struct reset_controller_dev *rcdev,
 		break;
 
 	case IMX8MP_RESET_PCIE_CTRL_APPS_EN:
+	case IMX8MP_RESET_PCIEPHY_PERST:
 		value = assert ? 0 : bit;
 		break;
 	}