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
From: Bjorn Andersson <[email protected]>
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 <[email protected]>
Tested-by: Evan Green <[email protected]>
Tested-by: Vinod Koul <[email protected]>
Signed-off-by: Vinod Koul <[email protected]>
---
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
We already define register offsets so use them in register layout.
Signed-off-by: Vinod Koul <[email protected]>
Reviewed-by: Jeffrey Hugo <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Jeffrey Hugo <[email protected]>
Reviewed-by: Can Guo <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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(-)
>
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 <[email protected]>
> Reviewed-by: Jeffrey Hugo <[email protected]>
> ---
> 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[] = {
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 <[email protected]>
>> Reviewed-by: Jeffrey Hugo <[email protected]>
>> ---
>> 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[] = {
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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