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

[PATCH v2 0/5] 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.

Changes in v2:
 - Drop patch 1 and pick the one Bjorn had already sent, makes timeout 10ms
 - Fix optional reset write as pointed by Can
 - Fix register define as pointed by Can

Bjorn Andersson (1):

  phy: qcom-qmp: Increase PHY ready timeout

Vinod Koul (4):
  phy: qcom-qmp: Use register defines
  phy: qcom-qmp: Add optional SW reset
  phy: qcom-qmp: remove duplicate powerdown write
  phy: qcom-qmp: remove no_pcs_sw_reset for sm8150

 drivers/phy/qualcomm/phy-qcom-qmp.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

-- 
2.23.0

[PATCH v2 1/5] phy: qcom-qmp: Increase PHY ready timeout

[Vinod Koul]

From: Bjorn Andersson <bjorn.andersson@linaro.org>

It's typical for the QHP PHY to take slightly above 1ms to initialize,
so increase the timeout of the PHY ready check to 10ms - as already done
in the downstream PCIe driver.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Tested-by: Evan Green <evgreen@chromium.org>
Tested-by: Vinod Koul <vkoul@kernel.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..66f91726b8b2 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		10000
 #define POWER_DOWN_DELAY_US_MIN			10
 #define POWER_DOWN_DELAY_US_MAX			11
 
-- 
2.23.0

[PATCH v2 2/5] 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>
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 66f91726b8b2..1196c85aa023 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_PCS_READY_STATUS]		= QPHY_V4_PCS_READY_STATUS,
 };
 
 static const struct qmp_phy_init_tbl msm8996_pcie_serdes_tbl[] = {
-- 
2.23.0

[PATCH v2 3/5] 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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 1196c85aa023..47a66d55107d 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -168,6 +168,7 @@ static const unsigned int sdm845_ufsphy_regs_layout[] = {
 static const unsigned int sm8150_ufsphy_regs_layout[] = {
 	[QPHY_START_CTRL]		= QPHY_V4_PHY_START,
 	[QPHY_PCS_READY_STATUS]		= QPHY_V4_PCS_READY_STATUS,
+	[QPHY_SW_RESET]			= QPHY_V4_SW_RESET,
 };
 
 static const struct qmp_phy_init_tbl msm8996_pcie_serdes_tbl[] = {
@@ -1023,6 +1024,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 +1395,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 +1480,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(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);
@@ -1651,6 +1659,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 v2 4/5] 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>
Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
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 47a66d55107d..2a12a0b3bd72 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -886,7 +886,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

[PATCH v2 5/5] phy: qcom-qmp: remove no_pcs_sw_reset for sm8150

[Vinod Koul]

SM8150 QMPY phy for UFS and onwards the PHY_SW_RESET is present in PHY's
PCS register so we should not mark no_pcs_sw_reset for sm8150 and
onwards

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 2a12a0b3bd72..2696640ee3f9 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -1393,7 +1393,6 @@ static const struct qmp_phy_cfg sm8150_ufsphy_cfg = {
 	.pwrdn_ctrl		= SW_PWRDN,
 
 	.is_dual_lane_phy	= true,
-	.no_pcs_sw_reset	= true,
 	.has_sw_reset		= true,
 };
 
-- 
2.23.0

Re: [PATCH v2 0/5] phy: qcom-qmp: Fixes and updates for sm8150

[Kishon Vijay Abraham I]

On 20/12/19 3:47 pm, Vinod Koul wrote:
> 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

patch 1 is applied to fixes, Thanks!

-Kishon
> 
> 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.
> 
> Changes in v2:
>  - Drop patch 1 and pick the one Bjorn had already sent, makes timeout 10ms
>  - Fix optional reset write as pointed by Can
>  - Fix register define as pointed by Can
> 
> Bjorn Andersson (1):
> 
>   phy: qcom-qmp: Increase PHY ready timeout
> 
> Vinod Koul (4):
>   phy: qcom-qmp: Use register defines
>   phy: qcom-qmp: Add optional SW reset
>   phy: qcom-qmp: remove duplicate powerdown write
>   phy: qcom-qmp: remove no_pcs_sw_reset for sm8150
> 
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 

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

[Can Guo]

On 2019-12-20 18:17, Vinod Koul 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 66f91726b8b2..1196c85aa023 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_PCS_READY_STATUS]		= QPHY_V4_PCS_READY_STATUS,

Missed QPHY_SW_RESET?

Regards,
Can Guo.

>  };
> 
>  static const struct qmp_phy_init_tbl msm8996_pcie_serdes_tbl[] = {

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

[Can Guo]

On 2019-12-23 16:43, Can Guo wrote:
> On 2019-12-20 18:17, Vinod Koul 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 66f91726b8b2..1196c85aa023 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_PCS_READY_STATUS]		= QPHY_V4_PCS_READY_STATUS,
> 
> Missed QPHY_SW_RESET?
> 
> Regards,
> Can Guo.
> 

