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
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 <[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..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
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]>
---
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
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 | 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
We already define register offsets so use them in register layout.
Signed-off-by: Vinod Koul <[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 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
On Thu, Dec 19, 2019 at 8:04 AM Vinod Koul <[email protected]> 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 <[email protected]>
> Signed-off-by: Vinod Koul <[email protected]>
Reviewed-by: Jeffrey Hugo <[email protected]>
Tested-by: Jeffrey Hugo <[email protected]>
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
>
On Thu, Dec 19, 2019 at 8:05 AM Vinod Koul <[email protected]> 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 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
>
On Thu, Dec 19, 2019 at 8:05 AM Vinod Koul <[email protected]> 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]>
Reviewed-by: Jeffrey Hugo <[email protected]>
> ---
> 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
>
On Thu, Dec 19, 2019 at 8:05 AM Vinod Koul <[email protected]> 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 <[email protected]>
Reviewed-by: Jeffrey Hugo <[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 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
>
On 19-12-19, 08:29, Jeffrey Hugo wrote:
> On Thu, Dec 19, 2019 at 8:04 AM Vinod Koul <[email protected]> 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 <[email protected]>
> > Signed-off-by: Vinod Koul <[email protected]>
>
> Reviewed-by: Jeffrey Hugo <[email protected]>
> Tested-by: Jeffrey Hugo <[email protected]>
>
> 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
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 <[email protected]>
> ---
> 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);
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 <[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 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.
On 2019-12-20 08:22, [email protected] 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 <[email protected]>
>> ---
>> 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.
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 <[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 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),
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 <[email protected]>
> Signed-off-by: Vinod Koul <[email protected]>
This does look familiar...
https://lore.kernel.org/linux-arm-msm/[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..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
>
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 <[email protected]>
> > Signed-off-by: Vinod Koul <[email protected]>
>
> This does look familiar...
>
> https://lore.kernel.org/linux-arm-msm/[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..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
On 20-12-19, 08:44, [email protected] 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 <[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 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
On 20-12-19, 08:49, [email protected] wrote:
> On 2019-12-20 08:22, [email protected] 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 <[email protected]>
> > > ---
> > > 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
On 2019-12-20 12:24, Vinod Koul wrote:
> On 20-12-19, 08:49, [email protected] wrote:
>> On 2019-12-20 08:22, [email protected] 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 <[email protected]>
>> > > ---
>> > > 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.
On 20-12-19, 14:00, Can Guo wrote:
> On 2019-12-20 12:24, Vinod Koul wrote:
> > On 20-12-19, 08:49, [email protected] wrote:
> > > On 2019-12-20 08:22, [email protected] 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
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, [email protected] wrote:
>> > > On 2019-12-20 08:22, [email protected] 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.
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
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
On 12/20/2019 6:19 AM, [email protected] wrote:
> On 2019-12-20 08:22, [email protected] 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 <[email protected]>
>>> ---
>>> 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