[PATCH 0/4] phy: qcom-qmp: Fixes and updates for sm8150

[PATCH 0/4] phy: qcom-qmp: Fixes and updates for sm8150

[Vinod Koul]

On 5.5-rc1 we have seen regression on UFS phy on 8998 and SM8150. As
suggested by Can increasing the timeout helps so we increase the phy init
timeout and that fixes the issue for 8998.  The patch 1 should be applied
to fixes for 5.5

For SM8150 we need additional SW reset so add additional SW reset and
configure if flag is defined, while at it do small updates to Use register
defines and remove duplicate powerdown write.

Vinod Koul (4):
  phy: qcom-qmp: Increase the phy init timeout
  phy: qcom-qmp: Use register defines
  phy: qcom-qmp: Add optional SW reset
  phy: qcom-qmp: remove duplicate powerdown write

 drivers/phy/qualcomm/phy-qcom-qmp.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

-- 
2.23.0

[PATCH 1/4] phy: qcom-qmp: Increase the phy init timeout

[Vinod Koul]

If we do full reset of the phy, it seems to take a couple of ms to come
up on my system so increase the timeout to 10ms.

This was found by full reset addition by commit 870b1279c7a0
("scsi: ufs-qcom: Add reset control support for host controller") and
fixes the regression to platforms by this commit.

Suggested-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 091e20303a14..c2e800a3825a 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -66,7 +66,7 @@
 /* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */
 #define CLAMP_EN				BIT(0) /* enables i/o clamp_n */
 
-#define PHY_INIT_COMPLETE_TIMEOUT		1000
+#define PHY_INIT_COMPLETE_TIMEOUT		100000
 #define POWER_DOWN_DELAY_US_MIN			10
 #define POWER_DOWN_DELAY_US_MAX			11
 
-- 
2.23.0

[PATCH 2/4] phy: qcom-qmp: Use register defines

[Vinod Koul]

