Re: [PATCH v2 6/7] phy: qualcomm: phy-qcom-qmp-ufs: Add High Speed Gear 5 support for SM8550

[Can Guo <quic_cang@quicinc.com>]

Hi Dmitry,

On 11/7/2023 9:18 PM, Dmitry Baryshkov wrote:
> On Tue, 7 Nov 2023 at 06:47, Can Guo <cang@qti.qualcomm.com> wrote:
>>
>> From: Can Guo <quic_cang@quicinc.com>
>>
>> On SM8550, two sets of UFS PHY settings are provided, one set is to support
>> HS-G5, another set is to support HS-G4 and lower gears. The two sets of PHY
>> settings are programming different values to different registers, mixing
>> the two sets and/or overwriting one set with another set is definitely not
>> blessed by UFS PHY designers. In order to add HS-G5 support for SM8550, we
>> need to split the two sets into their dedicated tables, and leave only the
>> common settings in the .tlbs. To have the PHY programmed with the correct
>> set of PHY settings, the submode passed to PHY driver must be either HS-G4
>> or HS-G5.
>>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-pcs-ufs-v6.h     |   2 +
>>   drivers/phy/qualcomm/phy-qcom-qmp-qserdes-com-v6.h |   2 +
>>   .../qualcomm/phy-qcom-qmp-qserdes-txrx-ufs-v6.h    |  12 +++
>>   drivers/phy/qualcomm/phy-qcom-qmp-ufs.c            | 112 ++++++++++++++++++---
>>   4 files changed, 115 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcs-ufs-v6.h b/drivers/phy/qualcomm/phy-qcom-qmp-pcs-ufs-v6.h
>> index c23d5e4..e563af5 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcs-ufs-v6.h
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcs-ufs-v6.h
>> @@ -18,6 +18,7 @@
>>   #define QPHY_V6_PCS_UFS_BIST_FIXED_PAT_CTRL            0x060
>>   #define QPHY_V6_PCS_UFS_TX_HSGEAR_CAPABILITY           0x074
>>   #define QPHY_V6_PCS_UFS_RX_HSGEAR_CAPABILITY           0x0bc
>> +#define QPHY_V6_PCS_UFS_RX_HS_G5_SYNC_LENGTH_CAPABILITY        0x12c
>>   #define QPHY_V6_PCS_UFS_DEBUG_BUS_CLKSEL               0x158
>>   #define QPHY_V6_PCS_UFS_LINECFG_DISABLE                        0x17c
>>   #define QPHY_V6_PCS_UFS_RX_MIN_HIBERN8_TIME            0x184
>> @@ -27,5 +28,6 @@
>>   #define QPHY_V6_PCS_UFS_READY_STATUS                   0x1a8
>>   #define QPHY_V6_PCS_UFS_TX_MID_TERM_CTRL1              0x1f4
>>   #define QPHY_V6_PCS_UFS_MULTI_LANE_CTRL1               0x1fc
>> +#define QPHY_V6_PCS_UFS_RX_HSG5_SYNC_WAIT_TIME         0x220
>>
>>   #endif
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-qserdes-com-v6.h b/drivers/phy/qualcomm/phy-qcom-qmp-qserdes-com-v6.h
>> index f420f8f..ef392ce 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-qserdes-com-v6.h
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-qserdes-com-v6.h
>> @@ -56,6 +56,8 @@
>>   #define QSERDES_V6_COM_SYS_CLK_CTRL                            0xe4
>>   #define QSERDES_V6_COM_SYSCLK_BUF_ENABLE                       0xe8
>>   #define QSERDES_V6_COM_PLL_IVCO                                        0xf4
>> +#define QSERDES_V6_COM_CMN_IETRIM                              0xfc
>> +#define QSERDES_V6_COM_CMN_IPTRIM                              0x100
>>   #define QSERDES_V6_COM_SYSCLK_EN_SEL                           0x110
>>   #define QSERDES_V6_COM_RESETSM_CNTRL                           0x118
>>   #define QSERDES_V6_COM_LOCK_CMP_EN                             0x120
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-qserdes-txrx-ufs-v6.h b/drivers/phy/qualcomm/phy-qcom-qmp-qserdes-txrx-ufs-v6.h
>> index 15bcb4b..48f31c8 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-qserdes-txrx-ufs-v6.h
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-qserdes-txrx-ufs-v6.h
>> @@ -10,10 +10,20 @@
>>   #define QSERDES_UFS_V6_TX_RES_CODE_LANE_RX                     0x2c
>>   #define QSERDES_UFS_V6_TX_RES_CODE_LANE_OFFSET_TX              0x30
>>   #define QSERDES_UFS_V6_TX_RES_CODE_LANE_OFFSET_RX              0x34
>> +#define QSERDES_UFS_V6_TX_LANE_MODE_1                          0x7c
>> +#define QSERDES_UFS_V6_TX_FR_DCC_CTRL                          0x108
>>
>>   #define QSERDES_UFS_V6_RX_UCDR_FASTLOCK_FO_GAIN_RATE2          0x08
>>   #define QSERDES_UFS_V6_RX_UCDR_FASTLOCK_FO_GAIN_RATE4          0x10
>> +#define QSERDES_UFS_V6_RX_UCDR_FASTLOCK_SO_GAIN_RATE4          0x24
>> +#define QSERDES_UFS_V6_RX_UCDR_FASTLOCK_COUNT_HIGH_RATE4       0x54
>> +#define QSERDES_UFS_V6_RX_UCDR_FO_GAIN_RATE2                   0xd4
>> +#define QSERDES_UFS_V6_RX_UCDR_FO_GAIN_RATE4                   0xdc
>> +#define QSERDES_UFS_V6_RX_UCDR_SO_GAIN_RATE4                   0xf0
>> +#define QSERDES_UFS_V6_RX_UCDR_PI_CONTROLS                     0xf4
>>   #define QSERDES_UFS_V6_RX_VGA_CAL_MAN_VAL                      0x178
>> +#define QSERDES_UFS_V6_RX_EQ_OFFSET_ADAPTOR_CNTRL1             0x1bc
>> +#define QSERDES_UFS_V6_RX_OFFSET_ADAPTOR_CNTRL3                        0x1c4
>>   #define QSERDES_UFS_V6_RX_MODE_RATE_0_1_B0                     0x208
>>   #define QSERDES_UFS_V6_RX_MODE_RATE_0_1_B1                     0x20c
>>   #define QSERDES_UFS_V6_RX_MODE_RATE_0_1_B3                     0x214
>> @@ -25,6 +35,8 @@
>>   #define QSERDES_UFS_V6_RX_MODE_RATE3_B5                                0x264
>>   #define QSERDES_UFS_V6_RX_MODE_RATE3_B8                                0x270
>>   #define QSERDES_UFS_V6_RX_MODE_RATE4_B3                                0x280
>> +#define QSERDES_UFS_V6_RX_MODE_RATE4_B4                                0x284
>>   #define QSERDES_UFS_V6_RX_MODE_RATE4_B6                                0x28c
>> +#define QSERDES_UFS_V6_RX_DLL0_FTUNE_CTRL                      0x2f8
>>
>>   #endif
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> index 3927eba..e0a01497 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> @@ -649,32 +649,51 @@ static const struct qmp_phy_init_tbl sm8550_ufsphy_serdes[] = {
>>          QMP_PHY_INIT_CFG(QSERDES_V6_COM_HSCLK_SEL_1, 0x11),
>>          QMP_PHY_INIT_CFG(QSERDES_V6_COM_HSCLK_HS_SWITCH_SEL_1, 0x00),
>>          QMP_PHY_INIT_CFG(QSERDES_V6_COM_LOCK_CMP_EN, 0x01),
>> -       QMP_PHY_INIT_CFG(QSERDES_V6_COM_VCO_TUNE_MAP, 0x04),
>> -       QMP_PHY_INIT_CFG(QSERDES_V6_COM_PLL_IVCO, 0x0f),
>> +
>>          QMP_PHY_INIT_CFG(QSERDES_V6_COM_VCO_TUNE_INITVAL2, 0x00),
>>          QMP_PHY_INIT_CFG(QSERDES_V6_COM_DEC_START_MODE0, 0x41),
>> -       QMP_PHY_INIT_CFG(QSERDES_V6_COM_CP_CTRL_MODE0, 0x0a),
>>          QMP_PHY_INIT_CFG(QSERDES_V6_COM_PLL_RCTRL_MODE0, 0x18),
>>          QMP_PHY_INIT_CFG(QSERDES_V6_COM_PLL_CCTRL_MODE0, 0x14),
>>          QMP_PHY_INIT_CFG(QSERDES_V6_COM_LOCK_CMP1_MODE0, 0x7f),
>>          QMP_PHY_INIT_CFG(QSERDES_V6_COM_LOCK_CMP2_MODE0, 0x06),
>> -       QMP_PHY_INIT_CFG(QSERDES_V6_COM_DEC_START_MODE0, 0x4c),
>> +};
>> +
>> +static const struct qmp_phy_init_tbl sm8550_ufsphy_hs_b_serdes[] = {
>> +       QMP_PHY_INIT_CFG(QSERDES_V6_COM_VCO_TUNE_MAP, 0x44),
>> +};
>> +
>> +static const struct qmp_phy_init_tbl sm8550_ufsphy_g4_serdes[] = {
>> +       QMP_PHY_INIT_CFG(QSERDES_V6_COM_VCO_TUNE_MAP, 0x04),
>> +       QMP_PHY_INIT_CFG(QSERDES_V6_COM_PLL_IVCO, 0x0f),
>>          QMP_PHY_INIT_CFG(QSERDES_V6_COM_CP_CTRL_MODE0, 0x0a),
>> -       QMP_PHY_INIT_CFG(QSERDES_V6_COM_PLL_RCTRL_MODE0, 0x18),
>> -       QMP_PHY_INIT_CFG(QSERDES_V6_COM_PLL_CCTRL_MODE0, 0x14),
>> -       QMP_PHY_INIT_CFG(QSERDES_V6_COM_LOCK_CMP1_MODE0, 0x99),
>> -       QMP_PHY_INIT_CFG(QSERDES_V6_COM_LOCK_CMP2_MODE0, 0x07),
> 
> Aside from moving these registers to the HS_G4 table, you are also
> changing these registers. It makes me think that there was an error in
> the original programming sequence.
> If that is correct, could you please split the patch into two pieces:
> - Fix programming sequence (add proper Fixes tags)
> - Split G4 and G5 tables.

You are correct, I don't know where the original values are from, but 
they look like a mixing of HS-G4 settings and HS-G5 settings to me, 
because I see some values are only supposed to be there for HS-G5, 
whereas the same MODE0 registers are programmed twice, I guess they 
should be MODE1 registers which are only required for HS-G4.

> 
>> +
>> +       QMP_PHY_INIT_CFG(QSERDES_V6_COM_DEC_START_MODE1, 0x4c),
>> +       QMP_PHY_INIT_CFG(QSERDES_V6_COM_CP_CTRL_MODE1, 0x0a),
>> +       QMP_PHY_INIT_CFG(QSERDES_V6_COM_PLL_RCTRL_MODE1, 0x18),
>> +       QMP_PHY_INIT_CFG(QSERDES_V6_COM_PLL_CCTRL_MODE1, 0x14),
>> +       QMP_PHY_INIT_CFG(QSERDES_V6_COM_LOCK_CMP1_MODE1, 0x99),
>> +       QMP_PHY_INIT_CFG(QSERDES_V6_COM_LOCK_CMP2_MODE1, 0x07),
> 
> I see all the MODE1 registers being only present in G4 and G5 tables.
> Should they be programmed for the modes lower than G4?
> 

As mentioned in the commit msg, on SM8550, only two sets of UFS PHY 
settings are provided, one set is to support HS-G5, another set is to 
support HS-G4.

Although HS-G5 set of PHY settings is backward comptiable and able to 
support all lower gears, UFS driver should make sure HS-G5 set of PHY 
settings are only used when UFS device really supports HS-G5.

For UFS devices which can only support HS-G4 or lower gears, UFS driver 
would use the HS-G4 set of PHY settings, in order to save power.

Thanks,
Can Guo.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy