Re: [PATCH v5 2/7] scsi: ufs-qcom: Add reset control support for host controller

From: cang
Date: Thu Dec 19 2019 - 02:12:35 EST

On 2019-12-18 12:12, Vinod Koul wrote:
On 18-12-19, 02:44, cang@xxxxxxxxxxxxxx wrote:

Hi Vinod and Jeffrey,

Let me summary here, now the 1000000us timeout works for both 845 and 8998.
However, 8150 still fails.

> > The bigger question is why is the reset causing the timeout to be
> > increased for sdm845 and not to work in case of sm8150! (Vinod)

I would not say this patch increases the timeout. With this patch,
the PCS polling timeout, per my profiling, the PCS ready usually needs
less than 5000us, which is the actual time needed for PCS bit to be ready.

The reason why 1000us worked for you is because, w/o the patch, UFS PHY
registers are retained from pre-kernel stage (bootloader i.e.), the PCS
ready
bit was set to 1 in pre-kernel stage, so when kernel driver reads it, it
returns
1, not even to be polled at all. It may seem "faster", but not the right
thing to do, because kernel stage may need different PHY settings than
pre-kernel stage, keeping the settings configured in pre-kernel stage is not
always right, so this patch is needed. And increasing 1000us to 1000000us
is the right thing to do, but not a hack.

As reg for the phy initialization timeout on 8150, I found there is
something
wrong with its settings in /drivers/phy/qualcomm/phy-qcom-qmp.c

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(QPHY_POWER_DOWN_CONTROL, 0x01)" should NOT appear in the
serdes
table! I haven't check who made this change, but please have a try after
remove
this line from sm8150_ufsphy_serdes_tbl.

That is me :) Looks like I made an error while porting from downstream. I
did a quick check to remove this and it doesn't work yet, let me recheck
the settings again ...

Thanks for your help!

Hi Vinod,

Indeed, you need to tweak your settings. I spent some time help you
figure this out. Try below change and please let me know if it can
resolve your problem.

I would not say this is a regression caused by my patch, it is just
my patch reveals something incorrect in the settings.

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 8e642a6..0cc9044 100755
--- 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 1000000
#define POWER_DOWN_DELAY_US_MIN 10
#define POWER_DOWN_DELAY_US_MAX 11

@@ -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,
};
@@ -885,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),
@@ -1390,7 +1390,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,
---

Aside of the phy settings, your DT needs some modifications too,
seems you copied most of them from sdm845.
https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/commit/?h=for-next&id=3834a2e92229ef26d30de28acb698b2b23d3e397

<--snip-->
+ ufs_mem_phy: phy@1d87000 {
+ compatible = "qcom,sm8150-qmp-ufs-phy";
+ reg = <0 0x01d87000 0 0x18c>;

The size 0x18c is wrong, in the code you are even accessing registers
whose offsets are beyond 0x18c, see

#define QSERDES_V4_COM_BIN_VCOCAL_CMP_CODE1_MODE0 0x1ac
#define QSERDES_V4_COM_BIN_VCOCAL_CMP_CODE2_MODE0 0x1b0
#define QSERDES_V4_COM_BIN_VCOCAL_CMP_CODE1_MODE1 0x1b4
#define QSERDES_V4_COM_BIN_VCOCAL_HSCLK_SEL 0x1bc
#define QSERDES_V4_COM_BIN_VCOCAL_CMP_CODE2_MODE1 0x1b8

FYI, the total size of serdes registers is 0x1c0.

<--snip-->
+ ufs_mem_phy_lanes: lanes@1d87400 {
+ reg = <0 0x01d87400 0 0x108>,
+ <0 0x01d87600 0 0x1e0>,
+ <0 0x01d87c00 0 0x1dc>,

Same as above, see

#define QPHY_V4_MULTI_LANE_CTRL1 0x1e0

FYI, the total size of PCS registers is 0x200

+ <0 0x01d87800 0 0x108>,
+ <0 0x01d87a00 0 0x1e0>;
+ #phy-cells = <0>;
+ };
<--snip-->