My bad, just saw you added it in another patch.

Can Guo.

>>  };
>> 
>>  static const struct qmp_phy_init_tbl msm8996_pcie_serdes_tbl[] = {

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

[Manu Gautam]

On 12/20/2019 3:47 PM, 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 | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 1196c85aa023..47a66d55107d 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -168,6 +168,7 @@ static const unsigned int sdm845_ufsphy_regs_layout[] = {
>  static const unsigned int sm8150_ufsphy_regs_layout[] = {
>  	[QPHY_START_CTRL]		= QPHY_V4_PHY_START,
>  	[QPHY_PCS_READY_STATUS]		= QPHY_V4_PCS_READY_STATUS,
> +	[QPHY_SW_RESET]			= QPHY_V4_SW_RESET,
>  };
>  
>  static const struct qmp_phy_init_tbl msm8996_pcie_serdes_tbl[] = {
> @@ -1023,6 +1024,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;


There is no need to add new flag. Existing code will take care of it for UFS once you
clear no_pcs_sw_reset flag.

>  };
>  
>  /**
> @@ -1391,6 +1395,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 +1480,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(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
> +

Not needed. POR value of the bit is '1'.


>  	if (cfg->has_phy_com_ctrl)
>  		qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
>  			     SW_PWRDN);
> @@ -1651,6 +1659,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);
> +

There is no need to add UFS specific change here as existing PHY driver can
handle PCS based PHY sw_reset and already does it for USB and PCIe.


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

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

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

[Vinod Koul]

Hi Manu,

On 23-12-19, 14:38, Manu Gautam wrote:
> 
> On 12/20/2019 3:47 PM, 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 | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index 1196c85aa023..47a66d55107d 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -168,6 +168,7 @@ static const unsigned int sdm845_ufsphy_regs_layout[] = {
> >  static const unsigned int sm8150_ufsphy_regs_layout[] = {
> >  	[QPHY_START_CTRL]		= QPHY_V4_PHY_START,
> >  	[QPHY_PCS_READY_STATUS]		= QPHY_V4_PCS_READY_STATUS,
> > +	[QPHY_SW_RESET]			= QPHY_V4_SW_RESET,
> >  };
> >  
> >  static const struct qmp_phy_init_tbl msm8996_pcie_serdes_tbl[] = {
> > @@ -1023,6 +1024,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;
> 
> 
> There is no need to add new flag. Existing code will take care of it for UFS once you
> clear no_pcs_sw_reset flag.
> 
> >  };
> >  
> >  /**
> > @@ -1391,6 +1395,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 +1480,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(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
> > +
> 
> Not needed. POR value of the bit is '1'.
> 
> 
> >  	if (cfg->has_phy_com_ctrl)
> >  		qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
> >  			     SW_PWRDN);
> > @@ -1651,6 +1659,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);
> > +
> 
> There is no need to add UFS specific change here as existing PHY driver can
> handle PCS based PHY sw_reset and already does it for USB and PCIe.

Thanks for the explanation in this and previous version.

I confirm that adding sw_reset and clearing .no_pcs_sw_reset does make
it work for me on UFS on SM8150.

I will drop this patch and send the update in v3

-- 
~Vinod