We already define register offsets so use them in register layout.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index c2e800a3825a..06f971ca518e 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -166,8 +166,8 @@ static const unsigned int sdm845_ufsphy_regs_layout[] = {
 };
 
 static const unsigned int sm8150_ufsphy_regs_layout[] = {
-	[QPHY_START_CTRL]		= 0x00,
-	[QPHY_PCS_READY_STATUS]		= 0x180,
+	[QPHY_START_CTRL]		= QPHY_V4_PHY_START,
+	[QPHY_SW_RESET]			= QPHY_V4_SW_RESET,
 };
 
 static const struct qmp_phy_init_tbl msm8996_pcie_serdes_tbl[] = {
-- 
2.23.0

[PATCH 3/4] phy: qcom-qmp: Add optional SW reset

[Vinod Koul]

For V4 QMP UFS Phy, we need to assert reset bits, configure the phy and
then deassert it, so add optional has_sw_reset flag and use that to
configure the QPHY_SW_RESET register.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 06f971ca518e..80304b7cd895 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -1023,6 +1023,9 @@ struct qmp_phy_cfg {
 
 	/* true, if PCS block has no separate SW_RESET register */
 	bool no_pcs_sw_reset;
+
+	/* true if sw reset needs to be invoked */
+	bool has_sw_reset;
 };
 
 /**
@@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = {
 
 	.is_dual_lane_phy	= true,
 	.no_pcs_sw_reset	= true,
+	.has_sw_reset		= true,
 };
 
 static void qcom_qmp_phy_configure(void __iomem *base,
@@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
 			     SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
 	}
 
+	if (cfg->has_sw_reset)
+		qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET);
+
 	if (cfg->has_phy_com_ctrl)
 		qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
 			     SW_PWRDN);
@@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy)
 	if (cfg->has_phy_dp_com_ctrl)
 		qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
 
+	if (cfg->has_sw_reset)
+		qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
+
 	/* start SerDes and Phy-Coding-Sublayer */
 	qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
 
-- 
2.23.0

[PATCH 4/4] phy: qcom-qmp: remove duplicate powerdown write

[Vinod Koul]

We already write to QPHY_POWER_DOWN_CONTROL in qcom_qmp_phy_com_init()
before invoking qcom_qmp_phy_configure() so remove the duplicate write.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 80304b7cd895..309ef15e46b0 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -885,7 +885,6 @@ static const struct qmp_phy_init_tbl msm8998_usb3_pcs_tbl[] = {
 };
 
 static const struct qmp_phy_init_tbl sm8150_ufsphy_serdes_tbl[] = {
-	QMP_PHY_INIT_CFG(QPHY_POWER_DOWN_CONTROL, 0x01),
 	QMP_PHY_INIT_CFG(QSERDES_V4_COM_SYSCLK_EN_SEL, 0xd9),
 	QMP_PHY_INIT_CFG(QSERDES_V4_COM_HSCLK_SEL, 0x11),
 	QMP_PHY_INIT_CFG(QSERDES_V4_COM_HSCLK_HS_SWITCH_SEL, 0x00),
-- 
2.23.0

Re: [PATCH 1/4] phy: qcom-qmp: Increase the phy init timeout

[Jeffrey Hugo]

On Thu, Dec 19, 2019 at 8:04 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> If we do full reset of the phy, it seems to take a couple of ms to come
> up on my system so increase the timeout to 10ms.
>
> This was found by full reset addition by commit 870b1279c7a0
> ("scsi: ufs-qcom: Add reset control support for host controller") and
> fixes the regression to platforms by this commit.
>
> Suggested-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>

Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
Tested-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

Tested on the Lenovo Miix 630 laptop (a msm8998 based system).  This
addresses the regression.

> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 091e20303a14..c2e800a3825a 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -66,7 +66,7 @@
>  /* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */
>  #define CLAMP_EN                               BIT(0) /* enables i/o clamp_n */
>
> -#define PHY_INIT_COMPLETE_TIMEOUT              1000
> +#define PHY_INIT_COMPLETE_TIMEOUT              100000
>  #define POWER_DOWN_DELAY_US_MIN                        10
>  #define POWER_DOWN_DELAY_US_MAX                        11
>
> --
> 2.23.0
>

Re: [PATCH 2/4] phy: qcom-qmp: Use register defines

[Jeffrey Hugo]

On Thu, Dec 19, 2019 at 8:05 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> We already define register offsets so use them in register layout.
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>

Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index c2e800a3825a..06f971ca518e 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -166,8 +166,8 @@ static const unsigned int sdm845_ufsphy_regs_layout[] = {
>  };
>
>  static const unsigned int sm8150_ufsphy_regs_layout[] = {
> -       [QPHY_START_CTRL]               = 0x00,
> -       [QPHY_PCS_READY_STATUS]         = 0x180,
> +       [QPHY_START_CTRL]               = QPHY_V4_PHY_START,
> +       [QPHY_SW_RESET]                 = QPHY_V4_SW_RESET,
>  };
>
>  static const struct qmp_phy_init_tbl msm8996_pcie_serdes_tbl[] = {
> --
> 2.23.0
>

Re: [PATCH 3/4] phy: qcom-qmp: Add optional SW reset

[Jeffrey Hugo]

On Thu, Dec 19, 2019 at 8:05 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> For V4 QMP UFS Phy, we need to assert reset bits, configure the phy and
> then deassert it, so add optional has_sw_reset flag and use that to
> configure the QPHY_SW_RESET register.
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>

Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 06f971ca518e..80304b7cd895 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -1023,6 +1023,9 @@ struct qmp_phy_cfg {
>
>         /* true, if PCS block has no separate SW_RESET register */
>         bool no_pcs_sw_reset;
> +
> +       /* true if sw reset needs to be invoked */
> +       bool has_sw_reset;
>  };
>
>  /**
> @@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = {
>
>         .is_dual_lane_phy       = true,
>         .no_pcs_sw_reset        = true,
> +       .has_sw_reset           = true,
>  };
>
>  static void qcom_qmp_phy_configure(void __iomem *base,
> @@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
>                              SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>         }
>
> +       if (cfg->has_sw_reset)
> +               qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET);
> +
>         if (cfg->has_phy_com_ctrl)
>                 qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
>                              SW_PWRDN);
> @@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy)
>         if (cfg->has_phy_dp_com_ctrl)
>                 qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
>
> +       if (cfg->has_sw_reset)
> +               qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
> +
>         /* start SerDes and Phy-Coding-Sublayer */
>         qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
>
> --
> 2.23.0
>

Re: [PATCH 4/4] phy: qcom-qmp: remove duplicate powerdown write

[Jeffrey Hugo]

On Thu, Dec 19, 2019 at 8:05 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> We already write to QPHY_POWER_DOWN_CONTROL in qcom_qmp_phy_com_init()
> before invoking qcom_qmp_phy_configure() so remove the duplicate write.
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>

Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 80304b7cd895..309ef15e46b0 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -885,7 +885,6 @@ static const struct qmp_phy_init_tbl msm8998_usb3_pcs_tbl[] = {
>  };
>
>  static const struct qmp_phy_init_tbl sm8150_ufsphy_serdes_tbl[] = {
> -       QMP_PHY_INIT_CFG(QPHY_POWER_DOWN_CONTROL, 0x01),
>         QMP_PHY_INIT_CFG(QSERDES_V4_COM_SYSCLK_EN_SEL, 0xd9),
>         QMP_PHY_INIT_CFG(QSERDES_V4_COM_HSCLK_SEL, 0x11),
>         QMP_PHY_INIT_CFG(QSERDES_V4_COM_HSCLK_HS_SWITCH_SEL, 0x00),
> --
> 2.23.0
>

Re: [PATCH 1/4] phy: qcom-qmp: Increase the phy init timeout

[Vinod Koul]

On 19-12-19, 08:29, Jeffrey Hugo wrote:
> On Thu, Dec 19, 2019 at 8:04 AM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > If we do full reset of the phy, it seems to take a couple of ms to come
> > up on my system so increase the timeout to 10ms.
> >
> > This was found by full reset addition by commit 870b1279c7a0
> > ("scsi: ufs-qcom: Add reset control support for host controller") and
> > fixes the regression to platforms by this commit.
> >
> > Suggested-by: Can Guo <cang@codeaurora.org>
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> 
> Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
> Tested-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
> 
> Tested on the Lenovo Miix 630 laptop (a msm8998 based system).  This
> addresses the regression.

Thanks Jeff for quick test and reviews! Appreciate it.

> > ---
> >  drivers/phy/qualcomm/phy-qcom-qmp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index 091e20303a14..c2e800a3825a 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -66,7 +66,7 @@
> >  /* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */
> >  #define CLAMP_EN                               BIT(0) /* enables i/o clamp_n */
> >
> > -#define PHY_INIT_COMPLETE_TIMEOUT              1000
> > +#define PHY_INIT_COMPLETE_TIMEOUT              100000
> >  #define POWER_DOWN_DELAY_US_MIN                        10
> >  #define POWER_DOWN_DELAY_US_MAX                        11
> >
> > --
> > 2.23.0
> >

-- 
~Vinod

Re: [PATCH 3/4] phy: qcom-qmp: Add optional SW reset

[cang]

On 2019-12-19 23:04, Vinod Koul wrote:
> For V4 QMP UFS Phy, we need to assert reset bits, configure the phy and
> then deassert it, so add optional has_sw_reset flag and use that to
> configure the QPHY_SW_RESET register.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
> b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 06f971ca518e..80304b7cd895 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -1023,6 +1023,9 @@ struct qmp_phy_cfg {
> 
>  	/* true, if PCS block has no separate SW_RESET register */
>  	bool no_pcs_sw_reset;
> +
> +	/* true if sw reset needs to be invoked */
> +	bool has_sw_reset;
>  };
> 
>  /**
> @@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg 
> = {
> 
>  	.is_dual_lane_phy	= true,
>  	.no_pcs_sw_reset	= true,
> +	.has_sw_reset		= true,
>  };
> 
>  static void qcom_qmp_phy_configure(void __iomem *base,
> @@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy 
> *qphy)
>  			     SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>  	}
> 
> +	if (cfg->has_sw_reset)
> +		qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET);
> +

Are you sure you want to set this in the serdes register? QPHY_SW_RESET
is in its pcs register.

>  	if (cfg->has_phy_com_ctrl)
>  		qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
>  			     SW_PWRDN);
> @@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy)
>  	if (cfg->has_phy_dp_com_ctrl)
>  		qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
> 
> +	if (cfg->has_sw_reset)
> +		qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
> +

Yet you are clearing it from pcs register.

Regards,
Can Guo

>  	/* start SerDes and Phy-Coding-Sublayer */
>  	qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);

Re: [PATCH 2/4] phy: qcom-qmp: Use register defines

[cang]

On 2019-12-19 23:04, Vinod Koul wrote:
> We already define register offsets so use them in register layout.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
> b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index c2e800a3825a..06f971ca518e 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -166,8 +166,8 @@ static const unsigned int 
> sdm845_ufsphy_regs_layout[] = {
>  };
> 
>  static const unsigned int sm8150_ufsphy_regs_layout[] = {
> -	[QPHY_START_CTRL]		= 0x00,
> -	[QPHY_PCS_READY_STATUS]		= 0x180,
> +	[QPHY_START_CTRL]		= QPHY_V4_PHY_START,
> +	[QPHY_SW_RESET]			= QPHY_V4_SW_RESET,
>  };
> 
>  static const struct qmp_phy_init_tbl msm8996_pcie_serdes_tbl[] = {

Why is the QPHY_PCS_READY_STATUS removed here? Then what "status" are we
polling here for UFS PHY?

<snip>
      if (cfg->type == PHY_TYPE_UFS) {
          status = pcs + cfg->regs[QPHY_PCS_READY_STATUS];
          mask = PCS_READY;
          ready = PCS_READY;
      } else {
<snip>

Can Guo.

Re: [PATCH 3/4] phy: qcom-qmp: Add optional SW reset

[cang]

On 2019-12-20 08:22, cang@codeaurora.org wrote:
> On 2019-12-19 23:04, Vinod Koul wrote:
>> For V4 QMP UFS Phy, we need to assert reset bits, configure the phy 
>> and
>> then deassert it, so add optional has_sw_reset flag and use that to
>> configure the QPHY_SW_RESET register.
>> 
>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>> ---
>>  drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>> 
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
>> b/drivers/phy/qualcomm/phy-qcom-qmp.c
>> index 06f971ca518e..80304b7cd895 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
>> @@ -1023,6 +1023,9 @@ struct qmp_phy_cfg {
>> 
>>  	/* true, if PCS block has no separate SW_RESET register */
>>  	bool no_pcs_sw_reset;
>> +
>> +	/* true if sw reset needs to be invoked */
>> +	bool has_sw_reset;
>>  };
>> 
>>  /**
>> @@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg 
>> sm8150_ufsphy_cfg = {
>> 
>>  	.is_dual_lane_phy	= true,
>>  	.no_pcs_sw_reset	= true,
>> +	.has_sw_reset		= true,
>>  };
>> 
>>  static void qcom_qmp_phy_configure(void __iomem *base,
>> @@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy 
>> *qphy)
>>  			     SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>>  	}
>> 
>> +	if (cfg->has_sw_reset)
>> +		qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET);
>> +
> 
> Are you sure you want to set this in the serdes register? QPHY_SW_RESET
> is in its pcs register.
> 
>>  	if (cfg->has_phy_com_ctrl)
>>  		qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
>>  			     SW_PWRDN);
>> @@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy)
>>  	if (cfg->has_phy_dp_com_ctrl)
>>  		qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
>> 
>> +	if (cfg->has_sw_reset)
>> +		qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
>> +
> 
> Yet you are clearing it from pcs register.
> 
> Regards,
> Can Guo
> 
>>  	/* start SerDes and Phy-Coding-Sublayer */
>>  	qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);

I thought your change would be like this

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c 
b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 8e642a6..a4ab4b7 100755
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -166,6 +166,7 @@ static const unsigned int 
sdm845_ufsphy_regs_layout[] = {
  };

  static const unsigned int sm8150_ufsphy_regs_layout[] = {
+       [QPHY_SW_RESET]                 = 0x08,
         [QPHY_START_CTRL]               = 0x00,
         [QPHY_PCS_READY_STATUS]         = 0x180,
  };
@@ -1390,7 +1391,6 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg 
= {
         .pwrdn_ctrl             = SW_PWRDN,

         .is_dual_lane_phy       = true,
-       .no_pcs_sw_reset        = true,
  };

  static void qcom_qmp_phy_configure(void __iomem *base,
@@ -1475,6 +1475,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy 
*qphy)
                              SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
         }

+       if ((cfg->type == PHY_TYPE_UFS) && (!cfg->no_pcs_sw_reset))
+               qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
+
         if (cfg->has_phy_com_ctrl)
                 qphy_setbits(serdes, 
cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
                              SW_PWRDN);

Regards,
Can Guo.

Re: [PATCH 4/4] phy: qcom-qmp: remove duplicate powerdown write

[Can Guo]

On 2019-12-19 23:04, Vinod Koul wrote:
> We already write to QPHY_POWER_DOWN_CONTROL in qcom_qmp_phy_com_init()
> before invoking qcom_qmp_phy_configure() so remove the duplicate write.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>

Reviewed-by: Can Guo <cang@codeaurora.org>

> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
> b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 80304b7cd895..309ef15e46b0 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -885,7 +885,6 @@ static const struct qmp_phy_init_tbl
> msm8998_usb3_pcs_tbl[] = {
>  };
> 
>  static const struct qmp_phy_init_tbl sm8150_ufsphy_serdes_tbl[] = {
> -	QMP_PHY_INIT_CFG(QPHY_POWER_DOWN_CONTROL, 0x01),
>  	QMP_PHY_INIT_CFG(QSERDES_V4_COM_SYSCLK_EN_SEL, 0xd9),
>  	QMP_PHY_INIT_CFG(QSERDES_V4_COM_HSCLK_SEL, 0x11),
>  	QMP_PHY_INIT_CFG(QSERDES_V4_COM_HSCLK_HS_SWITCH_SEL, 0x00),

Re: [PATCH 1/4] phy: qcom-qmp: Increase the phy init timeout

[Bjorn Andersson]

On Thu 19 Dec 07:04 PST 2019, Vinod Koul wrote:

> If we do full reset of the phy, it seems to take a couple of ms to come
> up on my system so increase the timeout to 10ms.
> 
> This was found by full reset addition by commit 870b1279c7a0
> ("scsi: ufs-qcom: Add reset control support for host controller") and
> fixes the regression to platforms by this commit.
> 
> Suggested-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>

This does look familiar...

https://lore.kernel.org/linux-arm-msm/20191107000917.1092409-3-bjorn.andersson@linaro.org/

> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 091e20303a14..c2e800a3825a 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -66,7 +66,7 @@
>  /* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */
>  #define CLAMP_EN				BIT(0) /* enables i/o clamp_n */
>  
> -#define PHY_INIT_COMPLETE_TIMEOUT		1000
> +#define PHY_INIT_COMPLETE_TIMEOUT		100000

100ms seems a little bit excessive, and we do end up waiting this long
when we have PCIe links without an attached device...

Do you need >10ms or could we just have my patch merged?

Regards,
Bjorn

>  #define POWER_DOWN_DELAY_US_MIN			10
>  #define POWER_DOWN_DELAY_US_MAX			11
>  
> -- 
> 2.23.0
> 

Re: [PATCH 1/4] phy: qcom-qmp: Increase the phy init timeout

[Vinod Koul]

On 19-12-19, 18:08, Bjorn Andersson wrote:
> On Thu 19 Dec 07:04 PST 2019, Vinod Koul wrote:
> 
> > If we do full reset of the phy, it seems to take a couple of ms to come
> > up on my system so increase the timeout to 10ms.
> > 
> > This was found by full reset addition by commit 870b1279c7a0
> > ("scsi: ufs-qcom: Add reset control support for host controller") and
> > fixes the regression to platforms by this commit.
> > 
> > Suggested-by: Can Guo <cang@codeaurora.org>
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> 
> This does look familiar...
> 
> https://lore.kernel.org/linux-arm-msm/20191107000917.1092409-3-bjorn.andersson@linaro.org/
> 
> > ---
> >  drivers/phy/qualcomm/phy-qcom-qmp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index 091e20303a14..c2e800a3825a 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -66,7 +66,7 @@
> >  /* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */
> >  #define CLAMP_EN				BIT(0) /* enables i/o clamp_n */
> >  
> > -#define PHY_INIT_COMPLETE_TIMEOUT		1000
> > +#define PHY_INIT_COMPLETE_TIMEOUT		100000
> 
> 100ms seems a little bit excessive, and we do end up waiting this long
> when we have PCIe links without an attached device...
> 
> Do you need >10ms or could we just have my patch merged?

Yeah I quick tested 10ms as well and seems good for me, so either ways
if fine, but lets get it applied quickly :-)

-- 
~Vinod

Re: [PATCH 2/4] phy: qcom-qmp: Use register defines

[Vinod Koul]

On 20-12-19, 08:44, cang@codeaurora.org wrote:
> On 2019-12-19 23:04, Vinod Koul wrote:
> > We already define register offsets so use them in register layout.
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-qmp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index c2e800a3825a..06f971ca518e 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -166,8 +166,8 @@ static const unsigned int
> > sdm845_ufsphy_regs_layout[] = {
> >  };
> > 
> >  static const unsigned int sm8150_ufsphy_regs_layout[] = {
> > -	[QPHY_START_CTRL]		= 0x00,
> > -	[QPHY_PCS_READY_STATUS]		= 0x180,
> > +	[QPHY_START_CTRL]		= QPHY_V4_PHY_START,
> > +	[QPHY_SW_RESET]			= QPHY_V4_SW_RESET,
> >  };
> > 
> >  static const struct qmp_phy_init_tbl msm8996_pcie_serdes_tbl[] = {
> 
> Why is the QPHY_PCS_READY_STATUS removed here? Then what "status" are we
> polling here for UFS PHY?
> 
> <snip>
>      if (cfg->type == PHY_TYPE_UFS) {
>          status = pcs + cfg->regs[QPHY_PCS_READY_STATUS];
>          mask = PCS_READY;
>          ready = PCS_READY;
>      } else {
> <snip>

Good catch Can, I dont think I intended it that way. Will fix it up!

Thanks
-- 
~Vinod

Re: [PATCH 3/4] phy: qcom-qmp: Add optional SW reset

[Vinod Koul]

On 20-12-19, 08:49, cang@codeaurora.org wrote:
> On 2019-12-20 08:22, cang@codeaurora.org wrote:
> > On 2019-12-19 23:04, Vinod Koul wrote:
> > > For V4 QMP UFS Phy, we need to assert reset bits, configure the phy
> > > and
> > > then deassert it, so add optional has_sw_reset flag and use that to
> > > configure the QPHY_SW_RESET register.
> > > 
> > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > > ---
> > >  drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > index 06f971ca518e..80304b7cd895 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > @@ -1023,6 +1023,9 @@ struct qmp_phy_cfg {
> > > 
> > >  	/* true, if PCS block has no separate SW_RESET register */
> > >  	bool no_pcs_sw_reset;
> > > +
> > > +	/* true if sw reset needs to be invoked */
> > > +	bool has_sw_reset;
> > >  };
> > > 
> > >  /**
> > > @@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg
> > > sm8150_ufsphy_cfg = {
> > > 
> > >  	.is_dual_lane_phy	= true,
> > >  	.no_pcs_sw_reset	= true,
> > > +	.has_sw_reset		= true,
> > >  };
> > > 
> > >  static void qcom_qmp_phy_configure(void __iomem *base,
> > > @@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct
> > > qmp_phy *qphy)
> > >  			     SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
> > >  	}
> > > 
> > > +	if (cfg->has_sw_reset)
> > > +		qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET);
> > > +
> > 
> > Are you sure you want to set this in the serdes register? QPHY_SW_RESET
> > is in its pcs register.
> > 
> > >  	if (cfg->has_phy_com_ctrl)
> > >  		qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
> > >  			     SW_PWRDN);
> > > @@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy)
> > >  	if (cfg->has_phy_dp_com_ctrl)
> > >  		qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
> > > 
> > > +	if (cfg->has_sw_reset)
> > > +		qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
> > > +
> > 
> > Yet you are clearing it from pcs register.

updated now, will send v2

> > 
> > Regards,
> > Can Guo
> > 
> > >  	/* start SerDes and Phy-Coding-Sublayer */
> > >  	qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
> 
> I thought your change would be like this
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
> b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 8e642a6..a4ab4b7 100755
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -166,6 +166,7 @@ static const unsigned int sdm845_ufsphy_regs_layout[] =
> {
>  };
> 
>  static const unsigned int sm8150_ufsphy_regs_layout[] = {
> +       [QPHY_SW_RESET]                 = 0x08,
>         [QPHY_START_CTRL]               = 0x00,
>         [QPHY_PCS_READY_STATUS]         = 0x180,
>  };
> @@ -1390,7 +1391,6 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = {
>         .pwrdn_ctrl             = SW_PWRDN,
> 
>         .is_dual_lane_phy       = true,
> -       .no_pcs_sw_reset        = true,
>  };
> 
>  static void qcom_qmp_phy_configure(void __iomem *base,
> @@ -1475,6 +1475,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
>                              SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>         }
> 
> +       if ((cfg->type == PHY_TYPE_UFS) && (!cfg->no_pcs_sw_reset))
> +               qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);

Well am not sure if no_pcs_sw_reset would do this and side effect on
other phys (somehow older ones dont seem to need this). That was the
reason for a new flag and to be used for specific instances

Thanks
-- 
~Vinod

Re: [PATCH 3/4] phy: qcom-qmp: Add optional SW reset

[Can Guo]

On 2019-12-20 12:24, Vinod Koul wrote:
> On 20-12-19, 08:49, cang@codeaurora.org wrote:
>> On 2019-12-20 08:22, cang@codeaurora.org wrote:
>> > On 2019-12-19 23:04, Vinod Koul wrote:
>> > > For V4 QMP UFS Phy, we need to assert reset bits, configure the phy
>> > > and
>> > > then deassert it, so add optional has_sw_reset flag and use that to
>> > > configure the QPHY_SW_RESET register.
>> > >
>> > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
>> > > ---
>> > >  drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++
>> > >  1 file changed, 10 insertions(+)
>> > >
>> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
>> > > b/drivers/phy/qualcomm/phy-qcom-qmp.c
>> > > index 06f971ca518e..80304b7cd895 100644
>> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
>> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
>> > > @@ -1023,6 +1023,9 @@ struct qmp_phy_cfg {
>> > >
>> > >  	/* true, if PCS block has no separate SW_RESET register */
>> > >  	bool no_pcs_sw_reset;
>> > > +
>> > > +	/* true if sw reset needs to be invoked */
>> > > +	bool has_sw_reset;
>> > >  };
>> > >
>> > >  /**
>> > > @@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg
>> > > sm8150_ufsphy_cfg = {
>> > >
>> > >  	.is_dual_lane_phy	= true,
>> > >  	.no_pcs_sw_reset	= true,
>> > > +	.has_sw_reset		= true,
>> > >  };
>> > >
>> > >  static void qcom_qmp_phy_configure(void __iomem *base,
>> > > @@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct
>> > > qmp_phy *qphy)
>> > >  			     SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>> > >  	}
>> > >
>> > > +	if (cfg->has_sw_reset)
>> > > +		qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET);
>> > > +
>> >
>> > Are you sure you want to set this in the serdes register? QPHY_SW_RESET
>> > is in its pcs register.
>> >
>> > >  	if (cfg->has_phy_com_ctrl)
>> > >  		qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
>> > >  			     SW_PWRDN);
>> > > @@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy)
>> > >  	if (cfg->has_phy_dp_com_ctrl)
>> > >  		qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
>> > >
>> > > +	if (cfg->has_sw_reset)
>> > > +		qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
>> > > +
>> >
>> > Yet you are clearing it from pcs register.
> 
> updated now, will send v2
> 
>> >
>> > Regards,
>> > Can Guo
>> >
>> > >  	/* start SerDes and Phy-Coding-Sublayer */
>> > >  	qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
>> 
>> I thought your change would be like this
>> 
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
>> b/drivers/phy/qualcomm/phy-qcom-qmp.c
>> index 8e642a6..a4ab4b7 100755
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
>> @@ -166,6 +166,7 @@ static const unsigned int 
>> sdm845_ufsphy_regs_layout[] =
>> {
>>  };
>> 
>>  static const unsigned int sm8150_ufsphy_regs_layout[] = {
>> +       [QPHY_SW_RESET]                 = 0x08,
>>         [QPHY_START_CTRL]               = 0x00,
>>         [QPHY_PCS_READY_STATUS]         = 0x180,
>>  };
>> @@ -1390,7 +1391,6 @@ static const struct qmp_phy_cfg 
>> sm8150_ufsphy_cfg = {
>>         .pwrdn_ctrl             = SW_PWRDN,
>> 
>>         .is_dual_lane_phy       = true,
>> -       .no_pcs_sw_reset        = true,
>>  };
>> 
>>  static void qcom_qmp_phy_configure(void __iomem *base,
>> @@ -1475,6 +1475,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy 
>> *qphy)
>>                              SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>>         }
>> 
>> +       if ((cfg->type == PHY_TYPE_UFS) && (!cfg->no_pcs_sw_reset))
>> +               qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
> 
> Well am not sure if no_pcs_sw_reset would do this and side effect on
> other phys (somehow older ones dont seem to need this). That was the
> reason for a new flag and to be used for specific instances
> 
> Thanks

Hi Vinod,

That is why I added the check as cfg->type == PHY_TYPE_UFS, meaning this
change will only apply to UFS.
FYI, start from 8150(include 8150), QPHY_SW_RESET is present in PHY's
PCS register. no_pcs_sw_reset = TRUE should only be given to 845 and 
older
targets, like 8998, because they don't have this QPHY_SW_RESET in PHY's
register per their design, that's why they leverage the reset control
provided by UFS controller.

Thanks,
Can Guo.

Re: [PATCH 3/4] phy: qcom-qmp: Add optional SW reset

[Vinod Koul]

On 20-12-19, 14:00, Can Guo wrote:
> On 2019-12-20 12:24, Vinod Koul wrote:
> > On 20-12-19, 08:49, cang@codeaurora.org wrote:
> > > On 2019-12-20 08:22, cang@codeaurora.org wrote:
> > > > On 2019-12-19 23:04, Vinod Koul wrote:
> > > >
> > > > >  	/* start SerDes and Phy-Coding-Sublayer */
> > > > >  	qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
> > > 
> > > I thought your change would be like this
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > index 8e642a6..a4ab4b7 100755
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > > @@ -166,6 +166,7 @@ static const unsigned int
> > > sdm845_ufsphy_regs_layout[] =
> > > {
> > >  };
> > > 
> > >  static const unsigned int sm8150_ufsphy_regs_layout[] = {
> > > +       [QPHY_SW_RESET]                 = 0x08,
> > >         [QPHY_START_CTRL]               = 0x00,
> > >         [QPHY_PCS_READY_STATUS]         = 0x180,
> > >  };
> > > @@ -1390,7 +1391,6 @@ static const struct qmp_phy_cfg
> > > sm8150_ufsphy_cfg = {
> > >         .pwrdn_ctrl             = SW_PWRDN,
> > > 
> > >         .is_dual_lane_phy       = true,
> > > -       .no_pcs_sw_reset        = true,
> > >  };
> > > 
> > >  static void qcom_qmp_phy_configure(void __iomem *base,
> > > @@ -1475,6 +1475,9 @@ static int qcom_qmp_phy_com_init(struct
> > > qmp_phy *qphy)
> > >                              SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
> > >         }
> > > 
> > > +       if ((cfg->type == PHY_TYPE_UFS) && (!cfg->no_pcs_sw_reset))
> > > +               qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
> > 
> > Well am not sure if no_pcs_sw_reset would do this and side effect on
> > other phys (somehow older ones dont seem to need this). That was the
> > reason for a new flag and to be used for specific instances
> > 
> > Thanks
> 
> Hi Vinod,
> 
> That is why I added the check as cfg->type == PHY_TYPE_UFS, meaning this
> change will only apply to UFS.
> FYI, start from 8150(include 8150), QPHY_SW_RESET is present in PHY's
> PCS register. no_pcs_sw_reset = TRUE should only be given to 845 and older
> targets, like 8998, because they don't have this QPHY_SW_RESET in PHY's
> register per their design, that's why they leverage the reset control
> provided by UFS controller.

I have removed no_pcs_sw_reset and tested.

Well as you said even with UFS we have variations between various chips,
so I thought leaving it separate might be better than creating a chance
of regression on older platforms!

Moreover, are we sure that the reset wont be there for other qmp phy's
in future other than UFS...

Thanks
-- 
~Vinod

Re: [PATCH 3/4] phy: qcom-qmp: Add optional SW reset

[Can Guo]

On 2019-12-20 15:10, Vinod Koul wrote:
> On 20-12-19, 14:00, Can Guo wrote:
>> On 2019-12-20 12:24, Vinod Koul wrote:
>> > On 20-12-19, 08:49, cang@codeaurora.org wrote:
>> > > On 2019-12-20 08:22, cang@codeaurora.org wrote:
>> > > > On 2019-12-19 23:04, Vinod Koul wrote:
>> > > >
>> > > > >  	/* start SerDes and Phy-Coding-Sublayer */
>> > > > >  	qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
>> > >
>> > > I thought your change would be like this
>> > >
>> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
>> > > b/drivers/phy/qualcomm/phy-qcom-qmp.c
>> > > index 8e642a6..a4ab4b7 100755
>> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
>> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
>> > > @@ -166,6 +166,7 @@ static const unsigned int
>> > > sdm845_ufsphy_regs_layout[] =
>> > > {
>> > >  };
>> > >
>> > >  static const unsigned int sm8150_ufsphy_regs_layout[] = {
>> > > +       [QPHY_SW_RESET]                 = 0x08,
>> > >         [QPHY_START_CTRL]               = 0x00,
>> > >         [QPHY_PCS_READY_STATUS]         = 0x180,
>> > >  };
>> > > @@ -1390,7 +1391,6 @@ static const struct qmp_phy_cfg
>> > > sm8150_ufsphy_cfg = {
>> > >         .pwrdn_ctrl             = SW_PWRDN,
>> > >
>> > >         .is_dual_lane_phy       = true,
>> > > -       .no_pcs_sw_reset        = true,
>> > >  };
>> > >
>> > >  static void qcom_qmp_phy_configure(void __iomem *base,
>> > > @@ -1475,6 +1475,9 @@ static int qcom_qmp_phy_com_init(struct
>> > > qmp_phy *qphy)
>> > >                              SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>> > >         }
>> > >
>> > > +       if ((cfg->type == PHY_TYPE_UFS) && (!cfg->no_pcs_sw_reset))
>> > > +               qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
>> >
>> > Well am not sure if no_pcs_sw_reset would do this and side effect on
>> > other phys (somehow older ones dont seem to need this). That was the
>> > reason for a new flag and to be used for specific instances
>> >
>> > Thanks
>> 
>> Hi Vinod,
>> 
>> That is why I added the check as cfg->type == PHY_TYPE_UFS, meaning 
>> this
>> change will only apply to UFS.
>> FYI, start from 8150(include 8150), QPHY_SW_RESET is present in PHY's
>> PCS register. no_pcs_sw_reset = TRUE should only be given to 845 and 
>> older
>> targets, like 8998, because they don't have this QPHY_SW_RESET in 
>> PHY's
>> register per their design, that's why they leverage the reset control
>> provided by UFS controller.
> 
> I have removed no_pcs_sw_reset and tested.
> 
> Well as you said even with UFS we have variations between various 
> chips,
> so I thought leaving it separate might be better than creating a chance
> of regression on older platforms!
> 
> Moreover, are we sure that the reset wont be there for other qmp phy's
> in future other than UFS...
> 
> Thanks

Hi Vinod

We are just removing the no_pcs_sw_reset for 8150, right? Why is it
possibly impacting 845 or older paltforms?

In future, we will no longer need no_pcs_sw_reset for any newer QCOM UFS
PHY designs, as it is only for 845 and older platforms.

I am sure QPHY_SW_RESET will be within PHY's address space since 8150.
Otherwise, it will be a regression in UFS PHY design. We had a lot of
discussion about this on 845 years ago, then design team decided to add
it on later platforms, so I don't see a reason to remove it again. :)

I am not sure about the other qmp phys, but so long as UFS PHY needs the
reset, we need to keep it, as phy-qcom-qmp.c is a common driver. I am
not sure if I get your point here. Please correct me I am wrong.

Thanks,

Can Guo.

Re: [PATCH 3/4] phy: qcom-qmp: Add optional SW reset

[Vinod Koul]

On 20-12-19, 15:41, Can Guo wrote:
> On 2019-12-20 15:10, Vinod Koul wrote:
> > On 20-12-19, 14:00, Can Guo wrote:
> Hi Vinod
> 
> We are just removing the no_pcs_sw_reset for 8150, right? Why is it
> possibly impacting 845 or older paltforms?
> 
> In future, we will no longer need no_pcs_sw_reset for any newer QCOM UFS
> PHY designs, as it is only for 845 and older platforms.
> 
> I am sure QPHY_SW_RESET will be within PHY's address space since 8150.
> Otherwise, it will be a regression in UFS PHY design. We had a lot of
> discussion about this on 845 years ago, then design team decided to add
> it on later platforms, so I don't see a reason to remove it again. :)
> 
> I am not sure about the other qmp phys, but so long as UFS PHY needs the
> reset, we need to keep it, as phy-qcom-qmp.c is a common driver. I am
> not sure if I get your point here. Please correct me I am wrong.

The argument here is that we are making this UFS specific and we do not
know if this will be true in future as QMP is a common phy, so adding a
separate flag helps to keep it independent and to be used in other
situations.

Thanks
-- 
~Vinod

Re: [PATCH 3/4] phy: qcom-qmp: Add optional SW reset

[Manu Gautam]

On 12/20/2019 1:23 PM, Vinod Koul wrote:
> On 20-12-19, 15:41, Can Guo wrote:
>> On 2019-12-20 15:10, Vinod Koul wrote:
>>> On 20-12-19, 14:00, Can Guo wrote:
>> Hi Vinod
>>
>> We are just removing the no_pcs_sw_reset for 8150, right? Why is it
>> possibly impacting 845 or older paltforms?
>>
>> In future, we will no longer need no_pcs_sw_reset for any newer QCOM UFS
>> PHY designs, as it is only for 845 and older platforms.
>>
>> I am sure QPHY_SW_RESET will be within PHY's address space since 8150.
>> Otherwise, it will be a regression in UFS PHY design. We had a lot of
>> discussion about this on 845 years ago, then design team decided to add
>> it on later platforms, so I don't see a reason to remove it again. :)
>>
>> I am not sure about the other qmp phys, but so long as UFS PHY needs the
>> reset, we need to keep it, as phy-qcom-qmp.c is a common driver. I am
>> not sure if I get your point here. Please correct me I am wrong.
> The argument here is that we are making this UFS specific and we do not
> know if this will be true in future as QMP is a common phy, so adding a
> separate flag helps to keep it independent and to be used in other
> situations.
>
> Thanks

We should just remove no_pcs_sw_reset and let existing code take care
of PHY reset for UFS.
QMP PHY reset for UFS was differently handled earlier compared to USB/PCie
and relied on core for PHY reset. That is not the case with addition of
PCS based sw_reset and this won't change in future. There is no need to
have UFS specific flag in this driver.


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/4] phy: qcom-qmp: Add optional SW reset

[Manu Gautam]

On 12/20/2019 6:19 AM, cang@codeaurora.org wrote:
> On 2019-12-20 08:22, cang@codeaurora.org wrote:
>> On 2019-12-19 23:04, Vinod Koul wrote:
>>> For V4 QMP UFS Phy, we need to assert reset bits, configure the phy and
>>> then deassert it, so add optional has_sw_reset flag and use that to
>>> configure the QPHY_SW_RESET register.
>>>
>>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>>> ---
>>>  drivers/phy/qualcomm/phy-qcom-qmp.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
>>> b/drivers/phy/qualcomm/phy-qcom-qmp.c
>>> index 06f971ca518e..80304b7cd895 100644
>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
>>> @@ -1023,6 +1023,9 @@ struct qmp_phy_cfg {
>>>
>>>      /* true, if PCS block has no separate SW_RESET register */
>>>      bool no_pcs_sw_reset;
>>> +
>>> +    /* true if sw reset needs to be invoked */
>>> +    bool has_sw_reset;
>>>  };
>>>
>>>  /**
>>> @@ -1391,6 +1394,7 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = {
>>>
>>>      .is_dual_lane_phy    = true,
>>>      .no_pcs_sw_reset    = true,
>>> +    .has_sw_reset        = true,
>>>  };
>>>
>>>  static void qcom_qmp_phy_configure(void __iomem *base,
>>> @@ -1475,6 +1479,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
>>>                   SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>>>      }
>>>
>>> +    if (cfg->has_sw_reset)
>>> +        qphy_setbits(serdes, cfg->regs[QPHY_SW_RESET], SW_RESET);
>>> +
>>
>> Are you sure you want to set this in the serdes register? QPHY_SW_RESET
>> is in its pcs register.
>>
>>>      if (cfg->has_phy_com_ctrl)
>>>          qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
>>>                   SW_PWRDN);
>>> @@ -1651,6 +1658,9 @@ static int qcom_qmp_phy_enable(struct phy *phy)
>>>      if (cfg->has_phy_dp_com_ctrl)
>>>          qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
>>>
>>> +    if (cfg->has_sw_reset)
>>> +        qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
>>> +
>>
>> Yet you are clearing it from pcs register.
>>
>> Regards,
>> Can Guo
>>
>>>      /* start SerDes and Phy-Coding-Sublayer */
>>>      qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
>
> I thought your change would be like this
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 8e642a6..a4ab4b7 100755
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -166,6 +166,7 @@ static const unsigned int sdm845_ufsphy_regs_layout[] = {
>  };
>
>  static const unsigned int sm8150_ufsphy_regs_layout[] = {
> +       [QPHY_SW_RESET]                 = 0x08,
>         [QPHY_START_CTRL]               = 0x00,
>         [QPHY_PCS_READY_STATUS]         = 0x180,
>  };
> @@ -1390,7 +1391,6 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = {
>         .pwrdn_ctrl             = SW_PWRDN,
>
>         .is_dual_lane_phy       = true,
> -       .no_pcs_sw_reset        = true,


This makes sense to me.

>  };
>
>  static void qcom_qmp_phy_configure(void __iomem *base,
> @@ -1475,6 +1475,9 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
>                              SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>         }
>
> +       if ((cfg->type == PHY_TYPE_UFS) && (!cfg->no_pcs_sw_reset))
> +               qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
> +


This change is not needed as POR value of SW_RESET bit is '1' which will be
set as part of GCC or clk_reset.
We just need to clear this bit which code already takes care of.


>         if (cfg->has_phy_com_ctrl)
>                 qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
>                              SW_PWRDN);
>
> Regards,
> Can Guo.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project